From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932316AbbKLQUU (ORCPT ); Thu, 12 Nov 2015 11:20:20 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:35012 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S932281AbbKLQUP (ORCPT ); Thu, 12 Nov 2015 11:20:15 -0500 Date: Thu, 12 Nov 2015 11:20:14 -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: <1447294778-27917-2-git-send-email-baolu.lu@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 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? > @@ -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