The reason it locks the parent hub, very briefly, is to force tasks
to serialize.
I also keep forgetting something that's changed between 2.6.6-rc and the latest bk-usb code, too ... hub_port_init() adds the device to the tree earlier now. That's eliminated most of the places that usbdev->serialize _couldn't_ act as the lock on the port (other than debouncing).
We've been ignoring hub->khubd_sem though. Or at least, I certainly have been overlooking it, because unlike address0_sem, the bus rwsem, BKL, usbdev->serialize (etc) it's not particularly been visible (that is, deadlocking drivers!) outside the hub code so far.
The "port waiters" notion looks like it it's probably worth pursuing, given the SCSI EH/disconnect issue.
Even without that issue. Without some way to serialize access to a port, you might have khubd trying to react to the status changes caused by another thread doing a port reset, for example.
Would it replace khubd_sem then? That's what khubd uses. I know I've been thinking usbdev->serialize would have that role. Do you think we really need two parallel locks like that?
I think we really should lock the device during configuration changes. A
certain amount of mutual exclusion with tasks that want to traverse all
the interfaces in the device (like /proc/bus/usb/devices) is required.
Not necessarily much. The interfaces won't change; only what's bound to them (which is safely locked when creating the "devices" file). The real exclusion is against sysfs, which I hope we agree is safely locked already. Only usbfs and sysfs do traversals now, other than khubd.
The bus rwsem might be sufficient for this, but it locks too much -- all the USB trees. In principle this mutual exclusion could be accomplished using something other than ->serialize, but why make things more complicated than they have to be? I don't think we lose much by doing regular device locking here.
If you're concerned with printing the active configuration, I suspect there's nothing wrong with printing a just-barely "old" value for it.
It turns out we will also need to distinguish cases (A) and (B). To do
Why?
I think I mentioned that somewhere lower down. In (A) the hub port is already acquired; in (B) it isn't. This affects what usb_reset_device() must do.
If "acquired" means "holds usbdev->serialize", that's what I pointed out could be made consistent by saying _neither_ holds it. Changing that would also mean no API change for usb_reset_device() is needed (since you'd wanted it just to distinguish cases A and B).
In addition, usb_reset_device() must be aware of the context in which it
is called. If the device is already locked (as it would be during
probe()) we should avoid trying to lock it again. If the device's hub's
port is already acquired (as it would be in (A)) we should avoid trying to
acquire it again. But now all the information needed to make these determinations is available. This means the device resets will work even during probe(), with no other modifications needed.
Hmm, wouldn't my own proposal for not holding usbdev->serialize also resolve that reset-during-probe issue? I get the impression you didn't quite see what I was driving at with it.
I may not have followed all the details of what you said. But for the reasons given above, I think we should hold ->serialize during config changes. Did you already provide a way of countering my objection? If you did then I missed it, sorry.
I think I did, but of course the best way to explain that point may be with a patch. When I get a moment, I may make one.
- 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
