On Sat, Aug 24, 2013 at 12:06 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Fri, 23 Aug 2013, Ming Lei wrote:
>
>> On Fri, Aug 23, 2013 at 4:39 AM, Alan Stern <st...@rowland.harvard.edu>
>> 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.
But the current interrupt from EHCI isn't be acked(USBSTS isn't cleared),
so EHCI HW don't know the irq has been handled, then it is reasonable
that the EHCI interrupt still interrupts CPU.
EHCI spec(4.15) also states the point clearly:
the host controller is not required to de-assert a currently active
interrupt condition when software sets the interrupt enables (in the
USBINR register, see Section 2.3.3) to a zero. The only reliable
method software should use for acknowledging an interrupt is by
transitioning the appropriate status bits in the USBSTS register
(Section 2.3.2) from a one to a zero.
>
>> > @@ -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.
As I said above, EHCI HW don't know the irq has been handled.
>
>> 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.
In my tasklet implementation, the next EHCI interrupt can be handled
after the current hard interrupt is handled, but in this patch the EHCI
hard irq can't be handled until the tasklet is handled, right?
>> Thirdly, fatal error should have been handled immediately inside hard
>> interrupt handler.
>
> Why? What difference does it make?
At least, EHCI HW generates the fatal irq ASAP, "This interrupt is not
delayed to the next interrupt threshold."(4.15.2.4), so it is reasonable for
software to handle it asap.
>
>> 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.
OK, but extra tasklet schedule delay might be caused.
>
>> > @@ -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.
Why doesn't any good to receive more IRWQs? And there should be
other interrupts(non transfer complete irq, such as port change, fatal
error, ...)
comes.
>
>> 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.
memory barrier only orders memory load/store, and it can't
make sure CPU always sees the latest value, also what is
the two memory load/store to order with the smp_wmb()? And
where is the smp_rmb() pair?
The same problem might exist for reading/writing 'irqs_are_masked'
in ehci_irq: the flag is set in ehci_irq() on CPU0, but it might not be
seen in another ehci_irq() on CPU1.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
