[PATCH] usb: core: disable USB2 LPM when suspending

[PATCH] usb: core: disable USB2 LPM when suspending

[AceLan Kao]

We found a S5 current leakage issue on Dell DW1820 WiFi/BT combo card
which uses Qualcomm QCA6174 SoC. It also comes with WiFi and BT failure
when encountered current leakage issue.
   1. Power on, both WiFi and BT work.
   2. Power off and found a current leakage issue(consumes ~0.5W)
   3. Power on, no WiFi and BT devices can be found in lspci and lsusb.
   4. Power off, there is no current leakage issue at S5.
   5. continue to 1.

From Qualcomm's report:
Based on the USB sniffer log, the difference between Linux and Windows
is USB LPM setting(no LPM transaction on Windows) which may leads to
the voltage leakage on Linux S5 state.

After checked the LPM related code and found, when system is going to
enter S5, it resumes the USB devices from runtime suspend and enables
USB2 LPM, and then it calls usb_dev_poweroff() -> usb_suspend(), and
leave USB2 LPM stays enabled.

Disable USB2 LPM in usb_suspend() fixes the issue mentioned above,
and try 30 times of s2idle, S3 and S5, the USB devices keep working
well. Disable USB2 LPM seems do no harm to the system.

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/usb/core/driver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index e76e95f62f76..ac5e60d7104f 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1463,6 +1463,9 @@ int usb_suspend(struct device *dev, pm_message_t msg)
 	struct usb_device	*udev = to_usb_device(dev);
 	int r;
 
+	if (udev->usb2_hw_lpm_enabled == 1)
+		usb_set_usb2_hardware_lpm(udev, 0);
+
 	unbind_no_pm_drivers_interfaces(udev);
 
 	/* From now on we are sure all drivers support suspend/resume
-- 
2.17.1

Re: [PATCH] usb: core: disable USB2 LPM when suspending

[Alan Stern]

On Thu, 20 Sep 2018, AceLan Kao wrote:

> We found a S5 current leakage issue on Dell DW1820 WiFi/BT combo card
> which uses Qualcomm QCA6174 SoC. It also comes with WiFi and BT failure
> when encountered current leakage issue.
>    1. Power on, both WiFi and BT work.
>    2. Power off and found a current leakage issue(consumes ~0.5W)
>    3. Power on, no WiFi and BT devices can be found in lspci and lsusb.
>    4. Power off, there is no current leakage issue at S5.
>    5. continue to 1.
> 
> From Qualcomm's report:
> Based on the USB sniffer log, the difference between Linux and Windows
> is USB LPM setting(no LPM transaction on Windows) which may leads to
> the voltage leakage on Linux S5 state.
> 
> After checked the LPM related code and found, when system is going to
> enter S5, it resumes the USB devices from runtime suspend and enables
> USB2 LPM, and then it calls usb_dev_poweroff() -> usb_suspend(), and
> leave USB2 LPM stays enabled.

But usb_suspend() -> usb_suspend_both() -> usb_suspend_device() -> 
generic_suspend() -> usb_port_suspend() -> 
usb_set_usb2_hardware_lpm(udev, 0).  So why does USB2 LPM stay enabled?

> Disable USB2 LPM in usb_suspend() fixes the issue mentioned above,
> and try 30 times of s2idle, S3 and S5, the USB devices keep working
> well. Disable USB2 LPM seems do no harm to the system.
> 
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>  drivers/usb/core/driver.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index e76e95f62f76..ac5e60d7104f 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1463,6 +1463,9 @@ int usb_suspend(struct device *dev, pm_message_t msg)
>  	struct usb_device	*udev = to_usb_device(dev);
>  	int r;
>  
> +	if (udev->usb2_hw_lpm_enabled == 1)
> +		usb_set_usb2_hardware_lpm(udev, 0);

At this point the device may still be in runtime suspend.  Is that 
really okay?

Alan Stern

> +
>  	unbind_no_pm_drivers_interfaces(udev);
>  
>  	/* From now on we are sure all drivers support suspend/resume
> 

Re: [PATCH] usb: core: disable USB2 LPM when suspending

[AceLan Kao]

Alan Stern <stern@rowland.harvard.edu> 於 2018年9月20日 週四 下午10:43寫道：
>
> On Thu, 20 Sep 2018, AceLan Kao wrote:
>
> > We found a S5 current leakage issue on Dell DW1820 WiFi/BT combo card
> > which uses Qualcomm QCA6174 SoC. It also comes with WiFi and BT failure
> > when encountered current leakage issue.
> >    1. Power on, both WiFi and BT work.
> >    2. Power off and found a current leakage issue(consumes ~0.5W)
> >    3. Power on, no WiFi and BT devices can be found in lspci and lsusb.
> >    4. Power off, there is no current leakage issue at S5.
> >    5. continue to 1.
> >
> > From Qualcomm's report:
> > Based on the USB sniffer log, the difference between Linux and Windows
> > is USB LPM setting(no LPM transaction on Windows) which may leads to
> > the voltage leakage on Linux S5 state.
> >
> > After checked the LPM related code and found, when system is going to
> > enter S5, it resumes the USB devices from runtime suspend and enables
> > USB2 LPM, and then it calls usb_dev_poweroff() -> usb_suspend(), and
> > leave USB2 LPM stays enabled.
>
> But usb_suspend() -> usb_suspend_both() -> usb_suspend_device() ->
> generic_suspend() -> usb_port_suspend() ->
> usb_set_usb2_hardware_lpm(udev, 0).  So why does USB2 LPM stay enabled?
Right, after checking the normal case, it calls
usb_set_usb2_hardware_lpm(udev, 0) eventually.
In the buggy machine, it doesn't reach that part and stops somewhere
after resume.
But unfortunately, I can't duplicate the issue after doing some
experiments anymore, so I can't get more info about that.
I'll dig further and get back to you when I have any progress.
>
> > Disable USB2 LPM in usb_suspend() fixes the issue mentioned above,
> > and try 30 times of s2idle, S3 and S5, the USB devices keep working
> > well. Disable USB2 LPM seems do no harm to the system.
> >
> > Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> > ---
> >  drivers/usb/core/driver.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index e76e95f62f76..ac5e60d7104f 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -1463,6 +1463,9 @@ int usb_suspend(struct device *dev, pm_message_t msg)
> >       struct usb_device       *udev = to_usb_device(dev);
> >       int r;
> >
> > +     if (udev->usb2_hw_lpm_enabled == 1)
> > +             usb_set_usb2_hardware_lpm(udev, 0);
>
> At this point the device may still be in runtime suspend.  Is that
> really okay?
>
> Alan Stern
>
> > +
> >       unbind_no_pm_drivers_interfaces(udev);
> >
> >       /* From now on we are sure all drivers support suspend/resume
> >
>