Re: USB OOPS 2.6.25-rc2-git1

[David Brownell <david-b@pacbell.net>]

On Thursday 21 February 2008, Alan Stern wrote:
> > ========= CUT HERE
> > Modify EHCI irq handling on the theory that at least some of the
> > "lost" IRQs are caused by goofage between multiple lowlevel IRQ
> > acking mechanisms:  try rescanning before we exit the handler, in
> > case the EHCI-internal ack (by clearing the irq status) doesn't
> > always suffice for IRQs triggered nearly back-to-back.
> > 
> > ---
> >  drivers/usb/host/ehci-hcd.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > --- g26.orig/drivers/usb/host/ehci-hcd.c      2008-02-20 13:26:00.000000000 -0800
> > +++ g26/drivers/usb/host/ehci-hcd.c   2008-02-20 13:54:37.000000000 -0800
> > @@ -638,6 +638,8 @@ static irqreturn_t ehci_irq (struct usb_
> >               return IRQ_NONE;
> >       }
> >  
> > +retrigger:
> > +
> >       /* clear (just) interrupts */
> >       ehci_writel(ehci, status, &ehci->regs->status);
> >       cmd = ehci_readl(ehci, &ehci->regs->command);
> > @@ -725,6 +727,12 @@ dead:
> >  
> >       if (bh)
> >               ehci_work (ehci);
> > +
> > +     status = ehci_readl(ehci, &ehci->regs->status);
> > +     status &= INTR_MASK;
> > +     if (status)
> > +             goto retrigger;
> > +
> >       spin_unlock (&ehci->lock);
> >       if (pcd_status & STS_PCD)
> >               usb_hcd_poll_rh_status(hcd);
> 
> There's one little problem here.  As a result of this change, the line 
> where pcd_status gets set (not shown in this patch) needs to be changed 
> to:
> 
>                 pcd_status |= (status & STS_PCD);

Actually, no change is needed.  It's initialized to zero, then
set to "status" given "if (status & STS_PCD)", and never cleared.
So if it's ever set, it stays set...

> 
> Then the test shown above can be simplified to:
> 
>         if (pcd_status)

True with the current code too ...