All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] xhci: Return if xHCI doesn't support LPM
@ 2020-05-20 10:18 Kai-Heng Feng
  2020-05-20 10:18 ` [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM Kai-Heng Feng
  0 siblings, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2020-05-20 10:18 UTC (permalink / raw)
  To: mathias.nyman
  Cc: Kai-Heng Feng, Greg Kroah-Hartman, open list:USB XHCI DRIVER, open list

Just return if xHCI is quirked to disable LPM. We can save some time
from reading registers and doing spinlocks.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/usb/host/xhci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index bee5deccc83d..14181a7ea375 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4390,6 +4390,9 @@ static int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
 	int		hird, exit_latency;
 	int		ret;
 
+	if (xhci->quirks & XHCI_HW_LPM_DISABLE)
+		return -EPERM;
+
 	if (hcd->speed >= HCD_USB3 || !xhci->hw_lpm_support ||
 			!udev->lpm_capable)
 		return -EPERM;
@@ -4412,7 +4415,7 @@ static int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
 	xhci_dbg(xhci, "%s port %d USB2 hardware LPM\n",
 			enable ? "enable" : "disable", port_num + 1);
 
-	if (enable && !(xhci->quirks & XHCI_HW_LPM_DISABLE)) {
+	if (enable) {
 		/* Host supports BESL timeout instead of HIRD */
 		if (udev->usb2_hw_lpm_besl_capable) {
 			/* if device doesn't have a preferred BESL value use a
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM
  2020-05-20 10:18 [PATCH 1/2] xhci: Return if xHCI doesn't support LPM Kai-Heng Feng
@ 2020-05-20 10:18 ` Kai-Heng Feng
  2020-06-08  3:58   ` Kai-Heng Feng
  2020-06-08 11:21   ` Mathias Nyman
  0 siblings, 2 replies; 8+ messages in thread
From: Kai-Heng Feng @ 2020-05-20 10:18 UTC (permalink / raw)
  To: mathias.nyman
  Cc: Kai-Heng Feng, Greg Kroah-Hartman, open list:USB XHCI DRIVER, open list

USB2 devices with LPM enabled may interrupt the system suspend:
[  932.510475] usb 1-7: usb suspend, wakeup 0
[  932.510549] hub 1-0:1.0: hub_suspend
[  932.510581] usb usb1: bus suspend, wakeup 0
[  932.510590] xhci_hcd 0000:00:14.0: port 9 not suspended
[  932.510593] xhci_hcd 0000:00:14.0: port 8 not suspended
..
[  932.520323] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03
..
[  932.591405] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
[  932.591414] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
[  932.591418] PM: Device 0000:00:14.0 failed to suspend async: error -16

During system suspend, USB core will let HC suspends the device if it
doesn't have remote wakeup enabled and doesn't have any children.
However, from the log above we can see that the usb 1-7 doesn't get bus
suspended due to not in U0. After a while the port finished U2 -> U0
transition, interrupts the suspend process.

The observation is that after disabling LPM, port doesn't transit to U0
immediately and can linger in U2. xHCI spec 4.23.5.2 states that the
maximum exit latency for USB2 LPM should be BESL + 10us. The BESL for
the affected device is advertised as 400us, which is still not enough
based on my testing result.

So let's use the maximum permitted latency, 10000, to poll for U0
status to solve the issue.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/usb/host/xhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 14181a7ea375..1058f604975b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4474,6 +4474,9 @@ static int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
 			mutex_lock(hcd->bandwidth_mutex);
 			xhci_change_max_exit_latency(xhci, udev, 0);
 			mutex_unlock(hcd->bandwidth_mutex);
+			readl_poll_timeout(ports[port_num]->addr, pm_val,
+					   (pm_val & PORT_PLS_MASK) == XDEV_U0,
+					   100, 10000);
 			return 0;
 		}
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM
  2020-05-20 10:18 ` [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM Kai-Heng Feng
@ 2020-06-08  3:58   ` Kai-Heng Feng
  2020-06-08  7:05     ` Greg Kroah-Hartman
  2020-06-08 11:21   ` Mathias Nyman
  1 sibling, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2020-06-08  3:58 UTC (permalink / raw)
  To: mathias.nyman; +Cc: Greg Kroah-Hartman, open list:USB XHCI DRIVER, open list



> On May 20, 2020, at 18:18, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> 
> USB2 devices with LPM enabled may interrupt the system suspend:
> [  932.510475] usb 1-7: usb suspend, wakeup 0
> [  932.510549] hub 1-0:1.0: hub_suspend
> [  932.510581] usb usb1: bus suspend, wakeup 0
> [  932.510590] xhci_hcd 0000:00:14.0: port 9 not suspended
> [  932.510593] xhci_hcd 0000:00:14.0: port 8 not suspended
> ..
> [  932.520323] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03
> ..
> [  932.591405] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
> [  932.591414] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
> [  932.591418] PM: Device 0000:00:14.0 failed to suspend async: error -16
> 
> During system suspend, USB core will let HC suspends the device if it
> doesn't have remote wakeup enabled and doesn't have any children.
> However, from the log above we can see that the usb 1-7 doesn't get bus
> suspended due to not in U0. After a while the port finished U2 -> U0
> transition, interrupts the suspend process.
> 
> The observation is that after disabling LPM, port doesn't transit to U0
> immediately and can linger in U2. xHCI spec 4.23.5.2 states that the
> maximum exit latency for USB2 LPM should be BESL + 10us. The BESL for
> the affected device is advertised as 400us, which is still not enough
> based on my testing result.
> 
> So let's use the maximum permitted latency, 10000, to poll for U0
> status to solve the issue.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

A gentle ping...

> ---
> drivers/usb/host/xhci.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 14181a7ea375..1058f604975b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4474,6 +4474,9 @@ static int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
> 			mutex_lock(hcd->bandwidth_mutex);
> 			xhci_change_max_exit_latency(xhci, udev, 0);
> 			mutex_unlock(hcd->bandwidth_mutex);
> +			readl_poll_timeout(ports[port_num]->addr, pm_val,
> +					   (pm_val & PORT_PLS_MASK) == XDEV_U0,
> +					   100, 10000);
> 			return 0;
> 		}
> 	}
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM
  2020-06-08  3:58   ` Kai-Heng Feng
@ 2020-06-08  7:05     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 8+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-08  7:05 UTC (permalink / raw)
  To: Kai-Heng Feng; +Cc: mathias.nyman, open list:USB XHCI DRIVER, open list

On Mon, Jun 08, 2020 at 11:58:40AM +0800, Kai-Heng Feng wrote:
> 
> 
> > On May 20, 2020, at 18:18, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> > 
> > USB2 devices with LPM enabled may interrupt the system suspend:
> > [  932.510475] usb 1-7: usb suspend, wakeup 0
> > [  932.510549] hub 1-0:1.0: hub_suspend
> > [  932.510581] usb usb1: bus suspend, wakeup 0
> > [  932.510590] xhci_hcd 0000:00:14.0: port 9 not suspended
> > [  932.510593] xhci_hcd 0000:00:14.0: port 8 not suspended
> > ..
> > [  932.520323] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03
> > ..
> > [  932.591405] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
> > [  932.591414] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
> > [  932.591418] PM: Device 0000:00:14.0 failed to suspend async: error -16
> > 
> > During system suspend, USB core will let HC suspends the device if it
> > doesn't have remote wakeup enabled and doesn't have any children.
> > However, from the log above we can see that the usb 1-7 doesn't get bus
> > suspended due to not in U0. After a while the port finished U2 -> U0
> > transition, interrupts the suspend process.
> > 
> > The observation is that after disabling LPM, port doesn't transit to U0
> > immediately and can linger in U2. xHCI spec 4.23.5.2 states that the
> > maximum exit latency for USB2 LPM should be BESL + 10us. The BESL for
> > the affected device is advertised as 400us, which is still not enough
> > based on my testing result.
> > 
> > So let's use the maximum permitted latency, 10000, to poll for U0
> > status to solve the issue.
> > 
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> A gentle ping...

It is the middle of the merge window, we can't do anything with any new
patch until after -rc1 is out.

You know this...

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM
  2020-05-20 10:18 ` [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM Kai-Heng Feng
  2020-06-08  3:58   ` Kai-Heng Feng
@ 2020-06-08 11:21   ` Mathias Nyman
  2020-06-09 10:15     ` Kai-Heng Feng
  1 sibling, 1 reply; 8+ messages in thread
From: Mathias Nyman @ 2020-06-08 11:21 UTC (permalink / raw)
  To: Kai-Heng Feng, mathias.nyman
  Cc: Greg Kroah-Hartman, open list:USB XHCI DRIVER, open list

On 20.5.2020 13.18, Kai-Heng Feng wrote:
> USB2 devices with LPM enabled may interrupt the system suspend:
> [  932.510475] usb 1-7: usb suspend, wakeup 0
> [  932.510549] hub 1-0:1.0: hub_suspend
> [  932.510581] usb usb1: bus suspend, wakeup 0
> [  932.510590] xhci_hcd 0000:00:14.0: port 9 not suspended
> [  932.510593] xhci_hcd 0000:00:14.0: port 8 not suspended
> ..
> [  932.520323] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03

400e03 = Connected, Enabled, U0 with port ink state change flag (PLC) set.

> ..
> [  932.591405] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
> [  932.591414] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
> [  932.591418] PM: Device 0000:00:14.0 failed to suspend async: error -16
> 
> During system suspend, USB core will let HC suspends the device if it
> doesn't have remote wakeup enabled and doesn't have any children.
> However, from the log above we can see that the usb 1-7 doesn't get bus
> suspended due to not in U0. After a while the port finished U2 -> U0
> transition, interrupts the suspend process.

In USB2 HW link PM the PLC flag should not be set in U2Exit -> U0 transitions.
Only case we should see a port change event is U2Entry -> U0 due to STALL or
error/timeout. (see xhci 4.23.5.1.1.1)

> 
> The observation is that after disabling LPM, port doesn't transit to U0
> immediately and can linger in U2. xHCI spec 4.23.5.2 states that the
> maximum exit latency for USB2 LPM should be BESL + 10us. The BESL for
> the affected device is advertised as 400us, which is still not enough
> based on my testing result.
> 
> So let's use the maximum permitted latency, 10000, to poll for U0
> status to solve the issue.

I don't recall all details, but it could be that disabling LPM before suspend
is unnecessary. 
At least xhci should be able to set a port to U3 from U1 and U2 (see xhci 4.15.1)
so that is one change that could be done to xhci_bus_suspend()

Also just noticed that we are not really checking L1S field in PORTPMSC register, 
this should tell if there was an issue with USB2 lpm state transitions, and
perhaps we should disable lpm for that device. 

Does the L1S field show anything unuaual in your case?
That could explain the port event with the PLC bit set.

I think we can avoid a readl_poll_timeout() solution in this case.

-Mathias

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM
  2020-06-08 11:21   ` Mathias Nyman
@ 2020-06-09 10:15     ` Kai-Heng Feng
  2020-06-19 14:19       ` Kai-Heng Feng
  0 siblings, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2020-06-09 10:15 UTC (permalink / raw)
  To: Mathias Nyman
  Cc: Mathias Nyman, Greg Kroah-Hartman, open list:USB XHCI DRIVER, open list



> On Jun 8, 2020, at 19:21, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
> 
> On 20.5.2020 13.18, Kai-Heng Feng wrote:
>> USB2 devices with LPM enabled may interrupt the system suspend:
>> [  932.510475] usb 1-7: usb suspend, wakeup 0
>> [  932.510549] hub 1-0:1.0: hub_suspend
>> [  932.510581] usb usb1: bus suspend, wakeup 0
>> [  932.510590] xhci_hcd 0000:00:14.0: port 9 not suspended
>> [  932.510593] xhci_hcd 0000:00:14.0: port 8 not suspended
>> ..
>> [  932.520323] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03
> 
> 400e03 = Connected, Enabled, U0 with port ink state change flag (PLC) set.
> 
>> ..
>> [  932.591405] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
>> [  932.591414] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
>> [  932.591418] PM: Device 0000:00:14.0 failed to suspend async: error -16
>> 
>> During system suspend, USB core will let HC suspends the device if it
>> doesn't have remote wakeup enabled and doesn't have any children.
>> However, from the log above we can see that the usb 1-7 doesn't get bus
>> suspended due to not in U0. After a while the port finished U2 -> U0
>> transition, interrupts the suspend process.
> 
> In USB2 HW link PM the PLC flag should not be set in U2Exit -> U0 transitions.
> Only case we should see a port change event is U2Entry -> U0 due to STALL or
> error/timeout. (see xhci 4.23.5.1.1.1)
> 
>> 
>> The observation is that after disabling LPM, port doesn't transit to U0
>> immediately and can linger in U2. xHCI spec 4.23.5.2 states that the
>> maximum exit latency for USB2 LPM should be BESL + 10us. The BESL for
>> the affected device is advertised as 400us, which is still not enough
>> based on my testing result.
>> 
>> So let's use the maximum permitted latency, 10000, to poll for U0
>> status to solve the issue.
> 
> I don't recall all details, but it could be that disabling LPM before suspend
> is unnecessary. 
> At least xhci should be able to set a port to U3 from U1 and U2 (see xhci 4.15.1)
> so that is one change that could be done to xhci_bus_suspend()

Yes, put the device to U3 directly does the trick.

> 
> Also just noticed that we are not really checking L1S field in PORTPMSC register, 
> this should tell if there was an issue with USB2 lpm state transitions, and
> perhaps we should disable lpm for that device. 
> 
> Does the L1S field show anything unuaual in your case?
> That could explain the port event with the PLC bit set.

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 2c255d0620b0..a2099d42e490 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1592,7 +1592,7 @@ static void handle_port_status(struct xhci_hcd *xhci,
 {
        struct usb_hcd *hcd;
        u32 port_id;
-       u32 portsc, cmd_reg;
+       u32 portsc, portpmsc, cmd_reg;
        int max_ports;
        int slot_id;
        unsigned int hcd_portnum;
@@ -1634,9 +1634,10 @@ static void handle_port_status(struct xhci_hcd *xhci,
        bus_state = &port->rhub->bus_state;
        hcd_portnum = port->hcd_portnum;
        portsc = readl(port->addr);
+       portpmsc = readl(port->addr + PORTPMSC);
 
-       xhci_dbg(xhci, "Port change event, %d-%d, id %d, portsc: 0x%x\n",
-                hcd->self.busnum, hcd_portnum + 1, port_id, portsc);
+       xhci_dbg(xhci, "Port change event, %d-%d, id %d, portsc: 0x%x, portpmsc 0x%0x\n",
+                hcd->self.busnum, hcd_portnum + 1, port_id, portsc, portpmsc);
 
        trace_xhci_handle_port_status(hcd_portnum, portsc);

[  685.460054] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03, portpmsc 0x1
[  685.460062] xhci_hcd 0000:00:14.0: resume root hub
[  685.460079] xhci_hcd 0000:00:14.0: handle_port_status: starting port polling.
[  685.460094] xhci_hcd 0000:00:14.0: xhci_hub_status_data: stopping port polling.
[  685.521685] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
[  685.521695] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
[  685.521699] PM: Device 0000:00:14.0 failed to suspend async: error -16

So after disabling LPM, it takes a long time to complete L1 transition, before transitioning to L0.

Kai-Heng

> 
> I think we can avoid a readl_poll_timeout() solution in this case.
> 
> -Mathias


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM
  2020-06-09 10:15     ` Kai-Heng Feng
@ 2020-06-19 14:19       ` Kai-Heng Feng
  2020-06-23  8:38         ` Mathias Nyman
  0 siblings, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2020-06-19 14:19 UTC (permalink / raw)
  To: Mathias Nyman
  Cc: Mathias Nyman, Greg Kroah-Hartman, open list:USB XHCI DRIVER, open list

Hi Mathias,

> On Jun 9, 2020, at 18:15, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> 
> 
> 
>> On Jun 8, 2020, at 19:21, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
>> 
>> On 20.5.2020 13.18, Kai-Heng Feng wrote:
>>> USB2 devices with LPM enabled may interrupt the system suspend:
>>> [  932.510475] usb 1-7: usb suspend, wakeup 0
>>> [  932.510549] hub 1-0:1.0: hub_suspend
>>> [  932.510581] usb usb1: bus suspend, wakeup 0
>>> [  932.510590] xhci_hcd 0000:00:14.0: port 9 not suspended
>>> [  932.510593] xhci_hcd 0000:00:14.0: port 8 not suspended
>>> ..
>>> [  932.520323] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03
>> 
>> 400e03 = Connected, Enabled, U0 with port ink state change flag (PLC) set.
>> 
>>> ..
>>> [  932.591405] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
>>> [  932.591414] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
>>> [  932.591418] PM: Device 0000:00:14.0 failed to suspend async: error -16
>>> 
>>> During system suspend, USB core will let HC suspends the device if it
>>> doesn't have remote wakeup enabled and doesn't have any children.
>>> However, from the log above we can see that the usb 1-7 doesn't get bus
>>> suspended due to not in U0. After a while the port finished U2 -> U0
>>> transition, interrupts the suspend process.
>> 
>> In USB2 HW link PM the PLC flag should not be set in U2Exit -> U0 transitions.
>> Only case we should see a port change event is U2Entry -> U0 due to STALL or
>> error/timeout. (see xhci 4.23.5.1.1.1)
>> 
>>> 
>>> The observation is that after disabling LPM, port doesn't transit to U0
>>> immediately and can linger in U2. xHCI spec 4.23.5.2 states that the
>>> maximum exit latency for USB2 LPM should be BESL + 10us. The BESL for
>>> the affected device is advertised as 400us, which is still not enough
>>> based on my testing result.
>>> 
>>> So let's use the maximum permitted latency, 10000, to poll for U0
>>> status to solve the issue.
>> 
>> I don't recall all details, but it could be that disabling LPM before suspend
>> is unnecessary. 
>> At least xhci should be able to set a port to U3 from U1 and U2 (see xhci 4.15.1)
>> so that is one change that could be done to xhci_bus_suspend()
> 
> Yes, put the device to U3 directly does the trick.

As discussed, will you pick this series over the v2?
Or is there anything I should improve for this one?

Kai-Heng

> 
>> 
>> Also just noticed that we are not really checking L1S field in PORTPMSC register, 
>> this should tell if there was an issue with USB2 lpm state transitions, and
>> perhaps we should disable lpm for that device. 
>> 
>> Does the L1S field show anything unuaual in your case?
>> That could explain the port event with the PLC bit set.
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 2c255d0620b0..a2099d42e490 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1592,7 +1592,7 @@ static void handle_port_status(struct xhci_hcd *xhci,
> {
>        struct usb_hcd *hcd;
>        u32 port_id;
> -       u32 portsc, cmd_reg;
> +       u32 portsc, portpmsc, cmd_reg;
>        int max_ports;
>        int slot_id;
>        unsigned int hcd_portnum;
> @@ -1634,9 +1634,10 @@ static void handle_port_status(struct xhci_hcd *xhci,
>        bus_state = &port->rhub->bus_state;
>        hcd_portnum = port->hcd_portnum;
>        portsc = readl(port->addr);
> +       portpmsc = readl(port->addr + PORTPMSC);
> 
> -       xhci_dbg(xhci, "Port change event, %d-%d, id %d, portsc: 0x%x\n",
> -                hcd->self.busnum, hcd_portnum + 1, port_id, portsc);
> +       xhci_dbg(xhci, "Port change event, %d-%d, id %d, portsc: 0x%x, portpmsc 0x%0x\n",
> +                hcd->self.busnum, hcd_portnum + 1, port_id, portsc, portpmsc);
> 
>        trace_xhci_handle_port_status(hcd_portnum, portsc);
> 
> [  685.460054] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03, portpmsc 0x1
> [  685.460062] xhci_hcd 0000:00:14.0: resume root hub
> [  685.460079] xhci_hcd 0000:00:14.0: handle_port_status: starting port polling.
> [  685.460094] xhci_hcd 0000:00:14.0: xhci_hub_status_data: stopping port polling.
> [  685.521685] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
> [  685.521695] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
> [  685.521699] PM: Device 0000:00:14.0 failed to suspend async: error -16
> 
> So after disabling LPM, it takes a long time to complete L1 transition, before transitioning to L0.
> 
> Kai-Heng
> 
>> 
>> I think we can avoid a readl_poll_timeout() solution in this case.
>> 
>> -Mathias


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM
  2020-06-19 14:19       ` Kai-Heng Feng
@ 2020-06-23  8:38         ` Mathias Nyman
  0 siblings, 0 replies; 8+ messages in thread
From: Mathias Nyman @ 2020-06-23  8:38 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: Mathias Nyman, Greg Kroah-Hartman, open list:USB XHCI DRIVER, open list

On 19.6.2020 17.19, Kai-Heng Feng wrote:
> Hi Mathias,
> 
>> On Jun 9, 2020, at 18:15, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>
>>
>>
>>> On Jun 8, 2020, at 19:21, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
>>>
>>> On 20.5.2020 13.18, Kai-Heng Feng wrote:
>>>> USB2 devices with LPM enabled may interrupt the system suspend:
>>>> [  932.510475] usb 1-7: usb suspend, wakeup 0
>>>> [  932.510549] hub 1-0:1.0: hub_suspend
>>>> [  932.510581] usb usb1: bus suspend, wakeup 0
>>>> [  932.510590] xhci_hcd 0000:00:14.0: port 9 not suspended
>>>> [  932.510593] xhci_hcd 0000:00:14.0: port 8 not suspended
>>>> ..
>>>> [  932.520323] xhci_hcd 0000:00:14.0: Port change event, 1-7, id 7, portsc: 0x400e03
>>>
>>> 400e03 = Connected, Enabled, U0 with port ink state change flag (PLC) set.
>>>
>>>> ..
>>>> [  932.591405] PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x30 returns -16
>>>> [  932.591414] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -16
>>>> [  932.591418] PM: Device 0000:00:14.0 failed to suspend async: error -16
>>>>
>>>> During system suspend, USB core will let HC suspends the device if it
>>>> doesn't have remote wakeup enabled and doesn't have any children.
>>>> However, from the log above we can see that the usb 1-7 doesn't get bus
>>>> suspended due to not in U0. After a while the port finished U2 -> U0
>>>> transition, interrupts the suspend process.
>>>
>>> In USB2 HW link PM the PLC flag should not be set in U2Exit -> U0 transitions.
>>> Only case we should see a port change event is U2Entry -> U0 due to STALL or
>>> error/timeout. (see xhci 4.23.5.1.1.1)
>>>
>>>>
>>>> The observation is that after disabling LPM, port doesn't transit to U0
>>>> immediately and can linger in U2. xHCI spec 4.23.5.2 states that the
>>>> maximum exit latency for USB2 LPM should be BESL + 10us. The BESL for
>>>> the affected device is advertised as 400us, which is still not enough
>>>> based on my testing result.
>>>>
>>>> So let's use the maximum permitted latency, 10000, to poll for U0
>>>> status to solve the issue.
>>>
>>> I don't recall all details, but it could be that disabling LPM before suspend
>>> is unnecessary. 
>>> At least xhci should be able to set a port to U3 from U1 and U2 (see xhci 4.15.1)
>>> so that is one change that could be done to xhci_bus_suspend()
>>
>> Yes, put the device to U3 directly does the trick.
> 
> As discussed, will you pick this series over the v2?
> Or is there anything I should improve for this one?
> 
> Kai-Heng

Added this to queueI

Thanks
-Mathias

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-06-23  8:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 10:18 [PATCH 1/2] xhci: Return if xHCI doesn't support LPM Kai-Heng Feng
2020-05-20 10:18 ` [PATCH 2/2] xhci: Poll for U0 after disabling USB2 LPM Kai-Heng Feng
2020-06-08  3:58   ` Kai-Heng Feng
2020-06-08  7:05     ` Greg Kroah-Hartman
2020-06-08 11:21   ` Mathias Nyman
2020-06-09 10:15     ` Kai-Heng Feng
2020-06-19 14:19       ` Kai-Heng Feng
2020-06-23  8:38         ` Mathias Nyman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.