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.
>
> Not-yet-signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>
> ---
>
>
> drivers/usb/host/ehci-hcd.c | 56 ++++++++++++++++++++++++++++++++++++--------
> drivers/usb/host/ehci.h | 4 +++
> 2 files changed, 51 insertions(+), 9 deletions(-)
>
> Index: usb-3.11/drivers/usb/host/ehci.h
> ===================================================================
> --- usb-3.11.orig/drivers/usb/host/ehci.h
> +++ usb-3.11/drivers/usb/host/ehci.h
> @@ -98,6 +98,10 @@ enum ehci_hrtimer_event {
> #define EHCI_HRTIMER_NO_EVENT 99
>
> struct ehci_hcd { /* one per controller */
> + /* tasklet and IRQ support */
> + struct tasklet_struct tasklet;
> + bool irqs_are_masked;
> +
> /* timing support */
> enum ehci_hrtimer_event next_hrtimer_event;
> unsigned enabled_hrtimer_events;
> Index: usb-3.11/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- usb-3.11.orig/drivers/usb/host/ehci-hcd.c
> +++ usb-3.11/drivers/usb/host/ehci-hcd.c
> @@ -140,6 +140,8 @@ static inline void ehci_set_intr_enable(
> ehci_writel(ehci, mask, &ehci->regs->intr_enable);
> }
>
> +static void ehci_tasklet_routine(unsigned long _ehci);
> +
> #include "ehci-dbg.c"
>
> /*-------------------------------------------------------------------------*/
> @@ -218,6 +220,7 @@ static int ehci_halt (struct ehci_hcd *e
>
> spin_unlock_irq(&ehci->lock);
> synchronize_irq(ehci_to_hcd(ehci)->irq);
> + tasklet_kill(&ehci->tasklet);
>
> return ehci_handshake(ehci, &ehci->regs->status,
> STS_HALT, STS_HALT, 16 * 125);
> @@ -468,6 +471,8 @@ static int ehci_init(struct usb_hcd *hcd
> struct ehci_qh_hw *hw;
>
> spin_lock_init(&ehci->lock);
> + tasklet_init(&ehci->tasklet, ehci_tasklet_routine,
> + (unsigned long) ehci);
>
> /*
> * keep io watchdog by default, those good HCDs could turn off it later
> @@ -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.
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.
Thirdly, fatal error should have been handled immediately inside hard
interrupt handler.
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.
> +}
> +
> +static void ehci_tasklet_routine(unsigned long _ehci)
> +{
> + struct ehci_hcd *ehci = (struct ehci_hcd *) _ehci;
> + struct usb_hcd *hcd = ehci_to_hcd(ehci);
> u32 status, masked_status, pcd_status = 0, cmd;
> int bh;
>
> - spin_lock (&ehci->lock);
> + spin_lock_irq(&ehci->lock);
>
> status = ehci_readl(ehci, &ehci->regs->status);
>
> @@ -713,11 +745,9 @@ static irqreturn_t ehci_irq (struct usb_
> */
> masked_status = status & (INTR_MASK | STS_FLR);
>
> - /* Shared IRQ? */
> - if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
> - spin_unlock(&ehci->lock);
> - return IRQ_NONE;
> - }
> + /* IRQ already handled? */
> + if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED))
> + goto done;
>
> /* clear (just) interrupts */
> ehci_writel(ehci, masked_status, &ehci->regs->status);
> @@ -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.
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 */
> + 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;
> }
>
> /*-------------------------------------------------------------------------*/
> @@ -1093,6 +1129,8 @@ int ehci_suspend(struct usb_hcd *hcd, bo
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> spin_unlock_irq(&ehci->lock);
>
> + synchronize_irq(ehci_to_hcd(ehci)->irq);
> + tasklet_kill(&ehci->tasklet);
> return 0;
> }
> EXPORT_SYMBOL_GPL(ehci_suspend);
>
--
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
