I rather agree with you. For the moment, though, I thought it was easier to add the wait_for_urb() function to the Bluetooth driver rather than putting it in the USB core. Maybe that should be done sooner instead of later, however.
Though I notice you synchronized on yet another event: refcount going to one!! I'm not sure "one" is always the right value there, but I like the simple test. Might list_empty(&urb->urb_list) be better? Drivers which use that probably do their own synchronization.
The problem is that while there are two distinct API events here, usb_unlink_urb() quite properly guarantees only the first one:
- urb unlink, moving the URB off the hardware's queue so it finishes early (canceling it)
- urb completion, giving the URB back to the driver.
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_.
When it can guarantee both, it does! When we talked about this before, you highlighted a case where usbcore could synchronize on the completion, but not the unlink: not both, and not the one the function is named for.
By the way, note the lifecycle issue here. If URBs could never be reused, sloppiness here could become more forgivable. But since they are reused, there are more object states.
That's very much like the "don't reuse struct device" rules that you're looking at in another context. That rule is a simplification, and one I can support because creating devices is rare ... but URBs can be submitted thousands of times per second by a single driver, so they really do need a lifecycle that includes re-use.
It's even more complicated by having two different periods for re-use: resubmission by the completion handler vs. re-use sometime later. The core treats these differently, in terms of things like bandwidth reservation.
Yes, more complicated ... which is one reason I may seem to get rather nitpicky in some of these areas! For example: the reservation is associated with the endpoint, not the URB. While that endpoint has a queue of URBs, its reservation is active.
From what I know (and my understanding isn't fully complete), disable_endpoint() isn't really for the benefit of the core. It's there to support HCs that have hardware requirements for freeing resources dedicated to individual endpoints. Since UHCI has no such hardware requirements, it shouldn't really need disable_endpoint(). And I think now the driver has been fixed up enough that many of the problems previously caused by its lack will no longer occur.
Yes, basically a place to handshake with hardware about state it's maintaining -- from perspective of the HC itself, which was the original motivation. UHCI doesn't care about that, but having this helps prevent oopsing and lockups with hardware like OHCI.
At the same time, the HCD ensures that there are no pending I/O operations queued on that endpoint ... they'd be using old endpoint state, like maxpacket 188 vs 968 (for an iso altsetting scenario, also keeping a bandwidth claim live) associated by the HCD with the endpoint QH. Because uhci-hcd uses a QH per URB, it doesn't care about that either ... though it keeps the bandwidth claim live longer.
So all's fine at the HCD/usbcore interface. But the result is that an URB state went from being relatively rare (on 2.4 USB) to common (2.6/UHCI) ... which can oops drivers that don't handle that state correctly, and there are more than a few.
If _all_ HCDs addressed that second point, almost all logic in driver disconnect() relating to unlinking URBs could be removed. That's most of the synchronous unlink calls ... that'd be a good way to get rid of all the synchronous unlink bugs! :)
This particular problem doesn't count, since it could crop up in other situations where disable_endpoint() hadn't been called, even for OHCI.
Except they're rare. Almost all current synchronous unlink calls from USB drivers are in disconnect() entries -- and hence almost all the driver bugs in that area.
Actually UHCI might be able to use a very simple fix. Right after complete_list empties, issue complete_all() on a completion event that uhci_disable_endpoint() is waiting for.
- Dave
------------------------------------------------------- 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
