On Thu, 8 Apr 2004, David Brownell wrote: > Alan Stern wrote: > > > > 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.) > > And last time this came up, I thought there was agreement that > adding some new synchronous urb_begone() function was fine. I > really think synchronous unlink should just vanish from the API; > it's virtually never going to be used correctly.
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. 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? > 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_. > Clearly driver disconnect must synchronize on the latter. > So when bluetooth uses an API that synchronizes on the former, > naturally it's broken. Exactly. > 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. > > 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. > > So a quick'n'dirty fix might be a uhci_disable_endpoint() > call that sleeps for N>1 msec ... that assumes the UHCI driver's > IRQ handler will be able to fire, and issue the completions. > > It might be good to do that, just to help cover up the drivers > that synchronize on the wrong thing. (Although as you noted, > Bluetooth needs to be fixed regardless.) I don't think that's a good idea. It would tend to hide problems without fixing them. As you noted, the bluetooth driver was broken regardless; other drivers will be also. >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. This particular problem doesn't count, since it could crop up in other situations where disable_endpoint() hadn't been called, even for OHCI. 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
