The major purpose of adding the facility to cancel synchronous messages is
to allow drivers to unlink the corresponding urbs during disconnect(). Okay, so let's keep the existing API pretty much as it is, but add an
additional argument to usb_bulk/control_msg(). This extra argument will
be something like a list_head; the driver can store the structure in its
per-device data.
I think that's not really necessary since usbcore has its own per-device state. If the goal is to to simplify driver tasks, no need to give them new requirements here.
I only put it in the driver because strictly speaking, this needs to be stored as per-interface state rather than per-device. If there's no problem with the core maintaining per-interface data, then fine. It would still require an additional argument to usb_bulk/control_msg(), to specify the interface.
Except for control messages, urb->pipe easily lets you track which endpoint and hence which interface the request is for.
Then we just have to
add a function that can be called during disconnect() to cancel all the
messages on the list, say usb_unlink_msgs(). That solves the problem in one fell swoop, while adding a minimum of additional complication.
I like the idea of a usb_unlink_all() primitive for several reasons, mostly because the current usb_unlink_one() style primitive is very complex: it has way too many strange and error-prone cases. When there are N requests queued, there are way more than N sequences in which they can be unlinked, some if which are a lot more demanding of the HCD (error paths etc).
It seems to me that an HC driver would have to implement usb_unlink_all() as a series of individual unlinks, because the "_all" refers to all urbs
for a particular interface, not all urbs for a particular device (or a
particular bus, which would be _particularly_ :-) easy to implement). So
it would be prone to just as many difficulties as usb_unlink_one(), except
perhaps that some of the locking would only have to be done once.
Actually the relevant primitive is "unlink all the requests to this endpoint"; they're owned by the hardware, and some of them are likely partially completed. That's the primitive which is generally useful, in the "driver unbind" cases (not necessarily just disconnect) as well as for config changes.
And as near as I can tell, nobody has ever needed usb_unlink_one() like we have today, except as a way to build usb_unlink_all().
If you mean that nobody has ever needed to cancel just a single synchronous message except as part of cancelling all of them for a disconnect, I agree.
Yes. Same is true when the driver uses asynchronous models too.
And yes, that patch I've got somewhere pending (others in the queue ahead of it) basically makes usbcore do the equivalent of unlink_all, ("all requests on this interface") before disconnect().
How does your patch know which interface a request is intended for?
Looks at the endpoint addressing information.
Also, perhaps it would be better to unlink them _after_ disconnect(). That way the driver won't be tempted to try to re-issue the requests when they complete with an error.
That last patch of mine makes submits fail ASAP, even before disconnect() is called ... so "reissue" can't happen, at least in the device disconnect code paths. Reconfig has never worked right AFAICT, needs work still.
Unlinking them _before_ is likely to make some drivers unhappy, but this is a case where I think the cleaner solution is to be preferred ... fewer ways that things can go haywire is Good.
However, it's not clear to me that your approach solves this problem either. Your approach is:
thread 1: (b) create/export usb_sg_request (a) initialize request (c) submit/wait
Actually more like (a) create/init, then (b) export. That's the design in any case, drivers can always add synchronization bugs.
I want to separate out creation from initialization. A struct
usb_sg_request is _created_ when its stack frame is entered (if it is a
stack-based variable) or by kmalloc (if it is dynamically allocated). Either way, it is potentially available for access by other threads as
soon as it is created. As you say, the driver is responsible for making sure this doesn't happen incorrectly.
Sure: (a1) create, (a2) init, (b) export. Physically it's possible to do (a1) (b) (a2), but that'd be a bug.
But the request is not _initialized_ until the call to usb_sg_init() returns. Creation and initialization are separate steps. Now if usb_sg_init() had done the allocation itself and returned a pointer to the caller, then I would agree that create/init was one step.
Agreed.
(And come to think of it, maybe that's what usb-storage is doing. I noticed it was pre-allocating the request in shared state, and don't remember if there was synchronization when request becomes cancelable.)
I don't think there are any synchronization errors in usb-storage. Cancellation of the shared usb_sg_request is synchronized by means of a
bit flag, using test_and_clear_bit(). The flag isn't set until
usb_sg_init() returns successfully, and it is always cleared after
usb_sg_wait().
Sounds right then -- false alarm, I don't expect to recall that without looking at the code!
What's to prevent it is drivers following the basic rule that un-initialized data structures must NEVER be exported to other threads ... never doing (b) export until (a) create/init is done.
Just so. The same basic rule prevents drivers from issuing cancellations for uninitialized data structures under my scheme as well, even in the presence of SMP/preemption. Which, although it has been snipped out of our last email interchange, is what you were first concerned about.
OK.
- Dave
-------------------------------------------------------
This SF.net email is sponsored by:Crypto Challenge is now open! Get cracking and register here for some mind boggling fun and the chance of winning an Apple iPod:
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0031en
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
