Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 5 Nov 2018 18:47:30 +0000 (UTC)
From:      Warner Losh <imp@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r340155 - head/sys/cam/scsi
Message-ID:  <201811051847.wA5IlUdA056116@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: imp
Date: Mon Nov  5 18:47:29 2018
New Revision: 340155
URL: https://svnweb.freebsd.org/changeset/base/340155

Log:
  Only assert locked for many async events.
  
  Many async events that we see are called for this specific path. When
  calling an async callback for a targetted device, XTP will lock that
  specific device's path lock (same as what cam_periph_lock does). For
  those AC_ events, assert we have the lock rather than trying to
  recusrively take it (which causes panics since it's not recursive).
  
  Add annotations about this and about the fact that AC_SCSI_AEN events
  are generated now only in the ata stack (which cannot have a scsi_da
  attachment). Leave it in place in case I've overlooked something as
  the code is harmless.
  
  This is fallout from my attempts to "fix" locking for softc->flags in
  r330796 that's not been triggered often enough to get my attention
  until now.
  
  Sponsored by: Netflix
  MFC After: 3 days
  Differential Revision: https://reviews.freebsd.org/D17837

Modified:
  head/sys/cam/scsi/scsi_da.c

Modified: head/sys/cam/scsi/scsi_da.c
==============================================================================
--- head/sys/cam/scsi/scsi_da.c	Mon Nov  5 18:12:41 2018	(r340154)
+++ head/sys/cam/scsi/scsi_da.c	Mon Nov  5 18:47:29 2018	(r340155)
@@ -2005,7 +2005,7 @@ daasync(void *callback_arg, u_int32_t code,
 
 	periph = (struct cam_periph *)callback_arg;
 	switch (code) {
-	case AC_FOUND_DEVICE:
+	case AC_FOUND_DEVICE:	/* callback to create periph, no locking yet */
 	{
 		struct ccb_getdev *cgd;
 		cam_status status;
@@ -2041,7 +2041,7 @@ daasync(void *callback_arg, u_int32_t code,
 				"due to status 0x%x\n", status);
 		return;
 	}
-	case AC_ADVINFO_CHANGED:
+	case AC_ADVINFO_CHANGED:	/* Doesn't touch periph */
 	{
 		uintptr_t buftype;
 
@@ -2064,8 +2064,10 @@ daasync(void *callback_arg, u_int32_t code,
 		ccb = (union ccb *)arg;
 
 		/*
-		 * Handle all UNIT ATTENTIONs except our own,
-		 * as they will be handled by daerror().
+		 * Handle all UNIT ATTENTIONs except our own, as they will be
+		 * handled by daerror(). Since this comes from a different periph,
+		 * that periph's lock is held, not ours, so we have to take it ours
+		 * out to touch softc flags.
 		 */
 		if (xpt_path_periph(ccb->ccb_h.path) != periph &&
 		    scsi_extract_sense_ccb(ccb,
@@ -2093,9 +2095,13 @@ daasync(void *callback_arg, u_int32_t code,
 		}
 		break;
 	}
-	case AC_SCSI_AEN:
+	case AC_SCSI_AEN:		/* Called for this path: periph locked */
+		/*
+		 * Appears to be currently unused for SCSI devices, only ata SIMs
+		 * generate this.
+		 */
+		cam_periph_assert(periph, MA_OWNED);
 		softc = (struct da_softc *)periph->softc;
-		cam_periph_lock(periph);
 		if (!cam_iosched_has_work_flags(softc->cam_iosched, DA_WORK_TUR) &&
 		    (softc->flags & DA_FLAG_TUR_PENDING) == 0) {
 			if (da_periph_acquire(periph, DA_REF_TUR) == 0) {
@@ -2103,31 +2109,28 @@ daasync(void *callback_arg, u_int32_t code,
 				daschedule(periph);
 			}
 		}
-		cam_periph_unlock(periph);
 		/* FALLTHROUGH */
-	case AC_SENT_BDR:
-	case AC_BUS_RESET:
+	case AC_SENT_BDR:		/* Called for this path: periph locked */
+	case AC_BUS_RESET:		/* Called for this path: periph locked */
 	{
 		struct ccb_hdr *ccbh;
 
+		cam_periph_assert(periph, MA_OWNED);
 		softc = (struct da_softc *)periph->softc;
 		/*
 		 * Don't fail on the expected unit attention
 		 * that will occur.
 		 */
-		cam_periph_lock(periph);
 		softc->flags |= DA_FLAG_RETRY_UA;
 		LIST_FOREACH(ccbh, &softc->pending_ccbs, periph_links.le)
 			ccbh->ccb_state |= DA_CCB_RETRY_UA;
-		cam_periph_unlock(periph);
 		break;
 	}
-	case AC_INQ_CHANGED:
-		cam_periph_lock(periph);
+	case AC_INQ_CHANGED:		/* Called for this path: periph locked */
+		cam_periph_assert(periph, MA_OWNED);
 		softc = (struct da_softc *)periph->softc;
 		softc->flags &= ~DA_FLAG_PROBED;
 		dareprobe(periph);
-		cam_periph_unlock(periph);
 		break;
 	default:
 		break;

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