On Fri, 9 Apr 2004, David Brownell wrote: > 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.
No, using the refcount is the only possibility. That's because the event must not take place until _after_ the completion routine has been called. The only thing usb_hcd_giveback_urb() does after running the completion routine is usb_put_urb(). On the other hand, it's true that in some exotic situations refcount = 1 might not be correct either. At least it was right for the bluetooth 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. Yeah: bad choice of name, bad API. > > 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. The no-pending-I/O part is less important, since the core insures that no new URBs can be submitted and all old ones are unlinked before calling disable_endpoint. Also, at some time UHCI is going to change over to using one QH per endpoint, not per URB. Implementing disable_endpoint may become more important then -- although still not definitely required. > 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. What URB state do you mean? Surely not "unlinked but still in use" -- that is no more common now than it ever was. Anyway it's not the state that's the problem. The problem is that the state is now quite likely to occur at a time when the driver is awake and active, namely, during unbinding. > 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! :) All except for the ones not associated with disconnect. In the bluetooth driver, for example, exactly the same code is also used for device close. The same problem could have cropped up there too, if a packet was received from the device at the wrong time. Of course that's much less likely. > > 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. I kind of like the idea of having an obscure problem like this _not_ be rare, so that it shows up early and is easy to diagnose and fix. If UHCI _had_ implemented disable_endpoint and someone encountered that race while closing the bluetooth device, it would have been impossible to reproduce or find the error. > 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. I wrote a patch that did just that back early last summer. If necessary I could revamp it. Just now I don't think it's necessary. 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
