[PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

[PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

[Samuel Holland]

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.

Fixes: ca8df97fe679 ("irqchip/riscv-aplic: Add support for MSI-mode")
Signed-off-by: Samuel Holland <samuel.holland@sifive.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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

[Anup Patel]

On Fri, Nov 15, 2024 at 1:31 AM Samuel Holland
<samuel.holland@sifive.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 ?

Regards,
Anup

>
> Fixes: ca8df97fe679 ("irqchip/riscv-aplic: Add support for MSI-mode")
> Signed-off-by: Samuel Holland <samuel.holland@sifive.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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

[Samuel Holland]

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@sifive.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@sifive.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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

[Anup Patel]

On Fri, Nov 15, 2024 at 9:27 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> 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@sifive.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].

The RISC-V kernel is heavily dependent on fw_devlink based probe ordering
and more upcoming drivers are going to increase this dependency.
For example, we also have RISC-V IOMMU driver that needs to be probed
after IMSIC since it can use MSIs.

I think we should ensure that fw_devlink can't be disabled/turned-off for the
RISC-V kernel. If this is not possible then we should have very verbose
boot-time warning when fw_devlink is disabled/turned-off.

Your other "interrupt-parent" related fix [1] looks fine to me.

Regards,
Anup

>
> 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@sifive.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@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Thomas Gleixner <tglx@linutronix.de>:

On Thu, 14 Nov 2024 12:01:30 -0800 you 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.
> 
> Fixes: ca8df97fe679 ("irqchip/riscv-aplic: Add support for MSI-mode")
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> 
> [...]

Here is the summary with links:
  - irqchip/riscv-aplic: Fix crash when MSI domain is missing
    https://git.kernel.org/riscv/c/1f181d1cda56

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv