--x+6KMIRAuhnl3hBn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,
I've found various problems in the scsipi detach code, for scsi devices,
when there is active commands for this device.
First, scsipi_kill_pending() will call scsi_kill_pending(), which will
go process the periph->periph_xferq queue, aborting each command with
XS_DRIVER_STUFFUP.
We have a problem here: it use
	while ((xs = TAILQ_FIRST(&periph->periph_xferq)) != NULL)
to process the queue, but scsipi_done() won't cause the command to
be removed from the queue yet: it will be in scsipi_complete() which
is called from the channel kernel thread. So we have a never-ending loop
here (I managed to hang hard a box with this).
If we change this to a TAILQ_FOREACH(), we still have a problem: we're
calling scsipi_done() on a xfer which has been handled to the controller.
The controller will also call scsipi_done() when this xfer is complete.
scsipi_done() is not designed to handle this.
So I think scsi_kill_pending() should just be a noop.

Then, in scsibusdetach() we shut down the channel before detaching the
targets, but we need the kernel thread to clean the command queue.
So, call scsipi_channel_shutdown() after scsipi_target_detach().

Now, if we just do this, hot-plug devices will not work as expected,
because the controller may never scsipi_done() the commands, or may
do it once the command timeout expire. We could either process the
outstanding commands in scsibusdetach(), or in the controller code itself.
It's probably fine to do it in scsibusdetach(), but we also need to handle
the callouts in this case (which was not done before). At first glance,
HBA drivers which support detach don't deal with the callouts at detach time.

Comments ?

Note that I didn't test this yet, as I don't have a USB device in hands,
and my SCSI test box is hung, probably in the scsi_kill_pending() loop,
and I can't get into debugger remotely.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

--x+6KMIRAuhnl3hBn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=diff

Index: scsi_base.c
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/scsi_base.c,v
retrieving revision 1.77
diff -u -r1.77 scsi_base.c
--- scsi_base.c	15 Nov 2001 09:48:16 -0000	1.77
+++ scsi_base.c	4 Aug 2004 23:21:43 -0000
@@ -159,10 +159,4 @@
 scsi_kill_pending(periph)
 	struct scsipi_periph *periph;
 {
-	struct scsipi_xfer *xs;
-
-	while ((xs = TAILQ_FIRST(&periph->periph_xferq)) != NULL) {
-		xs->error = XS_DRIVER_STUFFUP;
-		scsipi_done(xs);
-	}
 }
Index: scsiconf.c
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/scsiconf.c,v
retrieving revision 1.220
diff -u -r1.220 scsiconf.c
--- scsiconf.c	12 Mar 2004 23:00:40 -0000	1.220
+++ scsiconf.c	4 Aug 2004 23:21:45 -0000
@@ -305,16 +305,39 @@
 {
 	struct scsibus_softc *sc = (void *) self;
 	struct scsipi_channel *chan = sc->sc_channel;
+	struct scsipi_periph *periph;
+	int ctarget, clun;
+	struct scsipi_xfer *xs;
+
 
 	/*
-	 * Shut down the channel.
+	 * Process outstanding commands (which will never complete as the
+	 * controller is gone).
 	 */
-	scsipi_channel_shutdown(chan);
+	for (ctarget = 0; ctarget < chan->chan_ntargets; ctarget++) {
+		if (ctarget == chan->chan_id)
+			continue;
+		for (clun = 0; clun < chan->chan_nluns; clun++) {
+			periph = scsipi_lookup_periph(chan, ctarget, clun);
+			if (periph == NULL)
+				continue;
+			TAILQ_FOREACH(xs, &periph->periph_xferq, device_q) {
+				callout_stop(&xs->xs_callout);
+				xs->error = XS_DRIVER_STUFFUP;
+				scsipi_done(xs);
+			}
+		}
+	}
 
 	/*
-	 * Now detach all of the periphs.
+	 * Detach all of the periphs.
 	 */
 	return scsipi_target_detach(chan, -1, -1, flags);
+
+	/*
+	 * Now shut down the channel.
+	 */
+	scsipi_channel_shutdown(chan);
 }
 
 /*

--x+6KMIRAuhnl3hBn--