Date: Tue, 26 Nov 2002 15:26:19 -0800 (PST) From: Nate Lawson <nate@root.org> To: Yar Tikhiy <yar@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: Review by USB wizard wanted Message-ID: <Pine.BSF.4.21.0211261514380.87003-100000@root.org> In-Reply-To: <20021125164551.A26800@comp.chem.msu.su>
next in thread | previous in thread | raw e-mail | index | archive | help
I'm not a usb expert but I think your patch deserves some comments.
On Mon, 25 Nov 2002, Yar Tikhiy wrote:
> First, sometimes (especially, if twitching a memory stick out of
> the reader while the device is being detected) a transfer to the
> umass device is initiated *after* the device is already gone. System
> panic follows. The transfer is initiated when destroying the default
> pipe to the device. Indeed, the current usb_subr.c code will detach
> child devices first and destroy the default pipe then. Reverting
> this order eliminates the panic.
>
> Second, twitching a memory stick can cause CAM jam. That happens
> because the umass detach routine won't wake up the upper layer when
> processing a device with a pending transfer on it.
>
> Patches addressing the above problems are attached below.
> [...]
> --- usb_subr.c.orig Sat Nov 16 12:07:50 2002
> +++ usb_subr.c Fri Nov 22 15:45:35 2002
> @@ -1292,8 +1292,6 @@
> {
> int ifcidx, nifc;
>
> - if (dev->default_pipe != NULL)
> - usbd_kill_pipe(dev->default_pipe);
> if (dev->ifaces != NULL) {
> nifc = dev->cdesc->bNumInterface;
> for (ifcidx = 0; ifcidx < nifc; ifcidx++)
> @@ -1340,6 +1338,9 @@
> return;
> }
> #endif
> +
> + if (dev->default_pipe != NULL)
> + usbd_kill_pipe(dev->default_pipe);
>
> if (dev->subdevs != NULL) {
> DPRINTFN(3,("usb_disconnect_port: disconnect subdevs\n"));
>
This change looks good to me. With your patch, we need to be careful if
anyone ever calls usb_free_device() directly since the default pipe won't
be killed. But since I don't see it called by anyone else, it seems ok.
Why not make usb_free_device() static too?
> --- umass.c.orig Sat Nov 16 12:07:50 2002
> +++ umass.c Fri Nov 22 21:42:10 2002
> @@ -1033,6 +1033,13 @@
> /* detach the device from the SCSI host controller (SIM) */
> err = umass_cam_detach(sc);
>
> + /* if upper layer is waiting for a transfer to finish, wake it up */
> + if (sc->transfer_state != TSTATE_IDLE) {
> + sc->transfer_state = TSTATE_IDLE;
> + sc->transfer_cb(sc, sc->transfer_priv,
> + sc->transfer_datalen, STATUS_WIRE_FAILED);
> + }
> +
> for (i = 0; i < XFER_NR; i++)
> if (sc->transfer_xfer[i])
> usbd_free_xfer(sc->transfer_xfer[i]);
This looks good except you're passing the full datalen as the
residue. Does this address the situation where the data has been
partially transferred successfully? Or should you subtract
transfer_actlen in that case? I think your code is correct, I just am not
familiar enough with usb to be sure.
After addressing these questions, please send your patch to re@ for commit
approval.
-Nate
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0211261514380.87003-100000>