Re: [TESTPATCH v2] xhci: fix usb2 resume timing and races.

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 6403 bytes --]


Hi,

Mathias Nyman <mathias.nyman@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.nyman@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 majordomo@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]