xhci: Small cleanup of TD giveback and status tracking

xhci: Small cleanup of TD giveback and status tracking

[Michal Pecio]

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

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

[Michal Pecio]

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

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fe60b2ae9f1d..5233ed3e4ed6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -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))
-- 
2.48.1

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

[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 <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 5233ed3e4ed6..34905c8dee25 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -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);
 	}
 }
 
-- 
2.48.1

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

[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 <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 34905c8dee25..e0814282732b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -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) {
-- 
2.48.1