On 13.1.2020 11.18, Kai-Heng Feng wrote:
On Fri, Jan 10, 2020 at 11:27 PM Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
---
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
