Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 21 Dec 2018 20:30:53 +0000 (UTC)
From:      Conrad Meyer <cem@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r342355 - in head/sys/dev: mpr mps
Message-ID:  <201812212030.wBLKUrTr009269@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
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);
 }
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201812212030.wBLKUrTr009269>