Message-ID: <1bce34c6-c2d0-4b2b-a8f9-7bc8737702b1@sifive.com>
Date: Fri, 15 Nov 2024 09:57:00 -0600
From: Samuel Holland <samuel.holland@...ive.com>
To: Anup Patel <apatel@...tanamicro.com>
Cc: Anup Patel <anup@...infault.org>, Thomas Gleixner <tglx@...utronix.de>,
 Albert Ou <aou@...s.berkeley.edu>, Björn Töpel
 <bjorn@...osinc.com>, Palmer Dabbelt <palmer@...belt.com>,
 Paul Walmsley <paul.walmsley@...ive.com>, linux-kernel@...r.kernel.org,
 linux-riscv@...ts.infradead.org
Subject: Re: [PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

Hi Anup,

On 2024-11-15 9:42 AM, Anup Patel wrote:
> On Fri, Nov 15, 2024 at 1:31 AM Samuel Holland
> <samuel.holland@...ive.com> wrote:
>>
>> If the APLIC driver is probed before the IMSIC driver, the parent MSI
>> domain will be missing, which causes a NULL pointer dereference in
>> msi_create_device_irq_domain(). Avoid this by deferring probe until the
>> parent MSI domain is available. Use dev_err_probe() to avoid printing an
>> error message when returning -EPROBE_DEFER.
> 
> The -EPROBE_DEFER is not needed because we expect that platforms to
> use "msi-parent" DT property in APLIC DT node which in-turn allows Linux
> DD framework to re-order probing based on fw_devlink dependencies. The
> APLIC DT bindings mandates that any of "interrupt-extended" or "msi-parent"
> DT properties MUST be present.
> 
> Can you elaborate a bit more on how you are hitting this issue ?

I agree that fw_devlink should help avoid the situation where we need to return
-EPROBE_DEFER, but the kernel must still not crash even if fw_devlink is
disabled (which is a perfectly valid thing to do: "fw_devlink=off" on the kernel
command line) or if fw_devlink fails to come up with the ideal probe order.
fw_devlink is an optimization. It should not be relied on for correctness. In my
specific case, fw_devlink got the order wrong due to some false dependency
cycles, which I sent a patch for separately[1].

Regards,
Samuel

[1]:
https://lore.kernel.org/lkml/20241114195652.3068725-1-samuel.holland@sifive.com/

>> Fixes: ca8df97fe679 ("irqchip/riscv-aplic: Add support for MSI-mode")
>> Signed-off-by: Samuel Holland <samuel.holland@...ive.com>
>> ---
>>
>>  drivers/irqchip/irq-riscv-aplic-main.c | 3 ++-
>>  drivers/irqchip/irq-riscv-aplic-msi.c  | 3 +++
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
>> index 900e72541db9..93e7c51f944a 100644
>> --- a/drivers/irqchip/irq-riscv-aplic-main.c
>> +++ b/drivers/irqchip/irq-riscv-aplic-main.c
>> @@ -207,7 +207,8 @@ static int aplic_probe(struct platform_device *pdev)
>>         else
>>                 rc = aplic_direct_setup(dev, regs);
>>         if (rc)
>> -               dev_err(dev, "failed to setup APLIC in %s mode\n", msi_mode ? "MSI" : "direct");
>> +               dev_err_probe(dev, rc, "failed to setup APLIC in %s mode\n",
>> +                             msi_mode ? "MSI" : "direct");
>>
>>  #ifdef CONFIG_ACPI
>>         if (!acpi_disabled)
>> diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c
>> index 945bff28265c..fb8d1838609f 100644
>> --- a/drivers/irqchip/irq-riscv-aplic-msi.c
>> +++ b/drivers/irqchip/irq-riscv-aplic-msi.c
>> @@ -266,6 +266,9 @@ int aplic_msi_setup(struct device *dev, void __iomem *regs)
>>                         if (msi_domain)
>>                                 dev_set_msi_domain(dev, msi_domain);
>>                 }
>> +
>> +               if (!dev_get_msi_domain(dev))
>> +                       return -EPROBE_DEFER;
>>         }
>>
>>         if (!msi_create_device_irq_domain(dev, MSI_DEFAULT_DOMAIN, &aplic_msi_template,
>> --
>> 2.45.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists