[PATCH] ARM: gic: fix irq_alloc_descs handling for sparse irq

[PATCH] ARM: gic: fix irq_alloc_descs handling for sparse irq

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Commit "ARM: gic: add irq_domain support" (2071a2a4b8ed5292) broke SPARSE_IRQ
on platforms with GIC. When SPARSE_IRQ is enabled, all NR_IRQS or
mach_desc->nr_irqs will be allocated by arch_probe_nr_irqs(). This caused
irq_alloc_descs to allocate irq_descs after the pre-allocated space.

Make irq_alloc_descs search for an exact irq range and assume it has
been pre-allocated on failure. For DT probing dynamic allocation is used.
DT enabled platforms should set their nr_irqs to NR_IRQ_LEGACY and have all
irq_chips allocate their irq_descs with irq_alloc_descs if SPARSE_IRQ is
enabled.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/common/gic.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 65cf39d..8ab24a5 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -24,6 +24,7 @@
  */
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <linux/list.h>
 #include <linux/smp.h>
 #include <linux/cpu_pm.h>
@@ -571,7 +572,8 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
 	if (gic_nr == 0) {
 		gic_cpu_base_addr = cpu_base;
 		domain->hwirq_base = 16;
-		irq_start = (irq_start & ~31) + 16;
+		if (irq_start > 0)
+			irq_start = (irq_start & ~31) + 16;
 	} else
 		domain->hwirq_base = 32;
 
@@ -586,8 +588,13 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
 	gic->gic_irqs = gic_irqs;
 
 	domain->nr_irq = gic_irqs - domain->hwirq_base;
-	domain->irq_base = irq_alloc_descs(-1, irq_start, domain->nr_irq,
+	domain->irq_base = irq_alloc_descs(irq_start, 16, domain->nr_irq,
 					   numa_node_id());
+	if (IS_ERR_VALUE(domain->irq_base)) {
+		WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
+		     irq_start);
+		domain->irq_base = irq_start;
+	}
 	domain->priv = gic;
 	domain->ops = &gic_irq_domain_ops;
 	irq_domain_add(domain);
@@ -657,7 +664,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent)
 
 	domain->of_node = of_node_get(node);
 
-	gic_init(gic_cnt, 16, dist_base, cpu_base);
+	gic_init(gic_cnt, -1, dist_base, cpu_base);
 
 	if (parent) {
 		irq = irq_of_parse_and_map(node, 0);
-- 
1.7.5.4

[PATCH] ARM: gic: fix irq_alloc_descs handling for sparse irq

[Russell King - ARM Linux]

On Sat, Oct 22, 2011 at 03:20:08PM -0500, Rob Herring wrote:
> @@ -657,7 +664,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent)
>  
>  	domain->of_node = of_node_get(node);
>  
> -	gic_init(gic_cnt, 16, dist_base, cpu_base);
> +	gic_init(gic_cnt, -1, dist_base, cpu_base);
>  
>  	if (parent) {
>  		irq = irq_of_parse_and_map(node, 0);

You don't explain this change - '16' is used here to skip the SGI
interrupts which will never be passed to the generic IRQ subsystem
from the GIC.

Moreover, the second parameter is an unsigned integer, not a signed
integer.

And not only that, but:

        gic->irq_offset = (irq_start - 1) & ~31;

means that irq_offset ends up being -2 & ~31, or -32.  Do you really
want the PPIs to generate IRQ numbers from -16 to -1 ?  It doesn't stop
there:

        for (i = irq_start; i < irq_limit; i++) {
                irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq);
                irq_set_chip_data(i, gic);
                set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
        }

This will start from -1 to irq_limit (-1 + number of GIC IRQs).

Basically, -1 is not legal here - 1 is the minimum valid value that
this function takes for proper operation - but that's just wasteful, so
16 is the realistic minimum value.

[PATCH] ARM: gic: fix irq_alloc_descs handling for sparse irq

[Rob Herring]

Russell,

On 10/22/2011 03:36 PM, Russell King - ARM Linux wrote:
> On Sat, Oct 22, 2011 at 03:20:08PM -0500, Rob Herring wrote:
>> @@ -657,7 +664,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent)
>>  
>>  	domain->of_node = of_node_get(node);
>>  
>> -	gic_init(gic_cnt, 16, dist_base, cpu_base);
>> +	gic_init(gic_cnt, -1, dist_base, cpu_base);
>>  
>>  	if (parent) {
>>  		irq = irq_of_parse_and_map(node, 0);
> 
> You don't explain this change - '16' is used here to skip the SGI
> interrupts which will never be passed to the generic IRQ subsystem
> from the GIC.

This makes irq_alloc_descs do dynamic IRQ# assignment rather than
allocating at the specified IRQ#.

> 
> Moreover, the second parameter is an unsigned integer, not a signed
> integer.
> 
> And not only that, but:
> 
>         gic->irq_offset = (irq_start - 1) & ~31;
> 
> means that irq_offset ends up being -2 & ~31, or -32.  Do you really
> want the PPIs to generate IRQ numbers from -16 to -1 ?  It doesn't stop
> there:
> 
>         for (i = irq_start; i < irq_limit; i++) {
>                 irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq);
>                 irq_set_chip_data(i, gic);
>                 set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
>         }
> 
> This will start from -1 to irq_limit (-1 + number of GIC IRQs).
> 
> Basically, -1 is not legal here - 1 is the minimum valid value that
> this function takes for proper operation - but that's just wasteful, so
> 16 is the realistic minimum value.

You're not looking at the right version of gic code. This is the basis
of this patch which has changes to the lines you reference:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-September/067588.html

You are right about irq_start needing to be signed.

Rob

[PATCH v2] ARM: gic: fix irq_alloc_descs handling for sparse irq

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Commit "ARM: gic: add irq_domain support" (2071a2a4b8ed5292) broke SPARSE_IRQ
on platforms with GIC. When SPARSE_IRQ is enabled, all NR_IRQS or
mach_desc->nr_irqs will be allocated by arch_probe_nr_irqs(). This caused
irq_alloc_descs to allocate irq_descs after the pre-allocated space.

Make irq_alloc_descs search for an exact irq range and assume it has
been pre-allocated on failure. For DT probing dynamic allocation is used.
DT enabled platforms should set their nr_irqs to NR_IRQ_LEGACY and have all
irq_chips allocate their irq_descs with irq_alloc_descs if SPARSE_IRQ is
enabled.

gic_init irq_start param is changed to be signed with negative meaning do
dynamic Linux irq assigment.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
v2:
 - make irq_start signed

 arch/arm/common/gic.c               |   15 +++++++++++----
 arch/arm/include/asm/hardware/gic.h |    2 +-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 65cf39d..e9debd4 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -24,6 +24,7 @@
  */
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <linux/list.h>
 #include <linux/smp.h>
 #include <linux/cpu_pm.h>
@@ -550,7 +551,7 @@ const struct irq_domain_ops gic_irq_domain_ops = {
 #endif
 };
 
-void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
+void __init gic_init(unsigned int gic_nr, int irq_start,
 	void __iomem *dist_base, void __iomem *cpu_base)
 {
 	struct gic_chip_data *gic;
@@ -571,7 +572,8 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
 	if (gic_nr == 0) {
 		gic_cpu_base_addr = cpu_base;
 		domain->hwirq_base = 16;
-		irq_start = (irq_start & ~31) + 16;
+		if (irq_start > 0)
+			irq_start = (irq_start & ~31) + 16;
 	} else
 		domain->hwirq_base = 32;
 
@@ -586,8 +588,13 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
 	gic->gic_irqs = gic_irqs;
 
 	domain->nr_irq = gic_irqs - domain->hwirq_base;
-	domain->irq_base = irq_alloc_descs(-1, irq_start, domain->nr_irq,
+	domain->irq_base = irq_alloc_descs(irq_start, 16, domain->nr_irq,
 					   numa_node_id());
+	if (IS_ERR_VALUE(domain->irq_base)) {
+		WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
+		     irq_start);
+		domain->irq_base = irq_start;
+	}
 	domain->priv = gic;
 	domain->ops = &gic_irq_domain_ops;
 	irq_domain_add(domain);
@@ -657,7 +664,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent)
 
 	domain->of_node = of_node_get(node);
 
-	gic_init(gic_cnt, 16, dist_base, cpu_base);
+	gic_init(gic_cnt, -1, dist_base, cpu_base);
 
 	if (parent) {
 		irq = irq_of_parse_and_map(node, 0);
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 1a776a1..0ea4122 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -38,7 +38,7 @@
 extern void __iomem *gic_cpu_base_addr;
 extern struct irq_chip gic_arch_extn;
 
-void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
+void gic_init(unsigned int, int, void __iomem *, void __iomem *);
 int gic_of_init(struct device_node *node, struct device_node *parent);
 void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
-- 
1.7.5.4

[PATCH v2] ARM: gic: fix irq_alloc_descs handling for sparse irq

[Magnus Damm]

On Mon, Oct 24, 2011 at 11:36 PM, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@calxeda.com>
>
> Commit "ARM: gic: add irq_domain support" (2071a2a4b8ed5292) broke SPARSE_IRQ
> on platforms with GIC. When SPARSE_IRQ is enabled, all NR_IRQS or
> mach_desc->nr_irqs will be allocated by arch_probe_nr_irqs(). This caused
> irq_alloc_descs to allocate irq_descs after the pre-allocated space.
>
> Make irq_alloc_descs search for an exact irq range and assume it has
> been pre-allocated on failure. For DT probing dynamic allocation is used.
> DT enabled platforms should set their nr_irqs to NR_IRQ_LEGACY and have all
> irq_chips allocate their irq_descs with irq_alloc_descs if SPARSE_IRQ is
> enabled.
>
> gic_init irq_start param is changed to be signed with negative meaning do
> dynamic Linux irq assigment.
>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
> v2:
> ?- make irq_start signed

Hi Rob,

[Added Paul Mundt as CC]

Thanks for your work on improving the GIC code. I tested latest 3.2-rc
on some of our GIC platforms without DT earlier today and I ran into
some issues that I believe are related to the following commit:

f37a53c ARM: gic: fix irq_alloc_descs handling for sparse irq

With this patch included I get the following warning on SH-Mobile
AG5EVM and Kota2:

NR_IRQS:1024 nr_irqs:1024 1024
------------[ cut here ]------------
WARNING: at arch/arm/common/gic.c:607 gic_init+0x90/0x2e4()
Cannot allocate irq_descs @ IRQ16, assuming pre-allocated
[<c000c868>] (unwind_backtrace+0x0/0xe0) from [<c001857c>] (warn_slowpath_commo)
[<c001857c>] (warn_slowpath_common+0x48/0x60) from [<c00185d8>] (warn_slowpath_)
[<c00185d8>] (warn_slowpath_fmt+0x2c/0x3c) from [<c029ee08>] (gic_init+0x90/0x2)
[<c029ee08>] (gic_init+0x90/0x2e4) from [<c029f278>] (sh73a0_init_irq+0x30/0x18)
[<c029f278>] (sh73a0_init_irq+0x30/0x184) from [<c029c0b4>] (init_IRQ+0x14/0x1c)
[<c029c0b4>] (init_IRQ+0x14/0x1c) from [<c029a5cc>] (start_kernel+0x15c/0x2b8)
[<c029a5cc>] (start_kernel+0x15c/0x2b8) from [<4000803c>] (0x4000803c)
---[ end trace 1b75b31a2719ed1c ]---

The sh73a0 used on the AG5EVM board has a bunch of interrupt
controllers, but the GIC is initialized first.

As you can see by the printout above "nr_irqs" is set to 1024, but I
believe this may be a misconfiguration on my side. Or perhaps it's the
ARM default that needs to be adjusted, because it's certainly not
behaving like other architectures such as x86 and SH.

The x86 and SH implementations of sparse IRQ seem to return
NR_IRQS_LEGACY from arch_probe_nr_irqs(), but on ARM we default to
NR_IRQS unless the machine descriptor gives a different hint.

I could easily "fix" my platform by making it behave like x86 and SH:
--- 0001/arch/arm/mach-shmobile/board-ag5evm.c
+++ work/arch/arm/mach-shmobile/board-ag5evm.c	2011-11-14
21:45:24.000000000 +0900
@@ -607,6 +607,7 @@ struct sys_timer ag5evm_timer = {

 MACHINE_START(AG5EVM, "ag5evm")
 	.map_io		= ag5evm_map_io,
+	.nr_irqs	= NR_IRQS_LEGACY,
 	.init_irq	= sh73a0_init_irq,
 	.handle_irq	= shmobile_handle_irq_gic,
 	.init_machine	= ag5evm_init,

The above code makes sure that only 16 interrupts are reserved early
on, so the warning disappears and the nr_irqs is different:
NR_IRQS:1024 nr_irqs:16 16

I do however wonder if the ARM default is sane. Perhaps it is, but
just to experiment I decided to modify the common ARM code like this:
--- 0001/arch/arm/kernel/irq.c
+++ work/arch/arm/kernel/irq.c	2011-11-14 22:06:02.000000000 +0900
@@ -130,7 +130,7 @@ void __init init_IRQ(void)
 int __init arch_probe_nr_irqs(void)
 {
 	nr_irqs = machine_desc->nr_irqs ? machine_desc->nr_irqs : NR_IRQS;
-	return nr_irqs;
+	return NR_IRQS_LEGACY;
 }
 #endif

With the patch above the warning printout is disappeared and the
nr_irqs message is slightly different:
NR_IRQS:1024 nr_irqs:1024 16

I don't really have any strong feelings exactly how to fix this, but I
suspect that other non-DT GIC-enabled platforms will run into the same
problem unless they set .nr_irqs in their machine descriptor table.

Any ideas?

Thanks,

/ magnus

[PATCH v2] ARM: gic: fix irq_alloc_descs handling for sparse irq

[Rob Herring]

Magnus,

On 11/14/2011 07:19 AM, Magnus Damm wrote:
> On Mon, Oct 24, 2011 at 11:36 PM, Rob Herring <robherring2@gmail.com> wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> Commit "ARM: gic: add irq_domain support" (2071a2a4b8ed5292) broke SPARSE_IRQ
>> on platforms with GIC. When SPARSE_IRQ is enabled, all NR_IRQS or
>> mach_desc->nr_irqs will be allocated by arch_probe_nr_irqs(). This caused
>> irq_alloc_descs to allocate irq_descs after the pre-allocated space.
>>
>> Make irq_alloc_descs search for an exact irq range and assume it has
>> been pre-allocated on failure. For DT probing dynamic allocation is used.
>> DT enabled platforms should set their nr_irqs to NR_IRQ_LEGACY and have all
>> irq_chips allocate their irq_descs with irq_alloc_descs if SPARSE_IRQ is
>> enabled.
>>
>> gic_init irq_start param is changed to be signed with negative meaning do
>> dynamic Linux irq assigment.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> ---
>> v2:
>>  - make irq_start signed
> 
> Hi Rob,
> 
> [Added Paul Mundt as CC]
> 
> Thanks for your work on improving the GIC code. I tested latest 3.2-rc
> on some of our GIC platforms without DT earlier today and I ran into
> some issues that I believe are related to the following commit:
> 
> f37a53c ARM: gic: fix irq_alloc_descs handling for sparse irq
> 
> With this patch included I get the following warning on SH-Mobile
> AG5EVM and Kota2:
> 
> NR_IRQS:1024 nr_irqs:1024 1024
> ------------[ cut here ]------------
> WARNING: at arch/arm/common/gic.c:607 gic_init+0x90/0x2e4()
> Cannot allocate irq_descs @ IRQ16, assuming pre-allocated
> [<c000c868>] (unwind_backtrace+0x0/0xe0) from [<c001857c>] (warn_slowpath_commo)
> [<c001857c>] (warn_slowpath_common+0x48/0x60) from [<c00185d8>] (warn_slowpath_)
> [<c00185d8>] (warn_slowpath_fmt+0x2c/0x3c) from [<c029ee08>] (gic_init+0x90/0x2)
> [<c029ee08>] (gic_init+0x90/0x2e4) from [<c029f278>] (sh73a0_init_irq+0x30/0x18)
> [<c029f278>] (sh73a0_init_irq+0x30/0x184) from [<c029c0b4>] (init_IRQ+0x14/0x1c)
> [<c029c0b4>] (init_IRQ+0x14/0x1c) from [<c029a5cc>] (start_kernel+0x15c/0x2b8)
> [<c029a5cc>] (start_kernel+0x15c/0x2b8) from [<4000803c>] (0x4000803c)
> ---[ end trace 1b75b31a2719ed1c ]---
> 
> The sh73a0 used on the AG5EVM board has a bunch of interrupt
> controllers, but the GIC is initialized first.
> 
> As you can see by the printout above "nr_irqs" is set to 1024, but I
> believe this may be a misconfiguration on my side. Or perhaps it's the
> ARM default that needs to be adjusted, because it's certainly not
> behaving like other architectures such as x86 and SH.
> 
> The x86 and SH implementations of sparse IRQ seem to return
> NR_IRQS_LEGACY from arch_probe_nr_irqs(), but on ARM we default to
> NR_IRQS unless the machine descriptor gives a different hint.
> 

Yes, the ARM version definitely seems wrong to me.

> I could easily "fix" my platform by making it behave like x86 and SH:
> --- 0001/arch/arm/mach-shmobile/board-ag5evm.c
> +++ work/arch/arm/mach-shmobile/board-ag5evm.c	2011-11-14
> 21:45:24.000000000 +0900
> @@ -607,6 +607,7 @@ struct sys_timer ag5evm_timer = {
> 
>  MACHINE_START(AG5EVM, "ag5evm")
>  	.map_io		= ag5evm_map_io,
> +	.nr_irqs	= NR_IRQS_LEGACY,
>  	.init_irq	= sh73a0_init_irq,
>  	.handle_irq	= shmobile_handle_irq_gic,
>  	.init_machine	= ag5evm_init,
> 
> The above code makes sure that only 16 interrupts are reserved early
> on, so the warning disappears and the nr_irqs is different:
> NR_IRQS:1024 nr_irqs:16 16
> 

For now, this is the correct fix. shmobile is the only platform that
uses sparse irqs and correctly calls irq_alloc_descs.

> I do however wonder if the ARM default is sane. Perhaps it is, but
> just to experiment I decided to modify the common ARM code like this:
> --- 0001/arch/arm/kernel/irq.c
> +++ work/arch/arm/kernel/irq.c	2011-11-14 22:06:02.000000000 +0900
> @@ -130,7 +130,7 @@ void __init init_IRQ(void)
>  int __init arch_probe_nr_irqs(void)
>  {
>  	nr_irqs = machine_desc->nr_irqs ? machine_desc->nr_irqs : NR_IRQS;
> -	return nr_irqs;
> +	return NR_IRQS_LEGACY;
>  }
>  #endif
> 
> With the patch above the warning printout is disappeared and the
> nr_irqs message is slightly different:
> NR_IRQS:1024 nr_irqs:1024 16

Something like this was my initial fix, but that would pretty much break
sparse irq on every platform except shmobile. All the irq_chip
implementations need to be converted to use irqdomains and call have
calls to irq_alloc_descs (or perhaps we move that into irqdomain code).
There's around 50 implementations in arch/arm...

> 
> I don't really have any strong feelings exactly how to fix this, but I
> suspect that other non-DT GIC-enabled platforms will run into the same
> problem unless they set .nr_irqs in their machine descriptor table.
> 

I think shmobile is the only platform with sparse irq on by default with
a GIC.

Rob