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

[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