On Wed, 12 Mar 2003, David Brownell wrote:
> Alan Stern wrote:
> > 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.
> > 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.
> 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.
> 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?
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.
> > 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.
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.
> (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().
> > thread 2: (d) cancel the request
> >
> > What's to prevent (b) (d) (a) (c)? That's going to be just as bad.
>
> 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.
Alan Stern
-------------------------------------------------------
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
