# This is a BitKeeper generated patch for the following project: # Project Name: Linux kernel tree # This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas: # ChangeSet 1.454 -> 1.455 # drivers/usb/host/ohci-hcd.c 1.11 -> 1.12 # drivers/usb/host/ohci-q.c 1.7 -> 1.8 # drivers/usb/host/ohci.h 1.4 -> 1.5 # # The following is the BitKeeper ChangeSet Log # -------------------------------------------- # 02/06/05 david-b@pacbell.net 1.455 # [PATCH] ohci-hcd, fix urb unlink races # # 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. # -------------------------------------------- # diff -Nru a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c --- a/drivers/usb/host/ohci-hcd.c Wed Jun 5 12:28:53 2002 +++ b/drivers/usb/host/ohci-hcd.c Wed Jun 5 12:28:53 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 , 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) diff -Nru a/drivers/usb/host/ohci-q.c b/drivers/usb/host/ohci-q.c --- a/drivers/usb/host/ohci-q.c Wed Jun 5 12:28:53 2002 +++ b/drivers/usb/host/ohci-q.c Wed Jun 5 12:28:53 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 */ +#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); diff -Nru a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h --- a/drivers/usb/host/ohci.h Wed Jun 5 12:28:53 2002 +++ b/drivers/usb/host/ohci.h Wed Jun 5 12:28:53 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 */