On Thu, 12 Nov 2015, Lu Baolu wrote:
> Commit 655fe4effe0f ("usbcore: add sysfs support to xHCI usb3
> hardware LPM") introduced usb3_hardware_lpm sysfs node. This
> doesn't show the correct status of USB3 U1 and U2 LPM status.
>
> This patch fixes this by replacing usb3_hardware_lpm with two
> nodes, usb3_hardware_lpm_u1 (for U1) and usb3_hardware_lpm_u2
> (for U2), and recording the U1/U2 LPM status in right places.
>
> This patch should be back-ported to kernels as old as 4.3,
> that contains Commit 655fe4effe0f ("usbcore: add sysfs support
> to xHCI usb3 hardware LPM").
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
...
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3875,17 +3875,23 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
> return;
> }
>
> - if (usb_set_lpm_timeout(udev, state, timeout))
> + ret = usb_set_lpm_timeout(udev, state, timeout);
> + if (ret)
> /* If we can't set the parent hub U1/U2 timeout,
> * device-initiated LPM won't be allowed either, so let the xHCI
> * host know that this link state won't be enabled.
> */
> hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
> -
> /* Only a configured device will accept the Set Feature U1/U2_ENABLE */
> else if (udev->actconfig)
> usb_set_device_initiated_lpm(udev, state, true);
>
> + if (!ret) {
> + if (state == USB3_LPM_U1)
> + udev->usb3_lpm_u1_enabled = 1;
> + else if (state == USB3_LPM_U2)
> + udev->usb3_lpm_u2_enabled = 1;
> + }
This doesn't look right at all. What happens if ret is 0 but the
device isn't configured? You'll set the usb3_lpm_u*_enabled flag even
though LPM isn't really enabled.
Don't you want to set these flags inside the
usb_set_device_initiated_lpm() function, where you know whether the
action succeeded? And leave this routine unchanged?
> @@ -3925,6 +3931,12 @@ static int usb_disable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
> dev_warn(&udev->dev, "Could not disable xHCI %s timeout, "
> "bus schedule bandwidth may be impacted.\n",
> usb3_lpm_names[state]);
> +
> + if (state == USB3_LPM_U1)
> + udev->usb3_lpm_u1_enabled = 0;
> + else if (state == USB3_LPM_U2)
> + udev->usb3_lpm_u2_enabled = 0;
> +
> return 0;
> }
And then this won't be necessary.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html