On Wed, Dec 05, 2012 at 02:41:11PM -0500, Alan Stern wrote:
> On Tue, 4 Dec 2012, Sarah Sharp wrote:
>
> > When a hot reset fails on a USB 3.0 port, the current port reset code
> > recursively calls hub_port_reset inside hub_port_wait_reset. This isn't
> > ideal, since we should avoid recursive calls in the kernel, and it also
> > doesn't allow us to issue multiple warm resets on reset failures.
> >
> > Rip out the recursive call. Instead, add code to hub_port_reset to
> > issue a warm reset if the hot reset fails, and try multiple warm resets
> > before giving up on the port.
>
> This is better than before. There is still a little bit I don't quite
> follow...
>
> > In hub_port_wait_reset, remove the recursive call and re-indent. The
> > code is basically the same, except:
> >
> > 1. It bails out early if the port has transitioned to Inactive or
> > Compliance Mode after the reset completed.
> >
> > 2. It doesn't consider a connect status change to be a failed reset. If
> > multiple warm resets needed to be issued, the connect status may have
> > changed, so we need to ignore that and look at the port link state
> > instead. hub_port_reset will now do that.
>
> In fact, we might as well do the same thing for non-SuperSpeed
> connections too. The kernel probably is smart enough not to get
> confused if the user manages to replace one device with another while a
> reset is in progress.
Ok, I'll look into that, although I'm really not fond of the idea of
testing that much legacy code.
> > 3. It unconditionally sets udev->speed on all types of successful
> > resets. The old recursive code would set the port speed when the second
> > hub_port_reset returned.
> >
> > With the new code in hub_port_reset, the 'warm' variable may change on a
> > transition from a hot reset to a warm reset. So hub_port_finish_reset
> > could be called with 'warm' set to true when a connected USB device has
> > been reset. Fix the code by unconditionally updating the device number,
> > informing the host that the device has been reset, and setting the
> > device state to default.
>
> Why was it necessary to skip these things before, when "warm" was set?
> Was it merely an optimization? If not, do the same reasons still
> apply?
First, you need to understand that the old code did not handle connected
devices needing a warm reset well. There were only two situations that
the old code handled correctly: an empty port needing a warm reset, and a
hot reset that migrated to a warm reset.
When an empty port needed a warm reset, hub_port_reset was called with
the warm variable set. The code in hub_port_finish_reset would skip
telling the USB core and the xHC host that the device was reset, because
otherwise that would result in a NULL pointer dereference.
When a USB 3.0 device reset migrated to a warm reset, the recursive call
made the call stack look like this:
hub_port_reset(warm = false)
hub_wait_port_reset(warm = false)
hub_port_reset(warm = true)
hub_wait_port_reset(warm = true)
hub_port_finish_reset(warm = true)
(return up the call stack to the first wait)
hub_port_finish_reset(warm = false)
The old code didn't want to notify the USB core or the xHC host of device reset
twice, so it only did it in the second call to hub_port_finish_reset,
when warm was set to false. This was necessary because
before patch two ("USB: Ignore xHCI Reset Device status."), the USB core
would pay attention to the xHC Reset Device command error status, and
the second call would always fail.
Now that we no longer have the recursive call, and warm can change from
false to true in hub_port_reset, we need to have hub_port_finish_reset
unconditionally notify the USB core and the xHC of the device reset.
Does that make sense?
> > In hub_port_finish_reset, unconditionally clear the connect status
> > change (CSC) bit for USB 3.0 hubs when the port reset is done. If we
> > had to issue multiple warm resets for a device, that bit may have been
> > set if the device went into SS.Inactive and then was successfully warm
> > reset.
> >
> > Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
> > ---
> > drivers/usb/core/hub.c | 159 +++++++++++++++++++++--------------------------
> > 1 files changed, 71 insertions(+), 88 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index d8de712..a502857 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2538,80 +2538,39 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
>
> > - if (!(portstatus & USB_PORT_STAT_CONNECTION) ||
> > - hub_port_warm_reset_required(hub,
> > - portstatus))
> > - return -ENOTCONN;
> > + /* if we've finished resetting, then break out of
> > + * the loop
> > + */
> > + if (!(portstatus & USB_PORT_STAT_RESET) &&
>
> This test is redundant thanks to your 5/11 patch, right?
True.
> > @@ -2703,10 +2663,33 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
> > status);
> > }
> >
> > - /* return on disconnect or reset */
> > + /* Check for disconnect or reset */
> > if (status == 0 || status == -ENOTCONN || status == -ENODEV) {
> > - hub_port_finish_reset(hub, port1, udev, &status, warm);
> > - goto done;
> > + hub_port_finish_reset(hub, port1, udev, &status);
> > +
> > + if (!hub_is_superspeed(hub->hdev))
> > + goto done;
> > +
> > + /*
> > + * If a USB 3.0 device migrates from reset to an error
> > + * state, re-issue the warm reset.
> > + */
> > + if (hub_port_status(hub, port1,
> > + &portstatus, &portchange) < 0)
> > + goto done;
>
> Hmmm. Is there any reasonable way to get the portstatus value from
> hub_port_finish_reset instead of asking for it again?
Maybe. I'll take a look into it.
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
