Hi Greg,
It turns out this patch is necessary (along with two other patches) to
fix Bugzilla #42472, where someone could not get their NEC xHCI host
controller to enumerate a USB 3.0 device. This patch is way too big for
3.0 stable, right?
I'd like to make sure this issue with warm reset taking too long gets
fixed in the 3.0 stable branch, so maybe we could add a smaller patch to
extend the msleep in the original hub_port_warm_reset function?
Sarah Sharp
On Tue, Aug 30, 2011 at 06:01:45PM +0800, 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 increases the waiting time for warm reset and merges the function
> into hub_port_reset(), so it can handle both cold reset and warm reset, and
> factor out hub_port_finish_reset() to make the code looks cleaner.
>
> This fixes the issue that driver fails to clear BH reset change and port is
> "dead".
>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/core/hub.c | 216 +++++++++++++++++++++++++-----------------------
> 1 files changed, 112 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 338f91f..b0217dc 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2025,11 +2025,12 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>
> #define HUB_ROOT_RESET_TIME 50 /* times are in msec */
> #define HUB_SHORT_RESET_TIME 10
> +#define HUB_BH_RESET_TIME 50
> #define HUB_LONG_RESET_TIME 200
> #define HUB_RESET_TIMEOUT 500
>
> static int hub_port_wait_reset(struct usb_hub *hub, int port1,
> - struct usb_device *udev, unsigned int delay)
> + struct usb_device *udev, unsigned int delay, bool warm)
> {
> int delay_time, ret;
> u16 portstatus;
> @@ -2046,28 +2047,39 @@ 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;
> + /*
> + * Some buggy devices require a warm reset to be issued even
> + * when the port appears not to be connected.
> + */
> + if (!warm) {
> + /* 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)
> + return 0;
> }
>
> /* switch to the long delay after two short delay failures */
> @@ -2075,35 +2087,84 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
> delay = HUB_LONG_RESET_TIME;
>
> dev_dbg (hub->intfdev,
> - "port %d not reset yet, waiting %dms\n",
> - port1, delay);
> + "port %d not %sreset yet, waiting %dms\n",
> + port1, warm ? "warm " : "", delay);
> }
>
> return -EBUSY;
> }
>
> +static void hub_port_finish_reset(struct usb_hub *hub, int port1,
> + struct usb_device *udev, int *status, bool warm)
> +{
> + switch (*status) {
> + case 0:
> + if (!warm) {
> + struct usb_hcd *hcd;
> + /* TRSTRCY = 10 ms; plus some extra */
> + msleep(10 + 40);
> + update_devnum(udev, 0);
> + hcd = bus_to_hcd(udev->bus);
> + if (hcd->driver->reset_device) {
> + *status = hcd->driver->reset_device(hcd, udev);
> + if (*status < 0) {
> + dev_err(&udev->dev, "Cannot reset "
> + "HCD device state\n");
> + break;
> + }
> + }
> + }
> + /* FALL THROUGH */
> + case -ENOTCONN:
> + case -ENODEV:
> + clear_port_feature(hub->hdev,
> + port1, USB_PORT_FEAT_C_RESET);
> + /* FIXME need disconnect() for NOTATTACHED device */
> + if (warm) {
> + clear_port_feature(hub->hdev, port1,
> + USB_PORT_FEAT_C_BH_PORT_RESET);
> + clear_port_feature(hub->hdev, port1,
> + USB_PORT_FEAT_C_PORT_LINK_STATE);
> + } else {
> + usb_set_device_state(udev, *status
> + ? USB_STATE_NOTATTACHED
> + : USB_STATE_DEFAULT);
> + }
> + break;
> + }
> +}
> +
> +/* Handle port reset and port warm(BH) reset (for USB3 protocol ports) */
> static int hub_port_reset(struct usb_hub *hub, int port1,
> - struct usb_device *udev, unsigned int delay)
> + struct usb_device *udev, unsigned int delay, bool warm)
> {
> int i, status;
> - struct usb_hcd *hcd;
>
> - hcd = bus_to_hcd(udev->bus);
> - /* Block EHCI CF initialization during the port reset.
> - * Some companion controllers don't like it when they mix.
> - */
> - down_read(&ehci_cf_port_reset_rwsem);
> + if (!warm) {
> + /* Block EHCI CF initialization during the port reset.
> + * Some companion controllers don't like it when they mix.
> + */
> + down_read(&ehci_cf_port_reset_rwsem);
> + } else {
> + if (!hub_is_superspeed(hub->hdev)) {
> + dev_err(hub->intfdev, "only USB3 hub support "
> + "warm reset\n");
> + return -EINVAL;
> + }
> + }
>
> /* Reset the port */
> for (i = 0; i < PORT_RESET_TRIES; i++) {
> - status = set_port_feature(hub->hdev,
> - port1, USB_PORT_FEAT_RESET);
> - if (status)
> + status = set_port_feature(hub->hdev, port1, (warm ?
> + USB_PORT_FEAT_BH_PORT_RESET :
> + USB_PORT_FEAT_RESET));
> + if (status) {
> dev_err(hub->intfdev,
> - "cannot reset port %d (err = %d)\n",
> - port1, status);
> - else {
> - status = hub_port_wait_reset(hub, port1, udev, delay);
> + "cannot %sreset port %d (err = %d)\n",
> + warm ? "warm " : "", port1, status);
> + } else {
> + status = hub_port_wait_reset(hub, port1, udev, delay,
> + warm);
> if (status && status != -ENOTCONN)
> dev_dbg(hub->intfdev,
> "port_wait_reset: err = %d\n",
> @@ -2111,34 +2172,14 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
> }
>
> /* return on disconnect or reset */
> - switch (status) {
> - case 0:
> - /* TRSTRCY = 10 ms; plus some extra */
> - msleep(10 + 40);
> - update_devnum(udev, 0);
> - if (hcd->driver->reset_device) {
> - status = hcd->driver->reset_device(hcd, udev);
> - if (status < 0) {
> - dev_err(&udev->dev, "Cannot reset "
> - "HCD device state\n");
> - break;
> - }
> - }
> - /* FALL THROUGH */
> - case -ENOTCONN:
> - case -ENODEV:
> - clear_port_feature(hub->hdev,
> - port1, USB_PORT_FEAT_C_RESET);
> - /* FIXME need disconnect() for NOTATTACHED device */
> - usb_set_device_state(udev, status
> - ? USB_STATE_NOTATTACHED
> - : USB_STATE_DEFAULT);
> + if (status == 0 || status == -ENOTCONN || status == -ENODEV) {
> + hub_port_finish_reset(hub, port1, udev, &status, warm);
> goto done;
> }
>
> dev_dbg (hub->intfdev,
> - "port %d not enabled, trying reset again...\n",
> - port1);
> + "port %d not enabled, trying %sreset again...\n",
> + port1, warm ? "warm " : "");
> delay = HUB_LONG_RESET_TIME;
> }
>
> @@ -2146,45 +2187,11 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
> "Cannot enable port %i. Maybe the USB cable is bad?\n",
> port1);
>
> - done:
> - up_read(&ehci_cf_port_reset_rwsem);
> - return status;
> -}
> -
> -/* Warm reset a USB3 protocol port */
> -static int hub_port_warm_reset(struct usb_hub *hub, int port)
> -{
> - int ret;
> - u16 portstatus, portchange;
> -
> - if (!hub_is_superspeed(hub->hdev)) {
> - dev_err(hub->intfdev, "only USB3 hub support warm reset\n");
> - return -EINVAL;
> - }
> -
> - /* Warm reset the port */
> - ret = set_port_feature(hub->hdev,
> - port, USB_PORT_FEAT_BH_PORT_RESET);
> - if (ret) {
> - dev_err(hub->intfdev, "cannot warm reset port %d\n", port);
> - return ret;
> - }
> -
> - msleep(20);
> - ret = hub_port_status(hub, port, &portstatus, &portchange);
> -
> - if (portchange & USB_PORT_STAT_C_RESET)
> - clear_port_feature(hub->hdev, port, USB_PORT_FEAT_C_RESET);
> -
> - if (portchange & USB_PORT_STAT_C_BH_RESET)
> - clear_port_feature(hub->hdev, port,
> - USB_PORT_FEAT_C_BH_PORT_RESET);
> -
> - if (portchange & USB_PORT_STAT_C_LINK_STATE)
> - clear_port_feature(hub->hdev, port,
> - USB_PORT_FEAT_C_PORT_LINK_STATE);
> +done:
> + if (!warm)
> + up_read(&ehci_cf_port_reset_rwsem);
>
> - return ret;
> + return status;
> }
>
> /* Check if a port is power on */
> @@ -2814,7 +2821,7 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
>
> /* Reset the device; full speed may morph to high speed */
> /* FIXME a USB 2.0 device may morph into SuperSpeed on reset. */
> - retval = hub_port_reset(hub, port1, udev, delay);
> + retval = hub_port_reset(hub, port1, udev, delay, false);
> if (retval < 0) /* error or disconnect */
> goto fail;
> /* success, speed is known */
> @@ -2944,7 +2951,7 @@ hub_port_init (struct usb_hub *hub, struct usb_device *udev, int port1,
> buf->bMaxPacketSize0;
> kfree(buf);
>
> - retval = hub_port_reset(hub, port1, udev, delay);
> + retval = hub_port_reset(hub, port1, udev, delay, false);
> if (retval < 0) /* error or disconnect */
> goto fail;
> if (oldspeed != udev->speed) {
> @@ -3565,7 +3572,8 @@ static void hub_events(void)
> (portstatus & USB_PORT_STAT_LINK_STATE)
> == USB_SS_PORT_LS_SS_INACTIVE) {
> dev_dbg(hub_dev, "warm reset port %d\n", i);
> - hub_port_warm_reset(hub, i);
> + hub_port_reset(hub, i, NULL,
> + HUB_BH_RESET_TIME, true);
> }
>
> if (connect_change)
> --
> 1.7.4.1
>
>
--
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
