Skip to contentSkip to conversation

xhci: Small cleanup of TD giveback and status tracking

PATCH New Michal Pecio 1 participant 4 messages Sep 11, 2025

Subsystems (2)
USB USB xHCI driver
lore
Share & tools
lore

Michal Pecio

author
#0 lore raw

Hi,

I noticed some redundant 'status' parameters which obscure how URB status assignment really works (it's very simple at the moment) and create opportunity for errors like cleanup(cached_td, td->status).

Removing them seems harmless.

Also included is a copypasta cleanup and removal of some questionable IMO logic, partly because it was getting in the way.

Regards,
Michal

author
#1 lore raw

PATCH 1/3 usb: xhci: Don't change status on length mismatch

It seems unwise to hide errors simply because the driver lost track of how much data was actually transferred.

Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 1 -
 1 file changed, 1 deletion(-)
drivers/usb/host/xhci-ring.c +0 -1
index fe60b2ae9f1d..5233ed3e4ed6 100644
@@ -890,7 +890,6 @@ static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
 		xhci_warn(xhci, "URB req %u and actual %u transfer length mismatch\n",
 			  urb->transfer_buffer_length, urb->actual_length);
 		urb->actual_length = 0;
-		status = 0;
 	}
 	/* TD might be removed from td_list if we are giving back a cancelled URB */
 	if (!list_empty(&td->td_list))
Signature · 1 line
2.48.1
author
#2 lore raw

PATCH 2/3 usb: xhci: Deduplicate TD cleanup code

In reply to Michal Pecio

One case is almost a copy of xhci_td_cleanup(), the other isn't but it should - a TD still on this list hasn't had its bounce buffer unmapped yet or else xhci_td_cleanup() would have removed it from the list.

A side effect is that those TDs will now also be checked for transfer length mismatch and their giveback will be logged. Should be harmless, and the logging helped confirm that the patched code executes.

Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)
drivers/usb/host/xhci-ring.c +8 -20
index 5233ed3e4ed6..34905c8dee25 100644
@@ -901,6 +901,10 @@ static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
 	inc_td_cnt(urb);
 	/* Giveback the urb when all the tds are completed */
 	if (last_td_in_urb(td)) {
+		/* set isoc urb status to 0 just as EHCI, UHCI, and OHCI */
+		if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS && status != -ESHUTDOWN)
+			status = 0;
+
 		if ((urb->actual_length != urb->transfer_buffer_length &&
 		     (urb->transfer_flags & URB_SHORT_NOT_OK)) ||
 		    (status != 0 && !usb_endpoint_xfer_isoc(&urb->ep->desc)))

@@ -908,9 +912,6 @@ static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
 				 urb, urb->actual_length,
 				 urb->transfer_buffer_length, status);
 
-		/* set isoc urb status to 0 just as EHCI, UHCI, and OHCI */
-		if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
-			status = 0;
 		xhci_giveback_urb_in_irq(xhci, td, status);
 	}
 }

@@ -1304,16 +1305,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, struct xhci_ring *ring)
 	struct xhci_td *tmp;
 
 	list_for_each_entry_safe(cur_td, tmp, &ring->td_list, td_list) {
-		list_del_init(&cur_td->td_list);
-
-		if (!list_empty(&cur_td->cancelled_td_list))
-			list_del_init(&cur_td->cancelled_td_list);
-
-		xhci_unmap_td_bounce_buffer(xhci, ring, cur_td);
-
-		inc_td_cnt(cur_td->urb);
-		if (last_td_in_urb(cur_td))
-			xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
+		xhci_td_cleanup(xhci, cur_td, ring, -ESHUTDOWN);
 	}
 }
 

@@ -1354,13 +1346,9 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
 		xhci_kill_ring_urbs(xhci, ring);
 	}
 
-	list_for_each_entry_safe(cur_td, tmp, &ep->cancelled_td_list,
-			cancelled_td_list) {
-		list_del_init(&cur_td->cancelled_td_list);
-		inc_td_cnt(cur_td->urb);
-
-		if (last_td_in_urb(cur_td))
-			xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
+	list_for_each_entry_safe(cur_td, tmp, &ep->cancelled_td_list, cancelled_td_list) {
+		ring = xhci_urb_to_transfer_ring(xhci, cur_td->urb);
+		xhci_td_cleanup(xhci, cur_td, ring, -ESHUTDOWN);
 	}
 }
Signature · 1 line
2.48.1
author
#3 lore raw

PATCH 3/3 usb: xhci: Clean up TD status passing

In reply to Michal Pecio

xhci_td_cleanup() has a 'status' parameter, which determines completion status of the URB if this happens to be the last TD to be given back. Most callers simply pass td->status here.

One bogus case is skip_isoc_td(), which is called with status determined by some TD which triggered skipping, not the one being skipped, and the status is later ignored anyway because it's an isochronous URB.

All those parameters can be removed and td->status used automatically by xhci_td_cleanup(). This makes the URB status policy immediately clear.

Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
drivers/usb/host/xhci-ring.c +15 -16
index 34905c8dee25..e0814282732b 100644
@@ -870,10 +870,10 @@ static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
 	seg->bounce_offs = 0;
 }
 
-static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
-			    struct xhci_ring *ep_ring, int status)
+static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td, struct xhci_ring *ep_ring)
 {
 	struct urb *urb = NULL;
+	int status = td->status;
 
 	/* Clean up the endpoint's TD list */
 	urb = td->urb;

@@ -917,14 +917,13 @@ static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
 }
 
 /* Give back previous TD and move on to the next TD. */
-static void xhci_dequeue_td(struct xhci_hcd *xhci, struct xhci_td *td, struct xhci_ring *ring,
-			    u32 status)
+static void xhci_dequeue_td(struct xhci_hcd *xhci, struct xhci_td *td, struct xhci_ring *ring)
 {
 	ring->dequeue = td->end_trb;
 	ring->deq_seg = td->end_seg;
 	inc_deq(xhci, ring);
 
-	xhci_td_cleanup(xhci, td, ring, status);
+	xhci_td_cleanup(xhci, td, ring);
 }
 
 /* Complete the cancelled URBs we unlinked from td_list. */

@@ -941,7 +940,7 @@ static void xhci_giveback_invalidated_tds(struct xhci_virt_ep *ep)
 		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);
+			xhci_td_cleanup(ep->xhci, td, ring);
 		} else {
 			xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n",
 				 __func__, td->urb, td->cancel_status);

@@ -1305,7 +1304,8 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, struct xhci_ring *ring)
 	struct xhci_td *tmp;
 
 	list_for_each_entry_safe(cur_td, tmp, &ring->td_list, td_list) {
-		xhci_td_cleanup(xhci, cur_td, ring, -ESHUTDOWN);
+		cur_td->status = -ESHUTDOWN;
+		xhci_td_cleanup(xhci, cur_td, ring);
 	}
 }
 

@@ -1347,8 +1347,8 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
 	}
 
 	list_for_each_entry_safe(cur_td, tmp, &ep->cancelled_td_list, cancelled_td_list) {
-		ring = xhci_urb_to_transfer_ring(xhci, cur_td->urb);
-		xhci_td_cleanup(xhci, cur_td, ring, -ESHUTDOWN);
+		cur_td->status = -ESHUTDOWN;
+		xhci_td_cleanup(xhci, cur_td, xhci_urb_to_transfer_ring(xhci, cur_td->urb));
 	}
 }
 

@@ -1510,7 +1510,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 			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);
+			xhci_td_cleanup(ep->xhci, td, ep_ring);
 		} else {
 			xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n",
 				 __func__, td->urb, td->cancel_status);

@@ -2278,7 +2278,7 @@ static void finish_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		break;
 	}
 
-	xhci_dequeue_td(xhci, td, ep_ring, td->status);
+	xhci_dequeue_td(xhci, td, ep_ring);
 }
 
 /* sum trb lengths from the first trb up to stop_trb, _excluding_ stop_trb */

@@ -2500,8 +2500,7 @@ static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	finish_td(xhci, ep, ep_ring, td, trb_comp_code);
 }
 
-static void skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
-			 struct xhci_virt_ep *ep, int status)
+static void skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td, struct xhci_ring *ep_ring)
 {
 	struct urb_priv *urb_priv;
 	struct usb_iso_packet_descriptor *frame;

@@ -2517,7 +2516,7 @@ static void skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
 	/* calc actual length */
 	frame->actual_length = 0;
 
-	xhci_dequeue_td(xhci, td, ep->ring, status);
+	xhci_dequeue_td(xhci, td, ep_ring);
 }
 
 /*

@@ -2822,7 +2821,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 	if (td && td->error_mid_td && !trb_in_td(td, ep_trb_dma)) {
 		xhci_dbg(xhci, "Missing TD completion event after mid TD error\n");
-		xhci_dequeue_td(xhci, td, ep_ring, td->status);
+		xhci_dequeue_td(xhci, td, ep_ring);
 	}
 
 	/* If the TRB pointer is NULL, missed TDs will be skipped on the next event */

@@ -2862,7 +2861,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 				if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
 					return 0;
 
-				skip_isoc_td(xhci, td, ep, status);
+				skip_isoc_td(xhci, td, ep_ring);
 
 				if (!list_empty(&ep_ring->td_list)) {
 					if (ring_xrun_event) {
Signature · 1 line
2.48.1