Re: [PATCH 2/4] xhci: Improve detection of device initiated wake signal.

[Jack Pham <jackp@codeaurora.org>]

Hi Mathias,

On Thu, Mar 11, 2021 at 01:53:51PM +0200, Mathias Nyman wrote:
> A xHC USB 3 port might miss the first wake signal from a USB 3 device
> if the port LFPS reveiver isn't enabled fast enough after xHC resume.
> 
> xHC host will anyway be resumed by a PME# signal, but will go back to
> suspend if no port activity is seen.
> The device resends the U3 LFPS wake signal after a 100ms delay, but
> by then host is already suspended, starting all over from the
> beginning of this issue.
> 
> USB 3 specs say U3 wake LFPS signal is sent for max 10ms, then device
> needs to delay 100ms before resending the wake.
> 
> Don't suspend immediately if port activity isn't detected in resume.
> Instead add a retry. If there is no port activity then delay for 120ms,
> and re-check for port activity.

We have a use case with which this change is causing unnecessary delay.
Consider a USB2* device is attached and host is initiating the resume.
Since this is not a device initiated wakeup there wouldn't be any
pending event seen on the PORTSC registers, yet this adds an additional
120ms delay to re-check the PORTSC before returning and allowing the USB
core to perform resume signaling.

Is there a way to avoid this delay in that case?  Perhaps could we
distinguish whether we arrive here at xhci_resume() due to a
host-initiated resume vs. a device remote wakeup?

* I think it should be similar for attached USB3 devices as well, since
the host-initiated exit from U3 wouldn't happen until usb_port_resume().

Thanks,
Jack

> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index bd27bd670104..48a68fcf2b36 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1088,6 +1088,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
>  	struct usb_hcd		*secondary_hcd;
>  	int			retval = 0;
>  	bool			comp_timer_running = false;
> +	bool			pending_portevent = false;
>  
>  	if (!hcd->state)
>  		return 0;
> @@ -1226,13 +1227,22 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
>  
>   done:
>  	if (retval == 0) {
> -		/* Resume root hubs only when have pending events. */
> -		if (xhci_pending_portevent(xhci)) {
> +		/*
> +		 * Resume roothubs only if there are pending events.
> +		 * USB 3 devices resend U3 LFPS wake after a 100ms delay if
> +		 * the first wake signalling failed, give it that chance.
> +		 */
> +		pending_portevent = xhci_pending_portevent(xhci);
> +		if (!pending_portevent) {
> +			msleep(120);
> +			pending_portevent = xhci_pending_portevent(xhci);
> +		}
> +
> +		if (pending_portevent) {
>  			usb_hcd_resume_root_hub(xhci->shared_hcd);
>  			usb_hcd_resume_root_hub(hcd);
>  		}
>  	}

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project