On Fri, 13 Nov 2015, Lu, Baolu wrote:
> 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: sta...@vger.kernel.org
> >> Signed-off-by: Lu Baolu <baolu...@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.
Then maybe you can add a comment explaining this.
The patch still looks strange, though. Your new code does this:
ret = usb_set_lpm_timeout(...);
if (ret)
...
else if (udev->actconfig)
...
if (!ret) {
if (state == USB3_LPM_U1)
...
}
It would be better to do this:
if (usb_set_lpm_timeout(...)) {
...
} else {
if (udev->actconfig)
...
if (state == USB3_LPM_U1)
...
}
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
