[PATCH] of/irq: Refuse to of_irq_parse_one() more than one IRQ if #interrupt-cells = <0>

[PATCH] of/irq: Refuse to of_irq_parse_one() more than one IRQ if #interrupt-cells = <0>

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

An #interrupt-cells = <0> property may arguably be a right answer for an
interrupt controller having just one interrupt and no options to configure.
There are anyway already existing examples in the tree, both in DTs and in
the bindings.

Now the problem is that of_irq_count() called on an interrupt generating
device having one of the former controllers as parent would result in an
endless loop. It's especially unpleasant in the startup where
of_irq_count() <= ... <= of_platform_default_populate_init() will silently
hang forever (unless a watchdog bites).

Prevent others from spending the same time on debugging this by refusing to
parse more than one IRQ for such controllers.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/of/irq.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 6c843d54ebb11..b3a359c7641d3 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -381,6 +381,13 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 		goto out;
 	}
 
+	if (!intsize && index) {
+		pr_debug("%pOF trying to map IRQ %d in %pOF having #interrupt-cells = <0>\n",
+			 device, index, p);
+		res = -EINVAL;
+		goto out;
+	}
+
 	pr_debug(" parent=%pOF, intsize=%d\n", p, intsize);
 
 	/* Copy intspec into irq structure */
-- 
2.48.1

Re: [PATCH] of/irq: Refuse to of_irq_parse_one() more than one IRQ if #interrupt-cells = <0>

[Rob Herring]

On Fri, Mar 07, 2025 at 02:52:18PM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> An #interrupt-cells = <0> property may arguably be a right answer for an
> interrupt controller having just one interrupt and no options to configure.
> There are anyway already existing examples in the tree, both in DTs and in
> the bindings.

The existing examples are broken and hacks to take advantage of Linux 
implementation details (IRQCHIP_DECLARE()).

And #interrupt-cells==0 can't work with 'interrupts'.

> 
> Now the problem is that of_irq_count() called on an interrupt generating
> device having one of the former controllers as parent would result in an
> endless loop. It's especially unpleasant in the startup where
> of_irq_count() <= ... <= of_platform_default_populate_init() will silently
> hang forever (unless a watchdog bites).
> 
> Prevent others from spending the same time on debugging this by refusing to
> parse more than one IRQ for such controllers.

I'll happily take a dtschema patch to warn on 0 cells. Then you can find 
the problem at build time. I generally don't think it's the kernel's job 
to validate a DT, but if the code can handle something like this then 
that's good.

> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
>  drivers/of/irq.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 6c843d54ebb11..b3a359c7641d3 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -381,6 +381,13 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
>  		goto out;
>  	}
>  
> +	if (!intsize && index) {

Why are you checking index?

> +		pr_debug("%pOF trying to map IRQ %d in %pOF having #interrupt-cells = <0>\n",
> +			 device, index, p);
> +		res = -EINVAL;
> +		goto out;
> +	}
> +
>  	pr_debug(" parent=%pOF, intsize=%d\n", p, intsize);
>  
>  	/* Copy intspec into irq structure */
> -- 
> 2.48.1
> 

Re: [PATCH] of/irq: Refuse to of_irq_parse_one() more than one IRQ if #interrupt-cells = <0>

[Sverdlin, Alexander]

Hi Rob!

On Tue, 2025-03-11 at 12:55 -0500, Rob Herring wrote:
> > An #interrupt-cells = <0> property may arguably be a right answer for an
> > interrupt controller having just one interrupt and no options to configure.
> > There are anyway already existing examples in the tree, both in DTs and in
> > the bindings.
> 
> The existing examples are broken and hacks to take advantage of Linux 
> implementation details (IRQCHIP_DECLARE()).
> 
> And #interrupt-cells==0 can't work with 'interrupts'.

I agree.

> > Now the problem is that of_irq_count() called on an interrupt generating
> > device having one of the former controllers as parent would result in an
> > endless loop. It's especially unpleasant in the startup where
> > of_irq_count() <= ... <= of_platform_default_populate_init() will silently
> > hang forever (unless a watchdog bites).
> > 
> > Prevent others from spending the same time on debugging this by refusing to
> > parse more than one IRQ for such controllers.
> 
> I'll happily take a dtschema patch to warn on 0 cells. Then you can find 
> the problem at build time. I generally don't think it's the kernel's job 
> to validate a DT, but if the code can handle something like this then 
> that's good.
> 
> > 
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > ---
> >   drivers/of/irq.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 6c843d54ebb11..b3a359c7641d3 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -381,6 +381,13 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
> >   		goto out;
> >   	}
> >   
> > +	if (!intsize && index) {
> 
> Why are you checking index?

This way of_irq_count() gives "1" on the corresponding nodes.
Do you think that returning "-EINVAL" "if (!intsize)" would
make more sense? I'm concerned about this place because the
code doesn't even crash, but rather hangs until watchdog comes,
which means, there is no backtrace, nothing.

> > +		pr_debug("%pOF trying to map IRQ %d in %pOF having #interrupt-cells = <0>\n",
> > +			 device, index, p);
> > +		res = -EINVAL;
> > +		goto out;
> > +	}
> > +
> >   	pr_debug(" parent=%pOF, intsize=%d\n", p, intsize);
> >   
> >   	/* Copy intspec into irq structure */

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com