[PATCH v2 0/3] usb: core: lpm: add sysfs node for usb3 lpm permit

[PATCH v2 0/3] usb: core: lpm: add sysfs node for usb3 lpm permit

[Lu Baolu]

Hi,

This patch series is about to add a sysfs attribute, through which
users can disable or enable USB3 LPM (link power management) from
a USB port. Once LPM is disabled from the USB port, LPM between the
downstreaming device and the hub port will not be activated. This
helps users to use any LPM-unfriendly USB devices on any released
Linux kernels.

This patch series contains a fix and a cleanup as well.

Change log:
v1->v2:
Fix the "make htmldoc" warning reported by 0-day robot.

Thanks,
Baolu

Lu Baolu (3):
  usb: core: lpm: fix usb3_hardware_lpm sysfs node
  usb: core: lpm: add sysfs node for usb3 lpm permit
  usb: core: lpm: remove usb3_lpm_enabled in usb_device

 Documentation/ABI/testing/sysfs-bus-usb | 27 +++++++---
 Documentation/usb/power-management.txt  | 11 ++--
 drivers/usb/core/hub.c                  | 33 +++++++++---
 drivers/usb/core/hub.h                  |  5 +-
 drivers/usb/core/port.c                 | 89 ++++++++++++++++++++++++++++++++-
 drivers/usb/core/sysfs.c                | 31 ++++++++++--
 include/linux/usb.h                     |  6 ++-
 7 files changed, 174 insertions(+), 28 deletions(-)

-- 
2.1.4

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

[Lu Baolu]

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>
---
 Documentation/ABI/testing/sysfs-bus-usb | 16 +++++++++-------
 Documentation/usb/power-management.txt  | 11 ++++++-----
 drivers/usb/core/hub.c                  | 20 ++++++++++++++------
 drivers/usb/core/sysfs.c                | 31 ++++++++++++++++++++++++++-----
 include/linux/usb.h                     |  4 ++++
 5 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 3a4abfc..136ba17 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -134,19 +134,21 @@ Description:
 		enabled for the device. Developer can write y/Y/1 or n/N/0 to
 		the file to enable/disable the feature.
 
-What:		/sys/bus/usb/devices/.../power/usb3_hardware_lpm
-Date:		June 2015
+What:		/sys/bus/usb/devices/.../power/usb3_hardware_lpm_u1
+		/sys/bus/usb/devices/.../power/usb3_hardware_lpm_u2
+Date:		November 2015
 Contact:	Kevin Strasser <kevin.strasser@linux.intel.com>
+		Lu Baolu <baolu.lu@linux.intel.com>
 Description:
 		If CONFIG_PM is set and a USB 3.0 lpm-capable device is plugged
 		in to a xHCI host which supports link PM, it will check if U1
 		and U2 exit latencies have been set in the BOS descriptor; if
-		the check is is passed and the host supports USB3 hardware LPM,
+		the check is passed and the host supports USB3 hardware LPM,
 		USB3 hardware LPM will be enabled for the device and the USB
-		device directory will contain a file named
-		power/usb3_hardware_lpm. The file holds a string value (enable
-		or disable) indicating whether or not USB3 hardware LPM is
-		enabled for the device.
+		device directory will contain two files named
+		power/usb3_hardware_lpm_u1 and power/usb3_hardware_lpm_u2. These
+		files hold a string value (enable or disable) indicating whether
+		or not USB3 hardware LPM U1 or U2 is enabled for the device.
 
 What:		/sys/bus/usb/devices/.../removable
 Date:		February 2012
diff --git a/Documentation/usb/power-management.txt b/Documentation/usb/power-management.txt
index 4a15c90..0a94ffe 100644
--- a/Documentation/usb/power-management.txt
+++ b/Documentation/usb/power-management.txt
@@ -537,17 +537,18 @@ relevant attribute files are usb2_hardware_lpm and usb3_hardware_lpm.
 		can write y/Y/1 or n/N/0 to the file to	enable/disable
 		USB2 hardware LPM manually. This is for	test purpose mainly.
 
-	power/usb3_hardware_lpm
+	power/usb3_hardware_lpm_u1
+	power/usb3_hardware_lpm_u2
 
 		When a USB 3.0 lpm-capable device is plugged in to a
 		xHCI host which supports link PM, it will check if U1
 		and U2 exit latencies have been set in the BOS
 		descriptor; if the check is is passed and the host
 		supports USB3 hardware LPM, USB3 hardware LPM will be
-		enabled for the device and this file will be created.
-		The file holds a string value (enable or disable)
-		indicating whether or not USB3 hardware LPM is
-		enabled for the device.
+		enabled for the device and these files will be created.
+		The files hold a string value (enable or disable)
+		indicating whether or not USB3 hardware LPM U1 or U2
+		is enabled for the device.
 
 	USB Port Power Control
 	----------------------
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index bdeadc1..4eb7369 100644
--- 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;
+	}
 }
 
 /*
@@ -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;
 }
 
@@ -3959,8 +3971,6 @@ int usb_disable_lpm(struct usb_device *udev)
 	if (usb_disable_link_state(hcd, udev, USB3_LPM_U2))
 		goto enable_lpm;
 
-	udev->usb3_lpm_enabled = 0;
-
 	return 0;
 
 enable_lpm:
@@ -4018,8 +4028,6 @@ void usb_enable_lpm(struct usb_device *udev)
 
 	usb_enable_link_state(hcd, udev, USB3_LPM_U1);
 	usb_enable_link_state(hcd, udev, USB3_LPM_U2);
-
-	udev->usb3_lpm_enabled = 1;
 }
 EXPORT_SYMBOL_GPL(usb_enable_lpm);
 
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index d9ec2de..65b6e6b 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -531,7 +531,7 @@ static ssize_t usb2_lpm_besl_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(usb2_lpm_besl);
 
-static ssize_t usb3_hardware_lpm_show(struct device *dev,
+static ssize_t usb3_hardware_lpm_u1_show(struct device *dev,
 				      struct device_attribute *attr, char *buf)
 {
 	struct usb_device *udev = to_usb_device(dev);
@@ -539,7 +539,7 @@ static ssize_t usb3_hardware_lpm_show(struct device *dev,
 
 	usb_lock_device(udev);
 
-	if (udev->usb3_lpm_enabled)
+	if (udev->usb3_lpm_u1_enabled)
 		p = "enabled";
 	else
 		p = "disabled";
@@ -548,7 +548,26 @@ static ssize_t usb3_hardware_lpm_show(struct device *dev,
 
 	return sprintf(buf, "%s\n", p);
 }
-static DEVICE_ATTR_RO(usb3_hardware_lpm);
+static DEVICE_ATTR_RO(usb3_hardware_lpm_u1);
+
+static ssize_t usb3_hardware_lpm_u2_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct usb_device *udev = to_usb_device(dev);
+	const char *p;
+
+	usb_lock_device(udev);
+
+	if (udev->usb3_lpm_u2_enabled)
+		p = "enabled";
+	else
+		p = "disabled";
+
+	usb_unlock_device(udev);
+
+	return sprintf(buf, "%s\n", p);
+}
+static DEVICE_ATTR_RO(usb3_hardware_lpm_u2);
 
 static struct attribute *usb2_hardware_lpm_attr[] = {
 	&dev_attr_usb2_hardware_lpm.attr,
@@ -562,7 +581,8 @@ static struct attribute_group usb2_hardware_lpm_attr_group = {
 };
 
 static struct attribute *usb3_hardware_lpm_attr[] = {
-	&dev_attr_usb3_hardware_lpm.attr,
+	&dev_attr_usb3_hardware_lpm_u1.attr,
+	&dev_attr_usb3_hardware_lpm_u2.attr,
 	NULL,
 };
 static struct attribute_group usb3_hardware_lpm_attr_group = {
@@ -592,7 +612,8 @@ static int add_power_attributes(struct device *dev)
 		if (udev->usb2_hw_lpm_capable == 1)
 			rc = sysfs_merge_group(&dev->kobj,
 					&usb2_hardware_lpm_attr_group);
-		if (udev->lpm_capable == 1)
+		if (udev->speed == USB_SPEED_SUPER &&
+				udev->lpm_capable == 1)
 			rc = sysfs_merge_group(&dev->kobj,
 					&usb3_hardware_lpm_attr_group);
 	}
diff --git a/include/linux/usb.h b/include/linux/usb.h
index b9a2807..b79925d 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -511,6 +511,8 @@ struct usb3_lpm_parameters {
  * @usb2_hw_lpm_enabled: USB2 hardware LPM is enabled
  * @usb2_hw_lpm_allowed: Userspace allows USB 2.0 LPM to be enabled
  * @usb3_lpm_enabled: USB3 hardware LPM enabled
+ * @usb3_lpm_u1_enabled: USB3 hardware U1 LPM enabled
+ * @usb3_lpm_u2_enabled: USB3 hardware U2 LPM enabled
  * @string_langid: language ID for strings
  * @product: iProduct string, if present (static)
  * @manufacturer: iManufacturer string, if present (static)
@@ -584,6 +586,8 @@ struct usb_device {
 	unsigned usb2_hw_lpm_enabled:1;
 	unsigned usb2_hw_lpm_allowed:1;
 	unsigned usb3_lpm_enabled:1;
+	unsigned usb3_lpm_u1_enabled:1;
+	unsigned usb3_lpm_u2_enabled:1;
 	int string_langid;
 
 	/* static strings from the device */
-- 
2.1.4

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

[Alan Stern]

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?

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

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

[Lu, Baolu]

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.

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

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

[Alan Stern]

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.

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

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

[Lu Baolu]

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

[PATCH v2 2/3] usb: core: lpm: add sysfs node for usb3 lpm permit

[Lu Baolu]

USB3 LPM is default on in Linux kernel if both xHCI host controller
and the USB devices declare to be LPM-capable. Unfortunately, some
devices are known to work well with LPM disabled, but to be broken
if LPM is enabled, although it declares the LPM capability.  Users
won't be able to use this kind of devices, until someone puts them
in the kernel blacklist and gets the kernel upgraded.

This patch adds a sysfs node to permit or forbit USB3 LPM U1 or U2
entry for a port. The settings apply to both before and after device
enumeration. Supported values are "0" - neither u1 nor u2 permitted,
"u1" - only u1 is permitted, "u2" - only u2 is permitted, "u1_u2" -
both u1 and u2 are permitted. With this interface, users can use an
LPM-unfriendly USB device on a released Linux kernel.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-usb | 11 ++++
 drivers/usb/core/hub.c                  | 15 +++++-
 drivers/usb/core/hub.h                  |  5 +-
 drivers/usb/core/port.c                 | 89 ++++++++++++++++++++++++++++++++-
 4 files changed, 116 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 136ba17..0bd731c 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,17 @@ Description:
 		The file will read "hotplug", "wired" and "not used" if the
 		information is available, and "unknown" otherwise.
 
+What:		/sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
+Date:		November 2015
+Contact:	Lu Baolu <baolu.lu@linux.intel.com>
+Description:
+		Some USB3.0 devices are not friendly to USB3 LPM.  usb3_lpm_permit
+		attribute allows enabling/disabling usb3 lpm of a port. It takes
+		effect both before and after a usb device is enumerated. Supported
+		values are "0" if both u1 and u2 are NOT permitted, "u1" if only u1
+		is permitted, "u2" if only u2 is permitted, "u1_u2" if both u1 and
+		u2 are permitted.
+
 What:		/sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout
 Date:		May 2013
 Contact:	Mathias Nyman <mathias.nyman@linux.intel.com>
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4eb7369..9df568e 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4007,6 +4007,8 @@ EXPORT_SYMBOL_GPL(usb_unlocked_disable_lpm);
 void usb_enable_lpm(struct usb_device *udev)
 {
 	struct usb_hcd *hcd;
+	struct usb_hub *hub;
+	struct usb_port *port_dev;
 
 	if (!udev || !udev->parent ||
 			udev->speed != USB_SPEED_SUPER ||
@@ -4026,8 +4028,17 @@ void usb_enable_lpm(struct usb_device *udev)
 	if (udev->lpm_disable_count > 0)
 		return;
 
-	usb_enable_link_state(hcd, udev, USB3_LPM_U1);
-	usb_enable_link_state(hcd, udev, USB3_LPM_U2);
+	hub = usb_hub_to_struct_hub(udev->parent);
+	if (!hub)
+		return;
+
+	port_dev = hub->ports[udev->portnum - 1];
+
+	if (port_dev->usb3_lpm_u1_permit)
+		usb_enable_link_state(hcd, udev, USB3_LPM_U1);
+
+	if (port_dev->usb3_lpm_u2_permit)
+		usb_enable_link_state(hcd, udev, USB3_LPM_U2);
 }
 EXPORT_SYMBOL_GPL(usb_enable_lpm);
 
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 688817f..45d070d 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -92,6 +92,8 @@ struct usb_hub {
  * @status_lock: synchronize port_event() vs usb_port_{suspend|resume}
  * @portnum: port index num based one
  * @is_superspeed cache super-speed status
+ * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted.
+ * @usb3_lpm_u2_permit: whether USB3 U2 LPM is permitted.
  */
 struct usb_port {
 	struct usb_device *child;
@@ -104,6 +106,8 @@ struct usb_port {
 	struct mutex status_lock;
 	u8 portnum;
 	unsigned int is_superspeed:1;
+	unsigned int usb3_lpm_u1_permit:1;
+	unsigned int usb3_lpm_u2_permit:1;
 };
 
 #define to_usb_port(_dev) \
@@ -155,4 +159,3 @@ static inline int hub_port_debounce_be_stable(struct usb_hub *hub,
 {
 	return hub_port_debounce(hub, port1, false);
 }
-
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 2106183..cb18ce0 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -50,6 +50,72 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t usb3_lpm_permit_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	const char *p;
+
+	if (port_dev->usb3_lpm_u1_permit) {
+		if (port_dev->usb3_lpm_u2_permit)
+			p = "u1_u2";
+		else
+			p = "u1";
+	} else {
+		if (port_dev->usb3_lpm_u2_permit)
+			p = "u2";
+		else
+			p = "0";
+	}
+
+	return sprintf(buf, "%s\n", p);
+}
+
+static ssize_t usb3_lpm_permit_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	struct usb_device *udev = port_dev->child;
+	struct usb_hcd *hcd;
+
+	if (!strncmp(buf, "u1_u2", 5)) {
+		port_dev->usb3_lpm_u1_permit = 1;
+		port_dev->usb3_lpm_u2_permit = 1;
+
+	} else if (!strncmp(buf, "u1", 2)) {
+		port_dev->usb3_lpm_u1_permit = 1;
+		port_dev->usb3_lpm_u2_permit = 0;
+
+	} else if (!strncmp(buf, "u2", 2)) {
+		port_dev->usb3_lpm_u1_permit = 0;
+		port_dev->usb3_lpm_u2_permit = 1;
+
+	} else if (!strncmp(buf, "0", 1)) {
+		port_dev->usb3_lpm_u1_permit = 0;
+		port_dev->usb3_lpm_u2_permit = 0;
+	} else
+		return -EINVAL;
+
+	/* If device is connected to the port, disable or enable lpm
+	 * to make new u1 u2 setting take effect immediately.
+	 */
+	if (udev) {
+		hcd = bus_to_hcd(udev->bus);
+		if (!hcd)
+			return -EINVAL;
+		usb_lock_device(udev);
+		mutex_lock(hcd->bandwidth_mutex);
+		if (!usb_disable_lpm(udev))
+			usb_enable_lpm(udev);
+		mutex_unlock(hcd->bandwidth_mutex);
+		usb_unlock_device(udev);
+	}
+
+	return count;
+}
+static DEVICE_ATTR_RW(usb3_lpm_permit);
+
 static struct attribute *port_dev_attrs[] = {
 	&dev_attr_connect_type.attr,
 	NULL,
@@ -64,6 +130,21 @@ static const struct attribute_group *port_dev_group[] = {
 	NULL,
 };
 
+static struct attribute *port_dev_usb3_attrs[] = {
+	&dev_attr_usb3_lpm_permit.attr,
+	NULL,
+};
+
+static struct attribute_group port_dev_usb3_attr_grp = {
+	.attrs = port_dev_usb3_attrs,
+};
+
+static const struct attribute_group *port_dev_usb3_group[] = {
+	&port_dev_attr_grp,
+	&port_dev_usb3_attr_grp,
+	NULL,
+};
+
 static void usb_port_device_release(struct device *dev)
 {
 	struct usb_port *port_dev = to_usb_port(dev);
@@ -401,6 +482,7 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
 int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 {
 	struct usb_port *port_dev;
+	struct usb_device *hdev = hub->hdev;
 	int retval;
 
 	port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
@@ -417,7 +499,12 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 	port_dev->portnum = port1;
 	set_bit(port1, hub->power_bits);
 	port_dev->dev.parent = hub->intfdev;
-	port_dev->dev.groups = port_dev_group;
+	if (hub_is_superspeed(hdev)) {
+		port_dev->usb3_lpm_u1_permit = 1;
+		port_dev->usb3_lpm_u2_permit = 1;
+		port_dev->dev.groups = port_dev_usb3_group;
+	} else
+		port_dev->dev.groups = port_dev_group;
 	port_dev->dev.type = &usb_port_device_type;
 	port_dev->dev.driver = &usb_port_driver;
 	if (hub_is_superspeed(hub->hdev))
-- 
2.1.4

[PATCH v2 3/3] usb: core: lpm: remove usb3_lpm_enabled in usb_device

[Lu Baolu]

Commit 8306095fd2c1 ("USB: Disable USB 3.0 LPM in critical sections.")
adds usb3_lpm_enabled member to struct usb_device. There is no reference
to this member now. Hence, it could be removed.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/usb.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/usb.h b/include/linux/usb.h
index b79925d..89533ba 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -510,7 +510,6 @@ struct usb3_lpm_parameters {
  * @usb2_hw_lpm_besl_capable: device can perform USB2 hardware BESL LPM
  * @usb2_hw_lpm_enabled: USB2 hardware LPM is enabled
  * @usb2_hw_lpm_allowed: Userspace allows USB 2.0 LPM to be enabled
- * @usb3_lpm_enabled: USB3 hardware LPM enabled
  * @usb3_lpm_u1_enabled: USB3 hardware U1 LPM enabled
  * @usb3_lpm_u2_enabled: USB3 hardware U2 LPM enabled
  * @string_langid: language ID for strings
@@ -585,7 +584,6 @@ struct usb_device {
 	unsigned usb2_hw_lpm_besl_capable:1;
 	unsigned usb2_hw_lpm_enabled:1;
 	unsigned usb2_hw_lpm_allowed:1;
-	unsigned usb3_lpm_enabled:1;
 	unsigned usb3_lpm_u1_enabled:1;
 	unsigned usb3_lpm_u2_enabled:1;
 	int string_langid;
-- 
2.1.4

Re: [PATCH v2 3/3] usb: core: lpm: remove usb3_lpm_enabled in usb_device

[Alan Stern]

On Thu, 12 Nov 2015, Lu Baolu wrote:

> Commit 8306095fd2c1 ("USB: Disable USB 3.0 LPM in critical sections.")
> adds usb3_lpm_enabled member to struct usb_device. There is no reference
> to this member now. Hence, it could be removed.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Since patch 1/3 removes all the references to usb3_lpm_enabled, that 
patch should also remove the member itself.  In other words, you might 
as well fold this patch into that one.

Alan Stern

Re: [PATCH v2 3/3] usb: core: lpm: remove usb3_lpm_enabled in usb_device

[Lu, Baolu]

On 11/13/2015 12:20 AM, Alan Stern wrote:
> On Thu, 12 Nov 2015, Lu Baolu wrote:
>
>> Commit 8306095fd2c1 ("USB: Disable USB 3.0 LPM in critical sections.")
>> adds usb3_lpm_enabled member to struct usb_device. There is no reference
>> to this member now. Hence, it could be removed.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Since patch 1/3 removes all the references to usb3_lpm_enabled, that
> patch should also remove the member itself.  In other words, you might
> as well fold this patch into that one.

I was thinking that 1/3 is a fix patch. It could be back ported to
various kernels. If I merged this patch with 1/3, it might cause
problems, i.e. usb3_lpm_enabled is still used in that kernel.

Thanks,
-baolu

>
> Alan Stern
>
>