Re: [RFC PATCH 03/12] usb: xhci: simplify USBSTS register reset

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

On Thu,  5 Mar 2026 15:48:15 +0100, Niklas Neronin wrote:
> The current code clears the USB Status Register (USBSTS) and then sets
> the Enable Interrupt (EINT) bit to '1'. A mask (~0x1fff) is used to
> avoid modifying reserved bits 31:13, but it also inadvertently
> excludes reserved bits 7:5, 1. These fields are defined as RsvdZ, so
> writing '0' to them has no effect.

This paragraph is misleading because it suggests that treatment of bits
31:13 was correct and treatment of bits 7:5 was harmlessly wrong.

In reality, bits 7:5 were done correctly, while preserving bits 31:13
was a bug. They should be written as zero just like 7:5.

Table 5-3 is completely clear:
Software shall use ‘0’ for writes to these bits.

> According to the xHCI specification, section 5.1.1:
> "System software shall mask all reserved fields (Rsvd, RsvdP or RsvdZ) to
>  '0' before evaluating a register or data structure value. This will enable
>  current system software to run with future xHCI implementations that
>  define the reserved fields."

This quote seems irrelevant. We aren't evaluating anything here, just
changing one bit.

It obviously doesn't apply to the case of read-modify-write, because if
we mask RsvdP bits to zero and then write back, that's a bug too.

> 
> Given this, it is safe to write zero to all bits of USBSTS except EINT.
> This allows the mask and the register read to be removed, simplifying the
> code, avoiding potential future issues and removing unexplained hex.
> 
> USB Status Register; xHCI Specification, version 1.2 section 5.4.2
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a288f59c604c..ad095768ed48 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -705,7 +705,6 @@ EXPORT_SYMBOL_GPL(xhci_run);
>   */
>  void xhci_stop(struct usb_hcd *hcd)
>  {
> -	u32 temp;
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>  	struct xhci_interrupter *ir = xhci->interrupters[0];
>  
> @@ -740,8 +739,7 @@ void xhci_stop(struct usb_hcd *hcd)
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>  			"// Disabling event ring interrupts");
> -	temp = readl(&xhci->op_regs->status);
> -	writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);

Makes sense,

> +	writel(STS_EINT, &xhci->op_regs->status);

but what is actually the point of clearing this bit here at all?

This is USBSTS.EINT, not USBCMD.INTE. Per 5.4.2 (including notes),
this bit has no effect on anything, it's only for SW's convenience.
And we are shutting down the driver right now.

>  	xhci_disable_interrupter(xhci, ir);
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "cleaning up memory");
> @@ -1174,8 +1172,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  			return retval;
>  
>  		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
> -		temp = readl(&xhci->op_regs->status);
> -		writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
> +		writel(STS_EINT, &xhci->op_regs->status);
>  		xhci_disable_interrupter(xhci, xhci->interrupters[0]);

This runs right after xhci_reset(), so I suspect this whole block is
pointless. Apparently copy-pasted with other stuff from xhci_stop().

>  
>  		xhci_dbg(xhci, "cleaning up memory\n");
> -- 
> 2.50.1
>