Re: [PATCH v2 1/3] usb: core: lpm: fix usb3_hardware_lpm sysfs node

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxxxx
Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
...

--- 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.

Thanks,
-Baolu


@@ -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



--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]