Re: [PATCH] irqchip/riscv-aplic: Register syscore operations only once

[Thomas Gleixner <tglx@kernel.org>]

On Mon, Mar 09 2026 at 16:11, liu wrote:
> From: Jessica Liu <liu.xuemei1@zte.com.cn>
>
> We can have multiple APLIC instances so setup global state

s/We/A system/

Also which global state are you referring to? 

> and resgister syscore operations only once.

register

You also fail to describe what the problem is caused by registering the
syscore ops more than once. 

> Fixes: 95a8ddde3660 ("irqchip/riscv-aplic: Preserve APLIC states across suspend/resume")
> Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
> ---
>  drivers/irqchip/irq-riscv-aplic-main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
> index 4495ca26abf5..8299e0da6577 100644
> --- a/drivers/irqchip/irq-riscv-aplic-main.c
> +++ b/drivers/irqchip/irq-riscv-aplic-main.c
> @@ -20,6 +20,7 @@
>  #include "irq-riscv-aplic-main.h"
>
>  static LIST_HEAD(aplics);
> +static bool aplic_global_setup_done __ro_after_init;

Again. That global state reference is confusing at best.

>  static void aplic_restore_states(struct aplic_priv *priv)
>  {
> @@ -375,8 +376,10 @@ static int aplic_probe(struct platform_device *pdev)
>  	if (rc)
>  		dev_err_probe(dev, rc, "failed to setup APLIC in %s mode\n",
>  			      msi_mode ? "MSI" : "direct");
> -	else
> +	else if (!aplic_global_setup_done) {
>  		register_syscore(&aplic_syscore);
> +		aplic_global_setup_done = true;
> +	}

Aside of violating the bracket rules, this code is broken already in a
very subtle way:

If the probe fails then it should not invoke
acpi_dev_clear_dependencies() either.

Something like the below makes it entirely clear what this is about.

The fix for the probe fail vs. acpi dependencies obviously wants to be
split out into a separate patch.

Thanks,

        tglx
---
--- a/drivers/irqchip/irq-riscv-aplic-main.c
+++ b/drivers/irqchip/irq-riscv-aplic-main.c
@@ -116,6 +116,16 @@ static struct syscore aplic_syscore = {
 	.ops = &aplic_syscore_ops,
 };
 
+static bool aplic_syscore_registered __ro_after_init;
+
+static void aplic_syscore_init(void);
+{
+	if (!aplic_syscore_registered) {
+		register_syscore(&aplic_syscore);
+		aplic_syscore_registered = true;
+	}
+}
+
 static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)
 {
 	struct aplic_priv *priv = container_of(nb, struct aplic_priv, genpd_nb);
@@ -372,18 +382,20 @@ static int aplic_probe(struct platform_d
 		rc = aplic_msi_setup(dev, regs);
 	else
 		rc = aplic_direct_setup(dev, regs);
-	if (rc)
+
+	if (rc) {
 		dev_err_probe(dev, rc, "failed to setup APLIC in %s mode\n",
 			      msi_mode ? "MSI" : "direct");
-	else
-		register_syscore(&aplic_syscore);
+		return rc;
+	}
+
+	aplic_syscore_register();
 
 #ifdef CONFIG_ACPI
 	if (!acpi_disabled)
 		acpi_dev_clear_dependencies(ACPI_COMPANION(dev));
 #endif
-
-	return rc;
+	return 0;
 }
 
 static const struct of_device_id aplic_match[] = {


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