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) {
/*
* Some buggy devices can cause an NEC host controller
* to transition to the "Error" state after a hot port
* reset. This will show up as the port state in
* "Inactive", and the port may also report a
* disconnect. Forcing a warm port reset seems to make
* the device work.
*
* See https://bugzilla.kernel.org/show_bug.cgi?id=41752
*/
if (hub_port_warm_reset_required(hub, portstatus)) {
int ret;
if ((portchange & USB_PORT_STAT_C_CONNECTION))
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_CONNECTION);
if (portchange & USB_PORT_STAT_C_LINK_STATE)
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_PORT_LINK_STATE);
if (portchange & USB_PORT_STAT_C_RESET)
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_RESET);
dev_dbg(hub->intfdev, "hot reset failed, warm reset port %d\n",
port1);
ret = hub_port_reset(hub, port1,
udev, HUB_BH_RESET_TIME,
true);
if ((portchange & USB_PORT_STAT_C_CONNECTION))
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_CONNECTION);
return ret;
}
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.
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.
> 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.
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?
Sarah Sharp
--
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
