Re: [RFC PATCH] usb: xhci: use BIT_ULL for CRCR bits

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

On Thu, 30 Jul 2026 15:45:21 +1000, Lachlan Hodges wrote:
> The CRCR register is 64 bits wide - commit abe93f27cdd7
> ("xhci: use BIT macro") changed the flag definitions from (1 << n),
> a signed int, to BIT(n), an unsigned long. Within
> xhci_set_cmd_ring_deq(), the following operation is performed on the
> CRCR register:
> 
> ...
> 	crcr &= ~CMD_RING_PTR_MASK;
> 	crcr |= deq_dma;
> 	crcr &= ~CMD_RING_CYCLE;
> 	crcr |= xhci->cmd_ring->cycle_state;
> ...
> 
> Previously, ~CMD_RING_CYCLE was ~(int)1, a negative signed value
> (0xFFFFFFFE with the sign bit set). Widening a negative signed int to
> u64 sign-extends it to 0xFFFFFFFFFFFFFFFE, correctly clearing only bit
> 0 and preserving the 64-bit pointer written two lines above.
> 
> After the change when running on 32 bit kernels, ~CMD_RING_CYCLE is
> ~(unsigned long)1UL. On a 32-bit host this is an unsigned 32-bit
> value (0xFFFFFFFE, no sign bit). Widening an unsigned value to u64
> zero-extends it instead (0x00000000FFFFFFFE), so the subsequent AND
> silently clears bits 63:32 of crcr, truncating the command ring
> pointer that was just written before the value reaches hardware.
> 
> To fix, similar to how CMD_RING_PTR_MASK is defined, make sure we
> use the BIT_ULL variant when defining the CRCR bits.
> 
> Fixes: abe93f27cdd7 ("xhci: use BIT macro")
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Lachlan Hodges <lachlan.hodges@morsemicro.com>
> ---
> 
> Hi, I experienced this when running the tip of wireless-next on
> a raspberry pi 4B compiled for arm32. The main symptoms were the
> following log message:
> 
> [    0.549897] raspberrypi-firmware soc:firmware: Attached to firmware from 2021-02-25T12:11:39
> [    0.626859] xhci_hcd 0000:01:00.0: xHCI Host Controller
> [    0.626889] xhci_hcd 0000:01:00.0: new USB bus registered, assigned bus number 1
> [    0.812619] xhci_hcd 0000:01:00.0: hcc params 0x002841eb hci version 0x100 quirks 0x0000200000000890
> [    0.813188] xhci_hcd 0000:01:00.0: xHCI Host Controller
> [    0.813203] xhci_hcd 0000:01:00.0: new USB bus registered, assigned bus number 2
> [    0.813219] xhci_hcd 0000:01:00.0: Host supports USB 3.0 SuperSpeed
> [    0.813602] hub 1-0:1.0: USB hub found
> [    0.814052] hub 2-0:1.0: USB hub found
> [    0.952714] xhci_hcd 0000:01:00.0: ERROR mismatched command completion event
> 
> Additionally running lsusb just hangs. Running the same kernel compiled
> for aarch64 worked fine. Bisected to the commit in the Fixes line
> above. Additionally a USB device plugged in to the USB3.0 (or 2.0)
> did not enumerate. Once this patch is applied the USB device
> enumerates properly on the tip of wireless-next 4a0bd262df75 ("wifi:
> mac80211: fix per-STA profile length in cross-link CSA parsing").

Makes sense, including observed symptoms.

A system with IOMMU would fault on top of that, but the accesses
are reads, so hopefully no corruption in absence of IOMMU.

> Happy to test alternate fix proposals! Thanks.

I think the sort of code you quoted should work without surprises,
so widening these macros to ULL is right.

And there is a few more. They currently don't break anything, but
neither does CMD_RING_ABORT or CMD_RING_RUNNING for that matter.

ERST_EHB
EP_CTX_CYCLE_MASK

Let's see what Mathias thinks about it.

> 
> lachlan
> ---
>  drivers/usb/host/xhci.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index d02046a573e4..df014855c659 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -190,13 +190,13 @@ struct xhci_op_regs {
>  
>  /* CRCR - Command Ring Control Register - cmd_ring bitmasks */
>  /* bit 0 - Cycle bit indicates the ownership of the command ring */
> -#define CMD_RING_CYCLE		BIT(0)
> +#define CMD_RING_CYCLE		BIT_ULL(0)
>  /* stop ring operation after completion of the currently executing command */
> -#define CMD_RING_PAUSE		BIT(1)
> +#define CMD_RING_PAUSE		BIT_ULL(1)
>  /* stop ring immediately - abort the currently executing command */
> -#define CMD_RING_ABORT		BIT(2)
> +#define CMD_RING_ABORT		BIT_ULL(2)
>  /* true: command ring is running */
> -#define CMD_RING_RUNNING	BIT(3)
> +#define CMD_RING_RUNNING	BIT_ULL(3)
>  /* bits 63:6 - Command Ring pointer */
>  #define CMD_RING_PTR_MASK	GENMASK_ULL(63, 6)
>  
> -- 
> 2.43.0
>