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

[Rob Herring]

On Tue, Feb 15, 2022 at 08:20:39PM -0600, Samuel Holland 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.
> 
> 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).
> 
> Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> ---
> 
> Changes in v2:
>  - Add comments noting the assumptions made here
> 
>  drivers/of/irq.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 2b07677a386b..c7d14f5c4660 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -60,7 +60,12 @@ struct device_node *of_irq_find_parent(struct device_node *child)
>  		return NULL;
>  
>  	do {
> -		if (of_property_read_u32(child, "interrupt-parent", &parent)) {
> +		/*
> +		 * interrupts-extended can reference multiple parent domains.
> +		 * This only returns the first one.
> +		 */
> +		if (of_property_read_u32(child, "interrupt-parent", &parent) &&
> +		    of_property_read_u32(child, "interrupts-extended", &parent)) {
>  			p = of_get_parent(child);

of_irq_find_parent() fundamentally works with interrupt-parent. 
'Finding' the parent just doesn't make sense for 'interrupts-extended' 
because it is explicit. Other than the comment, what gets returned in 
the case of 'interrupts-extended' is ambiguous.

Also, this will walk parent nodes to find 'interrupts-extended'. While 
that's somewhat unlikely to occur, it is not what you want.

Instead, just check 'interrupts-extended' within of_irq_init() and 
then fallback to calling of_irq_find_parent(). Then the ambiguous 
nature of only looking at the 1st entry is in one place. (And more 
easily fixed if we ever need all the parents.)

Rob