linux-kernel - Re: [PATCH v2] xhci: Set port link to RxDetect if port is not enabled after resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <79060777-FF5B-4394-9A05-8573B6EF6578@canonical.com>
Date:   Thu, 7 May 2020 12:52:28 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc:     Mathias Nyman <mathias.nyman@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        "open list:USB NETWORKING DRIVERS" <linux-usb@...r.kernel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] xhci: Set port link to RxDetect if port is not enabled
 after resume



> On Apr 28, 2020, at 00:49, Mathias Nyman <mathias.nyman@...ux.intel.com> wrote:
> 
> On 27.4.2020 12.05, Kai-Heng Feng wrote:
>> 
>> 
>>> On Apr 23, 2020, at 19:25, Mathias Nyman <mathias.nyman@...ux.intel.com> 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