From b468eef03e7140e011eea19506e53aa68bcf4554 Mon Sep 17 00:00:00 2001 From: David Brownell Date: Tue, 9 Mar 2004 23:31:03 -0800 Subject: [PATCH] [PATCH] USB: fix OHCI list corruption Fix some OHCI TD list corruption issues: - Don't rewrite HC registers holding ED pointers until the HC had a good chance to finish using them. - Don't ever modify ed->hwTailP Adds text describing the different ED states. Adds TEMPORARY hack that may make a "rm_list becomes circular" bug continuable. --- drivers/usb/host/ohci-q.c | 103 +++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 34 deletions(-) diff --git a/drivers/usb/host/ohci-q.c b/drivers/usb/host/ohci-q.c index 2cead07ef548..787c4fd62c6c 100644 --- a/drivers/usb/host/ohci-q.c +++ b/drivers/usb/host/ohci-q.c @@ -187,6 +187,7 @@ static int ed_schedule (struct ohci_hcd *ohci, struct ed *ed) switch (ed->type) { case PIPE_CONTROL: if (ohci->ed_controltail == NULL) { + WARN_ON (ohci->hc_control & OHCI_CTRL_CLE); writel (ed->dma, &ohci->regs->ed_controlhead); } else { ohci->ed_controltail->ed_next = ed; @@ -203,6 +204,7 @@ static int ed_schedule (struct ohci_hcd *ohci, struct ed *ed) case PIPE_BULK: if (ohci->ed_bulktail == NULL) { + WARN_ON (ohci->hc_control & OHCI_CTRL_BLE); writel (ed->dma, &ohci->regs->ed_bulkhead); } else { ohci->ed_bulktail->ed_next = ed; @@ -271,27 +273,56 @@ static void periodic_unlink (struct ohci_hcd *ohci, struct ed *ed) * just the link to the ed is unlinked. * 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 + * (assuming it already started that, which needn't be true). + * + * ED_UNLINK is a transient state: the HC may still see this ED, but soon + * it won't. ED_SKIP means the HC will finish its current transaction, + * but won't start anything new. The TD queue may still grow; device + * drivers don't know about this HCD-internal state. + * + * When the HC can't see the ED, something changes ED_UNLINK to one of: + * + * - ED_OPER: when there's any request queued, the ED gets rescheduled + * immediately. HC should be working on them. + * + * - ED_IDLE: when there's no TD queue. there's no reason for the HC + * to care about this ED; safe to disable the endpoint. + * + * When finish_unlinks() runs later, after SOF interrupt, it will often + * complete one or more URB unlinks before making that state change. */ static void ed_deschedule (struct ohci_hcd *ohci, struct ed *ed) { ed->hwINFO |= ED_SKIP; + wmb (); + ed->state = ED_UNLINK; + /* To deschedule something from the control or bulk list, just + * clear CLE/BLE and wait. There's no safe way to scrub out list + * head/current registers until later, and "later" isn't very + * tightly specified. Figure 6-5 and Section 6.4.2.2 show how + * the HC is reading the ED queues (while we modify them). + * + * For now, ed_schedule() is "later". It might be good paranoia + * to scrub those registers in finish_unlinks(), in case of bugs + * that make the HC try to use them. + */ switch (ed->type) { case PIPE_CONTROL: + /* remove ED from the HC's list: */ if (ed->ed_prev == NULL) { if (!ed->hwNextED) { ohci->hc_control &= ~OHCI_CTRL_CLE; writel (ohci->hc_control, &ohci->regs->control); - writel (0, &ohci->regs->ed_controlcurrent); - // post those pci writes - (void) readl (&ohci->regs->control); - } - writel (le32_to_cpup (&ed->hwNextED), - &ohci->regs->ed_controlhead); + // a readl() later syncs CLE with the HC + } else + writel (le32_to_cpup (&ed->hwNextED), + &ohci->regs->ed_controlhead); } else { ed->ed_prev->ed_next = ed->ed_next; ed->ed_prev->hwNextED = ed->hwNextED; } + /* remove ED from the HCD's list: */ if (ohci->ed_controltail == ed) { ohci->ed_controltail = ed->ed_prev; if (ohci->ed_controltail) @@ -302,20 +333,20 @@ static void ed_deschedule (struct ohci_hcd *ohci, struct ed *ed) break; case PIPE_BULK: + /* remove ED from the HC's list: */ if (ed->ed_prev == NULL) { if (!ed->hwNextED) { ohci->hc_control &= ~OHCI_CTRL_BLE; writel (ohci->hc_control, &ohci->regs->control); - writel (0, &ohci->regs->ed_bulkcurrent); - // post those pci writes - (void) readl (&ohci->regs->control); - } - writel (le32_to_cpup (&ed->hwNextED), - &ohci->regs->ed_bulkhead); + // a readl() later syncs BLE with the HC + } else + writel (le32_to_cpup (&ed->hwNextED), + &ohci->regs->ed_bulkhead); } else { ed->ed_prev->ed_next = ed->ed_next; ed->ed_prev->hwNextED = ed->hwNextED; } + /* remove ED from the HCD's list: */ if (ohci->ed_bulktail == ed) { ohci->ed_bulktail = ed->ed_prev; if (ohci->ed_bulktail) @@ -426,13 +457,24 @@ done: /* request unlinking of an endpoint from an operational HC. * put the ep on the rm_list * real work is done at the next start frame (SF) hardware interrupt + * caller guarantees HCD is running, so hardware access is safe. */ static void start_ed_unlink (struct ohci_hcd *ohci, struct ed *ed) { ed->hwINFO |= ED_DEQUEUE; - ed->state = ED_UNLINK; ed_deschedule (ohci, ed); + /* rm_list is just singly linked, for simplicity */ + ed->ed_next = ohci->ed_rm_list; + ed->ed_prev = 0; + ohci->ed_rm_list = ed; + + /* enable SOF interrupt */ + writel (OHCI_INTR_SF, &ohci->regs->intrstatus); + writel (OHCI_INTR_SF, &ohci->regs->intrenable); + // flush those writes, and get latest HCCA contents + (void) readl (&ohci->regs->control); + /* 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 @@ -440,18 +482,6 @@ static void start_ed_unlink (struct ohci_hcd *ohci, struct ed *ed) */ ed->tick = OHCI_FRAME_NO(ohci->hcca) + 1; - /* rm_list is just singly linked, for simplicity */ - ed->ed_next = ohci->ed_rm_list; - ed->ed_prev = 0; - ohci->ed_rm_list = ed; - - /* enable SOF interrupt */ - if (HCD_IS_RUNNING (ohci->hcd.state)) { - writel (OHCI_INTR_SF, &ohci->regs->intrstatus); - writel (OHCI_INTR_SF, &ohci->regs->intrenable); - // flush those pci writes - (void) readl (&ohci->regs->control); - } } /*-------------------------------------------------------------------------* @@ -794,8 +824,6 @@ ed_halted (struct ohci_hcd *ohci, struct td *td, int cc, struct td *rev) next->next_dl_td = rev; rev = next; - if (ed->hwTailP == cpu_to_le32 (next->td_dma)) - ed->hwTailP = next->hwNextTD; ed->hwHeadP = next->hwNextTD | toggle; } @@ -881,6 +909,13 @@ finish_unlinks (struct ohci_hcd *ohci, u16 tick, struct pt_regs *regs) { struct ed *ed, **last; +ed = ohci->ed_rm_list; +if (ed && ed == ed->ed_next) { + printk ("RM_LIST LOOP! head %p, ed %p, ed->next %p\n", + ohci->ed_rm_list, ed, ed->ed_next); + ed->ed_next = 0; +} + rescan_all: for (last = &ohci->ed_rm_list, ed = *last; ed != NULL; ed = *last) { struct list_head *entry, *tmp; @@ -922,6 +957,10 @@ skip_ed: /* unlink urbs as requested, but rescan the list after * we call a completion since it might have unlinked * another (earlier) urb + * + * When we get here, the HC doesn't see this ed. But it + * must not be rescheduled until all completed URBs have + * been given back to the driver. */ rescan_this: completed = 0; @@ -941,12 +980,7 @@ rescan_this: continue; } - /* patch pointers hc uses ... tail, if we're removing - * an otherwise active td, and whatever td pointer - * points to this td - */ - if (ed->hwTailP == cpu_to_le32 (td->td_dma)) - ed->hwTailP = td->hwNextTD; + /* patch pointer hc uses */ savebits = *prev & ~cpu_to_le32 (TD_MASK); *prev = td->hwNextTD | savebits; @@ -965,9 +999,10 @@ rescan_this: /* ED's now officially unlinked, hc doesn't see */ ed->state = ED_IDLE; - ed->hwINFO &= ~(ED_SKIP | ED_DEQUEUE); ed->hwHeadP &= ~ED_H; ed->hwNextED = 0; + wmb (); + ed->hwINFO &= ~(ED_SKIP | ED_DEQUEUE); /* but if there's work queued, reschedule */ if (!list_empty (&ed->td_list)) { -- 2.39.5