Re: [PATCH] usb: ehci: Prevent missed ehci interrupts with edge-triggered MSI

[David Jeffery <djeffery@redhat.com>]

On Wed, Jul 14, 2021 at 10:27 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Jul 14, 2021 at 10:03:09AM -0400, David Jeffery wrote:
>
> Thanks, I have few minor comments, after addressing them feel free to add
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> > When MSI is used by the ehci driver, it can cause interrupts to be lost which
>
> ehci -> EHCI everywhere?
>

Are you asking for a capitalization change in the text or asking what
all is affected by the bug?

> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> > index 36f5bf6a0752..2283205d4b40 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> > +++ b/drivers/usb/host/ehci-hcd.c
> > @@ -704,14 +704,18 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
> >  {
> >       struct ehci_hcd         *ehci = hcd_to_ehci (hcd);
> >       u32                     status, masked_status, pcd_status = 0, cmd;
> > +     u32                     current_status;
>
> Perhaps
>
>         u32                     status, current_status, masked_status, pcd_status = 0;
>         u32                     cmd;
>
> ?
>

Is this a style preference?  I can change it and just did it in a way
to minimize line changes.

>
> >       int                     bh;
> >
> >       spin_lock(&ehci->lock);
> >
> > -     status = ehci_readl(ehci, &ehci->regs->status);
> > +     status = 0;
> >
> > +     current_status = ehci_readl(ehci, &ehci->regs->status);
> > +restart:
> > +     status |= current_status;
> >       /* e.g. cardbus physical eject */
> > -     if (status == ~(u32) 0) {
> > +     if (current_status == ~(u32) 0) {
> >               ehci_dbg (ehci, "device removed\n");
> >               goto dead;
> >       }
> > @@ -720,7 +724,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
> >        * We don't use STS_FLR, but some controllers don't like it to
> >        * remain on, so mask it out along with the other status bits.
> >        */
> > -     masked_status = status & (INTR_MASK | STS_FLR);
> > +     masked_status = current_status & (INTR_MASK | STS_FLR);
> >
> >       /* Shared IRQ? */
> >       if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
> > @@ -730,6 +734,12 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
> >
> >       /* clear (just) interrupts */
> >       ehci_writel(ehci, masked_status, &ehci->regs->status);
> > +
> > +     /* for edge interrupts, don't race with an interrupt bit being raised */
>
> for -> For
>

I'll update it.