lkml.org 
[lkml]   [2015]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/3] usb: core: lpm: fix usb3_hardware_lpm sysfs node
From
Date


On 11/13/2015 11:28 PM, Alan Stern wrote:
> 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: 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.
> Then maybe you can add a comment explaining this.

Yes, I will add comments for 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)
> ...
> }

Yes, this looks better. I will refactor this part of code.

>
> Alan Stern
>

Thank you.
-Baolu




\
 
 \ /
  Last update: 2015-11-14 08:21    [W:0.084 / U:1.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site