David and Greg:
Stepping back and trying to look at the bigger picture led to the following thoughts regarding the synchronous API.
Is there an extra "A" in the "Subject:" then? :)
First of all, what's the reason for including usb_control_msg() and usb_bulk_msg() in the API at all? Clearly, it's to make things easier for device drivers, in a number of ways:
There's some history too. And a "synchronous unlink" mode. (Yes: "modes are evil", as they say.)
(5). It provides timeouts, which are not so easy to program using the lower-level API calls.
And notice that those timeouts are just reasons to abort/unlink requests, while as you noted:
There is basically just one disadvantage: you can't abort or unlink the transfer, either
... except implicitly, by timeout.
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.
David's last proposal was that this handle could simply be an urb (or more
properly, a pointer to an urb). If a struct completion member were added into struct urb then all the required facilities would be present right there, which would make things very simple.
It was more of a "what happens if we try that approach" thought, actually. Lots of issues interact here.
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.
Now consider what happens when we add the following enhancements to the low-level API...
David's io_wait_completion() is a good thing. It belongs in the kernel proper, of course, but we are better off having it available in any form.
Actually io_wait_timeout() ... but I'm glad you agree that it's forward motion and useful cleanup. Assuming that one minor bug gets fixed. (Somewhere along the line, the urb stopped getting freed.)
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 ... :)
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.
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.
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).
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 ...
... 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.
This still has the problem (7) you noted, since nobody has access to the completion object used to synchronize completions.
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.
Unfortunately, to do this requires a far-reaching change. It probably should have been done earlier (for all I know, it was proposed but nobody thought it was a good idea or wanted to do it). Here's the idea: Change the
struct usb_device *dev;
member of struct urb into
struct usb_interface *intf;
Yes, far-reaching. All HCDs and drivers would change. And as you noted, there's a conceptual issue for enumeration/configuration control requests, which are made when no interfaces are valid.
The main point of the change is that drivers would not need to be responsible for cancelling all the transfers when their disconnect() routine is called -- the core would be able to do it for them. Since urbs would now be marked as belonging to a particular interface, it would be easy for the HC driver code to find and unlink all urbs attached to a given interface.
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.
- Dave
-------------------------------------------------------
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
