Matt,
In usb.c, we could patch usb_bulk_msg() to check for
a NULL <dev> and return -EINVAL if NULL.
[usb_submit_urb() already hanles dev == NULL.]
In usb.h, we could change __create_pipe() to return 0
if <dev> is NULL. That would take care of all uses of
usb_snd{type}pipe(). Or should we limit it to
usb_sndbulkpipe() ?
This latter change won't cause an error (just avoid an
oops).
Is anyone concerned about these suggested changes,
either from a performance standpoint or the behavior
of __create_pipe()?
Matt, Pavel -- have either of you tried to force the
problem with these kinds of changes? The patches
are trivial, of course.
~Randy
___________________________________________________
|Randy Dunlap Intel Corp., DAL Sr. SW Engr.|
|randy.dunlap.at.intel.com 503-696-2055|
|NOTE: Any views presented here are mine alone |
|and may not represent the views of my employer. |
|_________________________________________________|
> -----Original Message-----
> From: Matthew Dharm [mailto:[EMAIL PROTECTED]]
> Sent: Saturday, March 25, 2000 2:12 PM
> To: Pavel Machek
> Cc: Linux USB Developement Mailing List; Randy Dunlap; [EMAIL PROTECTED]
> Subject: Re: [linux-usb] PATCH: usb-storage: insmod/rmmod, resource
> freeing, ORB
>
> Interesting. Very interesting.
>
> Well, clearly one thing I can do is try to place locks around
> everything
> to eliminate this race condition. But this raises two concerns to me:
>
> (1) Lots of locks means poor performance
> (2) Lots of drivers probably suffer from this same problem.
> usb-storage
> is probably just one of the few that does so many repeated
> transfers over
> such a long period of time to make this visible.
>
> It occurs to me that the best answer is probably to make it so that
> usb_sndbulkpipe checks for a NULL device and make it so that
> usb_bulk_msg() can also handle a NULL device. Returning an error to
> indicate "invalid device" would probably be the best way to go.
>
> Taking this approach at a lower level would then make it easier for
> drivers which need to do multi-stage transactions to avoid race
> conditions. Otherwise, my driver needs to either lock around
> a large and
> time-lengthy piece of code, or lock and unlock repeatedly around each
> stage of the transaction.
>
> Matt Dharm
>
> On Thu, 23 Mar 2000, Pavel Machek wrote:
>
> > Hi!
> >
> > > (o) Fixes an over-agressive sanity check that affects ORB
> drives and
> > > someone's CD-ROM drive.
> > > (o) Frees _most_ resources when driver is rmmod'ed.
> About 16 bytes/device
> > > still remain. This is being worked on.
> > > (o) Allows the driver to be insmod'ed and rmmod'ed
> repeatedly. This works
> > > with OHCI, and Georg Acher has a patch for usb-uhci to
> fix this bug. A
> > > patch for uhci.o is also needed. Georg should be
> submitting his patch
> > > soon. I do not know when a patch from JE is coming.
> > > (o) Adds my name to the maintainers file
> >
> > Now I know why it oopses when I unplug it in error loop:
> >
> > at one of marked points, us->pusb_dev gets NULL, and because
> > usb_sndbulkpipe does no checking, oops is imminent.
> >
> > (Checking for NULL and returning prevents oops but induces panic bit
> > later.)
> >
> > static int Bulk_transport(Scsi_Cmnd *srb, struct us_data *us)
> > {
> > struct bulk_cb_wrap bcb;
> > struct bulk_cs_wrap bcs;
> > int result;
> > int pipe;
> > int partial;
> >
> > /* set up the command wrapper */
> > bcb.Signature = US_BULK_CB_SIGN;
> > bcb.DataTransferLength = us_transfer_length(srb);
> > bcb.Flags = US_DIRECTION(srb->cmnd[0]) << 7;
> > bcb.Tag = srb->serial_number;
> > bcb.Lun = 0;
> > bcb.Length = srb->cmd_len;
> >
> > /* construct the pipe handle */
> > ========>>>>>
> > pipe = usb_sndbulkpipe(us->pusb_dev, us->ep_out);
> >
> > /* copy the command payload */
> > memset(bcb.CDB, 0, sizeof(bcb.CDB));
> > memcpy(bcb.CDB, srb->cmnd, bcb.Length);
> >
> > /* send it to out endpoint */
> > US_DEBUGP("Bulk command S 0x%x T 0x%x L %d F %d CL %d\n",
> > bcb.Signature, bcb.Tag, bcb.DataTransferLength,
> > bcb.Flags, bcb.Length);
> > result = usb_bulk_msg(us->pusb_dev, pipe, &bcb,
> > US_BULK_CB_WRAP_LEN, &partial, HZ*5);
> > US_DEBUGP("Bulk command transfer result=%d\n", result);
> >
> > /* if we stall, we need to clear it before we go on */
> > if (result == -EPIPE) {
> > US_DEBUGP("clearing endpoint halt for pipe
> 0x%x\n", pipe);
> > usb_clear_halt(us->pusb_dev, pipe);
> > }
> >
> > /* if the command transfered well, then we go to the
> data stage */
> > if (result == 0) {
> > /* send/receive data payload, if there is any */
> > if (bcb.DataTransferLength) {
> > us_transfer(srb, bcb.Flags);
> > US_DEBUGP("Bulk data transfer result 0x%x\n",
> > srb->result);
> > }
> > }
> >
> > /* See flow chart on pg 15 of the Bulk Only Transport spec for
> > * an explanation of how this code works.
> > */
> >
> > /* construct the pipe handle */
> > =========>>>>>>>
> > pipe = usb_rcvbulkpipe(us->pusb_dev, us->ep_in);
> >
> > /* get CSW for device status */
> > result = usb_bulk_msg(us->pusb_dev, pipe, &bcs,
> > US_BULK_CS_WRAP_LEN, &partial, HZ*5);
> >
> > /* did the attempt to read the CSW fail? */
> > if (result == -EPIPE) {
> > US_DEBUGP("clearing endpoint halt for pipe
> 0x%x\n", pipe);
> > usb_clear_halt(us->pusb_dev, pipe);
> >
> > /* get the status again */
> > result = usb_bulk_msg(us->pusb_dev, pipe, &bcs,
> > US_BULK_CS_WRAP_LEN,
> &partial, HZ*5);
> >
> > /* if it fails again, we need a reset and
> return an error*/
> > if (result == -EPIPE) {
> > Bulk_reset(us);
> --
> Matthew Dharm Home:
> [EMAIL PROTECTED]
> Engineer, Qualcomm, Inc. Work:
> [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
