On Fri, 9 Apr 2004, Greg KH wrote:
> On Fri, Apr 09, 2004 at 11:24:44AM -0400, Alan Stern wrote:
> > Greg, what do you think? Should the core have a usb_wait_for_urb()
> > function that simply blocks until the URB's refcount goes back down to 1?
>
> I don't think it's necessary if a driver uses the usb_alloc_urb() and
> usb_free_urb() calls, right?
If the bluetooth driver had done that, it would not have experienced this
problem.
(There might still have been a related problem: rmmod might succeed in
unloading the driver from memory before all the completion handlers for
URBs-in-flight had run. Nobody has reported that problem to my knowledge.
Probably unmapping the code region will always take more than a few
milliseconds so it will never come up.)
> That being said:
>
> > Wasn't the whole point of the difference between them that asynchronous
> > usb_unlink_urb() guarantees only the first one whereas synchronous
> > usb_unlink_urb() tries to guarantee both? Unfortunately it only _tries_,
> > it doesn't _guarantee_.
>
> That's not ok. Synchronous usb_unlink_urb() really needs to work
> properly. No "tries", it must. Otherwise drivers have no idea if they
> can reuse the urb or not. That's not acceptable, and needs to get fixed
> as soon as possible.
This has been the heart of a long discussion between David and me. The
main points coming out of that discussion are:
1. Synchronous usb_unlink_urb() does work properly, in the sense
that it does what the kerneldoc says it will do.
2. Sometimes drivers need to know that an URB is available for
reuse, not just that it has been unlinked. Synchronous
unlinks are not the right way to do this, despite what people
may think (and despite the connotations of "synchronous").
3. Sometimes drivers need to know not that an URB is available
for deallocation (which won't matter if it uses usb_free_urb)
but rather that the completion handler has finished executing.
Again, synchronous unlinks are not the way to do this.
4. Most, but not all, of the time drivers need to know these things
only during their disconnect() routines. Most of the time
synchronous unlink returns with no error (in which case it does
what the driver needs) -- but that often fails during disconnect
when using UHCI!
5. Waiting for the URB's refcount to drop to 1 does accomplish the
tasks mentioned in (2) and (3), unless the driver has performed
some weird meddling with the count.
6. For many purposes, synchronous usb_unlink_urb could be replaced
by a combination of asynchronous unlink followed by wait_for_urb.
The two approaches differ when the completion handler resubmits
the URB: synchronous unlink will return after the handler runs
(if no errors crop up) whereas wait_for_urb will wait forever.
During disconnect this doesn't matter since the core guarantees
that no new URBs will be accepted for any endpoint in the
interface being unbound.
7. When the core calls a driver's disconnect() method, it guarantees
in addition that all in-flight URBs for endpoints in the
interface have been unlinked. However, it does _not_ guarantee
that those URBs have completed. Thanks to the presence of
disable_endpoint in the OHCI and EHCI drivers this happens as
a side-effect; it doesn't happen with the UHCI driver.
8. There are occasions other than disconnect when drivers do some
of these questionable things. Like the bluetooth hci_usb driver,
which shares code between its disconnect and device-close
handlers.
Plus maybe a few other things I can't remember right now. The whole thing
is kind of a mess. Ideally I would like to see synchronous unlink go away
entirely, to be replaced by async unlink, wait_for_urb, plus a simple
means of reliably preventing resubmission by completion handlers. Not an
easy thing to do, especially during a stable kernel series.
> So only uhci has this issue, right? Sounds like uhci need to implement
> the wait_for_urb() call internally to its driver to accomplish the
> synchronous unlink.
Well, as you can see it's a lot of interlocking issues, not just the lack
of some functionality in UHCI. While the lines of responsibility are
clearly drawn at the moment, not everyone feels they are drawn well. So
while this was definitely a bug in the bluetooth driver, one can argue
that the USB API should be changed to make the core do more of the work.
Until that change is made, however, we just have to keep in mind that
synchronous unlinks are liable to be sources of trouble.
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
