| Message ID | 20200311040456.25851-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xhci: Set port link to RxDetect if port is not enabled after resume | expand |
Hi Mathias, > On Mar 11, 2020, at 12:04, Kai-Heng Feng <kai.heng.feng@canonical.com> 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 <kai.heng.feng@canonical.com> 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 <kai.heng.feng@canonical.com> wrote: > > Hi Mathias, > >> On Mar 11, 2020, at 12:04, Kai-Heng Feng <kai.heng.feng@canonical.com> 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 <kai.heng.feng@canonical.com> > > 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 <kai.heng.feng@canonical.com> wrote: >> >> Hi Mathias, >> >>> On Mar 11, 2020, at 12:04, Kai-Heng Feng <kai.heng.feng@canonical.com> 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 <kai.heng.feng@canonical.com> >> >> 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 <mathias.nyman@linux.intel.com> wrote: > > On 22.4.2020 16.21, Kai-Heng Feng wrote: >> >> >>> On Mar 26, 2020, at 19:33, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: >>> >>> Hi Mathias, >>> >>>> On Mar 11, 2020, at 12:04, Kai-Heng Feng <kai.heng.feng@canonical.com> 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 <kai.heng.feng@canonical.com> >>> >>> 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 <mathias.nyman@linux.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. 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 <mathias.nyman@linux.intel.com> wrote: > > On 27.4.2020 12.05, Kai-Heng Feng wrote: >> >> >>> On Apr 23, 2020, at 19:25, Mathias Nyman <mathias.nyman@linux.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
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) {
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 <kai.heng.feng@canonical.com> --- drivers/usb/host/xhci-hub.c | 8 ++++++++ 1 file changed, 8 insertions(+)