[PATCH v4] USB: Don't enable LPM if it's already enabled

[PATCH v4] USB: Don't enable LPM if it's already enabled

[Kai-Heng Feng]

USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
after S3:
[ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
[ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)

After some experiments, I found that disabling LPM can workaround the
issue.

On some platforms, the USB power is cut during S3, so the driver uses
reset-resume to resume the device. During port resume, LPM gets enabled
twice, by usb_reset_and_verify_device() and usb_port_resume().

So let's enable LPM for just once, as this solves the issue for the
device in question.

Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
and usb_disable_usb2_hardware_lpm().

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v4:
 - Use usb_enable_usb2_hardware_lpm() and
   usb_disable_usb2_hardware_lpm() to control USB2 LPM.
v3:
 - Consolidate udev->usb2_hw_lpm_capable and udev->usb2_hw_lpm_enabled
  check to usb_set_usb2_hardware_lpm().
v2:
  - Check udev->usb2_hw_lpm_enabled before calling usb_port_resume().

 drivers/usb/core/driver.c  | 23 +++++++++++++++++++----
 drivers/usb/core/hub.c     | 16 ++++++----------
 drivers/usb/core/message.c |  3 +--
 drivers/usb/core/sysfs.c   |  5 ++++-
 drivers/usb/core/usb.h     | 10 ++++++++--
 5 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 53564386ed57..8987cec9549d 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1896,14 +1896,11 @@ int usb_runtime_idle(struct device *dev)
 	return -EBUSY;
 }
 
-int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
+static int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
 {
 	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
 	int ret = -EPERM;
 
-	if (enable && !udev->usb2_hw_lpm_allowed)
-		return 0;
-
 	if (hcd->driver->set_usb2_hw_lpm) {
 		ret = hcd->driver->set_usb2_hw_lpm(hcd, udev, enable);
 		if (!ret)
@@ -1913,6 +1910,24 @@ int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
 	return ret;
 }
 
+int usb_enable_usb2_hardware_lpm(struct usb_device *udev)
+{
+	if (!udev->usb2_hw_lpm_capable ||
+	    !udev->usb2_hw_lpm_allowed ||
+	    udev->usb2_hw_lpm_enabled)
+		return 0;
+
+	return usb_set_usb2_hardware_lpm(udev, 1);
+}
+
+int usb_disable_usb2_hardware_lpm(struct usb_device *udev)
+{
+	if (!udev->usb2_hw_lpm_enabled)
+		return 0;
+
+	return usb_set_usb2_hardware_lpm(udev, 0);
+}
+
 #endif /* CONFIG_PM */
 
 struct bus_type usb_bus_type = {
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 0f9381b69a3b..b4439ee2d144 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3210,8 +3210,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 	}
 
 	/* disable USB2 hardware LPM */
-	if (udev->usb2_hw_lpm_enabled == 1)
-		usb_set_usb2_hardware_lpm(udev, 0);
+	usb_disable_usb2_hardware_lpm(udev);
 
 	if (usb_disable_ltm(udev)) {
 		dev_err(&udev->dev, "Failed to disable LTM before suspend\n");
@@ -3249,8 +3248,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		usb_enable_ltm(udev);
  err_ltm:
 		/* Try to enable USB2 hardware LPM again */
-		if (udev->usb2_hw_lpm_capable == 1)
-			usb_set_usb2_hardware_lpm(udev, 1);
+		usb_enable_usb2_hardware_lpm(udev);
 
 		if (udev->do_remote_wakeup)
 			(void) usb_disable_remote_wakeup(udev);
@@ -3533,8 +3531,7 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 		hub_port_logical_disconnect(hub, port1);
 	} else  {
 		/* Try to enable USB2 hardware LPM */
-		if (udev->usb2_hw_lpm_capable == 1)
-			usb_set_usb2_hardware_lpm(udev, 1);
+		usb_enable_usb2_hardware_lpm(udev);
 
 		/* Try to enable USB3 LTM */
 		usb_enable_ltm(udev);
@@ -4425,7 +4422,7 @@ static void hub_set_initial_usb2_lpm_policy(struct usb_device *udev)
 	if ((udev->bos->ext_cap->bmAttributes & cpu_to_le32(USB_BESL_SUPPORT)) ||
 			connect_type == USB_PORT_CONNECT_TYPE_HARD_WIRED) {
 		udev->usb2_hw_lpm_allowed = 1;
-		usb_set_usb2_hardware_lpm(udev, 1);
+		usb_enable_usb2_hardware_lpm(udev);
 	}
 }
 
@@ -5638,8 +5635,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
 	/* Disable USB2 hardware LPM.
 	 * It will be re-enabled by the enumeration process.
 	 */
-	if (udev->usb2_hw_lpm_enabled == 1)
-		usb_set_usb2_hardware_lpm(udev, 0);
+	usb_disable_usb2_hardware_lpm(udev);
 
 	/* Disable LPM while we reset the device and reinstall the alt settings.
 	 * Device-initiated LPM, and system exit latency settings are cleared
@@ -5742,7 +5738,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
 
 done:
 	/* Now that the alt settings are re-installed, enable LTM and LPM. */
-	usb_set_usb2_hardware_lpm(udev, 1);
+	usb_enable_usb2_hardware_lpm(udev);
 	usb_unlocked_enable_lpm(udev);
 	usb_enable_ltm(udev);
 	usb_release_bos_descriptor(udev);
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index bfa5eda0cc26..4f33eb632a88 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1243,8 +1243,7 @@ void usb_disable_device(struct usb_device *dev, int skip_ep0)
 			dev->actconfig->interface[i] = NULL;
 		}
 
-		if (dev->usb2_hw_lpm_enabled == 1)
-			usb_set_usb2_hardware_lpm(dev, 0);
+		usb_disable_usb2_hardware_lpm(dev);
 		usb_unlocked_disable_lpm(dev);
 		usb_disable_ltm(dev);
 
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index ea18284dfa9a..7e88fdfe3cf5 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -528,7 +528,10 @@ static ssize_t usb2_hardware_lpm_store(struct device *dev,
 
 	if (!ret) {
 		udev->usb2_hw_lpm_allowed = value;
-		ret = usb_set_usb2_hardware_lpm(udev, value);
+		if (value)
+			ret = usb_enable_usb2_hardware_lpm(udev);
+		else
+			ret = usb_disable_usb2_hardware_lpm(udev);
 	}
 
 	usb_unlock_device(udev);
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 546a2219454b..d95a5358f73d 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -92,7 +92,8 @@ extern int usb_remote_wakeup(struct usb_device *dev);
 extern int usb_runtime_suspend(struct device *dev);
 extern int usb_runtime_resume(struct device *dev);
 extern int usb_runtime_idle(struct device *dev);
-extern int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable);
+extern int usb_enable_usb2_hardware_lpm(struct usb_device *udev);
+extern int usb_disable_usb2_hardware_lpm(struct usb_device *udev);
 
 #else
 
@@ -112,7 +113,12 @@ static inline int usb_autoresume_device(struct usb_device *udev)
 	return 0;
 }
 
-static inline int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
+static inline int usb_enable_usb2_hardware_lpm(struct usb_device *udev)
+{
+	return 0;
+}
+
+static inline int usb_disable_usb2_hardware_lpm(struct usb_device *udev)
 {
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v4] USB: Don't enable LPM if it's already enabled

[Kai Heng Feng]

Hi,

> On Dec 3, 2018, at 18:26, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> 
> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
> after S3:
> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
> 
> After some experiments, I found that disabling LPM can workaround the
> issue.
> 
> On some platforms, the USB power is cut during S3, so the driver uses
> reset-resume to resume the device. During port resume, LPM gets enabled
> twice, by usb_reset_and_verify_device() and usb_port_resume().
> 
> So let's enable LPM for just once, as this solves the issue for the
> device in question.
> 
> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
> and usb_disable_usb2_hardware_lpm().

Please review my new approach, hopefully this can be included in Linux v5.0. 

> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v4:
> - Use usb_enable_usb2_hardware_lpm() and
>   usb_disable_usb2_hardware_lpm() to control USB2 LPM.
> v3:
> - Consolidate udev->usb2_hw_lpm_capable and udev->usb2_hw_lpm_enabled
>  check to usb_set_usb2_hardware_lpm().
> v2:
>  - Check udev->usb2_hw_lpm_enabled before calling usb_port_resume().
> 
> drivers/usb/core/driver.c  | 23 +++++++++++++++++++----
> drivers/usb/core/hub.c     | 16 ++++++----------
> drivers/usb/core/message.c |  3 +--
> drivers/usb/core/sysfs.c   |  5 ++++-
> drivers/usb/core/usb.h     | 10 ++++++++--
> 5 files changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 53564386ed57..8987cec9549d 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1896,14 +1896,11 @@ int usb_runtime_idle(struct device *dev)
> 	return -EBUSY;
> }
> 
> -int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
> +static int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
> {
> 	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> 	int ret = -EPERM;
> 
> -	if (enable && !udev->usb2_hw_lpm_allowed)
> -		return 0;
> -
> 	if (hcd->driver->set_usb2_hw_lpm) {
> 		ret = hcd->driver->set_usb2_hw_lpm(hcd, udev, enable);
> 		if (!ret)
> @@ -1913,6 +1910,24 @@ int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
> 	return ret;
> }
> 
> +int usb_enable_usb2_hardware_lpm(struct usb_device *udev)
> +{
> +	if (!udev->usb2_hw_lpm_capable ||
> +	    !udev->usb2_hw_lpm_allowed ||
> +	    udev->usb2_hw_lpm_enabled)
> +		return 0;
> +
> +	return usb_set_usb2_hardware_lpm(udev, 1);
> +}
> +
> +int usb_disable_usb2_hardware_lpm(struct usb_device *udev)
> +{
> +	if (!udev->usb2_hw_lpm_enabled)
> +		return 0;
> +
> +	return usb_set_usb2_hardware_lpm(udev, 0);
> +}
> +
> #endif /* CONFIG_PM */
> 
> struct bus_type usb_bus_type = {
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 0f9381b69a3b..b4439ee2d144 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3210,8 +3210,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> 	}
> 
> 	/* disable USB2 hardware LPM */
> -	if (udev->usb2_hw_lpm_enabled == 1)
> -		usb_set_usb2_hardware_lpm(udev, 0);
> +	usb_disable_usb2_hardware_lpm(udev);
> 
> 	if (usb_disable_ltm(udev)) {
> 		dev_err(&udev->dev, "Failed to disable LTM before suspend\n");
> @@ -3249,8 +3248,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
> 		usb_enable_ltm(udev);
>  err_ltm:
> 		/* Try to enable USB2 hardware LPM again */
> -		if (udev->usb2_hw_lpm_capable == 1)
> -			usb_set_usb2_hardware_lpm(udev, 1);
> +		usb_enable_usb2_hardware_lpm(udev);
> 
> 		if (udev->do_remote_wakeup)
> 			(void) usb_disable_remote_wakeup(udev);
> @@ -3533,8 +3531,7 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> 		hub_port_logical_disconnect(hub, port1);
> 	} else  {
> 		/* Try to enable USB2 hardware LPM */
> -		if (udev->usb2_hw_lpm_capable == 1)
> -			usb_set_usb2_hardware_lpm(udev, 1);
> +		usb_enable_usb2_hardware_lpm(udev);
> 
> 		/* Try to enable USB3 LTM */
> 		usb_enable_ltm(udev);
> @@ -4425,7 +4422,7 @@ static void hub_set_initial_usb2_lpm_policy(struct usb_device *udev)
> 	if ((udev->bos->ext_cap->bmAttributes & cpu_to_le32(USB_BESL_SUPPORT)) ||
> 			connect_type == USB_PORT_CONNECT_TYPE_HARD_WIRED) {
> 		udev->usb2_hw_lpm_allowed = 1;
> -		usb_set_usb2_hardware_lpm(udev, 1);
> +		usb_enable_usb2_hardware_lpm(udev);
> 	}
> }
> 
> @@ -5638,8 +5635,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> 	/* Disable USB2 hardware LPM.
> 	 * It will be re-enabled by the enumeration process.
> 	 */
> -	if (udev->usb2_hw_lpm_enabled == 1)
> -		usb_set_usb2_hardware_lpm(udev, 0);
> +	usb_disable_usb2_hardware_lpm(udev);
> 
> 	/* Disable LPM while we reset the device and reinstall the alt settings.
> 	 * Device-initiated LPM, and system exit latency settings are cleared
> @@ -5742,7 +5738,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> 
> done:
> 	/* Now that the alt settings are re-installed, enable LTM and LPM. */
> -	usb_set_usb2_hardware_lpm(udev, 1);
> +	usb_enable_usb2_hardware_lpm(udev);
> 	usb_unlocked_enable_lpm(udev);
> 	usb_enable_ltm(udev);
> 	usb_release_bos_descriptor(udev);
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index bfa5eda0cc26..4f33eb632a88 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1243,8 +1243,7 @@ void usb_disable_device(struct usb_device *dev, int skip_ep0)
> 			dev->actconfig->interface[i] = NULL;
> 		}
> 
> -		if (dev->usb2_hw_lpm_enabled == 1)
> -			usb_set_usb2_hardware_lpm(dev, 0);
> +		usb_disable_usb2_hardware_lpm(dev);
> 		usb_unlocked_disable_lpm(dev);
> 		usb_disable_ltm(dev);
> 
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> index ea18284dfa9a..7e88fdfe3cf5 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -528,7 +528,10 @@ static ssize_t usb2_hardware_lpm_store(struct device *dev,
> 
> 	if (!ret) {
> 		udev->usb2_hw_lpm_allowed = value;
> -		ret = usb_set_usb2_hardware_lpm(udev, value);
> +		if (value)
> +			ret = usb_enable_usb2_hardware_lpm(udev);
> +		else
> +			ret = usb_disable_usb2_hardware_lpm(udev);
> 	}
> 
> 	usb_unlock_device(udev);
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index 546a2219454b..d95a5358f73d 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -92,7 +92,8 @@ extern int usb_remote_wakeup(struct usb_device *dev);
> extern int usb_runtime_suspend(struct device *dev);
> extern int usb_runtime_resume(struct device *dev);
> extern int usb_runtime_idle(struct device *dev);
> -extern int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable);
> +extern int usb_enable_usb2_hardware_lpm(struct usb_device *udev);
> +extern int usb_disable_usb2_hardware_lpm(struct usb_device *udev);
> 
> #else
> 
> @@ -112,7 +113,12 @@ static inline int usb_autoresume_device(struct usb_device *udev)
> 	return 0;
> }
> 
> -static inline int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
> +static inline int usb_enable_usb2_hardware_lpm(struct usb_device *udev)
> +{
> +	return 0;
> +}
> +
> +static inline int usb_disable_usb2_hardware_lpm(struct usb_device *udev)
> {
> 	return 0;
> }
> -- 
> 2.17.1
> 

Re: [PATCH v4] USB: Don't enable LPM if it's already enabled

[Alan Stern]

On Mon, 7 Jan 2019, Kai Heng Feng wrote:

> Hi,
> 
> > On Dec 3, 2018, at 18:26, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> > 
> > USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
> > after S3:
> > [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
> > [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
> > 
> > After some experiments, I found that disabling LPM can workaround the
> > issue.
> > 
> > On some platforms, the USB power is cut during S3, so the driver uses
> > reset-resume to resume the device. During port resume, LPM gets enabled
> > twice, by usb_reset_and_verify_device() and usb_port_resume().
> > 
> > So let's enable LPM for just once, as this solves the issue for the
> > device in question.
> > 
> > Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
> > and usb_disable_usb2_hardware_lpm().
> 
> Please review my new approach, hopefully this can be included in Linux v5.0. 
> 
> > 
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v4:
> > - Use usb_enable_usb2_hardware_lpm() and
> >   usb_disable_usb2_hardware_lpm() to control USB2 LPM.
> > v3:
> > - Consolidate udev->usb2_hw_lpm_capable and udev->usb2_hw_lpm_enabled
> >  check to usb_set_usb2_hardware_lpm().
> > v2:
> >  - Check udev->usb2_hw_lpm_enabled before calling usb_port_resume().
> > 
> > drivers/usb/core/driver.c  | 23 +++++++++++++++++++----
> > drivers/usb/core/hub.c     | 16 ++++++----------
> > drivers/usb/core/message.c |  3 +--
> > drivers/usb/core/sysfs.c   |  5 ++++-
> > drivers/usb/core/usb.h     | 10 ++++++++--
> > 5 files changed, 38 insertions(+), 19 deletions(-)

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH v4] USB: Don't enable LPM if it's already enabled

[Greg KH]

On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote:
> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
> after S3:
> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
> 
> After some experiments, I found that disabling LPM can workaround the
> issue.
> 
> On some platforms, the USB power is cut during S3, so the driver uses
> reset-resume to resume the device. During port resume, LPM gets enabled
> twice, by usb_reset_and_verify_device() and usb_port_resume().
> 
> So let's enable LPM for just once, as this solves the issue for the
> device in question.
> 
> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
> and usb_disable_usb2_hardware_lpm().
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

What kernel patch does this one "fix"?  Adding a "Fixes:" tag would be
good to try to figure out how far back in the kernel releases this
should be backported.

thanks,

greg k-h

Re: [PATCH v4] USB: Don't enable LPM if it's already enabled

[Kai Heng Feng]

> On Jan 8, 2019, at 00:16, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote:
>> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
>> after S3:
>> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
>> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
>> 
>> After some experiments, I found that disabling LPM can workaround the
>> issue.
>> 
>> On some platforms, the USB power is cut during S3, so the driver uses
>> reset-resume to resume the device. During port resume, LPM gets enabled
>> twice, by usb_reset_and_verify_device() and usb_port_resume().
>> 
>> So let's enable LPM for just once, as this solves the issue for the
>> device in question.
>> 
>> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
>> and usb_disable_usb2_hardware_lpm().
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> What kernel patch does this one "fix"?  Adding a "Fixes:" tag would be
> good to try to figure out how far back in the kernel releases this
> should be backported.

Fixes: de68bab4fa96 ("usb: Don't enable USB 2.0 Link PM by default.”)

The usb_set_usb2_hardware_lpm() was added to usb_reset_and_verify_device()
by this commit.

Kai-Heng

> 
> thanks,
> 
> greg k-h

Re: [PATCH v4] USB: Don't enable LPM if it's already enabled

[Greg KH]

On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote:
> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
> after S3:
> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
> 
> After some experiments, I found that disabling LPM can workaround the
> issue.
> 
> On some platforms, the USB power is cut during S3, so the driver uses
> reset-resume to resume the device. During port resume, LPM gets enabled
> twice, by usb_reset_and_verify_device() and usb_port_resume().
> 
> So let's enable LPM for just once, as this solves the issue for the
> device in question.
> 
> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
> and usb_disable_usb2_hardware_lpm().

I thought I asked for this to be two different patches.  One that does
the "consolidation", and then one that fixes the bug.  You are mixing
two different things here together, making it harder to review.

Can you please break this up and send a patch series, with the correct
"Fixes:" tag added to the second patch that actually fixes the issue?

thanks,

greg k-h

Re: [PATCH v4] USB: Don't enable LPM if it's already enabled

[Kai Heng Feng]

> On Jan 8, 2019, at 11:41 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote:
>> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
>> after S3:
>> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
>> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
>> 
>> After some experiments, I found that disabling LPM can workaround the
>> issue.
>> 
>> On some platforms, the USB power is cut during S3, so the driver uses
>> reset-resume to resume the device. During port resume, LPM gets enabled
>> twice, by usb_reset_and_verify_device() and usb_port_resume().
>> 
>> So let's enable LPM for just once, as this solves the issue for the
>> device in question.
>> 
>> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
>> and usb_disable_usb2_hardware_lpm().
> 
> I thought I asked for this to be two different patches.  One that does
> the "consolidation", and then one that fixes the bug.  You are mixing
> two different things here together, making it harder to review.
> 
> Can you please break this up and send a patch series, with the correct
> "Fixes:" tag added to the second patch that actually fixes the issue?

The consolidation itself is the fix, so I am not sure how to break this up.

In reset-resume case, LPM gets enabled twice, by
usb_reset_and_verify_device() and usb_port_resume().

If it’s a normal resume, LPM only gets enabled once, by
usb_port_resume().

Since all three checks (capable, allowed and enabled) are merged to
a single place, enabling LPM twice can be avoided, hence fixing the
issue.

Kai-Heng

> 
> thanks,
> 
> greg k-h

Re: [PATCH v4] USB: Don't enable LPM if it's already enabled

[Alan Stern]

On Wed, 9 Jan 2019, Kai Heng Feng wrote:

> > On Jan 8, 2019, at 11:41 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote:
> >> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
> >> after S3:
> >> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
> >> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
> >> 
> >> After some experiments, I found that disabling LPM can workaround the
> >> issue.
> >> 
> >> On some platforms, the USB power is cut during S3, so the driver uses
> >> reset-resume to resume the device. During port resume, LPM gets enabled
> >> twice, by usb_reset_and_verify_device() and usb_port_resume().
> >> 
> >> So let's enable LPM for just once, as this solves the issue for the
> >> device in question.
> >> 
> >> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
> >> and usb_disable_usb2_hardware_lpm().
> > 
> > I thought I asked for this to be two different patches.  One that does
> > the "consolidation", and then one that fixes the bug.  You are mixing
> > two different things here together, making it harder to review.
> > 
> > Can you please break this up and send a patch series, with the correct
> > "Fixes:" tag added to the second patch that actually fixes the issue?
> 
> The consolidation itself is the fix, so I am not sure how to break this up.
> 
> In reset-resume case, LPM gets enabled twice, by
> usb_reset_and_verify_device() and usb_port_resume().
> 
> If it’s a normal resume, LPM only gets enabled once, by
> usb_port_resume().
> 
> Since all three checks (capable, allowed and enabled) are merged to
> a single place, enabling LPM twice can be avoided, hence fixing the
> issue.

One approach would be to have the first patch add the new functions and
change the code to call them instead of the original function, but
leaves the checks the way they are now.  Then the second patch could
add the checks to the new functions and remove them from the call
sites.

I'm not sure if it's worth the effort to break things up that way, but 
it might make Greg happy.  :-)

Alan Stern

Re: [PATCH v4] USB: Don't enable LPM if it's already enabled

[Greg KH]

On Tue, Jan 08, 2019 at 12:34:17PM -0500, Alan Stern wrote:
> On Wed, 9 Jan 2019, Kai Heng Feng wrote:
> 
> > > On Jan 8, 2019, at 11:41 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > 
> > > On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote:
> > >> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
> > >> after S3:
> > >> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
> > >> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
> > >> 
> > >> After some experiments, I found that disabling LPM can workaround the
> > >> issue.
> > >> 
> > >> On some platforms, the USB power is cut during S3, so the driver uses
> > >> reset-resume to resume the device. During port resume, LPM gets enabled
> > >> twice, by usb_reset_and_verify_device() and usb_port_resume().
> > >> 
> > >> So let's enable LPM for just once, as this solves the issue for the
> > >> device in question.
> > >> 
> > >> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
> > >> and usb_disable_usb2_hardware_lpm().
> > > 
> > > I thought I asked for this to be two different patches.  One that does
> > > the "consolidation", and then one that fixes the bug.  You are mixing
> > > two different things here together, making it harder to review.
> > > 
> > > Can you please break this up and send a patch series, with the correct
> > > "Fixes:" tag added to the second patch that actually fixes the issue?
> > 
> > The consolidation itself is the fix, so I am not sure how to break this up.
> > 
> > In reset-resume case, LPM gets enabled twice, by
> > usb_reset_and_verify_device() and usb_port_resume().
> > 
> > If it’s a normal resume, LPM only gets enabled once, by
> > usb_port_resume().
> > 
> > Since all three checks (capable, allowed and enabled) are merged to
> > a single place, enabling LPM twice can be avoided, hence fixing the
> > issue.
> 
> One approach would be to have the first patch add the new functions and
> change the code to call them instead of the original function, but
> leaves the checks the way they are now.  Then the second patch could
> add the checks to the new functions and remove them from the call
> sites.

Yes, that is what I was looking for.  That way, if the "change" really
does cause problems, it is easier to revert/fix.

thanks,

greg k-h