xhci is unusable on some systems after driver switched to BIT() macro. Upper 32bits of 64bit CRCR command register are unintentionally cleared. Seen 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. 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. 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. [Mathias: use BIT_ULL() for ERST_EHB and EP_CTX_CYCLE_MASK as suggested by Michal Pecio, also include raspberry case in commit message]
Unified · Split
| @@ -190,13 +190,13 @@ struct xhci_op_regs { · 189 unmodified lines | ||
| 190 | 190 | |
| 191 | 191 | /* CRCR - Command Ring Control Register - cmd_ring bitmasks */ |
| 192 | 192 | /* bit 0 - Cycle bit indicates the ownership of the command ring */ |
| 193 | −#define CMD_RING_CYCLE BIT(0) | |
| 193 | +#define CMD_RING_CYCLE BIT_ULL(0) | |
| 194 | 194 | /* stop ring operation after completion of the currently executing command */ |
| 195 | −#define CMD_RING_PAUSE BIT(1) | |
| 195 | +#define CMD_RING_PAUSE BIT_ULL(1) | |
| 196 | 196 | /* stop ring immediately - abort the currently executing command */ |
| 197 | −#define CMD_RING_ABORT BIT(2) | |
| 197 | +#define CMD_RING_ABORT BIT_ULL(2) | |
| 198 | 198 | /* true: command ring is running */ |
| 199 | −#define CMD_RING_RUNNING BIT(3) | |
| 199 | +#define CMD_RING_RUNNING BIT_ULL(3) | |
| 200 | 200 | /* bits 63:6 - Command Ring pointer */ |
| 201 | 201 | #define CMD_RING_PTR_MASK GENMASK_ULL(63, 6) |
| 202 | 202 | |
| @@ -271,7 +271,7 @@ struct xhci_intr_reg { · 68 unmodified lines | ||
| 271 | 271 | * bit 3 - Event Handler Busy (EHB), whether the event ring is scheduled to be serviced by |
| 272 | 272 | * a work queue (or delayed service routine)? |
| 273 | 273 | */ |
| 274 | −#define ERST_EHB BIT(3) | |
| 274 | +#define ERST_EHB BIT_ULL(3) | |
| 275 | 275 | /* bits 63:4 - Event Ring Dequeue Pointer */ |
| 276 | 276 | #define ERST_PTR_MASK GENMASK_ULL(63, 4) |
| 277 | 277 | |
| @@ -502,7 +502,7 @@ struct xhci_ep_ctx { · 224 unmodified lines | ||
| 502 | 502 | #define CTX_TO_MAX_ESIT_PAYLOAD(p) (((p) >> 16) & 0xffff) |
| 503 | 503 | |
| 504 | 504 | /* deq bitmasks */ |
| 505 | −#define EP_CTX_CYCLE_MASK BIT(0) | |
| 505 | +#define EP_CTX_CYCLE_MASK BIT_ULL(0) | |
| 506 | 506 | /* bits 63:4 - TR Dequeue Pointer */ |
| 507 | 507 | #define TR_DEQ_PTR_MASK GENMASK_ULL(63, 4) |
| 508 | 508 | |