ARM: gic: use handle_fasteoi_irq for SPIs

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.

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 11 lines · Will Deacon

Quoted 4 lines

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?!

Quoted 10 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 10 lines

Quoted 5 lines

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

Quoted 7 lines

? ? ? 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.

Quoted 6 lines

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.

Quoted 6 lines · Will Deacon

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.

Quoted 2 lines

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.)

Quoted 7 lines · Rabin Vincent

Quoted 6 lines · Will Deacon

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 15 lines · Thomas Gleixner

Quoted 7 lines · Rabin Vincent

Quoted 6 lines · Will Deacon

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.

Quoted 6 lines

Quoted 2 lines

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.)

Quoted 7 lines

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'.

Quoted 7 lines · Russell King - ARM Linux

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.

Quoted 7 lines

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.

Quoted 4 lines · Thomas Gleixner

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.

Quoted 27 lines · Russell King - ARM Linux

Quoted 4 lines · Thomas Gleixner

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.

Quoted 3 lines

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.

Quoted 7 lines · Thomas Gleixner

Quoted 3 lines

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.

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.

Quoted 9 lines

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?

Quoted 10 lines

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).

Quoted 5 lines

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.

Quoted 9 lines

Quoted 9 lines

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).

Quoted 8 lines

Quoted 5 lines

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?

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

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).

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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).

Quoted 4 lines

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 ?

Quoted 5 lines

Quoted 5 lines

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).

Quoted 7 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

-----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

Quoted 1 line · Will Deacon

Quoted 1 line

Quoted 1 line

I don't think the cascaded handlers would have assumed that

because ack

Quoted 1 line

Quoted 1 line

Quoted 1 line

just sends EOI - it doesn't do any masking. We do have a

problem with

Quoted 1 line

Quoted 1 line

Quoted 1 line

the percpu_irq flow though (the GIC reference manual says that

EOIing a

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

non-active interrupt is UNPREDICTABLE).

Another easy hack is to set IRQ_PER_CPU in the irq_desc->status

for PPI

Quoted 1 line

Quoted 1 line

Quoted 1 line

interrupts and then check this in the ack routine. It's pretty

ugly, but

Quoted 1 line

Quoted 1 line

Quoted 1 line

it doesn't affect the common case and it at least postpones the

platform

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

How many chained handlers need to be fixed, when the whole gic

stuff

Quoted 3 lines

Quoted 1 line

switches to eoi ?

Well, grepping for set_chained_irq_handler yields a whole bunch of

platforms

Quoted 7 lines

but the set of these which appear to use the gic is only:

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

Quoted 1 line

I'll have a look through the code there and post some patches next

week.

Quoted 1 line

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.

Quoted 8 lines

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

Quoted 2 lines

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.