[PATCH 2/3] usb: xhci: Fix isochronous scheduling regression

[Mathias Nyman <mathias.nyman@linux.intel.com>]

From: Michal Pecio <michal.pecio@gmail.com>

An isoc URB without URB_ISO_ASAP should be scheduled immediately after
the previous one, unless it's the first submission or prior URBs have
completed without resubmitting and the endpoint became idle.

An HCD_BH driver must consider URBs pending completion in the BH queue
in addition to its own queue. Regrettably, core doesn't provide much
information, we can only know if we are being called by completion now.
This issue is as old as HCD_BH, affects ehci-hcd too and has no known
reproducible impact, as drivers generally resubmit from completion.

A recent patch tried to address it by looking at xHCI HW state instead.
Obviously, HW has no knowledge of the BH giveback queue either, and the
whole solution amounts to testing whether prior URBs have been unlinked
instead of completing normally - then a new stream is assumed.

This leads to false negatives when a driver simply allows the endpoint
to empty out and begins a new stream. New URBs are scheduled into the
past and promptly fail with -EXDEV status, causing data loss and worse,
because drivers get confused by premature completion, particularly when
multiple endpoints are started at once and required to stay in sync.

snd-usb-audio underruns the OUT endpoint when userspace fails to supply
playback data in time. If this is detected in duplex mode, IN URBs are
unlinked and both streams restarted. OUT underruns again before IN even
begins, another recovery is attempted and the cycle repeats.

Fix this by using the best criteria we can muster, taken from ehci-hcd.
This brings false negative rate back to zero and false positive rate to
less than ever before in xhci-hcd. Traditional logic was equivalent to:

	if (list_empty(&ep_ring->td_list) ||
	    GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
		// consider this URB a new stream

While free of false negatives, it had easily avoidable false positives:
* no check for completion in progress when the list is empty
* the ep_ctx check doesn't make up for it at all, but it adds a race -
  EP state can remain "stopped" for a while after the first submission

[mn: add debug message in possible false positive case where driver might
incorrectly assume new stream starts mid stream just because td list is
empty (URB enqueue is late), and workqueue isn't processing URB
completions for this endpoint at the moment]

Link: https://lore.kernel.org/linux-usb/20260813005635.34750f8c.michal.pecio@gmail.com/
Fixes: add8469b3e00 ("xhci: fix frame id calculation and checks for isoc URBs")
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 97a1b53c18ef..9847c5bfc41b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -4312,11 +4312,16 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
 	check_interval(urb, ep_ctx);
 
 	/*
-	 * Check if this starts the isoc data flow. Relies on hw setting ep ctx
-	 * state after doorbell ring. Consider adding list_empty(td_list) check
+	 * Schedule the URB discontiguously if all previous URBs have completed.
+	 * XXX core can't tell if completions are pending but not running yet.
 	 */
-	if (GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
+	if (list_empty(&ep_ring->td_list) &&
+	    !hcd_periodic_completion_in_progress(xhci_to_hcd(xhci), urb->ep)) {
+		if (GET_EP_CTX_STATE(ep_ctx) == EP_STATE_RUNNING)
+			xhci_dbg(xhci, "Unexpected running ring at isoc stream start, uframe: %d\n",
+				 xep->next_uframe);
 		xep->next_uframe = -1;
+	}
 
 	return xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index);
 }
-- 
2.43.0