On Tue, 9 Dec 2003, David Brownell wrote:
> There is no such thing as a reentrant call to giveback_urb(); that'd
> mean that somehow the same URB got queued twice. If nothing else, the
> data structures don't support such a notion ... only one queue! :)
Oliver has now convinced me of that. But the reasons are fairly subtle!
Consider dummy_hcd for example. There's no obvious reason preventing it
from calling giveback_urb directly from within the urb_dequeue routine.
(There's just a comment saying it doesn't need to.) But if it did, then
we could have:
URB submitted
URB completes normally -> call giveback_urb
giveback_urb calls completion handler
completion handler resubmits
another task on a different processor unlinks
the resubmitted URB
urb_dequeue immediately calls giveback_urb
Voila!
> > The problem with this simple two-state model is that it does not
> > facilitate an implementation of synchronous unlinking that waits until the
> > URB is idle. Of course, for anyone who believes that synchronous unlink
> > only means waiting until the completion handler returns, this doesn't
> > matter. But for device drivers that want to reuse an URB it does matter.
>
> But the "urb is idle" state is clearly a driver policy. All usbcore
> can know is whether it's queued for the endpoint. When an urb isn't
> queued, it doesn't (shouldn't!) care why ... there could be dozens
> of reasons why it's not queued (on an un-imaginative day). I can't
> see any reason why usbcore should even care about one of them.
There's one reason for the URB not being idle even though it's not queued
that the core _does_ know about (whether it cares or not): a completion
handler could be running. That lack in the two-state model was part of
the reason the awkward "splice" technique had to be used.
> You've talked a lot about this resubmission stuff, and I've never
> understood why you think this should matter to anyone except the
> device driver. Maybe you should explain that to me ... :)
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.
> > Anyway, the existing code doesn't follow this model exactly:
> > usb_unlink_urb checks whether urb->status is -EINPROGRESS before calling
> > the HC driver. According to the model it shouldn't do that; instead it
> > should check whether the URB is submitted (active). But we currently have
> > no way of doing this, since still-linked URBs can have status other than
> > -EINPROGRESS and non-linked URBs can have status equal to -EINPROGRESS.
>
> 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?
> The -EINPROGRESS test is hard to avoid, unless you change the semantics
> of unlink to discard status, either the "real completion" status or
> the "request canceled" status. Discarding either of those makes for
> trouble. Regardless, that's still a "submitted" state ... it's just
> that unlinks can't be done without trashing real completion status.
I don't follow what you're saying here. But since it doesn't matter
for the point of this thread, let's just pass over it.
> > Exactly -- no more. It doesn't protect the submission transition.
>
> That's because the caller/submitter guarantees the urb isn't in use.
> By definition, there's nothing to protect, no need to lock anything.
I was thinking more of the problems involved in trying to make sure that
an unlinked URB isn't resubmitted. Given the ability to disable
endpoints, this can be done quite easily with no locking required. Maybe
usb_disable_endpoint should be EXPORTed; I can imagine it might be handy.
(Although then we'd have to export usb_enable_endpoint also, and the
toggling semantics wouldn't be correct...)
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
