On Thu, 25 Aug 2011, Andiry Xu wrote:
> Current waiting time for warm(BH) reset in hub_port_warm_reset() is too
> short for xHC host to complete the warm reset and report a BH reset
> change.
>
> This patch extends the waiting time for warm reset and merges the function
> into hub_port_reset(), so it can handle both cold reset and warm reset.
>
> This fixes the issue that driver fails to clear BH reset change and port is
> "dead".
This code looks a little odd.
> @@ -2046,28 +2047,43 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
> if (ret < 0)
> return ret;
>
> - /* Device went away? */
> - if (!(portstatus & USB_PORT_STAT_CONNECTION))
> - return -ENOTCONN;
> -
> - /* bomb out completely if the connection bounced */
> - if ((portchange & USB_PORT_STAT_C_CONNECTION))
> - return -ENOTCONN;
> -
> - /* if we`ve finished resetting, then break out of the loop */
> - if (!(portstatus & USB_PORT_STAT_RESET) &&
> - (portstatus & USB_PORT_STAT_ENABLE)) {
> - if (hub_is_wusb(hub))
> - udev->speed = USB_SPEED_WIRELESS;
> - else if (hub_is_superspeed(hub->hdev))
> - udev->speed = USB_SPEED_SUPER;
> - else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
> - udev->speed = USB_SPEED_HIGH;
> - else if (portstatus & USB_PORT_STAT_LOW_SPEED)
> - udev->speed = USB_SPEED_LOW;
> - else
> - udev->speed = USB_SPEED_FULL;
> - return 0;
> + if (!warm) {
What's wrong with running all the code above for a warm reset?
> + /* Device went away? */
> + if (!(portstatus & USB_PORT_STAT_CONNECTION))
> + return -ENOTCONN;
> +
> + /* bomb out completely if the connection bounced */
> + if ((portchange & USB_PORT_STAT_C_CONNECTION))
> + return -ENOTCONN;
> +
> + /* if we`ve finished resetting, then break out of
> + * the loop
> + */
> + if (!(portstatus & USB_PORT_STAT_RESET) &&
> + (portstatus & USB_PORT_STAT_ENABLE)) {
> + if (hub_is_wusb(hub))
> + udev->speed = USB_SPEED_WIRELESS;
> + else if (hub_is_superspeed(hub->hdev))
> + udev->speed = USB_SPEED_SUPER;
> + else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
> + udev->speed = USB_SPEED_HIGH;
> + else if (portstatus & USB_PORT_STAT_LOW_SPEED)
> + udev->speed = USB_SPEED_LOW;
> + else
> + udev->speed = USB_SPEED_FULL;
> + return 0;
> + }
> + } else {
> + if (portchange & USB_PORT_STAT_C_BH_RESET) {
> + clear_port_feature(hub->hdev, port1,
> + USB_PORT_FEAT_C_BH_PORT_RESET);
> +
> + if (portchange & USB_PORT_STAT_C_LINK_STATE)
> + clear_port_feature(hub->hdev, port1,
> + USB_PORT_FEAT_C_PORT_LINK_STATE);
> +
Why do you need to clear these features specially for a warm reset?
Can't they be cleared the same way as USB_PORT_FEAT_C_RESET?
> /* Reset the port */
> for (i = 0; i < PORT_RESET_TRIES; i++) {
> - status = set_port_feature(hub->hdev,
> - port1, USB_PORT_FEAT_RESET);
> - if (status)
> + if (!warm)
> + status = set_port_feature(hub->hdev,
> + port1, USB_PORT_FEAT_RESET);
> + else
> + status = set_port_feature(hub->hdev,
> + port1, USB_PORT_FEAT_BH_PORT_RESET);
You could simplify this:
status = set_port_feature(hub->hdev, port1, (warm ?
USB_PORT_FEAT_BH_PORT_RESET :
USB_PORT_FEAT_RESET));
> @@ -2113,9 +2143,13 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
> /* return on disconnect or reset */
> switch (status) {
> case 0:
> + if (warm)
> + goto clear_reset_change;
> +
Why use a goto instead of doing: if (!warm) {...}?
> /* TRSTRCY = 10 ms; plus some extra */
> msleep(10 + 40);
> update_devnum(udev, 0);
> + hcd = bus_to_hcd(udev->bus);
Why was this line moved?
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
