I'm about ready to give up on device reset during probe.
Preventing it would need a per-interface state flag, FWIW; easily set by usbcore during probe(). Somewhat parallel to the flag I suggested be held during SET_CONFIGURATION...
(That is, there are kinds of locks that don't involve any synchronization in the way spinlocks and semaphores do. One example is a flag that's protected by a synchronizing lock. Using such non-synchronized locks implies some higher level of synchronization, or even polling.)
Consider this. There are three different paths that can call a driver's probe():
1. Newly attached device is configured by usb_new_device(). Both the device and its parent hub are locked.
Though I've described some alternatives to that. One issue is how to cope with configuration errors ... currently the solution involves counting retries. Likely additional per-port state would be needed to switch strategies there. That might be worth doing, given the irregular reports of devices that just keep retrying forever during enumeration. (I seem to remember reports from Olaf, but think he wasn't the only one with such devices.)
2. New configuration is installed through sysfs. The device is
locked but its parent hub isn't.
All codepaths should use compatible locking. For the _first_ time, we're really starting to look at how the hub driver locks ... we've gotten rid of several spurious locks (BKL, bus rwsem, address0_sem, even usbfs ps->devem), so this is really the first time the air has been clear enough to even see the hub-specific issues those others have masked!
By which I conclude: since that sysfs locking was designed to wend its way through the previous thicket of locking (now much thinned), and hasn't been around long enough that user mode code should care, it can be revisited. For example, by calling locktree().
3. Newly registered driver is matched to all unbound USB interfaces.
Neither the device nor its parent hub is locked.
But that probably doesn't actually _need_ much locking, beyond what the bus rwsem already provides. So the only issue there is that it's different.
The driver doesn't know (and shouldn't know!) which path has called it,
Right, but there can be constraints derived from the path. One example would be usbcore preventing SET_CONFIGURATION until the previous one completes (hence no recursion in probe). Another would be that drivers trying to RESET devices have to deal with the case where the device disappears at some point in that process ... which is "device morphed", as well as some funky error modes.
And now consider this: Every path calling probe() definitely has acquired the USB subsystem rwsem. Proper locking order requires that _no_ device locks are acquired while holding the rwsem!
... unless the calls from usbcore/hub don't hold such locks before calling the probe(). I did sketch partial solutions there, but we might not need to resolve that issue just yet.
You and Oliver have both disliked the use of dev->serialize in some of the contexts it's now used. OK, that's fair, but changing that would mean resolving those problems in other ways. Doable; not done.
All told, the easiest thing is simply to rule out device resets during probe.
I'm OK with that, though I suspect you'd agree it'll be worth remembering that issue for later.
Do you have a model for how DFU should trigger the reset? That's where the "queue resets" idea could help, but another option would be to have some special return frome probe() which means "please reset the device". That return could be processed during enumeration (SET_CONFIGURATION) as well as during usb_register_driver().
Here's another problem (first raised by Oliver). In hub_port_init(), the failure path simply disables the port. It doesn't check to see that the disable request succeeded. If it didn't succeed, we potentially have a device using address 0 on the bus and usb_address0_sem is unlocked. The way to cope with that is to reset the parent hub. This means calling hub_reset() while holding the address0 semaphore -- not feasible at all. I suppose another way to cope would be to disable but not reset the parent's port (working up the tree if necessary). This would still mean acquiring locks in the wrong order.
Hubs not disabling ports are severely broken; that code should probably at least retry a bit before giving up, and giving up should syslog an error at a severe level (maybe KERN_ALERT, telling user to unplug the device). Until that problem is resolved, khubd should ignore all new devices on that device ... it's no problem if something hangs out in USB_STATE_DEFAULT (with address 0), so long as there's only one of those per bus.
Maybe neither top-down nor bottom-up locking is workable. Maybe the best approach is to have a single bus-wide semaphore, and only allow one significant action to happen at a time. That would simplify a lot of problems. (Even the newly-registered driver problem, although that would require locking _all_ the buses.)
Top-down can clearly (to me!) work, and would have clear advantages given that userspace code can initiate those actions concurrently. Given that many potential concurrent accesses to the USB tree, the notion of any kind of bus-wide semaphore bothers me a lot unless it's only held for very brief times. (That is any such bus-wide lock shouldn't be used for synchronizing.)
I think we can just continue working through these problems one at a time.
- Dave
-------------------------------------------------------
This SF.Net email is sponsored by Sleepycat Software
Learn developer strategies Cisco, Motorola, Ericsson & Lucent use to deliver higher performing products faster, at low TCO.
http://www.sleepycat.com/telcomwpreg.php?From=osdnemail3
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
