On Fri, 23 Aug 2013, Ming Lei wrote:
> On Fri, Aug 23, 2013 at 4:39 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > This patch divides ehci-hcd's interrupt handler into a top half and a
> > bottom half, using a tasklet to execute the latter.
> >
> > The conversion is very straightforward. The only subtle point is that
> > we have to ignore interrupts that arrive while the tasklet is running
> > (i.e., from another device on a shared IRQ line).
>
> Not only for another device, the interrupt from ehci itself is still
> delay-handled.
No. The EHCI interrupt-enable register is set to 0 in the IRQ handler,
and it can't generate interrupt requests until the tasklet finishes.
Therefore there won't be any interrupts from the controller to ignore.
> > @@ -691,13 +696,40 @@ EXPORT_SYMBOL_GPL(ehci_setup);
> >
> > /*-------------------------------------------------------------------------*/
> >
> > -static irqreturn_t ehci_irq (struct usb_hcd *hcd)
> > +static irqreturn_t ehci_irq(struct usb_hcd *hcd)
> > {
> > struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> > + u32 status, masked_status;
> > +
> > + if (ehci->irqs_are_masked)
> > + return IRQ_NONE;
> > +
> > + /*
> > + * 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.
> > + */
> > + status = ehci_readl(ehci, &ehci->regs->status);
> > + masked_status = status & (INTR_MASK | STS_FLR);
> > +
> > + /* Shared IRQ? */
> > + if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED))
> > + return IRQ_NONE;
> > +
> > + /* Mask IRQs and let the tasklet do the work */
> > + ehci_writel(ehci, 0, &ehci->regs->intr_enable);
> > + ehci->irqs_are_masked = true;
> > + tasklet_hi_schedule(&ehci->tasklet);
> > + return IRQ_HANDLED;
>
> The irq is handled but not clearing its status, so interrupt controller
> might interrupt CPU continuously and may cause irq flood before
> the status is cleared. Disabling ehci interrupt may not prevent the
> irq flooding since interrupt controller may latch the unhandled(from
> ehci hw view) irq source.
I don't understand. If the interrupt controller has latched something,
the latch will be cleared by the system's IRQ handler. Otherwise _any_
IRQ would cause a flood.
> Secondly, not clearing USBSTS here may delay the next interrupt
> handling for the host controller itself, and the delay depends on the
> tasklet schedule delay.
Yes, of course. The same sort of thing is true for the original driver
-- the host controller can't issue a new interrupt until the handler
for the old one finishes.
Not handling interrupts right away is the price we pay for using a
bottom half. Your tasklet implementation isn't very different.
Although the interrupt handler may realize quickly that a transfer has
finished, there will still be a delay before the URB's completion
handler can run. And the length of that delay will depend on the
tasklet schedule delay.
> Thirdly, fatal error should have been handled immediately inside hard
> interrupt handler.
Why? What difference does it make?
> Finally, tasklet schedule should have been optimized here if the tasklet
> is running on another CPU, otherwise the current CPU may spin on
> the tasklet lock until the same tasklet is completed on another CPU.
That could be added in. But doesn't the core kernel's tasklet
implementation already take care of this? The tasklet_hi_action()
routine uses tasklet_trylock(), so it doesn't spin.
> > @@ -833,10 +863,16 @@ dead:
> >
> > if (bh)
> > ehci_work (ehci);
> > - spin_unlock (&ehci->lock);
> > +
> > + done:
> > + /* Unmask IRQs again */
> > + ehci->irqs_are_masked = false;
>
> The 'irqs_are_masked' should have been set 'false' after clearing
> USBSTS for enabling hard irq handling asap.
There's no point in enabling interrupts before they can be handled. As
long as the tasklet is running, it doesn't do any good to receive more
IRQs.
> Also if the latest value of irqs_are_masked isn't see by ehci_irq
> immediately, it will cause CPU interrupted by extra and unnecessary
> irq signal.
>
> > + smp_wmb(); /* Make sure ehci_irq() sees that assignment */
That's why I put in this memory barrier. It guarantees that
irqs_are_masked _will_ be seen by ehci_irq.
> > + ehci_writel(ehci, ehci->intr_mask, &ehci->regs->intr_enable);
> > +
> > + spin_unlock_irq(&ehci->lock);
> > if (pcd_status)
> > usb_hcd_poll_rh_status(hcd);
> > - return IRQ_HANDLED;
> > }
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
