Re: PCI: MSI interrupts masked using prohibited method

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, 17 Jul 2008, Matthew Wilcox wrote:
> 
> The device generates an MSI interrupt.
> Some magic happens in assembly, and the processor ends up in do_IRQ()
> which calls generic_handle_irq().  Because it's an MSI IRQ,
> handle_edge_irq() is called instead of __do_IRQ().

__do_IRQ() is the old all-in-one handler which is not called on
platforms which have GENERIC_HARDIRQS set. You can safely ignore what
__do_IRQ() does.
 
> There are two opportunities for calling the ->mask() here, one is:
> 
>         if (unlikely((desc->status & (IRQ_INPROGRESS | IRQ_DISABLED)) ||
>                     !desc->action)) {
>                 desc->status |= (IRQ_PENDING | IRQ_MASKED);
>                 mask_ack_irq(desc, irq);
> 
> The other is:
> 
>                 if (unlikely(!action)) {
>                         desc->chip->mask(irq);
> 
> This is all in generic code which knows nothing about any device-specific
> method of masking interrupts from the chip.

Right and it is not supposed to know anything about the hardware
details at all. The per irq setting can provide NOOP functions for all
the mask/mask_ack/unmask things when thats the right way for the
particular irq line.
 
> > Regardless, doesn't __do_IRQ() handle this already anyway?

That's irrelevant. All the interrupts are handled via
irq_desc[irq].handle_irq() when GENERIC_HARDIRQS is set.
 
> This is a good thought, let's follow it through.  What if we simply make
> ->mask a no-op for devices which don't support mask bits?

Yep. You can also use fasteoi_handler, which just calls ->eoi() after
the handler.
 
> MSIs are 'edge triggered' interrupts.  They're sent once and then
> forgotten by the hardware (as opposed to level triggered which will
> continue to be triggered until deasserted).
> 
> The first time through (assuming ->action is non-NULL ...), we won't
> try to mask the irq.  The second time through, IRQ_INPROGRESS will be set,
> so we try to mask_ack_irq().
> 
> How about we simply don't ack the irq at this point?  That should prevent
> it being triggered again, right?
> 
> Working on a patch to do this now ...

You want to use the fasteoi_handler.

Thanks,
	tglx