On Sat, 10 Jul 2004, David Brownell wrote:
> That's part of the point. It doesn't fix those problems which
> will show up most easily with suspend/resume of a device tree.
> We've been over that one before, not long after I first posted
> locktree() code and early CONFIG_USB_SUSPEND patches.
Yes. As I recall, we agreed that suspend/resume were special cases that
really did need to use locktree(), but other things (like disconnect) did
not.
> > There are relatively few places that need to lock more than one device at
> > a time, and they can all be fixed up. Most of them will be fairly simple
> > to change; probably usb_disconnect() will be the worst case.
>
> In part because its calling convention has always been kind of rude!
>
> So that "lock(dev); usb_disconnect(dev); unlock(dev); release(dev);"
> and similar "obviously correct" code sequences don't work, since it's
> got more than the usual amount of side-effects magic.
That's part of the problem, yes. Another part is the way usb_disconnect()
needs to receive a pointer to a pointer so that it can break the
appropriate tree link. Not to mention the fact that it has to be able to
disconnect root hubs, which don't have normal parents. On the whole,
usb_disconnect() is so special that I don't mind giving it individualized
treatment. If only it were called from fewer places!
> Only people working inside usbcore should ever need to worry about
> the complicated stuff. Everyone else just goes lock()...unlock().
>
> As currently coded (nyet integration tested) the rwsem is still
> hidden, but "hub-aware" code is allowed to down(&serialize)
> after lock_device() for the root of a subtree.
Okay, I'll do it that way.
> > I don't understand why this is needed. On all occasions when a device is
> > locked, apart from suspend() and resume(), it's not necessary to lock the
> > entire subtree below the device. Locktree() would be overkill.
>
> That's not very true. Disconnect processing will certainly lock
> the whole subtree. You can't reset hubs without resetting their
> children. You shouldn't be able to unbind the hub driver from
> a hub without disconnecting the children. And so on.
I disagree. Bear in mind that the whole point of locktree() is that it
prevents thread B from locking a sub-portion of the subtree already locked
by thread A. Thus, it prevents B from resuming a subset of the devices
that A is trying to suspend. There's no need for such a thing with
disconnect -- who cares if B tries to do something with devices that A has
disconnected (or is about to disconnect)? Or vice versa, who cares if B
wants to disconnect some devices that A has suspended (or is about to
suspend)? Locktree() is overkill here, as I said.
While it's true that you can't reset hubs without resetting their
children, in our current implementation you can't reset hubs without
_disconnecting_ their children. (Doesn't resetting an external hub cause
it to turn off power to all its ports?) Thus resetting falls into the
same category as disconnect and doesn't need locktree().
Likewise, since unbinding the hub driver from a hub now disconnects the
hub's children, again this is an instance of disconnect and doesn't need
locktree().
> Plus of course, if it's not a hub, none of that's an issue. It'll
> be transparent to device drivers that don't manipulate hubs, and
> the only cost will be a few instructions that prevent some nasty
> interference scenarios.
Instructions that involve taking unnecessary locks, causing unknown
delays, and not preventing any scenarios I've been able to think of.
> > It's not clear why usb_lock_device() has to check whether the device is
> > gone. If the device is gone, that fact will be detected later on anyway.
> > Why disturb all the existing code that assumes device locking always
> > succeeds?
>
> Any disturbance isn't actually much at all, considering that the code
> I changed is more or less from
>
> lock_device();
> if (it_vanished()) {
> unlock_device();
> return -ENODEV;
> }
> to
> if (lock_device() < 0)
> return -ENODEV;
Okay, that's easily added into usb_lock_device() if you really want. But
given that locktree() is going to explicitly call down(&udev->serialize),
it will have to make this check for itself anyway. So I don't see much
advantage to adding it into usb_lock_device().
> Inconsistencies in locking models always bother me, since they
> normally provide very pleasant spots for bug colonies to settle.
>
> I'd ask it this way: why should lock_for_reset() ever succeed
> in a case where lock() would fail?
Actually there is one case where lock_for_reset() succeeds and lock()
would fail. It's by design. If you call lock_for_reset() from within
probe() it immediately returns success, without actually locking anything
-- there's no need since the device lock is already held. Of course if
you tried a simple lock() under those circumstances it would hang.
In fact, that special case was one of the two main reasons for creating
lock_for_reset(). The other special case was locking during disconnect(),
which fails immediately rather than hanging.
Alan Stern
-------------------------------------------------------
This SF.Net email sponsored by Black Hat Briefings & Training.
Attend Black Hat Briefings & Training, Las Vegas July 24-29 -
digital self defense, top technical experts, no vendor pitches,
unmatched networking opportunities. Visit www.blackhat.com
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
