* [PATCH] USB: Don't enable LPM if it's already enabled
@ 2018-10-30 5:54 Kai-Heng Feng
2018-10-30 6:16 ` kbuild test robot
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2018-10-30 5:54 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, linux-kernel, 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.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Don't enable LPM if it's already enabled
2018-10-30 5:54 [PATCH] USB: Don't enable LPM if it's already enabled Kai-Heng Feng
@ 2018-10-30 6:16 ` kbuild test robot
2018-10-30 6:30 ` kbuild test robot
2018-10-30 14:24 ` Mathias Nyman
2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2018-10-30 6:16 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: kbuild-all, gregkh, linux-usb, linux-kernel, Kai-Heng Feng
[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33442 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Don't enable LPM if it's already enabled
2018-10-30 5:54 [PATCH] USB: Don't enable LPM if it's already enabled Kai-Heng Feng
2018-10-30 6:16 ` kbuild test robot
@ 2018-10-30 6:30 ` kbuild test robot
2018-10-30 14:24 ` Mathias Nyman
2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2018-10-30 6:30 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: kbuild-all, gregkh, linux-usb, linux-kernel, Kai-Heng Feng
[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]
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: i386-randconfig-x074-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=i386
All warnings (new ones prefixed by >>):
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 ||
~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35726 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Don't enable LPM if it's already enabled
2018-10-30 5:54 [PATCH] USB: Don't enable LPM if it's already enabled Kai-Heng Feng
2018-10-30 6:16 ` kbuild test robot
2018-10-30 6:30 ` kbuild test robot
@ 2018-10-30 14:24 ` Mathias Nyman
2018-10-30 15:00 ` Alan Stern
2 siblings, 1 reply; 6+ messages in thread
From: Mathias Nyman @ 2018-10-30 14:24 UTC (permalink / raw)
To: Kai-Heng Feng, gregkh; +Cc: linux-usb, linux-kernel, Alan Stern
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 <kai.heng.feng@canonical.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Don't enable LPM if it's already enabled
2018-10-30 14:24 ` Mathias Nyman
@ 2018-10-30 15:00 ` Alan Stern
2018-10-30 15:07 ` Kai Heng Feng
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2018-10-30 15:00 UTC (permalink / raw)
To: Mathias Nyman; +Cc: Kai-Heng Feng, gregkh, linux-usb, linux-kernel
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 <kai.heng.feng@canonical.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Don't enable LPM if it's already enabled
2018-10-30 15:00 ` Alan Stern
@ 2018-10-30 15:07 ` Kai Heng Feng
0 siblings, 0 replies; 6+ messages in thread
From: Kai Heng Feng @ 2018-10-30 15:07 UTC (permalink / raw)
To: Alan Stern; +Cc: Mathias Nyman, gregkh, linux-usb, linux-kernel
> On Oct 30, 2018, at 23:00, Alan Stern <stern@rowland.harvard.edu> 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 <kai.heng.feng@canonical.com>
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-30 15:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30 5:54 [PATCH] USB: Don't enable LPM if it's already enabled Kai-Heng Feng
2018-10-30 6:16 ` kbuild test robot
2018-10-30 6:30 ` kbuild test robot
2018-10-30 14:24 ` Mathias Nyman
2018-10-30 15:00 ` Alan Stern
2018-10-30 15:07 ` Kai Heng Feng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).