| Message ID | 20250220234458.4bf8dcba@foxbook (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xhci: ring queuing cleanups | expand |
On 21.2.2025 0.44, Michal Pecio wrote: > This function checks if the queued Set Deq pointer really belongs to the > ring it was queued on and updates the ring's dequeue state. It also used > to count free TRBs, but that part has been removed. > > The code is needlessly complex and inefficent, walking TRBs one by one. > And it could "jump off the segment into la-la-land" if a segment doesn't > end with a link TRB or if the link points to another link. > > Make if faster, simpler and more robust. Upgrade xhci_dbg() to xhci_err() > because this situation is a bug and shouldn't happen. > > Signed-off-by: Michal Pecio <michal.pecio@gmail.com> update_ring_for_set_deq_completion() isn't needed anymore. Niklas already wrote a patch to remove it. It's sitting in my for-usb-next branch https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/commit/?h=for-usb-next&id=ee7dab196a7dfc48a1608274329323cb10b4340d Thanks Mathias
On Fri, 21 Feb 2025 15:23:11 +0200, Mathias Nyman wrote: > update_ring_for_set_deq_completion() isn't needed anymore. > Niklas already wrote a patch to remove it. > > It's sitting in my for-usb-next branch > > https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/commit/?h=for-usb-next&id=ee7dab196a7dfc48a1608274329323cb10b4340d I know that it's pure paranoia, but this patch removes a quiet debug message about an obviously abnormal and likely harmful condition. My patch turned it into a nice red error. This will be something to keep in mind, for example somebody could write a patch which reclaims an empty transfer ring segment without chceking if there is a Set TR Dequeue pending to this segemnt. That could lead to very interesting outcomes if all TRBs in the segment are No-Op'ed and the next page is a transfer ring of other endpoint. Other than that, I suppose there is currently no problem. The loop for finding new dequeue position doesn't seem prone to jumping off the ring as long as the ring itself isn't corrupted. Michal
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 965bffce301e..c983d22842dc 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -92,6 +92,11 @@ static bool trb_is_link(union xhci_trb *trb) return TRB_TYPE_LINK_LE32(trb->link.control); } +static bool trb_on_seg(struct xhci_segment *seg, union xhci_trb *trb) +{ + return seg->trbs <= trb && trb < seg->trbs + TRBS_PER_SEGMENT; +} + static bool last_trb_on_seg(struct xhci_segment *seg, union xhci_trb *trb) { return trb == &seg->trbs[TRBS_PER_SEGMENT - 1]; @@ -1332,41 +1337,25 @@ void xhci_hc_died(struct xhci_hcd *xhci) usb_hc_died(xhci_to_hcd(xhci)); } +/* Check if queued pointer is on the ring and update dequeue state */ static void update_ring_for_set_deq_completion(struct xhci_hcd *xhci, struct xhci_virt_device *dev, struct xhci_ring *ep_ring, unsigned int ep_index) { - union xhci_trb *dequeue_temp; + union xhci_trb *deq = dev->eps[ep_index].queued_deq_ptr; + struct xhci_segment *seg; - dequeue_temp = ep_ring->dequeue; - - /* If we get two back-to-back stalls, and the first stalled transfer - * ends just before a link TRB, the dequeue pointer will be left on - * the link TRB by the code in the while loop. So we have to update - * the dequeue pointer one segment further, or we'll jump off - * the segment into la-la-land. - */ - if (trb_is_link(ep_ring->dequeue)) { - ep_ring->deq_seg = ep_ring->deq_seg->next; - ep_ring->dequeue = ep_ring->deq_seg->trbs; - } - - while (ep_ring->dequeue != dev->eps[ep_index].queued_deq_ptr) { - /* We have more usable TRBs */ - ep_ring->dequeue++; - if (trb_is_link(ep_ring->dequeue)) { - if (ep_ring->dequeue == - dev->eps[ep_index].queued_deq_ptr) - break; - ep_ring->deq_seg = ep_ring->deq_seg->next; - ep_ring->dequeue = ep_ring->deq_seg->trbs; - } - if (ep_ring->dequeue == dequeue_temp) { - xhci_dbg(xhci, "Unable to find new dequeue pointer\n"); - break; + /* Search starting from the last known position */ + xhci_for_each_ring_seg(ep_ring->deq_seg, seg) { + if (seg == dev->eps[ep_index].queued_deq_seg && trb_on_seg(seg, deq)) { + ep_ring->deq_seg = seg; + ep_ring->dequeue = deq; + return; } } + + xhci_err(xhci, "Set Deq pointer not on ring\n"); } /*
This function checks if the queued Set Deq pointer really belongs to the ring it was queued on and updates the ring's dequeue state. It also used to count free TRBs, but that part has been removed. The code is needlessly complex and inefficent, walking TRBs one by one. And it could "jump off the segment into la-la-land" if a segment doesn't end with a link TRB or if the link points to another link. Make if faster, simpler and more robust. Upgrade xhci_dbg() to xhci_err() because this situation is a bug and shouldn't happen. Signed-off-by: Michal Pecio <michal.pecio@gmail.com> --- drivers/usb/host/xhci-ring.c | 43 ++++++++++++++---------------------- 1 file changed, 16 insertions(+), 27 deletions(-)