Author: cem Date: Fri Dec 21 20:30:52 2018 New Revision: 342355 URL: https://svnweb.freebsd.org/changeset/base/342355
Log: mps(4), mpr(4): remove SATA ID command cancellation hack Add a generic mechanism to override mp?_wait_command's timeout behavior, which continues to invoke reinit by default. Invokers who set cm_timeout_handler may avoid automatic reinit and do their own handling. Adapt mp?sas_get_sata_identify to this mechanism and remove its callout hack. Reviewed by: scottl Sponsored by: Dell EMC Isilon Differential Revision: https://reviews.freebsd.org/D18614 Modified: head/sys/dev/mpr/mpr.c head/sys/dev/mpr/mpr_sas_lsi.c head/sys/dev/mpr/mprvar.h head/sys/dev/mps/mps.c head/sys/dev/mps/mps_sas_lsi.c head/sys/dev/mps/mpsvar.h Modified: head/sys/dev/mpr/mpr.c ============================================================================== --- head/sys/dev/mpr/mpr.c Fri Dec 21 20:29:16 2018 (r342354) +++ head/sys/dev/mpr/mpr.c Fri Dec 21 20:30:52 2018 (r342355) @@ -3815,12 +3815,15 @@ mpr_wait_command(struct mpr_softc *sc, struct mpr_comm } if (error == EWOULDBLOCK) { - mpr_dprint(sc, MPR_FAULT, "Calling Reinit from %s, timeout=%d," - " elapsed=%jd\n", __func__, timeout, - (intmax_t)cur_time.tv_sec); - rc = mpr_reinit(sc); - mpr_dprint(sc, MPR_FAULT, "Reinit %s\n", (rc == 0) ? "success" : - "failed"); + if (cm->cm_timeout_handler == NULL) { + mpr_dprint(sc, MPR_FAULT, "Calling Reinit from %s, timeout=%d," + " elapsed=%jd\n", __func__, timeout, + (intmax_t)cur_time.tv_sec); + rc = mpr_reinit(sc); + mpr_dprint(sc, MPR_FAULT, "Reinit %s\n", (rc == 0) ? "success" : + "failed"); + } else + cm->cm_timeout_handler(sc, cm); if (sc->mpr_flags & MPR_FLAGS_REALLOCATED) { /* * Tell the caller that we freed the command in a Modified: head/sys/dev/mpr/mpr_sas_lsi.c ============================================================================== --- head/sys/dev/mpr/mpr_sas_lsi.c Fri Dec 21 20:29:16 2018 (r342354) +++ head/sys/dev/mpr/mpr_sas_lsi.c Fri Dec 21 20:30:52 2018 (r342355) @@ -124,7 +124,7 @@ static int mprsas_add_pcie_device(struct mpr_softc *sc static int mprsas_get_sata_identify(struct mpr_softc *sc, u16 handle, Mpi2SataPassthroughReply_t *mpi_reply, char *id_buffer, int sz, u32 devinfo); -static void mprsas_ata_id_timeout(void *data); +static void mprsas_ata_id_timeout(struct mpr_softc *, struct mpr_command *); int mprsas_get_sas_address_for_sata_disk(struct mpr_softc *sc, u64 *sas_address, u16 handle, u32 device_info, u8 *is_SATA_SSD); static int mprsas_volume_add(struct mpr_softc *sc, @@ -1167,23 +1167,19 @@ mprsas_get_sata_identify(struct mpr_softc *sc, u16 han cm->cm_length = htole32(sz); /* - * Start a timeout counter specifically for the SATA ID command. This - * is used to fix a problem where the FW does not send a reply sometimes + * Use a custom handler to avoid reinit'ing the controller on timeout. + * This fixes a problem where the FW does not send a reply sometimes * when a bad disk is in the topology. So, this is used to timeout the * command so that processing can continue normally. */ - mpr_dprint(sc, MPR_XINFO, "%s start timeout counter for SATA ID " - "command\n", __func__); - callout_reset(&cm->cm_callout, MPR_ATA_ID_TIMEOUT * hz, - mprsas_ata_id_timeout, cm); - error = mpr_wait_command(sc, &cm, 60, CAN_SLEEP); - mpr_dprint(sc, MPR_XINFO, "%s stop timeout counter for SATA ID " - "command\n", __func__); - /* XXX KDM need to fix the case where this command is destroyed */ - callout_stop(&cm->cm_callout); + cm->cm_timeout_handler = mprsas_ata_id_timeout; - if (cm != NULL) - reply = (Mpi2SataPassthroughReply_t *)cm->cm_reply; + error = mpr_wait_command(sc, &cm, MPR_ATA_ID_TIMEOUT, CAN_SLEEP); + + /* mprsas_ata_id_timeout does not reset controller */ + KASSERT(cm != NULL, ("%s: surprise command freed", __func__)); + + reply = (Mpi2SataPassthroughReply_t *)cm->cm_reply; if (error || (reply == NULL)) { /* FIXME */ /* @@ -1210,61 +1206,28 @@ out: /* * If the SATA_ID_TIMEOUT flag has been set for this command, don't free * it. The command and buffer will be freed after sending an Abort - * Task TM. If the command did timeout, use EWOULDBLOCK. + * Task TM. */ if ((cm->cm_flags & MPR_CM_FLAGS_SATA_ID_TIMEOUT) == 0) { mpr_free_command(sc, cm); free(buffer, M_MPR); - } else if (error == 0) - error = EWOULDBLOCK; + } return (error); } static void -mprsas_ata_id_timeout(void *data) +mprsas_ata_id_timeout(struct mpr_softc *sc, struct mpr_command *cm) { - struct mpr_softc *sc; - struct mpr_command *cm; - cm = (struct mpr_command *)data; - sc = cm->cm_sc; - mtx_assert(&sc->mpr_mtx, MA_OWNED); - - mpr_dprint(sc, MPR_INFO, "%s checking ATA ID command %p sc %p\n", + mpr_dprint(sc, MPR_INFO, "%s ATA ID command timeout cm %p sc %p\n", __func__, cm, sc); - if ((callout_pending(&cm->cm_callout)) || - (!callout_active(&cm->cm_callout))) { - mpr_dprint(sc, MPR_INFO, "%s ATA ID command almost timed out\n", - __func__); - return; - } - callout_deactivate(&cm->cm_callout); /* - * Run the interrupt handler to make sure it's not pending. This - * isn't perfect because the command could have already completed - * and been re-used, though this is unlikely. + * The Abort Task cannot be sent from here because the driver has not + * completed setting up targets. Instead, the command is flagged so + * that special handling will be used to send the abort. */ - mpr_intr_locked(sc); - if (cm->cm_state == MPR_CM_STATE_FREE) { - mpr_dprint(sc, MPR_INFO, "%s ATA ID command almost timed out\n", - __func__); - return; - } - - mpr_dprint(sc, MPR_INFO, "ATA ID command timeout cm %p\n", cm); - - /* - * Send wakeup() to the sleeping thread that issued this ATA ID command. - * wakeup() will cause msleep to return a 0 (not EWOULDBLOCK), and this - * will keep reinit() from being called. This way, an Abort Task TM can - * be issued so that the timed out command can be cleared. The Abort - * Task cannot be sent from here because the driver has not completed - * setting up targets. Instead, the command is flagged so that special - * handling will be used to send the abort. - */ cm->cm_flags |= MPR_CM_FLAGS_SATA_ID_TIMEOUT; - wakeup(cm); } static int Modified: head/sys/dev/mpr/mprvar.h ============================================================================== --- head/sys/dev/mpr/mprvar.h Fri Dec 21 20:29:16 2018 (r342354) +++ head/sys/dev/mpr/mprvar.h Fri Dec 21 20:30:52 2018 (r342355) @@ -252,6 +252,7 @@ struct mpr_command { uint32_t cm_req_busaddr; bus_addr_t cm_sense_busaddr; struct callout cm_callout; + mpr_command_callback_t *cm_timeout_handler; }; struct mpr_column_map { @@ -613,6 +614,7 @@ mpr_alloc_command(struct mpr_softc *sc) TAILQ_REMOVE(&sc->req_list, cm, cm_link); cm->cm_state = MPR_CM_STATE_BUSY; + cm->cm_timeout_handler = NULL; return (cm); } @@ -654,6 +656,7 @@ mpr_alloc_high_priority_command(struct mpr_softc *sc) TAILQ_REMOVE(&sc->high_priority_req_list, cm, cm_link); cm->cm_state = MPR_CM_STATE_BUSY; + cm->cm_timeout_handler = NULL; return (cm); } Modified: head/sys/dev/mps/mps.c ============================================================================== --- head/sys/dev/mps/mps.c Fri Dec 21 20:29:16 2018 (r342354) +++ head/sys/dev/mps/mps.c Fri Dec 21 20:30:52 2018 (r342355) @@ -3103,12 +3103,15 @@ mps_wait_command(struct mps_softc *sc, struct mps_comm } if (error == EWOULDBLOCK) { - mps_dprint(sc, MPS_FAULT, "Calling Reinit from %s, timeout=%d," - " elapsed=%jd\n", __func__, timeout, - (intmax_t)cur_time.tv_sec); - rc = mps_reinit(sc); - mps_dprint(sc, MPS_FAULT, "Reinit %s\n", (rc == 0) ? "success" : - "failed"); + if (cm->cm_timeout_handler == NULL) { + mps_dprint(sc, MPS_FAULT, "Calling Reinit from %s, timeout=%d," + " elapsed=%jd\n", __func__, timeout, + (intmax_t)cur_time.tv_sec); + rc = mps_reinit(sc); + mps_dprint(sc, MPS_FAULT, "Reinit %s\n", (rc == 0) ? "success" : + "failed"); + } else + cm->cm_timeout_handler(sc, cm); if (sc->mps_flags & MPS_FLAGS_REALLOCATED) { /* * Tell the caller that we freed the command in a Modified: head/sys/dev/mps/mps_sas_lsi.c ============================================================================== --- head/sys/dev/mps/mps_sas_lsi.c Fri Dec 21 20:29:16 2018 (r342354) +++ head/sys/dev/mps/mps_sas_lsi.c Fri Dec 21 20:30:52 2018 (r342355) @@ -123,7 +123,7 @@ static int mpssas_add_device(struct mps_softc *sc, u16 static int mpssas_get_sata_identify(struct mps_softc *sc, u16 handle, Mpi2SataPassthroughReply_t *mpi_reply, char *id_buffer, int sz, u32 devinfo); -static void mpssas_ata_id_timeout(void *data); +static void mpssas_ata_id_timeout(struct mps_softc *, struct mps_command *); int mpssas_get_sas_address_for_sata_disk(struct mps_softc *sc, u64 *sas_address, u16 handle, u32 device_info, u8 *is_SATA_SSD); static int mpssas_volume_add(struct mps_softc *sc, @@ -959,23 +959,19 @@ mpssas_get_sata_identify(struct mps_softc *sc, u16 han cm->cm_length = htole32(sz); /* - * Start a timeout counter specifically for the SATA ID command. This - * is used to fix a problem where the FW does not send a reply sometimes + * Use a custom handler to avoid reinit'ing the controller on timeout. + * This fixes a problem where the FW does not send a reply sometimes * when a bad disk is in the topology. So, this is used to timeout the * command so that processing can continue normally. */ - mps_dprint(sc, MPS_XINFO, "%s start timeout counter for SATA ID " - "command\n", __func__); - callout_reset(&cm->cm_callout, MPS_ATA_ID_TIMEOUT * hz, - mpssas_ata_id_timeout, cm); - error = mps_wait_command(sc, &cm, 60, CAN_SLEEP); - mps_dprint(sc, MPS_XINFO, "%s stop timeout counter for SATA ID " - "command\n", __func__); - /* XXX KDM need to fix the case where this command is destroyed */ - callout_stop(&cm->cm_callout); + cm->cm_timeout_handler = mpssas_ata_id_timeout; - if (cm != NULL) - reply = (Mpi2SataPassthroughReply_t *)cm->cm_reply; + error = mps_wait_command(sc, &cm, MPS_ATA_ID_TIMEOUT, CAN_SLEEP); + + /* mpssas_ata_id_timeout does not reset controller */ + KASSERT(cm != NULL, ("%s: surprise command freed", __func__)); + + reply = (Mpi2SataPassthroughReply_t *)cm->cm_reply; if (error || (reply == NULL)) { /* FIXME */ /* @@ -1002,62 +998,28 @@ out: /* * If the SATA_ID_TIMEOUT flag has been set for this command, don't free * it. The command and buffer will be freed after sending an Abort - * Task TM. If the command did timeout, use EWOULDBLOCK. + * Task TM. */ - if ((cm != NULL) - && (cm->cm_flags & MPS_CM_FLAGS_SATA_ID_TIMEOUT) == 0) { + if ((cm->cm_flags & MPS_CM_FLAGS_SATA_ID_TIMEOUT) == 0) { mps_free_command(sc, cm); free(buffer, M_MPT2); - } else if (error == 0) - error = EWOULDBLOCK; + } return (error); } static void -mpssas_ata_id_timeout(void *data) +mpssas_ata_id_timeout(struct mps_softc *sc, struct mps_command *cm) { - struct mps_softc *sc; - struct mps_command *cm; - cm = (struct mps_command *)data; - sc = cm->cm_sc; - mtx_assert(&sc->mps_mtx, MA_OWNED); - - mps_dprint(sc, MPS_INFO, "%s checking ATA ID command %p sc %p\n", + mps_dprint(sc, MPS_INFO, "%s ATA ID command timeout cm %p sc %p\n", __func__, cm, sc); - if ((callout_pending(&cm->cm_callout)) || - (!callout_active(&cm->cm_callout))) { - mps_dprint(sc, MPS_INFO, "%s ATA ID command almost timed out\n", - __func__); - return; - } - callout_deactivate(&cm->cm_callout); /* - * Run the interrupt handler to make sure it's not pending. This - * isn't perfect because the command could have already completed - * and been re-used, though this is unlikely. + * The Abort Task cannot be sent from here because the driver has not + * completed setting up targets. Instead, the command is flagged so + * that special handling will be used to send the abort. */ - mps_intr_locked(sc); - if (cm->cm_state == MPS_CM_STATE_FREE) { - mps_dprint(sc, MPS_INFO, "%s ATA ID command almost timed out\n", - __func__); - return; - } - - mps_dprint(sc, MPS_INFO, "ATA ID command timeout cm %p\n", cm); - - /* - * Send wakeup() to the sleeping thread that issued this ATA ID command. - * wakeup() will cause msleep to return a 0 (not EWOULDBLOCK), and this - * will keep reinit() from being called. This way, an Abort Task TM can - * be issued so that the timed out command can be cleared. The Abort - * Task cannot be sent from here because the driver has not completed - * setting up targets. Instead, the command is flagged so that special - * handling will be used to send the abort. - */ cm->cm_flags |= MPS_CM_FLAGS_SATA_ID_TIMEOUT; - wakeup(cm); } static int Modified: head/sys/dev/mps/mpsvar.h ============================================================================== --- head/sys/dev/mps/mpsvar.h Fri Dec 21 20:29:16 2018 (r342354) +++ head/sys/dev/mps/mpsvar.h Fri Dec 21 20:30:52 2018 (r342355) @@ -250,6 +250,7 @@ struct mps_command { uint32_t cm_req_busaddr; uint32_t cm_sense_busaddr; struct callout cm_callout; + mps_command_callback_t *cm_timeout_handler; }; struct mps_column_map { @@ -581,6 +582,7 @@ mps_alloc_command(struct mps_softc *sc) TAILQ_REMOVE(&sc->req_list, cm, cm_link); cm->cm_state = MPS_CM_STATE_BUSY; + cm->cm_timeout_handler = NULL; return (cm); } @@ -622,6 +624,7 @@ mps_alloc_high_priority_command(struct mps_softc *sc) TAILQ_REMOVE(&sc->high_priority_req_list, cm, cm_link); cm->cm_state = MPS_CM_STATE_BUSY; + cm->cm_timeout_handler = NULL; return (cm); } _______________________________________________ [email protected] mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "[email protected]"
