I haven't tried to reproduce this problem at all yet -- the powers that be
gave me about 20 hours of notice that I had to pack everything in my
office to be moved, and all my stuff (including my 3 computers) is still
in transit.
Pavel -- give these patch ideas from Randy a try. I really think that
this is the right way to go. And, since you first reported the problem,
it seems to make sense that you'd have the best luck testing these ideas.
Matt
On Sun, 26 Mar 2000, Dunlap, Randy wrote:
> 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]
>
--
Matthew Dharm Home: [EMAIL PROTECTED]
Engineer, Qualcomm, Inc. Work: [EMAIL PROTECTED]
It was a new hope.
-- Dust Puppy
User Friendly, 12/25/1998
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
