David: I got three messages from you all at once on this topic; I'll try to reply to them all together.
On Mon, 2 Jun 2003, David Brownell wrote: > I'm not really clear how usb_device_altered() would really > need to be different from usb_device_reset() ... except > > (a) reset() tries to preserve device address. Why bother? The address doesn't matter. > (b) reset() tries to preserve driver bindings -- unless the > descriptors happened to change, when it clearly won't. > Why shouldn't altered() do that too, in the exceptional > case that the descriptors _didn't_ change? reset() checks only that the _device_ descriptor didn't change -- and under my proposal even that wouldn't be necessary. altered() could preserve bindings if possible, but it would easier (and simpler for the drivers too) if it didn't. > (c) reset() tries to be synchronous ... but implementing > it that way causes problems, like adding a bad lock > visibility to the API. Why shouldn't it be async, > with maybe a completion callback? As far as I'm concerned, making reset() use a callback would be just fine. > In short, I don't see value in having two API calls. The real issue I'm trying to address here is that there are two logically different tasks, and they are most naturally handled by two API calls. One task (reset) is for use by drivers during error recovery: reset the device back to a known good state. The other task (altered) is for updating the core's state when the device has undergone some significant change. In principle, these two tasks could be performed by the same function. It would reset the device, then re-read the descriptors and check for changes. In the simplest form, it would unbind all the drivers if the descriptors changed at all but otherwise leave the bindings alone. In a more complicated form, it would retain the bindings for interfaces that are unchanged and remove the others -- although to me that sounds like a lot more effort than it's worth and maybe not even completely well defined. In actual use, I expect the reset() task to be performed now and then by drivers whenever they need it for error recovery. I expect the altered() task to be carried out under highly formalized circumstances, like downloading firmware during a probe(). The driver always knows which of the two it wants, so why not provide that information to the core? Furthermore, the overhead involved in re-reading the descriptors and checking for changes is totally unnecessary for the reset() task. > Other than that synchony issue, I suspect that making the > hub driver just reuse dev->children[port-1], instead of > reallocation, would simplify a number of problems: both > types of code can reuse the standard "used every day" > enumeration logic. (And both should use the standard > "device is gone" disconnect logic, "used every day", in > some mode, as both you and Oliver have noted in other > contexts.) I must be misunderstanding what you mean here. Line 884 of hub.c (in usb_hub_port_connect_change()) is: hub->children[port] = dev; So it looks like the code already reuses the appropriate slot in the children[] array. > There aren't so many Linux drivers that load firmware after > all. We can arrange to support their state transitions > explicitly. For example: > > - EZ-USB device "renumeration" has a final device-initiated > reset ... the hub driver will always notice a USB_STATE_* > transition from ADDRESS to POWERED even in the absense > of a usb_device_altered() call. That's part of why it's > so easy for fxload to do it from userspace. How does the device initiate a port reset? I thought that could only happen as a result of the host setting the PORT_RESET feature. > - The "USB Device Firmware Update" (DFU) spec has a final > host-initated reset ... enter DFU configuration, load > firmware, issue DFU_DETACH, then host resets CONFIGURED > back to POWERED and the hub driver takes over. Likewise, > the usb_device_altered() call is not needed. > > - I recall mention of a handful of purely proprietary > models too ... but the ones I remember also transitioned > to POWERED. Which one of those wants usb_device_altered()? Under my scheme they all do, since usb_device_reset() won't re-read the descriptors. > The basic model to use for firmware loading is that the original > device disappears. That's right. > Aarg -- incorrect data could oops. Scrub out state in all cases. > If that means the driver bugs must be fixed faster -- fine. Seems to me an oops would be a pretty big inducement to fix driver bugs :-) But I'm basically in agreement here. > > On a different tack... The operation of setting a new configuration will > > need to be handled by a separate thread (separate from the calling device > > driver, I mean), just like usb_device_altered(). Fitting a proper API for > > this may require some effort; I notice that several drivers already call > > usb_set_configuration() synchronously. Some set the config to its > > already-existing value, others blindly set it to 1. This will have to be > > examined closely and in some cases changed. Trying to set a new value > > synchronously will almost certainly result in deadlock. Even trying to > > reset the current value, which normally should be innocuous, might cause > > deadlock if the call failed. > > There are few drivers that set configurations now. 'cd drivers/usb ; grep usb_set_configuration */*.c' provides this list (after filtering out core routines and comments): class/cdc-acm.c, media/dabusb.c, media/stv680.c, misc/tiglusb.c, misc/tiglusb.c, net/cdc-ether.c, serial/empeg.c, serial/ipaq.c, serial/visor.c, storage/usb.c More than a few, I think. > Near as I can tell, that dates from an era when usbcore didn't; > and anyway, usb_set_configuration() has only recently begun > to behave correctly outside of "device not yet used, and it's > already in that configuration" stages of configuration (where > the fact that usbcore didn't reset all the relevant device > state couldn't matter). I would argue that usb_set_configuration() doesn't yet behave properly outside the stage you mention. > > Concerning parallelization of the operations involved in > > connect/reset/disconnect processing, David writes: > > > > > >>I suspect a per-hub "struct work_struct" would suffice, > >>though it's TBD exactly what work would be done there. > > > > > > Note that there operations require a separate thread or work-queue > > equivalent, because they have to take place in process context. I can > > think of several other possible arrangements: > > > > (A) One work-queue entry per request, created on demand. This > > would involve creating lots of little kernel processes and would make it > > difficult to handle requests in the correct order. > > There aren't so many cases where an order is required though. > Disconnecting a hub with children is probably the only case > worth looking at. There are others. How about disconnect rapidly followed by connect on the same port? You don't want to handle those in the wrong order. > > (B) One persistent kernel thread per bus. This could result in a > > large number of largely dormant threads. On the plus side, this is a > > natural level for handling address-0 serialization. > > > > (C) One single kernel thread to do everything -- i.e., khubd. > > That's what we've got now. > > > > I don't know what's best; maybe something not listed here. > > There's also what I had mentioned: > > (0) one work_struct per hub, scheduled as needed by the hub > status change URB callback, usb_device_reset(), etc. > > It _could_ handle config change events too, which would > make it responsible for "new usb inteface" paths through > driver binding. (Not "new usb driver" paths.) I meant for that to be included in the list; that's why I said "several _other_ possible arrangements". > Yes. Likewise there's a problem in the current driver > probe logic ... drivers expect it's their responsibility > to choose altsettings, but there's now code that sets > the host-side record to every legal value (not the device!). > > I'd rather seee it formal: drivers own altsettings, usbcore > leaves them alone except for when drivers change them. That'd > apply for probes during reset() too, not just first-enumeration. I was under the impression that's how it works already. Where does the core change an altsetting (other than when asked by a driver)? Anyway, that's certainly how it should be. > > So the question is this: If disabling a port fails, should we reset the > > entire hub (and thereby reset all the other devices attached to it), or > > should we power down just that one port (which will make it useless until > > the entire hub is reset)? > > Just power down that one port -- unless it's the only active one > on that hub. If you have to give up, and hub indicator LEDs are > available on that hub, set that one to blink orange (or whatever). Or, if it's the only active port, reset the hub and try to disable the port again. If that fails, power it down and proceed as discussed before. > (BTW: Blinking the port indicator LEDs will likely need a per-hub > timer callback to queue a control URB changing indicator status.) Yet more complications... > > I see what you mean. We want to avoid having any URBs in flight while a > > configuration change occurs. The code that David has written for > > disabling endpoints might do the job, with no need for any additional > > locking. > > That _must_ be used whenever the device has a config change event, > such as setting an altsetting or configuration -- or resetting. Why must it be used? In thinking it over, I don't see anything bad about submitting URBs at any time, except after the struct usb_device is gone, which will never happen. Do the host drivers maintain lists of currently available endpoints, and will they get upset if the list changes while an URB is in flight? > If we take the simple and "obviously correct" path of disconnecting > all drivers along the reset paths, we can allow reset completion > handlers to re-bind before we kick in the sysfs code that manages > other binding logic. This doesn't make sense. The only way to disconnect a driver is by calling its disconnect() routine, and when that happens the driver will erase all its saved state concerning the device. There won't be anything left to re-bind reset completion handlers or anything else to. > > What do we do about URBs sent to endpoint 0 by a driver during the time > > that device_reset() is re-establishing the original altsettings? We can't > > disable endpoint 0. > > If we disconnect() drivers up front, then such calls won't happen. But the reset() call will exist specifically for the purpose of setting the device state while leaving all the driver bindings unchanged. > IMO, if probe() changes the firmware, the right thing to > do is to fail and require whatever is driving the probe() > to handle the "device is now gone" case. It needs to do > that anyway, so there's no point in adding a bunch of new > failure modes. I agree entirely. Alan Stern ------------------------------------------------------- This SF.net email is sponsored by: eBay Get office equipment for less on eBay! http://adfarm.mediaplex.com/ad/ck/711-11697-6916-5 _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel