From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754417AbbKMP2t (ORCPT ); Fri, 13 Nov 2015 10:28:49 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:37744 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1754632AbbKMP2s (ORCPT ); Fri, 13 Nov 2015 10:28:48 -0500 Date: Fri, 13 Nov 2015 10:28:47 -0500 (EST) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: "Lu, Baolu" cc: Mathias Nyman , Greg Kroah-Hartman , , , Subject: Re: [PATCH v2 1/3] usb: core: lpm: fix usb3_hardware_lpm sysfs node In-Reply-To: <56457B35.6000205@linux.intel.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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