On Wed, 10 Dec 2003, David Brownell wrote: > Alan Stern wrote: > > Consider this proposal for an addition (not a change!) to the existing > > API. Let's define 3 new bits in urb->flags: URB_LINKED, URB_COMPLETING, > > and URB_INVALID (maybe someone can come up with a better name than that > > last one). URB_LINKED will be set by the HCD precisely at the time the > > URB is linked. URB_COMPLETING will be set by the core during the time the > > URB's completion handler is running. (And the core will carefully set > > URB_COMPLETING in giveback_urb before or at exactly the same time as > > clearing URB_LINKED.) > > I won't like such new mode bits any more than the current > current URB_ASYNC_UNLINK, I have to say ... :(
But you put a FIXME comment in hcd.c (or was it someone else?) saying "use better explicit urb state". The first two bits are just that. I agree that maybe urb->flags isn't the best place to store them... but that's a side issue. The URB_INVALID flag is no worse than URB_ASYNC_UNLINK. At least it's a real flag and not a different entry point in disguise. > > Then usb_wait_for_urb() becomes a simple wait loop, polling for > > > > (urb->flags & (URB_LINKED | URB_COMPLETING)) == 0 > > > > Very simple, easily seen to be precise and correct. > > But then, so is retrying synchronous usb_unlink_urb() when > it returns -EBUSY. Usually just once, unless the device > driver's resubmit logic isn't properly disabled. Simple I will grant you, but not correct. If the completion handler is already running, usb_unlink_urb() doesn't return -EBUSY. > > The third flag is to help drivers avoid the need for locking > > synchronization to prevent resubmission. The idea is very simple: If the > > URB_INVALID flag is set, a low-level HC driver will refuse to link the > > URB. This test _must_ be made within the same spinlock-protected region > > that contains point A, so that it is sufficiently atomic with respect to > > submission and unlinking. > > The usb_sg_request model solves this class of problem by forcing > an explicit re-initialization before resubmission. More importantly, it doesn't use completion handlers. I don't see that it would make any significant difference if resubmitted URBs were required to be re-initialized first. > > With these mechanisms available, a driver can safely stop an URB from > > being resubmitted and wait for it to be idle as follows: > > > > urb->flags |= URB_INVALID; > > usb_unlink_urb(&urb); // Asynchronous unlink > > usb_wait_for_urb(&urb); > > urb->flags &= ~URB_INVALID; > > And also code in the resubmit path too, yes? No! What I wrote above is correct as it stands. > Robust drivers > would need to change their resubmit logic, you're creating > new fault paths. Again, no. The resubmit logic could even be simplified; there would no longer be any need to check whether or not resubmission should take place. It could always be attempted; when not appropriate it would simply fail. > I guess I don't see why usbcore should be involved there. > Drivers without some "I'm shutting down" state flag can > easily add one and use it to control whether they resubmit; It's not so simple. At the minimum a spinlock is needed as well. Put it the other way: Since the core can easily add this mechanism, why should drivers have to get involved with extra locking? > and it'll help with other situations that an URB_INVALID > flag can't address (since it's URB-wide, not device-wide). What other situations? > What drivers are suffering now from that "resubmits during > driver shutdown" syndrome ... do you have any in mind, or > is this concern not that concrete? It'd be simpler just > to fix those drivers; they need changes in any case. In fact I do have an example in mind. Just recently Greg started catching up on some long-delayed patches. One of them was a change I made to the hub driver, adding precisely the type of synchronization needed to insure that the no-resubmit logic worked correctly. It was while pondering over how I could have avoided that extra work that I came up with these ideas. If these changes had been in place, fixing hub.c would have been a lot easier (change one occurrence of usb_unlink_urb() to the 4-line function above, nothing else required -- maybe even Greg wouldn't have asked me to split it out into a separate patch!). > Of course, if these drivers weren't trying to mix up the synchronous > and asynchronous APIs, AND have completion handlers which inappropriately > resubmit, they'd not have any problems either. There _is_ no synchronous API apart from synchronous usb_unlink_urb. So if you're going to use it at all, you can hardly avoid mixing it up with the asynchronous API (i.e., everything else). Yes, if all drivers were written correctly we wouldn't have these problems. We probably also wouldn't have synchronous unlink_urb; what it does just isn't useful for a driver. Alan Stern ------------------------------------------------------- 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
