* [RESEND] [PATCH v2 1/3] xhci: Ensure link state is U3 after setting USB_SS_PORT_LS_U3 @ 2020-02-05 11:26 Kai-Heng Feng 2020-02-05 11:26 ` [RESEND] [PATCH v2 2/3] xhci: Wait until link state trainsits to U0 after setting USB_SS_PORT_LS_U0 Kai-Heng Feng 2020-02-05 11:26 ` [RESEND] [PATCH v2 3/3] USB: Disable LPM on WD19's Realtek Hub Kai-Heng Feng 0 siblings, 2 replies; 6+ messages in thread From: Kai-Heng Feng @ 2020-02-05 11:26 UTC (permalink / raw) To: mathias.nyman, gregkh, stern Cc: acelan.kao, linux-usb, linux-kernel, Kai-Heng Feng The xHCI spec doesn't specify the upper bound of U3 transition time. For some devices 20ms is not enough, so we need to make sure the link state is in U3 before further actions. I've tried to use U3 Entry Capability by setting U3 Entry Enable in config register, however the port change event for U3 transition interrupts the system suspend process. For now let's use the less ideal method by polling PLS. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v2: - Remove some redundant debug messages. - Use msleep loop outside if spinlock to stop pegging CPU. drivers/usb/host/xhci-hub.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 7a3a29e5e9d2..d3c5bcf76755 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -1313,7 +1313,16 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, xhci_set_link_state(xhci, ports[wIndex], link_state); spin_unlock_irqrestore(&xhci->lock, flags); - msleep(20); /* wait device to enter */ + if (link_state == USB_SS_PORT_LS_U3) { + int retries = 10; + + while (retries--) { + msleep(10); /* wait device to enter */ + temp = readl(ports[wIndex]->addr); + if ((temp & PORT_PLS_MASK) == XDEV_U3) + break; + } + } spin_lock_irqsave(&xhci->lock, flags); temp = readl(ports[wIndex]->addr); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RESEND] [PATCH v2 2/3] xhci: Wait until link state trainsits to U0 after setting USB_SS_PORT_LS_U0 2020-02-05 11:26 [RESEND] [PATCH v2 1/3] xhci: Ensure link state is U3 after setting USB_SS_PORT_LS_U3 Kai-Heng Feng @ 2020-02-05 11:26 ` Kai-Heng Feng 2020-02-10 14:31 ` Mathias Nyman 2020-02-05 11:26 ` [RESEND] [PATCH v2 3/3] USB: Disable LPM on WD19's Realtek Hub Kai-Heng Feng 1 sibling, 1 reply; 6+ messages in thread From: Kai-Heng Feng @ 2020-02-05 11:26 UTC (permalink / raw) To: mathias.nyman, gregkh, stern Cc: acelan.kao, linux-usb, linux-kernel, Kai-Heng Feng Like U3 case, xHCI spec doesn't specify the upper bound of U0 transition time. The 20ms is not enough for some devices. Intead of polling PLS or PLC, we can facilitate the port change event to know that the link transits to U0 is completed. While at it, also separate U0 and U3 case to make the code cleaner. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v2: - Seperate U0 and U3 case. - Prevent setting U0 from non-U state. - Move the completion from per port to bus_state. drivers/usb/host/xhci-hub.c | 43 +++++++++++++++++++++++++----------- drivers/usb/host/xhci-mem.c | 2 ++ drivers/usb/host/xhci-ring.c | 1 + drivers/usb/host/xhci.h | 1 + 4 files changed, 34 insertions(+), 13 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index d3c5bcf76755..0a5d8b28b99f 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -1297,7 +1297,32 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, wIndex, link_state); goto error; } + + if (link_state == USB_SS_PORT_LS_U0) { + if ((temp & PORT_PLS_MASK) == XDEV_U0) + break; + + if (!((temp & PORT_PLS_MASK) == XDEV_U1 || + (temp & PORT_PLS_MASK) == XDEV_U2 || + (temp & PORT_PLS_MASK) == XDEV_U3)) { + xhci_warn(xhci, "Can only set port %d to U0 from U state\n", + wIndex); + goto error; + } + reinit_completion(&bus_state->link_change_done[wIndex]); + xhci_set_link_state(xhci, ports[wIndex], USB_SS_PORT_LS_U0); + spin_unlock_irqrestore(&xhci->lock, flags); + if (!wait_for_completion_timeout(&bus_state->link_change_done[wIndex], + msecs_to_jiffies(100))) + xhci_dbg(xhci, "missing U0 port change event for port %d\n", + wIndex); + spin_lock_irqsave(&xhci->lock, flags); + temp = readl(ports[wIndex]->addr); + break; + } + if (link_state == USB_SS_PORT_LS_U3) { + int retries = 10; slot_id = xhci_find_slot_id_by_port(hcd, xhci, wIndex + 1); if (slot_id) { @@ -1308,26 +1333,18 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, xhci_stop_device(xhci, slot_id, 1); spin_lock_irqsave(&xhci->lock, flags); } - } - - xhci_set_link_state(xhci, ports[wIndex], link_state); - - spin_unlock_irqrestore(&xhci->lock, flags); - if (link_state == USB_SS_PORT_LS_U3) { - int retries = 10; - + xhci_set_link_state(xhci, ports[wIndex], USB_SS_PORT_LS_U3); + spin_unlock_irqrestore(&xhci->lock, flags); while (retries--) { msleep(10); /* wait device to enter */ temp = readl(ports[wIndex]->addr); if ((temp & PORT_PLS_MASK) == XDEV_U3) break; } - } - spin_lock_irqsave(&xhci->lock, flags); - - temp = readl(ports[wIndex]->addr); - if (link_state == USB_SS_PORT_LS_U3) + spin_lock_irqsave(&xhci->lock, flags); + temp = readl(ports[wIndex]->addr); bus_state->suspended_ports |= 1 << wIndex; + } break; case USB_PORT_FEAT_POWER: /* diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b1388fa2f36..aceb8c1af775 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -2531,6 +2531,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) xhci->usb3_rhub.bus_state.resume_done[i] = 0; /* Only the USB 2.0 completions will ever be used. */ init_completion(&xhci->usb2_rhub.bus_state.rexit_done[i]); + init_completion(&xhci->usb2_rhub.bus_state.link_change_done[i]); + init_completion(&xhci->usb3_rhub.bus_state.link_change_done[i]); } if (scratchpad_alloc(xhci, flags)) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index d23f7408c81f..4d0f8dab069a 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1677,6 +1677,7 @@ static void handle_port_status(struct xhci_hcd *xhci, (portsc & PORT_PLS_MASK) == XDEV_U1 || (portsc & PORT_PLS_MASK) == XDEV_U2)) { xhci_dbg(xhci, "resume SS port %d finished\n", port_id); + complete(&bus_state->link_change_done[hcd_portnum]); /* We've just brought the device into U0/1/2 through either the * Resume state after a device remote wakeup, or through the * U3Exit state after a host-initiated resume. If it's a device diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 13d8838cd552..b5d443ce0750 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1694,6 +1694,7 @@ struct xhci_bus_state { /* Which ports are waiting on RExit to U0 transition. */ unsigned long rexit_ports; struct completion rexit_done[USB_MAXCHILDREN]; + struct completion link_change_done[USB_MAXCHILDREN]; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND] [PATCH v2 2/3] xhci: Wait until link state trainsits to U0 after setting USB_SS_PORT_LS_U0 2020-02-05 11:26 ` [RESEND] [PATCH v2 2/3] xhci: Wait until link state trainsits to U0 after setting USB_SS_PORT_LS_U0 Kai-Heng Feng @ 2020-02-10 14:31 ` Mathias Nyman 0 siblings, 0 replies; 6+ messages in thread From: Mathias Nyman @ 2020-02-10 14:31 UTC (permalink / raw) To: Kai-Heng Feng, mathias.nyman, gregkh, stern Cc: acelan.kao, linux-usb, linux-kernel On 5.2.2020 13.26, Kai-Heng Feng wrote: > Like U3 case, xHCI spec doesn't specify the upper bound of U0 transition > time. The 20ms is not enough for some devices. > > Intead of polling PLS or PLC, we can facilitate the port change event to > know that the link transits to U0 is completed. > > While at it, also separate U0 and U3 case to make the code cleaner. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > v2: > - Seperate U0 and U3 case. > - Prevent setting U0 from non-U state. > - Move the completion from per port to bus_state. > > drivers/usb/host/xhci-hub.c | 43 +++++++++++++++++++++++++----------- > drivers/usb/host/xhci-mem.c | 2 ++ > drivers/usb/host/xhci-ring.c | 1 + > drivers/usb/host/xhci.h | 1 + > 4 files changed, 34 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index d3c5bcf76755..0a5d8b28b99f 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -1297,7 +1297,32 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > wIndex, link_state); > goto error; > } > + > + if (link_state == USB_SS_PORT_LS_U0) { > + if ((temp & PORT_PLS_MASK) == XDEV_U0) > + break; > + > + if (!((temp & PORT_PLS_MASK) == XDEV_U1 || > + (temp & PORT_PLS_MASK) == XDEV_U2 || > + (temp & PORT_PLS_MASK) == XDEV_U3)) { > + xhci_warn(xhci, "Can only set port %d to U0 from U state\n", > + wIndex); Port link state could be XDEV_RESUME or maybe even XDEV_RECOVERY if we race with a device initiated resume. I'll need to check how we should handle those. > + goto error; > + } > + reinit_completion(&bus_state->link_change_done[wIndex]); > + xhci_set_link_state(xhci, ports[wIndex], USB_SS_PORT_LS_U0); > + spin_unlock_irqrestore(&xhci->lock, flags); > + if (!wait_for_completion_timeout(&bus_state->link_change_done[wIndex], > + msecs_to_jiffies(100))) Minor nit: rename link_change_done[] to u3exit_done[] see xhci 4.19.1.2.13.2 "U3", and the Host Initiated resume substate transition > + xhci_dbg(xhci, "missing U0 port change event for port %d\n", > + wIndex); > + spin_lock_irqsave(&xhci->lock, flags); > + temp = readl(ports[wIndex]->addr); > + break; > + } > + > if (link_state == USB_SS_PORT_LS_U3) { > + int retries = 10; > slot_id = xhci_find_slot_id_by_port(hcd, xhci, > wIndex + 1); > if (slot_id) { > @@ -1308,26 +1333,18 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > xhci_stop_device(xhci, slot_id, 1); > spin_lock_irqsave(&xhci->lock, flags); > } > - } > - > - xhci_set_link_state(xhci, ports[wIndex], link_state); > - > - spin_unlock_irqrestore(&xhci->lock, flags); > - if (link_state == USB_SS_PORT_LS_U3) { > - int retries = 10; > - > + xhci_set_link_state(xhci, ports[wIndex], USB_SS_PORT_LS_U3); > + spin_unlock_irqrestore(&xhci->lock, flags); > while (retries--) { > msleep(10); /* wait device to enter */ > temp = readl(ports[wIndex]->addr); > if ((temp & PORT_PLS_MASK) == XDEV_U3) > break; > } > - } > - spin_lock_irqsave(&xhci->lock, flags); > - > - temp = readl(ports[wIndex]->addr); > - if (link_state == USB_SS_PORT_LS_U3) > + spin_lock_irqsave(&xhci->lock, flags); > + temp = readl(ports[wIndex]->addr); > bus_state->suspended_ports |= 1 << wIndex; > + } > break; > case USB_PORT_FEAT_POWER: > /* > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 3b1388fa2f36..aceb8c1af775 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -2531,6 +2531,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > xhci->usb3_rhub.bus_state.resume_done[i] = 0; > /* Only the USB 2.0 completions will ever be used. */ > init_completion(&xhci->usb2_rhub.bus_state.rexit_done[i]); > + init_completion(&xhci->usb2_rhub.bus_state.link_change_done[i]); Not needed for usb2, it doesn't support Set Port Feature PORT_LINK_STATE requests > + init_completion(&xhci->usb3_rhub.bus_state.link_change_done[i]); rename to u3exit_done > } > > if (scratchpad_alloc(xhci, flags)) > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index d23f7408c81f..4d0f8dab069a 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -1677,6 +1677,7 @@ static void handle_port_status(struct xhci_hcd *xhci, > (portsc & PORT_PLS_MASK) == XDEV_U1 || > (portsc & PORT_PLS_MASK) == XDEV_U2)) { > xhci_dbg(xhci, "resume SS port %d finished\n", port_id); > + complete(&bus_state->link_change_done[hcd_portnum]); > /* We've just brought the device into U0/1/2 through either the > * Resume state after a device remote wakeup, or through the > * U3Exit state after a host-initiated resume. If it's a device > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 13d8838cd552..b5d443ce0750 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1694,6 +1694,7 @@ struct xhci_bus_state { > /* Which ports are waiting on RExit to U0 transition. */ > unsigned long rexit_ports; > struct completion rexit_done[USB_MAXCHILDREN]; > + struct completion link_change_done[USB_MAXCHILDREN]; > }; > > I can do these minor changes and try out these first two patches. No need to resend. The last 3/3 patch isn't really tied to the first two, or even xhci, and should probably go as a separate patch. -Mathias ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RESEND] [PATCH v2 3/3] USB: Disable LPM on WD19's Realtek Hub 2020-02-05 11:26 [RESEND] [PATCH v2 1/3] xhci: Ensure link state is U3 after setting USB_SS_PORT_LS_U3 Kai-Heng Feng 2020-02-05 11:26 ` [RESEND] [PATCH v2 2/3] xhci: Wait until link state trainsits to U0 after setting USB_SS_PORT_LS_U0 Kai-Heng Feng @ 2020-02-05 11:26 ` Kai-Heng Feng 2020-03-11 3:32 ` Kai-Heng Feng 1 sibling, 1 reply; 6+ messages in thread From: Kai-Heng Feng @ 2020-02-05 11:26 UTC (permalink / raw) To: mathias.nyman, gregkh, stern Cc: acelan.kao, linux-usb, linux-kernel, Kai-Heng Feng Realtek Hub (0bda:0x0487) used in Dell Dock WD19 sometimes drops off the bus when bringing underlying ports from U3 to U0. Disabling LPM on the hub during setting link state is not enough, so let's disable LPM completely for this hub. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v2: - Use quirk instead of the original approach. drivers/usb/core/quirks.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c index 6b6413073584..2fb7c1602280 100644 --- a/drivers/usb/core/quirks.c +++ b/drivers/usb/core/quirks.c @@ -371,6 +371,9 @@ static const struct usb_device_id usb_quirk_list[] = { { USB_DEVICE(0x0b05, 0x17e0), .driver_info = USB_QUIRK_IGNORE_REMOTE_WAKEUP }, + /* Realtek hub in Dell WD19 (Type-C) */ + { USB_DEVICE(0x0bda, 0x0487), .driver_info = USB_QUIRK_NO_LPM }, + /* Action Semiconductor flash disk */ { USB_DEVICE(0x10d6, 0x2200), .driver_info = USB_QUIRK_STRING_FETCH_255 }, -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND] [PATCH v2 3/3] USB: Disable LPM on WD19's Realtek Hub 2020-02-05 11:26 ` [RESEND] [PATCH v2 3/3] USB: Disable LPM on WD19's Realtek Hub Kai-Heng Feng @ 2020-03-11 3:32 ` Kai-Heng Feng 2020-03-11 14:15 ` Alan Stern 0 siblings, 1 reply; 6+ messages in thread From: Kai-Heng Feng @ 2020-03-11 3:32 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alan Stern, Mathias Nyman, AceLan Kao, open list:USB NETWORKING DRIVERS, open list Hi Greg, > On Feb 5, 2020, at 19:26, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Realtek Hub (0bda:0x0487) used in Dell Dock WD19 sometimes drops off the > bus when bringing underlying ports from U3 to U0. > > Disabling LPM on the hub during setting link state is not enough, so > let's disable LPM completely for this hub. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> As Mathias stated, this patch can be considered as a separate one. Can you please review and merge this patch? Kai-Heng > --- > v2: > - Use quirk instead of the original approach. > > drivers/usb/core/quirks.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c > index 6b6413073584..2fb7c1602280 100644 > --- a/drivers/usb/core/quirks.c > +++ b/drivers/usb/core/quirks.c > @@ -371,6 +371,9 @@ static const struct usb_device_id usb_quirk_list[] = { > { USB_DEVICE(0x0b05, 0x17e0), .driver_info = > USB_QUIRK_IGNORE_REMOTE_WAKEUP }, > > + /* Realtek hub in Dell WD19 (Type-C) */ > + { USB_DEVICE(0x0bda, 0x0487), .driver_info = USB_QUIRK_NO_LPM }, > + > /* Action Semiconductor flash disk */ > { USB_DEVICE(0x10d6, 0x2200), .driver_info = > USB_QUIRK_STRING_FETCH_255 }, > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND] [PATCH v2 3/3] USB: Disable LPM on WD19's Realtek Hub 2020-03-11 3:32 ` Kai-Heng Feng @ 2020-03-11 14:15 ` Alan Stern 0 siblings, 0 replies; 6+ messages in thread From: Alan Stern @ 2020-03-11 14:15 UTC (permalink / raw) To: Kai-Heng Feng Cc: Greg Kroah-Hartman, Mathias Nyman, AceLan Kao, open list:USB NETWORKING DRIVERS, open list On Wed, 11 Mar 2020, Kai-Heng Feng wrote: > Hi Greg, > > > On Feb 5, 2020, at 19:26, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > > > Realtek Hub (0bda:0x0487) used in Dell Dock WD19 sometimes drops off the > > bus when bringing underlying ports from U3 to U0. > > > > Disabling LPM on the hub during setting link state is not enough, so > > let's disable LPM completely for this hub. > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > As Mathias stated, this patch can be considered as a separate one. > Can you please review and merge this patch? > > Kai-Heng > > > --- > > v2: > > - Use quirk instead of the original approach. > > > > drivers/usb/core/quirks.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c > > index 6b6413073584..2fb7c1602280 100644 > > --- a/drivers/usb/core/quirks.c > > +++ b/drivers/usb/core/quirks.c > > @@ -371,6 +371,9 @@ static const struct usb_device_id usb_quirk_list[] = { > > { USB_DEVICE(0x0b05, 0x17e0), .driver_info = > > USB_QUIRK_IGNORE_REMOTE_WAKEUP }, > > > > + /* Realtek hub in Dell WD19 (Type-C) */ > > + { USB_DEVICE(0x0bda, 0x0487), .driver_info = USB_QUIRK_NO_LPM }, > > + > > /* Action Semiconductor flash disk */ > > { USB_DEVICE(0x10d6, 0x2200), .driver_info = > > USB_QUIRK_STRING_FETCH_255 }, > > -- > > 2.17.1 Acked-by: Alan Stern <stern@rowland.harvard.edu> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-11 14:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-05 11:26 [RESEND] [PATCH v2 1/3] xhci: Ensure link state is U3 after setting USB_SS_PORT_LS_U3 Kai-Heng Feng 2020-02-05 11:26 ` [RESEND] [PATCH v2 2/3] xhci: Wait until link state trainsits to U0 after setting USB_SS_PORT_LS_U0 Kai-Heng Feng 2020-02-10 14:31 ` Mathias Nyman 2020-02-05 11:26 ` [RESEND] [PATCH v2 3/3] USB: Disable LPM on WD19's Realtek Hub Kai-Heng Feng 2020-03-11 3:32 ` Kai-Heng Feng 2020-03-11 14:15 ` Alan Stern
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).