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?
> 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. :-)
> 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.
That's the natural approach. I question whether it provides adequate
protection of the "for each device" part.
> 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.
Certainly we want to avoid doing that.
> > 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.)
I agree with that.
> > ->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.
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.
> > 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.
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.
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
