[PATCH v2] USB: Disable USB2 LPM at shutdown

[PATCH v2] USB: Disable USB2 LPM at shutdown

[Kai-Heng Feng]

The QCA Rome USB Bluetooth controller has several issues once LPM gets
enabled:
- Fails to get enumerated in coldboot. [1]
- Drains more power (~ 0.2W) when the system is in S5. [2]
- Disappears after a warmboot. [2]

The issue happens because the device lingers at LPM L1 in S5, so device
can't get enumerated even after a reboot.

Disable LPM at shutdown to solve the issue.

[1] https://bugs.launchpad.net/bugs/1757218
[2] https://patchwork.kernel.org/patch/10607097/

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2: Use new LPM helpers.

 drivers/usb/core/port.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a06a4b5fbb1..bbbb35fa639f 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -285,6 +285,14 @@ static int usb_port_runtime_suspend(struct device *dev)
 }
 #endif
 
+static void usb_port_shutdown(struct device *dev)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+
+	if (port_dev->child)
+		usb_disable_usb2_hardware_lpm(port_dev->child);
+}
+
 static const struct dev_pm_ops usb_port_pm_ops = {
 #ifdef CONFIG_PM
 	.runtime_suspend =	usb_port_runtime_suspend,
@@ -301,6 +309,7 @@ struct device_type usb_port_device_type = {
 static struct device_driver usb_port_driver = {
 	.name = "usb",
 	.owner = THIS_MODULE,
+	.shutdown = usb_port_shutdown,
 };
 
 static int link_peers(struct usb_port *left, struct usb_port *right)
-- 
2.17.1

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Greg KH]

On Thu, Jan 24, 2019 at 02:16:43PM +0800, Kai-Heng Feng wrote:
> The QCA Rome USB Bluetooth controller has several issues once LPM gets
> enabled:
> - Fails to get enumerated in coldboot. [1]
> - Drains more power (~ 0.2W) when the system is in S5. [2]
> - Disappears after a warmboot. [2]
> 
> The issue happens because the device lingers at LPM L1 in S5, so device
> can't get enumerated even after a reboot.
> 
> Disable LPM at shutdown to solve the issue.
> 
> [1] https://bugs.launchpad.net/bugs/1757218
> [2] https://patchwork.kernel.org/patch/10607097/
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2: Use new LPM helpers.
> 
>  drivers/usb/core/port.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 1a06a4b5fbb1..bbbb35fa639f 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -285,6 +285,14 @@ static int usb_port_runtime_suspend(struct device *dev)
>  }
>  #endif
>  
> +static void usb_port_shutdown(struct device *dev)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +
> +	if (port_dev->child)
> +		usb_disable_usb2_hardware_lpm(port_dev->child);
> +}
> +
>  static const struct dev_pm_ops usb_port_pm_ops = {
>  #ifdef CONFIG_PM
>  	.runtime_suspend =	usb_port_runtime_suspend,
> @@ -301,6 +309,7 @@ struct device_type usb_port_device_type = {
>  static struct device_driver usb_port_driver = {
>  	.name = "usb",
>  	.owner = THIS_MODULE,
> +	.shutdown = usb_port_shutdown,
>  };

So you now do this for all ports in the system, no matter what is
plugged in or not.  Are you _SURE_ you want to do that?  It seems like a
big hammer to solve just one single device's problems.

thanks,

greg k-h

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Kai-Heng Feng]

> On Jan 30, 2019, at 16:21, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Thu, Jan 24, 2019 at 02:16:43PM +0800, Kai-Heng Feng wrote:
>> The QCA Rome USB Bluetooth controller has several issues once LPM gets
>> enabled:
>> - Fails to get enumerated in coldboot. [1]
>> - Drains more power (~ 0.2W) when the system is in S5. [2]
>> - Disappears after a warmboot. [2]
>> 
>> The issue happens because the device lingers at LPM L1 in S5, so device
>> can't get enumerated even after a reboot.
>> 
>> Disable LPM at shutdown to solve the issue.
>> 
>> [1] https://bugs.launchpad.net/bugs/1757218
>> [2] https://patchwork.kernel.org/patch/10607097/
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> v2: Use new LPM helpers.
>> 
>> drivers/usb/core/port.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 1a06a4b5fbb1..bbbb35fa639f 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -285,6 +285,14 @@ static int usb_port_runtime_suspend(struct device *dev)
>> }
>> #endif
>> 
>> +static void usb_port_shutdown(struct device *dev)
>> +{
>> +	struct usb_port *port_dev = to_usb_port(dev);
>> +
>> +	if (port_dev->child)
>> +		usb_disable_usb2_hardware_lpm(port_dev->child);
>> +}
>> +
>> static const struct dev_pm_ops usb_port_pm_ops = {
>> #ifdef CONFIG_PM
>> 	.runtime_suspend =	usb_port_runtime_suspend,
>> @@ -301,6 +309,7 @@ struct device_type usb_port_device_type = {
>> static struct device_driver usb_port_driver = {
>> 	.name = "usb",
>> 	.owner = THIS_MODULE,
>> +	.shutdown = usb_port_shutdown,
>> };
> 
> So you now do this for all ports in the system, no matter what is
> plugged in or not.  Are you _SURE_ you want to do that?  It seems like a
> big hammer to solve just one single device's problems.

Yes I think this should be universally applied.

I don’t think the bug only happens to one device. Users won’t find this
unless they connect their laptop to a power meter.

Platform may not completely cut off USB bus power during shutdown,
so the device transits to L1 after system shutdown. Now xHC is disabled
so nothing can bring it back to L0 or L2.

Kai-Heng

> 
> thanks,
> 
> greg k-h

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Kai-Heng Feng]

at 00:01, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

>
>
>> On Jan 30, 2019, at 16:21, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> On Thu, Jan 24, 2019 at 02:16:43PM +0800, Kai-Heng Feng wrote:
>>> The QCA Rome USB Bluetooth controller has several issues once LPM gets
>>> enabled:
>>> - Fails to get enumerated in coldboot. [1]
>>> - Drains more power (~ 0.2W) when the system is in S5. [2]
>>> - Disappears after a warmboot. [2]
>>>
>>> The issue happens because the device lingers at LPM L1 in S5, so device
>>> can't get enumerated even after a reboot.
>>>
>>> Disable LPM at shutdown to solve the issue.
>>>
>>> [1] https://bugs.launchpad.net/bugs/1757218
>>> [2] https://patchwork.kernel.org/patch/10607097/
>>>
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> ---
>>> v2: Use new LPM helpers.
>>>
>>> drivers/usb/core/port.c | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>>> index 1a06a4b5fbb1..bbbb35fa639f 100644
>>> --- a/drivers/usb/core/port.c
>>> +++ b/drivers/usb/core/port.c
>>> @@ -285,6 +285,14 @@ static int usb_port_runtime_suspend(struct device  
>>> *dev)
>>> }
>>> #endif
>>>
>>> +static void usb_port_shutdown(struct device *dev)
>>> +{
>>> +	struct usb_port *port_dev = to_usb_port(dev);
>>> +
>>> +	if (port_dev->child)
>>> +		usb_disable_usb2_hardware_lpm(port_dev->child);
>>> +}
>>> +
>>> static const struct dev_pm_ops usb_port_pm_ops = {
>>> #ifdef CONFIG_PM
>>> 	.runtime_suspend =	usb_port_runtime_suspend,
>>> @@ -301,6 +309,7 @@ struct device_type usb_port_device_type = {
>>> static struct device_driver usb_port_driver = {
>>> 	.name = "usb",
>>> 	.owner = THIS_MODULE,
>>> +	.shutdown = usb_port_shutdown,
>>> };
>>
>> So you now do this for all ports in the system, no matter what is
>> plugged in or not.  Are you _SURE_ you want to do that?  It seems like a
>> big hammer to solve just one single device's problems.
>
> Yes I think this should be universally applied.
>
> I don’t think the bug only happens to one device. Users won’t find this
> unless they connect their laptop to a power meter.
>
> Platform may not completely cut off USB bus power during shutdown,
> so the device transits to L1 after system shutdown. Now xHC is disabled
> so nothing can bring it back to L0 or L2.

It would be great if someone can come up with better ideas.

We can also use other approaches, but currently this is the only way I can  
think of.

Kai-Heng

>
> Kai-Heng
>
>> thanks,
>>
>> greg k-h

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Kai-Heng Feng]

at 18:22, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

> at 00:01, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>
>>> On Jan 30, 2019, at 16:21, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Thu, Jan 24, 2019 at 02:16:43PM +0800, Kai-Heng Feng wrote:
>>>> The QCA Rome USB Bluetooth controller has several issues once LPM gets
>>>> enabled:
>>>> - Fails to get enumerated in coldboot. [1]
>>>> - Drains more power (~ 0.2W) when the system is in S5. [2]
>>>> - Disappears after a warmboot. [2]
>>>>
>>>> The issue happens because the device lingers at LPM L1 in S5, so device
>>>> can't get enumerated even after a reboot.
>>>>
>>>> Disable LPM at shutdown to solve the issue.
>>>>
>>>> [1] https://bugs.launchpad.net/bugs/1757218
>>>> [2] https://patchwork.kernel.org/patch/10607097/
>>>>
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>> ---
>>>> v2: Use new LPM helpers.
>>>>
>>>> drivers/usb/core/port.c | 9 +++++++++
>>>> 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>>>> index 1a06a4b5fbb1..bbbb35fa639f 100644
>>>> --- a/drivers/usb/core/port.c
>>>> +++ b/drivers/usb/core/port.c
>>>> @@ -285,6 +285,14 @@ static int usb_port_runtime_suspend(struct device  
>>>> *dev)
>>>> }
>>>> #endif
>>>>
>>>> +static void usb_port_shutdown(struct device *dev)
>>>> +{
>>>> +	struct usb_port *port_dev = to_usb_port(dev);
>>>> +
>>>> +	if (port_dev->child)
>>>> +		usb_disable_usb2_hardware_lpm(port_dev->child);
>>>> +}
>>>> +
>>>> static const struct dev_pm_ops usb_port_pm_ops = {
>>>> #ifdef CONFIG_PM
>>>> 	.runtime_suspend =	usb_port_runtime_suspend,
>>>> @@ -301,6 +309,7 @@ struct device_type usb_port_device_type = {
>>>> static struct device_driver usb_port_driver = {
>>>> 	.name = "usb",
>>>> 	.owner = THIS_MODULE,
>>>> +	.shutdown = usb_port_shutdown,
>>>> };
>>>
>>> So you now do this for all ports in the system, no matter what is
>>> plugged in or not.  Are you _SURE_ you want to do that?  It seems like a
>>> big hammer to solve just one single device's problems.
>>
>> Yes I think this should be universally applied.
>>
>> I don’t think the bug only happens to one device. Users won’t find this
>> unless they connect their laptop to a power meter.
>>
>> Platform may not completely cut off USB bus power during shutdown,
>> so the device transits to L1 after system shutdown. Now xHC is disabled
>> so nothing can bring it back to L0 or L2.
>
> It would be great if someone can come up with better ideas.
>
> We can also use other approaches, but currently this is the only way I  
> can think of.

Alan and Mathias,

It would be great if you can review this patch, or give me some suggestion.

Kai-Heng

>
> Kai-Heng
>
>> Kai-Heng
>>
>>> thanks,
>>>
>>> greg k-h

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Kai-Heng Feng]

at 15:55, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

> at 18:22, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>
>> at 00:01, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>
>>>> On Jan 30, 2019, at 16:21, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>
>>>> On Thu, Jan 24, 2019 at 02:16:43PM +0800, Kai-Heng Feng wrote:
>>>>> The QCA Rome USB Bluetooth controller has several issues once LPM gets
>>>>> enabled:
>>>>> - Fails to get enumerated in coldboot. [1]
>>>>> - Drains more power (~ 0.2W) when the system is in S5. [2]
>>>>> - Disappears after a warmboot. [2]
>>>>>
>>>>> The issue happens because the device lingers at LPM L1 in S5, so device
>>>>> can't get enumerated even after a reboot.
>>>>>
>>>>> Disable LPM at shutdown to solve the issue.
>>>>>
>>>>> [1] https://bugs.launchpad.net/bugs/1757218
>>>>> [2] https://patchwork.kernel.org/patch/10607097/
>>>>>
>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>> ---
>>>>> v2: Use new LPM helpers.
>>>>>
>>>>> drivers/usb/core/port.c | 9 +++++++++
>>>>> 1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>>>>> index 1a06a4b5fbb1..bbbb35fa639f 100644
>>>>> --- a/drivers/usb/core/port.c
>>>>> +++ b/drivers/usb/core/port.c
>>>>> @@ -285,6 +285,14 @@ static int usb_port_runtime_suspend(struct  
>>>>> device *dev)
>>>>> }
>>>>> #endif
>>>>>
>>>>> +static void usb_port_shutdown(struct device *dev)
>>>>> +{
>>>>> +	struct usb_port *port_dev = to_usb_port(dev);
>>>>> +
>>>>> +	if (port_dev->child)
>>>>> +		usb_disable_usb2_hardware_lpm(port_dev->child);
>>>>> +}
>>>>> +
>>>>> static const struct dev_pm_ops usb_port_pm_ops = {
>>>>> #ifdef CONFIG_PM
>>>>> 	.runtime_suspend =	usb_port_runtime_suspend,
>>>>> @@ -301,6 +309,7 @@ struct device_type usb_port_device_type = {
>>>>> static struct device_driver usb_port_driver = {
>>>>> 	.name = "usb",
>>>>> 	.owner = THIS_MODULE,
>>>>> +	.shutdown = usb_port_shutdown,
>>>>> };
>>>>
>>>> So you now do this for all ports in the system, no matter what is
>>>> plugged in or not.  Are you _SURE_ you want to do that?  It seems like a
>>>> big hammer to solve just one single device's problems.
>>>
>>> Yes I think this should be universally applied.
>>>
>>> I don’t think the bug only happens to one device. Users won’t find this
>>> unless they connect their laptop to a power meter.
>>>
>>> Platform may not completely cut off USB bus power during shutdown,
>>> so the device transits to L1 after system shutdown. Now xHC is disabled
>>> so nothing can bring it back to L0 or L2.
>>
>> It would be great if someone can come up with better ideas.
>>
>> We can also use other approaches, but currently this is the only way I  
>> can think of.
>
> Alan and Mathias,
>
> It would be great if you can review this patch, or give me some suggestion.

Can I get some advice here?
I really think disabling LPM should be universally applied.

Kai-Heng

>
> Kai-Heng
>
>> Kai-Heng
>>
>>> Kai-Heng
>>>
>>>> thanks,
>>>>
>>>> greg k-h

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Alan Stern]

On Thu, 6 Jun 2019, Kai-Heng Feng wrote:

> at 15:55, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> 
> > at 18:22, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> >
> >> at 00:01, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> >>
> >>>> On Jan 30, 2019, at 16:21, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>>>
> >>>> On Thu, Jan 24, 2019 at 02:16:43PM +0800, Kai-Heng Feng wrote:
> >>>>> The QCA Rome USB Bluetooth controller has several issues once LPM gets
> >>>>> enabled:
> >>>>> - Fails to get enumerated in coldboot. [1]
> >>>>> - Drains more power (~ 0.2W) when the system is in S5. [2]
> >>>>> - Disappears after a warmboot. [2]
> >>>>>
> >>>>> The issue happens because the device lingers at LPM L1 in S5, so device
> >>>>> can't get enumerated even after a reboot.
> >>>>>
> >>>>> Disable LPM at shutdown to solve the issue.
> >>>>>
> >>>>> [1] https://bugs.launchpad.net/bugs/1757218
> >>>>> [2] https://patchwork.kernel.org/patch/10607097/
> >>>>>
> >>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >>>>> ---
> >>>>> v2: Use new LPM helpers.
> >>>>>
> >>>>> drivers/usb/core/port.c | 9 +++++++++
> >>>>> 1 file changed, 9 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> >>>>> index 1a06a4b5fbb1..bbbb35fa639f 100644
> >>>>> --- a/drivers/usb/core/port.c
> >>>>> +++ b/drivers/usb/core/port.c
> >>>>> @@ -285,6 +285,14 @@ static int usb_port_runtime_suspend(struct  
> >>>>> device *dev)
> >>>>> }
> >>>>> #endif
> >>>>>
> >>>>> +static void usb_port_shutdown(struct device *dev)
> >>>>> +{
> >>>>> +	struct usb_port *port_dev = to_usb_port(dev);
> >>>>> +
> >>>>> +	if (port_dev->child)
> >>>>> +		usb_disable_usb2_hardware_lpm(port_dev->child);
> >>>>> +}
> >>>>> +
> >>>>> static const struct dev_pm_ops usb_port_pm_ops = {
> >>>>> #ifdef CONFIG_PM
> >>>>> 	.runtime_suspend =	usb_port_runtime_suspend,
> >>>>> @@ -301,6 +309,7 @@ struct device_type usb_port_device_type = {
> >>>>> static struct device_driver usb_port_driver = {
> >>>>> 	.name = "usb",
> >>>>> 	.owner = THIS_MODULE,
> >>>>> +	.shutdown = usb_port_shutdown,
> >>>>> };
> >>>>
> >>>> So you now do this for all ports in the system, no matter what is
> >>>> plugged in or not.  Are you _SURE_ you want to do that?  It seems like a
> >>>> big hammer to solve just one single device's problems.
> >>>
> >>> Yes I think this should be universally applied.
> >>>
> >>> I don’t think the bug only happens to one device. Users won’t find this
> >>> unless they connect their laptop to a power meter.
> >>>
> >>> Platform may not completely cut off USB bus power during shutdown,
> >>> so the device transits to L1 after system shutdown. Now xHC is disabled
> >>> so nothing can bring it back to L0 or L2.
> >>
> >> It would be great if someone can come up with better ideas.
> >>
> >> We can also use other approaches, but currently this is the only way I  
> >> can think of.
> >
> > Alan and Mathias,
> >
> > It would be great if you can review this patch, or give me some suggestion.
> 
> Can I get some advice here?
> I really think disabling LPM should be universally applied.
> 
> Kai-Heng

I agree with Kai-Heng, this seems like a fairly light-weight solution 
to a reasonable problem.

As to the issue of how much it will slow down system shutdowns, I have 
no idea.  Probably not very much, unless somebody has an unusually 
large number of USB devices plugged in, but only testing can give a 
real answer.

I suppose we could add an HCD flag for host controllers which require 
this workaround.  Either way, it's probably not a very big deal.

Alan Stern

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Kai-Heng Feng]

at 22:17, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, 6 Jun 2019, Kai-Heng Feng wrote:
>
>> at 15:55, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>
>>> at 18:22, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>
>>>> at 00:01, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>>
>>>>>> On Jan 30, 2019, at 16:21, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>>>
>>>>>> On Thu, Jan 24, 2019 at 02:16:43PM +0800, Kai-Heng Feng wrote:
>>>>>>> The QCA Rome USB Bluetooth controller has several issues once LPM  
>>>>>>> gets
>>>>>>> enabled:
>>>>>>> - Fails to get enumerated in coldboot. [1]
>>>>>>> - Drains more power (~ 0.2W) when the system is in S5. [2]
>>>>>>> - Disappears after a warmboot. [2]
>>>>>>>
>>>>>>> The issue happens because the device lingers at LPM L1 in S5, so  
>>>>>>> device
>>>>>>> can't get enumerated even after a reboot.
>>>>>>>
>>>>>>> Disable LPM at shutdown to solve the issue.
>>>>>>>
>>>>>>> [1] https://bugs.launchpad.net/bugs/1757218
>>>>>>> [2] https://patchwork.kernel.org/patch/10607097/
>>>>>>>
>>>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>>>> ---
>>>>>>> v2: Use new LPM helpers.
>>>>>>>
>>>>>>> drivers/usb/core/port.c | 9 +++++++++
>>>>>>> 1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>>>>>>> index 1a06a4b5fbb1..bbbb35fa639f 100644
>>>>>>> --- a/drivers/usb/core/port.c
>>>>>>> +++ b/drivers/usb/core/port.c
>>>>>>> @@ -285,6 +285,14 @@ static int usb_port_runtime_suspend(struct
>>>>>>> device *dev)
>>>>>>> }
>>>>>>> #endif
>>>>>>>
>>>>>>> +static void usb_port_shutdown(struct device *dev)
>>>>>>> +{
>>>>>>> +	struct usb_port *port_dev = to_usb_port(dev);
>>>>>>> +
>>>>>>> +	if (port_dev->child)
>>>>>>> +		usb_disable_usb2_hardware_lpm(port_dev->child);
>>>>>>> +}
>>>>>>> +
>>>>>>> static const struct dev_pm_ops usb_port_pm_ops = {
>>>>>>> #ifdef CONFIG_PM
>>>>>>> 	.runtime_suspend =	usb_port_runtime_suspend,
>>>>>>> @@ -301,6 +309,7 @@ struct device_type usb_port_device_type = {
>>>>>>> static struct device_driver usb_port_driver = {
>>>>>>> 	.name = "usb",
>>>>>>> 	.owner = THIS_MODULE,
>>>>>>> +	.shutdown = usb_port_shutdown,
>>>>>>> };
>>>>>>
>>>>>> So you now do this for all ports in the system, no matter what is
>>>>>> plugged in or not.  Are you _SURE_ you want to do that?  It seems  
>>>>>> like a
>>>>>> big hammer to solve just one single device's problems.
>>>>>
>>>>> Yes I think this should be universally applied.
>>>>>
>>>>> I don’t think the bug only happens to one device. Users won’t find this
>>>>> unless they connect their laptop to a power meter.
>>>>>
>>>>> Platform may not completely cut off USB bus power during shutdown,
>>>>> so the device transits to L1 after system shutdown. Now xHC is disabled
>>>>> so nothing can bring it back to L0 or L2.
>>>>
>>>> It would be great if someone can come up with better ideas.
>>>>
>>>> We can also use other approaches, but currently this is the only way I
>>>> can think of.
>>>
>>> Alan and Mathias,
>>>
>>> It would be great if you can review this patch, or give me some  
>>> suggestion.
>>
>> Can I get some advice here?
>> I really think disabling LPM should be universally applied.
>>
>> Kai-Heng
>
> I agree with Kai-Heng, this seems like a fairly light-weight solution
> to a reasonable problem.

Thanks for your review.

>
> As to the issue of how much it will slow down system shutdowns, I have
> no idea.  Probably not very much, unless somebody has an unusually
> large number of USB devices plugged in, but only testing can give a
> real answer.

In addition to that, only USB2 devices that enable LPM will slow down  
shutdown process.
Right now only internally connected USB2 devices enable LPM, so the numbers  
are even lower.

>
> I suppose we could add an HCD flag for host controllers which require
> this workaround.  Either way, it's probably not a very big deal.

IMO this is not necessary. Only xHCI that reports hw_lpm_support will be  
affected. At least for PC, this only became true after Whiskey Lake.

Kai-Heng

>
> Alan Stern

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Kai-Heng Feng]

Hi Greg,

at 17:22, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

> at 22:17, Alan Stern <stern@rowland.harvard.edu> wrote:
>>
>> I agree with Kai-Heng, this seems like a fairly light-weight solution
>> to a reasonable problem.
>
> Thanks for your review.
>
>> As to the issue of how much it will slow down system shutdowns, I have
>> no idea.  Probably not very much, unless somebody has an unusually
>> large number of USB devices plugged in, but only testing can give a
>> real answer.
>
> In addition to that, only USB2 devices that enable LPM will slow down  
> shutdown process.
> Right now only internally connected USB2 devices enable LPM, so the  
> numbers are even lower.
>
>> I suppose we could add an HCD flag for host controllers which require
>> this workaround.  Either way, it's probably not a very big deal.
>
> IMO this is not necessary. Only xHCI that reports hw_lpm_support will be  
> affected. At least for PC, this only became true after Whiskey Lake.
>
> Kai-Heng
>
>> Alan Stern

This patch is included in Ubuntu’s kernel for a while now, and there’s no  
regression report so far.
Please consider merge this patch.

Kai-Heng

Re: [PATCH v2] USB: Disable USB2 LPM at shutdown

[Greg KH]

On Mon, Aug 05, 2019 at 08:58:33PM +0800, Kai-Heng Feng wrote:
> Hi Greg,
> 
> at 17:22, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> 
> > at 22:17, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > 
> > > I agree with Kai-Heng, this seems like a fairly light-weight solution
> > > to a reasonable problem.
> > 
> > Thanks for your review.
> > 
> > > As to the issue of how much it will slow down system shutdowns, I have
> > > no idea.  Probably not very much, unless somebody has an unusually
> > > large number of USB devices plugged in, but only testing can give a
> > > real answer.
> > 
> > In addition to that, only USB2 devices that enable LPM will slow down
> > shutdown process.
> > Right now only internally connected USB2 devices enable LPM, so the
> > numbers are even lower.
> > 
> > > I suppose we could add an HCD flag for host controllers which require
> > > this workaround.  Either way, it's probably not a very big deal.
> > 
> > IMO this is not necessary. Only xHCI that reports hw_lpm_support will be
> > affected. At least for PC, this only became true after Whiskey Lake.
> > 
> > Kai-Heng
> > 
> > > Alan Stern
> 
> This patch is included in Ubuntu’s kernel for a while now, and there’s no
> regression report so far.
> Please consider merge this patch.

I do not see a patch here at all, sorry.  Please resend it.

greg k-h