diff mbox

[v2,5/5] xhci: Remove recursive call to xhci_handle_event

Message ID 20110329200017.GA25480@dtor-ws.eng.vmware.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Dmitry Torokhov March 29, 2011, 8 p.m. UTC 
On Sunday, March 27, 2011 09:53:00 pm Matt Evans wrote:
> @@ -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?

Thanks!

Comments

David Laight March 30, 2011, 7:51 a.m. UTC | #1 
> > -	xhci_handle_event(xhci);
> > +	while (xhci_handle_event(xhci)) {}
> > 
> 
> I must admit I dislike the style with empty loop bodies...

I would use an explicit 'continue;' for the body
of an otherwise empty loop.

	David
Matt Evans March 30, 2011, 11:10 p.m. UTC | #2 
On 30/03/11 07:00, Dmitry Torokhov wrote:
> On Sunday, March 27, 2011 09:53:00 pm Matt Evans wrote:
>> @@ -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?

Well, although I don't mind empty while()s at all (they're clean and obvious
IMHO) I would remove an empty blightful while loop with something like this:

do {
	ret = xhci_handle_event(xhci);
} while (ret > 0);

;-) (Not sure that refactoring its contents into the IRQ handler is a good idea,
if that area's going to be revisited soon to extend error
handling/reporting.)

Cheers,


Matt
diff mbox

Patch

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 0e30281..8e6d8fa 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2131,26 +2131,12 @@  cleanup:
  * This function handles all OS-owned events on the event ring.  It may drop
  * xhci->lock between event processing (e.g. to pass up port status changes).
  */
-static void xhci_handle_event(struct xhci_hcd *xhci)
+static void xhci_handle_event(struct xhci_hcd *xhci, union xhci_trb *event)
 {
-	union xhci_trb *event;
-	int update_ptrs = 1;
+	bool update_ptrs = true;
 	int ret;
 
 	xhci_dbg(xhci, "In %s\n", __func__);
-	if (!xhci->event_ring || !xhci->event_ring->dequeue) {
-		xhci->error_bitmask |= 1 << 1;
-		return;
-	}
-
-	event = xhci->event_ring->dequeue;
-	/* Does the HC or OS own the TRB? */
-	if ((event->event_cmd.flags & TRB_CYCLE) !=
-			xhci->event_ring->cycle_state) {
-		xhci->error_bitmask |= 1 << 2;
-		return;
-	}
-	xhci_dbg(xhci, "%s - OS owns TRB\n", __func__);
 
 	/* FIXME: Handle more event types. */
 	switch ((event->event_cmd.flags & TRB_TYPE_BITMASK)) {
@@ -2163,7 +2149,7 @@  static void xhci_handle_event(struct xhci_hcd *xhci)
 		xhci_dbg(xhci, "%s - calling handle_port_status\n", __func__);
 		handle_port_status(xhci, event);
 		xhci_dbg(xhci, "%s - returned from handle_port_status\n", __func__);
-		update_ptrs = 0;
+		update_ptrs = false;
 		break;
 	case TRB_TYPE(TRB_TRANSFER):
 		xhci_dbg(xhci, "%s - calling handle_tx_event\n", __func__);
@@ -2172,7 +2158,7 @@  static void xhci_handle_event(struct xhci_hcd *xhci)
 		if (ret < 0)
 			xhci->error_bitmask |= 1 << 9;
 		else
-			update_ptrs = 0;
+			update_ptrs = false;
 		break;
 	default:
 		if ((event->event_cmd.flags & TRB_TYPE_BITMASK) >= TRB_TYPE(48))
@@ -2180,21 +2166,9 @@  static void xhci_handle_event(struct xhci_hcd *xhci)
 		else
 			xhci->error_bitmask |= 1 << 3;
 	}
-	/* Any of the above functions may drop and re-acquire the lock, so check
-	 * to make sure a watchdog timer didn't mark the host as non-responsive.
-	 */
-	if (xhci->xhc_state & XHCI_STATE_DYING) {
-		xhci_dbg(xhci, "xHCI host dying, returning from "
-				"event handler.\n");
-		return;
-	}
 
 	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);
 }
 
 /*
@@ -2258,34 +2232,37 @@  hw_died:
 		xhci_writel(xhci, irq_pending, &xhci->ir_set->irq_pending);
 	}
 
-	if (xhci->xhc_state & XHCI_STATE_DYING) {
-		xhci_dbg(xhci, "xHCI dying, ignoring interrupt. "
-				"Shouldn't IRQs be disabled?\n");
-		/* Clear the event handler busy flag (RW1C);
-		 * the event ring should be empty.
-		 */
-		temp_64 = xhci_read_64(xhci, &xhci->ir_set->erst_dequeue);
-		xhci_write_64(xhci, temp_64 | ERST_EHB,
-				&xhci->ir_set->erst_dequeue);
-		spin_unlock(&xhci->lock);
-
-		return IRQ_HANDLED;
-	}
-
 	event_ring_deq = xhci->event_ring->dequeue;
+
 	/* FIXME this should be a delayed service routine
 	 * that clears the EHB.
 	 */
-	xhci_handle_event(xhci);
+	xhci_dbg(xhci, "%s - Begin processing TRBs\n", __func__);
+
+	while (!(xhci->xhc_state & XHCI_STATE_DYING)) {
+		union xhci_trb *event = xhci->event_ring->dequeue;
+
+		/* Does the HC or OS own the TRB? */
+		if ((event->event_cmd.flags & TRB_CYCLE) !=
+				xhci->event_ring->cycle_state) {
+			break;
+		}
+
+		xhci_dbg(xhci, "%s - OS owns TRB\n", __func__);
+		xhci_handle_event(xhci, event);
+	}
 
 	temp_64 = xhci_read_64(xhci, &xhci->ir_set->erst_dequeue);
-	/* If necessary, update the HW's version of the event ring deq ptr. */
-	if (event_ring_deq != xhci->event_ring->dequeue) {
+
+	if (unlikely(xhci->xhc_state & XHCI_STATE_DYING)) {
+		xhci_dbg(xhci, "%s: xHCI is dying, exiting ISR\n", __func__);
+	} else if (event_ring_deq != xhci->event_ring->dequeue) {
+		/* Update the HW's version of the event ring deq ptr. */
 		deq = xhci_trb_virt_to_dma(xhci->event_ring->deq_seg,
-				xhci->event_ring->dequeue);
+					xhci->event_ring->dequeue);
 		if (deq == 0)
-			xhci_warn(xhci, "WARN something wrong with SW event "
-					"ring dequeue ptr.\n");
+			xhci_warn(xhci,
+				  "WARN something wrong with SW event ring dequeue ptr.\n");
 		/* Update HC event ring dequeue pointer */
 		temp_64 &= ERST_PTR_MASK;
 		temp_64 |= ((u64) deq & (u64) ~ERST_PTR_MASK);