On Thu, 1 Nov 2012, Sarah Sharp wrote:
> On Thu, Nov 01, 2012 at 03:46:37PM -0400, Alan Stern wrote:
> > On Thu, 1 Nov 2012, Sarah Sharp wrote:
> >
> > > Alan, can you take a look at the patch and see what I can do in the case
> > > where five warm port resets fail? I think it's a pretty unlikely
> > > scenario, but if you have a good solution, I'm all ears.
> >
> > Hmmm. Overall I'm not really sure about the logic in that routine.
> > It would make more sense for the decision about whether to issue a warm
> > vs. a hot reset to be made by the caller.
>
> Are you talking about this bit of code in hub_port_wait_reset?
>
> if (!warm) {
...
> ret = hub_port_reset(hub, port1,
> udev, HUB_BH_RESET_TIME,
> true);
> If so, then yes, I agree that the decision to issue a warm reset should
> probably be made in hub_port_reset() instead.
>
> Actually, there's a couple other issues I see with warm reset.
>
> First, under USB 3.0 hubs (and roothubs), a failed hot reset will
> automatically move to a warm reset. That means the warm reset change
> bit can be set if we only asked for a hot reset.
> hub_port_finish_reset() assumes that only the hot reset change bit needs
> to be cleared if warm is false, but it may also need to clear the warm
> reset change bit. This needs to be fixed, or we can lose port status
> change events.
That should go away once hub_port_wait_reset stops doing its own warm
resets.
> Second, this bit of code in hub_port_finish_reset() looks wrong:
>
> case 0:
> if (!warm) {
> struct usb_hcd *hcd;
> /* TRSTRCY = 10 ms; plus some extra */
> msleep(10 + 40);
> update_devnum(udev, 0);
> hcd = bus_to_hcd(udev->bus);
> if (hcd->driver->reset_device) {
> *status = hcd->driver->reset_device(hcd, udev);
> if (*status < 0) {
> dev_err(&udev->dev, "Cannot reset "
> "HCD device state\n");
> break;
> }
> }
> }
>
> Why wouldn't we want to notify the xHCI driver that the device was reset
> if we issued a warm reset? Maybe that's due to the code in
> hub_port_wait_reset() that initiates a warm reset when the normal reset
> fails? hub_port_finish_reset() would get called twice with the current
> code structure, and I think this code snippet is trying to work around
> the fact that we can only notify the xHC once that the device has been
> reset. Once the Reset Device command has completed, the xHC will move
> its internal device state to Default, and subsequent Reset Device
> commands will fail with a context error until the device is
> re-addressed.
>
> I'm actually not sure we even want to break on a Reset Device command
> failure. If we do, we never clear the port reset change bits (warm or
> normal), which would cause port status change events to be lost.
Straightening out the overall logic should help a lot. And yes, when
an attempted reset is finished (successfully or not), the reset-change
bits always need to be cleared.
> > Anyway, if five resets in a row all fail then we should do our best to
> > ignore the device. Let the user unplug it and try again. Will this
> > patch do that?
>
> I think the patch will leave the port in SS.Inactive (the old code would
> too). The port will stay in that state (even when a device is
> connected) until either the USB core issues a warm port reset, or the
> port is disabled. But if we disable a SuperSpeed port, we don't get
> connect events until we set the link state to RxDetect. So there's
> basically no way to get an automatic notification of a device connect
> when the port goes into this state.
Even if the device is physically unplugged and plugged back in?
> Can we set a timer to kick khubd after some amount of time (30 seconds?)
> and make hub_events check for ports in SS.Inactive even if none of the
> change bits are set?
I don't like that idea. It would be much simpler to put the port back
into a useful state. For example, disable the port, then put it into
RxDetect, then clear any change bits, and set the port's bit in
hub->removed_bits.
(hub->removed_bits was meant to help implement Windows' "Safely remove
hardware" feature. When a port's bit is set, khubd will ignore any
device attached to that port until it sees a connect-change.)
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
