From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751096AbbKNHSs (ORCPT ); Sat, 14 Nov 2015 02:18:48 -0500 Received: from mga02.intel.com ([134.134.136.20]:11697 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750897AbbKNHSr (ORCPT ); Sat, 14 Nov 2015 02:18:47 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,292,1444719600"; d="scan'208";a="850937511" Subject: Re: [PATCH v2 1/3] usb: core: lpm: fix usb3_hardware_lpm sysfs node To: Alan Stern References: Cc: Mathias Nyman , Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org From: Lu Baolu Message-ID: <5646E056.6060403@linux.intel.com> Date: Sat, 14 Nov 2015 15:18:46 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> ... >>> >>>> --- 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