David:
I got three messages from you all at once on this topic; I'll try to reply to them all together.
There was a bit too much email on that topic for me to read it as it happened, so I caught up all at once ... :)
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.
So that part doesn't need to differ from re-enumeration.
(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.
That doesn't answer my question though. Key point is that the "two" functions really don't need to be "two"; its a case of one coin, two sides.
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.
I'm pointing out that they're logically two sides of the same coin, once you consider the fault handling/recovery behaviours.
What one side views as fault recovery, the other side views as the expected/desired behavior.
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.
Yes. Particularly since the error handling for each task turns it into the other ...
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.
That "complicated form" doesn't seem necessary to me. After all, if any interfaces changed, the descriptors changed -- which is the first coin, first side. And if they didn't, it's the first coin, second side.
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?
Because the driver doesn't know which of the two will really end up happening: a firmware crash might cause a need to reload, or maybe descriptors didn't change after the reload and the effect is just to re-initialize internal state.
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.
But there's also the allocation of "dev". There may be a need to flag the "re-using old state" path in the enumeration logic ... easily known when children[portnum-1] is non-null, or when a given descriptor handle is non-null.
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.
Have a look at the EZ-USB doc to see the details, which I fudged a bit. It initiates re-enumeration, I forget exactly how, and clearly can't lose power since that'd mean the firmware got lost. (It's all detailed in one of the earlier chapters in the technical reference manual, which is a big PDF doc you can download from Cypress.)
- 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.
EZ-USB devices wouldn't have the chance, since the re-enumeration is driven from the device's new firmware.
In the "one coin, two sides" approach, it happens "for free"...
The basic model to use for firmware loading is that the original device disappears.
That's right.
At which point, why bother with an altered() call, since the enumeration logic will already be handling everything right?
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.
Well, cdc-ether should vanish, and Oliver recently analysed the usage of those. A number used it as a "soft reset", logically equivalent to going to config 0 then the current config. And others used it un-necessarily:
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.
I know for a fact that "previously" it didn't, and that even in the current code the sysfs files are still handled wrong. Were there other issues you're thinking of? The main open issue I'm aware of is handling the interfaces in sysfs.
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.
So implement the hub's work_struct logic correctly. It's clearly simpler if only that one task is handling this stuff.
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.
The device probe logic used to do it in 2.4 ... looks like someone fixed that in 2.5 when I wasn't looking, good!
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?
Yes, the HCDs have lists of currently active endpoints; in fact, the HC may be using them. Yes, changing them needs to be sycnchronized with usbcore, because things can break rudely if something smashes that hardware state. (Only UHCI is at all forgiving in that respect.)
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.
Now you're talking about a different issue: drivers re-using state. It's why I mentioned re-using the original usbcore pointers on these reset paths, until we know it's impossible (because the descriptors changed). It's not the best handle on the device (since pointers get re-used), but it's one option.
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.
I think saving the old driver state should be the responsibility of the driver that calls usb_reset_device(), not usbcore.
The whole model of a "hard" reset that retains driver state has always bothered me. I don't know how we ended up with it ... in particular, did anyone even explore the notion of a "soft" reset, like set_config(0) then set_config(previous)? Normally the whole point of a "hard" reset is to re-init.
We only use that "hard" reset in usb-storage; one of the
SCSI-ish scanner drivers, where there's a "FIXME untested"
comment; and usbfs, with 'err("this function is broken")'
in the runtime ... and we get a lot of problems when
usb-storage runs down those paths, too.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.
Good -- now all we need is a few incremental patches! :)
- Dave
Alan Stern
------------------------------------------------------- This SF.net email is sponsored by: Etnus, makers of TotalView, The best thread debugger on the planet. Designed with thread debugging features you've never dreamed of, try TotalView 6 free at www.etnus.com. _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
