[PATCH] of/irq: Use interrupts-extended to find parent

[PATCH] of/irq: Use interrupts-extended to find parent

[Samuel Holland]

Some OF irqchips, such as the RISC-V PLIC, use interrupts-extended to
specify their parent domain(s). That binding does not allow using the
interrupt-parent property in the irqchip node, which prevents
of_irq_init from properly detecting the irqchip hierarchy.

If no interrupt-parent property is present in the enclosing bus or root
node, then desc->interrupt_parent will be NULL for both the per-CPU
RISC-V INTCs (the actual root domains) and the RISC-V PLIC. Similarly,
if the bus or root node specifies `interrupt-parent = <&plic>`, then
of_irq_init will hit the `desc->interrupt_parent == np` check, and again
all parents will be NULL. So things happen to work today for some boards
due to Makefile ordering.

However, things break when another irqchip ("foo") is stacked on top of
the PLIC. The bus/root node will have `interrupt-parent = <&foo>`,
since that is what all of the other peripherals need. When of_irq_init
runs, it will try to find the PLIC's parent domain. But because
of_irq_find_parent ignores interrupts-extended, it will fall back to
using the interrupt-parent property of the PLIC's parent node (i.e. the
bus or root node), and see "foo" as the PLIC's parent domain. But this
is wrong, because "foo" is actually the PLIC's child domain!

So of_irq_init wrongly attempts to init the stacked irqchip before the
PLIC. This fails and breaks boot.

Fix this by having of_irq_find_parent return the first node referenced
by interrupts-extended when that property is present. Even if the
property references multiple different IRQ domains, this will still work
reliably in of_irq_init as long as all referenced domains are the same
distance away from some root domain (e.g. the RISC-V INTCs referenced by
the PLIC's interrupts-extended are always all root domains).

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/of/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 2b07677a386b..0c20e22b91f5 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -60,7 +60,8 @@ struct device_node *of_irq_find_parent(struct device_node *child)
 		return NULL;
 
 	do {
-		if (of_property_read_u32(child, "interrupt-parent", &parent)) {
+		if (of_property_read_u32(child, "interrupt-parent", &parent) &&
+		    of_property_read_u32(child, "interrupts-extended", &parent)) {
 			p = of_get_parent(child);
 		} else	{
 			if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
-- 
2.33.1

Re: [PATCH] of/irq: Use interrupts-extended to find parent

[Marc Zyngier]

On Mon, 14 Feb 2022 05:13:17 +0000,
Samuel Holland <samuel@sholland.org> wrote:
> 
> Some OF irqchips, such as the RISC-V PLIC, use interrupts-extended to
> specify their parent domain(s). That binding does not allow using the
> interrupt-parent property in the irqchip node, which prevents
> of_irq_init from properly detecting the irqchip hierarchy.

Is this because there is more than a single interrupt parent?

> 
> If no interrupt-parent property is present in the enclosing bus or root
> node, then desc->interrupt_parent will be NULL for both the per-CPU
> RISC-V INTCs (the actual root domains) and the RISC-V PLIC. Similarly,
> if the bus or root node specifies `interrupt-parent = <&plic>`, then
> of_irq_init will hit the `desc->interrupt_parent == np` check, and again
> all parents will be NULL. So things happen to work today for some boards
> due to Makefile ordering.
> 
> However, things break when another irqchip ("foo") is stacked on top of
> the PLIC. The bus/root node will have `interrupt-parent = <&foo>`,
> since that is what all of the other peripherals need. When of_irq_init
> runs, it will try to find the PLIC's parent domain. But because
> of_irq_find_parent ignores interrupts-extended, it will fall back to
> using the interrupt-parent property of the PLIC's parent node (i.e. the
> bus or root node), and see "foo" as the PLIC's parent domain. But this
> is wrong, because "foo" is actually the PLIC's child domain!

Let me see if I parsed this correctly. You have:

             int-parent        int-extended
	foo -----------> PLIC --------------> root-irqchip

Is that correct?

> 
> So of_irq_init wrongly attempts to init the stacked irqchip before the
> PLIC. This fails and breaks boot.
> 
> Fix this by having of_irq_find_parent return the first node referenced
> by interrupts-extended when that property is present. Even if the
> property references multiple different IRQ domains, this will still work
> reliably in of_irq_init as long as all referenced domains are the same
> distance away from some root domain (e.g. the RISC-V INTCs referenced by
> the PLIC's interrupts-extended are always all root domains).

I'm a bit worried that the distance assumption may not always hold.
Maybe it isn't something we need to deal with right now, but a comment
wouldn't hurt to make this assumption clear.

>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/of/irq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 2b07677a386b..0c20e22b91f5 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -60,7 +60,8 @@ struct device_node *of_irq_find_parent(struct device_node *child)
>  		return NULL;
>  
>  	do {
> -		if (of_property_read_u32(child, "interrupt-parent", &parent)) {
> +		if (of_property_read_u32(child, "interrupt-parent", &parent) &&
> +		    of_property_read_u32(child, "interrupts-extended", &parent)) {
>  			p = of_get_parent(child);
>  		} else	{
>  			if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)

With the comment added:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] of/irq: Use interrupts-extended to find parent

[Samuel Holland]

Hi,

On 2/14/22 4:17 AM, Marc Zyngier wrote:
> On Mon, 14 Feb 2022 05:13:17 +0000,
> Samuel Holland <samuel@sholland.org> wrote:
>>
>> Some OF irqchips, such as the RISC-V PLIC, use interrupts-extended to
>> specify their parent domain(s). That binding does not allow using the
>> interrupt-parent property in the irqchip node, which prevents
>> of_irq_init from properly detecting the irqchip hierarchy.
> 
> Is this because there is more than a single interrupt parent?

If you're asking why the PLIC binding specifies interrupts-extended, yes, that's
because there is a separate RISC-V INTC instance for each CPU, so the PLIC has
as many parent domains as there are CPUs.

>> If no interrupt-parent property is present in the enclosing bus or root
>> node, then desc->interrupt_parent will be NULL for both the per-CPU
>> RISC-V INTCs (the actual root domains) and the RISC-V PLIC. Similarly,
>> if the bus or root node specifies `interrupt-parent = <&plic>`, then
>> of_irq_init will hit the `desc->interrupt_parent == np` check, and again
>> all parents will be NULL. So things happen to work today for some boards
>> due to Makefile ordering.
>>
>> However, things break when another irqchip ("foo") is stacked on top of
>> the PLIC. The bus/root node will have `interrupt-parent = <&foo>`,
>> since that is what all of the other peripherals need. When of_irq_init
>> runs, it will try to find the PLIC's parent domain. But because
>> of_irq_find_parent ignores interrupts-extended, it will fall back to
>> using the interrupt-parent property of the PLIC's parent node (i.e. the
>> bus or root node), and see "foo" as the PLIC's parent domain. But this
>> is wrong, because "foo" is actually the PLIC's child domain!
> 
> Let me see if I parsed this correctly. You have:
> 
>              int-parent        int-extended
> 	foo -----------> PLIC --------------> root-irqchip
> 
> Is that correct?

Yes.

>>
>> So of_irq_init wrongly attempts to init the stacked irqchip before the
>> PLIC. This fails and breaks boot.
>>
>> Fix this by having of_irq_find_parent return the first node referenced
>> by interrupts-extended when that property is present. Even if the
>> property references multiple different IRQ domains, this will still work
>> reliably in of_irq_init as long as all referenced domains are the same
>> distance away from some root domain (e.g. the RISC-V INTCs referenced by
>> the PLIC's interrupts-extended are always all root domains).
> 
> I'm a bit worried that the distance assumption may not always hold.
> Maybe it isn't something we need to deal with right now, but a comment
> wouldn't hurt to make this assumption clear.

OK, I will add a comment to v2.

Regards,
Samuel

>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/of/irq.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 2b07677a386b..0c20e22b91f5 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -60,7 +60,8 @@ struct device_node *of_irq_find_parent(struct device_node *child)
>>  		return NULL;
>>  
>>  	do {
>> -		if (of_property_read_u32(child, "interrupt-parent", &parent)) {
>> +		if (of_property_read_u32(child, "interrupt-parent", &parent) &&
>> +		    of_property_read_u32(child, "interrupts-extended", &parent)) {
>>  			p = of_get_parent(child);
>>  		} else	{
>>  			if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
> 
> With the comment added:
> 
> Acked-by: Marc Zyngier <maz@kernel.org>
> 
> 	M.
>