On Sat, 8 Mar 2003, David Brownell wrote:
> Is there an extra "A" in the "Subject:" then? :)
Yes, sorry about that. Slip of the fingers.
> And notice that those timeouts are just reasons to abort/unlink
> requests
Indeed. I can't think offhand of any other reasons for having timeouts
for USB requests.
> > Our recent efforts have been aimed at replacing the API with a pair of
> > function calls: one to submit and one to wait-for-completion/timeout.
>
> There's also the usb_sg_*() model, friendly to the SCSI layer. Those
> timeouts are triggered by some other activity (like scsi-eh tasks),
> and the init call is more significant in the request lifecycle.
Yes, certainly. My previous email was meant to refer primarily to
usb_control/bulk_msg(), not usb_sg_*().
> > Or would it? The urb in question has to be allocated and freed somehow,
> > and either the driver or the core has to take care of this. By (2), this
> > ought to be handled by the core. So the submit routine allocates the urb
> > and passes a pointer to it back to the caller, while the
> > wait-for-completion/timeout routine frees the urb. But this leads to an
> > unavoidable race should the driver try to unlink the urb: There's no way
> > to insure that the unlink call arrives before the urb has been freed.
>
> Whatever handles cancelation (and maybe timeout) just needs to synchronize
> so they can't confuse themselves like that. Just like today.
[Just like today -- what?]
I agree about the need for synchronization. The point is that with the
only data structure available to the client being a pointer to an urb,
there _is_ no way to carry out this synchronization. Doing so would
require using one of the synchronization objects contained within the urb,
and they aren't available once the urb has been freed.
Of course, there's always the possibility of augmenting the data
structure. Something containing a pointer to an urb and a spinlock, for
example. Then you're starting to get close to the proposal I sent you
about a week ago.
> > Adding a struct completion member to struct urb is also a powerful tool.
> > For instance, consider this advantage: There will no longer be any need
> > to have a completion routine for every urb. The core can simply check,
> > and if the pointer to the completion handler is NULL then the core can
> > just call complete_all() on the struct completion member.
>
>
> Minor issue that complete_all() isn't currently exported to modules.
> That's part of why I'd been assuming complete() would be used here;
> it's also a simpler model. But complete_all() is sort-of "there to
> be used" I suppose ... :)
It's easy enough to export it. My reason for using complete_all() rather
than just complete() was that I can imagine situations where more than one
thread is waiting for the urb to finish up. For instance, the normal
driver operating thread and the khubd thread (in disconnect()).
> This is the kind of dovetailing I was noticing with respect to the unlink
> logic too. We'd been talking about ways to clean up hcd_unlink_urb()
> by adding a _pointer to_ a "struct completion" to the urb, used by the
> core+hcd for synchronous unlinks.
You know, this discussion is highly reminiscent of the Win-32 API. That
includes 3 different means of asynchronous notification: signalling a
synchronization object, calling a subroutine, and posting a message to an
application's global message queue. It seems as though the Linux USB API
is trying to move in that same general direction (Heaven forbid Linux
should resemble Windows!).
> > Putting (a) and (b) together, it's clear the complete_all() has to be
> > called from within the completion function. That makes sense; after all,
> > the driver best knows when the proper time is to awaken the waiting tasks.
>
> I don't know that I'd agree with all the assumptions you were making
> to reach that conclusion, particularly about interactions between the
> synchronous calls and the asynchronous ones.
Can you spell out more precisely which assumptions you don't like? And
also which synchronous calls you mean -- my discussion of complete_all()
being called from within a completion routine was meant to refer to an
entirely asynchronous context.
> > Another advantage of doing this is that it entirely removes the need for a
> > synchronous form of usb_unlink_urb()! Provided the driver makes sure that
> > the completion function calls comlete_all() at the proper time (or if
> > there is no completion function), the effect of a synchronous unlink can
> > be achieved just be doing:
> >
> > usb_unlink_urb(urb);
> > wait_for_completion(&urb->cmplt);
>
> That's actually something any driver can already ensure, with no change
> to usbcore, if they have a correctly synchronized completion object (or
> similar per-request state).
Yes. But there's an advantage to formalizing and providing core support
for a common but currently ad-hoc technique.
> > With just these two API additions, look at what usb_bulk_msg() turns into
> > as a series of low-level API calls (again without error checking):
> >
> > urb = usb_allocate_urb(...);
> > usb_fill_bulk_urb(urb, ...); // No completion handler
> > usb_submit_urb(urb);
> > if (io_wait_completion(&urb->cmplt, timeout) == -ETIMEDOUT) {
> > usb_unlink_urb(urb);
> > io_wait_completion(&urb->cmplt, 0);
> > }
> > /* Save urb->status, urb->actual_length */
> > usb_free_urb(urb);
>
> Make that io_wait_timeout(..., MAX_SCHEDULE_TIMEOUT) the second time
> around, remove the urb alloc/free ...
So where does the urb get allocated and freed? Is this the confusion you
mentioned above?
> ... and it starts to look like usb_start_wait_urb() in that patch
> I sent (with io_wait_timeout). usb_bulk_msg() or usb_control_msg()
> allocate and partially initialize the URB, usb_start_wait_urb() does
> the rest ... all that "middle" code already exists.
I considered your model for the scatter-gather I/O functions, where the
first call merely initializes the data structures and the urbs aren't
actually submitted until the start-wait function is called. It seemed
like unnecessary complication and overhead, at least when handling a
single control or bulk message. There was no reason not to simply start
the I/O during the first function call.
> > Here is a radical new thought. Suppose we could gain advantage (7)
> > (although not (6)) without changing the synchronous API at all! Since (6)
> > isn't really necessary, this would be among the better, if not the best,
> > of all possible worlds.
>
> I think that fixing (7) is by definition an API change ... ;)
> Not necessarily a syntax change, that's true.
But not a change to the _synchronous_ API, i.e., usb_bulk/control_msg().
> Hmm, I'm at work on a less invasive change that does much the same,
> as part of fixing bad assumptions about how endpoint state can work.
I would have been pretty surprised if everyone had said "Wow! Your
suggestion is just great -- let's convert all our drivers first thing!"
:-) But keep my idea in mind while working on your change.
Alan Stern
-------------------------------------------------------
This SF.net email is sponsored by: Etnus, makers of TotalView, The debugger
for complex code. Debugging C/C++ programs can leave you feeling lost and
disoriented. TotalView can help you find your way. Available on major UNIX
and Linux platforms. Try it free. www.etnus.com
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
