From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 199FF146D53 for ; Mon, 9 Mar 2026 09:17:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773047829; cv=none; b=f5YQ9hnRk+F5yJH3ew8SzZIWbNZk3QJmE4PQS96oTY9sxQmpRJPSRPoicj74+BzkAIO2A+4X5jD3I02Zci7NM7FJxmWz0MoutlVA7fAPZYnTO2SLewi8SR6h1dB3C5FuWTupN+yL3fuVyX4RAX5iEs8/lGQI+ARfMK5W0phx6/0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773047829; c=relaxed/simple; bh=AOq78/xUyD/cvsmtwcE4UeNHcHh26M25Rpqpzr9frWQ=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=ubs36RkXFuegw7/Q5gynK2zrGHakgrU27vTTo3N0Cc+6h+TH2wIRTkcI0x0ZVA9rMF5dWAJ5ZihNf/lDfPY/V2sw5gFNPcl0/eypEoUWYi+j+gEWHHxYX6doACYtR8qgS5/SBNQwVHR6bpu1yCFPGUciX0aznp+iAli34nFWdG4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=khkcHtmT; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="khkcHtmT" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E4B4CC4CEF7; Mon, 9 Mar 2026 09:17:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1773047828; bh=AOq78/xUyD/cvsmtwcE4UeNHcHh26M25Rpqpzr9frWQ=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=khkcHtmTOmF8C3/XyMez0tQ8r3bYiKTto9pyHzI50pE8wtCHwkD/ZNJXhhJ4aJ42I 1BLardMK6qQMqYYxZxSmTPaaAdS1lCIMbQ9twYgSabLZGpAp1SGDCcjWktcKaz2Hey 57oelZJramA6H0xmmQHWCWGB33ScpJsvF4GQGHKyiogN+np4NQLbOAgo0egL/h7Jbs h9vHX+dxj5zq5wdzFoXThac23/f5BA2KL7Rtmg6FDxkrmxTZ28rjKH2J2FhZjvt3t/ HAemAp4/iUNpAHvB/grm8eDEUPo2vQ042Gx5GM4J0/diCI0t+i4ocoZ/MLxXIeqYNW PbXkbtaYvgLlw== From: Thomas Gleixner To: liu.xuemei1@zte.com.cn, anup@brainfault.org, pjw@kernel.org, palmer@dabbelt.com, aou@eecs.berkeley.edu, alex@ghiti.fr, liujingqi@lanxincomputing.com, cyan.yang@sifive.com, nick.hu@sifive.com, yongxuan.wang@sifive.com Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] irqchip/riscv-aplic: Register syscore operations only once In-Reply-To: <20260309161140133ZhM76Wt3uPliHU5_R8O0j@zte.com.cn> References: <20260309161140133ZhM76Wt3uPliHU5_R8O0j@zte.com.cn> Date: Mon, 09 Mar 2026 10:17:04 +0100 Message-ID: <87ms0hs6pb.ffs@tglx> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain On Mon, Mar 09 2026 at 16:11, liu wrote: > From: Jessica Liu > > 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 > --- > 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[] = {