On Tue, 16 Sep 2003, David Brownell wrote:
> Alan Stern wrote:
> >
> > I've been thinking that it would be good to allow drivers to call
> > usb_set_configuration() and usb_reset_device() during probe(). Doing so
> > would require breaking each operation up into two parts:
>
> I've been thinking differently for the "no thanks, but this driver
> should work in some other config" probe() case. For cdc-acm,
> which is the only problem I know about today.
>
> I don't like any driver to be making these policy decisions as
> absolutes. Some other driver, not yet (mod)probed, might have better
> luck binding to this interface (or might already be bound to another
> interface in this config!) ... the config may be a lot better than
> this driver thinks it is, with just its current local perspective!
Of course, the other possibility is that the driver might be a
special-purpose one, written just for that device, and it _knows_ which is
the correct configuration to use. The core ought to allow for that kind
of arrangement.
(What I originally had in mind was more to do with usb_reset_device(),
however the issues are very similar.)
> The notion I've recently been tossing around is this. Usbcore can
> easily notice that that after N seconds since usb_set_configuration():
>
> - It's still true that none of the interfaces has a driver;
>
> - There's still a config we haven't tried, and it even fits within
> the power budget available for that port;
>
> - So make some task call usb_set_configuration(that-config), to let
> the usb drivers try binding to that one.
>
> No new API needed; user/admin control is possible with sysfs
> (write bConfigurationValue); and it's simple enough that it won't
> go haywire. In particular, it stops after bNumConfigurations
> attempts, or when a semi-functional configuration has been found.
>
> I suspect that kind of solution would get rid of the cdc-acm issue,
> as well as solving the corresponding issue for the CDC Ethernet
> class support...
That would probably work, although it seems a little elaborate. Maybe it
would be best simply to leave all decisions of this sort to a user
process, like a hotplug script. Using the sysfs interface, your scheme
could be carried out that way.
> > the immediate part, done at the time of the call, which unbinds
> > and destroys all the interfaces _except_ the caller's, and sends the
> > actual SET_CONFIGURATION or PORT_RESET message;
>
> I don't like the notion of having such an immediate action for config
> issues, for the reasons sketched above: we don't know that this config
> is really so bad. Device drivers only have local knowledge, not global.
After thinking some more, it seems clear that the immediate part would
have to unbind the interfaces but would not be able to destroy them, since
during a probe() the driver-model lock would already be held.
> > the deferred part, running in another thread (using
> > schedule_work(), for example) would unbind and destroy any remaining
> > interfaces and finish whatever needed to be done: choose a configuration,
> > probe interfaces, or perhaps destroy the struct usb_device.
>
> So whatever task runs would look at the device and do the right
> thing. The "choose a config" steps could be as I sketched above,
> and of course the updated usb_set_configuration() is in charge
> of probing (as it must be).
That's an implementation issue. usb_set_configuration() might end up
being the function that does the immediate part of the operation, and then
the function running the deferred part would be in charge of probing.
On the other hand, if the configuration needs to be set or changed in a
context where the driver-model lock isn't already held (so not during a
probe), there's no need to separate the immediate and deferred parts.
However, we still might want the probing to take place in a different
process from the one that called usb_set_configuration(), so perhaps
keeping the immediate-vs.-deferred distinction would still be a good thing
to do.
> Is that "destroy the device" bit needed for the "probe()-does-DFU"
> case, where a re-enumeration gets forced? I recall that coming
> up, it's something of a new case (to have working) which I keep
> thinking will want a new api call.
That's one case where it's needed. Another is if the device gets
disconnected during a probe. The deferred code should realize what has
happened and not try to enumerate or probe a defunct device.
> > The deferred part could also be invoked by the hub driver to handle
> > connect and disconnect events. This would help take some of the load off
> > the khubd thread, which right now is trying to do too much.
>
> As we've discussed. Yes, I think the disconnect() bit would be easy
> enough to snip off in that way -- and we'd get better DFU support
> from it, right away.
I wonder if using schedule_work() is the right thing to do here -- doesn't
that just end up taking khubd's problems and dumping them onto keventd?
Maybe we should spawn a new thread to handle each device as it comes up.
> The connect processing -- going from ADDRESS to CONFIGURED state
> the first time around -- could also be split out. In fact it'd be
> easy to make it use that "try the next config" logic I sketched...
Yes. In the back of my mind, there's been the idea that some subroutine
would look like this:
switch (usb_dev->state) {
case DISCONNECTED:
destroy the device;
break;
case CONNECTED:
usb_reset_port();
usb_choose_address();
usb_set_address();
usb_get_config();
// Fall through
case ADDRESS:
cf = choose_configuration();
usb_set_configuration(cf);
// Fall through
case CONFIGURED;
probe interfaces;
}
Maybe parts of this belong in the khubd thread, but something along these
lines would be elegant.
> > Implementing this would also mean changing the API, since callers of
> > usb_set_configuration() and usb_reset_device() would have to pass in a
> > pointer to the interface they drive. That shouldn't pose much of a
> > problem, since neither of these routines is called from very many places.
>
> I can see that for the current usb_reset_device() functionality,
> but that's it.
>
> The whole point of _changing_ a configuration is that it's going
> to change -- so all "current" interfaces become invalid! Remember
> that almost all the previous users of usb_set_configuration() are
> now using usb_reset_configuration(), which has the right kind of
> semantics (no config changes except to altsetting 0).
The considerations for usb_reset_device() should be pretty much the same
as for usb_set_configuration(). If the reset succeeds and the descriptors
don't change and everything else works, then nobody needs to be unbound or
probed. But if that doesn't happen, the situation is much the same as if
the configuration was deliberately changed.
The real issue is ownership of usb_dev->serialize. set_config() -- and
reset_device() when something goes wrong -- must possess the mutex. But
if they're called during probe() then the thread they belong to already
owns it; they don't need to acquire it again. So there has to be some way
for the caller to say "Don't try to grab the serialize mutex because you
already own it somewhere earlier in the call stack". Yes it's awkward,
but I don't see how else to do it.
One way around this problem is simply to outlaw calling either routine
from within a probe. But I think that may be going too far. For example,
usb-storage uses usb_reset_device() as a last-ditch error recovery method,
and it doesn't distinguish whether the error occurred during probe or
during normal operation.
Alan Stern
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
