On Wed, 26 Feb 2025 14:41:44 +0200, Mathias Nyman wrote:
> Updated my for-usb-next branch to this v3 version
A few remarks regarding "Add helper to find trb from its dma address":
xhci_dma_to_trb():
This function could use xhci_for_each_ring_seg.
The use of in_range() perhaps deserves a comment, because its
correctness is not as obvious as it might seem.
Long story short, my own version:
/*
* Look up a TRB on the ring by its DMA address, return NULL if not found.
* Start from deq_seg to optimize for event handling use.
*
* Note: false positive is possible if dma < TRB_SEGMENT_SIZE *and*
* seg->dma > (dma_addr_t) 0 - TRB_SEGMENT_SIZE, but that shouldn't
* happen if seg->dma is an allocation of size TRB_SEGMENT_SIZE.
*/
static union xhci_trb *xhci_dma_to_trb(struct xhci_ring *ring, dma_addr_t dma)
{
struct xhci_segment *seg;
xhci_for_each_ring_seg(ring->deq_seg, seg)
if (in_range(dma, seg->dma, TRB_SEGMENT_SIZE))
return seg->trbs + (dma - seg->dma) / sizeof(seg->trbs[0]);
return NULL;
}
>+ struct xhci_td *matched_td;
This variable is only used as bool so it could be declared as such.
Other places still use 'td' and assume that it equals 'matched_td'.
And that's OK because there is no need for iterating further after
the matching TD is found.
>+ /* find the transfer trb this events points to */
>+ if (ep_trb_dma)
>+ ep_trb = xhci_dma_to_trb(ep_ring, ep_trb_dma);
This may deserve a dedicated warning. It's a pathology. Either the
event is bogus due to internal corruption in the HC, or it's executing
TRBs from a wrong ring due to damaged/ignored Link TRB or bad Set Deq.
Or we completely screwed up and are looking at a wrong ep_ring here.
>- if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !ep_trb_dma)
>+ if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !ep_trb)
> return 0;
Good idea. I think I would go further and refuse to handle anything
when (ep_trb_dma && !ep_trb). Nothing is going to match, nothing good
will come from trying as far as I see.
But that's a behavior change, so maybe material for a separate patch.
>+ matched_td = NULL;
>
> /* Is this a TRB in the currently executing TD? */
>- ep_seg = trb_in_td(xhci, td, ep_trb_dma, false);
>+ if (trb_in_td(xhci, td, ep_trb_dma, false))
>+ matched_td = td;
If 'matched_td' is declared as bool, this will become simply:
matched_td = trb_in_td(xhci, td, ep_trb_dma, false);
Regards,
Michal
