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
