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.
> 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?
> 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?
> @@ -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?
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
