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

["Michał Pecio" <michal.pecio@gmail.com>]

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.