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@xxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
