Hi Alan,
> 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.
many thanks for looking at the problem. I am not an expert when it comes
to USB internals, but from your explanation I ask myself why we don't
have an unlink function that will wait until the last reference to the
URB is gone. Why do we have to do it in the driver itself?
> 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.
Good points. Do you have a patch for me?
Regards
Marcel
-------------------------------------------------------
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
