Let's see if we can't clear away some of the issues.
On Mon, 3 May 2004, David Brownell wrote:
> Alan Stern wrote:
> >
> > I'm talking about protecting the physical topology of the entire tree.
>
> I still don't see why the _entire_ tree needs that sort
> of protection. I hope I'm not missing something.
It doesn't. On the other hand, we might not want to use hub->serialize to
provide this protection, for a reason I'll explain below. Basically,
though, what we want is some sort of read-protection while traversing the
tree whereas serialize provides write-protection. (It's not quite so
simple but that's the general idea.) Making serialize an rwsem doesn't
quite solve the problem (see below). Adding a global topology semaphore
appears to be the easiest solution.
> > And I would find it acceptable
> > to use hubdev->serialize to protect the links at the inner nodes if only I
> > could see how to also get usb_reset_device() working reliably. And
> > useable from within probe().
>
> I think usb_reset_device(dev) would just need to call locktree(dev),
> then fail cleanly on -ENODEV returns.
>
> As for usable from probe() ... I just had the germ of an idea. How about
> splitting out the "logical" topology lock (protecting SET_CONFIGURATION,
> while it's probing every interface) from the "physical" topology lock
> (hub->serialize)? A single bit in "struct usb_device" would suffice
> to prevent nested calls to the usb_set_configuration() routine, during
> probe(), which would cause self-deadlock (and other chaos).
I've been considering possibilities along those lines as well, but they're
still far too cloudy to mention here. Instead, here's a description of
the picture I'm building up:
Locks will always go top-down. This automatically solves the
"disconnecting a subtree and the whole tree simultaneously" problem.
When the thread starting from the top reaches the hub above the subtree
(i.e., the hub that reported a port connect change) it will find that hub
locked by the subtree-disconnect thread. When the hub is unlocked the
entire subtree below the port will be gone, so the whole-tree disconnect
can proceed without difficulty.
The big question is how to make device resets work. It's pretty clear
that usb_reset_device(udev) needs to lock both the parent hub and udev.
The problem is that the driver's disconnect() and suspend() routines will
probably block waiting for usb_reset_device to return, so we can't simply
acquire the semaphores in the usual way.
This approach ought to work for acquiring hub->serialize:
for (;;) {
if (udev->state != USB_STATE_CONFIGURED)
return -ENODEV;
if (down_trylock(&hub->serialize))
break;
wait_ms(100);
}
Yes, this is a polling loop. It's not so bad, because device resets are
very rare. Furthermore, when a driver does a device reset it expects to
encounter a long time delay.
This won't deadlock with disconnect caused by unplugging the device (or
unbinding the hub driver!) because the device's state will be changed to
USB_STATE_NOTATTACHED. It won't deadlock with disconnect caused by
rmmod'ing the driver because that won't lock the hub. To prevent
deadlocks with suspend, your patch would have to change the device's state
before calling its suspend() routine -- or else the driver's suspend()
routine would have to avoid waiting for usb_reset_device to return.
(Incidentally, I don't see the need for usb_suspend_device() to call
locktree(). Wouldn't it suffice to lock the hub and then check that it is
still USB_STATE_CONFIGURED?)
Next we need to lock udev itself. However, if some other thread already
has locked udev (for suspend or unbind) then we generally want to fail.
So a simple down_trylock() will suffice.
There are exceptions, though. What if udev is locked because a different
interface is being bound or unbound? I don't know -- this comes under the
heading of resetting a device with more than one interface, so let's
ignore it.
What if udev is locked because of a tree traversal? Then we don't want to
fail; we just want to wait until the lock is released. This is the
special reason I mentioned at the top of this email. Now you see the
problem with making serialize into an rwsem: We want to acquire a write
lock, we want to wait if the lock is currently held for reading, and we
want to fail if the lock is currently held for writing. Rwsems won't do
that. But if tree traversal used a separate semaphore then everything
would be all right -- we could acquire udev->serialize and ignore the
topology semaphore since we aren't going to change any children[]
pointers.
The last problem is dealing with device resets during probe. The first
part of this problem -- locking the hub -- might be easy. Is there any
reason why usb_new_device() can't be called with the hub unlocked?
The second part of the problem is trickier. Do we need to hold
udev->serialize while probing? Certainly we want to avoid suspend during
probe and vice versa. Apart from that, perhaps it doesn't matter. Or
perhaps we could adopt something along the lines of your suggestion, so
that usb_reset_device() would know if it was being called from within
probe and could skip acquiring the device lock. (But what if it was
called by a driver for an interface other than the one being probed?)
Or as a last resort, we can rule out resets during probe -- any driver
that wants to do it will have to use a separate kernel thread or work
queue. The disadvantage is that probe() will have to return before
learning the result of the reset.
Alan Stern
-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g.
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
