On Wed, Nov 14, 2012 at 10:57:44AM -0500, Alan Stern wrote:
> 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?
Yes.
> 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.
On a device connect, the port can go into either Compliance Mode or
Inactive on various link failures. Also, the ports can change state
into Inactive if the U1/U2 exit fails. So if we have Link PM enabled
for a USB 3.0 device, the port can go into Inactive at any time.
Therefore we need to be able to handle both Inactive and Compliance in
hub_events.
> > 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.
Ok, good to know.
> > 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.
I will have to double check if the removed_bits do make a difference. I
don't think it does.
> 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.
Agreed.
> 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".
I think I was just copying the comment from the port debounce function.
I'll happy change my copy. :)
> 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.
Yes, it's a fundamental change to the port handling code. I do want to
get it into stable, because other people are likely to run into this
issue, but I'm really concerned about the amount of code changes. Maybe
it shouldn't be marked for stable at all?
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
