ARM: gic: fix irq_alloc_descs handling for sparse irq

Quoted 7 lines · Rob Herring

arch/arm/common/gic.c +1 -1

@@ -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);
 

arch/arm/common/gic.c int __init gic_of_init(struct device_node *node, struct device_node *parent) · line 670

Quoted 2 lines

arch/arm/common/gic.c +0 -0

 	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.

From: Rob Herring <[email protected]>

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 <[email protected]>

---
v2:
?- make irq_start signed

Quoted 13 lines · Rob Herring

From: Rob Herring <[email protected]>

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.

Quoted 7 lines

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

Signed-off-by Rob Herring <[email protected]>

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

arch/arm/mach-shmobile/board-ag5evm.c +0 -0

21:45:24.000000000 +0900

arch/arm/mach-shmobile/board-ag5evm.c +1 -0

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

arch/arm/kernel/irq.c +1 -1

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