[PATCH 5/5] xhci: Remove recursive call to xhci_handle_event

[PATCH 5/5] xhci: Remove recursive call to xhci_handle_event

[Matt Evans]

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

Signed-off-by: Matt Evans <matt@ozlabs.org>
---
 drivers/usb/host/xhci-ring.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b46efd9..97bedd6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2131,7 +2131,7 @@ 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 int xhci_handle_event(struct xhci_hcd *xhci)
 {
 	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__);
 	rmb();
@@ -2187,15 +2187,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;
 }
 
 /*
@@ -2277,7 +2279,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. */
-- 
1.7.0.4

Re: [PATCH 5/5] xhci: Remove recursive call to xhci_handle_event

[Sergei Shtylyov]

Hello.

On 25-03-2011 10:44, Matt Evans wrote:

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

> Signed-off-by: Matt Evans<matt@ozlabs.org>
> ---
>   drivers/usb/host/xhci-ring.c |   16 +++++++++-------
>   1 files changed, 9 insertions(+), 7 deletions(-)

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index b46efd9..97bedd6 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
[...]
> @@ -2277,7 +2279,7 @@ hw_died:
>   	/* FIXME this should be a delayed service routine
>   	 * that clears the EHB.
>   	 */
> -	xhci_handle_event(xhci);
> +	while (xhci_handle_event(xhci)) {};

    Semicolon not needed after }. Perhaps the committer could change this...

WBR, Sergei

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

[Matt Evans]

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

Signed-off-by: Matt Evans <matt@ozlabs.org>
---
1 byte smaller after Sergei's suggestion.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d6aa880..9c51d69 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2131,7 +2131,7 @@ 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 int xhci_handle_event(struct xhci_hcd *xhci)
 {
 	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. */
-- 
1.7.0.4

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

[Sarah Sharp]

On Mon, Mar 28, 2011 at 03:53:00PM +1100, Matt Evans wrote:
> Make the caller loop while there are events to handle, instead.
> 
> Signed-off-by: Matt Evans <matt@ozlabs.org>
> ---
> 1 byte smaller after Sergei's suggestion.
> 
>  drivers/usb/host/xhci-ring.c |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d6aa880..9c51d69 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2131,7 +2131,7 @@ 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 int xhci_handle_event(struct xhci_hcd *xhci)

Can you add documentation here about what this function returns?

Also, there's the issue of error handling.  It's kind of a future
functionality, rather than a needed bug fix, but this code reminded me
of the issue.

If we get a critical error, like when the xHCI host controller is dead
(the XHCI_STATE_DYING case), then the interrupt handler really shouldn't
call xhci_handle_event again.  There's no point in processing the event
ring further if something is seriously wrong with the host controller,
as it can be writing absolute garbage to the ring at that point.  This
isn't what's been done in the past (we just blindly process), so we
might find more bugs in software/hardware implementations.

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().

Sarah Sharp

>  {
>  	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. */
> -- 
> 1.7.0.4
> 

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

[Benjamin Herrenschmidt]

On Mon, 2011-03-28 at 15:34 -0700, Sarah Sharp wrote:
> 
> 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.

Cheers,
Ben.

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

[Sarah Sharp]

On Tue, Mar 29, 2011 at 10:58:54AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2011-03-28 at 15:34 -0700, Sarah Sharp wrote:
> > 
> > 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.

Sorry for the sloppy language, yes, I understand I still need the status
register read if only legacy IRQs are enabled.

Sarah Sharp

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

[Dmitry Torokhov]

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!

-- 
Dmitry

From: Dmitry Torokhov <dtor@vmware.com>
Subject: [PATCH] USB: xhci: avoid recursion in xhci_handle_event

Instead of having xhci_handle_event call itself if there are more
outstanding TRBs push the loop logic up one level, into xhci_irq().

xchI_handle_event() does not check for presence of event_ring
anymore since the ring is always allocated. Also, encountering
a TRB that does not belong to OS is a normal condition that
causes us to stop processing and exit ISR and so is not reported
in xhci->error_bitmask.

Also consolidate handling of the event handler busy flag in
xhci_irq().

Reported-by: Matt Evans <matt@ozlabs.org>
Signed-off-by: Dmitry Torokhov <dtor@vmware.com>
---
 drivers/usb/host/xhci-ring.c |   77 +++++++++++++++---------------------------
 1 files changed, 27 insertions(+), 50 deletions(-)

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);
-- 
1.7.4.1

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

[David Laight]

=20
> > -	xhci_handle_event(xhci);
> > +	while (xhci_handle_event(xhci)) {}
> >=20
>=20
> 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

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

[Matt Evans]

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