| Message ID | 20200103084008.3579-2-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] xhci: Ensure link state is U3 after setting USB_SS_PORT_LS_U3 | expand |
On 3.1.2020 10.40, 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. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/usb/host/xhci-hub.c | 8 +++++++- > drivers/usb/host/xhci-mem.c | 1 + > drivers/usb/host/xhci-ring.c | 1 + > drivers/usb/host/xhci.h | 1 + > 4 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index 2b2e9d004dbf..07886a1bce62 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -1310,11 +1310,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > spin_lock_irqsave(&xhci->lock, flags); > } > } > + if (link_state == USB_SS_PORT_LS_U0) > + reinit_completion(&ports[wIndex]->link_state_changed); All the other suspend and resume related port flags/completions are in struct xhci_bus_state. See for example rexit_done[]. Not sure that is a better place but at least it would be consistent. Could actually make sense to move more of them to the xhci_port structure, but perhaps in some later suspend/resume rework patch. > > 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_U0) { > + if (!wait_for_completion_timeout(&ports[wIndex]->link_state_changed, msecs_to_jiffies(100))) > + xhci_dbg(xhci, "missing U0 port change event for port %d-%d\n", hcd->self.busnum, wIndex + 1); We might be waiting for completion here in unnecessary. No completion is called if port is already in U0, either set by xhci_bus_resume(), or we race with a device initiated resume. Maybe read the current port link state first, and don't do anything if it's already in U0, or fail if it's in a state where we can't resume to U0. > + } else > + msleep(20); /* wait device to enter */ > spin_lock_irqsave(&xhci->lock, flags); Code might also be cleaner if we have separate if() statements for U0 and U3 link states, and skip the generic xhci_set_link_state() USB 3.2 specs only support PORT_LINK_STATE request feature selectors for U0, U1, U2, U3, SS.Disabled, Rx.Detect and Compliance mode. Out of these xhci driver already handles the SS.Disabled, Rx.detect and Compliance in separate if statements, and xHC hardware can't force a U1 or U2 state by writing the PLS field of the PORTSC register, so the xhci_set_link_state() here is only useful for U0 and U3. So maybe something like this: if (link_state == U0) if (active_link_state == U0) break else if (active_link_state != a proper link state) return error xhci_set_link_state(U0) wait_for_completion_timeout() break; if (link_state == U3) xhci_stop_device(slot_id) xhci_set_link_state(U3) for (max 10 tries) { msleep_range(~10ms) if (readl(PORTSC(PLS) == U3) break } break > > if (link_state == USB_SS_PORT_LS_U3) { > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 3b1388fa2f36..c760a28e3556 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -2268,6 +2268,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) > xhci->hw_ports[i].addr = &xhci->op_regs->port_status_base + > NUM_PORT_REGS * i; > xhci->hw_ports[i].hw_portnum = i; > + init_completion(&xhci->hw_ports[i].link_state_changed); > } > > xhci->rh_bw = kcalloc_node(num_ports, sizeof(*xhci->rh_bw), flags, > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index d23f7408c81f..44d91a53bf07 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(&port->link_state_changed); Completion will only be called if there was a port link change (PLC bit set) and link is in U0/U1/U2. Completion will also be called for device initiated resume even when no one is waiting for it. (probably harmless) -Mathias
On Fri, Jan 10, 2020 at 11:27 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 3.1.2020 10.40, 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. > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/usb/host/xhci-hub.c | 8 +++++++- > > drivers/usb/host/xhci-mem.c | 1 + > > drivers/usb/host/xhci-ring.c | 1 + > > drivers/usb/host/xhci.h | 1 + > > 4 files changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > > index 2b2e9d004dbf..07886a1bce62 100644 > > --- a/drivers/usb/host/xhci-hub.c > > +++ b/drivers/usb/host/xhci-hub.c > > @@ -1310,11 +1310,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, > > spin_lock_irqsave(&xhci->lock, flags); > > } > > } > > + if (link_state == USB_SS_PORT_LS_U0) > > + reinit_completion(&ports[wIndex]->link_state_changed); > > All the other suspend and resume related port flags/completions are > in struct xhci_bus_state. See for example rexit_done[]. > Not sure that is a better place but at least it would be consistent. > > Could actually make sense to move more of them to the xhci_port structure, > but perhaps in some later suspend/resume rework patch. Ok. Should I keep this part of the patch as is? Or move it to xhci_bus_state and probably move it back to xhci_port in later rework patch? > > > > 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_U0) { > > + if (!wait_for_completion_timeout(&ports[wIndex]->link_state_changed, msecs_to_jiffies(100))) > > + xhci_dbg(xhci, "missing U0 port change event for port %d-%d\n", hcd->self.busnum, wIndex + 1); > > We might be waiting for completion here in unnecessary. > No completion is called if port is already in U0, either set by > xhci_bus_resume(), or we race with a device initiated resume. Is there a way to know if device initiated resume is inplace? > > Maybe read the current port link state first, and don't do anything if it's > already in U0, or fail if it's in a state where we can't resume to U0. What happens if device initiated resume happens right after we query the PLS? > > > + } else > > + msleep(20); /* wait device to enter */ > > spin_lock_irqsave(&xhci->lock, flags); > > Code might also be cleaner if we have separate if() statements for U0 and U3 link > states, and skip the generic xhci_set_link_state() > > USB 3.2 specs only support PORT_LINK_STATE request feature selectors for > U0, U1, U2, U3, SS.Disabled, Rx.Detect and Compliance mode. > Out of these xhci driver already handles the SS.Disabled, Rx.detect and Compliance in > separate if statements, and xHC hardware can't force a U1 or U2 state by writing > the PLS field of the PORTSC register, so the xhci_set_link_state() here > is only useful for U0 and U3. > > So maybe something like this: > > if (link_state == U0) > if (active_link_state == U0) > break > else if (active_link_state != a proper link state) > return error > xhci_set_link_state(U0) > wait_for_completion_timeout() > break; > > if (link_state == U3) > xhci_stop_device(slot_id) > xhci_set_link_state(U3) > for (max 10 tries) { > msleep_range(~10ms) > if (readl(PORTSC(PLS) == U3) > break > } > break Ok, will rework the next patch in this direction. Kai-Heng > > > > if (link_state == USB_SS_PORT_LS_U3) { > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > > index 3b1388fa2f36..c760a28e3556 100644 > > --- a/drivers/usb/host/xhci-mem.c > > +++ b/drivers/usb/host/xhci-mem.c > > @@ -2268,6 +2268,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) > > xhci->hw_ports[i].addr = &xhci->op_regs->port_status_base + > > NUM_PORT_REGS * i; > > xhci->hw_ports[i].hw_portnum = i; > > + init_completion(&xhci->hw_ports[i].link_state_changed); > > } > > > > xhci->rh_bw = kcalloc_node(num_ports, sizeof(*xhci->rh_bw), flags, > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > > index d23f7408c81f..44d91a53bf07 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(&port->link_state_changed); > > Completion will only be called if there was a port link change (PLC bit set) > and link is in U0/U1/U2. Completion will also be called for device > initiated resume even when no one is waiting for it. (probably harmless) > > -Mathias
On 13.1.2020 11.18, Kai-Heng Feng wrote: > On Fri, Jan 10, 2020 at 11:27 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: >> >> On 3.1.2020 10.40, 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. >>> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>> --- >>> drivers/usb/host/xhci-hub.c | 8 +++++++- >>> drivers/usb/host/xhci-mem.c | 1 + >>> drivers/usb/host/xhci-ring.c | 1 + >>> drivers/usb/host/xhci.h | 1 + >>> 4 files changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c >>> index 2b2e9d004dbf..07886a1bce62 100644 >>> --- a/drivers/usb/host/xhci-hub.c >>> +++ b/drivers/usb/host/xhci-hub.c >>> @@ -1310,11 +1310,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, >>> spin_lock_irqsave(&xhci->lock, flags); >>> } >>> } >>> + if (link_state == USB_SS_PORT_LS_U0) >>> + reinit_completion(&ports[wIndex]->link_state_changed); >> >> All the other suspend and resume related port flags/completions are >> in struct xhci_bus_state. See for example rexit_done[]. >> Not sure that is a better place but at least it would be consistent. >> >> Could actually make sense to move more of them to the xhci_port structure, >> but perhaps in some later suspend/resume rework patch. > > Ok. Should I keep this part of the patch as is? Or move it to > xhci_bus_state and probably move it back to xhci_port in later rework > patch? Maybe move it to xhci_bus_state for now. > >>> >>> 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_U0) { >>> + if (!wait_for_completion_timeout(&ports[wIndex]->link_state_changed, msecs_to_jiffies(100))) >>> + xhci_dbg(xhci, "missing U0 port change event for port %d-%d\n", hcd->self.busnum, wIndex + 1); >> >> We might be waiting for completion here in unnecessary. >> No completion is called if port is already in U0, either set by >> xhci_bus_resume(), or we race with a device initiated resume. > > Is there a way to know if device initiated resume is inplace? Yes, before xhci interrupt handler handles the device initiated resume PLS is XDEV_RESUME and PLC is set. After the interrupt handler PLS goes from XDEV_RESUME to XDEV_RECOVERY to XDEV_U0. A bit is set for bus_state->port_remote_wakeup, and on usb core side also bus->resuming_ports |= bit is set, (having both may be a bit redundant, we might be able to get rid of bus_state->port_remote_wakeup, but not right now) > >> >> Maybe read the current port link state first, and don't do anything if it's >> already in U0, or fail if it's in a state where we can't resume to U0. > > What happens if device initiated resume happens right after we query the PLS? Not sure, fortunately the drivers task is to write XDEV_U0 to PLS both when we want a host initiated resume, or when we want to react on a device initiated resume. So hopefully that's ok, but this race exists. Better keep calling completion for both host and device initiated resume cases when port reaches U0/U1/U2 to avoid waiting for the completion unnecessary, like you current patch does. -Mathias
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 2b2e9d004dbf..07886a1bce62 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -1310,11 +1310,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, spin_lock_irqsave(&xhci->lock, flags); } } + if (link_state == USB_SS_PORT_LS_U0) + reinit_completion(&ports[wIndex]->link_state_changed); 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_U0) { + if (!wait_for_completion_timeout(&ports[wIndex]->link_state_changed, msecs_to_jiffies(100))) + xhci_dbg(xhci, "missing U0 port change event for port %d-%d\n", hcd->self.busnum, wIndex + 1); + } else + msleep(20); /* wait device to enter */ spin_lock_irqsave(&xhci->lock, flags); if (link_state == USB_SS_PORT_LS_U3) { diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b1388fa2f36..c760a28e3556 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -2268,6 +2268,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) xhci->hw_ports[i].addr = &xhci->op_regs->port_status_base + NUM_PORT_REGS * i; xhci->hw_ports[i].hw_portnum = i; + init_completion(&xhci->hw_ports[i].link_state_changed); } xhci->rh_bw = kcalloc_node(num_ports, sizeof(*xhci->rh_bw), flags, diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index d23f7408c81f..44d91a53bf07 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(&port->link_state_changed); /* 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..b86a664453dc 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1708,6 +1708,7 @@ struct xhci_port { int hw_portnum; int hcd_portnum; struct xhci_hub *rhub; + struct completion link_state_changed; }; struct xhci_hub {
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. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/usb/host/xhci-hub.c | 8 +++++++- drivers/usb/host/xhci-mem.c | 1 + drivers/usb/host/xhci-ring.c | 1 + drivers/usb/host/xhci.h | 1 + 4 files changed, 10 insertions(+), 1 deletion(-)