On Tue, 13 Nov 2012, Sarah Sharp wrote:
> Hi Alan,
>
> I've been working on the warm reset fix patch, and I've run into a
> couple questions. A draft patch is attached, but it's not in any way
> done. :)
This is indeed a difficult problem to get one's mind around.
> In hub_events, we check whether the hub port is in the Inactive or
> Compliance Mode state, and issue a warm port reset. If there was a
> device attached to that port, don't we need to call the driver's
> prereset hook before we reset the device?
This depends on how the port got into the Inactive or Compliance Mode
state in the first place. Do ports sometimes change state
spontaneously?
Normally I wouldn't expect a port to be in a weird state like that
except possibly when the hub is activated, or following a resume or a
hub reset. Which would mean that your question would never arise; the
worst that would happen is that a warm port reset would be needed
during a resume, in which case we should make it a reset-resume.
> I'm wondering if we should just remove this bit of code in hub_events:
>
> /* Warm reset a USB3 protocol port if it's in
> - * SS.Inactive state.
> + * SS.Inactive state or Compliance Mode.
> */
> if (hub_port_warm_reset_required(hub, portstatus)) {
> - dev_dbg(hub_dev, "warm reset port %d\n", i);
> - hub_port_reset(hub, i, NULL,
> + int status = hub_port_reset(hub, i,
> + hub->ports[i - 1]->child,
> HUB_BH_RESET_TIME, true);
> + /* XXX: Not sure if we should set device state.
> + * Should this be passed a 0 or 1?
> + */
> + if (status)
> + hub_port_disable(hub, i, 1);
> + connect_change = 1;
> }
>
> and just let hub_port_connect_change() issue the warm port reset,
> disconnect the device, etc.
Or maybe deal with it in hub_activate(). The code shouldn't be in
hub_events() if the situation will never arise during normal usage.
> Also, if the warm rest fails, I'm also not sure if I should ask
> hub_port_disable to set the device state:
>
> + /* XXX: Not sure if we should set device state.
> + * Should this be passed a 0 or 1?
> + */
> + if (status)
> + hub_port_disable(hub, i, 1);
Normally hub_port_disable() is always called with a 1. There's only
one place where a 0 is needed; that's in the failure path of
hub_port_init(), as called from usb_reset_and_verify_device(). This is
because we try multiple times to reset the device, and we don't want it
to go away if some of the attempts fail.
> I added some test code to the xHCI hub code to test the patch out, and I
> verified that when a port in Compliance Mode is detected, it will warm
> reset the port several times, and then fall back to disabling and
> re-enabling the port. A device connect after that port disable is
> detected and the device is enumerated. So the patch works, it just
> needs some cleanup/tweaking.
What happens if you don't set the bit in removed_bits? It's not clear
whether this will make any difference. You wrote in the patch
description: "We need to be able to disable a USB 3.0 port for this
case where a device is completely unresponsive during enumeration and
keeps connecting." I don't know if that can work. Certainly it
doesn't work with USB-2 ports; if a device keeps connecting and
disconnecting then we have to deal with it each time.
Speaking of cleanups, I noticed a couple of things in the sample patch
that could be improved.
You added a big comment in hub_port_disable(). This comment
merely repeats what you already had written above
hub_usb3_port_disable(), so it's not needed.
You wrote:
> + dev_warn(hub->intfdev, "Could not disable port %d after %dms\n",
> + port1, total_time);
Ugh. I wouldn't display a message saying "1500ms", any more
than I would ask someone to call in "3hours" or say that a file
contained "536bytes".
The cleanup certainly looks worthwhile, but I didn't do a detailed
review. The changes are too big and far-reaching to be understood just
by reading the patch.
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
