On Mon, Sep 16, 2002 at 01:28:44PM -0400, James Chacon wrote:

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

Cool.

 > 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).

The actual changes themselves look good to me, but I have a couple of
tiny nits to pick :-)

 > +	printf(": %d %s, %d luns per target\n",
 > +	    chan->chan_ntargets, chan->chan_ntargets == 1 ? "target":"targets",
 > +	    chan->chan_nluns);
 > +

Construct that as:

	printf(": %s target%s, %d lun%s per target\n",
	    chan->chan_ntargets,
	    chan->chan_ntargets == 1 ? "" : "s",
	    chan->chan_nluns,
	    chan->chan_nluns == 1 ? "" : "s");

 >  	/* 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;
 > +	}

Instead of using "scsi_initq_inited", just use TAILQ_INITIALIZER when
the scsi_initq_head is declard.

 > -	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) {

Style guide says use "for (;;) {" for forever loops.

 > +		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);
 > +

Just another style nit: (*chan->chan_init_cb)(...);

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

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>