On Fri, 2 Jan 2004, David Brownell wrote:
> > For error recovery. If an attempt to disable the port fails, we can't
> > simply leave things as they stand -- there will be an unaddressed device
> > attached to the bus. It will be necessary to disconnect the entire hub to
> > avoid problems. And doing that requires a lock on the hub's parent.
> I hope you're actually saying that if the device is in the DEFAULT
> state (a brief instant during port reset) that might need to happen.
As John Cleese once said, that was indeed the very nub of my gist.
> Right now nothing does such things ... has that been making trouble?
As far as I know it has never happened.
> Hubs that can't disable ports would seem to be rather broken...
Quite so. Actually, the most likely way I could imagine this happening is
if the hub is unplugged at just the wrong time. Of course, in that case
it doesn't matter whether we try to reset the hub or not. Broken hubs
seem much more unlikely.
This ties in with the earlier part of this thread. Managing
malfunctioning hubs is an important aspect of maintaining the topology of
the USB bus. Right now, hub.c contains special-case code to reset a hub
for error-recovery. It ought to be possible to reuse that code for the
situation when the hub won't disable a port.
Even better, all the special-case code in hub.c:hub_reset() could be
removed if we had a way to specify that usb_reset_device() should follow
the "device morphed" path, regardless of whether the descriptors actually
did change. For instance, we might say that this should happen whenever
the device being reset is a hub.
This is related to a subtle point concerning usb_disconnect(). Normally
it's called when we want to remove a usb_device plus everything below it
in the tree. But there are a few cases when the code wants to remove
everything below the device while leaving the device itself intact. It
does this by calling usb_disconnect() repeatedly, for each child of
the device -- awkward but workable.
One of those cases is in hub_reset(), which I mentioned above. The others
are rather analogous although different in detail; they occur when a host
controller fails -- everything below the root hub is disconnected but the
root hub remains. In a way, failure of an HC can be treated as failure of
the virtual root hub.
The point I want to make here is that these cases could be simplified in a
way by calling usb_reset_device() on the hub in question, letting it
remove the hub along with all the children. If the hub is still present
afterward, it will automatically be reprobed. Root hubs will require
special treatment, of course, but in the end this may turn out to be a
better approach.
Getting back to the original topic, here are two points that I think
everyone can agree on:
When usb_disconnect() is processing the children[] array for
a hub, it doesn't want the hub driver interfering by handling
attach/unplug events concurrently.
Because usb_disconnect() will remove the hub device after
processing all its children, preventing the hub driver from
interfering by means of a lock or semaphore is the wrong approach.
When the lock is released, the hub driver would unblock and go
ahead to attach or remove a child from the hub which is itself
about to go away. Instead, the hub driver should be prevented
from interfering by being unbound from the hub.
Given this reasoning, there can be no doubt that it's best for
usb_disconnect() to unbind the hub driver before processing the children[]
array of a hub. There are other corollaries too, such as that
usb_disconnect() needs to be able to tell whether a given device is a hub
or not. I think that's a reasonable requirement; hubs are so closely
connected to the maintenance of the bus topology that usbcore _should_ be
aware of which devices are hubs.
If usb_disconnect() follows this approach of unbinding the hub driver
before doing anything else, then there's no possibility of contention for
the children[] array between the hub driver and anything else. Of course,
there's still the possibility of contention among other threads calling
usb_disconnect(). The best way to avoid this is for usb_disconnect() to
hold usbdev->serialize while processing a device and its children.
In fact usb_disconnect() already does this anyway. David, does this
amount to maintaining the same kind of lock tree you were talking about?
There's one more point. usb_disconnect() is passed as an argument a
pointer to the device entry in its parent's children[] array, and one of
the first things it does is to zero out that entry. That means it ought
to hold the parent's usbdev->serialize, but of course it can't acquire
that semaphore on its own. The caller has to do it. This could be
handled in two ways. In the first, the caller simply grabs the semaphore
before calling usb_disconnect().
In the second, which I like better, the caller grabs the parent's
serialize and zeros out the entry by itself. Then it releases the
semaphore and calls usb_disconnect(), passing it a pointer to the device
rather than a pointer to a pointer.
This would permit an attractive concurrency in handling unplug events.
Khubd could remove the pointer from the parent hub to the child device,
then another thread could carry out all the work involved in removing the
device structures, unbinding drivers, and so on. Khubd would not have to
wait for that to complete -- I think; I'm not so sure about the underlying
driver-model children pointers that would remain.
Alan Stern
-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials.
Become an expert in LINUX or just sharpen your skills. Sign up for IBM's
Free Linux Tutorials. Learn everything from the bash shell to sys admin.
Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
