Date: Thu, 28 May 2020 23:23:49 +0000 (UTC) From: Warner Losh <imp@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r361608 - in stable/12/sys/dev: mpr mps Message-ID: <202005282323.04SNNnFX043554@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: imp Date: Thu May 28 23:23:49 2020 New Revision: 361608 URL: https://svnweb.freebsd.org/changeset/base/361608 Log: MFC r358308: Before issing the REMOVE_DEVICE command to the firmware, make sure that all commands have completed. It's not OK to force complete any pending commands before we send the REMOVE_DEVICE. Instead, make sure that all pending commands are complete before sending that. By trying to second guess the firmware here, we run the risk of completing commands twice, which leads to corruption. This removes the forced completion of commands introduced in r218811. So it's a partial backout of that commit, but replaces it with a more rebust mechanism. Either these commands will complete due to the TARGET RESET, or they will timeout and be aborted, but they will all complete. Add assert that all commands are complete to REMOVE_DEVICE completion routine. We attempt to assure this programatically, so we shouldn't have any commands in the queue because we've waited for them all. Any commands that make it into our action routine after we mark the target in removal will complete immediately with an error. When we're removing a target that's not a volume, advertise up the stack that it's actually gone, as opposed to having a transient selection error we should retry. Do this both in the action routine, and when we get a notification of an aborted command. We don't do this for volumes because the driver tries hard not to advertise to the OS a volume has disappeared. Apply these changes to both mpr and mps since they are based on quite similar designs. Discussed with: scottl@ Differential Revision: https://reviews.freebsd.org/D23768 Modified: stable/12/sys/dev/mpr/mpr_sas.c stable/12/sys/dev/mpr/mpr_sas.h stable/12/sys/dev/mps/mps_sas.c stable/12/sys/dev/mps/mps_sas.h Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/dev/mpr/mpr_sas.c ============================================================================== --- stable/12/sys/dev/mpr/mpr_sas.c Thu May 28 22:05:50 2020 (r361607) +++ stable/12/sys/dev/mpr/mpr_sas.c Thu May 28 23:23:49 2020 (r361608) @@ -559,7 +559,6 @@ mprsas_remove_device(struct mpr_softc *sc, struct mpr_ MPI2_SCSI_TASK_MANAGE_REPLY *reply; MPI2_SAS_IOUNIT_CONTROL_REQUEST *req; struct mprsas_target *targ; - struct mpr_command *next_cm; uint16_t handle; MPR_FUNCTRACE(sc); @@ -609,7 +608,18 @@ mprsas_remove_device(struct mpr_softc *sc, struct mpr_ tm->cm_complete = mprsas_remove_complete; tm->cm_complete_data = (void *)(uintptr_t)handle; - mpr_map_command(sc, tm); + /* + * Wait to send the REMOVE_DEVICE until all the commands have cleared. + * They should be aborted or time out and we'll kick thus off there + * if so. + */ + if (TAILQ_FIRST(&targ->commands) == NULL) { + mpr_dprint(sc, MPR_INFO, "No pending commands: starting remove_device\n"); + mpr_map_command(sc, tm); + targ->pending_remove_tm = NULL; + } else { + targ->pending_remove_tm = tm; + } mpr_dprint(sc, MPR_INFO, "clearing target %u handle 0x%04x\n", targ->tid, handle); @@ -618,14 +628,6 @@ mprsas_remove_device(struct mpr_softc *sc, struct mpr_ "connector name (%4s)\n", targ->encl_level, targ->encl_slot, targ->connector_name); } - TAILQ_FOREACH_SAFE(tm, &targ->commands, cm_link, next_cm) { - union ccb *ccb; - - mpr_dprint(sc, MPR_XINFO, "Completing missed command %p\n", tm); - ccb = tm->cm_complete_data; - mprsas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE); - mprsas_scsiio_complete(sc, tm); - } } static void @@ -641,7 +643,16 @@ mprsas_remove_complete(struct mpr_softc *sc, struct mp reply = (MPI2_SAS_IOUNIT_CONTROL_REPLY *)tm->cm_reply; handle = (uint16_t)(uintptr_t)tm->cm_complete_data; + targ = tm->cm_targ; + /* + * At this point, we should have no pending commands for the target. + * The remove target has just completed. + */ + KASSERT(TAILQ_FIRST(&targ->commands) == NULL, + ("%s: no commands should be pending\n", __func__)); + + /* * Currently there should be no way we can hit this case. It only * happens when we have a failure to allocate chain frames, and * task management commands don't have S/G lists. @@ -673,7 +684,6 @@ mprsas_remove_complete(struct mpr_softc *sc, struct mp */ if ((le16toh(reply->IOCStatus) & MPI2_IOCSTATUS_MASK) == MPI2_IOCSTATUS_SUCCESS) { - targ = tm->cm_targ; targ->handle = 0x0; targ->encl_handle = 0x0; targ->encl_level_valid = 0x0; @@ -1963,13 +1973,17 @@ mprsas_action_scsiio(struct mprsas_softc *sassc, union /* * If devinfo is 0 this will be a volume. In that case don't tell CAM * that the volume has timed out. We want volumes to be enumerated - * until they are deleted/removed, not just failed. + * until they are deleted/removed, not just failed. In either event, + * we're removing the target due to a firmware event telling us + * the device is now gone (as opposed to some transient event). Since + * we're opting to remove failed devices from the OS's view, we need + * to propagate that status up the stack. */ if (targ->flags & MPRSAS_TARGET_INREMOVAL) { if (targ->devinfo == 0) mprsas_set_ccbstatus(ccb, CAM_REQ_CMP); else - mprsas_set_ccbstatus(ccb, CAM_SEL_TIMEOUT); + mprsas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE); xpt_done(ccb); return; } @@ -2843,15 +2857,22 @@ mprsas_scsiio_complete(struct mpr_softc *sc, struct mp * potential risk of flagging false failures in the event * of a topology-related error (e.g. a SAS expander problem * causes a command addressed to a drive to fail), but - * avoiding getting into an infinite retry loop. + * avoiding getting into an infinite retry loop. However, + * if we get them while were moving a device, we should + * fail the request as 'not there' because the device + * is effectively gone. */ - mprsas_set_ccbstatus(ccb, CAM_REQ_CMP_ERR); + if (cm->cm_targ->flags & MPRSAS_TARGET_INREMOVAL) + mprsas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE); + else + mprsas_set_ccbstatus(ccb, CAM_REQ_CMP_ERR); mpr_dprint(sc, MPR_INFO, - "Controller reported %s tgt %u SMID %u loginfo %x\n", + "Controller reported %s tgt %u SMID %u loginfo %x%s\n", mpr_describe_table(mpr_iocstatus_string, le16toh(rep->IOCStatus) & MPI2_IOCSTATUS_MASK), target_id, cm->cm_desc.Default.SMID, - le32toh(rep->IOCLogInfo)); + le32toh(rep->IOCLogInfo), + (cm->cm_targ->flags & MPRSAS_TARGET_INREMOVAL) ? " departing" : ""); mpr_dprint(sc, MPR_XINFO, "SCSIStatus %x SCSIState %x xfercount %u\n", rep->SCSIStatus, rep->SCSIState, @@ -2897,6 +2918,21 @@ mprsas_scsiio_complete(struct mpr_softc *sc, struct mp if (mprsas_get_ccbstatus(ccb) != CAM_REQ_CMP) { ccb->ccb_h.status |= CAM_DEV_QFRZN; xpt_freeze_devq(ccb->ccb_h.path, /*count*/ 1); + } + + /* + * Check to see if we're removing the device. If so, and this is the + * last command on the queue, proceed with the deferred removal of the + * device. Note, for removing a volume, this won't trigger because + * pending_remove_tm will be NULL. + */ + if (cm->cm_targ->flags & MPRSAS_TARGET_INREMOVAL) { + if (TAILQ_FIRST(&cm->cm_targ->commands) == NULL && + cm->cm_targ->pending_remove_tm != NULL) { + mpr_dprint(sc, MPR_INFO, "Last pending command complete: starting remove_device\n"); + mpr_map_command(sc, cm->cm_targ->pending_remove_tm); + cm->cm_targ->pending_remove_tm = NULL; + } } mpr_free_command(sc, cm); Modified: stable/12/sys/dev/mpr/mpr_sas.h ============================================================================== --- stable/12/sys/dev/mpr/mpr_sas.h Thu May 28 22:05:50 2020 (r361607) +++ stable/12/sys/dev/mpr/mpr_sas.h Thu May 28 23:23:49 2020 (r361608) @@ -64,6 +64,7 @@ struct mprsas_target { SLIST_HEAD(, mprsas_lun) luns; TAILQ_HEAD(, mpr_command) commands; struct mpr_command *tm; + struct mpr_command *pending_remove_tm; TAILQ_HEAD(, mpr_command) timedout_commands; uint16_t exp_dev_handle; uint16_t phy_num; Modified: stable/12/sys/dev/mps/mps_sas.c ============================================================================== --- stable/12/sys/dev/mps/mps_sas.c Thu May 28 22:05:50 2020 (r361607) +++ stable/12/sys/dev/mps/mps_sas.c Thu May 28 23:23:49 2020 (r361608) @@ -556,7 +556,6 @@ mpssas_remove_device(struct mps_softc *sc, struct mps_ MPI2_SCSI_TASK_MANAGE_REPLY *reply; MPI2_SAS_IOUNIT_CONTROL_REQUEST *req; struct mpssas_target *targ; - struct mps_command *next_cm; uint16_t handle; MPS_FUNCTRACE(sc); @@ -609,18 +608,22 @@ mpssas_remove_device(struct mps_softc *sc, struct mps_ tm->cm_complete = mpssas_remove_complete; tm->cm_complete_data = (void *)(uintptr_t)handle; - mps_map_command(sc, tm); + /* + * Wait to send the REMOVE_DEVICE until all the commands have cleared. + * They should be aborted or time out and we'll kick thus off there + * if so. + */ + if (TAILQ_FIRST(&targ->commands) == NULL) { + mps_dprint(sc, MPS_INFO, "No pending commands: starting remove_device\n"); + mps_map_command(sc, tm); + targ->pending_remove_tm = NULL; + } else { + targ->pending_remove_tm = tm; + } + mps_dprint(sc, MPS_XINFO, "clearing target %u handle 0x%04x\n", targ->tid, handle); - TAILQ_FOREACH_SAFE(tm, &targ->commands, cm_link, next_cm) { - union ccb *ccb; - - mps_dprint(sc, MPS_XINFO, "Completing missed command %p\n", tm); - ccb = tm->cm_complete_data; - mpssas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE); - mpssas_scsiio_complete(sc, tm); - } } static void @@ -635,8 +638,17 @@ mpssas_remove_complete(struct mps_softc *sc, struct mp reply = (MPI2_SAS_IOUNIT_CONTROL_REPLY *)tm->cm_reply; handle = (uint16_t)(uintptr_t)tm->cm_complete_data; + targ = tm->cm_targ; /* + * At this point, we should have no pending commands for the target. + * The remove target has just completed. + */ + KASSERT(TAILQ_FIRST(&targ->commands) == NULL, + ("%s: no commands should be pending\n", __func__)); + + + /* * Currently there should be no way we can hit this case. It only * happens when we have a failure to allocate chain frames, and * task management commands don't have S/G lists. @@ -670,7 +682,6 @@ mpssas_remove_complete(struct mps_softc *sc, struct mp */ if ((le16toh(reply->IOCStatus) & MPI2_IOCSTATUS_MASK) == MPI2_IOCSTATUS_SUCCESS) { - targ = tm->cm_targ; targ->handle = 0x0; targ->encl_handle = 0x0; targ->encl_slot = 0x0; @@ -1712,13 +1723,17 @@ mpssas_action_scsiio(struct mpssas_softc *sassc, union /* * If devinfo is 0 this will be a volume. In that case don't tell CAM * that the volume has timed out. We want volumes to be enumerated - * until they are deleted/removed, not just failed. + * until they are deleted/removed, not just failed. In either event, + * we're removing the target due to a firmware event telling us + * the device is now gone (as opposed to some transient event). Since + * we're opting to remove failed devices from the OS's view, we need + * to propagate that status up the stack. */ if (targ->flags & MPSSAS_TARGET_INREMOVAL) { if (targ->devinfo == 0) mpssas_set_ccbstatus(ccb, CAM_REQ_CMP); else - mpssas_set_ccbstatus(ccb, CAM_SEL_TIMEOUT); + mpssas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE); xpt_done(ccb); return; } @@ -2351,15 +2366,22 @@ mpssas_scsiio_complete(struct mps_softc *sc, struct mp * potential risk of flagging false failures in the event * of a topology-related error (e.g. a SAS expander problem * causes a command addressed to a drive to fail), but - * avoiding getting into an infinite retry loop. + * avoiding getting into an infinite retry loop. However, + * if we get them while were moving a device, we should + * fail the request as 'not there' because the device + * is effectively gone. */ - mpssas_set_ccbstatus(ccb, CAM_REQ_CMP_ERR); + if (cm->cm_targ->flags & MPSSAS_TARGET_INREMOVAL) + mpssas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE); + else + mpssas_set_ccbstatus(ccb, CAM_REQ_CMP_ERR); mps_dprint(sc, MPS_INFO, - "Controller reported %s tgt %u SMID %u loginfo %x\n", + "Controller reported %s tgt %u SMID %u loginfo %x%s\n", mps_describe_table(mps_iocstatus_string, le16toh(rep->IOCStatus) & MPI2_IOCSTATUS_MASK), target_id, cm->cm_desc.Default.SMID, - le32toh(rep->IOCLogInfo)); + le32toh(rep->IOCLogInfo), + (cm->cm_targ->flags & MPSSAS_TARGET_INREMOVAL) ? " departing" : ""); mps_dprint(sc, MPS_XINFO, "SCSIStatus %x SCSIState %x xfercount %u\n", rep->SCSIStatus, rep->SCSIState, @@ -2398,6 +2420,21 @@ mpssas_scsiio_complete(struct mps_softc *sc, struct mp if (mpssas_get_ccbstatus(ccb) != CAM_REQ_CMP) { ccb->ccb_h.status |= CAM_DEV_QFRZN; xpt_freeze_devq(ccb->ccb_h.path, /*count*/ 1); + } + + /* + * Check to see if we're removing the device. If so, and this is the + * last command on the queue, proceed with the deferred removal of the + * device. Note, for removing a volume, this won't trigger because + * pending_remove_tm will be NULL. + */ + if (cm->cm_targ->flags & MPSSAS_TARGET_INREMOVAL) { + if (TAILQ_FIRST(&cm->cm_targ->commands) == NULL && + cm->cm_targ->pending_remove_tm != NULL) { + mps_dprint(sc, MPS_INFO, "Last pending command complete: starting remove_device\n"); + mps_map_command(sc, cm->cm_targ->pending_remove_tm); + cm->cm_targ->pending_remove_tm = NULL; + } } mps_free_command(sc, cm); Modified: stable/12/sys/dev/mps/mps_sas.h ============================================================================== --- stable/12/sys/dev/mps/mps_sas.h Thu May 28 22:05:50 2020 (r361607) +++ stable/12/sys/dev/mps/mps_sas.h Thu May 28 23:23:49 2020 (r361608) @@ -62,6 +62,7 @@ struct mpssas_target { SLIST_HEAD(, mpssas_lun) luns; TAILQ_HEAD(, mps_command) commands; struct mps_command *tm; + struct mps_command *pending_remove_tm; TAILQ_HEAD(, mps_command) timedout_commands; uint16_t exp_dev_handle; uint16_t phy_num;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202005282323.04SNNnFX043554>