On Tue, 9 Dec 2003, Alan Stern wrote:
> Simply stated, things that matter to a device driver can thereby be of
> interest to the core. If by providing a simple -- though not strictly
> necessary -- service the core can make device drivers easier to write and
> more obviously error-free, then the core should provide it.
In case that wasn't clear enough, consider this rhetorical question: Why
does the core contain usb_ifnum_to_if()? It's a simple routine; any
driver that needs it could easily include its own version.
Answer: It performs a useful task that would otherwise require duplicating
code. It avoids a common mistaken assumption (that interfaces are stored
in numerical order starting from 0). It makes it easier for drivers to do
something useful and do it correctly.
It's for exactly these same reasons that I think the core should
facilitate waiting for URBs to be idle.
> > Curious -- I just posted a patch that tests whether the urb is on the
> > queue. Mostly since looking at the AIO code reminded me that'd be a
> > good idea, so I updated the gadget controller drivers to do that on
> > their unlink paths and finally sent in the host-side version. That's
> > what you said we "have no way of doing", right?
>
> Yeah. Although I'm aware of the existence of the urb_list stuff, it
> hasn't really sunken in. And I notice that it is protected by a spinlock.
> Does this mean that your two states correspond formally to an URB being on
> or off the dev->urb_list?
I take that back. After thinking some more, it became clear that the
urb_list test does _not_ detect whether the URB is linked.
Part of the problem is that I don't remember what precise definition you
were using for what it means for an URB to be linked. Here's my
definition, an operational one:
An URB is linked when the low-level HC driver thinks it is.
This can be made a little clearer through the following considerations.
In any HC driver there must be two points in the code, one on the submit
path, the other on the dequeue path, presumably protected by the same
spinlock. Let's call them point A and point B. In a race between
submit_urb and unlink_urb, the result is decided by which point is reached
first. If the submit thread reaches A before the unlink thread reaches B,
then the URB will get linked and then unlinked. If the unlink thread
reaches B before the submit thread reaches A, then the unlink will fail
(because the URB wasn't linked to begin with) and the link will succeed --
so the URB will end up being linked.
I can tell you exactly where these two points are in the UHCI driver; no
doubt you know where they are in OHCI and EHCI.
In short, whether an URB is linked has nothing to do with whether it's on
the urb_list. All that matters is whether the HC driver successfully
passed through point A. If usb_unlink_urb() thinks differently then it is
buggy. (And just to prevent some kibitzer from picking on a loose end,
of course the URB stops being linked when the HC driver calls
giveback_urb.)
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.)
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.
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.
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;
No locking is needed in the driver; the core takes of it automatically.
When this sequence completes, the URB is guaranteed not to be linked and
the completion handler is guaranteed not to be running. Even if it always
resubmits. (Although it is necessary that the handler does not clear
urb->flags!) The URB can safely be reused or the driver module unloaded.
The sequence above could be bundled up into a single subroutine that would
essentially replace the current synchronous unlink_urb. It wouldn't do
the same thing, but it would do what the drivers want.
To implement this requires very minimal changes in the core and the HC
drivers. If anyone would like to see a patch, I'll work one up.
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
