On Mon, 5 Jan 2004, David Brownell wrote:
> Resetting a hub also resets devices on downstream ports ...
> including the DVD you may have been burning! That's
> not something that seems safe to try automatically on
> all kinds of devices. For mice -- probably safe.
Yep, it's a dangerous thing to do. But that's what the hub driver
currently does.
> I'm not sure how often that special case code has been used;
> I don't recall noticing it happen (Maybe that's because
> it's reporting with KERN_DEBUG. It should use dev_err.)
Maybe it never has happened. Not having had much experience with them, I
would imagine that hubs are usually pretty reliable.
> > 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.
>
> There are a lot of kinds of reset, and it's clear to me that the
> current usb_reset_device() semantics are sometimes irrelevant.
>
> There should probably be a "powercycle the port" reset too.
> (Gets back again to hub-wide resets. Not all hubs can do per-port
> switching, sometimes power switching is ganged, so that'd force
> all ports to powercycle not just the one.)
That would be a good feature to have, if we can decide how to deal with
the problem of power-ganged ports.
> Why would you say hubs should act that way? It'd make sense
> to me for hubs to be like usb-storage, and support a limited
> amount of history during resets. Enough to do a more graceful
> restart for devices that have significant driver state (like
> the SCSI stack, which can show as files in a user's GUI).
A hub doesn't have any state that it needs to preserve across a reset, or
at least none that can't be reconstructed after the reset. Since the
reset will automatically disconnect all downstream devices, we not only
don't want to save the current state, we want to get rid of the state as
easily as possible!
A more sophisticated approach to this whole thing would be to make drivers
aware that their device might undergo a reset for external reasons at any
time. If the driver had some way to be notified of an upcoming reset then
it could take steps to restore the device to its original state
afterwards. Thus all drivers for devices downstream from a malfunctioning
hub could be notified before (and again after) the hub is reset.
Similarly, drivers bound to multiple interfaces could be notified when one
of them decides to reset the device. (But what is a driver supposed to do
about incoming requests during the reset period when the device is
unavailable? Fail them, delay them, or whatever?)
I suspect this is all too esoteric for us to worry about, at least for
now.
> > 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.
>
> It shouldn't be just "in a way"! And in fact, if we had a
> clean way to fail root hubs, we could handle some power management
> scenarios a lot better ... and also do bus resets in the generic
> layer, rather than UHCI and OHCI having private schemes, and EHCI
> (and surely other out-of-tree hcds) having none at all.
Right now the only way hubs can fail is by not responding properly (or at
all!) to the periodic status request. Almost by definition, that doesn't
happen with root hubs. We could emulate it in the same way we virtualize
the other functions of a root hub. Is that what you are suggesting?
> > 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.
>
> Logically, usb_disconnect() is part of the hub driver. This is
> one of those "left hand and right hand must cooperate" deals.
Sort of. That's undoubtedly true when usb_disconnect() is called as a
result of a hub reporting a connect-change event on a port. It's not so
clear when usb_disconnect() is called as a result of deregistering an
entire bus because the HC driver is being unloaded.
> > 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.
>
> I can imagine particular ways of using locks or semaphores
> to avoid ... but that doesn't mean I think the problem isn't
> solvable in a lock-based solution space!
Well, the straighforward approach of locking the hub while processing it
and its children won't work. In fact, it would probably cause deadlock
because of the way the hub driver uses its own private lock to prevent
being unbound from a hub while the hub is actively in use. The hub driver
should be using the serialize semaphore instead of its own khubd_sem.
> > 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.
>
> Again, this is left-hand/right-hand stuff. Two threads going
> into the hub management code shouldn't be able to confuse that
> code at all.
>
> There are two things I don't like about that "instead". One
> is trying to use a negative ("unbound") to imply a positive
> ("reserved for purpose X") ... which tends to be a bug
> waiting to happen. The other is that I trust the driver model
> to maintain a "bound" state, but not "unbound".
It sounds like you're saying that the hub driver needs a better scheme for
managing its event list. If it used usbdev->serialize in place of
hub->khubd_sem, then it would never try to update the children[] array
while the hub was in the process of being disconnected. But this would
trigger a BUG() in hub_events(), and port connect-change events would
still accumulate for the driver to try and cope with afterwards.
The only way around this would be for usb_disconnect(), in addition to
holding serialize, to remove the hub from the event list and prevent it
from ever being added back on. But that's exactly what unbinding the hub
driver would do!
> No ... better just say that drivers for single-interface devices
> sometimes need to manage their resets. Maybe they'd need a
> usb_driver.reset() hook; maybe not. But usb-storage clearly
> expects to recover from a reset, and I think eventually the hub
> code should too (for PM resume and other cases, although in at
> least some of those cases other driver callbacks may apply too).
What can the hub driver do to recover from a reset? It can't preserve its
children -- they will have been disconnected and they will show up again
in the usual way as connect events once the reset is complete.
> The question is IMO whether usbcore should need special
> hub-only mechanisms (like dev->children) for that info,
> in addition to the two lists of hubs. (One is maintained
> by the hub driver ... it could as well use the list kept
> by the driver model.)
I considered that. The list kept by the driver model isn't suitable
because it includes _all_ the struct devices that are children of the hub.
Not all of these are struct usb_devices -- for example, the hub's single
interface is represented by a device that is a child of the hub's device.
Also, the information maintained by the driver model doesn't include any
indication of which port a child usb_device is attached to, although that
could easily be added to struct usb_device. (There is the bus_id string,
but that's a pretty flimsy way of doing it.)
> > In fact usb_disconnect() already does this anyway. David, does this
> > amount to maintaining the same kind of lock tree you were talking about?
>
> Sort of ... but just on that one path, so not really. For
> example the hub_event_list isn't ordered to understand hub
> topology, so if you unplug a tree of hubs you might not see
> the topmost hub starting disconnect processing first.
No, you almost certainly would. The hub upstream of the disconnected
subtree would report the event within 32 ms (or whatever the status
interrupt URB period is) whereas the missing hubs would cause single
errors instead, and the hub driver doesn't do anything until it
accumulates something like 10 errors. So disconnect processing would
start at the top of the tree, not in the middle.
> Ideally, the whole tree would be marked as "disconnected"
> first, so everything would start to quiesce ASAP. Then the
> graceful shutdown processing would start, disconnecting the
> devices in no great hurry. (You touch on that later too.)
I think that is how the current plan would end up operating.
> > 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.
>
> I don't like that "grab parent's lock" especially, but the whole thing
> has always been messy. If the cleanup involves more than one lock,
> I'd rather the lock direction go top down. That way the software can
> stop at the last hub still working on that path.
What I suggested _is_ top down. The caller first grabs the parent's
serialize (top), then calls usb_disconnect() which locks the child (down).
If this was a normal disconnect event (not an HCD rmmod), the caller would
be khubd and would already own the parent's lock because it would be in
the midst of event-list processing for the parent.
> > 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.
>
> Yes, that's a good way to allow concurrency: as soon as a device
> is marked "dead", take it out of the tree (sysfs would _maybe_ not
> show it any more) and queue it someplace for graceful shutdown.
>
> Whether or not that shutdown processing is concurrent, i/o on all
> child devices should start quiescing ASAP. By the time the shutdown
> processing gets around to any device, maybe it'll already have been
> quiesced so the shutdown part can be that much faster.
I'm not sure what you mean here. Why do you expect I/O on child devices
to start quiescing quickly? More likely you'll see a flurry of error
notifications and retries. Eventually things will die down, but you
shouldn't expect them to stop until the drivers have been notified that
their device has disconnected -- and that is part of the shutdown
processing! The only way to make things stop ASAP is to run the
disconnect (shutdown) processing ASAP.
> I'm not sure how the driver model pointers would work either, but
> that shouldn't let us stop getting rid of same-role pointers in
> usbcore!
Until you have some of telling whether a driver-model pointer points at a
usb_device or at something else, there doesn't seem to be any good way to
do this.
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
