Messages in this thread Patch in this message | | | From | David Brownell <> | Subject | Re: WARNING: at drivers/usb/host/ehci-hcd.c:287 | Date | Tue, 4 Mar 2008 20:25:30 -0800 |
| |
On Tuesday 04 March 2008, Christian Kujau wrote: > On Tue, 4 Mar 2008, David Brownell wrote:
> > There are other lurking issues though. > > uuh...let's hope they don't wake up?
Nah. See the patch I just attached to
http://bugzilla.kernel.org/show_bug.cgi?id=10078
... appended here for reference.
- Dave
======== CUT HERE The recent EHCI driver update to split the IAA watchdog timer out from the other timers made several things work better, but not everything; and it created a couple new issues in bugzilla. Ergo this patch:
- Handle a should-be-rare SMP race between the watchdog firing and (very late) IAA interrupts;
- Remove a shouldn't-have-been-added WARN_ON() test;
- Guard against one observed OOPS;
- If this watchdog fires during clean HC shutdown, it should act as a NOP instead of interfering with the shutdown sequence;
- Guard against silicon errata hypothesized by some vendors: * IAA status latch broken, but IAAD cleared OK; * IAAD wasn't cleared when IAA status got reported;
The WARN_ON is in bugzilla as 10168; the OOPS as 10078.
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> --- drivers/usb/host/ehci-hcd.c | 60 +++++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 15 deletions(-)
--- g26.orig/drivers/usb/host/ehci-hcd.c 2008-03-04 17:24:22.000000000 -0800 +++ g26/drivers/usb/host/ehci-hcd.c 2008-03-04 20:08:28.000000000 -0800 @@ -257,23 +257,44 @@ static void ehci_iaa_watchdog(unsigned l { struct ehci_hcd *ehci = (struct ehci_hcd *) param; unsigned long flags; - u32 status, cmd; spin_lock_irqsave (&ehci->lock, flags); - WARN_ON(!ehci->reclaim); - status = ehci_readl(ehci, &ehci->regs->status); - cmd = ehci_readl(ehci, &ehci->regs->command); - ehci_dbg(ehci, "IAA watchdog: status %x cmd %x\n", status, cmd); - - /* lost IAA irqs wedge things badly; seen first with a vt8235 */ - if (ehci->reclaim) { - if (status & STS_IAA) { - ehci_vdbg (ehci, "lost IAA\n"); + /* Lost IAA irqs wedge things badly; seen first with a vt8235. + * So we need this watchdog, but must protect it against both + * (a) SMP races against real IAA firing and retriggering, and + * (b) clean HC shutdown, when IAA watchdog was pending. + */ + if (ehci->reclaim + && !timer_pending(&ehci->iaa_watchdog) + && HC_IS_RUNNING(ehci_to_hcd(ehci)->state)) { + u32 cmd, status; + + /* If we get here, IAA is *REALLY* late. It's barely + * conceivable that the system is so busy that CMD_IAAD + * is still legitimately set, so let's be sure it's + * clear before we read STS_IAA. (The HC should clear + * CMD_IAAD when it sets STS_IAA.) + */ + cmd = ehci_readl(ehci, &ehci->regs->command); + if (cmd & CMD_IAAD) + ehci_writel(ehci, cmd & ~CMD_IAAD, + &ehci->regs->command); + + /* If IAA is set here it either legitimately triggered + * before we cleared IAAD above (but _way_ late, so we'll + * still count it as lost) ... or a silicon erratum: + * - VIA seems to set IAA without triggering the IRQ; + * - IAAD potentially cleared without setting IAA. + */ + status = ehci_readl(ehci, &ehci->regs->status); + if ((status & STS_IAA) || !(cmd & CMD_IAAD)) { COUNT (ehci->stats.lost_iaa); ehci_writel(ehci, STS_IAA, &ehci->regs->status); } - ehci_writel(ehci, cmd & ~CMD_IAAD, &ehci->regs->command); + + ehci_vdbg(ehci, "IAA watchdog: status %x cmd %x\n", + status, cmd); end_unlink_async(ehci); } @@ -607,7 +628,7 @@ static int ehci_run (struct usb_hcd *hcd static irqreturn_t ehci_irq (struct usb_hcd *hcd) { struct ehci_hcd *ehci = hcd_to_ehci (hcd); - u32 status, pcd_status = 0; + u32 status, pcd_status = 0, cmd; int bh; spin_lock (&ehci->lock); @@ -628,7 +649,7 @@ static irqreturn_t ehci_irq (struct usb_ /* clear (just) interrupts */ ehci_writel(ehci, status, &ehci->regs->status); - ehci_readl(ehci, &ehci->regs->command); /* unblock posted write */ + cmd = ehci_readl(ehci, &ehci->regs->command); bh = 0; #ifdef VERBOSE_DEBUG @@ -649,8 +670,17 @@ static irqreturn_t ehci_irq (struct usb_ /* complete the unlinking of some qh [4.15.2.3] */ if (status & STS_IAA) { - COUNT (ehci->stats.reclaim); - end_unlink_async(ehci); + /* guard against (alleged) silicon errata */ + if (cmd & CMD_IAAD) { + ehci_writel(ehci, cmd & ~CMD_IAAD, + &ehci->regs->command); + ehci_dbg(ehci, "IAA with IAAD still set?\n"); + } + if (ehci->reclaim) { + COUNT(ehci->stats.reclaim); + end_unlink_async(ehci); + } else + ehci_dbg(ehci, "IAA with nothing to reclaim?\n"); } /* remote wakeup [4.3.1] */
| |