LinuxLists.cc - [PATCH] USB: Don't enable LPM if it's already enabled

2018-10-30 05:55:43

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

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.

Signed-off-by: Kai-Heng Feng <[email protected]>
---
drivers/usb/core/driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 53564386ed57..e11d2eac76b6 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1901,7 +1901,8 @@ 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)
+ if (enable && !udev->usb2_hw_lpm_allowed ||
+ udev->usb2_hw_lpm_enabled == enable)
return 0;

if (hcd->driver->set_usb2_hw_lpm) {
--
2.19.1

2018-10-30 06:17:57

by kernel test robot

[permalink] [raw]

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

Hi Kai-Heng,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v4.19 next-20181029]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Kai-Heng-Feng/USB-Don-t-enable-LPM-if-it-s-already-enabled/20181030-135717
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-x002-201843 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

All warnings (new ones prefixed by >>):

In file included from include/linux/ioport.h:13:0,
from include/linux/device.h:15,
from drivers/usb/core/driver.c:28:
drivers/usb/core/driver.c: In function 'usb_set_usb2_hardware_lpm':
drivers/usb/core/driver.c:1904:13: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
if (enable && !udev->usb2_hw_lpm_allowed ||
~~~~~~~^~~~~~~~~~~
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/usb/core/driver.c:1904:2: note: in expansion of macro 'if'
if (enable && !udev->usb2_hw_lpm_allowed ||
^~
drivers/usb/core/driver.c:1904:13: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
if (enable && !udev->usb2_hw_lpm_allowed ||
~~~~~~~^~~~~~~~~~~
include/linux/compiler.h:58:42: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/usb/core/driver.c:1904:2: note: in expansion of macro 'if'
if (enable && !udev->usb2_hw_lpm_allowed ||
^~
drivers/usb/core/driver.c:1904:13: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
if (enable && !udev->usb2_hw_lpm_allowed ||
~~~~~~~^~~~~~~~~~~
include/linux/compiler.h:69:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^~~~
>> drivers/usb/core/driver.c:1904:2: note: in expansion of macro 'if'
if (enable && !udev->usb2_hw_lpm_allowed ||
^~

vim +/if +1904 drivers/usb/core/driver.c

1898
1899 int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
1900 {
1901 struct usb_hcd *hcd = bus_to_hcd(udev->bus);
1902 int ret = -EPERM;
1903
> 1904 if (enable && !udev->usb2_hw_lpm_allowed ||
1905 udev->usb2_hw_lpm_enabled == enable)
1906 return 0;
1907
1908 if (hcd->driver->set_usb2_hw_lpm) {
1909 ret = hcd->driver->set_usb2_hw_lpm(hcd, udev, enable);
1910 if (!ret)
1911 udev->usb2_hw_lpm_enabled = enable;
1912 }
1913
1914 return ret;
1915 }
1916

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (3.10 kB)
.config.gz (32.66 kB)
Download all attachments

2018-10-30 06:32:14

by kernel test robot

[permalink] [raw]

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

Attachments:

(No filename) (1.72 kB)
.config.gz (34.89 kB)
Download all attachments

2018-10-30 14:23:12

by Mathias Nyman

[permalink] [raw]

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

On 30.10.2018 07:54, 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.
>
> Signed-off-by: Kai-Heng Feng <[email protected]>
> ---
> drivers/usb/core/driver.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 53564386ed57..e11d2eac76b6 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1901,7 +1901,8 @@ 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)
> + if (enable && !udev->usb2_hw_lpm_allowed ||
> + udev->usb2_hw_lpm_enabled == enable)
> return 0;
>
> if (hcd->driver->set_usb2_hw_lpm) {
>

Something like that would probably work.

Would it make sense to skip USB2 hw LPM enabling in usb_port_resume() if
port was just reset (and thus LPM enabled)?

something like this:

--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3520,7 +3520,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)
+ if (udev->usb2_hw_lpm_capable == 1 && !udev->reset_resume)
usb_set_usb2_hardware_lpm(udev, 1);

/* Try to enable USB3 LTM */

-Mathias

2018-10-30 15:04:52

by Alan Stern

[permalink] [raw]

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

On Tue, 30 Oct 2018, Mathias Nyman wrote:

> On 30.10.2018 07:54, 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.
> >
> > Signed-off-by: Kai-Heng Feng <[email protected]>
> > ---
> > drivers/usb/core/driver.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index 53564386ed57..e11d2eac76b6 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -1901,7 +1901,8 @@ 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)
> > + if (enable && !udev->usb2_hw_lpm_allowed ||
> > + udev->usb2_hw_lpm_enabled == enable)
> > return 0;
> >
> > if (hcd->driver->set_usb2_hw_lpm) {
> >
>
> Something like that would probably work.
>
> Would it make sense to skip USB2 hw LPM enabling in usb_port_resume() if
> port was just reset (and thus LPM enabled)?
>
> something like this:
>
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3520,7 +3520,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)
> + if (udev->usb2_hw_lpm_capable == 1 && !udev->reset_resume)
> usb_set_usb2_hardware_lpm(udev, 1);
>
> /* Try to enable USB3 LTM */

Why not simply test whether udev->usb2_hw_lpm_enabled is already true?

if (udev->usb2_hw_lpm_capable == 1 &&
!udev->usb2_hw_lpm_enabled)

Or even put this extra test into usb_set_usb2_hardware_lpm().

Alan Stern

2018-10-30 15:09:25

by Kai-Heng Feng

[permalink] [raw]

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

> On Oct 30, 2018, at 23:00, Alan Stern <[email protected]> wrote:
>
> On Tue, 30 Oct 2018, Mathias Nyman wrote:
>
>> On 30.10.2018 07:54, 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.
>>>
>>> Signed-off-by: Kai-Heng Feng <[email protected]>
>>> ---
>>> drivers/usb/core/driver.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>>> index 53564386ed57..e11d2eac76b6 100644
>>> --- a/drivers/usb/core/driver.c
>>> +++ b/drivers/usb/core/driver.c
>>> @@ -1901,7 +1901,8 @@ 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)
>>> + if (enable && !udev->usb2_hw_lpm_allowed ||
>>> + udev->usb2_hw_lpm_enabled == enable)
>>> return 0;
>>>
>>> if (hcd->driver->set_usb2_hw_lpm) {
>>>
>>
>> Something like that would probably work.
>>
>> Would it make sense to skip USB2 hw LPM enabling in usb_port_resume() if
>> port was just reset (and thus LPM enabled)?
>>
>> something like this:
>>
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -3520,7 +3520,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)
>> + if (udev->usb2_hw_lpm_capable == 1 && !udev->reset_resume)
>> usb_set_usb2_hardware_lpm(udev, 1);
>>
>> /* Try to enable USB3 LTM */
>
> Why not simply test whether udev->usb2_hw_lpm_enabled is already true?
>
> if (udev->usb2_hw_lpm_capable == 1 &&
> !udev->usb2_hw_lpm_enabled)
>
> Or even put this extra test into usb_set_usb2_hardware_lpm().

I think it makes sense to merge all checks into usb_set_usb2_hardware_lpm().

I’ll resend one with this suggestion.

Kai-Heng

>
> Alan Stern