> On Apr 28, 2020, at 00:49, Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> wrote:
>
> On 27.4.2020 12.05, Kai-Heng Feng wrote:
>>
>>
>>> On Apr 23, 2020, at 19:25, Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> 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?
>
> Limiting your code to USB3 devices that xhi_bus_suspend forced to U3 would look
> something like this:
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 9eca1fe81061..0f16dd936ab8 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1789,6 +1789,15 @@ int xhci_bus_resume(struct usb_hcd *hcd)
> case XDEV_RESUME:
> /* resume already initiated */
> break;
> + case XDEV_DISABLED:
> + if (hcd->speed >= HCD_USB3) {
> + portsc = xhci_port_state_to_neutral(
> + portsc);
> + portsc &= ~PORT_PLS_MASK;
> + portsc |= PORT_LINK_STROBE |
> + XDEV_RXDETECT;
> + }
> + /* fall through for both USB3 abd USB2 */
> default:
> /* not in a resumeable state, ignore it */
> clear_bit(port_index,
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
