diff mbox series

USB: Don't enable LPM if it's already enabled

Message ID 20181030055452.25969-1-kai.heng.feng@canonical.com (mailing list archive)
State New, archived
Headers show
Series USB: Don't enable LPM if it's already enabled | expand

Commit Message

Kai-Heng Feng Oct. 30, 2018, 5:54 a.m. UTC
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(-)

Comments

kernel test robot Oct. 30, 2018, 6:16 a.m. UTC | #1
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
kernel test robot Oct. 30, 2018, 6:30 a.m. UTC | #2
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
Mathias Nyman Oct. 30, 2018, 2:24 p.m. UTC | #3
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
Alan Stern Oct. 30, 2018, 3 p.m. UTC | #4
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
Kai-Heng Feng Oct. 30, 2018, 3:07 p.m. UTC | #5
> 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
diff mbox series

Patch

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