Re: [RFC][PATCH] Cascaded interrupts: a simple solution

[Ingo Molnar <mingo@elte.hu>]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> I figured out that it would be very simple to make that much cleaner 
> and generic by simply defining a new interrupt flag SA_CASCADEIRQ to 
> attach to the cascade irq, and have the handler fetch the interrupts 
> from the slave controller and return them to the core.

> Note the fact that it's looping on the handler until IRQ_NONE is 
> returned, since a cascaded controller might issue only one irq 
> upstream for any combination of downstream interrupts, it must be 
> "polled" until it has no more interrupt to return.

We have solved this problem in the genirq patchset, but in a different 
way. No matter how much i'd like to see a simple solution for a hard 
problem, we believe the SA_CASCADEIRQ method is insufficient, for a 
number of reasons.

The main goal of genirq: to merge the ARM architecture to the generic 
IRQ layer and thus make the generic IRQ layer truly generic. Secondary 
goal: to not disturb any of the current genirq architectures (i.e. stay 
fully compatible). The ARM IRQ layer is exotic in some respects, but it 
is certainly the most advanced IRQ layer in terms of PIC-topology 
handling, given the rich variety of ARM hardware in existence.

Just some stats to back this up: arch/arm*/ has 118 separate PIC 
implementations, and amongst them there are more than 40 that need 
demultiplex handlers. As a comparison, arch/[ppc/powerpc] sports 27 PIC 
implementations, amongst which there are 5 that need demultiplex 
handlers. arch/i386 has 10 PIC implementations and none of them need 
demultiplex handlers [there are minor forms of cascading in x86 
hardware, but none need true irq-vector demultiplexing].

Most architectures modeled their IRQ layer after i386 IRQ layer, and 
this resulted in the first phase of generic IRQ layer being quite 
similar to the i386 layer. But the largest and most versatile Linux IRQ 
subsystem was left out of the original genirq design (done by yours 
truly), and many good bits (amongst them the right way to handle 
cascading/chaining) i missed.

Current status of genirq: it has been part of the -rt tree for more than 
a year, and an earlier version has been sent to lkml once already - 
Thomas has submitted an improved version of it to the ARM lists roughly 
a week ago and it is currently being tested on many ARM boards, with 
good results.

(I suspect you are aware of our genirq efforts, hence did you Cc: Thomas 
and me? If you are aware of it, please address the differences between 
the two approaches and outline why you chose a different solution - 
thanks!)

Most PIC implementations do not need to worry about PIC cascading, and 
neither the SA_CASCADEIRQ nor the genirq approach impacts them. So my 
analysis of your patch only involves true cascaded PICs and the ways to 
support them cleanly.

For cascaded PICs, the main problem with the SA_CASCADEIRQ approach is 
that it assumes that cascading/demultiplexing can be handled via a 
"return irq" method, in an iterative way.

But this does not match the demultiplexing model that happens on the 
majority of ARM boards for example: there a bitmask is read from the 
secondary interrupt controller, and that bitmask might have multiple 
bits set. By returning only one bit [which iteration model your 
interface forces], the other bits can be lost. On some hardware those 
missed bits might be regenerated, but there is PIC hardware where that 
information is permanently lost and we end up losing interrupts.

Such mask-based demultiplexing PICs are not limited to ARM, they occur 
in the PPC world too, for example in arch/ppc/syslib/m82xx_pci.c, 
pq2pci_irq_demux():

 static irqreturn_t
 pq2pci_irq_demux(int irq, void *dev_id, struct pt_regs *regs)
 {
         unsigned long stat, mask, pend;
         int bit;

         for(;;) {
                 stat = *(volatile unsigned long *) PCI_INT_STAT_REG;
                 mask = *(volatile unsigned long *) PCI_INT_MASK_REG;
                 pend = stat & ~mask & 0xf0000000;
                 if (!pend)
                         break;
                 for (bit = 0; pend != 0; ++bit, pend <<= 1) {
                         if (pend & 0x80000000)
                                 __do_IRQ(NR_CPM_INTS + bit, regs);
                 }
         }

         return IRQ_HANDLED;
 }

If PCI_INT_MASK_REG is a read-once register, then the SA_CASCADEIRQ 
method could result in lost interrupts. (i'm not totally sure, but i 
think in this specific case that register is read-once and it also 
involves an auto-ack. In any case, there definitely is ARM hardware 
where this equivalent register is read-once.)

Even if PCI_INT_MASK_REG could be read in a non-destructive way, 
multiple bits would need multiple iterations and multiple (unnecessary) 
passes over the whole bitmask - and they would thus also need 
unnecessary IO cycles to re-fetch the mask itself.

(Depending on how many different interrupt sources a secondary PIC 
connects, and how frequently those are risen, this might or might not be 
a real performance problem. But in any case, the "return irq" method is 
certainly an ugly and unnatural model for such PICs and there is no 
clean way to handle this type of cascading via the 'return irq' method.)

So the solution we took in genirq was to delegate the act of 
demultiplexing into the _demultiplexing handler_, by adopting the ARM
IRQ layer's approach of calling desc_handle_irq(irq, desc, regs). For
example:

 static void locomo_gpio_handler(unsigned int irq, struct irqdesc *desc,
                                 struct pt_regs *regs)
 {
 [...]
                irq = LOCOMO_IRQ_GPIO_START;
                d = irq_desc + LOCOMO_IRQ_GPIO_START;
                for (i = 0; i <= 15; i++, irq++, d++) {
                        if (req & (0x0001 << i)) {
                                desc_handle_irq(irq, d, regs);
                        }
                }
 [...]
 }

desc_handle_irq() does the locking and the calling of the highlevel irq 
handler of the secondary PIC. [which then processes the device IRQ 
handler actions and does any pre/post ACKing logic] There is no impact 
to non-cascading PIC designs in this model either.

Another, conceptual level problem is that (ab-)using the irq handler 
methods to return an actual interrupt number is a layering violation. 
There is not much in common between an IRQ demultiplexer function and an 
interrupt handler function. The act of cascading two PICs _inevitably_ 
means that there is a 1:N relationship between the two PICs, while an 
IRQ handler is normally a 1:1 relationship between a hardware device and 
a PIC. (there are exceptions like shared interrupts, but the norm we are 
designing for is a 1:1 relationship) There are many other fundamental 
differences too. Thus in our genirq work we have separated these two 
concepts.

In terms of patch merging (unless there are some arguments i've 
overlooked), due to the reasons above i'm against merging SA_CASCADEIRQ, 
even if it's relatively simple. It does not solve the demultiplexing 
problem for most of the ARM handlers (nor for the PPC example i cited), 
hence a separate variant has to be implemented anyway which results in 
unnecessary code and concept duplication.

The current PPC/powerpc approach of calling __do_IRQ() might be hacky, 
but it's functional, so neither is there any instant urgency AFAICS. If 
our more complete genirq approach is rejected for whatever reason then 
the SA_CASCADEIRQ patch can still be revisited as a secondary choice.

Nor can i see any big cleanup effect in terms of per-arch PIC code, the 
patch actually adds a bit of code:

 arch/powerpc/platforms/powermac/pic.c |   39 ++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 11 deletions(-)

	Ingo