Looks good to me too- FWIW.


On Mon, 16 Sep 2002, James Chacon wrote:

> 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 */
>