And maybe I'll have a chance to look at the patch, not just that one issue you raised? :)
Alan Stern wrote:
On Mon, 12 Apr 2004, David Brownell wrote:
Hmm, could you elaborate? The bus rwsem is only to protect
Capsule summary: you're assuming a "topology change lock".
Isn't that one of the major functions of the bus rwsem?
Yes and no. It's used to protect data structures ... for very brief moments, and individual driver probe() calls; so "no".
But "yes", if you hold it a long time, it could serve as such a lock ... but usbcore doesn't use it that way (today).
Possibly of interest: bus_for_each_dev() uses the bus rwsem as a topology readlock. But I don't think that's normally used for "long" operations.
OK, you're strictly correct from the driver model perspective, but that doesn't contradict what I said either.
Sure it does -- you said the rwsem protects _only_ against changes in binding. :-)
It's a bit squishy. Today, that's all it really accomplishes; the device create protection is pretty much invisible, even inside usbcore. Drivers see it as I said.
You're right that it _could_ be used more broadly, though.
It's oddly like observing that while we have an army to protect our borders, it could easily be (ab)used in wars of aggression ... ;)
If you didn't have such a "global synch" model, what problems would arise? I don't think the current code is guaranteed to provide a globally consistent snapshot of the device tree.
Here's a problem that can arise. The usb_device_dump() function in devices.c contains this code:
/* Now look at all of this device's children. */
for (chix = 0; chix < usbdev->maxchild; chix++) {
if (usbdev->children[chix]) {
ret = usb_device_dump(buffer, nbytes, skip_bytes, file_offset, usbdev->children[chix],
bus, level + 1, chix, ++cnt);
What happens if the child on port chix is disconnected between the time of the test and the time of the recursive function call? I don't care if we
don't give a globally consistent snapshot, but causing an oops isn't acceptable.
Top down lock chaining might help ... what happens if there's a requirement that the caller of that routine holds usbdev->serialize? (That'd replace that little patch I sent before.)
Clearly that "if (usbdev->children[chix])" branch, and usb_device_read(), would need to wrap their calls with code to grab and release that lock.
But that'd solve problems like "how do N users cat /proc/bus/usb/devices at the same time without total serialization". (Except for that rude "usb_bus_list_lock" ... which could become an rwsem.)
However, it'd probably force a few issues in khubd (as you comment).
Then of course we end up running into problems about trying to acquire the two locks in the correct order...
Given that the physical hierarchy is (root) hub down, the logical one for such a case should do the same thing ... I'd say, going just by rules of thumb! One could hold hub->dev->serialize while scanning its children.
In addition the code that removes children (usb_disconnect) must be changed to hold the parent hub's ->serialize. It wouldn't hurt to do that while adding children as well, although it's not necessary.
I'd just go by a general rule: dev->serialize protects both kinds of sysfs child: interfaces (config changes) and also usb devices (only for hubs, dev->children).
Like I said originally, the locking here needs to be fixed. I wasn't sure what was the best way to do it; looks like you've identified the necessary changes.
Well, I've sketched out an approach I'm convinced is good, and you seem reasonably persuaded. But given the problem, I expect a few more issues to surface!
- Dave
Alan Stern
------------------------------------------------------- This SF.Net email is sponsored by: IBM Linux Tutorials Free Linux tutorial presented by Daniel Robbins, President and CEO of GenToo technologies. Learn everything from fundamentals to system administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
