xhci: Remove recursive call to xhci_handle_event

Make the caller loop while there are events to handle, instead.

Signed-off-by Matt Evans<[email protected]>

---
  drivers/usb/host/xhci-ring.c |   16 +++++++++-------
  1 files changed, 9 insertions(+), 7 deletions(-)

drivers/usb/host/xhci-ring.c +0 -0

index b46efd9..97bedd6 100644

Make the caller loop while there are events to handle, instead.

Signed-off-by Matt Evans <[email protected]>

---
1 byte smaller after Sergei's suggestion.

 drivers/usb/host/xhci-ring.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

drivers/usb/host/xhci-ring.c +0 -0

index d6aa880..9c51d69 100644

drivers/usb/host/xhci-ring.c +8 -6

 {
 	union xhci_trb *event;
 	int update_ptrs = 1;

@@ -2140,7 +2140,7 @@ static void xhci_handle_event(struct xhci_hcd *xhci)
 	xhci_dbg(xhci, "In %s\n", __func__);
 	if (!xhci->event_ring || !xhci->event_ring->dequeue) {
 		xhci->error_bitmask |= 1 << 1;
-		return;
+		return 0;
 	}
 
 	event = xhci->event_ring->dequeue;

@@ -2148,7 +2148,7 @@ static void xhci_handle_event(struct xhci_hcd *xhci)
 	if ((le32_to_cpu(event->event_cmd.flags) & TRB_CYCLE) !=
 	    xhci->event_ring->cycle_state) {
 		xhci->error_bitmask |= 1 << 2;
-		return;
+		return 0;
 	}
 	xhci_dbg(xhci, "%s - OS owns TRB\n", __func__);
 

@@ -2192,15 +2192,17 @@ static void xhci_handle_event(struct xhci_hcd *xhci)
 	if (xhci->xhc_state & XHCI_STATE_DYING) {
 		xhci_dbg(xhci, "xHCI host dying, returning from "
 				"event handler.\n");
-		return;
+		return 0;
 	}
 
 	if (update_ptrs)
 		/* Update SW event ring dequeue pointer */
 		inc_deq(xhci, xhci->event_ring, true);
 
-	/* Are there more items on the event ring? */
-	xhci_handle_event(xhci);
+	/* Are there more items on the event ring?  Caller will call us again to
+	 * check.
+	 */
+	return 1;
 }
 
 /*

@@ -2282,7 +2284,7 @@ hw_died:
 	/* FIXME this should be a delayed service routine
 	 * that clears the EHB.
 	 */
-	xhci_handle_event(xhci);
+	while (xhci_handle_event(xhci)) {}
 
 	temp_64 = xhci_read_64(xhci, &xhci->ir_set->erst_dequeue);
 	/* If necessary, update the HW's version of the event ring deq ptr. */

Signature · 1 line

1.7.0.4

What I'd like to do is take out the read of the status register out of the interrupt handler (which is killing performance), and make it only check the status register when xhci_handle_event() returns a negative error status. If the status register shows the host controller has a critical error, the driver should call usb_hcd_died().

Quoted 6 lines · Sarah Sharp

What I'd like to do is take out the read of the status register out of the interrupt handler (which is killing performance), and make it only check the status register when xhci_handle_event() returns a negative error status. If the status register shows the host controller has a critical error, the driver should call usb_hcd_died().

Be careful with removing that read...

Without MSIs, that read is what guarantees that all pending DMA writes by the xHCI have been "flushed" before you start poking at memory.

IE. If the chip writes an event and sends an LSI, without that read, you might get the interrupt before the writes to memory have completed and your driver will "miss" the event.

With MSIs (provided they are not broken on your PCI host bridge of course, this is typically the #1 cause of MSI breakage), you don't need that as the MSI itself is a DMA store by the device which is ordered after the stores done to update the event. So by the time you get the MSI interrupt, you -should- have all the updates visible in memory.

But that means that your PCI host bridge is doing the right thing, by ensuring whatever queues to the coherency domain it has have been properly flushed before it signals the interrupts caused by the MSI to the processors. Hopefully most systems get that right nowadays.

Point is: you need to keep that read if MSIs aren't enabled.

Quoted 3 lines

(quoted diff) +1 -1

-	xhci_handle_event(xhci);
+	while (xhci_handle_event(xhci)) {}

=20

=20
I must admit I dislike the style with empty loop bodies...

drivers/usb/host/xhci-ring.c hw_died: · line 2288

Quoted 7 lines · Matt Evans

drivers/usb/host/xhci-ring.c +1 -1

@@ -2282,7 +2284,7 @@ hw_died:
 	/* FIXME this should be a delayed service routine
 	 * that clears the EHB.
 	 */
-	xhci_handle_event(xhci);
+	while (xhci_handle_event(xhci)) {}
 

I must admit I dislike the style with empty loop bodies, do you think we could have something like below instead?