| Message ID | 20250206103428.1034784-4-niklas.neronin@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: xhci: improve trb_in_td() | expand |
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 > + 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. 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). Regards, Michal
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
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index a69972cc400c..23337c9d34c1 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -281,51 +281,45 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, * If the suspect DMA address is a TRB in this TD, this function returns that * TRB's segment. Otherwise it returns 0. */ -static struct xhci_segment *trb_in_td(struct xhci_td *td, dma_addr_t suspect_dma) +static struct xhci_segment *trb_in_td(struct xhci_td *td, dma_addr_t dma) { - dma_addr_t start_dma; - dma_addr_t end_seg_dma; - dma_addr_t end_trb_dma; - struct xhci_segment *cur_seg; + struct xhci_segment *seg = td->start_seg; - start_dma = xhci_trb_virt_to_dma(td->start_seg, td->start_trb); - cur_seg = td->start_seg; - - do { - if (start_dma == 0) - return NULL; - /* We may get an event for a Link TRB in the middle of a TD */ - end_seg_dma = xhci_trb_virt_to_dma(cur_seg, - &cur_seg->trbs[TRBS_PER_SEGMENT - 1]); - /* If the end TRB isn't in this segment, this is set to 0 */ - end_trb_dma = xhci_trb_virt_to_dma(cur_seg, td->end_trb); - - if (end_trb_dma > 0) { - /* The end TRB is in this segment, so suspect should be here */ - if (start_dma <= end_trb_dma) { - if (suspect_dma >= start_dma && suspect_dma <= end_trb_dma) - return cur_seg; - } else { - /* Case for one segment with - * a TD wrapped around to the top - */ - if ((suspect_dma >= start_dma && - suspect_dma <= end_seg_dma) || - (suspect_dma >= cur_seg->dma && - suspect_dma <= end_trb_dma)) - return cur_seg; - } + if (td->start_seg == td->end_seg) { + if (td->start_trb <= td->end_trb) { + if (xhci_trb_virt_to_dma(td->start_seg, td->start_trb) <= dma && + dma <= xhci_trb_virt_to_dma(td->end_seg, td->end_trb)) + return seg; return NULL; } - /* Might still be somewhere in this segment */ - if (suspect_dma >= start_dma && suspect_dma <= end_seg_dma) - return cur_seg; - cur_seg = cur_seg->next; - start_dma = xhci_trb_virt_to_dma(cur_seg, &cur_seg->trbs[0]); - } while (cur_seg != td->start_seg); + /* 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)) + return seg; + seg = seg->next; + } - return NULL; + /* Loop through segment which don't contain the DMA address. */ + while (dma < seg->dma || (seg->dma + TRB_SEGMENT_SIZE) <= dma) { + if (seg == td->end_seg) + return NULL; + + seg = seg->next; + if (seg == td->start_seg) + return NULL; + } + + 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; } /*
Function trb_in_td() searches for a DMA address within a segment ring, starting from 'start_seg'. If the DMA address is found within a segment and is positioned between 'start_trb' and 'end_trb', the function returns the segment. If not, it returns 'NULL'. See ring example at the end. The original implementation is overly complex and suboptimal. Key enhancements include: - Utilize 'end_seg' pointer as counterpart to 'start_seg', narrowing the search scope from start to end segment, improving efficiency. - Prioritizing the most frequent scenario where the start and end segments are identical, and 'start_trb' precedes end_trb', by checking this case first for quicker resolution. - Clarifying the handling of TD wrap-around cases, where a TD spans back to the start segment, making start and end segments equal, but with 'end_trb' preceding 'start_trb', for better readability and maintainability. ============================= Example ==================================== Segment ring, consisting of 3 segments (A,B,C) each containing 3 TRBs (1,2,3). Any segment can be start/end seg, and any TRB inside start seg can be start TRB, vise versa. +---+ +---+ +---+ C --> | A |-->| B |-->| C |--> A +---+ +---+ +---+ | | | +---+ +---+ +---+ | 1 | | 1 | | 1 | | 2 | | 2 | | 2 | | 3 | | 3 | | 3 | +---+ +---+ +---+ Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com> --- drivers/usb/host/xhci-ring.c | 72 +++++++++++++++++------------------- 1 file changed, 33 insertions(+), 39 deletions(-)