Re: [PATCH 3/4] usb: xhci: rework and simplify trb_in_td()

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 19.2.2025 10.56, Michał Pecio wrote:
> Hi,
> 
>> +		/* Edge case, the TD wrapped around to the start segment. */
>> +		if (xhci_trb_virt_to_dma(td->end_seg, td->end_trb) < dma &&
>> +		    dma < xhci_trb_virt_to_dma(td->start_seg, td->start_trb))
>> +			return NULL;
>> +		if (seg->dma <= dma && dma <= (seg->dma + TRB_SEGMENT_SIZE))
> 
> It should be strict inequality for the upper bound here.
> 
> Note that this wraparound case souldn't be happening (the driver avoids
> moving enqueue into deq_seg to simplify ring expansion) so no amount of
> testing will catch problems here, until maybe something changes one day.
> 
>> +			return seg;
>> +		seg = seg->next;
>> +	}
> 
> The situation is tricky now, because we are either in start_seg and
> end_seg is elsewhere or in start_seg->next and wraparound. But it looks
> like the loop below will work OK for either case.
> 
>> +	/* Loop through segment which don't contain the DMA address. */
>> +	while (dma < seg->dma || (seg->dma + TRB_SEGMENT_SIZE) <= dma) {
> 
> This condition looks like it could use the in_range() macro.
> 
>> +		if (seg == td->end_seg)
>> +			return NULL;
>> +
>> +		seg = seg->next;
>> +		if (seg == td->start_seg)
>> +			return NULL;
> 
> I suppose this only happens if end_seg is not on the ring, fair enough.
> 
>> +	}
> 
> Maybe a comment here? Something like:

> 
> * At this point seg contains the dma and either:
> * a. start_seg != end_seg and seg can be anywhere
> * b. start_seg == end_seg in wraparound case and seg != start_seg

Agreed, a comment here would help.

> 
>> +	if (seg == td->start_seg) {
>> +		if (dma < xhci_trb_virt_to_dma(td->start_seg, td->start_trb))
>> +			return NULL;
>> +	} else if (seg == td->end_seg) {
>> +		if (xhci_trb_virt_to_dma(td->end_seg, td->end_trb) < dma)
>> +			return NULL;
>> +	}
>> +	return seg;
> 
> This should be corrent, but it's not something immediately obvious.
> 
> Not sure if this new implementation is really simpler than the old one.
> I wonder if it wouldn't make sense to reorder this after the API change
> (patch 4/4) to allow emergency revert if something unexpected shows up.

Had to draw several cases on paper to go through this new version.
But I might just be used to the old one

> 
> As for efficiency, those virt_to_dma translations aren't exactly free
> and there are two. Maybe it could be faster to translate dma to virt
> once and then compare. Sometimes also sizeof(*) < sizeof(dma_addr_t).

Agreed

dma_addr_t start_dma = xhci_trb_virt_to_dma(td->start_seg, td->start_trb);
dma_addr_t end_dma = xhci_trb_virt_to_dma(td->end_seg, td->end_trb);

comparisons will then be a lot easier to read with start_dma and end_dma

-Mathias