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

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

On 19/02/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.

Thanks, you are correct. I'll change this to in_range64() and the bellow to !in_range64().

> 
>> +	/* Loop through segment which don't contain the DMA address. */
>> +	while (dma < seg->dma || (seg->dma + TRB_SEGMENT_SIZE) <= dma) {
> 
> 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

Sure, I'll add some comments.

> 
>> +	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.

I'll try this, the only issue is that the new API defines the range from;
 'start_trb' in 'start_seg' to 'end_trb' in 'end_seg'.
But the current version does not use 'end_seg'. So, if check checkpatch.pl
does not complain about unused 'end_seg' variable I will swap patch 4 and 5 places.

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

I tried introducing xhci_dma_to_trb_virt() in this patch set, but decided against it.
In my opinion it is very minor optimization and I would rather introduce this function
in a separate patch set. I reworked trb_in_td() with this change in mind.

> Sometimes also sizeof(*) < sizeof(dma_addr_t).

This is an interesting idea. I just created a reverse xhci_virt_trb_to_dma(), i.e. xhci_dma_to_virt_trb().
I'll explore this idea. Thanks.

> 
> Regards,
> Michal