Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 21 Feb 2011 18:11:56 +0000 (UTC)
From:      "Kenneth D. Merry" <ken@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r218922 - stable/8/sys/dev/mps
Message-ID:  <201102211811.p1LIBuGu078768@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: ken
Date: Mon Feb 21 18:11:56 2011
New Revision: 218922
URL: http://svn.freebsd.org/changeset/base/218922

Log:
  MFC: r218812
  
  Fix several issues with the mps(4) driver.
  
  When the driver ran out of DMA chaining buffers, it kept the timeout for
  the I/O, and I/O would stall.
  
  The driver was not freezing the device queue on errors.
  
  mps.c:		Pull command completion logic into a separate
  		function, and call the callback/wakeup for commands
  		that are never sent due to lack of chain buffers.
  
  		Add a number of extra diagnostic sysctl variables.
  
  		Handle pre-hardware errors for configuration I/O.
  		This doesn't panic the system, but it will fail the
  		configuration I/O and there is no retry mechanism.
  		So the device probe will not succeed.  This should
  		be a very uncommon situation, however.
  
  mps_sas.c:	Freeze the SIM queue when we run out of chain
  		buffers, and unfreeze it when more commands
  		complete.
  
  		Freeze the device queue when errors occur, so that
  		CAM can insure proper command ordering.
  
  		Report pre-hardware errors for task management
  		commands.  In general, that shouldn't be possible
  		because task management commands don't have S/G
  		lists, and that is currently the only error path
  		before we get to the hardware.
  
  		Handle pre-hardware errors (like out of chain
  		elements) for SMP requests.  That shouldn't happen
  		either, since we should have enough space for two
  		S/G elements in the standard request.
  
  		For commands that end with
  		MPI2_IOCSTATUS_SCSI_IOC_TERMINATED and
  		MPI2_IOCSTATUS_SCSI_EXT_TERMINATED, return them
  		with CAM_REQUEUE_REQ to retry them unconditionally.
  		These seem to be related to back end, transport
  		related problems that are hopefully transient.  We
  		don't want to go through the retry count for
  		something that is not a permanent error.
  
  		Keep track of the number of outstanding I/Os.
  
  mpsvar.h:	Track the number of free chain elements.
  
  		Add variables for the number of outstanding I/Os,
  		and I/O high water mark.
  
  		Add variables to track the number of free chain
  		buffers and the chain low water mark, as well as
  		the number of chain allocation failures.
  
  		Add I/O state flags and an attach done flag.

Modified:
  stable/8/sys/dev/mps/mps.c
  stable/8/sys/dev/mps/mps_sas.c
  stable/8/sys/dev/mps/mpsvar.h
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)

Modified: stable/8/sys/dev/mps/mps.c
==============================================================================
--- stable/8/sys/dev/mps/mps.c	Mon Feb 21 16:55:53 2011	(r218921)
+++ stable/8/sys/dev/mps/mps.c	Mon Feb 21 18:11:56 2011	(r218922)
@@ -62,6 +62,7 @@ static void mps_startup(void *arg);
 static void mps_startup_complete(struct mps_softc *sc, struct mps_command *cm);
 static int mps_send_iocinit(struct mps_softc *sc);
 static int mps_attach_log(struct mps_softc *sc);
+static __inline void mps_complete_command(struct mps_command *cm);
 static void mps_dispatch_event(struct mps_softc *sc, uintptr_t data, MPI2_EVENT_NOTIFICATION_REPLY *reply);
 static void mps_config_complete(struct mps_softc *sc, struct mps_command *cm);
 static void mps_periodic(void *);
@@ -387,6 +388,15 @@ mps_enqueue_request(struct mps_softc *sc
 
 	mps_dprint(sc, MPS_TRACE, "%s\n", __func__);
 
+	if (sc->mps_flags & MPS_FLAGS_ATTACH_DONE)
+		mtx_assert(&sc->mps_mtx, MA_OWNED);
+
+	if ((cm->cm_desc.Default.SMID < 1)
+	 || (cm->cm_desc.Default.SMID >= sc->num_reqs)) {
+		mps_printf(sc, "%s: invalid SMID %d, desc %#x %#x\n",
+			   __func__, cm->cm_desc.Default.SMID,
+			   cm->cm_desc.Words.High, cm->cm_desc.Words.Low);
+	}
 	mps_regwrite(sc, MPI2_REQUEST_DESCRIPTOR_POST_LOW_OFFSET,
 	    cm->cm_desc.Words.Low);
 	mps_regwrite(sc, MPI2_REQUEST_DESCRIPTOR_POST_HIGH_OFFSET,
@@ -732,6 +742,7 @@ mps_alloc_requests(struct mps_softc *sc)
 		chain->chain_busaddr = sc->chain_busaddr +
 		    i * sc->facts->IOCRequestFrameSize * 4;
 		mps_free_chain(sc, chain);
+		sc->chain_free_lowwater++;
 	}
 
 	/* XXX Need to pick a more precise value */
@@ -855,6 +866,26 @@ mps_attach(struct mps_softc *sc)
 	    &sc->allow_multiple_tm_cmds, 0,
 	    "allow multiple simultaneous task management cmds");
 
+	SYSCTL_ADD_INT(&sc->sysctl_ctx, SYSCTL_CHILDREN(sc->sysctl_tree),
+	    OID_AUTO, "io_cmds_active", CTLFLAG_RD,
+	    &sc->io_cmds_active, 0, "number of currently active commands");
+
+	SYSCTL_ADD_INT(&sc->sysctl_ctx, SYSCTL_CHILDREN(sc->sysctl_tree),
+	    OID_AUTO, "io_cmds_highwater", CTLFLAG_RD,
+	    &sc->io_cmds_highwater, 0, "maximum active commands seen");
+
+	SYSCTL_ADD_INT(&sc->sysctl_ctx, SYSCTL_CHILDREN(sc->sysctl_tree),
+	    OID_AUTO, "chain_free", CTLFLAG_RD,
+	    &sc->chain_free, 0, "number of free chain elements");
+
+	SYSCTL_ADD_INT(&sc->sysctl_ctx, SYSCTL_CHILDREN(sc->sysctl_tree),
+	    OID_AUTO, "chain_free_lowwater", CTLFLAG_RD,
+	    &sc->chain_free_lowwater, 0,"lowest number of free chain elements");
+
+	SYSCTL_ADD_QUAD(&sc->sysctl_ctx, SYSCTL_CHILDREN(sc->sysctl_tree),
+	    OID_AUTO, "chain_alloc_fail", CTLFLAG_RD,
+	    &sc->chain_alloc_fail, "chain allocation failures");
+
 	if ((error = mps_transition_ready(sc)) != 0)
 		return (error);
 
@@ -895,6 +926,8 @@ mps_attach(struct mps_softc *sc)
 	sc->num_reqs = MIN(MPS_REQ_FRAMES, sc->facts->RequestCredit);
 	sc->num_replies = MIN(MPS_REPLY_FRAMES + MPS_EVT_REPLY_FRAMES,
 	    sc->facts->MaxReplyDescriptorPostQueueDepth) - 1;
+	mps_dprint(sc, MPS_INFO, "num_reqs %d, num_replies %d\n", sc->num_reqs,
+		   sc->num_replies);
 	TAILQ_INIT(&sc->req_list);
 	TAILQ_INIT(&sc->chain_list);
 	TAILQ_INIT(&sc->tm_list);
@@ -968,6 +1001,8 @@ mps_attach(struct mps_softc *sc)
 		error = EINVAL;
 	}
 
+	sc->mps_flags |= MPS_FLAGS_ATTACH_DONE;
+
 	return (error);
 }
 
@@ -1167,6 +1202,22 @@ mps_free(struct mps_softc *sc)
 	return (0);
 }
 
+static __inline void
+mps_complete_command(struct mps_command *cm)
+{
+	if (cm->cm_flags & MPS_CM_FLAGS_POLLED)
+		cm->cm_flags |= MPS_CM_FLAGS_COMPLETE;
+
+	if (cm->cm_complete != NULL)
+		cm->cm_complete(cm->cm_sc, cm);
+
+	if (cm->cm_flags & MPS_CM_FLAGS_WAKEUP) {
+		mps_dprint(cm->cm_sc, MPS_TRACE, "%s: waking up %p\n",
+			   __func__, cm);
+		wakeup(cm);
+	}
+}
+
 void
 mps_intr(void *data)
 {
@@ -1293,16 +1344,8 @@ mps_intr_locked(void *data)
 			break;
 		}
 
-		if (cm != NULL) {
-			if (cm->cm_flags & MPS_CM_FLAGS_POLLED)
-				cm->cm_flags |= MPS_CM_FLAGS_COMPLETE;
-
-			if (cm->cm_complete != NULL)
-				cm->cm_complete(sc, cm);
-
-			if (cm->cm_flags & MPS_CM_FLAGS_WAKEUP)
-				wakeup(cm);
-		}
+		if (cm != NULL)
+			mps_complete_command(cm);
 
 		desc->Words.Low = 0xffffffff;
 		desc->Words.High = 0xffffffff;
@@ -1663,6 +1706,8 @@ mps_data_cb(void *arg, bus_dma_segment_t
 		if (error != 0) {
 			/* Resource shortage, roll back! */
 			mps_printf(sc, "out of chain frames\n");
+			cm->cm_flags |= MPS_CM_FLAGS_CHAIN_FAILED;
+			mps_complete_command(cm);
 			return;
 		}
 	}
@@ -1802,6 +1847,15 @@ mps_config_complete(struct mps_softc *sc
 		bus_dmamap_unload(sc->buffer_dmat, cm->cm_dmamap);
 	}
 
+	/*
+	 * XXX KDM need to do more error recovery?  This results in the
+	 * device in question not getting probed.
+	 */
+	if ((cm->cm_flags & MPS_CM_FLAGS_ERROR_MASK) != 0) {
+		params->status = MPI2_IOCSTATUS_BUSY;
+		goto bailout;
+	}
+
 	reply = (MPI2_CONFIG_REPLY *)cm->cm_reply;
 	params->status = reply->IOCStatus;
 	if (params->hdr.Ext.ExtPageType != 0) {
@@ -1814,6 +1868,8 @@ mps_config_complete(struct mps_softc *sc
 		params->hdr.Struct.PageVersion = reply->Header.PageVersion;
 	}
 
+bailout:
+
 	mps_free_command(sc, cm);
 	if (params->callback != NULL)
 		params->callback(sc, params);

Modified: stable/8/sys/dev/mps/mps_sas.c
==============================================================================
--- stable/8/sys/dev/mps/mps_sas.c	Mon Feb 21 16:55:53 2011	(r218921)
+++ stable/8/sys/dev/mps/mps_sas.c	Mon Feb 21 18:11:56 2011	(r218922)
@@ -520,6 +520,18 @@ mpssas_remove_device(struct mps_softc *s
 
 	mpssas_complete_tm_request(sc, cm, /*free_cm*/ 0);
 
+	/*
+	 * 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.
+	 */
+	if ((cm->cm_flags & MPS_CM_FLAGS_ERROR_MASK) != 0) {
+		mps_printf(sc, "%s: cm_flags = %#x for remove of handle %#04x! "
+			   "This should not happen!\n", __func__, cm->cm_flags,
+			   handle);
+		return;
+	}
+
 	if (reply->IOCStatus != MPI2_IOCSTATUS_SUCCESS) {
 		mps_printf(sc, "Failure 0x%x reseting device 0x%04x\n", 
 		   reply->IOCStatus, handle);
@@ -1100,6 +1112,17 @@ mpssas_abort_complete(struct mps_softc *
 
 	req = (MPI2_SCSI_TASK_MANAGE_REQUEST *)cm->cm_req;
 
+	/*
+	 * 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.
+	 */
+	if ((cm->cm_flags & MPS_CM_FLAGS_ERROR_MASK) != 0) {
+		mps_printf(sc, "%s: cm_flags = %#x for abort on handle %#04x! "
+			   "This should not happen!\n", __func__, cm->cm_flags,
+			   req->DevHandle);
+	}
+
 	mps_printf(sc, "%s: abort request on handle %#04x SMID %d "
 		   "complete\n", __func__, req->DevHandle, req->TaskMID);
 
@@ -1216,7 +1239,8 @@ mpssas_tm_complete(struct mps_softc *sc,
 
 	resp = (MPI2_SCSI_TASK_MANAGE_REPLY *)cm->cm_reply;
 
-	resp->ResponseCode = error;
+	if (resp != NULL)
+		resp->ResponseCode = error;
 
 	/*
 	 * Call the callback for this command, it will be
@@ -1366,6 +1390,7 @@ mpssas_action_scsiio(struct mpssas_softc
 	}
 
 	req = (MPI2_SCSI_IO_REQUEST *)cm->cm_req;
+	bzero(req, sizeof(*req));
 	req->DevHandle = targ->handle;
 	req->Function = MPI2_FUNCTION_SCSI_IO_REQUEST;
 	req->MsgFlags = 0;
@@ -1447,6 +1472,10 @@ mpssas_action_scsiio(struct mpssas_softc
 	cm->cm_complete_data = ccb;
 	cm->cm_targ = targ;
 
+	sc->io_cmds_active++;
+	if (sc->io_cmds_active > sc->io_cmds_highwater)
+		sc->io_cmds_highwater = sc->io_cmds_active;
+
 	TAILQ_INSERT_TAIL(&sc->io_list, cm, cm_link);
 	callout_reset(&cm->cm_callout, (ccb->ccb_h.timeout * hz) / 1000,
 	   mpssas_scsiio_timeout, cm);
@@ -1468,11 +1497,17 @@ mpssas_scsiio_complete(struct mps_softc 
 
 	callout_stop(&cm->cm_callout);
 	TAILQ_REMOVE(&sc->io_list, cm, cm_link);
+	sc->io_cmds_active--;
 
 	sassc = sc->sassc;
 	ccb = cm->cm_complete_data;
 	rep = (MPI2_SCSI_IO_REPLY *)cm->cm_reply;
 
+	/*
+	 * XXX KDM if the chain allocation fails, does it matter if we do
+	 * the sync and unload here?  It is simpler to do it in every case,
+	 * assuming it doesn't cause problems.
+	 */
 	if (cm->cm_data != NULL) {
 		if (cm->cm_flags & MPS_CM_FLAGS_DATAIN)
 			dir = BUS_DMASYNC_POSTREAD;
@@ -1482,9 +1517,34 @@ mpssas_scsiio_complete(struct mps_softc 
 		bus_dmamap_unload(sc->buffer_dmat, cm->cm_dmamap);
 	}
 
-	if (sassc->flags & MPSSAS_QUEUE_FROZEN) {
-		ccb->ccb_h.flags |= CAM_RELEASE_SIMQ;
-		sassc->flags &= ~MPSSAS_QUEUE_FROZEN;
+	if ((cm->cm_flags & MPS_CM_FLAGS_ERROR_MASK) != 0) {
+		/*
+		 * We ran into an error after we tried to map the command,
+		 * so we're getting a callback without queueing the command
+		 * to the hardware.  So we set the status here, and it will
+		 * be retained below.  We'll go through the "fast path",
+		 * because there can be no reply when we haven't actually
+		 * gone out to the hardware.
+		 */
+		ccb->ccb_h.status |= CAM_REQUEUE_REQ;
+
+		/*
+		 * Currently the only error included in the mask is
+		 * MPS_CM_FLAGS_CHAIN_FAILED, which means we're out of
+		 * chain frames.  We need to freeze the queue until we get
+		 * a command that completed without this error, which will
+		 * hopefully have some chain frames attached that we can
+		 * use.  If we wanted to get smarter about it, we would
+		 * only unfreeze the queue in this condition when we're
+		 * sure that we're getting some chain frames back.  That's
+		 * probably unnecessary.
+		 */
+		if ((sassc->flags & MPSSAS_QUEUE_FROZEN) == 0) {
+			xpt_freeze_simq(sassc->sim, 1);
+			sassc->flags |= MPSSAS_QUEUE_FROZEN;
+			mps_printf(sc, "Error sending command, freezing "
+				   "SIM queue\n");
+		}
 	}
 
 	/* Take the fast path to completion */
@@ -1492,6 +1552,15 @@ mpssas_scsiio_complete(struct mps_softc 
 		if ((ccb->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_INPROG) {
 			ccb->ccb_h.status = CAM_REQ_CMP;
 			ccb->csio.scsi_status = SCSI_STATUS_OK;
+
+			if (sassc->flags & MPSSAS_QUEUE_FROZEN) {
+				ccb->ccb_h.status |= CAM_RELEASE_SIMQ;
+				sassc->flags &= ~MPSSAS_QUEUE_FROZEN;
+				mps_printf(sc, "Unfreezing SIM queue\n");
+			}
+		} else {
+			ccb->ccb_h.status |= CAM_DEV_QFRZN;
+			xpt_freeze_devq(ccb->ccb_h.path, /*count*/ 1);
 		}
 		mps_free_command(sc, cm);
 		xpt_done(ccb);
@@ -1547,7 +1616,16 @@ mpssas_scsiio_complete(struct mps_softc 
 		break;
 	case MPI2_IOCSTATUS_SCSI_IOC_TERMINATED:
 	case MPI2_IOCSTATUS_SCSI_EXT_TERMINATED:
+#if 0
 		ccb->ccb_h.status = CAM_REQ_ABORTED;
+#endif
+		mps_printf(sc, "(%d:%d:%d) terminated ioc %x scsi %x state %x "
+			   "xfer %u\n", xpt_path_path_id(ccb->ccb_h.path),
+			   xpt_path_target_id(ccb->ccb_h.path),
+			   xpt_path_lun_id(ccb->ccb_h.path),
+			   rep->IOCStatus, rep->SCSIStatus, rep->SCSIState,
+			   rep->TransferCount);
+		ccb->ccb_h.status = CAM_REQUEUE_REQ;
 		break;
 	case MPI2_IOCSTATUS_INVALID_SGL:
 		mps_print_scsiio_cmd(sc, cm);
@@ -1601,6 +1679,15 @@ mpssas_scsiio_complete(struct mps_softc 
 	if (rep->SCSIState & MPI2_SCSI_STATE_RESPONSE_INFO_VALID)
 		ccb->ccb_h.status = CAM_REQ_CMP_ERR;
 
+	if (sassc->flags & MPSSAS_QUEUE_FROZEN) {
+		ccb->ccb_h.status |= CAM_RELEASE_SIMQ;
+		sassc->flags &= ~MPSSAS_QUEUE_FROZEN;
+		mps_printf(sc, "Command completed, unfreezing SIM queue\n");
+	}
+	if ((ccb->ccb_h.status & CAM_STATUS_MASK) != CAM_REQ_CMP) {
+		ccb->ccb_h.status |= CAM_DEV_QFRZN;
+		xpt_freeze_devq(ccb->ccb_h.path, /*count*/ 1);
+	}
 	mps_free_command(sc, cm);
 	xpt_done(ccb);
 }
@@ -1615,6 +1702,20 @@ mpssas_smpio_complete(struct mps_softc *
 	union ccb *ccb;
 
 	ccb = cm->cm_complete_data;
+
+	/*
+	 * Currently there should be no way we can hit this case.  It only
+	 * happens when we have a failure to allocate chain frames, and SMP
+	 * commands require two S/G elements only.  That should be handled
+	 * in the standard request size.
+	 */
+	if ((cm->cm_flags & MPS_CM_FLAGS_ERROR_MASK) != 0) {
+		mps_printf(sc, "%s: cm_flags = %#x on SMP request!\n",
+			   __func__, cm->cm_flags);
+		ccb->ccb_h.status = CAM_REQ_CMP_ERR;
+		goto bailout;
+	}
+
 	rpl = (MPI2_SMP_PASSTHROUGH_REPLY *)cm->cm_reply;
 	if (rpl == NULL) {
 		mps_dprint(sc, MPS_INFO, "%s: NULL cm_reply!\n", __func__);
@@ -1994,6 +2095,19 @@ mpssas_resetdev_complete(struct mps_soft
 	resp = (MPI2_SCSI_TASK_MANAGE_REPLY *)cm->cm_reply;
 	ccb = cm->cm_complete_data;
 
+	if ((cm->cm_flags & MPS_CM_FLAGS_ERROR_MASK) != 0) {
+		MPI2_SCSI_TASK_MANAGE_REQUEST *req;
+
+		req = (MPI2_SCSI_TASK_MANAGE_REQUEST *)cm->cm_req;
+
+		mps_printf(sc, "%s: cm_flags = %#x for reset of handle %#04x! "
+			   "This should not happen!\n", __func__, cm->cm_flags,
+			   req->DevHandle);
+
+		ccb->ccb_h.status = CAM_REQ_CMP_ERR;
+		goto bailout;
+	}
+
 	printf("resetdev complete IOCStatus= 0x%x ResponseCode= 0x%x\n",
 	    resp->IOCStatus, resp->ResponseCode);
 
@@ -2002,6 +2116,7 @@ mpssas_resetdev_complete(struct mps_soft
 	else
 		ccb->ccb_h.status = CAM_REQ_CMP_ERR;
 
+bailout:
 	mpssas_complete_tm_request(sc, cm, /*free_cm*/ 1);
 
 	xpt_done(ccb);

Modified: stable/8/sys/dev/mps/mpsvar.h
==============================================================================
--- stable/8/sys/dev/mps/mpsvar.h	Mon Feb 21 16:55:53 2011	(r218921)
+++ stable/8/sys/dev/mps/mpsvar.h	Mon Feb 21 18:11:56 2011	(r218922)
@@ -92,6 +92,8 @@ struct mps_command {
 #define MPS_CM_FLAGS_ACTIVE		(1 << 6)
 #define MPS_CM_FLAGS_USE_UIO		(1 << 7)
 #define MPS_CM_FLAGS_SMP_PASS		(1 << 8)
+#define	MPS_CM_FLAGS_CHAIN_FAILED	(1 << 9)
+#define	MPS_CM_FLAGS_ERROR_MASK		MPS_CM_FLAGS_CHAIN_FAILED
 	u_int				cm_state;
 #define MPS_CM_STATE_FREE		0
 #define MPS_CM_STATE_BUSY		1
@@ -119,9 +121,15 @@ struct mps_softc {
 #define MPS_FLAGS_MSI		(1 << 1)
 #define MPS_FLAGS_BUSY		(1 << 2)
 #define MPS_FLAGS_SHUTDOWN	(1 << 3)
+#define	MPS_FLAGS_ATTACH_DONE	(1 << 4)
 	u_int				mps_debug;
 	u_int				allow_multiple_tm_cmds;
 	int				tm_cmds_active;
+	int				io_cmds_active;
+	int				io_cmds_highwater;
+	int				chain_free;
+	int				chain_free_lowwater;
+	uint64_t			chain_alloc_fail;
 	struct sysctl_ctx_list		sysctl_ctx;
 	struct sysctl_oid		*sysctl_tree;
 	struct mps_command		*commands;
@@ -229,8 +237,13 @@ mps_alloc_chain(struct mps_softc *sc)
 {
 	struct mps_chain *chain;
 
-	if ((chain = TAILQ_FIRST(&sc->chain_list)) != NULL)
+	if ((chain = TAILQ_FIRST(&sc->chain_list)) != NULL) {
 		TAILQ_REMOVE(&sc->chain_list, chain, chain_link);
+		sc->chain_free--;
+		if (sc->chain_free < sc->chain_free_lowwater)
+			sc->chain_free_lowwater = sc->chain_free;
+	} else
+		sc->chain_alloc_fail++;
 	return (chain);
 }
 
@@ -240,6 +253,7 @@ mps_free_chain(struct mps_softc *sc, str
 #if 0
 	bzero(chain->chain, 128);
 #endif
+	sc->chain_free++;
 	TAILQ_INSERT_TAIL(&sc->chain_list, chain, chain_link);
 }
 



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