Messages in this thread | | | Subject | Re: [PATCH v2 1/3] usb: core: lpm: fix usb3_hardware_lpm sysfs node | From | "Lu, Baolu" <> | Date | Fri, 13 Nov 2015 13:55:01 +0800 |
| |
On 11/13/2015 12:20 AM, Alan Stern wrote: > 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@vger.kernel.org >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > ... > >> --- 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?
My understand is that both hub and device can initiate LPM. As soon as usb_set_lpm_timeout(valid_timeout_value) returns 0, the hub-initiated LPM is enabled. Thus, LPM is enabled no matter the result of usb_set_device_initiated_lpm(). The only difference is whether device is able to initiate LPM.
On disable side, as soon as usb_set_lpm_timeout(0) return 0, hub initiated LPM is disabled. Hub will disallows link to enter U1/U2 as well, even device is initiating LPM. Hence LPM is disabled as soon as hub LPM timeout set to 0, no matter device-initiated LPM is disabled or not.
Thanks, -Baolu
> >> @@ -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 > >
| |