Subject: Request for review on scsipi changes
To: None <tech-kern@netbsd.org>
From: James Chacon <jchacon@genuity.net>
List: tech-kern
Date: 09/16/2002 13:28:44
Today scsipi forks a kthread off for each bus and all work happens within
the thread.

However in one place it attempts to do some scsi probes outside of this model,
which is at autoconfiguration time. The current code uses config_interrupts
and a callback to do the initial device probe and subattach's as required.

The problem with this is thread context. It runs the initial probes inside
of whatever thread configured the bus in the first place. For firewire this
is the fwohci thread for instance. What happens in the probes is this thread
is then put to sleep waiting on a scsipi_complete to be called from an
interrupt handler. That then causes a deadlock for that thread since in cases 
of firewire the fwohci thread is just concerned with pulling packets off the
wire and notifying upper layer devices to go to work.

So the 2 choices to fix this were:

1. Make the main firewire driver aware of it's scsipi attachment and figure
   out a clean way to post the scsipi_complete's. 

2. The rest of the scsipi model works once the kthread is forked off
   and all work happens within the thread. (it can be put to sleep waiting on
   a scsipi_complete call and nothing else will deadlock). So to solve this,
   force the device probes and subattach's to happen within the kthread.

After talking with Jason a few months ago it seemed like #2 is the cleaner
way to do this and makes the most sense.

Attached are patches that do #2. Of the 2 sides to scsipi only the scsi end
needed this as atapi doesn't have this problem. To account for "jitter" and
to make sure all devices always attach in the same order a queue is used to
force all probes to occur in bus attach order.

I'd like any comments/concerns on these before I commit them as it affects
a fairly critical area of code for systems. I've been running with them in
a 2 scsibus configured box for months now and every reboot probes in the
correct order. (and firewire doesn't deadlock anymore).

James

Index: sys/dev/scsipi/atapiconf.c
===================================================================
RCS file: /cvsroot/syssrc/sys/dev/scsipi/atapiconf.c,v
retrieving revision 1.48
diff -u -r1.48 atapiconf.c
--- sys/dev/scsipi/atapiconf.c	2002/04/01 20:37:41	1.48
+++ sys/dev/scsipi/atapiconf.c	2002/09/16 16:35:07
@@ -186,6 +186,8 @@
 	printf(": %d targets\n", chan->chan_ntargets);
 
 	/* Initialize the channel. */
+	chan->chan_init_cb = NULL;
+	chan->chan_init_cb_arg = NULL;
 	scsipi_channel_init(chan);
 
 	/* Probe the bus for devices. */
Index: sys/dev/scsipi/scsiconf.c
===================================================================
RCS file: /cvsroot/syssrc/sys/dev/scsipi/scsiconf.c,v
retrieving revision 1.187
diff -u -r1.187 scsiconf.c
--- sys/dev/scsipi/scsiconf.c	2002/09/06 13:23:31	1.187
+++ sys/dev/scsipi/scsiconf.c	2002/09/16 16:35:07
@@ -67,6 +67,8 @@
 #include <sys/conf.h>
 #include <sys/fcntl.h>
 #include <sys/scsiio.h>
+#include <sys/queue.h>
+#include <sys/lock.h>
 
 #include <dev/scsipi/scsi_all.h>
 #include <dev/scsipi/scsipi_all.h>
@@ -81,6 +83,15 @@
 	NULL,
 };
 
+struct scsi_initq {
+	struct scsipi_channel *sc_channel;
+	TAILQ_ENTRY(scsi_initq) scsi_initq;
+};
+
+static TAILQ_HEAD(, scsi_initq) scsi_initq_head;
+static int scsi_initq_inited = 0;
+static struct simplelock scsibus_interlock = SIMPLELOCK_INITIALIZER;
+
 int	scsi_probe_device __P((struct scsibus_softc *, int, int));
 
 int	scsibusmatch __P((struct device *, struct cfdata *, void *));
@@ -107,7 +118,7 @@
 };
 
 int	scsibusprint __P((void *, const char *));
-void	scsibus_config_interrupts __P((struct device *));
+void	scsibus_config __P((struct scsipi_channel *, void *));
 
 const struct scsipi_bustype scsi_bustype = {
 	SCSIPI_BUSTYPE_SCSI,
@@ -161,47 +172,78 @@
 {
 	struct scsibus_softc *sc = (void *) self;
 	struct scsipi_channel *chan = aux;
+	struct scsi_initq *scsi_initq;
 
 	sc->sc_channel = chan;
 	chan->chan_name = sc->sc_dev.dv_xname;
 
+	printf(": %d %s, %d luns per target\n",
+	    chan->chan_ntargets, chan->chan_ntargets == 1 ? "target":"targets",
+	    chan->chan_nluns);
+
 	/* Initialize the channel structure first */
+	chan->chan_init_cb = scsibus_config;
+	chan->chan_init_cb_arg = sc;
+	
+	scsi_initq = malloc(sizeof(struct scsi_initq), M_DEVBUF, M_WAITOK);
+	if (!scsi_initq_inited) {
+		TAILQ_INIT(&scsi_initq_head);
+		scsi_initq_inited = 1;
+	}
+	scsi_initq->sc_channel = chan;
+	TAILQ_INSERT_TAIL(&scsi_initq_head, scsi_initq, scsi_initq);
 	if (scsipi_channel_init(chan)) {
 		printf(": failed to init channel\n");
 		return;
 	}
 
-	printf(": %d targets, %d luns per target\n",
-	    chan->chan_ntargets, chan->chan_nluns);
-
-
-	/*
-	 * Defer configuration of the children until interrupts
-	 * are enabled.
-	 */
-	config_interrupts(self, scsibus_config_interrupts);
 }
 
 void
-scsibus_config_interrupts(self)
-	struct device *self;
+scsibus_config(chan, arg)
+	struct scsipi_channel *chan;
+	void *arg;
 {
-	struct scsibus_softc *sc = (void *) self;
+	struct scsibus_softc *sc = arg;
+	struct scsi_initq *scsi_initq;
 
 #ifndef SCSI_DELAY
 #define SCSI_DELAY 2
 #endif
 
-	if ((sc->sc_channel->chan_flags & SCSIPI_CHAN_NOSETTLE) == 0 &&
+	config_pending_incr();
+
+	/* Make sure the devices probe in scsibus order to avoid jitter. */
+	simple_lock(&scsibus_interlock);
+	while (1) {
+		scsi_initq = TAILQ_FIRST(&scsi_initq_head);
+		if (scsi_initq->sc_channel == chan)
+			break;
+		ltsleep(&scsi_initq_head, PRIBIO, "scsi_initq", 0, 
+		    &scsibus_interlock);
+	}
+
+	simple_unlock(&scsibus_interlock);
+
+	if ((chan->chan_flags & SCSIPI_CHAN_NOSETTLE) == 0 &&
 	    SCSI_DELAY > 0) {
 		printf("%s: waiting %d seconds for devices to settle...\n",
-		    self->dv_xname, SCSI_DELAY);
+		    sc->sc_dev.dv_xname, SCSI_DELAY);
 		/* ...an identifier we know no one will use... */
-		(void) tsleep(scsibus_config_interrupts, PRIBIO,
+		(void) tsleep(scsibus_config, PRIBIO,
 		    "scsidly", SCSI_DELAY * hz);
 	}
 
 	scsi_probe_bus(sc, -1, -1);
+
+	simple_lock(&scsibus_interlock);
+	TAILQ_REMOVE(&scsi_initq_head, scsi_initq, scsi_initq);
+	simple_unlock(&scsibus_interlock);
+
+	free(scsi_initq, M_DEVBUF);
+	wakeup(&scsi_initq_head);
+
+	config_pending_decr();
 }
 
 int
Index: sys/dev/scsipi/scsipi_base.c
===================================================================
RCS file: /cvsroot/syssrc/sys/dev/scsipi/scsipi_base.c,v
retrieving revision 1.78
diff -u -r1.78 scsipi_base.c
--- sys/dev/scsipi/scsipi_base.c	2002/07/26 14:11:34	1.78
+++ sys/dev/scsipi/scsipi_base.c	2002/09/16 16:35:08
@@ -2075,6 +2075,9 @@
 	struct scsipi_xfer *xs;
 	int s;
 
+	if (chan->chan_init_cb)
+		chan->chan_init_cb(chan, chan->chan_init_cb_arg);
+
 	s = splbio();
 	chan->chan_flags |= SCSIPI_CHAN_TACTIVE;
 	splx(s);
Index: sys/dev/scsipi/scsipiconf.h
===================================================================
RCS file: /cvsroot/syssrc/sys/dev/scsipi/scsipiconf.h,v
retrieving revision 1.69
diff -u -r1.69 scsipiconf.h
--- sys/dev/scsipi/scsipiconf.h	2002/05/16 02:54:21	1.69
+++ sys/dev/scsipi/scsipiconf.h	2002/09/16 16:35:08
@@ -298,6 +298,10 @@
 	/* callback we may have to call from completion thread */
 	void (*chan_callback) __P((struct scsipi_channel *, void *));
 	void *chan_callback_arg;
+
+	/* callback we may have to call after forking the kthread */
+	void (*chan_init_cb) __P((struct scsipi_channel *, void *));
+	void *chan_init_cb_arg;
 };
 
 /* chan_flags */