diff mbox series

[RFC] xhci: fix matching completion events with TDs

Message ID 20240123112230.4d9e1ef0@foxbook (mailing list archive)
State Superseded
Headers show
Series [RFC] xhci: fix matching completion events with TDs | expand

Commit Message

MichaƂ Pecio Jan. 23, 2024, 10:22 a.m. UTC
A trb_in_td() call is used to determine if a completion event matches
one of the TRBs of the currently executing TD. This function is given
ep_ring->deq_seg and ep_ring->dequeue as the starting point to search,
but these have been observed to be out of sync with td->start_seg and
td->first_trb of the current TD under some circumstances when the EP
is being stopped.

Consequently, spurious completion events have been observed to become
erroneously associated with an unrelated pending TD.

Since the ring is being searched for the specific purpose of finding
a match with current TD, always start from current TD's first TRB.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---

Questions:

Is this a good idea? Is it useful? Or am I missing something?

Should a diagnostic be emitted when these pointers don't match? Or when
they don't match AND something else is also wrong?

And yes, I am observing unexpected completion events when stopping an
isochronous endpoint on NEC uPD720200. They used to retire the current
TD and "TRB not in TD" appeared when its true completion arrived, now
they correctly give "TRB not in TD" right away. I consider it progress,
though the origin of spurious completions is still unknown.


 drivers/usb/host/xhci-ring.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9673354d70d5..d9be5023abe6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2809,7 +2809,7 @@  static int handle_tx_event(struct xhci_hcd *xhci,
 			td_num--;
 
 		/* Is this a TRB in the currently executing TD? */
-		ep_seg = trb_in_td(xhci, ep_ring->deq_seg, ep_ring->dequeue,
+		ep_seg = trb_in_td(xhci, td->start_seg, td->first_trb,
 				td->last_trb, ep_trb_dma, false);
 
 		/*
@@ -2877,9 +2877,8 @@  static int handle_tx_event(struct xhci_hcd *xhci,
 					"part of current TD ep_index %d "
 					"comp_code %u\n", ep_index,
 					trb_comp_code);
-				trb_in_td(xhci, ep_ring->deq_seg,
-					  ep_ring->dequeue, td->last_trb,
-					  ep_trb_dma, true);
+				trb_in_td(xhci, td->start_seg, td->first_trb,
+					  td->last_trb, ep_trb_dma, true);
 				return -ESHUTDOWN;
 			}
 		}