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

[Marc Zyngier <maz@kernel.org>]

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.