On Sun, 18 Apr 2010 21:14:12 +0200 (CEST)
Thomas Gleixner <t...@linutronix.de> wrote:
> On Sun, 18 Apr 2010, Guillaume Knispel wrote:
> > Now everything seems to work fine: my device was not previously not
> > interrupting anymore after typically 1 or 2 minutes (because the
> > interrupt signal stays at level low until the device is served, so
>
> So you are having a level interrupt device and the irq line is handled
> by an edge type handler ? Why not configuring the irq line for level
> and in the first place ?
Not really the problem here, it indeed helped me do discover the real
bug (because I would not have seen that an interrupt went missing if the
device have generated another one all by itself soon after that).
I would of course prefer to have this line in level trigger, but the
pic just can't do it for this signal, so I play with masking and
unmasking interrupts on the device side to regenerate edges when needed.
When interrupts are enabled, I do that in the isr, so if it is not
called after a falling edge it won't be anymore in the future.
What is a real bug IMHO is that when in falling edge trigger mode,
the sequence:
- disable_irq(),
- 1 falling edge,
- enable_irq()
doesn't lead to the isr being called just after enable_irq().
This is because:
* __disable_irq basically do (when really disabling)
desc->status |= IRQ_DISABLED;
desc->chip->disable(irq); <= noop or racy see under
* arch/powerpc/sysdev/cpm2_pic.c does not define a disable() in
struct irq_chip cpm2_pic. So default_disable is used, and
it's a noop. For pics where this is not a noop, there can be a race
if the interrupt triggers before the disable(): the flow handler will
still be executed just after desc->lock is unlocked in
disable_irq_nosync(). So it does not really matters whether
disable() is implemented or is a noop.
* When the flow handler is executed after disable_irq_nosync(),
handle_edge_irq() does:
desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
desc->status |= (IRQ_PENDING | IRQ_MASKED);
mask_ack_irq(desc, irq);
The responsibility of triggering the interrupt has now been
transferred from the pic to the kernel (the interrupt has been acked
at pic level), so I would say that if the kernel does not run replay
it later, it will be a bug.
* When __enable_irq() is called, it does:
unsigned int status = desc->status & ~IRQ_DISABLED;
desc->status = status | IRQ_NOPROBE;
check_irq_resend(desc, irq);
* if it's an edge triggered IRQ (and in my case it is)
check_irq_resend() sometimes, if the planets are aligned, resend the
IRQ. Planet alignement is defined by retrigger being provided for the
irq_chip and retrigger succeed, or CONFIG_HARDIRQS_SW_RESEND is
defined (in which case and if retrigger is absent or has failed, the
hardirq is indeed executed in a tasklet -- which might maybe cause
obscure problems?).
My guess is that CONFIG_HARDIRQS_SW_RESEND was meant to be defined on
any platform where a pic can exist and can do edge trigger but does not
implement retrigger or implements a retrigger that can fail (because
otherwise interrupts can be missed).
This seems to be a quite unknown fact that might have been missed by
multiple irq_chips (and could be missed by more in the future), and I
would propose to just unconditionally build the resend_irqs() tasklet
and its scheduling code, and completely remove
CONFIG_HARDIRQS_SW_RESEND, at least if it is sure that executing a flow
handler in a tasklet can never cause obscure problems.
Cheers!
--
Guillaume Knispel
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
