[PATCH RFC 0/2] xhci: Simplify TD cancellation and drop some states

[PATCH RFC 0/2] xhci: Simplify TD cancellation and drop some states

[Michal Pecio]

Hi,

Thoughts about this?

Thanks to asynchronous giveback we can give back TDs at any time we
want without dropping xhci->lock and worrying what we might find when
we take it back. So give back TDs as soon as they are no longer needed
instead of keeping them around in TD_CLEARED state and using a second
pass through the list to give them back.

This turns out to be a surprisingly simple patch and I gave it a quick
test and it's working fine.

The second patch removes another almost meaningless cancel status and
shortens the code a bit while adding functionality which may or may not
turn out to be useful. And hopefully won't be harmful?

That too was tested, by patching xhci_move_dequeue_past_td() to fail
randomly and unlinking from multiple streams on UAS. Works exactly as
described in the commit message.

Overall, the driver lost 40 lines of code and 2KB in binary size. The
latter is surprising, I think I haven't removed 2KB worth of code, but
maybe it affects how gcc inlines or otherwise changes functions. YMMV.

Regards,
Michal

[PATCH RFC 1/2] usb: xhci: Drop the TD_CLEARED cancel status

[Michal Pecio]

There seems to be no need to keep invalidated TDs around and require
callers to follow up with xhci_giveback_invalidated_tds(), so make
xhci_invalidate_cancelled_tds() give back invalidated TDs right away.

TD_CLEARED cancel status is no longer useful and therefore removed.
Code complexity and overhead of repeated list traversals are reduced.

There is no more need to debug interactions between these functions,
so a big source of xhci_dbg() noise (and potential bugs) goes away.
From now, "Removing cancelled TD starting at ..." really means that
the TD is being removed, unless one of the errors below is logged or
dynamic debug indicates that Set TR Dequeue is queued or deferred.

Also clean up some debug noise from xhci_handle_cmd_set_deq(), which
practically duplicates xhci_invalidate_cancelled_tds() messages that
will be printed just before this completion handler returns.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 49 +++++++-----------------------------
 drivers/usb/host/xhci.h      |  1 -
 2 files changed, 9 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 12087f2f9db7..fd0f8a9196e2 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -927,30 +927,6 @@ static void xhci_dequeue_td(struct xhci_hcd *xhci, struct xhci_td *td, struct xh
 	xhci_td_cleanup(xhci, td, ring, status);
 }
 
-/* Complete the cancelled URBs we unlinked from td_list. */
-static void xhci_giveback_invalidated_tds(struct xhci_virt_ep *ep)
-{
-	struct xhci_ring *ring;
-	struct xhci_td *td, *tmp_td;
-
-	list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list,
-				 cancelled_td_list) {
-
-		ring = xhci_urb_to_transfer_ring(ep->xhci, td->urb);
-
-		if (td->cancel_status == TD_CLEARED) {
-			xhci_dbg(ep->xhci, "%s: Giveback cancelled URB %p TD\n",
-				 __func__, td->urb);
-			xhci_td_cleanup(ep->xhci, td, ring, td->status);
-		} else {
-			xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n",
-				 __func__, td->urb, td->cancel_status);
-		}
-		if (ep->xhci->xhc_state & XHCI_STATE_DYING)
-			return;
-	}
-}
-
 static int xhci_reset_halted_ep(struct xhci_hcd *xhci, unsigned int slot_id,
 				unsigned int ep_index, enum xhci_ep_reset_type reset_type)
 {
@@ -1031,7 +1007,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 	struct xhci_td		*td = NULL;
 	struct xhci_td		*tmp_td = NULL;
 	struct xhci_td		*cached_td = NULL;
-	struct xhci_ring	*ring;
+	struct xhci_ring	*ring, *cached_ring = NULL;
 	u64			hw_deq;
 	unsigned int		slot_id = ep->vdev->slot_id;
 	int			err;
@@ -1070,7 +1046,6 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 
 		if (td->cancel_status == TD_HALTED || trb_in_td(td, hw_deq)) {
 			switch (td->cancel_status) {
-			case TD_CLEARED: /* TD is already no-op */
 			case TD_CLEARING_CACHE: /* set TR deq command already queued */
 				break;
 			case TD_DIRTY: /* TD is cached, clear it */
@@ -1092,16 +1067,18 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 						  td->urb, cached_td->urb,
 						  td->urb->stream_id);
 					td_to_noop(cached_td, false);
-					cached_td->cancel_status = TD_CLEARED;
+					xhci_td_cleanup(xhci, cached_td, cached_ring,
+							cached_td->status);
 				}
 				td_to_noop(td, false);
 				td->cancel_status = TD_CLEARING_CACHE;
+				cached_ring = ring;
 				cached_td = td;
 				break;
 			}
 		} else {
 			td_to_noop(td, false);
-			td->cancel_status = TD_CLEARED;
+			xhci_td_cleanup(xhci, td, ring, td->status);
 		}
 	}
 
@@ -1123,10 +1100,12 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 			if (td->cancel_status != TD_CLEARING_CACHE &&
 			    td->cancel_status != TD_CLEARING_CACHE_DEFERRED)
 				continue;
-			xhci_warn(xhci, "Failed to clear cancelled cached URB %p, mark clear anyway\n",
+			xhci_warn(xhci, "Failed to clear cancelled cached URB %p, give back anyway\n",
 				  td->urb);
 			td_to_noop(td, false);
-			td->cancel_status = TD_CLEARED;
+			ring = xhci_urb_to_transfer_ring(xhci, td->urb);
+			xhci_td_cleanup(xhci, td, ring, td->status);
+
 		}
 	}
 	return 0;
@@ -1142,7 +1121,6 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 void xhci_process_cancelled_tds(struct xhci_virt_ep *ep)
 {
 	xhci_invalidate_cancelled_tds(ep);
-	xhci_giveback_invalidated_tds(ep);
 }
 
 /*
@@ -1295,7 +1273,6 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
 	ep->ep_state &= ~EP_STOP_CMD_PENDING;
 
 	/* Otherwise ring the doorbell(s) to restart queued transfers */
-	xhci_giveback_invalidated_tds(ep);
 	ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
 }
 
@@ -1517,13 +1494,9 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 				 cancelled_td_list) {
 		ep_ring = xhci_urb_to_transfer_ring(ep->xhci, td->urb);
 		if (td->cancel_status == TD_CLEARING_CACHE) {
-			td->cancel_status = TD_CLEARED;
 			xhci_dbg(ep->xhci, "%s: Giveback cancelled URB %p TD\n",
 				 __func__, td->urb);
 			xhci_td_cleanup(ep->xhci, td, ep_ring, td->status);
-		} else {
-			xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n",
-				 __func__, td->urb, td->cancel_status);
 		}
 	}
 cleanup:
@@ -1538,8 +1511,6 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 		xhci_invalidate_cancelled_tds(ep);
 		/* Try to restart the endpoint if all is done */
 		ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
-		/* Start giving back any TDs invalidated above */
-		xhci_giveback_invalidated_tds(ep);
 	} else {
 		/* Restart any rings with pending URBs */
 		xhci_dbg(ep->xhci, "%s: All TDs cleared, ring doorbell\n", __func__);
@@ -1574,8 +1545,6 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,
 	/* Clear our internal halted state */
 	ep->ep_state &= ~EP_HALTED;
 
-	xhci_giveback_invalidated_tds(ep);
-
 	/* if this was a soft reset, then restart */
 	if ((le32_to_cpu(trb->generic.field[3])) & TRB_TSP)
 		ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e87ffb284ab2..94394db271bf 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1297,7 +1297,6 @@ enum xhci_cancelled_td_status {
 	TD_HALTED,
 	TD_CLEARING_CACHE,
 	TD_CLEARING_CACHE_DEFERRED,
-	TD_CLEARED,
 };
 
 struct xhci_td {
-- 
2.48.1

[PATCH RFC 2/2] usb: xhci: Drop the TD_CLEARING_CACHE_DEFERRED cancel

[Michal Pecio]

The last remaining use is inside xhci_invalidate_cancelled_tds() for
destroying deferred TDs under certain failure conditions.

Instead of violating the spec by crudely erasing those deferred TDs,
we may give them one last chance to get their Set TR Dequeue. Maybe
the prior failure was caused by some corruption of one stream's ring
and other streams are fine, maybe it was a transient -ENOMEM.

Or maybe we want to cover this obscure case with less extra code.

Remove the search for TD_CLEARING_CACHE_DEFERRED TDs, nuke the one
cached TD we tried and failed to skip, and jump back to the main
loop to handle any TDs still in TD_DIRTY or TD_HALTED state. These
are our deferred TDs - all other dirty TDs have been given back.

If we fail again, the deferred TD will be nuked and so on, until we
run out of TDs. We are under the lock so nobody can keep submitting
and cancelling them forever.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 26 +++++++++-----------------
 drivers/usb/host/xhci.h      |  1 -
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fd0f8a9196e2..91a9aa6aec77 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1021,6 +1021,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 
 	xhci = ep->xhci;
 
+again:
 	list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) {
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
 			       "Removing canceled TD starting at 0x%llx (dma) in stream %u URB %p",
@@ -1050,14 +1051,12 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 				break;
 			case TD_DIRTY: /* TD is cached, clear it */
 			case TD_HALTED:
-			case TD_CLEARING_CACHE_DEFERRED:
 				if (cached_td) {
 					if (cached_td->urb->stream_id != td->urb->stream_id) {
 						/* Multiple streams case, defer move dq */
 						xhci_dbg(xhci,
 							 "Move dq deferred: stream %u URB %p\n",
 							 td->urb->stream_id, td->urb);
-						td->cancel_status = TD_CLEARING_CACHE_DEFERRED;
 						break;
 					}
 
@@ -1091,22 +1090,15 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 					cached_td);
 	if (err) {
 		/* Failed to move past cached td, just set cached TDs to no-op */
-		list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) {
-			/*
-			 * Deferred TDs need to have the deq pointer set after the above command
-			 * completes, so if that failed we just give up on all of them (and
-			 * complain loudly since this could cause issues due to caching).
-			 */
-			if (td->cancel_status != TD_CLEARING_CACHE &&
-			    td->cancel_status != TD_CLEARING_CACHE_DEFERRED)
-				continue;
-			xhci_warn(xhci, "Failed to clear cancelled cached URB %p, give back anyway\n",
-				  td->urb);
-			td_to_noop(td, false);
-			ring = xhci_urb_to_transfer_ring(xhci, td->urb);
-			xhci_td_cleanup(xhci, td, ring, td->status);
+		xhci_warn(xhci, "Failed to clear cancelled cached URB %p, give back anyway\n",
+				cached_td->urb);
+		td_to_noop(cached_td, false);
+		xhci_td_cleanup(xhci, cached_td, cached_ring, cached_td->status);
 
-		}
+		/* Try to clear any deferred TDs on other streams */
+		cached_ring = NULL;
+		cached_td = NULL;
+		goto again;
 	}
 	return 0;
 }
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 94394db271bf..1eaa70f7e62e 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1296,7 +1296,6 @@ enum xhci_cancelled_td_status {
 	TD_DIRTY = 0,
 	TD_HALTED,
 	TD_CLEARING_CACHE,
-	TD_CLEARING_CACHE_DEFERRED,
 };
 
 struct xhci_td {
-- 
2.48.1

Re: [PATCH RFC 1/2] usb: xhci: Drop the TD_CLEARED cancel status

[Mathias Nyman]

On 1.9.2025 9.48, Michal Pecio wrote:
> There seems to be no need to keep invalidated TDs around and require
> callers to follow up with xhci_giveback_invalidated_tds(), so make
> xhci_invalidate_cancelled_tds() give back invalidated TDs right away.
> 
> TD_CLEARED cancel status is no longer useful and therefore removed.
> Code complexity and overhead of repeated list traversals are reduced.
> 
> There is no more need to debug interactions between these functions,
> so a big source of xhci_dbg() noise (and potential bugs) goes away.
>  From now, "Removing cancelled TD starting at ..." really means that
> the TD is being removed, unless one of the errors below is logged or
> dynamic debug indicates that Set TR Dequeue is queued or deferred.
> 
> Also clean up some debug noise from xhci_handle_cmd_set_deq(), which
> practically duplicates xhci_invalidate_cancelled_tds() messages that
> will be printed just before this completion handler returns.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> ---


Looks good to me.

xhci_giveback_invalidated_tds() was anyways called right after
xhci_invalidate_cancelled_tds() under the same lock protection
so we can as well give back those non-cached cancelled TDs right away.

Thanks
Mathias

Re: [PATCH RFC 2/2] usb: xhci: Drop the TD_CLEARING_CACHE_DEFERRED cancel

[Mathias Nyman]

On 1.9.2025 9.48, Michal Pecio wrote:
> The last remaining use is inside xhci_invalidate_cancelled_tds() for
> destroying deferred TDs under certain failure conditions.
> 
> Instead of violating the spec by crudely erasing those deferred TDs,
> we may give them one last chance to get their Set TR Dequeue. Maybe
> the prior failure was caused by some corruption of one stream's ring
> and other streams are fine, maybe it was a transient -ENOMEM.
> 
> Or maybe we want to cover this obscure case with less extra code.
> 
> Remove the search for TD_CLEARING_CACHE_DEFERRED TDs, nuke the one
> cached TD we tried and failed to skip, and jump back to the main
> loop to handle any TDs still in TD_DIRTY or TD_HALTED state. These
> are our deferred TDs - all other dirty TDs have been given back.
> 
> If we fail again, the deferred TD will be nuked and so on, until we
> run out of TDs. We are under the lock so nobody can keep submitting
> and cancelling them forever.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>

Looks good to me.

Gives every stream ring a chance to move past the cancelled TD, unlike
the current code that bails out all stream rings on first set TR Deq failure.

Thanks
Mathias

Re: [PATCH RFC 1/2] usb: xhci: Drop the TD_CLEARED cancel status

[Michał Pecio]

On Mon, 1 Sep 2025 12:47:10 +0300, Mathias Nyman wrote:
> On 1.9.2025 9.48, Michal Pecio wrote:
> > There seems to be no need to keep invalidated TDs around and require
> > callers to follow up with xhci_giveback_invalidated_tds(), so make
> > xhci_invalidate_cancelled_tds() give back invalidated TDs right away.
> > 
> > TD_CLEARED cancel status is no longer useful and therefore removed.
> > Code complexity and overhead of repeated list traversals are reduced.
> > 
> > There is no more need to debug interactions between these functions,
> > so a big source of xhci_dbg() noise (and potential bugs) goes away.
> >  From now, "Removing cancelled TD starting at ..." really means that
> > the TD is being removed, unless one of the errors below is logged or
> > dynamic debug indicates that Set TR Dequeue is queued or deferred.
> > 
> > Also clean up some debug noise from xhci_handle_cmd_set_deq(), which
> > practically duplicates xhci_invalidate_cancelled_tds() messages that
> > will be printed just before this completion handler returns.
> > 
> > Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> > ---  
> 
> 
> Looks good to me.
> 
> xhci_giveback_invalidated_tds() was anyways called right after
> xhci_invalidate_cancelled_tds() under the same lock protection
> so we can as well give back those non-cached cancelled TDs right away.

Thank you, that's what it looked like to me.

The only reason I could imagine for possibly wanting this state to
exist is if somebody somehow demanded that cancelled TDs are given
back in some particular order (submit order, cancellation order),
but this driver has been delaying pending TD giveback due to Set Deq
for years. Nobody seems to be complaining and I couldn't find any
mention of giveback order in places like usb_unlink_urb().

I'm thikning about a v2 with small changes:

1. add a dynamic debug when a cached TD is selected for clearing -
   currently only the deffered cached TDs are logged instantly and
   we need to look for "Set TR Deq ptr ..." announcement later to
   know which TD was chosen.

2. add a patch to replace all 'break' with 'continue' in this function,
   because they break out of a switch nested in a loop, skip over an
   inactive 'else' clause and trigger a new iteration immediately.

Regards,
Michal