On Wed, 10 Dec 2003, David Brownell wrote:
> > More importantly, usb_sg_request doesn't have completion handlers. It's
> > not really asynchronous, in the sense that drivers eventually have to wait
> > for a request to complete.
>
> It's always been an explicitly synchronous model -- no completion
> handler for app code, just whatever happens after usb_sg_wait().
> It resembles usb_bulk_msg() except that there's actually a way to
> cancel the request: an async unlink.
>
> Usbcore can't add an exactly corresponding usb_urb_wait() since
> it doesn't have a way to cancel urbs before they're submitted
> (to prevent submit from succeeding in some exotic races).
Just that sort of mechanism was one of the things I am proposing -- and
below you propose something like it too!
> > This is an important difference and it's worth spending some time on it.
> > Consider what happens when the two definitions disagree. Suppose an URB
> > is on the submit path on CPU-1 and has reached a point where it's on the
> > usbcore list but the HC driver hasn't yet accepted it. What happens now
> > if CPU-2 calls usb_unlink_urb for that URB?
>
> Not what you outline:
>
> > Answer: usbcore removes it from the list and tells the HC driver to
> > dequeue it. The HC driver refuses to do so (because it hasn't been queued
> > yet). Regardless, the core returns from the unlink call with the URB
> > removed from its list.
>
> Actually, no ... usbcore marks the URB as pending-completion by
> urb->status != -EINPROGRESS (a point you intentionally skipped
> over yesterday), and then kicks the HCD. It's still linked.
> Even if the HCD ignored kicks given before requests are linked
> to the hardware, the failure you describe wouldn't happen.
You're right, the failure mode would be different. I don't know about
OHCI and EHCI, but the UHCI driver does ignore kicks given before requests
are queued to the hardware. IMO that's a hole in the URB model and it
ought to be plugged. It means that usb_unlink_urb would return
successfully (-EINPROGRESS) but the URB would remain queued. Fortunately
it's easy to fix.
> I like the idea of a "nuke all the urbs!" request. It's easy
> do; we have endpoint_disable() already. Just:
>
> - In usb_device, add 32 bits of "is it enabled" flags
> - Make endpoint_disable() and usb_submit_urb() use those flags
> - Export some is_endpoint_enabled() primitive
> - Have usb_endpoint_disable() do the obvious
> - Define usb_endpoint_reenable() likewise
> - ... anything else? (urb_begone wouldn't be needed!!)
>
> I don't actually recall any case where unlink _one_ of the urbs
> on a given endpoint was really the answer ... "all" is generally
> the answer. I can think of several places we could remove code,
> given that primitive...
It sounds good. I would change it somewhat so that the new disable
mechanism wouldn't affect the hardware state at all; it would simply
prevent the future submission of URBs for that endpoint while leaving
ones that are already linked alone. There remains the problem that you
would never want to disable ep0 like this, but device drivers should
never resubmit URBs to ep0 anyway.
> I'd actually call that "tedious" locking, not tricky. (Some
> would say all locks are tricky. Bah!) And I think the current
> "endpoint disable" semantics aren't quite right there -- it'd
> likely be better if it didn't invalidate hardware state, surely
> some hardware will complain if we do that needlessly.
This new "endpoint disable" could help avoid the need for extra locking in
drivers, tricky, tedious, or otherwise. Does making it software-only
interfere with any of the other uses you had in mind?
During this thread I've been harping on three separate problems:
1. Adding a core mechanism to aid drivers in preventing
resubmissions without additional locking. The new endpoint-disable
routine should take care of that (other than in the exceptional case where
a completion handler resubmits an URB for a different endpoint!).
2. Proper URB state tracking so that everyone knows unambiguously
when it is or isn't linked. Fixing the hole mentioned above would take
care of this. It would only be necessary to add a test in the spinlock
protected part of the submit pathway in UHCI (maybe OHCI and EHCI as well)
for
urb->status == -EINPROGRESS
and to fail the submission if the test fails. With that done, the test
for whether an URB is linked is very simple: !list_empty(&urb->urb_list).
So this will no longer be a problem for me.
3. Adding a core mechanism to make it easy for device drivers to
tell that their completion handlers aren't running and won't be called.
Of course, drivers could already do this themselves. They would need to
create an atomic_t counter, increment it before every submit_urb, and
decrement it after every unsuccessful submission and at the end of every
completion handler. When the counter is 0, no URBS are linked and no
completion handlers are running.
Since so many drivers would need to do this, why not provide something
like it in the core? Shucks, maybe we don't need a new counter at all; we
could use the existing urb->count. Provided the driver hasn't called
usb_free_urb (or taken extra references with usb_get_urb), when urb->count
is 1 the URB is unlinked and not in a completion handler. We just have to
make sure that the submit test for software-endpoint-disabled is made
_after_ the call to usb_get_urb in hcd_submit_urb. How does that sound?
Put these solutions all together, and we end up with this form of
synchronous unlinking:
new software-only endpoint disable
usb_unlink_urb // Asynchronous!
while (atomic_read(&urb->count) > 1)
wait_ms(10);
new software-only endpoint enable
No changes are needed to the resubmit pathways and only a single function
call has to be altered!
> Yeah ... maybe we could wrap up this email thread that way.
> That'd make a lot of us happier!
Me too!
Alan Stern
P.S.: Oliver:
In a flash of deja vu, I recalled a previous occasion when we discussed
whether giveback_urb or completion handlers could be called reentrantly.
You brought up the fact that IRQ handlers aren't reentrant, which would
prevent it from happening. My response was to propose a bizarre but not
impossible scenario where a completion handler resubmits its URB, but to a
different device attached to a different controller using a different IRQ
channel!
I don't think we have to worry about this happening in practice :-)
-------------------------------------------------------
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
