Date: Sun, 1 Dec 2002 14:21:30 +0300 From: Yar Tikhiy <yar@freebsd.org> To: Nate Lawson <nate@root.org> Cc: freebsd-hackers@freebsd.org Subject: Re: Review by USB wizard wanted Message-ID: <20021201142129.B94232@comp.chem.msu.su> In-Reply-To: <Pine.BSF.4.21.0211261514380.87003-100000@root.org>; from nate@root.org on Tue, Nov 26, 2002 at 03:26:19PM -0800 References: <20021125164551.A26800@comp.chem.msu.su> <Pine.BSF.4.21.0211261514380.87003-100000@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Nov 26, 2002 at 03:26:19PM -0800, Nate Lawson wrote:
> I'm not a usb expert but I think your patch deserves some comments.
Thank you!
> 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.
Of course, I've made sure it's the only point where usb_free_device()
would be called from.
> Why not make usb_free_device() static too?
Good point!
> > --- 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 examining the case, I think transfer_actlen should be subtracted
from transfer_datalen there for consistency. However, Nick Hibma has
told me the residue argument value didn't matter in most cases of error.
--
Yar
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?20021201142129.B94232>