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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c109aeb3-225e-3750-4b86-cef227d11ab5@sholland.org>
Date:   Mon, 14 Feb 2022 20:16:35 -0600
From:   Samuel Holland <samuel@...lland.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] of/irq: Use interrupts-extended to find parent

Hi,

On 2/14/22 4:17 AM, Marc Zyngier wrote:
> On Mon, 14 Feb 2022 05:13:17 +0000,
> Samuel Holland <samuel@...lland.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@...lland.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@...nel.org>
> 
> 	M.
>