Re: [PATCH 8/9] usb: xhci: improve debug messages during suspend

[Michal Pecio <michal.pecio@gmail.com>]

On Fri, 27 Mar 2026 13:34:39 +0100, Niklas Neronin wrote:
> Improve debug output for suspend failures, particularly when the
> controller handshake does not complete. This will become important as
> upcoming patches significantly rework the resume path, making more
> detailed suspend-side messages valuable for debugging.

As an aside, I think that if this series will cause any problems,
it will be some stale data structures surviving after resume, not
anything going wrong here.

> Add an explicit check of the Save/Restore Error (SRE) flag after a
> successful Save State (CSS) operation. The xHCI specification
> (note in section 4.23.2) states:
> 
>  "After a Save or Restore State operation completes, the
>   Save/Restore Error (SRE) flag in USBSTS should be checked to
>   ensure the operation completed successfully."
> 
> Currently, the SRE error is only observed and warning is printed.
> This patch does not introduce deeper error handling, as the correct
> response is unclear and changes to suspend behavior may risk
> regressions once the resume path is updated.

I think patch 10/9 should add setting xhci->broken_suspend if this is
detected. It's ridiculous to try State Restore after State Save error.
At best, it should fail. At worst, it might not fail...

> 
> Additionally, simplify and clean up the suspend USBSTS CSS/SSS
> handling code, improving readability and quirk handling for AMD
> SNPS xHC controllers that occasionally do not clear the SSS bit.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---

>  drivers/usb/host/xhci.c | 65 +++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 658419eb6827..232e6143ac4b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -957,11 +957,11 @@ static bool xhci_pending_portevent(struct xhci_hcd *xhci)
>   */
>  int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  {
> -	int			rc = 0;
> +	int			err;
>  	unsigned int		delay = XHCI_MAX_HALT_USEC * 2;
>  	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
>  	u32			command;
> -	u32			res;
> +	u32			usbsts;
>  
>  	if (!hcd->state)
>  		return 0;
> @@ -1007,11 +1007,10 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  	/* Some chips from Fresco Logic need an extraordinary delay */
>  	delay *= (xhci->quirks & XHCI_SLOW_SUSPEND) ? 10 : 1;
>  
> -	if (xhci_handshake(&xhci->op_regs->status,
> -		      STS_HALT, STS_HALT, delay)) {
> -		xhci_warn(xhci, "WARN: xHC CMD_RUN timeout\n");
> -		spin_unlock_irq(&xhci->lock);
> -		return -ETIMEDOUT;
> +	err = xhci_handshake(&xhci->op_regs->status, STS_HALT, STS_HALT, delay);
> +	if (err) {
> +		xhci_warn(xhci, "Clearing Run/Stop bit failed %d\n", err);
> +		goto handshake_error;
>  	}
>  	xhci_clear_command_ring(xhci);
>  
> @@ -1022,28 +1021,34 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  	command = readl(&xhci->op_regs->command);
>  	command |= CMD_CSS;
>  	writel(command, &xhci->op_regs->command);
> +
> +	err = xhci_handshake(&xhci->op_regs->status, STS_SAVE, 0, 20 * USEC_PER_MSEC);
> +	usbsts = readl(&xhci->op_regs->status);
>  	xhci->broken_suspend = 0;
> -	if (xhci_handshake(&xhci->op_regs->status,
> -				STS_SAVE, 0, 20 * 1000)) {
> -	/*
> -	 * AMD SNPS xHC 3.0 occasionally does not clear the
> -	 * SSS bit of USBSTS and when driver tries to poll
> -	 * to see if the xHC clears BIT(8) which never happens
> -	 * and driver assumes that controller is not responding
> -	 * and times out. To workaround this, its good to check
> -	 * if SRE and HCE bits are not set (as per xhci
> -	 * Section 5.4.2) and bypass the timeout.
> -	 */
> -		res = readl(&xhci->op_regs->status);
> -		if ((xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) &&
> -		    (((res & STS_SRE) == 0) &&
> -				((res & STS_HCE) == 0))) {
> -			xhci->broken_suspend = 1;
> -		} else {
> -			xhci_warn(xhci, "WARN: xHC save state timeout\n");
> -			spin_unlock_irq(&xhci->lock);
> -			return -ETIMEDOUT;
> +	if (err) {
> +		/*
> +		 * AMD SNPS xHC 3.0 occasionally does not clear the
> +		 * SSS bit of USBSTS and when driver tries to poll
> +		 * to see if the xHC clears BIT(8) which never happens
> +		 * and driver assumes that controller is not responding
> +		 * and times out. To workaround this, its good to check
> +		 * if SRE and HCE bits are not set (as per xhci
> +		 * Section 5.4.2) and bypass the timeout.
> +		 */
> +		if (!(xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND)) {
> +			xhci_warn(xhci, "Controller Save State failed %d\n", err);
> +			goto handshake_error;
>  		}
> +
> +		if (usbsts & (STS_SRE | STS_HCE)) {
> +			xhci_warn(xhci, "Controller Save State failed, USBSTS 0x%08x\n", usbsts);
> +			goto handshake_error;
> +		}
> +
> +		xhci_dbg(xhci, "SNPS broken suspend, save state unreliable\n");
> +		xhci->broken_suspend = 1;
> +	} else if (usbsts & STS_SRE) {
> +		xhci_warn(xhci, "Suspend Save Error (SRE), USBSTS 0x%08x\n", usbsts);
>  	}

This is a bit complicated and those warns are almost identical.
Would it be a problem to simply:

if (SRE | HCE)
    xhci_warn("Save State error USBSTS %x")
    // patch 10/9: set broken_suspend here
    // patch 11/9: add HSE to this list

if (err)
    if (quirk)
        xhci->broken_suspend = 1;
    else
        xhci_warn("Save State timeout")
        goto handshake_error

This may sometimes print both warnings, but not a big deal.

>  	spin_unlock_irq(&xhci->lock);
>  
> @@ -1059,7 +1064,11 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  				__func__);
>  	}
>  
> -	return rc;
> +	return 0;
> +
> +handshake_error:
> +	spin_unlock_irq(&xhci->lock);
> +	return -ETIMEDOUT;
>  }
>  EXPORT_SYMBOL_GPL(xhci_suspend);
>  
> -- 
> 2.50.1
>