[PATCH v2] usb: xhci: improve TR Dequeue Pointer mask

[PATCH v2] usb: xhci: improve TR Dequeue Pointer mask

[Niklas Neronin]

Address the naming and usage of the TR Dequeue Pointer mask in the xhci
driver. The Endpoint Context Field at offset 0x08 is defined as follows:
 Bit 0		Dequeue Cycle State (DCS)
 Bits 3:1	RsvdZ (Reserved and Zero)
 Bits 63:4	TR Dequeue Pointer

When extracting the TR Dequeue Pointer for an Endpoint without Streams,
in xhci_handle_cmd_set_deq(), the inverted Dequeue Cycle State mask
(~EP_CTX_CYCLE_MASK) is used, inadvertently including the Reserved bits.
Although bits 3:1 are typically zero, using the incorrect mask could cause
issues.

The existing mask, named "SCTX_DEQ_MASK," is misleading because "SCTX"
implies exclusivity to Stream Contexts, whereas the TR Dequeue Pointer is
applicable to both Stream and non-Stream Contexts.

Rename the mask to "TR_DEQ_PTR_MASK", utilize GENMASK_ULL() macro and use
the mask when handling the TR Dequeue Pointer field.

Function xhci_get_hw_deq() returns the Endpoint Context Field 0x08, either
directly from the Endpoint context or a Stream.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
v2:
* Rebased on top of 6.16.
* Combined all 3 patches from v1 into one patch.

 drivers/usb/host/xhci-ring.c | 12 ++++++------
 drivers/usb/host/xhci.h      |  3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ecd757d482c5..d2ccf19292aa 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -711,7 +711,7 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
 		return -ENODEV;
 	}
 
-	hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id);
+	hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id) & TR_DEQ_PTR_MASK;
 	new_seg = ep_ring->deq_seg;
 	new_deq = ep_ring->dequeue;
 	new_cycle = le32_to_cpu(td->end_trb->generic.field[3]) & TRB_CYCLE;
@@ -723,7 +723,7 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
 	 */
 	do {
 		if (!hw_dequeue_found && xhci_trb_virt_to_dma(new_seg, new_deq)
-		    == (dma_addr_t)(hw_dequeue & ~0xf)) {
+		    == (dma_addr_t)hw_dequeue) {
 			hw_dequeue_found = true;
 			if (td_last_trb_found)
 				break;
@@ -1066,7 +1066,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 		 */
 		hw_deq = xhci_get_hw_deq(xhci, ep->vdev, ep->ep_index,
 					 td->urb->stream_id);
-		hw_deq &= ~0xf;
+		hw_deq &= TR_DEQ_PTR_MASK;
 
 		if (td->cancel_status == TD_HALTED || trb_in_td(td, hw_deq)) {
 			switch (td->cancel_status) {
@@ -1156,7 +1156,7 @@ static struct xhci_td *find_halted_td(struct xhci_virt_ep *ep)
 
 	if (!list_empty(&ep->ring->td_list)) { /* Not streams compatible */
 		hw_deq = xhci_get_hw_deq(ep->xhci, ep->vdev, ep->ep_index, 0);
-		hw_deq &= ~0xf;
+		hw_deq &= TR_DEQ_PTR_MASK;
 		td = list_first_entry(&ep->ring->td_list, struct xhci_td, td_list);
 		if (trb_in_td(td, hw_deq))
 			return td;
@@ -1481,7 +1481,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 		u64 deq;
 		/* 4.6.10 deq ptr is written to the stream ctx for streams */
 		if (ep->ep_state & EP_HAS_STREAMS) {
-			deq = le64_to_cpu(stream_ctx->stream_ring) & SCTX_DEQ_MASK;
+			deq = le64_to_cpu(stream_ctx->stream_ring) & TR_DEQ_PTR_MASK;
 
 			/*
 			 * Cadence xHCI controllers store some endpoint state
@@ -1497,7 +1497,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 				stream_ctx->reserved[1] = 0;
 			}
 		} else {
-			deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
+			deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
 		}
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
 			"Successful Set TR Deq Ptr cmd, deq = @%08llx", deq);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index a20f4e7cd43a..59ff84ba2d4a 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -500,7 +500,8 @@ struct xhci_ep_ctx {
 
 /* deq bitmasks */
 #define EP_CTX_CYCLE_MASK		(1 << 0)
-#define SCTX_DEQ_MASK			(~0xfL)
+/* bits 63:4 - TR Dequeue Pointer */
+#define TR_DEQ_PTR_MASK			GENMASK_ULL(63, 4)
 
 
 /**
-- 
2.50.1

Re: [PATCH v2] usb: xhci: improve TR Dequeue Pointer mask

[Michał Pecio]

On Tue, 26 Aug 2025 15:06:56 +0200, Niklas Neronin wrote:
> Address the naming and usage of the TR Dequeue Pointer mask in the xhci
> driver. The Endpoint Context Field at offset 0x08 is defined as follows:
>  Bit 0		Dequeue Cycle State (DCS)
>  Bits 3:1	RsvdZ (Reserved and Zero)
>  Bits 63:4	TR Dequeue Pointer
> 
> When extracting the TR Dequeue Pointer for an Endpoint without Streams,
> in xhci_handle_cmd_set_deq(), the inverted Dequeue Cycle State mask
> (~EP_CTX_CYCLE_MASK) is used, inadvertently including the Reserved bits.
> Although bits 3:1 are typically zero, using the incorrect mask could cause
> issues.
> 
> The existing mask, named "SCTX_DEQ_MASK," is misleading because "SCTX"
> implies exclusivity to Stream Contexts, whereas the TR Dequeue Pointer is
> applicable to both Stream and non-Stream Contexts.
> 
> Rename the mask to "TR_DEQ_PTR_MASK", utilize GENMASK_ULL() macro and use
> the mask when handling the TR Dequeue Pointer field.
> 
> Function xhci_get_hw_deq() returns the Endpoint Context Field 0x08, either
> directly from the Endpoint context or a Stream.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
> v2:
> * Rebased on top of 6.16.
> * Combined all 3 patches from v1 into one patch.
> 
>  drivers/usb/host/xhci-ring.c | 12 ++++++------
>  drivers/usb/host/xhci.h      |  3 ++-
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index ecd757d482c5..d2ccf19292aa 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -711,7 +711,7 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
>  		return -ENODEV;
>  	}
>  
> -	hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id);
> +	hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id) & TR_DEQ_PTR_MASK;
>  	new_seg = ep_ring->deq_seg;
>  	new_deq = ep_ring->dequeue;
>  	new_cycle = le32_to_cpu(td->end_trb->generic.field[3]) & TRB_CYCLE;
> @@ -723,7 +723,7 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
>  	 */
>  	do {
>  		if (!hw_dequeue_found && xhci_trb_virt_to_dma(new_seg, new_deq)
> -		    == (dma_addr_t)(hw_dequeue & ~0xf)) {
> +		    == (dma_addr_t)hw_dequeue) {
>  			hw_dequeue_found = true;
>  			if (td_last_trb_found)
>  				break;
> @@ -1066,7 +1066,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
>  		 */
>  		hw_deq = xhci_get_hw_deq(xhci, ep->vdev, ep->ep_index,
>  					 td->urb->stream_id);
> -		hw_deq &= ~0xf;
> +		hw_deq &= TR_DEQ_PTR_MASK;
>  
>  		if (td->cancel_status == TD_HALTED || trb_in_td(td, hw_deq)) {
>  			switch (td->cancel_status) {
> @@ -1156,7 +1156,7 @@ static struct xhci_td *find_halted_td(struct xhci_virt_ep *ep)
>  
>  	if (!list_empty(&ep->ring->td_list)) { /* Not streams compatible */
>  		hw_deq = xhci_get_hw_deq(ep->xhci, ep->vdev, ep->ep_index, 0);
> -		hw_deq &= ~0xf;
> +		hw_deq &= TR_DEQ_PTR_MASK;
>  		td = list_first_entry(&ep->ring->td_list, struct xhci_td, td_list);
>  		if (trb_in_td(td, hw_deq))
>  			return td;
> @@ -1481,7 +1481,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
>  		u64 deq;
>  		/* 4.6.10 deq ptr is written to the stream ctx for streams */
>  		if (ep->ep_state & EP_HAS_STREAMS) {
> -			deq = le64_to_cpu(stream_ctx->stream_ring) & SCTX_DEQ_MASK;
> +			deq = le64_to_cpu(stream_ctx->stream_ring) & TR_DEQ_PTR_MASK;
>  
>  			/*
>  			 * Cadence xHCI controllers store some endpoint state
> @@ -1497,7 +1497,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
>  				stream_ctx->reserved[1] = 0;
>  			}
>  		} else {
> -			deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
> +			deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
>  		}
>  		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
>  			"Successful Set TR Deq Ptr cmd, deq = @%08llx", deq);

Looks good and I see no more 0xf contstants remaining in this file.

> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index a20f4e7cd43a..59ff84ba2d4a 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -500,7 +500,8 @@ struct xhci_ep_ctx {
>  
>  /* deq bitmasks */
>  #define EP_CTX_CYCLE_MASK		(1 << 0)
> -#define SCTX_DEQ_MASK			(~0xfL)
> +/* bits 63:4 - TR Dequeue Pointer */
> +#define TR_DEQ_PTR_MASK			GENMASK_ULL(63, 4)

I don't care much about this rename, but I can't help but notice that
naming is not consistent with the related EP_CTX_CYCLE_MASK above and
that it too applies to both types of contexts.

If I wanted to fix this, I would just drop the 'EP_' and 'S' prefixes.

Re: [PATCH v2] usb: xhci: improve TR Dequeue Pointer mask

[Neronin, Niklas]

On 28/08/2025 11.12, Michał Pecio wrote:
> On Tue, 26 Aug 2025 15:06:56 +0200, Niklas Neronin wrote:
>> Address the naming and usage of the TR Dequeue Pointer mask in the xhci
>> driver. The Endpoint Context Field at offset 0x08 is defined as follows:
>>  Bit 0		Dequeue Cycle State (DCS)
>>  Bits 3:1	RsvdZ (Reserved and Zero)
>>  Bits 63:4	TR Dequeue Pointer
>>
>> When extracting the TR Dequeue Pointer for an Endpoint without Streams,
>> in xhci_handle_cmd_set_deq(), the inverted Dequeue Cycle State mask
>> (~EP_CTX_CYCLE_MASK) is used, inadvertently including the Reserved bits.
>> Although bits 3:1 are typically zero, using the incorrect mask could cause
>> issues.
>>
>> The existing mask, named "SCTX_DEQ_MASK," is misleading because "SCTX"
>> implies exclusivity to Stream Contexts, whereas the TR Dequeue Pointer is
>> applicable to both Stream and non-Stream Contexts.
>>
>> Rename the mask to "TR_DEQ_PTR_MASK", utilize GENMASK_ULL() macro and use
>> the mask when handling the TR Dequeue Pointer field.
>>
>> Function xhci_get_hw_deq() returns the Endpoint Context Field 0x08, either
>> directly from the Endpoint context or a Stream.
>>
>> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
>> ---
...
> 
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index a20f4e7cd43a..59ff84ba2d4a 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -500,7 +500,8 @@ struct xhci_ep_ctx {
>>  
>>  /* deq bitmasks */
>>  #define EP_CTX_CYCLE_MASK		(1 << 0)
>> -#define SCTX_DEQ_MASK			(~0xfL)
>> +/* bits 63:4 - TR Dequeue Pointer */
>> +#define TR_DEQ_PTR_MASK			GENMASK_ULL(63, 4)
> 
> I don't care much about this rename, but I can't help but notice that
> naming is not consistent with the related EP_CTX_CYCLE_MASK above and
> that it too applies to both types of contexts.
> 
> If I wanted to fix this, I would just drop the 'EP_' and 'S' prefixes.

I have an Endpoint Context patch series in progress that will address all
Endpoint Context macro naming, comments, and related issues.

Because, this patch set is quite old, v1 sent in February this year, all
my current patches are built and tested on top of it, so this is why I
am sending it first.

Best Regards,
Niklas