In my opinion these semantics are insufficient. Certainly they are not appropriate for the way usb_unlink_urb() is actually used in many drivers. A very hasty survey of source files in drivers/usb/class showns that most synchronous unlinkers require these semantics instead: Regardless of whether the URB was in progress or not, when the routine returns the URB must no longer be linked and must be available for immediate reuse.
But did any kernel ever guarantee those semantics? I don't think so. Certainly 2.6 never did, and neither OHCI nor EHCI on 2.4 does...
Even today (because of "no uhci_endpoint_disable") drivers that don't wait for all their completions in disconnect() will have unique problems. That family of problems was worse on 2.4 kernels.
Drivers now could easily get those cleanup semantics with something much smaller than your patch (though hcd_unlink_urb should still get rid of that splice logic!):
static inline void urb_begone(struct urb *urb)
{
while (usb_unlink_urb (urb) == -EBUSY) {
set_current_state (TASK_UNINTERRUPTIBLE);
schedule_timeout (HZ/100);
}
}I still prefer async unlinks though ... maybe because it pushes device drivers to pay closer attention and avoid odd stuff ...
Another problem is that there is no protection against possible races involving one task unlinking an URB at the same time that another is submitting it. (That's true for asynchronous unlinks as well.) While this might seem an unlikely scenario, it certainly could happen if a completion function automatically resubmits and a disconnect() routine unluckily tries to unlink the URB just during the resubmission.
... like that. Any driver which is that confused needs work in other areas too, rest assured. Use a spinlock to test that "should I resubmit" state flag; it's easy to solve that issue safely, and all the resubmits are now explicit, hence easier to find/check/fix.
A related situation is drivers unlinking urbs that aren't actually submitted. If it's just not checking, then it's harmless sloppiness. But if the driver can't know whether that urb was submitted ... what keeps it from modifying a "busy" urb and submitting that?
Clearly this is not something to be added to the kernel until 2.7 starts up. A related welcome change would be to create separate functions for doing synchronous and asynchronous unlinks rather than relying on the URB_ASYNC_UNLINK flag.
I think an urb_begone() should be relatively doable almost instantly as a driver-specific helper. Likely every driver that tries to use synchronous unlink should be audited for disconnect and resubmit, and one mark that it's been audited (if not entirely fixed) could be using urb_begone() rather than a synchronous usb_unlink_urb().
I've come to like the idea of splitting the synch and asynch paths more completely, FWIW. The new-style HCDs won't even notice if that's done inside usbcore, but the old-style ones would have more difficulty. And I suspect 2.6 should keep old-style HCD APIs (but just deprecate them).
- Dave
------------------------------------------------------- This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
