[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Will Deacon]

Currently, the gic uses handle_level_irq for handling SPIs (Shared
Peripheral Interrupts), requiring active interrupts to be masked at
the distributor level during IRQ handling.

On a virtualised system, only the CPU interfaces are virtualised in
hardware. Accesses to the distributor must be trapped by the hypervisor,
adding latency to the critical interrupt path in Linux.

This patch modifies the GIC code to use handle_fasteoi_irq for handling
interrupts, which only requires us to signal EOI to the CPU interface
when handling is complete. Cascaded IRQ handling is also updated so that
EOI is signalled after handling.

Note that commit 846afbd1 ("GIC: Dont disable INT in ack callback") broke
cascading interrupts by forgetting to add IRQ masking. This is no longer
an issue because the unmask call is now unnecessary.

Tested on Versatile Express and Realview EB (1176 w/ cascaded GICs).

Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Reposting because I forgot the CCs the first time round...

 arch/arm/common/gic.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 2243772..9def30b 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -71,13 +71,6 @@ static inline unsigned int gic_irq(struct irq_data *d)
 /*
  * Routines to acknowledge, disable and enable interrupts
  */
-static void gic_ack_irq(struct irq_data *d)
-{
-	spin_lock(&irq_controller_lock);
-	writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
-	spin_unlock(&irq_controller_lock);
-}
-
 static void gic_mask_irq(struct irq_data *d)
 {
 	u32 mask = 1 << (d->irq % 32);
@@ -96,6 +89,11 @@ static void gic_unmask_irq(struct irq_data *d)
 	spin_unlock(&irq_controller_lock);
 }
 
+static void gic_eoi_irq(struct irq_data *d)
+{
+	writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+}
+
 static int gic_set_type(struct irq_data *d, unsigned int type)
 {
 	void __iomem *base = gic_dist_base(d);
@@ -174,9 +172,6 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 	unsigned int cascade_irq, gic_irq;
 	unsigned long status;
 
-	/* primary controller ack'ing */
-	chip->irq_ack(&desc->irq_data);
-
 	spin_lock(&irq_controller_lock);
 	status = readl(chip_data->cpu_base + GIC_CPU_INTACK);
 	spin_unlock(&irq_controller_lock);
@@ -192,15 +187,15 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 		generic_handle_irq(cascade_irq);
 
  out:
-	/* primary controller unmasking */
-	chip->irq_unmask(&desc->irq_data);
+	/* primary controller EOI */
+	chip->irq_eoi(&desc->irq_data);
 }
 
 static struct irq_chip gic_chip = {
 	.name			= "GIC",
-	.irq_ack		= gic_ack_irq,
 	.irq_mask		= gic_mask_irq,
 	.irq_unmask		= gic_unmask_irq,
+	.irq_eoi		= gic_eoi_irq,
 	.irq_set_type		= gic_set_type,
 #ifdef CONFIG_SMP
 	.irq_set_affinity	= gic_set_cpu,
@@ -275,7 +270,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	for (i = irq_start; i < irq_limit; i++) {
 		set_irq_chip(i, &gic_chip);
 		set_irq_chip_data(i, gic);
-		set_irq_handler(i, handle_level_irq);
+		set_irq_handler(i, handle_fasteoi_irq);
 		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
 	}
 
-- 
1.7.0.4

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Rabin Vincent]

On Mon, Feb 14, 2011 at 20:56, Will Deacon <will.deacon@arm.com> wrote:
> Currently, the gic uses handle_level_irq for handling SPIs (Shared
> Peripheral Interrupts), requiring active interrupts to be masked at
> the distributor level during IRQ handling.
>
> On a virtualised system, only the CPU interfaces are virtualised in
> hardware. Accesses to the distributor must be trapped by the hypervisor,
> adding latency to the critical interrupt path in Linux.
>
> This patch modifies the GIC code to use handle_fasteoi_irq for handling
> interrupts, which only requires us to signal EOI to the CPU interface
> when handling is complete. Cascaded IRQ handling is also updated so that
> EOI is signalled after handling.

Several of the platforms using the GIC also have GPIO code which uses
set_irq_chained_handler().  I think you will have to modify all of
these to call irq_eoi() appropriately and not the other functions.
Some of these will also likely be used with other interrupt handlers
than the GIC, though.

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Will Deacon]

Hi Rabin,

> On Mon, Feb 14, 2011 at 20:56, Will Deacon <will.deacon@arm.com> wrote:
> > Currently, the gic uses handle_level_irq for handling SPIs (Shared
> > Peripheral Interrupts), requiring active interrupts to be masked at
> > the distributor level during IRQ handling.
> >
> > On a virtualised system, only the CPU interfaces are virtualised in
> > hardware. Accesses to the distributor must be trapped by the hypervisor,
> > adding latency to the critical interrupt path in Linux.
> >
> > This patch modifies the GIC code to use handle_fasteoi_irq for handling
> > interrupts, which only requires us to signal EOI to the CPU interface
> > when handling is complete. Cascaded IRQ handling is also updated so that
> > EOI is signalled after handling.
> 
> Several of the platforms using the GIC also have GPIO code which uses
> set_irq_chained_handler().  I think you will have to modify all of
> these to call irq_eoi() appropriately and not the other functions.
> Some of these will also likely be used with other interrupt handlers
> than the GIC, though.

Hmm, I had a quick look at some platforms that do this (mach-dove and 
plat-spear) and I don't see what the problem is. They use their own irq_chip
structures, with their own function pointers, so this doesn't seem to relate
to the GIC at all. What am I missing?!

Cheers,

Will

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Rabin Vincent]

On Wed, Feb 16, 2011 at 18:39, Will Deacon <will.deacon@arm.com> wrote:
>> On Mon, Feb 14, 2011 at 20:56, Will Deacon <will.deacon@arm.com> wrote:
>> > This patch modifies the GIC code to use handle_fasteoi_irq for handling
>> > interrupts, which only requires us to signal EOI to the CPU interface
>> > when handling is complete. Cascaded IRQ handling is also updated so that
>> > EOI is signalled after handling.
>>
>> Several of the platforms using the GIC also have GPIO code which uses
>> set_irq_chained_handler(). ?I think you will have to modify all of
>> these to call irq_eoi() appropriately and not the other functions.
>> Some of these will also likely be used with other interrupt handlers
>> than the GIC, though.
>
> Hmm, I had a quick look at some platforms that do this (mach-dove and
> plat-spear) and I don't see what the problem is. They use their own irq_chip
> structures, with their own function pointers, so this doesn't seem to relate
> to the GIC at all. What am I missing?!

The chained handlers are usually installed on GIC interrupts.  So, when
a chained handler does something like this

	desc->irq_data.chip->irq_unmask(&desc->irq_data);

the desc->irq_data.chip refers to the gic_chip.  These handlers are
written with the knowledge of what flow handler the GIC uses and what
functions it implements, so when you change that, the chained handler
code will not work correctly, and they'll need to be updated just like
you've updated the cascade IRQ handler.

In fact, I think that 846afbd1 ("GIC: Dont disable INT in ack callback")
has broken not just GIC cascading interrupts but assumptions in several
of these chained handlers, since several of them seem to have been
written assuming (invalidly) that irq_ack() masks the interrupt, but
this is no longer the case with the GIC after that commit.

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Will Deacon]

Hi Rabin,

> >> Several of the platforms using the GIC also have GPIO code which uses
> >> set_irq_chained_handler().  I think you will have to modify all of
> >> these to call irq_eoi() appropriately and not the other functions.
> >> Some of these will also likely be used with other interrupt handlers
> >> than the GIC, though.
> >
> > Hmm, I had a quick look at some platforms that do this (mach-dove and
> > plat-spear) and I don't see what the problem is. They use their own irq_chip
> > structures, with their own function pointers, so this doesn't seem to relate
> > to the GIC at all. What am I missing?!
> 
> The chained handlers are usually installed on GIC interrupts.  So, when
> a chained handler does something like this
> 
> 	desc->irq_data.chip->irq_unmask(&desc->irq_data);
> 
> the desc->irq_data.chip refers to the gic_chip.  These handlers are
> written with the knowledge of what flow handler the GIC uses and what
> functions it implements, so when you change that, the chained handler
> code will not work correctly, and they'll need to be updated just like
> you've updated the cascade IRQ handler.

Ah yes, thanks for the explanation. After looking at the plat-omap code
I finally understand what's going on and I can't help but feel that the
chained GPIO handlers are terminally broken! The generic irq chip high-level
handlers (handle_{edge,level}_irq for example) at least check to see if
the irq_chip functions are non-NULL before calling them.

Ideally, the chained handler would be able to query the irq_chip to find
out what types of IRQ flow-control it supports and then assume that behaviour.
 
> In fact, I think that 846afbd1 ("GIC: Dont disable INT in ack callback")
> has broken not just GIC cascading interrupts but assumptions in several
> of these chained handlers, since several of them seem to have been
> written assuming (invalidly) that irq_ack() masks the interrupt, but
> this is no longer the case with the GIC after that commit.

Yep - it was further reaching that I originally thought. The question now is:
is it worth changing all of these handlers or are we better off hacking the gic
code so that .irq_ack calls .irq_eoi? In the case of the latter, your performance
will suck in a virtualised environment, but that's better than broken.

Cheers,

Will

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Rabin Vincent]

(adding tglx)

On Wed, Feb 16, 2011 at 21:47, Will Deacon <will.deacon@arm.com> wrote:
>> >> Several of the platforms using the GIC also have GPIO code which uses
>> >> set_irq_chained_handler(). ?I think you will have to modify all of
>> >> these to call irq_eoi() appropriately and not the other functions.
>> >> Some of these will also likely be used with other interrupt handlers
>> >> than the GIC, though.
>> >
>> > Hmm, I had a quick look at some platforms that do this (mach-dove and
>> > plat-spear) and I don't see what the problem is. They use their own irq_chip
>> > structures, with their own function pointers, so this doesn't seem to relate
>> > to the GIC at all. What am I missing?!
>>
>> The chained handlers are usually installed on GIC interrupts. ?So, when
>> a chained handler does something like this
>>
>> ? ? ? desc->irq_data.chip->irq_unmask(&desc->irq_data);
>>
>> the desc->irq_data.chip refers to the gic_chip. ?These handlers are
>> written with the knowledge of what flow handler the GIC uses and what
>> functions it implements, so when you change that, the chained handler
>> code will not work correctly, and they'll need to be updated just like
>> you've updated the cascade IRQ handler.
>
> Ah yes, thanks for the explanation. After looking at the plat-omap code
> I finally understand what's going on and I can't help but feel that the
> chained GPIO handlers are terminally broken! The generic irq chip high-level
> handlers (handle_{edge,level}_irq for example) at least check to see if
> the irq_chip functions are non-NULL before calling them.
>
> Ideally, the chained handler would be able to query the irq_chip to find
> out what types of IRQ flow-control it supports and then assume that behaviour.

Thomas, suggestions on how best to handle this?  (Some of these chained
handlers are the ones in plat-omap/gpio.c, plat-nomadik/gpio.c, and
mach-s5pv310/irq-combiner.c.)

>
>> In fact, I think that 846afbd1 ("GIC: Dont disable INT in ack callback")
>> has broken not just GIC cascading interrupts but assumptions in several
>> of these chained handlers, since several of them seem to have been
>> written assuming (invalidly) that irq_ack() masks the interrupt, but
>> this is no longer the case with the GIC after that commit.
>
> Yep - it was further reaching that I originally thought. The question now is:
> is it worth changing all of these handlers or are we better off hacking the gic
> code so that .irq_ack calls .irq_eoi? In the case of the latter, your performance
> will suck in a virtualised environment, but that's better than broken.

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Thomas Gleixner]

On Wed, 16 Feb 2011, Rabin Vincent wrote:
> On Wed, Feb 16, 2011 at 21:47, Will Deacon <will.deacon@arm.com> wrote:
> > Ah yes, thanks for the explanation. After looking at the plat-omap code
> > I finally understand what's going on and I can't help but feel that the
> > chained GPIO handlers are terminally broken! The generic irq chip high-level
> > handlers (handle_{edge,level}_irq for example) at least check to see if
> > the irq_chip functions are non-NULL before calling them.
> >
> > Ideally, the chained handler would be able to query the irq_chip to find
> > out what types of IRQ flow-control it supports and then assume that behaviour.
> 
> Thomas, suggestions on how best to handle this?  (Some of these chained
> handlers are the ones in plat-omap/gpio.c, plat-nomadik/gpio.c, and
> mach-s5pv310/irq-combiner.c.)

I'm not much of a fan of those chained handlers. They work fine, when
they are tied into the irq_chip implementation of a SoC where the
chained handler code is 1:1 related to that irq_chip.

Once you start assigning those handlers somewhere else or even using
the same handler for different underlying primary irq chips, then it's
a lost case and wreckage like this is just lurking around the corner.

The only sane way to deal with this is to install a regular interrupt
handler with request/setup_irq() and do the demultiplexing from
there. That way the demux handler does not have to worry about the
underlying primary chip at all. It does not have to worry whether this
chip uses level, fasteoi, edge or whatever. It just works.

The runtime overhead of going through that path is minimal and really
not worth the pain.

Thanks,

	tglx

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Russell King - ARM Linux]

On Wed, Feb 16, 2011 at 08:20:17PM +0100, Thomas Gleixner wrote:
> On Wed, 16 Feb 2011, Rabin Vincent wrote:
> > On Wed, Feb 16, 2011 at 21:47, Will Deacon <will.deacon@arm.com> wrote:
> > > Ah yes, thanks for the explanation. After looking at the plat-omap code
> > > I finally understand what's going on and I can't help but feel that the
> > > chained GPIO handlers are terminally broken! The generic irq chip high-level
> > > handlers (handle_{edge,level}_irq for example) at least check to see if
> > > the irq_chip functions are non-NULL before calling them.
> > >
> > > Ideally, the chained handler would be able to query the irq_chip to find
> > > out what types of IRQ flow-control it supports and then assume that behaviour.
> > 
> > Thomas, suggestions on how best to handle this?  (Some of these chained
> > handlers are the ones in plat-omap/gpio.c, plat-nomadik/gpio.c, and
> > mach-s5pv310/irq-combiner.c.)
> 
> I'm not much of a fan of those chained handlers. They work fine, when
> they are tied into the irq_chip implementation of a SoC where the
> chained handler code is 1:1 related to that irq_chip.
> 
> Once you start assigning those handlers somewhere else or even using
> the same handler for different underlying primary irq chips, then it's
> a lost case and wreckage like this is just lurking around the corner.

That's a result of genirq.  With the ARM non-genirq implementation
where we had just ack, mask and unmask, it all turned out much easier.
In fact, we hardly ever called any of these functions in chained
handlers (except ack) - the only time when mask and unmask were called
was when we had an IRQ multiplexer which didn't allow us to mask the
individual interrupts.

With the advent of genirq and its expansion of the irq chip methods it's
become much more complicated.  That's one of the faults of trying to
generalize this across different architectures.

You can't do without chained handlers on ARM - we used to do it without
them and it was proven to be a complete disaster.  Dropped interrupts
and various drivers just didn't work with chained interrupt controllers.

So getting rid of it is also a 'lost cause'.

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Thomas Gleixner]

On Thu, 17 Feb 2011, Russell King - ARM Linux wrote:
> On Wed, Feb 16, 2011 at 08:20:17PM +0100, Thomas Gleixner wrote:
> > On Wed, 16 Feb 2011, Rabin Vincent wrote:
> > > On Wed, Feb 16, 2011 at 21:47, Will Deacon <will.deacon@arm.com> wrote:
> > > > Ah yes, thanks for the explanation. After looking at the plat-omap code
> > > > I finally understand what's going on and I can't help but feel that the
> > > > chained GPIO handlers are terminally broken! The generic irq chip high-level
> > > > handlers (handle_{edge,level}_irq for example) at least check to see if
> > > > the irq_chip functions are non-NULL before calling them.
> > > >
> > > > Ideally, the chained handler would be able to query the irq_chip to find
> > > > out what types of IRQ flow-control it supports and then assume that behaviour.
> > > 
> > > Thomas, suggestions on how best to handle this?  (Some of these chained
> > > handlers are the ones in plat-omap/gpio.c, plat-nomadik/gpio.c, and
> > > mach-s5pv310/irq-combiner.c.)
> > 
> > I'm not much of a fan of those chained handlers. They work fine, when
> > they are tied into the irq_chip implementation of a SoC where the
> > chained handler code is 1:1 related to that irq_chip.
> > 
> > Once you start assigning those handlers somewhere else or even using
> > the same handler for different underlying primary irq chips, then it's
> > a lost case and wreckage like this is just lurking around the corner.
> 
> That's a result of genirq.  With the ARM non-genirq implementation
> where we had just ack, mask and unmask, it all turned out much easier.
> In fact, we hardly ever called any of these functions in chained
> handlers (except ack) - the only time when mask and unmask were called
> was when we had an IRQ multiplexer which didn't allow us to mask the
> individual interrupts.
> 
> With the advent of genirq and its expansion of the irq chip methods it's
> become much more complicated.  That's one of the faults of trying to
> generalize this across different architectures.
> 
> You can't do without chained handlers on ARM - we used to do it without
> them and it was proven to be a complete disaster.  Dropped interrupts
> and various drivers just didn't work with chained interrupt controllers.

I really have a hard time to see the difference between doing:

  chained_handler()
	fiddle_with_primary_chip()
	demux()
	fiddle_with_primary_chip()
	
and

  handle_xxxx_irq()
	fiddle_with_primary_chip()

	chained_handler()
		demux()

	fiddle_with_primary_chip()

Why should that drop more interrupt? The only difference is that you
make the demux handler oblivious of the underlying primary chip and
let the chip chose the appropriate flow handler which issues the right
chip methods.

> So getting rid of it is also a 'lost cause'.

I didn't say that we need to get rid of chained handlers. You just
have to be very careful about them. Nothing new - with or without
genirq.

Thanks,

	tglx

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Russell King - ARM Linux]

On Thu, Feb 17, 2011 at 10:38:08AM +0100, Thomas Gleixner wrote:
> On Thu, 17 Feb 2011, Russell King - ARM Linux wrote:
> > That's a result of genirq.  With the ARM non-genirq implementation
> > where we had just ack, mask and unmask, it all turned out much easier.
> > In fact, we hardly ever called any of these functions in chained
> > handlers (except ack) - the only time when mask and unmask were called
> > was when we had an IRQ multiplexer which didn't allow us to mask the
> > individual interrupts.
> > 
> > With the advent of genirq and its expansion of the irq chip methods it's
> > become much more complicated.  That's one of the faults of trying to
> > generalize this across different architectures.
> > 
> > You can't do without chained handlers on ARM - we used to do it without
> > them and it was proven to be a complete disaster.  Dropped interrupts
> > and various drivers just didn't work with chained interrupt controllers.
> 
> I really have a hard time to see the difference between doing:
> 
>   chained_handler()
> 	fiddle_with_primary_chip()
> 	demux()
> 	fiddle_with_primary_chip()
> 	
> and
> 
>   handle_xxxx_irq()
> 	fiddle_with_primary_chip()
> 
> 	chained_handler()
> 		demux()
> 
> 	fiddle_with_primary_chip()
> 
> Why should that drop more interrupt? The only difference is that you
> make the demux handler oblivious of the underlying primary chip and
> let the chip chose the appropriate flow handler which issues the right
> chip methods.

The difference is that properly implemented chained handlers do *not*
mask the interrupt on the parent IRQ controller, thereby allowing
other interrupts to be serviced as appropriate.

The issue that this whole implementation was designed in the first place
to fix was lost IDE interrupts for CF cards connected to the PCMCIA/CF
interfaces of SA1111 devices.  These were lost because the CF interrupt
output is a level-based output, but the SA11xx interrupt inputs are all
edge based.

When the CF interrupt is connected to a SA1111 GPIO, and using the old
interrupt system, edge transitions were _often_ missed resulting in
frequent 'lost interrupt' complaints from the IDE layer.  Exactly why
this was could never be ascertained (probably because the hardware
combination I had at the time did not allow me to setup this scenario.)

With the chained interrupt handler implementation, things were setup to
ensure that as much of the interrupt levels were left enabled so that
there was symetrical handling across the entire interrupt tree, and the
result of that was no more lost interrupts.

There is a lot of difference between using a conventional 'flow' handler
which does masking/unmasking of the parent interrupt and a properly
written chained handler which avoids that masking.

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Thomas Gleixner]

On Thu, 17 Feb 2011, Russell King - ARM Linux wrote:
> On Thu, Feb 17, 2011 at 10:38:08AM +0100, Thomas Gleixner wrote:
> > Why should that drop more interrupt? The only difference is that you
> > make the demux handler oblivious of the underlying primary chip and
> > let the chip chose the appropriate flow handler which issues the right
> > chip methods.
> 
> The difference is that properly implemented chained handlers do *not*
> mask the interrupt on the parent IRQ controller, thereby allowing
> other interrupts to be serviced as appropriate.
> 
> The issue that this whole implementation was designed in the first place
> to fix was lost IDE interrupts for CF cards connected to the PCMCIA/CF
> interfaces of SA1111 devices.  These were lost because the CF interrupt
> output is a level-based output, but the SA11xx interrupt inputs are all
> edge based.
> 
> When the CF interrupt is connected to a SA1111 GPIO, and using the old
> interrupt system, edge transitions were _often_ missed resulting in
> frequent 'lost interrupt' complaints from the IDE layer.  Exactly why
> this was could never be ascertained (probably because the hardware
> combination I had at the time did not allow me to setup this scenario.)
> 
> With the chained interrupt handler implementation, things were setup to
> ensure that as much of the interrupt levels were left enabled so that
> there was symetrical handling across the entire interrupt tree, and the
> result of that was no more lost interrupts.
> 
> There is a lot of difference between using a conventional 'flow' handler
> which does masking/unmasking of the parent interrupt and a properly
> written chained handler which avoids that masking.

Depends on the underlying flow handler. fasteoi does not mask, but I
can see your point. If you have that oddball SA11xx thing, you are
forced to do that, but that is tied into SA11xx and confined.

But if you start to bolt that across platforms with various underlying
chips it simply does not work at all.

For sane primary irq chip implementations it should not matter at all
whether you do it with a chained handler directly or from a regular
requested interrupt.

Thanks,

	tglx

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Russell King - ARM Linux]

On Thu, Feb 17, 2011 at 11:43:38AM +0100, Thomas Gleixner wrote:
> On Thu, 17 Feb 2011, Russell King - ARM Linux wrote:
> > On Thu, Feb 17, 2011 at 10:38:08AM +0100, Thomas Gleixner wrote:
> > > Why should that drop more interrupt? The only difference is that you
> > > make the demux handler oblivious of the underlying primary chip and
> > > let the chip chose the appropriate flow handler which issues the right
> > > chip methods.
> > 
> > The difference is that properly implemented chained handlers do *not*
> > mask the interrupt on the parent IRQ controller, thereby allowing
> > other interrupts to be serviced as appropriate.
> > 
> > The issue that this whole implementation was designed in the first place
> > to fix was lost IDE interrupts for CF cards connected to the PCMCIA/CF
> > interfaces of SA1111 devices.  These were lost because the CF interrupt
> > output is a level-based output, but the SA11xx interrupt inputs are all
> > edge based.
> > 
> > When the CF interrupt is connected to a SA1111 GPIO, and using the old
> > interrupt system, edge transitions were _often_ missed resulting in
> > frequent 'lost interrupt' complaints from the IDE layer.  Exactly why
> > this was could never be ascertained (probably because the hardware
> > combination I had at the time did not allow me to setup this scenario.)
> > 
> > With the chained interrupt handler implementation, things were setup to
> > ensure that as much of the interrupt levels were left enabled so that
> > there was symetrical handling across the entire interrupt tree, and the
> > result of that was no more lost interrupts.
> > 
> > There is a lot of difference between using a conventional 'flow' handler
> > which does masking/unmasking of the parent interrupt and a properly
> > written chained handler which avoids that masking.
> 
> Depends on the underlying flow handler. fasteoi does not mask, but I
> can see your point. If you have that oddball SA11xx thing, you are
> forced to do that, but that is tied into SA11xx and confined.

It isn't - what I highlight above is the correct way to deal with
chained interrupts - which is to leave the parent interrupt enabled
_unless_ there is a good reason not to.

It also needs to be symetrical otherwise you unbalance the interrupt
handling such that those interrupts which are downstream of the primary
controller become less likely to be handled, and you end up with some
interrupts being soo pessimistic its untrue.  This commonly happens
when people don't think and just throw loop after loop into their
flow handlers.

The last reason for this is to ensure that IRQ handling is *fast* and
*efficient*.  Using the standard handlers adds a hell of a lot of
unnecessary overhead which just isn't required.

Look, I put a lot of time and effort into giving ARM an interrupt handling
subsystem which actually _works_ for the hardware that we commonly have,
and I'm not having all that thrown out in the name of 'generalization'.
If genirq can't cope with it, then ARM needs to avoid genirq.

Chained flow handlers are here to stay on ARM.

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Thomas Gleixner]

On Thu, 17 Feb 2011, Russell King - ARM Linux wrote:
> On Thu, Feb 17, 2011 at 11:43:38AM +0100, Thomas Gleixner wrote:
> > > There is a lot of difference between using a conventional 'flow' handler
> > > which does masking/unmasking of the parent interrupt and a properly
> > > written chained handler which avoids that masking.
> > 
> > Depends on the underlying flow handler. fasteoi does not mask, but I
> > can see your point. If you have that oddball SA11xx thing, you are
> > forced to do that, but that is tied into SA11xx and confined.
> 
> It isn't - what I highlight above is the correct way to deal with
> chained interrupts - which is to leave the parent interrupt enabled
> _unless_ there is a good reason not to.
> 
> It also needs to be symetrical otherwise you unbalance the interrupt
> handling such that those interrupts which are downstream of the primary
> controller become less likely to be handled, and you end up with some
> interrupts being soo pessimistic its untrue.  This commonly happens
> when people don't think and just throw loop after loop into their
> flow handlers.
> 
> The last reason for this is to ensure that IRQ handling is *fast* and
> *efficient*.  Using the standard handlers adds a hell of a lot of
> unnecessary overhead which just isn't required.
> 
> Look, I put a lot of time and effort into giving ARM an interrupt handling
> subsystem which actually _works_ for the hardware that we commonly have,
> and I'm not having all that thrown out in the name of 'generalization'.
> If genirq can't cope with it, then ARM needs to avoid genirq.
> 
> Chained flow handlers are here to stay on ARM.

I have no intention to remove them. If they are done correct they are
not a problem at all. And correct means: tied to the underlying
primary chip.

The shit hits the fan when a chained handler implementation is trying
to be agnostic about the actual underlying primary chip. That won't
ever work and is broken. The only way to make a demux handler agnostic
is to use a regular requested interrupt handler. That's all what I'm
saying.

You have to actually use your brain when implementing a chained
handler. Looking through a bunch of implementations I found stuff,
which is basically a poorly implemented flow handler. Worst example:
fsl_msi_cascade().

ARM ones are mostly sane, but e.g. nmk_gpio_irq_handler() is not
really one which fits your description. It's trying to deal with
different underlying primary chips obviously, which is wrong in the
first place.

Thanks,

	tglx

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Will Deacon]

> You have to actually use your brain when implementing a chained
> handler. Looking through a bunch of implementations I found stuff,
> which is basically a poorly implemented flow handler. Worst example:
> fsl_msi_cascade().
> 
> ARM ones are mostly sane, but e.g. nmk_gpio_irq_handler() is not
> really one which fits your description. It's trying to deal with
> different underlying primary chips obviously, which is wrong in the
> first place.

Right, so to get back to the original discussion about how to handle
chained handlers if the high-level flow type of the IRQ chip is altered
it seems that there are two options:

1.) Update all of the chained handlers to use the new flow-control
2.) Retain backwards compatibility if a chained handler decides to
    use the old method of flow control (specifically, leave an ack
    implementation in the GIC code after moving to fasteoi).

Obviously, I'd rather go with option (2) and let platforms migrate
over time if they choose to. Now, given that the ack function is really
not something you want to do in a virtualised environment (because it
pokes the distributor), is it worth sticking a
WARN_ON_ONCE(cpu_has_virtualisation()); in the ack code?

Cheers,

Will

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Thomas Gleixner]

On Thu, 17 Feb 2011, Will Deacon wrote:

> > You have to actually use your brain when implementing a chained
> > handler. Looking through a bunch of implementations I found stuff,
> > which is basically a poorly implemented flow handler. Worst example:
> > fsl_msi_cascade().
> > 
> > ARM ones are mostly sane, but e.g. nmk_gpio_irq_handler() is not
> > really one which fits your description. It's trying to deal with
> > different underlying primary chips obviously, which is wrong in the
> > first place.
> 
> Right, so to get back to the original discussion about how to handle
> chained handlers if the high-level flow type of the IRQ chip is altered
> it seems that there are two options:
> 
> 1.) Update all of the chained handlers to use the new flow-control
> 2.) Retain backwards compatibility if a chained handler decides to
>     use the old method of flow control (specifically, leave an ack
>     implementation in the GIC code after moving to fasteoi).
> 
> Obviously, I'd rather go with option (2) and let platforms migrate
> over time if they choose to. Now, given that the ack function is really
> not something you want to do in a virtualised environment (because it
> pokes the distributor), is it worth sticking a
> WARN_ON_ONCE(cpu_has_virtualisation()); in the ack code?

#2 is less painful and just works. The fasteoi stuff does not use ack
IIRC so it wont hurt.

vs. the WARN_ON_ONCE(), I have no real opinion.

Thanks,

	tglx

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Abhijeet Dharmapurikar]

>> Right, so to get back to the original discussion about how to handle
>> chained handlers if the high-level flow type of the IRQ chip is altered
>> it seems that there are two options:
>>
>> 1.) Update all of the chained handlers to use the new flow-control
>> 2.) Retain backwards compatibility if a chained handler decides to
>>     use the old method of flow control (specifically, leave an ack
>>     implementation in the GIC code after moving to fasteoi).
>>
>> Obviously, I'd rather go with option (2) and let platforms migrate
>> over time if they choose to. Now, given that the ack function is really
>> not something you want to do in a virtualised environment (because it
>> pokes the distributor), is it worth sticking a
>> WARN_ON_ONCE(cpu_has_virtualisation()); in the ack code?
> 
> #2 is less painful and just works. The fasteoi stuff does not use ack
> IIRC so it wont hurt.

On an MSM we use handle_percpu_irq for PPIs, if we have ack and eoi we 
will end up EOI ing the interrupt twice so #2 wont work. Also all the 
cascaded handlers would have assumed that the ack function masks the 
interrupt, it is best we fix all of them to use eoi at the end (just 
like handle_fasteoi_irq). Please tell me how you guys locate such 
cascaded handlers?

Sorry for my earlier patch - didnt realize that the chaging the ack will 
break the chained handler. My board doesnt have cascaded gics. The only 
chained handler I knew was gpio-v2.c and it does the correct thing of 
calling an desc->chip->ack at the end.

BTW, I am all in favor of using handle_fasteoi_irq for shared PPIs , It 
will fix a problem I pointed out here.
http://kerneltrap.org/mailarchive/linux-kernel/2010/12/30/4664338

--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm 
Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Will Deacon]

> >> Right, so to get back to the original discussion about how to handle
> >> chained handlers if the high-level flow type of the IRQ chip is altered
> >> it seems that there are two options:
> >>
> >> 1.) Update all of the chained handlers to use the new flow-control
> >> 2.) Retain backwards compatibility if a chained handler decides to
> >>     use the old method of flow control (specifically, leave an ack
> >>     implementation in the GIC code after moving to fasteoi).
> >>
> >> Obviously, I'd rather go with option (2) and let platforms migrate
> >> over time if they choose to. Now, given that the ack function is really
> >> not something you want to do in a virtualised environment (because it
> >> pokes the distributor), is it worth sticking a
> >> WARN_ON_ONCE(cpu_has_virtualisation()); in the ack code?
> >
> > #2 is less painful and just works. The fasteoi stuff does not use ack
> > IIRC so it wont hurt.
> 
> On an MSM we use handle_percpu_irq for PPIs, if we have ack and eoi we
> will end up EOI ing the interrupt twice so #2 wont work. Also all the
> cascaded handlers would have assumed that the ack function masks the
> interrupt, it is best we fix all of them to use eoi at the end (just
> like handle_fasteoi_irq). Please tell me how you guys locate such
> cascaded handlers?

I don't think the cascaded handlers would have assumed that because ack
just sends EOI - it doesn't do any masking. We do have a problem with
the percpu_irq flow though (the GIC reference manual says that EOIing a
non-active interrupt is UNPREDICTABLE).

Another easy hack is to set IRQ_PER_CPU in the irq_desc->status for PPI
interrupts and then check this in the ack routine. It's pretty ugly, but
it doesn't affect the common case and it at least postpones the platform
changes.
 
Will

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Thomas Gleixner]

On Fri, 18 Feb 2011, Will Deacon wrote:
> > >> Right, so to get back to the original discussion about how to handle
> > >> chained handlers if the high-level flow type of the IRQ chip is altered
> > >> it seems that there are two options:
> > >>
> > >> 1.) Update all of the chained handlers to use the new flow-control
> > >> 2.) Retain backwards compatibility if a chained handler decides to
> > >>     use the old method of flow control (specifically, leave an ack
> > >>     implementation in the GIC code after moving to fasteoi).
> > >>
> > >> Obviously, I'd rather go with option (2) and let platforms migrate
> > >> over time if they choose to. Now, given that the ack function is really
> > >> not something you want to do in a virtualised environment (because it
> > >> pokes the distributor), is it worth sticking a
> > >> WARN_ON_ONCE(cpu_has_virtualisation()); in the ack code?
> > >
> > > #2 is less painful and just works. The fasteoi stuff does not use ack
> > > IIRC so it wont hurt.
> > 
> > On an MSM we use handle_percpu_irq for PPIs, if we have ack and eoi we
> > will end up EOI ing the interrupt twice so #2 wont work. Also all the
> > cascaded handlers would have assumed that the ack function masks the
> > interrupt, it is best we fix all of them to use eoi at the end (just
> > like handle_fasteoi_irq). Please tell me how you guys locate such
> > cascaded handlers?
> 
> I don't think the cascaded handlers would have assumed that because ack
> just sends EOI - it doesn't do any masking. We do have a problem with
> the percpu_irq flow though (the GIC reference manual says that EOIing a
> non-active interrupt is UNPREDICTABLE).
> 
> Another easy hack is to set IRQ_PER_CPU in the irq_desc->status for PPI
> interrupts and then check this in the ack routine. It's pretty ugly, but
> it doesn't affect the common case and it at least postpones the platform
> changes.

Conditionals in irq_chip callbacks are almost always a sign of
doom. Don't do that.

How many chained handlers need to be fixed, when the whole gic stuff
switches to eoi ?

Thanks,

	tglx

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Will Deacon]

> > I don't think the cascaded handlers would have assumed that because ack
> > just sends EOI - it doesn't do any masking. We do have a problem with
> > the percpu_irq flow though (the GIC reference manual says that EOIing a
> > non-active interrupt is UNPREDICTABLE).
> >
> > Another easy hack is to set IRQ_PER_CPU in the irq_desc->status for PPI
> > interrupts and then check this in the ack routine. It's pretty ugly, but
> > it doesn't affect the common case and it at least postpones the platform
> > changes.
> 
> Conditionals in irq_chip callbacks are almost always a sign of
> doom. Don't do that.

Ok, that was a hack too far! Let's fix this properly.
 
> How many chained handlers need to be fixed, when the whole gic stuff
> switches to eoi ?

Well, grepping for set_chained_irq_handler yields a whole bunch of platforms
but the set of these which appear to use the gic is only:

mach-msm
mach-s5pv310
mach-shmobile
mach-tegra

I'll have a look through the code there and post some patches next week.
Hopefully if I've missed anybody, they'll shout then.

Cheers,

Will

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Colin Cross]

On Fri, Feb 18, 2011 at 4:09 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > I don't think the cascaded handlers would have assumed that because ack
>> > just sends EOI - it doesn't do any masking. We do have a problem with
>> > the percpu_irq flow though (the GIC reference manual says that EOIing a
>> > non-active interrupt is UNPREDICTABLE).
>> >
>> > Another easy hack is to set IRQ_PER_CPU in the irq_desc->status for PPI
>> > interrupts and then check this in the ack routine. It's pretty ugly, but
>> > it doesn't affect the common case and it at least postpones the platform
>> > changes.
>>
>> Conditionals in irq_chip callbacks are almost always a sign of
>> doom. Don't do that.
>
> Ok, that was a hack too far! Let's fix this properly.
>
>> How many chained handlers need to be fixed, when the whole gic stuff
>> switches to eoi ?
>
> Well, grepping for set_chained_irq_handler yields a whole bunch of platforms
> but the set of these which appear to use the gic is only:
>
> mach-msm
> mach-s5pv310
> mach-shmobile
> mach-tegra
>
> I'll have a look through the code there and post some patches next week.
> Hopefully if I've missed anybody, they'll shout then.

omap4 uses the gic, and uses chained handlers in plat-omap/gpio.c.
plat-omap/gpio.c seems to handle similar gpio hardware connected to
different IRQ controllers on different SoCs - non-gic on omap2-3, and
gic on omap4.

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Santosh Shilimkar]

> -----Original Message-----
> From: Colin Cross [mailto:ccross at google.com]
> Sent: Saturday, February 19, 2011 12:01 AM
> To: Will Deacon
> Cc: Thomas Gleixner; Rabin Vincent; Abhijeet Dharmapurikar; Russell
> King - ARM Linux; linux-arm-kernel at lists.infradead.org; Santosh
> Shilimkar
> Subject: Re: [PATCH] ARM: gic: use handle_fasteoi_irq for SPIs
>
> On Fri, Feb 18, 2011 at 4:09 AM, Will Deacon <will.deacon@arm.com>
> wrote:
> >> > I don't think the cascaded handlers would have assumed that
> because ack
> >> > just sends EOI - it doesn't do any masking. We do have a
> problem with
> >> > the percpu_irq flow though (the GIC reference manual says that
> EOIing a
> >> > non-active interrupt is UNPREDICTABLE).
> >> >
> >> > Another easy hack is to set IRQ_PER_CPU in the irq_desc->status
> for PPI
> >> > interrupts and then check this in the ack routine. It's pretty
> ugly, but
> >> > it doesn't affect the common case and it at least postpones the
> platform
> >> > changes.
> >>
> >> Conditionals in irq_chip callbacks are almost always a sign of
> >> doom. Don't do that.
> >
> > Ok, that was a hack too far! Let's fix this properly.
> >
> >> How many chained handlers need to be fixed, when the whole gic
> stuff
> >> switches to eoi ?
> >
> > Well, grepping for set_chained_irq_handler yields a whole bunch of
> platforms
> > but the set of these which appear to use the gic is only:
> >
> > mach-msm
> > mach-s5pv310
> > mach-shmobile
> > mach-tegra
> >
> > I'll have a look through the code there and post some patches next
> week.
> > Hopefully if I've missed anybody, they'll shout then.
>
> omap4 uses the gic, and uses chained handlers in plat-omap/gpio.c.
> plat-omap/gpio.c seems to handle similar gpio hardware connected to
> different IRQ controllers on different SoCs - non-gic on omap2-3,
> and gic on omap4.

That's right.

Thanks Colin for pointing it. I almost missed this
thread.

Regards,
Santosh

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Will Deacon]

> > Well, grepping for set_chained_irq_handler yields a whole bunch of platforms
> > but the set of these which appear to use the gic is only:
> >
> > mach-msm
> > mach-s5pv310
> > mach-shmobile
> > mach-tegra
> >
> > I'll have a look through the code there and post some patches next week.
> > Hopefully if I've missed anybody, they'll shout then.
> 
> omap4 uses the gic, and uses chained handlers in plat-omap/gpio.c.
> plat-omap/gpio.c seems to handle similar gpio hardware connected to
> different IRQ controllers on different SoCs - non-gic on omap2-3, and
> gic on omap4.

Thanks for spotting this Colin. My grep-fu failed me because the gic
initialisation for omap is under a different directory to the GPIO cascaded
IRQ stuff (plat- vs mach-).

I'll add OMAP to the list.

Will

[PATCH] ARM: gic: use handle_fasteoi_irq for SPIs

[Will Deacon]

Currently, the gic uses handle_level_irq for handling SPIs (Shared
Peripheral Interrupts), requiring active interrupts to be masked at
the distributor level during IRQ handling.

On a virtualised system, only the CPU interfaces are virtualised in
hardware. Accesses to the distributor must be trapped by the hypervisor,
adding latency to the critical interrupt path in Linux.

This patch modifies the GIC code to use handle_fasteoi_irq for handling
interrupts, which only requires us to signal EOI to the CPU interface
when handling is complete. Cascaded IRQ handling is also updated so that
EOI is signalled after handling.

Note that commit 846afbd1 ("GIC: Dont disable INT in ack callback") broke
cascading interrupts by forgetting to add IRQ masking. This is no longer
an issue because the unmask call is now unnecessary.

Tested on Versatile Express and Realview EB (1176 w/ cascaded GICs).

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/common/gic.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 2243772..9def30b 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -71,13 +71,6 @@ static inline unsigned int gic_irq(struct irq_data *d)
 /*
  * Routines to acknowledge, disable and enable interrupts
  */
-static void gic_ack_irq(struct irq_data *d)
-{
-	spin_lock(&irq_controller_lock);
-	writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
-	spin_unlock(&irq_controller_lock);
-}
-
 static void gic_mask_irq(struct irq_data *d)
 {
 	u32 mask = 1 << (d->irq % 32);
@@ -96,6 +89,11 @@ static void gic_unmask_irq(struct irq_data *d)
 	spin_unlock(&irq_controller_lock);
 }
 
+static void gic_eoi_irq(struct irq_data *d)
+{
+	writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+}
+
 static int gic_set_type(struct irq_data *d, unsigned int type)
 {
 	void __iomem *base = gic_dist_base(d);
@@ -174,9 +172,6 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 	unsigned int cascade_irq, gic_irq;
 	unsigned long status;
 
-	/* primary controller ack'ing */
-	chip->irq_ack(&desc->irq_data);
-
 	spin_lock(&irq_controller_lock);
 	status = readl(chip_data->cpu_base + GIC_CPU_INTACK);
 	spin_unlock(&irq_controller_lock);
@@ -192,15 +187,15 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 		generic_handle_irq(cascade_irq);
 
  out:
-	/* primary controller unmasking */
-	chip->irq_unmask(&desc->irq_data);
+	/* primary controller EOI */
+	chip->irq_eoi(&desc->irq_data);
 }
 
 static struct irq_chip gic_chip = {
 	.name			= "GIC",
-	.irq_ack		= gic_ack_irq,
 	.irq_mask		= gic_mask_irq,
 	.irq_unmask		= gic_unmask_irq,
+	.irq_eoi		= gic_eoi_irq,
 	.irq_set_type		= gic_set_type,
 #ifdef CONFIG_SMP
 	.irq_set_affinity	= gic_set_cpu,
@@ -275,7 +270,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	for (i = irq_start; i < irq_limit; i++) {
 		set_irq_chip(i, &gic_chip);
 		set_irq_chip_data(i, gic);
-		set_irq_handler(i, handle_level_irq);
+		set_irq_handler(i, handle_fasteoi_irq);
 		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
 	}
 
-- 
1.7.0.4