On Apr 28, 2020, at 00:49, Mathias Nyman [off-list ref] wrote:

On 27.4.2020 12.05, Kai-Heng Feng wrote:

quoted

quoted

On Apr 23, 2020, at 19:25, Mathias Nyman [off-list ref] wrote:

Was this roothub port forcefully suspended xhci_bus_suspend()?
i.e. was a bit set in bus_state->bus_suspended for this port?

No, it's a USB3 device so it was set to U3 via USB_PORT_FEAT_LINK_STATE.

Logs show port was first forced by xhci_bus_suspend() to U3  ("port 0 not
suspended" message)
and only later set to U3 by USB_PORT_FEAT_LINK_STATE.
Seems line wrong order or race.

The "port 0 not suspended" is actually for 3-1, if we print bus num and port + 1:
[  213.732977] xhci_hcd 0000:3f:00.0: port 3-1 not suspended

For port 4-1 it's always suspended before suspend the bus.
I'll send a patch to adjust the debug message for better clarity.

while suspended we get a port event about a connect status change,
showing port link state is in disabled.
Cherry picked messages:

[ 1330.021450] xhci_hcd 0000:3f:00.0: port 0 not suspended
[ 1330.036822] xhci_hcd 0000:3f:00.0: Set port 4-1 link state, portsc: 0x1203, write 0x11261
[ 1331.020736] xhci_hcd 0000:3f:00.0: Port change event, 4-1, id 1, portsc: 0x20280
[ 1331.020738] xhci_hcd 0000:3f:00.0: resume root hub
[ 1331.020741] xhci_hcd 0000:3f:00.0: handle_port_status: starting port polling.

If we force the port link state to U3 in xhci_bus_suspend() maybe it would make
sense to try and recover it in xhci_bus_resume(). But only for that forced
port.

None of the previous suspend/resume cycles in the logs went smooth either.
Each time device 4-1 was forced to U3 bus xhci_bus_suspend(), and at resume the
link was stuck in polling until timeout, after which it went to compliance mode,
and had to be warm reset to recover.

If my observation above is true, port 4-1 is indeed suspended by usb_port_suspend() rather than xhci_bus_suspend().

We could add the code to recover USB3 ports from disabled state in case we
forced them to U3, but the rootcause of theus suspend/resume issues should
be found as well

Seems like the issue doesn't happen if the host system use S2Idle instead of S3.
However, I can't see any difference in xHCI driver with different suspend methods.
Maybe the root cause is that, ASMedia controller and SMSC hub are just quirky?

This doesn't work because port 4-1 isn't suspended by xhci_bus_suspend().

Maybe we can restrict the case to ports that are suspended by USB_PORT_FEAT_LINK_STATE.
Is the following patch looks good to you?

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index f37316d2c8fa..dc2e14ea308d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c

@@ -1787,6 +1787,16 @@ int xhci_bus_resume(struct usb_hcd *hcd)
                        clear_bit(port_index, &bus_state->bus_suspended);
                        continue;
                }
+
+               if (bus_state->suspended_ports & (1 << port_index)) {
+                       if ((portsc & PORT_PLS_MASK) == XDEV_DISABLED &&
+                           hcd->speed >= HCD_USB3) {
+                               portsc = xhci_port_state_to_neutral(portsc);
+                               portsc &= ~PORT_PLS_MASK;
+                               portsc |= PORT_LINK_STROBE | XDEV_RXDETECT;
+                       }
+               }
+
                /* resume if we suspended the link, and it is still suspended */
                if (test_bit(port_index, &bus_state->bus_suspended))
                        switch (portsc & PORT_PLS_MASK) {

Kai-Heng

-Mathias