lkml.org 
[lkml]   [2008]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: WARNING: at drivers/usb/host/ehci-hcd.c:287
Date
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] */

\
 
 \ /
  Last update: 2008-03-05 05:29    [W:1.549 / U:0.548 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site