Hi, Mathias Nyman <mathias.ny...@linux.intel.com> writes: > usb2 ports need to signal resume for 20ms before moving to U0 state.
at least 20ms ;-) Recently, we decided to drive resume for 40ms to
support devices with broken FW.
> Both device and host can initiate resume.
>
> On host initated resume port is set to resume state, sleep 20ms,
sleep 40ms:
include/linux/usb.h:232:#define USB_RESUME_TIMEOUT 40 /* ms */
> and finally set port to U0 state.
>
> On device initated resume a port status interrupt with a port in resume
> state in issued. The interrupt handler tags a resume_done[port]
> timestamp with current time + 20ms, and kick roothub timer.
+ 40ms ?
> Root hub timer requests for port status, finds the port in resume state,
> checks if resume_done[port] timestamp passed, and set port to U0 state.
>
> There are a few issues with this approach,
> 1. A host initated resume will also generate a resume event, the event
> handler will find the port in resume state, believe it's a device
> initated and act accordingly.
>
> 2. A port status request might cut the 20ms resume signalling short if a
> get_port_status request is handled during the 20ms host resume.
> The port will be found in resume state. The timestamp is not set leading
> to time_after_eq(jiffoes, timestamp) returning true, as timestamp = 0.
> get_port_status will proceed with moving the port to U0.
>
> 3. If an error, or anything else happends to the port during device
> initated 20ms resume signalling it will leave all device resume
> parameters hanging uncleared preventing further resume.
>
> Fix this by using the existing resuming_ports bitfield to indicate if
> resume signalling timing is taken care of.
> Also check if the resume_done[port] is set before using it in time
> comparison. Also clear out any resume signalling related variables if port
> is not in U0 or Resume state.
>
> v2. fix parentheses when checking for uncleared resume variables.
> we want: if ((unclear1 OR unclear2 ) AND !in_resume AND !in_U3) { .. }
this 'v2' note doesn't have to go into commit log, IMO.
> Signed-off-by: Mathias Nyman <mathias.ny...@linux.intel.com>
> ---
> drivers/usb/host/xhci-hub.c | 38 ++++++++++++++++++++++++++++++++++++--
> drivers/usb/host/xhci-ring.c | 3 ++-
> 2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 78241b5..a750298 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -616,8 +616,30 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
> if ((raw_port_status & PORT_RESET) ||
> !(raw_port_status & PORT_PE))
> return 0xffffffff;
> - if (time_after_eq(jiffies,
> - bus_state->resume_done[wIndex])) {
> + /* did port event handler already start 20ms resume timing? */
> + if (!bus_state->resume_done[wIndex]) {
> + /* If not, maybe we are in a host initated resume? */
> + if (test_bit(wIndex, &bus_state->resuming_ports)) {
> + /* Host initated resume doesn't time the resume
> + * signalling using resume_done[].
> + * It manually sets RESUME state, sleeps 20ms
> + * and sets U0 state. This should probably be
> + * changed, but not right now, do nothing
> + */
> + } else {
> + /* port resume was discovered now and here,
> + * start resume timing
> + */
> + unsigned long timeout = jiffies +
> + msecs_to_jiffies(USB_RESUME_TIMEOUT);
> +
> + set_bit(wIndex, &bus_state->resuming_ports);
> + bus_state->resume_done[wIndex] = timeout;
> + mod_timer(&hcd->rh_timer, timeout);
> + }
> + /* Has resume been signalled for 20ms? yet? */
How about: "Has resume been signalled for at least 20ms?"
Or even better:
Has resume been signalled for at least USB_RESUME_TIMEOUT ms yet ?
> + } else if (time_after_eq(jiffies,
> + bus_state->resume_done[wIndex])) {
> int time_left;
>
> xhci_dbg(xhci, "Resume USB2 port %d\n",
> @@ -665,6 +687,16 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
> status |= USB_PORT_STAT_SUSPEND;
> }
> }
> + /* Clear stale usb2 resume signalling variables in case port changed
> + * state during 20ms resume signalling. For example on error
> + */
> + if ((bus_state->resume_done[wIndex] ||
> + test_bit(wIndex, &bus_state->resuming_ports)) &&
> + (raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
> + (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME) {
> + bus_state->resume_done[wIndex] = 0;
> + clear_bit(wIndex, &bus_state->resuming_ports);
> + }
> if ((raw_port_status & PORT_PLS_MASK) == XDEV_U0
> && (raw_port_status & PORT_POWER)
> && (bus_state->suspended_ports & (1 << wIndex))) {
> @@ -995,6 +1027,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq,
> u16 wValue,
> if ((temp & PORT_PE) == 0)
> goto error;
>
> + set_bit(wIndex, &bus_state->resuming_ports);
> xhci_set_link_state(xhci, port_array, wIndex,
> XDEV_RESUME);
> spin_unlock_irqrestore(&xhci->lock, flags);
> @@ -1002,6 +1035,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq,
> u16 wValue,
> spin_lock_irqsave(&xhci->lock, flags);
> xhci_set_link_state(xhci, port_array, wIndex,
> XDEV_U0);
> + clear_bit(wIndex, &bus_state->resuming_ports);
> }
> bus_state->port_c_suspend |= 1 << wIndex;
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 97ffe39..3743bb2 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1583,7 +1583,8 @@ static void handle_port_status(struct xhci_hcd *xhci,
> */
> bogus_port_status = true;
> goto cleanup;
> - } else {
> + } else if (!test_bit(faked_port_index,
> + &bus_state->resuming_ports)) {
> xhci_dbg(xhci, "resume HS port %d\n", port_id);
> bus_state->resume_done[faked_port_index] = jiffies +
> msecs_to_jiffies(USB_RESUME_TIMEOUT);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
balbi
signature.asc
Description: PGP signature
