[PATCH v2 0/6] Migrate GIC to fasteoi flow control

[PATCH v2 0/6] Migrate GIC to fasteoi flow control

[Will Deacon]

Hello,

This is version 2 of the patch series originally posted here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-February/043083.html

Changes since v1:

	* Added plat-nomadik changes after related patch from Rabin
	* Fixed msm changes to call irq_eoi correctly
	* No longer in quoted-printable encoding (thanks Colin)

This is going to be a PITA to merge so any thoughts relating to that
are welcome.

Thanks,

Will


Will Deacon (6):
  ARM: gic: use handle_fasteoi_irq for SPIs
  ARM: omap: update GPIO chained IRQ handler to use EOI in parent chip
  ARM: tegra: update GPIO chained IRQ handler to use EOI in parent chip
  ARM: s5pv310: update IRQ combiner to use EOI in parent chip
  ARM: msm: update GPIO chained IRQ handler to use EOI in parent chip
  ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

 arch/arm/common/gic.c                |   23 +++++++++--------------
 arch/arm/mach-msm/gpio-v2.c          |    3 ++-
 arch/arm/mach-s5pv310/irq-combiner.c |    7 ++-----
 arch/arm/mach-tegra/gpio.c           |   17 +----------------
 arch/arm/plat-nomadik/gpio.c         |    2 ++
 arch/arm/plat-omap/gpio.c            |    5 ++++-
 6 files changed, 20 insertions(+), 37 deletions(-)

[PATCH 1/6] 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>
---
 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 2/6] ARM: omap: update GPIO chained IRQ handler to use EOI in parent chip

[Will Deacon]

On OMAP4, gpio_irq_handler can be chained to a physical interrupt using
the GIC as the primary IRQ chip. Now that the GIC uses the fasteoi flow
model, the chained handler must invoke ->irq_eoi once handling is
complete.

This patch adds a conditional call to ->irq_eoi in the GPIO IRQ handler
for OMAP platforms. For OMAP implementations using other primary
interrupt controllers, the ->irq_ack call remains and is also made
conditional on the support offered by the controller.

Cc: Colin Cross <ccross@google.com>
Acked-by: Tony Lindgren <tony@atomide.com>
Acked-and-tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/plat-omap/gpio.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 971d186..1d2d1c7 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -1144,7 +1144,8 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	u32 retrigger = 0;
 	int unmasked = 0;
 
-	desc->irq_data.chip->irq_ack(&desc->irq_data);
+	if (desc->irq_data.chip->irq_ack)
+		desc->irq_data.chip->irq_ack(&desc->irq_data);
 
 	bank = get_irq_data(irq);
 #ifdef CONFIG_ARCH_OMAP1
@@ -1238,6 +1239,8 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 exit:
 	if (!unmasked)
 		desc->irq_data.chip->irq_unmask(&desc->irq_data);
+	if (desc->irq_data.chip->irq_eoi)
+		desc->irq_data.chip->irq_eoi(&desc->irq_data);
 }
 
 static void gpio_irq_shutdown(struct irq_data *d)
-- 
1.7.0.4

[PATCH 3/6] ARM: tegra: update GPIO chained IRQ handler to use EOI in parent chip

[Will Deacon]

The chained GPIO IRQ handler on Tegra calls ->irq_ack on the parent
chip prior to handling the interrupt.

This patch updates the code to use ->irq_eoi now that the GIC has moved
to using the fasteoi flow model.

Acked-by: Colin Cross <ccross@android.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/mach-tegra/gpio.c |   17 +----------------
 1 files changed, 1 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-tegra/gpio.c b/arch/arm/mach-tegra/gpio.c
index ad80488..5019b01 100644
--- a/arch/arm/mach-tegra/gpio.c
+++ b/arch/arm/mach-tegra/gpio.c
@@ -219,9 +219,6 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	struct tegra_gpio_bank *bank;
 	int port;
 	int pin;
-	int unmasked = 0;
-
-	desc->irq_data.chip->irq_ack(&desc->irq_data);
 
 	bank = get_irq_data(irq);
 
@@ -233,23 +230,11 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 
 		for_each_set_bit(pin, &sta, 8) {
 			__raw_writel(1 << pin, GPIO_INT_CLR(gpio));
-
-			/* if gpio is edge triggered, clear condition
-			 * before executing the hander so that we don't
-			 * miss edges
-			 */
-			if (lvl & (0x100 << pin)) {
-				unmasked = 1;
-				desc->irq_data.chip->irq_unmask(&desc->irq_data);
-			}
-
 			generic_handle_irq(gpio_to_irq(gpio + pin));
 		}
 	}
 
-	if (!unmasked)
-		desc->irq_data.chip->irq_unmask(&desc->irq_data);
-
+	desc->irq_data.chip->irq_eoi(&desc->irq_data);
 }
 
 #ifdef CONFIG_PM
-- 
1.7.0.4

[PATCH 4/6] ARM: s5pv310: update IRQ combiner to use EOI in parent chip

[Will Deacon]

The IRQ combiner code invokes the ->irq_{un}mask routines of the parent
chip.

This patch updates the cascaded handler to use EOI now that the GIC has
moved to using the fasteoi flow model.

Tested-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/mach-s5pv310/irq-combiner.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-s5pv310/irq-combiner.c b/arch/arm/mach-s5pv310/irq-combiner.c
index 1ea4a9e..24d5604 100644
--- a/arch/arm/mach-s5pv310/irq-combiner.c
+++ b/arch/arm/mach-s5pv310/irq-combiner.c
@@ -59,9 +59,6 @@ static void combiner_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 	unsigned int cascade_irq, combiner_irq;
 	unsigned long status;
 
-	/* primary controller ack'ing */
-	chip->irq_ack(&desc->irq_data);
-
 	spin_lock(&irq_controller_lock);
 	status = __raw_readl(chip_data->base + COMBINER_INT_STATUS);
 	spin_unlock(&irq_controller_lock);
@@ -79,8 +76,8 @@ static void combiner_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 combiner_chip = {
-- 
1.7.0.4

[PATCH 5/6] ARM: msm: update GPIO chained IRQ handler to use EOI in parent chip

[Will Deacon]

The chained GPIO IRQ handler on MSM8x60 calls ->ack on the parent chip
after handling the interrupt.

This patch updates the code to use ->irq_eoi now that the GIC has moved
to using the fasteoi flow model.

Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/mach-msm/gpio-v2.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-msm/gpio-v2.c b/arch/arm/mach-msm/gpio-v2.c
index 0de19ec..90a968f 100644
--- a/arch/arm/mach-msm/gpio-v2.c
+++ b/arch/arm/mach-msm/gpio-v2.c
@@ -310,6 +310,7 @@ static int msm_gpio_irq_set_type(unsigned int irq, unsigned int flow_type)
 static void msm_summary_irq_handler(unsigned int irq, struct irq_desc *desc)
 {
 	unsigned long i;
+	struct irq_chip *chip = get_irq_desc_chip(desc);
 
 	for (i = find_first_bit(msm_gpio.enabled_irqs, NR_GPIO_IRQS);
 	     i < NR_GPIO_IRQS;
@@ -318,7 +319,7 @@ static void msm_summary_irq_handler(unsigned int irq, struct irq_desc *desc)
 			generic_handle_irq(msm_gpio_to_irq(&msm_gpio.gpio_chip,
 							   i));
 	}
-	desc->chip->ack(irq);
+	chip->irq_eoi(&desc->irq_data);
 }
 
 static int msm_gpio_irq_set_wake(unsigned int irq, unsigned int on)
-- 
1.7.0.4

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Will Deacon]

The chained GPIO IRQ handler for the nomadik platform can be called
with the GIC as its host chip on the mach-ux500 machines.

This patch updates the code to use ->irq_eoi when it is available.

Cc: Rabin Vincent <rabin@rab.in>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/plat-nomadik/gpio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-nomadik/gpio.c b/arch/arm/plat-nomadik/gpio.c
index 1e88ecb..51cc71b 100644
--- a/arch/arm/plat-nomadik/gpio.c
+++ b/arch/arm/plat-nomadik/gpio.c
@@ -538,6 +538,8 @@ static void nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	}
 
 	host_chip->irq_unmask(&desc->irq_data);
+	if (host_chip->irq_eoi)
+		host_chip->irq_eoi(&desc->irq_data);
 }
 
 static int nmk_gpio_init_irq(struct nmk_gpio_chip *nmk_chip)
-- 
1.7.0.4

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Russell King - ARM Linux]

On Mon, Feb 28, 2011 at 01:33:42PM +0000, Will Deacon wrote:
> The chained GPIO IRQ handler for the nomadik platform can be called
> with the GIC as its host chip on the mach-ux500 machines.
> 
> This patch updates the code to use ->irq_eoi when it is available.
> 
> Cc: Rabin Vincent <rabin@rab.in>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/plat-nomadik/gpio.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-nomadik/gpio.c b/arch/arm/plat-nomadik/gpio.c
> index 1e88ecb..51cc71b 100644
> --- a/arch/arm/plat-nomadik/gpio.c
> +++ b/arch/arm/plat-nomadik/gpio.c
> @@ -538,6 +538,8 @@ static void nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>  	}
>  
>  	host_chip->irq_unmask(&desc->irq_data);
> +	if (host_chip->irq_eoi)
> +		host_chip->irq_eoi(&desc->irq_data);

I notice in some you delete the irq_unmask, others you leave it.  Shouldn't
they all be similar?

Maybe we do something like:

static inline void chained_irq_exit(struct irq_chip *chip, struct irq_desc *desc)
{
	if (chip->irq_eoi)
		chip->irq_eoi(&desc->irq_data);
	else
		chip->irq_unmask(&desc->irq_data);
}

to cover these exit paths in a common way?

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Will Deacon]

Hi Russell,

> On Mon, Feb 28, 2011 at 01:33:42PM +0000, Will Deacon wrote:
> > The chained GPIO IRQ handler for the nomadik platform can be called
> > with the GIC as its host chip on the mach-ux500 machines.
> >
> > This patch updates the code to use ->irq_eoi when it is available.
> >
> > Cc: Rabin Vincent <rabin@rab.in>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm/plat-nomadik/gpio.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/plat-nomadik/gpio.c b/arch/arm/plat-nomadik/gpio.c
> > index 1e88ecb..51cc71b 100644
> > --- a/arch/arm/plat-nomadik/gpio.c
> > +++ b/arch/arm/plat-nomadik/gpio.c
> > @@ -538,6 +538,8 @@ static void nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> >  	}
> >
> >  	host_chip->irq_unmask(&desc->irq_data);
> > +	if (host_chip->irq_eoi)
> > +		host_chip->irq_eoi(&desc->irq_data);
> 
> I notice in some you delete the irq_unmask, others you leave it.  Shouldn't
> they all be similar?

It depends on whether or not the parent chip is always the GIC. In some cases
it can be a different IRQ chip, so we need to call the functions conditionally.
 
> Maybe we do something like:
> 
> static inline void chained_irq_exit(struct irq_chip *chip, struct irq_desc *desc)
> {
> 	if (chip->irq_eoi)
> 		chip->irq_eoi(&desc->irq_data);
> 	else
> 		chip->irq_unmask(&desc->irq_data);
> }
> 
> to cover these exit paths in a common way?

Yes, that is cleaner and potentially less confusing. We'll also need an entry
function which will do something like:

	if (!chip->irq_eoi)
		chip->irq_mask(&desc->irq_data);

which does look pretty odd, so factoring it out (with a comment) will
make it a little clearer.

I'll see if I get any feedback about nomadik and msm and then post a v3
with the entry/exit functions.

Will

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Thomas Gleixner]

On Mon, 28 Feb 2011, Will Deacon wrote:
> > On Mon, Feb 28, 2011 at 01:33:42PM +0000, Will Deacon wrote:
> > > The chained GPIO IRQ handler for the nomadik platform can be called
> > > with the GIC as its host chip on the mach-ux500 machines.
> > >
> > > This patch updates the code to use ->irq_eoi when it is available.
> > >
> > > Cc: Rabin Vincent <rabin@rab.in>
> > > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > > ---
> > >  arch/arm/plat-nomadik/gpio.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/arm/plat-nomadik/gpio.c b/arch/arm/plat-nomadik/gpio.c
> > > index 1e88ecb..51cc71b 100644
> > > --- a/arch/arm/plat-nomadik/gpio.c
> > > +++ b/arch/arm/plat-nomadik/gpio.c
> > > @@ -538,6 +538,8 @@ static void nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> > >  	}
> > >
> > >  	host_chip->irq_unmask(&desc->irq_data);
> > > +	if (host_chip->irq_eoi)
> > > +		host_chip->irq_eoi(&desc->irq_data);
> > 
> > I notice in some you delete the irq_unmask, others you leave it.  Shouldn't
> > they all be similar?
> 
> It depends on whether or not the parent chip is always the GIC. In some cases
> it can be a different IRQ chip, so we need to call the functions conditionally.
>  
> > Maybe we do something like:
> > 
> > static inline void chained_irq_exit(struct irq_chip *chip, struct irq_desc *desc)
> > {
> > 	if (chip->irq_eoi)
> > 		chip->irq_eoi(&desc->irq_data);
> > 	else
> > 		chip->irq_unmask(&desc->irq_data);
> > }
> > 
> > to cover these exit paths in a common way?
> 
> Yes, that is cleaner and potentially less confusing. We'll also need an entry
> function which will do something like:
> 
> 	if (!chip->irq_eoi)
> 		chip->irq_mask(&desc->irq_data);
> 
> which does look pretty odd, so factoring it out (with a comment) will
> make it a little clearer.

And that's the whole mess I was ranting about in the first place. A
chained handler is tied to the underlying primary chip and not a half
arsed flow handler which handles magically the availability or the
absence of irq chip functions. That's just doomed.

And for those cases I really want a reasonable explanation why the
chained handler can't be implemented as a bog standard interrupt
handler which relies on the underlying irq chip implementation to
select the proper flow handler in the first place.

Really folks, count the number of instructions which a normal flow
handler adds. We talk about what? Not more than 100 instructions.

So what's the gain of a barebone chained handler over a regular
interrupt:

 - 100 instructions less
 - lack of statistics
 - lack of affinity setting via standard interfaces
 - making the chained handler aware of the underlying hardware

While a bog standard interrupt handler which is installed via
request/setup_irq() has only the 100 instructions more downside.
There is no other difference.

So please think about whether the 100 instructions are worth the
trouble or not.

Thanks,

	tglx

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Russell King - ARM Linux]

On Mon, Feb 28, 2011 at 08:16:25PM +0100, Thomas Gleixner wrote:
> So what's the gain of a barebone chained handler over a regular
> interrupt:
> 
>  - 100 instructions less
>  - lack of statistics

We don't want statistics.  Don't care how many times we go to look
at the interrupt controller - and actually it's really wrong to count
them in the first place.  Counting them means that you double-count
these interrupt events, and the more layers of interrupt controllers
there are the worse that problem gets.

So no, that's a definite argument *for* chained handers.

>  - lack of affinity setting via standard interfaces

Don't want affinity for them, as setting their affinity means that
you then end up forcing the affinity for the sub-interrupts too.
How you do cope with the high-level interrupt having affinity to
CPU0 but a lower level interrupt being affine to CPU1 only?

It's non-sensible, and is broken.  So no, again this isn't an
argument for not using chained handlers.  It's an argument *for*
them.

Sorry, but I think this stuff is right, and chained handlers like
these have their place.

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Thomas Gleixner]

On Mon, 28 Feb 2011, Russell King - ARM Linux wrote:

> On Mon, Feb 28, 2011 at 08:16:25PM +0100, Thomas Gleixner wrote:
> > So what's the gain of a barebone chained handler over a regular
> > interrupt:
> > 
> >  - 100 instructions less
> >  - lack of statistics
> 
> We don't want statistics.  Don't care how many times we go to look
> at the interrupt controller - and actually it's really wrong to count
> them in the first place.  Counting them means that you double-count
> these interrupt events, and the more layers of interrupt controllers
> there are the worse that problem gets.
> 
> So no, that's a definite argument *for* chained handers.

Errm, why do you double account them ? The accounting goes to
different irq numbers, but we could exclude them from accounting and
showing up in proc/interrupts easily.
 
> >  - lack of affinity setting via standard interfaces
> 
> Don't want affinity for them, as setting their affinity means that
> you then end up forcing the affinity for the sub-interrupts too.
> How you do cope with the high-level interrupt having affinity to
> CPU0 but a lower level interrupt being affine to CPU1 only?
> 
> It's non-sensible, and is broken.  So no, again this isn't an
> argument for not using chained handlers.  It's an argument *for*
> them.

Well, moving the whole group to a particular cpu is sensible and the
sub interrupts don't have a set_affinity function anyway as they are
always called on the cpu on which the primary interrupt is handled.

> Sorry, but I think this stuff is right, and chained handlers like
> these have their place.

I'm not against chained handlers per se. I'm against creating a
necessarity to make a chained handler deal with various different
underlying primary irq chips and their oddities. That's simply wrong
and broken.

I don't say you have to use the existing flow handlers, if they are
too heavy weight for your purpose, but pushing conditional flow
handling into the chained handler is violating all principles of
layering.

The underlying primary chip implementation knows about the flow
requirements, so it should install the correct handler so the chained
handler just needs to deal with the sub interrupts which it knows how
to handle.

So if you don't want accounting and affinity setting (which I think is
bad), then we still can solve it in an elegant and simple way.

1) Primary chip installs a rudimentary flow handler which deals with
   the requirements of the chip.

   primary_handler_eoi()
   {
	if (desc->action)
		action();
	chip->irq_eoi();
   }

   primary_handler_level()
   {
	chip->irq_mask();
	if (desc->action)
		action();
	chip->irq_unmask();
   }

   The interupt descriptor is marked IRQ_PRIMARY or whatever

2) Demux handler is installed via:

   setup_irq(PRIMARY_IRQ, IRQ_DEMUX, demux_handler);

That's a few lines in the core code to make this work with the same
functionality than your current solution, but avoiding to touch any
demux handler when you change the underlying irq chip
implementation. It simply just works because the demux handler is
agnostic of the primary chip.

Thanks,

	tglx

[PATCH 3/6] ARM: tegra: update GPIO chained IRQ handler to use EOI in parent chip

[Sergei Shtylyov]

Hello.

On 28-02-2011 16:33, Will Deacon wrote:

> The chained GPIO IRQ handler on Tegra calls ->irq_ack on the parent
> chip prior to handling the interrupt.

> This patch updates the code to use ->irq_eoi now that the GIC has moved
> to using the fasteoi flow model.

> Acked-by: Colin Cross<ccross@android.com>
> Signed-off-by: Will Deacon<will.deacon@arm.com>
> ---
>   arch/arm/mach-tegra/gpio.c |   17 +----------------
>   1 files changed, 1 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/gpio.c b/arch/arm/mach-tegra/gpio.c
> index ad80488..5019b01 100644
> --- a/arch/arm/mach-tegra/gpio.c
> +++ b/arch/arm/mach-tegra/gpio.c
> @@ -219,9 +219,6 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>   	struct tegra_gpio_bank *bank;
>   	int port;
>   	int pin;
> -	int unmasked = 0;
> -
> -	desc->irq_data.chip->irq_ack(&desc->irq_data);

    Won't this code break after the first patch as it removes irq_ack() 
method? I.e. shouldn't the patches be combined to keep them bisectable?

WBR, Sergei

[PATCH 4/6] ARM: s5pv310: update IRQ combiner to use EOI in parent chip

[Sergei Shtylyov]

On 28-02-2011 16:33, Will Deacon wrote:

> The IRQ combiner code invokes the ->irq_{un}mask routines of the parent
> chip.

> This patch updates the cascaded handler to use EOI now that the GIC has
> moved to using the fasteoi flow model.

> Tested-by: Kyungmin Park<kyungmin.park@samsung.com>
> Signed-off-by: Will Deacon<will.deacon@arm.com>
> ---
>   arch/arm/mach-s5pv310/irq-combiner.c |    7 ++-----
>   1 files changed, 2 insertions(+), 5 deletions(-)

> diff --git a/arch/arm/mach-s5pv310/irq-combiner.c b/arch/arm/mach-s5pv310/irq-combiner.c
> index 1ea4a9e..24d5604 100644
> --- a/arch/arm/mach-s5pv310/irq-combiner.c
> +++ b/arch/arm/mach-s5pv310/irq-combiner.c
> @@ -59,9 +59,6 @@ static void combiner_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
>   	unsigned int cascade_irq, combiner_irq;
>   	unsigned long status;
>
> -	/* primary controller ack'ing */
> -	chip->irq_ack(&desc->irq_data);
> -

    Same question about bisectability here...

WBR, Sergei

[PATCH 3/6] ARM: tegra: update GPIO chained IRQ handler to use EOI in parent chip

[Will Deacon]

Sergei,

> On 28-02-2011 16:33, Will Deacon wrote:
> 
> > The chained GPIO IRQ handler on Tegra calls ->irq_ack on the parent
> > chip prior to handling the interrupt.
> 
> > This patch updates the code to use ->irq_eoi now that the GIC has moved
> > to using the fasteoi flow model.
> 
> > Acked-by: Colin Cross<ccross@android.com>
> > Signed-off-by: Will Deacon<will.deacon@arm.com>
> > ---
> >   arch/arm/mach-tegra/gpio.c |   17 +----------------
> >   1 files changed, 1 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm/mach-tegra/gpio.c b/arch/arm/mach-tegra/gpio.c
> > index ad80488..5019b01 100644
> > --- a/arch/arm/mach-tegra/gpio.c
> > +++ b/arch/arm/mach-tegra/gpio.c
> > @@ -219,9 +219,6 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> >   	struct tegra_gpio_bank *bank;
> >   	int port;
> >   	int pin;
> > -	int unmasked = 0;
> > -
> > -	desc->irq_data.chip->irq_ack(&desc->irq_data);
> 
>     Won't this code break after the first patch as it removes irq_ack()
> method? I.e. shouldn't the patches be combined to keep them bisectable?

If you read the cover letter, I said:

> This is going to be a PITA to merge so any thoughts relating to that
> are welcome.

I'm aware of the bisecting issues and Russell has come up with a solution
using entry/exit functions to sort this out (with the GIC changes moved to
the end of the series).

But we have bigger fish to fry.

The whole question about whether or not chained handlers should be written
with multiple parent chips in mind is a hot topic. Thomas has some interesting
ideas for the core code which will allow the chained handler not to care about
flow control. Once we've sorted out the mechanism for this stuff, the merging
should be fairly straightforward because the chip details will be irrelevant.

Will

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Russell King - ARM Linux]

On Tue, Mar 01, 2011 at 11:57:48AM +0100, Thomas Gleixner wrote:
> On Mon, 28 Feb 2011, Russell King - ARM Linux wrote:
> 
> > On Mon, Feb 28, 2011 at 08:16:25PM +0100, Thomas Gleixner wrote:
> > > So what's the gain of a barebone chained handler over a regular
> > > interrupt:
> > > 
> > >  - 100 instructions less
> > >  - lack of statistics
> > 
> > We don't want statistics.  Don't care how many times we go to look
> > at the interrupt controller - and actually it's really wrong to count
> > them in the first place.  Counting them means that you double-count
> > these interrupt events, and the more layers of interrupt controllers
> > there are the worse that problem gets.
> > 
> > So no, that's a definite argument *for* chained handers.
> 
> Errm, why do you double account them ? The accounting goes to
> different irq numbers, but we could exclude them from accounting and
> showing up in proc/interrupts easily.

We don't want the parent interrupt counted because the interrupt sum
which is exported from the kernel via the various statistics interface
will be screwed by this.  For N levels of classical interrupt handlers,
causing I interrupts, you end up with I*N interrupts reported.

So if you have a 100Hz interrupt coming from a timer on a 2nd level IRQ
controller, you'll end up seeing a 200Hz interrupt rate, which is quite
obviously wrong.

> > Don't want affinity for them, as setting their affinity means that
> > you then end up forcing the affinity for the sub-interrupts too.
> > How you do cope with the high-level interrupt having affinity to
> > CPU0 but a lower level interrupt being affine to CPU1 only?
> > 
> > It's non-sensible, and is broken.  So no, again this isn't an
> > argument for not using chained handlers.  It's an argument *for*
> > them.
> 
> Well, moving the whole group to a particular cpu is sensible and the
> sub interrupts don't have a set_affinity function anyway as they are
> always called on the cpu on which the primary interrupt is handled.

Not quite correct - there's systems with two GICs chained one after each
other.  The second GIC will not have a NULL set_affinity function.
However, it is meaningless to set the affinity for the second GIC as it
is not multi-CPU aware.

> > Sorry, but I think this stuff is right, and chained handlers like
> > these have their place.
> 
> I'm not against chained handlers per se. I'm against creating a
> necessarity to make a chained handler deal with various different
> underlying primary irq chips and their oddities. That's simply wrong
> and broken.

That wasn't a problem before genirq came along and invented many more
different ways for 'flow' handlers to call into the lower level IRQ
specific code.

> I don't say you have to use the existing flow handlers, if they are
> too heavy weight for your purpose, but pushing conditional flow
> handling into the chained handler is violating all principles of
> layering.

Chained handlers *are* a *direct* replacement for flow handlers because
the normal flow handler is *meaningless* for chained handlers.  Trying
to separate them into different layers is a mistake in itself.

Chained handlers require the parent to behave differently.  Eg, in the
case of a level IRQ input, you don't want IRQs to be masked just because
we're in the middle of servicing a down-stream interrupt.  You want it
left enabled so that if the ultimate device handler decides it needs to
have IRQs enabled, then all interrupts are serviced in a fair manner.

If you mask the parent interrupt, then only primary level interrupts
get serviced and fairness goes out the window.

Maybe you never got that point when you decided to create genirq from
my work, but that's *your* problem, not mine.  And it seems you're
hell bent on breaking this for ARM.  So, I say again - if you persist
with this, I'll replace the ARM IRQ implementation and stop using genirq
because you're taking it in a direction which will cause problems for
ARM.  And I *really* mean that.

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Thomas Gleixner]

On Tue, 1 Mar 2011, Russell King - ARM Linux wrote:
> On Tue, Mar 01, 2011 at 11:57:48AM +0100, Thomas Gleixner wrote:
> > On Mon, 28 Feb 2011, Russell King - ARM Linux wrote:
> > 
> > > On Mon, Feb 28, 2011 at 08:16:25PM +0100, Thomas Gleixner wrote:
> > > > So what's the gain of a barebone chained handler over a regular
> > > > interrupt:
> > > > 
> > > >  - 100 instructions less
> > > >  - lack of statistics
> > > 
> > > We don't want statistics.  Don't care how many times we go to look
> > > at the interrupt controller - and actually it's really wrong to count
> > > them in the first place.  Counting them means that you double-count
> > > these interrupt events, and the more layers of interrupt controllers
> > > there are the worse that problem gets.
> > > 
> > > So no, that's a definite argument *for* chained handers.
> > 
> > Errm, why do you double account them ? The accounting goes to
> > different irq numbers, but we could exclude them from accounting and
> > showing up in proc/interrupts easily.
> 
> We don't want the parent interrupt counted because the interrupt sum
> which is exported from the kernel via the various statistics interface
> will be screwed by this.  For N levels of classical interrupt handlers,
> causing I interrupts, you end up with I*N interrupts reported.
> 
> So if you have a 100Hz interrupt coming from a timer on a 2nd level IRQ
> controller, you'll end up seeing a 200Hz interrupt rate, which is quite
> obviously wrong.

Fair enough.
 
> > > Don't want affinity for them, as setting their affinity means that
> > > you then end up forcing the affinity for the sub-interrupts too.
> > > How you do cope with the high-level interrupt having affinity to
> > > CPU0 but a lower level interrupt being affine to CPU1 only?
> > > 
> > > It's non-sensible, and is broken.  So no, again this isn't an
> > > argument for not using chained handlers.  It's an argument *for*
> > > them.
> > 
> > Well, moving the whole group to a particular cpu is sensible and the
> > sub interrupts don't have a set_affinity function anyway as they are
> > always called on the cpu on which the primary interrupt is handled.
> 
> Not quite correct - there's systems with two GICs chained one after each
> other.  The second GIC will not have a NULL set_affinity function.
> However, it is meaningless to set the affinity for the second GIC as it
> is not multi-CPU aware.

Right. It's pointless for the secondary chip, but seeting the affinity
of the primary interrupt can be still useful..
 
> > > Sorry, but I think this stuff is right, and chained handlers like
> > > these have their place.
> > 
> > I'm not against chained handlers per se. I'm against creating a
> > necessarity to make a chained handler deal with various different
> > underlying primary irq chips and their oddities. That's simply wrong
> > and broken.
> 
> That wasn't a problem before genirq came along and invented many more
> different ways for 'flow' handlers to call into the lower level IRQ
> specific code.

It added merily two new flow handlers. One of them is fasteoi, which
was not necessary in the original ARM implementation. So where is the
point?

> > I don't say you have to use the existing flow handlers, if they are
> > too heavy weight for your purpose, but pushing conditional flow
> > handling into the chained handler is violating all principles of
> > layering.
> 
> Chained handlers *are* a *direct* replacement for flow handlers because
> the normal flow handler is *meaningless* for chained handlers.  Trying
> to separate them into different layers is a mistake in itself.

Did you even look at the separation I suggested?
 
> Chained handlers require the parent to behave differently.  Eg, in the
> case of a level IRQ input, you don't want IRQs to be masked just because
> we're in the middle of servicing a down-stream interrupt.  You want it
> left enabled so that if the ultimate device handler decides it needs to
> have IRQs enabled, then all interrupts are serviced in a fair manner.
> 
> If you mask the parent interrupt, then only primary level interrupts
> get serviced and fairness goes out the window.

Errm. I did never say that we disable the parent interrupt by any
means except when the chained handler explicitely wants to do that,
which is pretty much pointlesss nowadays, as we run all interrupt
handlers with interrupts disabled.

> Maybe you never got that point when you decided to create genirq from
> my work, but that's *your* problem, not mine.  And it seems you're

I pretty well understand the reasoning behind it and you know that.

Sigh, I explained it more than once: The chained handlers as they are,
are completely fine and not going away.

The only point where we disagree is an implementation detail:

The problem at hand which started this discussion wants to have
chained handlers which are able to deal with different underlying
primary irq chips.

My point is that having a chained handler which does:

   if (chip->irq_ack)
	chip->irq_ack();

   demux_interrupts();

   if (chip->irq_eoi)
	chip->irq_eoi();

or whatever abnominations you come up with is wrong.

Such a demux handler should be agnostic of the underlying chip to
avoid wreckage like the one which is currently discussed which is
caused by a change to the underlying primary chip.

So my take on this is to have very simplistic primary flow handlers
which are selected by the primary chip implementation to allow the
underlying demux handler to be agnostic of the primary chip. They
don't have anything to do with the normal handle_irq_* flow handlers
and should be explicitely named different. They even should be
implemented in the arch/ chip implmentation and not be part of the
generic handlers as long as they are not identified as repeated
patterns.

Can you please take the time and explain me the difference of the
following:

irqchip1.c

struct irq_chip1;

handle_primary_irq(int irq, struct irq_desc *desc)
{
	chip->irq_ack();
	desc->demux();
}

init()
{
	irq_set_chip(PRIMARY_IRQ, &irq_chip1);
	irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq);
}

irqchip2.c

struct irq_chip2;

handle_primary_irq(int irq, struct irq_desc *desc)
{
	desc->demux();
	chip->irq_eoi();
}

init()
{
	irq_set_chip(PRIMARY_IRQ, &irq_chip2);
	irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq);
}

demux.c

demux_handler(...)
{
	for_each_secondary_irq(irq)
		generic_handle_irq(irq);
}

init()
{
	irq_set_demux_handler(PRIMARY_IRQ, demux_handler);
}

versus:

irqchip1.c

struct irq_chip1;

init()
{
	irq_set_chip(PRIMARY_IRQ, &irq_chip1);
}

irqchip2.c

struct irq_chip2;

init()
{
	irq_set_chip(PRIMARY_IRQ, &irq_chip2);
}

demux.c

demux_handler(...)
{
	if (chip->irq_ack())
		chip->irq_ack();

	for_each_secondary_irq(irq)
		generic_handle_irq(irq);

	if (chip->irq_eoi())
		chip->irq_eoi();
}

init()
{
	irq_set_chained_handler(PRIMARY_IRQ, demux_handler);
}

Both are functionally equivivalent except that

     - my solution adds an indirect call

     - your solution requires explicit knowledge about the underlying
       possible primary irq chips

Now go to the above examples and add irq_chip3.c which requires
another different method of treating the primary interrupt. Lets
assume irq_chip3 is some oddball hardware.

My solution will look like this:

irqchip3.c

struct irq_chip3;

handle_primary_irq(int irq, struct irq_desc *desc)
{
	fiddle_with_some_special_register();
	desc->demux();
	chip->irq_eoi();
}

init()
{
	irq_set_chip(PRIMARY_IRQ, &irq_chip3);
	irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq);
}

Yours will look like:

irqchip3.c

struct irq_chip3;

init()
{
	irq_set_chip(PRIMARY_IRQ, &irq_chip3);
}

And change all affected demux handlers to:

demux_handler(...)
{
	if (running_on_irq_chip3)
		fiddle_with_some_special_register();
		
	if (chip->irq_ack())
		chip->irq_ack();

	for_each_secondary_irq(irq)
		generic_handle_irq(irq);

	if (chip->irq_eoi())
		chip->irq_eoi();
}

If you are still convinced that adding that extra stuff to the demux
handlers is the right way to go, I'm not in the way. I merily tried to
provide a generic solution to this, which avoid adding all this extra
knowledge into the demux handlers.

> hell bent on breaking this for ARM.  So, I say again - if you persist
> with this, I'll replace the ARM IRQ implementation and stop using genirq
> because you're taking it in a direction which will cause problems for
> ARM.  And I *really* mean that.

You are not threatening me with that. You are threatening your ARM
users as they rely on much more than the f*cking chained handler
details.

Thanks,

	tglx

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Russell King - ARM Linux]

On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote:
> Can you please take the time and explain me the difference of the
> following:

Can you please take the time to realise that sometimes the combination
between the primary and secondary interrupt controller is so tied that
this kind of thing can't happen?

Sometimes the secondary interrupt controller *must* mask the parent
interrupt because it has no masking capability of its own.

Sometimes the secondary interrupt controller *must* cause the primary
controller to ack the interrupt status *before* the secondary handler
reads the status.

You can not say "the primary controller behaves like X so we always do
X" because the action required on the primary is a combination of how
the primary behaves *and* how the secondary behaves.

Consider for example an edge triggered interrupt connected to a secondary
controller - yes, we have that combination.  When you read the secondary
status register and clear those interrupts which are pending, the
secondary controller resets to inactive its output.  If there's still
pending interrupts, it re-asserts the output causing a new edge.

If your primary 'flow' handler were to acknowledge the interrupt *after*
the demux, you'd lose interrupts.

However, if the very same primary interrupt controller is connected to
a FPGA based oring of several interrupt sources, this has to ack before
reading the status, read the status, process *all* interrupts, re-ack,
re-read the status and repeat until the status register indicates no
further interrupts are pending.

You can't deal with these two situations if you tie all the primary
flow handling outside of the secondary demux handler.  And it's
*wrong* to do so.  The behaviour required for the primary controller
inherently depends on the secondary controller.

It may not be clean from a software point of view, but that's hardware
for you, and we have to make this work.  You *can't* do that by
separating the primary flow from the demux.

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Thomas Gleixner]

On Tue, 1 Mar 2011, Russell King - ARM Linux wrote:

> On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote:
> > Can you please take the time and explain me the difference of the
> > following:
> 
> Can you please take the time to realise that sometimes the combination
> between the primary and secondary interrupt controller is so tied that
> this kind of thing can't happen?
> 
> Sometimes the secondary interrupt controller *must* mask the parent
> interrupt because it has no masking capability of its own.
> 
> Sometimes the secondary interrupt controller *must* cause the primary
> controller to ack the interrupt status *before* the secondary handler
> reads the status.
> 
> You can not say "the primary controller behaves like X so we always do
> X" because the action required on the primary is a combination of how
> the primary behaves *and* how the secondary behaves.
> 
> Consider for example an edge triggered interrupt connected to a secondary
> controller - yes, we have that combination.  When you read the secondary
> status register and clear those interrupts which are pending, the
> secondary controller resets to inactive its output.  If there's still
> pending interrupts, it re-asserts the output causing a new edge.
> 
> If your primary 'flow' handler were to acknowledge the interrupt *after*
> the demux, you'd lose interrupts.
> 
> However, if the very same primary interrupt controller is connected to
> a FPGA based oring of several interrupt sources, this has to ack before
> reading the status, read the status, process *all* interrupts, re-ack,
> re-read the status and repeat until the status register indicates no
> further interrupts are pending.
> 
> You can't deal with these two situations if you tie all the primary
> flow handling outside of the secondary demux handler.  And it's
> *wrong* to do so.  The behaviour required for the primary controller
> inherently depends on the secondary controller.
> 
> It may not be clean from a software point of view, but that's hardware
> for you, and we have to make this work.  You *can't* do that by
> separating the primary flow from the demux.

You are talking about very specific cases, where the demux handler is
tighlty integrated into the primary chip implementation. No discussion
about that. I do not want to change anything there. Period.

The problem which caused that discussion has nothing to do with the
above. It's about bog standard nested controllers which do not have a
tight coupling to the the primary chip. But changing the primary chip
from ack based to eoi based requires to touch multiple demux handlers
while a primary chip agnostic solution for these cases would avoid
that. W/O any negative side effects.

Your oddball FPGA example has nothing to do with that and can happily
use the existing model. Such a demux handler will never have to deal
with different primary chips.

Thanks,

	tglx

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Russell King - ARM Linux]

On Wed, Mar 02, 2011 at 12:44:05AM +0100, Thomas Gleixner wrote:
> The problem which caused that discussion has nothing to do with the
> above. It's about bog standard nested controllers which do not have a
> tight coupling to the the primary chip. But changing the primary chip
> from ack based to eoi based requires to touch multiple demux handlers
> while a primary chip agnostic solution for these cases would avoid
> that. W/O any negative side effects.

If you think its soo damned simple, maybe you should put the effort in
and fix all this up, get it tested as fully working on the affected
platforms, and show the patches then.

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Russell King - ARM Linux]

On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote:
> Errm. I did never say that we disable the parent interrupt by any
> means except when the chained handler explicitely wants to do that,
> which is pretty much pointlesss nowadays, as we run all interrupt
> handlers with interrupts disabled.

And that's now why some platforms struggle to work, and we're having
to bodge around this - like the ARM platforms with MMC support.  Like
some other platforms where having IRQs disabled during IDE prevents
interrupts being recevied for long periods of time (longer than the
100Hz tick period).

I *violently* disagree with the direction that genirq is heading.  It's
*actively* breaking stuff.  What's really annoying is that problems like
the above I did point out, but you seem happy to completely ignore them.
The result is that more and more ARM platforms *are* becoming utterly
useless, or requiring additional complexity being shoved into subsystems
to cope with this crap.

What we need is a *decent* IRQ support system.  Not something created out
of religious arguments which is what we have now.

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Thomas Gleixner]

On Wed, 2 Mar 2011, Russell King - ARM Linux wrote:

> On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote:
> > Errm. I did never say that we disable the parent interrupt by any
> > means except when the chained handler explicitely wants to do that,
> > which is pretty much pointlesss nowadays, as we run all interrupt
> > handlers with interrupts disabled.
> 
> And that's now why some platforms struggle to work, and we're having
> to bodge around this - like the ARM platforms with MMC support.  Like

Whats the problem for MMC?

> some other platforms where having IRQs disabled during IDE prevents
> interrupts being recevied for long periods of time (longer than the
> 100Hz tick period).

That was discussed to death already and the general agreement was that
those handlers should either enable interrupts themself, when it's
required, or being converted to threaded handlers. An interrupt
handler or any other code section which runs more than 10ms with
interrupts disabled is a bug by definition.

> I *violently* disagree with the direction that genirq is heading.  It's
> *actively* breaking stuff.  What's really annoying is that problems like
> the above I did point out, but you seem happy to completely ignore them.
> The result is that more and more ARM platforms *are* becoming utterly
> useless, or requiring additional complexity being shoved into subsystems
> to cope with this crap.
> 
> What we need is a *decent* IRQ support system.  Not something created out
> of religious arguments which is what we have now.
 
I'm not religious about it, at least not more than you with your total
refusement to distinguish between special case oddball FPGA demux and
bog standard functional irq chips.

Thanks,

	tglx

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Will Deacon]

Hi Thomas,

> Can you please take the time and explain me the difference of the
> following:
> 
> irqchip1.c
> 
> struct irq_chip1;
> 
> handle_primary_irq(int irq, struct irq_desc *desc)
> {
> 	chip->irq_ack();
> 	desc->demux();
> }
> 
> init()
> {
> 	irq_set_chip(PRIMARY_IRQ, &irq_chip1);
> 	irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq);
> }

I think with this approach you get the exact opposite problem; that
is the primary irq_chip doesn't know which IRQs are going to be
demuxed so it cannot know at init time which IRQs need their primary 
handler set. Is the idea that you set_primary_handler for all IRQs,
stash that in the descriptor somewhere and then replace handle_irq
with the primary handler when a demux handler is registered?

I guess I'm missing something here,

Cheers,

Will

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Thomas Gleixner]

On Wed, 2 Mar 2011, Will Deacon wrote:

> Hi Thomas,
> 
> > Can you please take the time and explain me the difference of the
> > following:
> > 
> > irqchip1.c
> > 
> > struct irq_chip1;
> > 
> > handle_primary_irq(int irq, struct irq_desc *desc)
> > {
> > 	chip->irq_ack();
> > 	desc->demux();
> > }
> > 
> > init()
> > {
> > 	irq_set_chip(PRIMARY_IRQ, &irq_chip1);
> > 	irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq);
> > }
> 
> I think with this approach you get the exact opposite problem; that
> is the primary irq_chip doesn't know which IRQs are going to be
> demuxed so it cannot know at init time which IRQs need their primary 
> handler set. Is the idea that you set_primary_handler for all IRQs,
> stash that in the descriptor somewhere and then replace handle_irq
> with the primary handler when a demux handler is registered?
> 
> I guess I'm missing something here,

No, I missed that. Darn, yes this would need storing the
primary_handler in some separate pointer or having a callback into the
chip implementation to make this fully distangled.

Grmbl.

	tglx

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Russell King - ARM Linux]

On Wed, Mar 02, 2011 at 10:25:02AM +0100, Thomas Gleixner wrote:
> On Wed, 2 Mar 2011, Russell King - ARM Linux wrote:
> > On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote:
> > > Errm. I did never say that we disable the parent interrupt by any
> > > means except when the chained handler explicitely wants to do that,
> > > which is pretty much pointlesss nowadays, as we run all interrupt
> > > handlers with interrupts disabled.
> > 
> > And that's now why some platforms struggle to work, and we're having
> > to bodge around this - like the ARM platforms with MMC support.  Like
> 
> Whats the problem for MMC?

The FIFO interrupt gets completely buggered by USB interrupts such that
we can't read data fast enough.  With the old IRQF_DISABLED system,
the FIFO interrupt was marked IRQF_DISABLED as it was *fast* and needed
not to be interrupted.  Other interrupts didn't have IRQF_DISABLED set.

That means such interrupts can be handled while other longer interrupts
are in progress.  All that's gone out of the window now that all IRQ
handlers are run with IRQs permanently disabled.

And when this kind of behaviour becomes system specific (eg, the USB
device driver doesn't have this problem on x86 so why should it change)
then it all becomes impossible.  There's hardware modifications in
progress to add deeper FIFOs (which need to be extended to something
idiotic like 512 32-bit words to work around the problem) and adaptive
clocking schemes which retry transfers at slower clock rates in the hope
that the IRQ handler can keep up.

It's a complete mess.

Another example - a platform I've worked on requires tight timings from
I2C in order for the CPU not to be spontaneously rebooted.  A timeout
on I2C will cause this.  We managed to get this platform to work reliably
by using IRQF_DISABLED and having the standard behaviour (IRQs enabled
during other interrupts).  Now - god knows if it'll work reliably anymore,
my guess is that with latest kernels it'll be impossibly unreliable.

> > some other platforms where having IRQs disabled during IDE prevents
> > interrupts being recevied for long periods of time (longer than the
> > 100Hz tick period).
> 
> That was discussed to death already and the general agreement was that
> those handlers should either enable interrupts themself, when it's
> required, or being converted to threaded handlers. An interrupt
> handler or any other code section which runs more than 10ms with
> interrupts disabled is a bug by definition.

I haven't noticed PATA getting this support.  So how do platforms force
device drivers which turn out to be problematical to conform to their
timing issues when there wasn't a problem with previous kernels?

> > I *violently* disagree with the direction that genirq is heading.  It's
> > *actively* breaking stuff.  What's really annoying is that problems like
> > the above I did point out, but you seem happy to completely ignore them.
> > The result is that more and more ARM platforms *are* becoming utterly
> > useless, or requiring additional complexity being shoved into subsystems
> > to cope with this crap.
> > 
> > What we need is a *decent* IRQ support system.  Not something created out
> > of religious arguments which is what we have now.
>  
> I'm not religious about it, at least not more than you with your total
> refusement to distinguish between special case oddball FPGA demux and
> bog standard functional irq chips.

Clearly you don't want to know about the problems all this stuff is
causing.  Maybe you like being responsible for breaking the most ARM
platforms with your changes - are you trying for an entry in the
Guiness Book of World Records for this, because I think that's where
you're heading.

I don't agree with the distinction that you're trying to make.  It only
works for a simple subset of cases - but it seems you're overall attitude
is that you only care about a small subset of easy cases and to hell
with everything else.

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Thomas Gleixner]

On Wed, 2 Mar 2011, Russell King - ARM Linux wrote:
> On Wed, Mar 02, 2011 at 10:25:02AM +0100, Thomas Gleixner wrote:
> > On Wed, 2 Mar 2011, Russell King - ARM Linux wrote:
> > > On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote:
> > > > Errm. I did never say that we disable the parent interrupt by any
> > > > means except when the chained handler explicitely wants to do that,
> > > > which is pretty much pointlesss nowadays, as we run all interrupt
> > > > handlers with interrupts disabled.
> > > 
> > > And that's now why some platforms struggle to work, and we're having
> > > to bodge around this - like the ARM platforms with MMC support.  Like
> > 
> > Whats the problem for MMC?
> 
> The FIFO interrupt gets completely buggered by USB interrupts such that
> we can't read data fast enough.  With the old IRQF_DISABLED system,
> the FIFO interrupt was marked IRQF_DISABLED as it was *fast* and needed
> not to be interrupted.  Other interrupts didn't have IRQF_DISABLED set.
> 
> That means such interrupts can be handled while other longer interrupts
> are in progress.  All that's gone out of the window now that all IRQ
> handlers are run with IRQs permanently disabled.
> 
> And when this kind of behaviour becomes system specific (eg, the USB
> device driver doesn't have this problem on x86 so why should it change)

I wouldn't say that. USB is known to be a trouble spot even on
x86. And Linus argued for enforcing IRQ_DISABLED to weed out such
crap. Anything which runs longer than a few micro seconds in the hard
interrupt context needs to be fixed.

> I haven't noticed PATA getting this support.  So how do platforms force
> device drivers which turn out to be problematical to conform to their
> timing issues when there wasn't a problem with previous kernels?

As I said above. PATA was talked about going threaded for years, but
nobody cared as the irq enabled vs. IRQ_DISABLED mode made things
magically work most of the time, except when you added some other
random driver, which required IRQ_DISABLED as well and spent a
millisecond and more in its handler. I know that pain and I wasted
enough time with the wreckage caused by long running handlers.

So at some point we had to make a decision:

Either let the status quo, which is not satisfying at all and causes
headache left and right, or create a situation which enforces that
shit gets cleaned up.

The general consensus was that enforcing the shit to be cleaned up is
the better long term strategy even when it causes some short term
irritation.

If that hits ARM full force, sorry, but it's not my lonely decision in
order to get an Guinness Book entry.

I assume, you know which drivers are responsible for this kind of
wreckage. Could you provide a list?

> > > I *violently* disagree with the direction that genirq is heading.  It's
> > > *actively* breaking stuff.  What's really annoying is that problems like
> > > the above I did point out, but you seem happy to completely ignore them.
> > > The result is that more and more ARM platforms *are* becoming utterly
> > > useless, or requiring additional complexity being shoved into subsystems
> > > to cope with this crap.
> > > 
> > > What we need is a *decent* IRQ support system.  Not something created out
> > > of religious arguments which is what we have now.
> >  
> > I'm not religious about it, at least not more than you with your total
> > refusement to distinguish between special case oddball FPGA demux and
> > bog standard functional irq chips.
> 
> Clearly you don't want to know about the problems all this stuff is
> causing.  Maybe you like being responsible for breaking the most ARM

I want to hear about problems and I'm the least person who refuses to
help. And you are very well aware of that.

Let's take it the other way around. Do you want to know about the pain
of maintaining generic infrastructure like this and being blocked to
change a few things in the core code just because nobody is talking to
you and takes it for granted, that fiddling in the guts of the generic
code by circumventing it (and I'm not talking about chained handlers
here) is a good idea, just because the header files are public
accessible via include/linux ?

> platforms with your changes - are you trying for an entry in the
> Guiness Book of World Records for this, because I think that's where
> you're heading.

Didn't know that they have a category for that. Can you point me to
the website where to apply?

> I don't agree with the distinction that you're trying to make.  It only
> works for a simple subset of cases - but it seems you're overall attitude
> is that you only care about a small subset of easy cases and to hell
> with everything else.

That's not true at all. I said more than once that I understand why
you want to keep the original concept of those chained handlers and
I'm not going to change anything about them. I saw it a bit too
simplistic at the beginning of the discussion and admitted that and
corrected my POV right away.

I'm merily trying to help and provide infrastructure for repeating
patterns. If those patterns happen to be the simple ones, then we
still want to avoid copied and duplicated code all over the place.

Again, I'm not trying to kill the current chained handlers at all
neither do I believe that everything is nail because we have a
hammer. I'm trying to understand the problems instead of being fixated
on a method which turned out to solve one of them.

Thanks,

	tglx

[PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip

[Will Deacon]

Hi Thomas,

> > > irqchip1.c
> > >
> > > struct irq_chip1;
> > >
> > > handle_primary_irq(int irq, struct irq_desc *desc)
> > > {
> > > 	chip->irq_ack();
> > > 	desc->demux();
> > > }
> > >
> > > init()
> > > {
> > > 	irq_set_chip(PRIMARY_IRQ, &irq_chip1);
> > > 	irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq);
> > > }
> >
> > I think with this approach you get the exact opposite problem; that
> > is the primary irq_chip doesn't know which IRQs are going to be
> > demuxed so it cannot know at init time which IRQs need their primary
> > handler set. Is the idea that you set_primary_handler for all IRQs,
> > stash that in the descriptor somewhere and then replace handle_irq
> > with the primary handler when a demux handler is registered?
> >
> > I guess I'm missing something here,
> 
> No, I missed that. Darn, yes this would need storing the
> primary_handler in some separate pointer or having a callback into the
> chip implementation to make this fully distangled.

You could remove the need for pointer switching by having a bitmap
indicating the flow control supported by the primary chip and then
having a single primary handler implementation which checks the map.
Not especially nice but it does allow for demux handlers to exist
where they are sufficient.

Will