On Mon, 12 Apr 2004, David Brownell wrote:
Alan Stern wrote:
(1) The locking in devices.c needs to be fixed. The USB subsystem rwsem should be held over a much larger part of the code. That should be pretty easy to do. I don't think it's necessary to use usb_get_dev or to lock usbdev->serialize, but someone should verify this.
Hmm, could you elaborate? The bus rwsem is only to protect
Capsule summary: you're assuming a "topology change lock".
against changes to driver bindings, and there's only one chunk of code that accesses those bindings (for a brief moment, already protected by that rwsem).
Maybe the bus rwsem _used_ to protect only against changes to driver bindings, but _now_ it also protects against adding/removing devices to the bus (and adding/removing drivers if you care about that).
OK, you're strictly correct from the driver model perspective, but that doesn't contradict what I said either. The bus rwsem is grabbed when devices or drivers are created, which is also the time driver bindings are changed (which is why we care), but there are several kinds of entity:
- USB Interfaces. Created only by holders of dev->serialize. - USB Devices. Created by khubd. - USB Hubs. Special devices, managed by usbcore/khubd. - USB Drivers. Created by module's init code.
So what I said is true: dev->serialize protects against add/remove
of the devices that drivers care about ("interfaces"), and anyone
caring about a stable set of interface-to-driver bindings must hold
the bus readlock instead. That's the lock hierarchy of today.The current code is logically "for each device, dump its descriptors and their bindings" ... so I described per-device locking.
I did find a missing claim of dev->serialize though, in the usb_device_dump() routine ... at least, this was in one patch I sent ages ago, but nothing I recent. It'd protect against device reconfig/reset/disconnect. See the attached patch.
Many of these things can be protected against by acquiring either ->serialize or the bus rwsem. Reconfig (I assume by that you mean changing the active configuration) wouldn't cause any problems even if no lock was held, nor would device reset in the non-morphing case. In fact, the only thing I can think of offhand that ->serialize would protect against and the bus rwsem wouldn't is a "device-morphed" reset, and of course we don't yet support those anyway.
It's a question of lock scope. Grabbing the bus rwsem locks every USB device in the whole system. I described a less global scheme.
One example of where a global scheme would have issues: when fetching the string descriptors takes a long time. Then "cat devices" could block most of the USB subsystem, not just the one device which was actually misbehaving.
It's not entirely clear (to me anyway) that holding ->serialize will provide sufficient protection against disconnection. That is, when a device is disconnected, the core doesn't acquire the parent hub's
At any given instant, the usbcore data records for a given device will be consistent if dev->serialize is used to lock it. Plus of course it must have been consistent when "dev" was located -- yes? Doesn't matter if it was disconnected later; it was OK to start. (And next time, it won't be there.)
->serialize. So when traversing the device tree below that hub we run the
risk of trying to access a nonexistent device structure. It seems that
the _only_ way to protect against these changes in the bus topology is by using the bus rwsem.
I'm not sure why you'd want to protect against topology changes. That's global state, so of course my "synchronize locally" reflex kicks right in ... creating central synch points, to be held during I/O and state changes, can cause trouble. Why care if a device you already printed (or haven't yet printed) is unplugged?
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.
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.
- 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
