Re: [PATCH 1/7] usb: xhci-dbgcap: correct DMA address handling

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

On Wed,  3 Sep 2025 19:01:21 +0200, Niklas Neronin wrote:
> Address the handling of addresses within the dbc_handle_xfer_event(),
> ensuring accurate extraction and comparison.
> 
> Variable details:
> 'r->trb_dma'		    : A DMA address.
> 'ep_ctx->deq'		    : A le64, bits 63:4 contain the DMA address.
> 'event->trans_event.buffer' : A le64, bits 63:0 contain the DMA address.
> 
> 1. Convert 'ep_ctx->deq' and 'event->trans_event.buffer' addresses to
>    the CPU's native byte order.
> 
> 2. Use mask; TR_DEQ_PTR_MASK (bits 63:4) to extract the address from
>    'ep_ctx->deq', replacing the previous use of ~TRB_CYCLE (bits 63:1).
> 
> Why not use 'dma_addr_t' for the address?
> The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
> system architecture or xHCI driver flags, whereas the 64-bit address field
> size remains constant. Since hardware cannot be fully trusted, it's better
> to print the entire 64-bit address to detect any non-zero values in the
> upper 32 bits. This approach ensures that potential issues are easily
> detected.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-dbgcap.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> index 06a2edb9e86e..2f24a3a54439 100644
> --- a/drivers/usb/host/xhci-dbgcap.c
> +++ b/drivers/usb/host/xhci-dbgcap.c
> @@ -724,6 +724,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>  	u32			comp_code;
>  	size_t			remain_length;
>  	struct dbc_request	*req = NULL, *r;
> +	u64			ep_trb, deq;
>  
>  	comp_code	= GET_COMP_CODE(le32_to_cpu(event->generic.field[2]));
>  	remain_length	= EVENT_TRB_LEN(le32_to_cpu(event->generic.field[2]));
> @@ -733,10 +734,11 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>  	ep_ctx		= (ep_id == EPID_OUT) ?
>  				dbc_bulkout_ctx(dbc) : dbc_bulkin_ctx(dbc);
>  	ring		= dep->ring;
> +	ep_trb		= le64_to_cpu(event->trans_event.buffer);

In other places this variable would be called ep_trb_dma
and ep_trb would be xhci_trb*.

>  
>  	/* Match the pending request: */
>  	list_for_each_entry(r, &dep->list_pending, list_pending) {
> -		if (r->trb_dma == event->trans_event.buffer) {
> +		if (r->trb_dma == ep_trb) {

And here it is being compared with a something_dma variable.

>  			req = r;
>  			break;
>  		}
> @@ -768,8 +770,9 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>  		status = -comp_code;
>  		break;
>  	case COMP_STALL_ERROR:
> +		deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
>  		dev_warn(dbc->dev, "Stall error at bulk TRB %llx, remaining %zu, ep deq %llx\n",
> -			 event->trans_event.buffer, remain_length, ep_ctx->deq);
> +			 ep_trb, remain_length, deq);
>  		status = 0;
>  		dep->halted = 1;
>  
> @@ -788,7 +791,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>  		 * TRB again.
>  		 */
>  
> -		if ((ep_ctx->deq & ~TRB_CYCLE) == event->trans_event.buffer) {
> +		if (deq == ep_trb) {
>  			dev_dbg(dbc->dev, "Ep stopped on Stalled TRB\n");
>  			if (remain_length == req->length) {
>  				dev_dbg(dbc->dev, "Spurious stall event, keep req\n");
> -- 
> 2.50.1
>