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]
It's monday. It must be monday.
-- Greg
User Friendly, 5/4/1998
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
