On Wed, 29 Jan 2003, David Brownell wrote:
> > I don't think I understand your objection. Are you saying that if a
> > driver calls usb_unlink_urb() while the urb's completion handler is
> > running, then back when the handler was originally called the value of
> > urb->status wasn't -ECONNRESET or -ENOENT?
>
> When a task (or even IRQ handler) on one CPU tries to unlink, the request
> may be in MANY stages of completing on another CPU. That shouldn't matter
> at all to whatever is unlinking ... in your patch, it _did_ matter.
Hmm... I'm beginning to suspect that you misread my patch (or possibly
that I miswrote it). Yes, an urb can be in any of several different
stages of completing. For the sake of argument, let's define some of
these stages:
0: Starts when the hardware finishes its USB transaction and
signals an interrupt.
1: Starts when the interrupt handler changes urb->status from
-EINPROGRESS to something else.
2: Starts when usb_hcd_giveback_urb() calls the completion handler
(or in my patch, locks urb->handler_lock).
After the handler returns (or urb->handler_lock is released), the
completion process is over and the urb is idle, assuming it wasn't
resubmitted.
As far as I'm concerned (i.e., looking at things strictly from the
software point of view), during stage 0 the urb is essentially still
active; the completion process has not yet begun. Agreed, an unlink call
down to the low-level driver at this time might produce an error code, but
that call would still have to be made. My patch handles this correctly;
in fact, the existing code is unchanged.
Stage 1. I discuss this below.
Stage 2. The problems of handling this correctly are so messy, especially
when you consider the possibility that the completion handler might have
resubmitted the urb, that I basically punted. The patch makes
hcd_unlink_urb() spin until stage 2 is over, thus essentially pretending
that the unlink call didn't arrive during stage 2 after all. Since the
handler must be running on a different cpu, there's no way the caller can
tell exactly what the relative order of the two events (call to unlink vs.
handler returning) actually was, without using some very dubious
synchronization mechanism. Is this what you object to?
> > That's clearly unavoidable and
> > hardly a violation of the API spec; after all, the instance of the urb for
> > which the handler is running _wasn't_ cancelled (at least, not by this
> > call to usb_unlink_urb()).
>
> But you're returning a success code, reporting that it was.
No, I'm not. Or at least, I don't intend to.
We're talking about unlinking during stage 1 here. The code sees that
urb->status != -EINPROGRESS, so it returns -EBUSY to its caller
(indicating that the urb was already in the process of finishing up) --
that's not a success code -- and it leaves urb->status alone (since the
urb didn't finish as a result of the call to usb_unlink_urb()).
> Ignoring
> the issue of whether the unlink request is synchronous, discarding the
> current -EBUSY return code means that drivers written to block until
> they get an -ECONNRESET or -ENOENT completion would never wake up.
That's why I think you are misreading the patch. It doesn't discard the
current -EBUSY return code.
> > Maybe it doesn't really need solving. But as things stand, the API spec
> > probably could use some revision. It says:
> >
> > * When the URB_ASYNC_UNLINK transfer flag for the URB is clear, this
> > * request is synchronous. Success is indicated by returning zero,
> > * at which time the urb will have been unlinked,
> > * and the completion function will see status -ENOENT.
> >
> > I note that this makes no guarantees about when the completion function
> > will run with respect to the return from usb_unlink_urb(); it only says
> > that the status will be -ENOENT and that the urb will have been unlinked.
>
> Maybe it should have said completion function "will have seen", but
> I don't naturally gravitate to the future perfect tense! :)
>
> But you seemed to be concerned about when the completion function
> finishes, while all that does is talk about when it starts.
It doesn't even do that. It says nothing about when the completion
function will run or will return; it only says that the function will see
status -ENOENT. Yes, I am concerned about when the completion function
finishes, since that's what matters to a driver. If we can't guarantee
that the completion function will have finished when a synchronous unlink
returns (with a success code), then there's no reason for a driver _ever_
to use a synchronous unlink.
> With
> HCDs using the "hcd.c" code we know that answer. I'd not object
> to requiring all HCDs to do that, since I'm not involved with any
> of the ones that would need to change.
I'm not familiar with that part of the core. Can you tell me what other
ones would need to change, and where the appropriate source files are?
Alan Stern
-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
