> On Mar 6, 2020, at 03:02, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, 5 Mar 2020, Kai-Heng Feng 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. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> >> --- >> >> P.S. This is and differnt (and should be more correct) approach to solve >> https://lore.kernel.org/lkml/20191129174115.31683-1-kai.heng.feng@xxxxxxxxxxxxx/ >> >> drivers/usb/core/hub.c | 14 +++++++++++++- >> 1 file changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >> index 54cd8ef795ec..352e33c84d6a 100644 >> --- a/drivers/usb/core/hub.c >> +++ b/drivers/usb/core/hub.c >> @@ -3537,12 +3537,24 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) >> >> /* Skip the initial Clear-Suspend step for a remote wakeup */ >> status = hub_port_status(hub, port1, &portstatus, &portchange); >> - if (status == 0 && !port_is_suspended(hub, portstatus)) { >> + if (status == 0 && !port_is_suspended(hub, portstatus) >> + && (portstatus & USB_PORT_STAT_ENABLE)) { > > This doesn't look right. We want to skip the step the clears the > port's suspend feature whenever the feature is already clear, > regardless of whether the port is enabled. > > Besides, isn't this new test racy? Suppose the port was enabled when > we called hub_port_status above, but it got disabled in the nanoseconds > since then? > >> if (portchange & USB_PORT_STAT_C_SUSPEND) >> pm_wakeup_event(&udev->dev, 0); >> goto SuspendCleared; >> } >> >> + /* xHCI 4.19.1.2.1 */ >> + if (hub_is_superspeed(hub->hdev)) { >> + if (!(portstatus & USB_PORT_STAT_ENABLE)) >> + status = hub_set_port_link_state(hub, port1, >> + USB_SS_PORT_LS_RX_DETECT); >> + >> + if (status) >> + dev_dbg(&port_dev->dev, >> + "can't set to RxDetect, status %d\n", status); >> + } >> + > > So maybe this part belongs later in the routine. Or maybe it shouldn't > be in the hub driver at all -- since it is xHCI-specific, shouldn't it > go into the xhci-hcd driver? Ok, I'll put this change into xhci instead. It should also avoid the race you mentioned above. Kai-Heng > > Alan Stern > >> /* see 7.1.7.7; affects power usage, but not budgeting */ >> if (hub_is_superspeed(hub->hdev)) >> status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U0);
