On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
then connects to ASMedia xHCI's root hub:
/: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
|__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
|__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
The port is disabled after resume:
xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
According to xHCI 4.19.1.2.1, we should set link to RxDetect to transit
it from disabled state to disconnected state, which allows the port to
be set to U0 and completes the resume process.
My own test shows port can still resume when it's not enabled, as long
as its link is in U states. So constrain the new logic only when link is
not in any U state.
Signed-off-by: Kai-Heng Feng <[email protected]>
---
drivers/usb/host/xhci-hub.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index a9c87eb8951e..263f9a9237a1 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1776,6 +1776,14 @@ int xhci_bus_resume(struct usb_hcd *hcd)
clear_bit(port_index, &bus_state->bus_suspended);
continue;
}
+
+ /* 4.19.1.2.1 */
+ if (!(portsc & PORT_PE) && (portsc & PORT_PLS_MASK) > XDEV_U3) {
+ 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) {
--
2.17.1
Hi Mathias,
> On Mar 11, 2020, at 12:04, Kai-Heng Feng <[email protected]> wrote:
>
> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
> then connects to ASMedia xHCI's root hub:
>
> /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
> |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
>
> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
>
> The port is disabled after resume:
> xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
>
> According to xHCI 4.19.1.2.1, we should set link to RxDetect to transit
> it from disabled state to disconnected state, which allows the port to
> be set to U0 and completes the resume process.
>
> My own test shows port can still resume when it's not enabled, as long
> as its link is in U states. So constrain the new logic only when link is
> not in any U state.
>
> Signed-off-by: Kai-Heng Feng <[email protected]>
Do you think this is a proper fix?
Kai-Heng
> ---
> drivers/usb/host/xhci-hub.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index a9c87eb8951e..263f9a9237a1 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1776,6 +1776,14 @@ int xhci_bus_resume(struct usb_hcd *hcd)
> clear_bit(port_index, &bus_state->bus_suspended);
> continue;
> }
> +
> + /* 4.19.1.2.1 */
> + if (!(portsc & PORT_PE) && (portsc & PORT_PLS_MASK) > XDEV_U3) {
> + 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) {
> --
> 2.17.1
>
> On Mar 26, 2020, at 19:33, Kai-Heng Feng <[email protected]> wrote:
>
> Hi Mathias,
>
>> On Mar 11, 2020, at 12:04, Kai-Heng Feng <[email protected]> wrote:
>>
>> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
>> then connects to ASMedia xHCI's root hub:
>>
>> /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
>> |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
>>
>> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
>> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
>> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
>>
>> The port is disabled after resume:
>> xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
>>
>> According to xHCI 4.19.1.2.1, we should set link to RxDetect to transit
>> it from disabled state to disconnected state, which allows the port to
>> be set to U0 and completes the resume process.
>>
>> My own test shows port can still resume when it's not enabled, as long
>> as its link is in U states. So constrain the new logic only when link is
>> not in any U state.
>>
>> Signed-off-by: Kai-Heng Feng <[email protected]>
>
> Do you think this is a proper fix?
Another gentle ping...
>
> Kai-Heng
>
>> ---
>> drivers/usb/host/xhci-hub.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index a9c87eb8951e..263f9a9237a1 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -1776,6 +1776,14 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>> clear_bit(port_index, &bus_state->bus_suspended);
>> continue;
>> }
>> +
>> + /* 4.19.1.2.1 */
>> + if (!(portsc & PORT_PE) && (portsc & PORT_PLS_MASK) > XDEV_U3) {
>> + 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) {
>> --
>> 2.17.1
>>
>
On 22.4.2020 16.21, Kai-Heng Feng wrote:
>
>
>> On Mar 26, 2020, at 19:33, Kai-Heng Feng <[email protected]> wrote:
>>
>> Hi Mathias,
>>
>>> On Mar 11, 2020, at 12:04, Kai-Heng Feng <[email protected]> wrote:
>>>
>>> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
>>> then connects to ASMedia xHCI's root hub:
>>>
>>> /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>>> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
>>> |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
>>>
>>> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
>>> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
>>> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
>>>
>>> The port is disabled after resume:
>>> xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
That sound like the actual problem.
Any idea how the port link ends up ends up in the disabled state?
Was there a request to set port feature, link state to SS.Disabled?
Or timeouts at resume from U3
U3 -> Recovery (timeout) -> Rx.Detect -> Polling (Timeout) -> SS.Disabled
>>>
>>> According to xHCI 4.19.1.2.1, we should set link to RxDetect to transit
>>> it from disabled state to disconnected state, which allows the port to
>>> be set to U0 and completes the resume process.
>>>
>>> My own test shows port can still resume when it's not enabled, as long
>>> as its link is in U states. So constrain the new logic only when link is
>>> not in any U state.
Sounds more like re-enumerate than proper resume if we go to RxDetect in between
>>>
>>> Signed-off-by: Kai-Heng Feng <[email protected]>
>>
>> Do you think this is a proper fix?
>
> Another gentle ping...
>
>>
>> Kai-Heng
>>
>>> ---
>>> drivers/usb/host/xhci-hub.c | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>>> index a9c87eb8951e..263f9a9237a1 100644
>>> --- a/drivers/usb/host/xhci-hub.c
>>> +++ b/drivers/usb/host/xhci-hub.c
>>> @@ -1776,6 +1776,14 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>>> clear_bit(port_index, &bus_state->bus_suspended);
>>> continue;
>>> }
>>> +
>>> + /* 4.19.1.2.1 */
>>> + if (!(portsc & PORT_PE) && (portsc & PORT_PLS_MASK) > XDEV_U3) {
>>> + portsc = xhci_port_state_to_neutral(portsc);
>>> + portsc &= ~PORT_PLS_MASK;
>>> + portsc |= PORT_LINK_STROBE | XDEV_RXDETECT;
>>> + }
>>> +
This doesn't look like the right solution.
This will set the link state to RxDetect for any USB3 roothub port
that is currently not in U0/U1/U2/U3 or Recovery.
Was this roothub port forcefully suspended xhci_bus_suspend()?
i.e. was a bit set in bus_state->bus_suspended for this port?
Any other logs or traces that could explan what's going on?
Is the resume due to a wakeup (device initiated resume) from this device,
or just a host initiated resume?
Thanks
-Mathias
> On Apr 23, 2020, at 19:25, Mathias Nyman <[email protected]> wrote:
>
> On 22.4.2020 16.21, Kai-Heng Feng wrote:
>>
>>
>>> On Mar 26, 2020, at 19:33, Kai-Heng Feng <[email protected]> wrote:
>>>
>>> Hi Mathias,
>>>
>>>> On Mar 11, 2020, at 12:04, Kai-Heng Feng <[email protected]> wrote:
>>>>
>>>> On Dell TB16, Realtek USB ethernet (r8152) connects to an SMSC hub which
>>>> then connects to ASMedia xHCI's root hub:
>>>>
>>>> /: Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>>>> |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/7p, 5000M
>>>> |__ Port 2: Dev 3, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
>>>>
>>>> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
>>>> Bus 004 Device 002: ID 0424:5537 Standard Microsystems Corp. USB5537B
>>>> Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. RTL8153 Gigabit Ethernet Adapter
>>>>
>>>> The port is disabled after resume:
>>>> xhci_hcd 0000:3f:00.0: Get port status 4-1 read: 0x280, return 0x280
>
> That sound like the actual problem.
> Any idea how the port link ends up ends up in the disabled state?
Not really. The port link status just became SS.Disabled after S3 wakeup.
> Was there a request to set port feature, link state to SS.Disabled?
No, it was correctly set to U3 prior to system S3.
> Or timeouts at resume from U3
> U3 -> Recovery (timeout) -> Rx.Detect -> Polling (Timeout) -> SS.Disabled
PLS is SS.Disabled, by reading portsc in xhci_bus_resume().
>
>>>>
>>>> According to xHCI 4.19.1.2.1, we should set link to RxDetect to transit
>>>> it from disabled state to disconnected state, which allows the port to
>>>> be set to U0 and completes the resume process.
>>>>
>>>> My own test shows port can still resume when it's not enabled, as long
>>>> as its link is in U states. So constrain the new logic only when link is
>>>> not in any U state.
>
> Sounds more like re-enumerate than proper resume if we go to RxDetect in between
Ok. So should we handle this case inside xhci_bus_resume() or somewhere else?
>
>>>>
>>>> Signed-off-by: Kai-Heng Feng <[email protected]>
>>>
>>> Do you think this is a proper fix?
>>
>> Another gentle ping...
>>
>>>
>>> Kai-Heng
>>>
>>>> ---
>>>> drivers/usb/host/xhci-hub.c | 8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>>>> index a9c87eb8951e..263f9a9237a1 100644
>>>> --- a/drivers/usb/host/xhci-hub.c
>>>> +++ b/drivers/usb/host/xhci-hub.c
>>>> @@ -1776,6 +1776,14 @@ int xhci_bus_resume(struct usb_hcd *hcd)
>>>> clear_bit(port_index, &bus_state->bus_suspended);
>>>> continue;
>>>> }
>>>> +
>>>> + /* 4.19.1.2.1 */
>>>> + if (!(portsc & PORT_PE) && (portsc & PORT_PLS_MASK) > XDEV_U3) {
>>>> + portsc = xhci_port_state_to_neutral(portsc);
>>>> + portsc &= ~PORT_PLS_MASK;
>>>> + portsc |= PORT_LINK_STROBE | XDEV_RXDETECT;
>>>> + }
>>>> +
> This doesn't look like the right solution.
> This will set the link state to RxDetect for any USB3 roothub port
> that is currently not in U0/U1/U2/U3 or Recovery.
>
> 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.
>
> Any other logs or traces that could explan what's going on?
Read port status in xhci_bus_resume():
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index f37316d2c8fa..c2e9ac3dc227 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1778,7 +1778,7 @@ int xhci_bus_resume(struct usb_hcd *hcd)
port_index = max_ports;
while (port_index--) {
portsc = readl(ports[port_index]->addr);
-
+ xhci_info(xhci, "DEBUG: port %d-%d, portsc 0x%0x pls %d\n", hcd->self.busnum, port_index + 1, portsc, (portsc & PORT_PLS_MASK) >> 5);
/* warm reset CAS limited ports stuck in polling/compliance */
if ((xhci->quirks & XHCI_MISSING_CAS) &&
(hcd->speed >= HCD_USB3) &&
Here's the log. The log is not full because it's too big to upload:
https://pastebin.ubuntu.com/p/24hzqx2nGD/
> Is the resume due to a wakeup (device initiated resume) from this device,
> or just a host initiated resume?
It's a host initiated resume.
>
> Thanks
> -Mathias
On 27.4.2020 12.05, Kai-Heng Feng wrote:
>
>
>> On Apr 23, 2020, at 19:25, Mathias Nyman <[email protected]> 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.
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.
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
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,
-Mathias
> On Apr 28, 2020, at 00:49, Mathias Nyman <[email protected]> wrote:
>
> On 27.4.2020 12.05, Kai-Heng Feng wrote:
>>
>>
>>> On Apr 23, 2020, at 19:25, Mathias Nyman <[email protected]> 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