On Wed, Jul 14, 2021 at 10:27 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>
> > 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.
