On Tue, 2 Mar 2004, David Brownell wrote:
> Alan Stern wrote:
> > "Unlink during submit" means that during the brief interval after
> > hcd_submit_urb() has released the hcd_data_lock and before the HCD's
> > enqueue() method has committed to accepting the URB, hcd_unlink_urb()
> > manages to run. There's a race between the HCD enqueue() and dequeue()
> > methods, and with "unlink during submit" dequeue() wins.
>
> Actually, there's no race there.
Sure there is. Maybe you mean that it doesn't matter, but there's
definitely a race.
> The dequeue() method just kicks
> the HCD; as soon as hcd_unlink_urb() drops the hcd_data_lock,
> then usb_unlink_urb() is guaranteed to succeed. Which means that
> the HCD _will_ giveback() ... that's what success means.
Clearly this is a gray area in the API.
Suppose usb_submit_urb() fails in the HCD for some legitimate reason, such
as lack of memory. Then giveback certainly should not be called.
Generalizing, if usb_submit_urb() fails because of an unlink request
before the URB could be queued, again giveback shouldn't be called. The
prime guarantee provided by the API is that it will return the URB in one
of two ways; either submit fails with an error code or else the completion
routine is eventually called. Not both.
The real question is: If usb_unlink_urb() is called while usb_submit_urb()
is still running, and if usb_submit_urb() ultimately returns an error,
does it then make sense for usb_unlink_urb() to return success? There
are good arguments for both answers. Yes it should return success,
because it succeeded in getting the URB away from the HCD. No it
shouldn't, because you can't succeed in unlinking an URB that wasn't
linked to begin with. (Of course, it really was linked for a brief period
while hcd_submit_urb() was running, but it was unlinked again at the end
of that routine. As far as usb_submit_urb() is concerned, the URB wasn't
successfully linked.)
I wouldn't mind if a special return code for usb_unlink_urb() was invented
to cover this situation, like -ENOMSG.
> > The dequeue() method doesn't have to queue the URB for giveback because
> > the enqueue is about to fail. (Making that failure happen is important,
> > though!) All the enqueue() method has to do is return an error code.
>
> No, that's why I highlighted this. Since usb_unlink_urb() succeeded
> in marking the urb as having been unlinked, the driver is expecting
> to get a completion callback.
>
> But returning an error code means that there will NOT be a completion
> callback ... as there would if this had been detected even a slight
> bit later, if dequeue() had been called after the request had been
> passed off to the hardware.
Changing the return code from usb_unlink_urb() would answer this
objection.
I suppose the alternate approach, the one you have in mind, is to have the
enqueue call succeed but mark the URB for immediate failure (and not queue
it into the hardware schedule). This seems like a lot of extra work when
it would suffice to do nothing at all.
> > In fact, I don't see why your patch needed to acquire urb->lock. As in
> > the UHCI driver, the enqueue() and dequeue() methods guarantee mutual
> > exclusion through the use of ohci->lock. Once enqueue() owns that lock it
>
> They guarantee mutual exclusion at the level of the HCD.
>
> But urb->status is under control of urb->lock, so whenever it's
> tested or modified between submit() and complete(), that lock
> must be held.
>
> You maybe right that in this case it's OK to break that locking
> policy, but I prefer not to do that. If for no other reason than
> to prevent scheduling this urb to the hardware... :)
In general it's true that when a variable is protected by a lock, it's not
necessary to acquire the lock before performing a simple read (or test).
There's no advantage to
spin_lock(&lock);
x = protected_var;
spin_unlock(&lock);
over
x = protected_var;
so long as the read operation is atomic (x won't get some mixed-up
combination of values if protected_var is written at the same time as the
read occurs). The same sequence of events would happen during execution
of either code block.
Alan Stern
-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
