On Thu, 8 Apr 2004, Marcel Holtmann wrote:
> Hi Alan,
>
> > I think this patch for the USB Bluetooth driver will prevent the problems
> > you've been seeing. Let me know what happens.
>
> why do we need a modification of the USB Bluetooth driver? In the case
> of using an OHCI based card I don't see any oopses at URB unlink.
It really is an error in the Bluetooth driver. There's a difference
between the OHCI and UHCI drivers that has the effect of making the error
much less likely when using OHCI.
In more detail:
The code in hci_usb_unlink_urbs() calls usb_unlink_urb() to perform a
synchronous unlink of each pending URB. It then moves each URB to the
completed list, and then frees everything on the completed list. By doing
this, the code implicitly assumes that when usb_unlink_urb() returns the
URB will have completed and be ready to be deallocated. That's not always
true.
(David, are you reading this? It's a perfect example of the sort of error
the current API encourages.)
You can get more information from the kerneldoc for usb_unlink_urb(), but
basically it only guarantees that the URB will have completed if the
return value is 0 -- if there's an error then all bets are off. The code
in hci_usb_unlink_urbs() doesn't even look at the return value -- although
doing so wouldn't help much, since you have to wait for the URB to
complete in any case.
Normally, of course, there is no error and the code works fine. But there
is one particular situation where an error is bound to occur: If the URB
has _already_ been unlinked asynchronously, then usb_unlink_urb() will
return immediately without waiting for the earlier unlink to complete.
As it happens, the USB core performs exactly this kind of asynchronous
unlink for every active URB when a device is unplugged. The consequence
is that the Bluetooth driver was deallocating URBs while the UHCI driver
was still using them, and that led to the oopses.
They didn't show up with the OHCI driver because that driver implements a
disable_endpoint() method. The method waits until all URBS for a given
endpoint are completely finished, thereby preventing the Bluetooth driver
from deallocating the storage too early. UHCI doesn't yet implement
disable_endpoint(), which accounts for the difference you see.
However, just because the driver works with OHCI doesn't mean it's right.
Other events can cause usb_unlink_urb() to return an error, and this can
happen during any device close, not just during disconnect (which is when
disable_endpoint() is normally called). The patch I wrote really is
needed, to guarantee that the URB is not still in use when it is freed.
By the way, I noticed a couple of other little things in the driver that
could be improved. The first is _urb_alloc() -- it should call
usb_init_urb() rather than initializing the URB by hand. The second was
the test of the HCI_RUNNING flags bit in the hci_usb_{t,r}x_complete()
functions. Those tests should be made while holding hci->completion_lock
for proper synchronization. Otherwise you can have a race on an SMP
system in which hci_usb_rx_complete() finds the bit is set; then on
another processor hci_usb_close() clears the bit, grabs & releases the
lock, and unlinks all the active URBs; then the first processor acquires
the lock and resubmits the URB.
Alan Stern
-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
