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

["Neronin, Niklas" <niklas.neronin@linux.intel.com>]

On 09/09/2025 13.13, Michal Pecio wrote:
> 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*.

The variable is named 'ep_trb' because it represents a u64 value, not a DMA address.

The choice to avoid using a 'dma_addr_t' variable for the 64-bit value is intentional,
as it can prevent the loss of the upper 32 bits.

> 
>>  
>>  	/* 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.

dma_addr_t trb_dma	- is a DMA address
u64        ep_trb	- is a 64-bit value, which, if correct, represents a DMA address.

Best Regards,
Niklas