[PATCH] arm: irq: Allow for specification of no preallocated irqs

[PATCH] arm: irq: Allow for specification of no preallocated irqs

[Michael Bohan]

For cases with SPARSE_IRQ enabled, irqs preallocated with
arch_probe_nr_irqs() are already marked as allocated in the
allocated_irqs bitmap. As a consequence, irq chip drivers that
allocate irqs will feel one of two behaviors:

1. An allocation will succeed with the starting irq_base one
more than the preallocated irqs. This will thus waste the
preceeding interrupt resources that were preallocated, unless a
legacy chip driver happens to assume ownership of these by some
platform definition. The GIC driver is a typical primary chip
driver, and abides to the allocation APIs. So this can be a
problem in many trivial usecases.

2. An allocation will fail with < 0. This can also happen in the
GIC driver, which interprets this value as meaning the irq_descs
are already preallocated. But in Device Tree configurations, the
fallback irq_base is -1. This results in an invalid irq_base
value.

Looking forward, we are moving towards a world where preallocation
of irqs is no longer necessary. irq_domain is scoped to handle all
irq_desc allocations in the future. Thus, we should support
configurations where the platform wants to preallocate no irqs.

One easy way to achieve this is to allow for
machine_desc->nr_irqs < 0, which indicates not to preallocate any
interrupts.

Signed-off-by: Michael Bohan <mbohan@codeaurora.org>
---
 arch/arm/include/asm/mach/arch.h |    2 +-
 arch/arm/kernel/irq.c            |   14 ++++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index d7692ca..cc6506a 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -22,7 +22,7 @@ struct machine_desc {
 	const char *const 	*dt_compat;	/* array of device tree
 						 * 'compatible' strings	*/
 
-	unsigned int		nr_irqs;	/* number of IRQs */
+	int			nr_irqs;	/* number of IRQs */
 
 #ifdef CONFIG_ZONE_DMA
 	unsigned long		dma_zone_size;	/* size of DMA-able area */
diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 3efd82c..f74b173 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -129,8 +129,18 @@ void __init init_IRQ(void)
 #ifdef CONFIG_SPARSE_IRQ
 int __init arch_probe_nr_irqs(void)
 {
-	nr_irqs = machine_desc->nr_irqs ? machine_desc->nr_irqs : NR_IRQS;
-	return nr_irqs;
+	/*
+	 * machine_desc->nr_irqs < 0 is a special case that
+	 * specifies not to preallocate any irq_descs.
+	 */
+	if (machine_desc->nr_irqs < 0) {
+		nr_irqs = 0;
+		return nr_irqs;
+	} else {
+		nr_irqs = machine_desc->nr_irqs ?
+			  machine_desc->nr_irqs : NR_IRQS;
+		return nr_irqs;
+	}
 }
 #endif
 
-- 
1.7.8.3

Re: [PATCH] arm: irq: Allow for specification of no preallocated irqs

[Russell King - ARM Linux]

On Thu, Jan 19, 2012 at 02:43:44PM -0800, Michael Bohan wrote:
> For cases with SPARSE_IRQ enabled, irqs preallocated with
> arch_probe_nr_irqs() are already marked as allocated in the
> allocated_irqs bitmap. As a consequence, irq chip drivers that
> allocate irqs will feel one of two behaviors:
> 
> 1. An allocation will succeed with the starting irq_base one
> more than the preallocated irqs. This will thus waste the
> preceeding interrupt resources that were preallocated, unless a
> legacy chip driver happens to assume ownership of these by some
> platform definition. The GIC driver is a typical primary chip
> driver, and abides to the allocation APIs. So this can be a
> problem in many trivial usecases.
> 
> 2. An allocation will fail with < 0. This can also happen in the
> GIC driver, which interprets this value as meaning the irq_descs
> are already preallocated. But in Device Tree configurations, the
> fallback irq_base is -1. This results in an invalid irq_base
> value.
> 
> Looking forward, we are moving towards a world where preallocation
> of irqs is no longer necessary. irq_domain is scoped to handle all
> irq_desc allocations in the future. Thus, we should support
> configurations where the platform wants to preallocate no irqs.

Actually, leave nr_irqs unsigned.  Even when we have no preallocation,
we do not want to allow anything to get IRQ0.  Platforms which don't
want to have any preallocated IRQs should set NR_IRQS to zero as well
as their platforms nr_irqs entry.

That's basically how it works today, so no code changes should be
necessary.

Re: [PATCH] arm: irq: Allow for specification of no preallocated irqs

[Rob Herring]

On 01/19/2012 04:43 PM, Michael Bohan wrote:
> For cases with SPARSE_IRQ enabled, irqs preallocated with
> arch_probe_nr_irqs() are already marked as allocated in the
> allocated_irqs bitmap. As a consequence, irq chip drivers that
> allocate irqs will feel one of two behaviors:
> 
> 1. An allocation will succeed with the starting irq_base one
> more than the preallocated irqs. This will thus waste the
> preceeding interrupt resources that were preallocated, unless a
> legacy chip driver happens to assume ownership of these by some
> platform definition. The GIC driver is a typical primary chip
> driver, and abides to the allocation APIs. So this can be a
> problem in many trivial usecases.
> 
> 2. An allocation will fail with < 0. This can also happen in the
> GIC driver, which interprets this value as meaning the irq_descs
> are already preallocated. But in Device Tree configurations, the
> fallback irq_base is -1. This results in an invalid irq_base
> value.
> 
> Looking forward, we are moving towards a world where preallocation
> of irqs is no longer necessary. irq_domain is scoped to handle all
> irq_desc allocations in the future. Thus, we should support
> configurations where the platform wants to preallocate no irqs.
> 
> One easy way to achieve this is to allow for
> machine_desc->nr_irqs < 0, which indicates not to preallocate any
> interrupts.
> 
> Signed-off-by: Michael Bohan <mbohan@codeaurora.org>

I don't know if you saw my recent series on NR_IRQS clean-up (Make
mach/irqs.h optional).

No doubt that arch_probe_nr_irqs is doing the wrong thing on ARM, but no
pre-allocation is not what we want either. We ultimately want
arch_probe_nr_irqs to return NR_IRQS_LEGACY (16) to reserve IRQ0 (aka
NO_IRQ) and legacy ISA IRQs. With my series, NR_IRQS is set to
NR_IRQS_LEGACY for SPARSE_IRQ. You can accomplish the same thing without
that series by setting .nr_irqs to NR_IRQS for non-DT and to
NR_IRQS_LEGACY for DT. For platforms to work in single kernel builds,
they will need to select SPARSE_IRQ.

Rob

> ---
>  arch/arm/include/asm/mach/arch.h |    2 +-
>  arch/arm/kernel/irq.c            |   14 ++++++++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
> index d7692ca..cc6506a 100644
> --- a/arch/arm/include/asm/mach/arch.h
> +++ b/arch/arm/include/asm/mach/arch.h
> @@ -22,7 +22,7 @@ struct machine_desc {
>  	const char *const 	*dt_compat;	/* array of device tree
>  						 * 'compatible' strings	*/
>  
> -	unsigned int		nr_irqs;	/* number of IRQs */
> +	int			nr_irqs;	/* number of IRQs */
>  
>  #ifdef CONFIG_ZONE_DMA
>  	unsigned long		dma_zone_size;	/* size of DMA-able area */
> diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
> index 3efd82c..f74b173 100644
> --- a/arch/arm/kernel/irq.c
> +++ b/arch/arm/kernel/irq.c
> @@ -129,8 +129,18 @@ void __init init_IRQ(void)
>  #ifdef CONFIG_SPARSE_IRQ
>  int __init arch_probe_nr_irqs(void)
>  {
> -	nr_irqs = machine_desc->nr_irqs ? machine_desc->nr_irqs : NR_IRQS;
> -	return nr_irqs;
> +	/*
> +	 * machine_desc->nr_irqs < 0 is a special case that
> +	 * specifies not to preallocate any irq_descs.
> +	 */
> +	if (machine_desc->nr_irqs < 0) {
> +		nr_irqs = 0;
> +		return nr_irqs;
> +	} else {
> +		nr_irqs = machine_desc->nr_irqs ?
> +			  machine_desc->nr_irqs : NR_IRQS;
> +		return nr_irqs;
> +	}
>  }
>  #endif
>  

Re: [PATCH] arm: irq: Allow for specification of no preallocated irqs

[Michael Bohan]

On 1/19/2012 3:04 PM, Rob Herring wrote:
> No doubt that arch_probe_nr_irqs is doing the wrong thing on ARM, but no
> pre-allocation is not what we want either. We ultimately want
> arch_probe_nr_irqs to return NR_IRQS_LEGACY (16) to reserve IRQ0 (aka
> NO_IRQ) and legacy ISA IRQs. With my series, NR_IRQS is set to
> NR_IRQS_LEGACY for SPARSE_IRQ. You can accomplish the same thing without
> that series by setting .nr_irqs to NR_IRQS for non-DT and to
> NR_IRQS_LEGACY for DT. For platforms to work in single kernel builds,
> they will need to select SPARSE_IRQ.

One issue here is that IRQ_BITMAP_BITS is defined as a function of 
NR_IRQS. Currently, there's a hack in place that arbitrarily tacks on 
8196 bits to the end, giving the max virq supported as 8212 with your 
patches. Unfortunately, the system I'm running on will require higher 
values than that, so this actually breaks me.

It seems like the right solution to this problem is to have the 
allocated_irqs bitmap expandable at runtime. Or perhaps use a different 
data structure to begin with?

Mike

Re: [PATCH] arm: irq: Allow for specification of no preallocated irqs

[Rob Herring]

On 01/19/2012 06:29 PM, Michael Bohan wrote:
> On 1/19/2012 3:04 PM, Rob Herring wrote:
>> No doubt that arch_probe_nr_irqs is doing the wrong thing on ARM, but no
>> pre-allocation is not what we want either. We ultimately want
>> arch_probe_nr_irqs to return NR_IRQS_LEGACY (16) to reserve IRQ0 (aka
>> NO_IRQ) and legacy ISA IRQs. With my series, NR_IRQS is set to
>> NR_IRQS_LEGACY for SPARSE_IRQ. You can accomplish the same thing without
>> that series by setting .nr_irqs to NR_IRQS for non-DT and to
>> NR_IRQS_LEGACY for DT. For platforms to work in single kernel builds,
>> they will need to select SPARSE_IRQ.
> 
> One issue here is that IRQ_BITMAP_BITS is defined as a function of
> NR_IRQS. Currently, there's a hack in place that arbitrarily tacks on
> 8196 bits to the end, giving the max virq supported as 8212 with your
> patches. Unfortunately, the system I'm running on will require higher
> values than that, so this actually breaks me.
> 
> It seems like the right solution to this problem is to have the
> allocated_irqs bitmap expandable at runtime. Or perhaps use a different
> data structure to begin with?

I believe the correct solution is using the radix tree in irqdomain as
Ben H suggested. Does that not work?

Rob

Re: [PATCH] arm: irq: Allow for specification of no preallocated irqs

[Grant Likely]

On Thu, Jan 19, 2012 at 04:29:43PM -0800, Michael Bohan wrote:
> On 1/19/2012 3:04 PM, Rob Herring wrote:
> >No doubt that arch_probe_nr_irqs is doing the wrong thing on ARM, but no
> >pre-allocation is not what we want either. We ultimately want
> >arch_probe_nr_irqs to return NR_IRQS_LEGACY (16) to reserve IRQ0 (aka
> >NO_IRQ) and legacy ISA IRQs. With my series, NR_IRQS is set to
> >NR_IRQS_LEGACY for SPARSE_IRQ. You can accomplish the same thing without
> >that series by setting .nr_irqs to NR_IRQS for non-DT and to
> >NR_IRQS_LEGACY for DT. For platforms to work in single kernel builds,
> >they will need to select SPARSE_IRQ.
> 
> One issue here is that IRQ_BITMAP_BITS is defined as a function of
> NR_IRQS. Currently, there's a hack in place that arbitrarily tacks
> on 8196 bits to the end, giving the max virq supported as 8212 with
> your patches. Unfortunately, the system I'm running on will require
> higher values than that, so this actually breaks me.

Are that many irqs actually going to be in-use at one time?  If not, then the driver should use the irq_domain radix tree reverse map anyway.

g.