This patch:
- Fixes a longstanding urb unlink race, by switching to a single queue
for EDs being unlinked. The previous two-queue scheme was sensitive to
IRQ latencies: one extra millisecond would make it use the wrong queue.
This updated scheme should handle latencies of up to 32K microseconds
(Cthulu forfend :) and slightly shrinks object code size.
- Related (mostly) cleanup. Some functions and one ED field renamed, ED
layout is a smidgeon more compact (even given more data), driver version
string doesn't reflect CVS, debug message only comes out in verbose mode.
Please merge to Linus' tree. Some of this should probably get backported
to the 2.4 tree, I suspect this as the root cause of a bug someone recently
ran into ... as well as various seemingly random flakes folk have reported.
(Fix to a related ED unlink problem should build on this.)
- Dave
--- ./drivers/usb-dist/host/ohci.h Sun Apr 14 12:18:56 2002
+++ ./drivers/usb/host/ohci.h Sat Jun 1 11:24:36 2002
@@ -27,22 +27,29 @@
__u32 hwNextED; /* next ED in list */
/* rest are purely for the driver's use */
- struct ed *ed_prev;
- __u8 int_period;
- __u8 int_branch;
- __u8 int_load;
- __u8 int_interval;
- __u8 state; // ED_{NEW,UNLINK,OPER}
+ dma_addr_t dma; /* addr of ED */
+ struct ed *ed_prev; /* for non-interrupt EDs */
+
+ u8 type; /* PIPE_{BULK,...} */
+ u8 interval; /* interrupt, isochronous */
+ union {
+ struct intr_info { /* interrupt */
+ u8 int_period;
+ u8 int_branch;
+ u8 int_load;
+ };
+ u16 last_iso; /* isochronous */
+ };
+
+ u8 state; /* ED_{NEW,UNLINK,OPER} */
#define ED_NEW 0x00 /* unused, no dummy td */
#define ED_UNLINK 0x01 /* dummy td, maybe linked to hc */
#define ED_OPER 0x02 /* dummy td, _is_ linked to hc */
#define ED_URB_DEL 0x08 /* for unlinking; masked in */
- __u8 type;
- __u16 last_iso;
+ /* HC may see EDs on rm_list until next frame (frame_no == tick) */
+ u16 tick;
struct ed *ed_rm_list;
-
- dma_addr_t dma; /* addr of ED */
} __attribute__ ((aligned(16)));
#define ED_MASK ((u32)~0x0f) /* strip hw status in low addr bits */
@@ -335,7 +342,7 @@
struct ohci_hcca *hcca;
dma_addr_t hcca_dma;
- struct ed *ed_rm_list [2]; /* to be removed */
+ struct ed *ed_rm_list; /* to be removed */
struct ed *ed_bulktail; /* last in bulk list */
struct ed *ed_controltail; /* last in ctrl list */
--- ./drivers/usb-dist/host/ohci-hcd.c Sun May 26 15:55:03 2002
+++ ./drivers/usb/host/ohci-hcd.c Sat Jun 1 12:50:45 2002
@@ -12,6 +12,9 @@
*
* History:
*
+ * 2002/06/01 remember frame when HC won't see EDs any more; use that info
+ * to fix urb unlink races caused by interrupt latency assumptions;
+ * minor ED field and function naming updates
* 2002/01/18 package as a patch for 2.5.3; this should match the
* 2.4.17 kernel modulo some bugs being fixed.
*
@@ -106,7 +109,7 @@
* - lots more testing!!
*/
-#define DRIVER_VERSION "$Revision: 1.9 $"
+#define DRIVER_VERSION "2002-Jun-01"
#define DRIVER_AUTHOR "Roman Weissgaerber <[EMAIL PROTECTED]>, David Brownell"
#define DRIVER_DESC "USB 1.1 'Open' Host Controller (OHCI) Driver"
@@ -287,7 +290,7 @@
}
urb_priv->state = URB_DEL;
- ed_unlink (urb->dev, urb_priv->ed);
+ start_urb_unlink (ohci, urb_priv->ed);
spin_unlock_irqrestore (&ohci->lock, flags);
} else {
/*
@@ -508,16 +511,15 @@
/* could track INTR_SO to reduce available PCI/... bandwidth */
- // FIXME: this assumes SOF (1/ms) interrupts don't get lost...
- if (ints & OHCI_INTR_SF) {
- unsigned int frame = le16_to_cpu (ohci->hcca->frame_no) & 1;
+ /* handle any pending URB/ED unlinks, leaving INTR_SF enabled
+ * when there's still unlinking to be done (next frame).
+ */
+ spin_lock (&ohci->lock);
+ if (ohci->ed_rm_list)
+ finish_unlinks (ohci, le16_to_cpu (ohci->hcca->frame_no));
+ if ((ints & OHCI_INTR_SF) != 0 && !ohci->ed_rm_list)
writel (OHCI_INTR_SF, ®s->intrdisable);
- if (ohci->ed_rm_list [!frame] != NULL) {
- dl_del_list (ohci, !frame);
- }
- if (ohci->ed_rm_list [frame] != NULL)
- writel (OHCI_INTR_SF, ®s->intrenable);
- }
+ spin_unlock (&ohci->lock);
writel (ints, ®s->intrstatus);
writel (OHCI_INTR_MIE, ®s->intrenable);
@@ -719,8 +721,7 @@
for (i = 0; i < NUM_INTS; i++) ohci->hcca->int_table [i] = 0;
/* no EDs to remove */
- ohci->ed_rm_list [0] = NULL;
- ohci->ed_rm_list [1] = NULL;
+ ohci->ed_rm_list = NULL;
/* empty control and bulk lists */
ohci->ed_isotail = NULL;
@@ -802,7 +803,7 @@
ohci->disabled = 0;
ohci->sleeping = 0;
ohci->hc_control = OHCI_CONTROL_INIT | OHCI_USB_OPER;
- if (!ohci->ed_rm_list [0] && !ohci->ed_rm_list [1]) {
+ if (!ohci->ed_rm_list) {
if (ohci->ed_controltail)
ohci->hc_control |= OHCI_CTRL_CLE;
if (ohci->ed_bulktail)
--- ./drivers/usb-dist/host/ohci-q.c Fri May 3 05:59:03 2002
+++ ./drivers/usb/host/ohci-q.c Sat Jun 1 13:27:21 2002
@@ -208,8 +208,7 @@
}
ed->ed_prev = ohci->ed_controltail;
if (!ohci->ed_controltail
- && !ohci->ed_rm_list [0]
- && !ohci->ed_rm_list [1]
+ && !ohci->ed_rm_list
&& !ohci->sleeping
) {
ohci->hc_control |= OHCI_CTRL_CLE;
@@ -227,8 +226,7 @@
}
ed->ed_prev = ohci->ed_bulktail;
if (!ohci->ed_bulktail
- && !ohci->ed_rm_list [0]
- && !ohci->ed_rm_list [1]
+ && !ohci->ed_rm_list
&& !ohci->sleeping
) {
ohci->hc_control |= OHCI_CTRL_BLE;
@@ -240,16 +238,16 @@
case PIPE_INTERRUPT:
load = ed->int_load;
interval = ep_2_n_interval (ed->int_period);
- ed->int_interval = interval;
+ ed->interval = interval;
int_branch = ep_int_balance (ohci, interval, load);
ed->int_branch = int_branch;
for (i = 0; i < ep_rev (6, interval); i += inter) {
inter = 1;
for (ed_p = & (ohci->hcca->int_table [ep_rev (5, i) +
int_branch]);
- (*ed_p != 0) && ((dma_to_ed (ohci, le32_to_cpup
(ed_p)))->int_interval >= interval);
+ (*ed_p != 0) && ((dma_to_ed (ohci, le32_to_cpup
+(ed_p)))->interval >= interval);
ed_p = & ((dma_to_ed (ohci, le32_to_cpup
(ed_p)))->hwNextED))
- inter = ep_rev (6, (dma_to_ed (ohci,
le32_to_cpup (ed_p)))->int_interval);
+ inter = ep_rev (6, (dma_to_ed (ohci,
+le32_to_cpup (ed_p)))->interval);
ed->hwNextED = *ed_p;
*ed_p = cpu_to_le32 (ed->dma);
}
@@ -260,7 +258,7 @@
case PIPE_ISOCHRONOUS:
ed->hwNextED = 0;
- ed->int_interval = 1;
+ ed->interval = 1;
if (ohci->ed_isotail != NULL) {
ohci->ed_isotail->hwNextED = cpu_to_le32 (ed->dma);
ed->ed_prev = ohci->ed_isotail;
@@ -270,7 +268,7 @@
for (ed_p = & (ohci->hcca->int_table [ep_rev (5, i)]);
*ed_p != 0;
ed_p = & ((dma_to_ed (ohci, le32_to_cpup
(ed_p)))->hwNextED))
- inter = ep_rev (6, (dma_to_ed (ohci,
le32_to_cpup (ed_p)))->int_interval);
+ inter = ep_rev (6, (dma_to_ed (ohci,
+le32_to_cpup (ed_p)))->interval);
*ed_p = cpu_to_le32 (ed->dma);
}
ed->ed_prev = NULL;
@@ -311,7 +309,7 @@
* the link from the ed still points to another operational ed or 0
* so the HC can eventually finish the processing of the unlinked ed
*/
-static int ep_unlink (struct ohci_hcd *ohci, struct ed *ed)
+static int start_ed_unlink (struct ohci_hcd *ohci, struct ed *ed)
{
int i;
@@ -357,8 +355,8 @@
break;
case PIPE_INTERRUPT:
- periodic_unlink (ohci, ed, ed->int_branch, ed->int_interval);
- for (i = ed->int_branch; i < NUM_INTS; i += ed->int_interval)
+ periodic_unlink (ohci, ed, ed->int_branch, ed->interval);
+ for (i = ed->int_branch; i < NUM_INTS; i += ed->interval)
ohci->ohci_int_load [i] -= ed->int_load;
#ifdef OHCI_VERBOSE_DEBUG
ohci_dump_periodic (ohci, "UNLINK_INT");
@@ -384,6 +382,10 @@
/* FIXME ED's "unlink" state is indeterminate;
* the HC might still be caching it (till SOF).
+ * - use ed_rm_list and finish_unlinks(), adding some state that
+ * prevents clobbering hw linkage before the appropriate SOF
+ * - a speedup: when only one urb is queued on the ed, save 1msec
+ * by making start_urb_unlink() use this routine to deschedule.
*/
ed->state = ED_UNLINK;
return 0;
@@ -478,11 +480,8 @@
* put the ep on the rm_list and stop the bulk or ctrl list
* real work is done at the next start frame (SF) hardware interrupt
*/
-static void ed_unlink (struct usb_device *usb_dev, struct ed *ed)
+static void start_urb_unlink (struct ohci_hcd *ohci, struct ed *ed)
{
- unsigned int frame;
- struct ohci_hcd *ohci = hcd_to_ohci (usb_dev->bus->hcpriv);
-
/* already pending? */
if (ed->state & ED_URB_DEL)
return;
@@ -503,9 +502,15 @@
break;
}
- frame = le16_to_cpu (ohci->hcca->frame_no) & 0x1;
- ed->ed_rm_list = ohci->ed_rm_list [frame];
- ohci->ed_rm_list [frame] = ed;
+ /* SF interrupt might get delayed; record the frame counter value that
+ * indicates when the HC isn't looking at it, so concurrent unlinks
+ * behave. frame_no wraps every 2^16 msec, and changes right before
+ * SF is triggered.
+ */
+ ed->tick = le16_to_cpu (ohci->hcca->frame_no) + 1;
+
+ ed->ed_rm_list = ohci->ed_rm_list;
+ ohci->ed_rm_list = ed;
/* enable SOF interrupt */
if (!ohci->sleeping) {
@@ -816,10 +821,12 @@
if (td_list->ed->hwHeadP & ED_H) {
if (urb_priv && ((td_list->index + 1)
< urb_priv->length)) {
+#ifdef OHCI_VERBOSE_DEBUG
dbg ("urb %p TD %d of %d, patch ED",
td_list->urb,
1 + td_list->index,
urb_priv->length);
+#endif
td_list->ed->hwHeadP =
(urb_priv->td [urb_priv->length - 1]->hwNextTD
& __constant_cpu_to_le32 (TD_MASK))
@@ -841,27 +848,37 @@
/*-------------------------------------------------------------------------*/
-/* there are some pending requests to unlink
- * - some URBs/TDs if urb_priv->state == URB_DEL
- */
-static void dl_del_list (struct ohci_hcd *ohci, unsigned int frame)
+/* wrap-aware logic stolen from <linux/jiffies.h> */
+#define tick_before(t1,t2) ((((s16)(t1))-((s16)(t2))) < 0)
+
+/* there are some urbs/eds to unlink; called in_irq(), with HCD locked */
+static void finish_unlinks (struct ohci_hcd *ohci, u16 tick)
{
- unsigned long flags;
- struct ed *ed;
- __u32 edINFO;
- __u32 tdINFO;
- struct td *td = NULL, *td_next = NULL,
- *tdHeadP = NULL, *tdTailP;
- __u32 *td_p;
+ struct ed *ed, **last;
int ctrl = 0, bulk = 0;
- spin_lock_irqsave (&ohci->lock, flags);
-
- for (ed = ohci->ed_rm_list [frame]; ed != NULL; ed = ed->ed_rm_list) {
+ for (last = &ohci->ed_rm_list, ed = *last; ed != NULL; ed = *last) {
+ struct td *td, *td_next, *tdHeadP, *tdTailP;
+ u32 *td_p;
+ int unlinked;
+
+ /* only take off EDs that the HC isn't using, accounting for
+ * frame counter wraps. completion callbacks might prepend
+ * EDs to the list, they'll be checked next irq.
+ */
+ if (tick_before (tick, ed->tick)) {
+ last = &ed->ed_rm_list;
+ continue;
+ }
+ *last = ed->ed_rm_list;
+ ed->ed_rm_list = 0;
+ unlinked = 0;
+ /* unlink urbs from first one requested to queue end;
+ * leave earlier urbs alone
+ */
tdTailP = dma_to_td (ohci, le32_to_cpup (&ed->hwTailP));
tdHeadP = dma_to_td (ohci, le32_to_cpup (&ed->hwHeadP));
- edINFO = le32_to_cpup (&ed->hwINFO);
td_p = &ed->hwHeadP;
for (td = tdHeadP; td != tdTailP; td = td_next) {
@@ -870,8 +887,11 @@
td_next = dma_to_td (ohci,
le32_to_cpup (&td->hwNextTD));
- if ((urb_priv->state == URB_DEL)) {
- tdINFO = le32_to_cpup (&td->hwINFO);
+ if (unlinked || (urb_priv->state == URB_DEL)) {
+ u32 tdINFO = le32_to_cpup (&td->hwINFO);
+
+ unlinked = 1;
+
/* HC may have partly processed this TD */
if (TD_CC_GET (tdINFO) < 0xE)
td_done (urb, td);
@@ -880,22 +900,32 @@
/* URB is done; clean up */
if (++ (urb_priv->td_cnt) == urb_priv->length) {
- spin_unlock_irqrestore (&ohci->lock,
- flags);
+ if (urb->status == -EINPROGRESS)
+ urb->status = -ECONNRESET;
+ spin_unlock (&ohci->lock);
finish_urb (ohci, urb);
- spin_lock_irqsave (&ohci->lock, flags);
+ spin_lock (&ohci->lock);
}
} else {
td_p = &td->hwNextTD;
}
}
+ /* FIXME actually want four cases here:
+ * (a) finishing URB unlink
+ * [a1] no URBs queued, so start ED unlink
+ * [a2] some (earlier) URBs still linked, re-enable
+ * (b) finishing ED unlink
+ * [b1] no URBs queued, ED is truly idle now
+ * [b2] URBs now queued, link ED back into schedule
+ * right now we only have (a)
+ */
ed->state &= ~ED_URB_DEL;
tdHeadP = dma_to_td (ohci, le32_to_cpup (&ed->hwHeadP));
if (tdHeadP == tdTailP) {
if (ed->state == ED_OPER)
- ep_unlink (ohci, ed);
+ start_ed_unlink (ohci, ed);
td_free (ohci, tdTailP);
ed->hwINFO = ED_SKIP;
ed->state = ED_NEW;
@@ -918,7 +948,7 @@
writel (0, &ohci->regs->ed_controlcurrent);
if (bulk) /* reset bulk list */
writel (0, &ohci->regs->ed_bulkcurrent);
- if (!ohci->ed_rm_list [!frame]) {
+ if (!ohci->ed_rm_list) {
if (ohci->ed_controltail)
ohci->hc_control |= OHCI_CTRL_CLE;
if (ohci->ed_bulktail)
@@ -926,9 +956,6 @@
writel (ohci->hc_control, &ohci->regs->control);
}
}
-
- ohci->ed_rm_list [frame] = NULL;
- spin_unlock_irqrestore (&ohci->lock, flags);
}
@@ -939,7 +966,7 @@
* Process normal completions (error or success) and clean the schedules.
*
* This is the main path for handing urbs back to drivers. The only other
- * path is dl_del_list(), which unlinks URBs by scanning EDs, instead of
+ * path is finish_unlinks(), which unlinks URBs using ed_rm_list, instead of
* scanning the (re-reversed) donelist as this does.
*/
static void dl_done_list (struct ohci_hcd *ohci, struct td *td)
@@ -960,7 +987,7 @@
/* If all this urb's TDs are done, call complete().
* Interrupt transfers are the only special case:
* they're reissued, until "deleted" by usb_unlink_urb
- * (real work done in a SOF intr, by dl_del_list).
+ * (real work done in a SOF intr, by finish_unlinks).
*/
if (urb_priv->td_cnt == urb_priv->length) {
int resubmit;
@@ -980,7 +1007,7 @@
if ((ed->hwHeadP & __constant_cpu_to_le32 (TD_MASK))
== ed->hwTailP
&& (ed->state == ED_OPER))
- ep_unlink (ohci, ed);
+ start_ed_unlink (ohci, ed);
td = td_next;
}
spin_unlock_irqrestore (&ohci->lock, flags);
