[PATCH 0/3] Exynos PMU fixes for cpu hotplug and cpuidle routines

[PATCH 0/3] Exynos PMU fixes for cpu hotplug and cpuidle routines

[Alexey Klimov]

This was reported by Sashiko here:
https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
and was mainly introduced by enabling cpu hotplug
support and cpuidle for gs101-based SoCs.

One patch removes strange usage of smp_processor_id() and
other patches deal with a few missing error paths issues
here and there in setup_cpuhp_and_cpuidle() and around.

Tested on gs101-raven device, I don't see any regressions
but testing from others will be appreciated.

Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
---
Alexey Klimov (3):
      soc: samsung: exynos-pmu: use target cpu ID in hotplug callbacks
      soc: samsung: exynos-pmu: fix use-after-free of interrupt generator node
      soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

 drivers/soc/samsung/exynos-pmu.c | 75 ++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 15 deletions(-)
---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260605-exynos-pmu-cpuhp-idle-fixes-32f5ed7c969f

Best regards,
-- 
Alexey Klimov <alexey.klimov@linaro.org>

[PATCH 1/3] soc: samsung: exynos-pmu: use target cpu ID in hotplug callbacks

[Alexey Klimov]

The CPU hotplug state callbacks __gs101_cpu_pmu_online() and
__gs101_cpu_pmu_offline() currently partially use smp_processor_id() to
determine the target register offset for the CPU inform hints. This may
be fine for cpuidle flow but broken for cpu hotplug where the target
cpu is passed as an argument and could be different from cpu where
that is executing (e.g. CPU 0 offlining CPU 1), meaning that
smp_processor_id() returns the id of local CPU but hotplug flow
deals with another CPU core undergoing the transition.

This causes the pmu driver to write power down and power on configuration
hints to the wrong hardware registers, messing up the power state of active
cores and failing to configure the target core. Fix this by removing the
cpuhint variable entirely and utilizing the target 'cpu' argument passed
to the callbacks by the hotplug core infrastructure.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
Fixes: 598995027b91 ("soc: samsung: exynos-pmu: enable CPU hotplug support for gs101")
Cc: stable@vger.kernel.org
Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
---
 drivers/soc/samsung/exynos-pmu.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index d58376c38179..6e635872247a 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -235,11 +235,10 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
 static int __gs101_cpu_pmu_online(unsigned int cpu)
 	__must_hold(&pmu_context->cpupm_lock)
 {
-	unsigned int cpuhint = smp_processor_id();
 	u32 reg, mask;
 
 	/* clear cpu inform hint */
-	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
+	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
 		     CPU_INFORM_CLEAR);
 
 	mask = BIT(cpu);
@@ -296,12 +295,10 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
 static int __gs101_cpu_pmu_offline(unsigned int cpu)
 	__must_hold(&pmu_context->cpupm_lock)
 {
-	unsigned int cpuhint = smp_processor_id();
 	u32 reg, mask;
 
 	/* set cpu inform hint */
-	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
-		     CPU_INFORM_C2);
+	regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu), CPU_INFORM_C2);
 
 	mask = BIT(cpu);
 	regmap_update_bits(pmu_context->pmuintrgen, GS101_GRP2_INTR_BID_ENABLE,

-- 
2.51.0

[PATCH 2/3] soc: samsung: exynos-pmu: fix use-after-free of interrupt generator node

[Alexey Klimov]

The setup_cpuhp_and_cpuidle() parses the device tree node for the
interrupt generation block via of_parse_phandle() and decrements its
reference count using of_node_put() immediately after fetching the resource
address. However, later the intr_gen_node pointer is passed into
of_syscon_register_regmap().

Fix this by moving the of_node_put() invocation to after the
of_syscon_register_regmap() call, and adding it to correct error paths.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
Fixes: 78b72897a5c8 ("soc: samsung: exynos-pmu: Enable CPU Idle for gs101")
Cc: stable@vger.kernel.org
Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
---
 drivers/soc/samsung/exynos-pmu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 6e635872247a..9636287f6794 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -428,23 +428,30 @@ static int setup_cpuhp_and_cpuidle(struct device *dev)
 	 * syscon provided regmap.
 	 */
 	ret = of_address_to_resource(intr_gen_node, 0, &intrgen_res);
-	of_node_put(intr_gen_node);
+	if (ret) {
+		of_node_put(intr_gen_node);
+		return ret;
+	}
 
 	virt_addr = devm_ioremap(dev, intrgen_res.start,
 				 resource_size(&intrgen_res));
-	if (!virt_addr)
+	if (!virt_addr) {
+		of_node_put(intr_gen_node);
 		return -ENOMEM;
+	}
 
 	pmu_context->pmuintrgen = devm_regmap_init_mmio(dev, virt_addr,
 							&regmap_pmu_intr);
 	if (IS_ERR(pmu_context->pmuintrgen)) {
 		dev_err(dev, "failed to initialize pmu-intr-gen regmap\n");
+		of_node_put(intr_gen_node);
 		return PTR_ERR(pmu_context->pmuintrgen);
 	}
 
 	/* register custom mmio regmap with syscon */
 	ret = of_syscon_register_regmap(intr_gen_node,
 					pmu_context->pmuintrgen);
+	of_node_put(intr_gen_node);
 	if (ret)
 		return ret;
 

-- 
2.51.0

[PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

[Alexey Klimov]

The setup_cpuhp_and_cpuidle() initialisation sequence currently ignores
the return values of cpuhp_setup_state(), cpu_pm_register_notifier(), and
register_reboot_notifier(). If any of these registrations fail during
probe() routine, the driver returns 0, leaving the driver partially
configured.

Furthermore, if anything after setup_cpuhp_and_cpuidle() fails in probe()
routine, for instance devm_mfd_add_devices(), the probe() lacks an error
path and leaves notifiers and cpu hotplug states registered.

Introduce variables for the cpu hotplug state IDs in exynos_pmu_context
struct, that should be initialised to CPUHP_INVALID by default. Check all
return codes in setup_cpuhp_and_cpuidle(), and add an error path to remove
registered states on failure. Finally, add destroy_cpuhp_and_cpuidle()
helper to safely tear down notifiers and cpu hotplug states.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
Fixes: 78b72897a5c8 ("soc: samsung: exynos-pmu: Enable CPU Idle for gs101")
Cc: stable@vger.kernel.org
Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
---
 drivers/soc/samsung/exynos-pmu.c | 57 ++++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 9636287f6794..846313a28e9a 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -38,6 +38,8 @@ struct exynos_pmu_context {
 	unsigned long *in_cpuhp;
 	bool sys_insuspend;
 	bool sys_inreboot;
+	int cpuhp_prepare_state;
+	int cpuhp_online_state;
 };
 
 void __iomem *pmu_base_addr;
@@ -404,6 +406,17 @@ static struct notifier_block exynos_cpupm_reboot_nb = {
 	.notifier_call = exynos_cpupm_reboot_notifier,
 };
 
+static void destroy_cpuhp_and_cpuidle(void)
+{
+	cpu_pm_unregister_notifier(&gs101_cpu_pm_notifier);
+	unregister_reboot_notifier(&exynos_cpupm_reboot_nb);
+
+	if (pmu_context->cpuhp_prepare_state != CPUHP_INVALID)
+		cpuhp_remove_state(pmu_context->cpuhp_prepare_state);
+	if (pmu_context->cpuhp_online_state != CPUHP_INVALID)
+		cpuhp_remove_state(pmu_context->cpuhp_online_state);
+}
+
 static int setup_cpuhp_and_cpuidle(struct device *dev)
 {
 	struct device_node *intr_gen_node;
@@ -465,16 +478,42 @@ static int setup_cpuhp_and_cpuidle(struct device *dev)
 		gs101_cpuhp_pmu_online(cpu);
 
 	/* register CPU hotplug callbacks */
-	cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,	"soc/exynos-pmu:prepare",
-			  gs101_cpuhp_pmu_online, NULL);
+	pmu_context->cpuhp_prepare_state = CPUHP_INVALID;
+	pmu_context->cpuhp_online_state = CPUHP_INVALID;
 
-	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
-			  NULL, gs101_cpuhp_pmu_offline);
+	ret = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "soc/exynos-pmu:prepare",
+				gs101_cpuhp_pmu_online, NULL);
+	if (ret < 0)
+		return ret;
+
+	pmu_context->cpuhp_prepare_state = ret;
+
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
+				NULL, gs101_cpuhp_pmu_offline);
+	if (ret < 0)
+		goto clean_cpuhp_states;
+
+	pmu_context->cpuhp_online_state = ret;
 
 	/* register CPU PM notifiers for cpuidle */
-	cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
-	register_reboot_notifier(&exynos_cpupm_reboot_nb);
-	return 0;
+	ret = cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
+	if (ret)
+		goto clean_cpuhp_states;
+
+	ret = register_reboot_notifier(&exynos_cpupm_reboot_nb);
+	if (!ret)
+		/* Success */
+		return ret;
+
+	cpu_pm_unregister_notifier(&gs101_cpu_pm_notifier);
+
+clean_cpuhp_states:
+	if (pmu_context->cpuhp_prepare_state != CPUHP_INVALID)
+		cpuhp_remove_state(pmu_context->cpuhp_prepare_state);
+	if (pmu_context->cpuhp_online_state != CPUHP_INVALID)
+		cpuhp_remove_state(pmu_context->cpuhp_online_state);
+
+	return ret;
 }
 
 static int exynos_pmu_probe(struct platform_device *pdev)
@@ -548,8 +587,10 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 
 	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, exynos_pmu_devs,
 				   ARRAY_SIZE(exynos_pmu_devs), NULL, 0, NULL);
-	if (ret)
+	if (ret) {
+		destroy_cpuhp_and_cpuidle();
 		return ret;
+	}
 
 	if (devm_of_platform_populate(dev))
 		dev_err(dev, "Error populating children, reboot and poweroff might not work properly\n");

-- 
2.51.0

Re: [PATCH 1/3] soc: samsung: exynos-pmu: use target cpu ID in hotplug callbacks

[Peter Griffin]

Hi Alexey,

Thanks for your patch.

On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>
> The CPU hotplug state callbacks __gs101_cpu_pmu_online() and
> __gs101_cpu_pmu_offline() currently partially use smp_processor_id() to
> determine the target register offset for the CPU inform hints. This may
> be fine for cpuidle flow but broken for cpu hotplug where the target
> cpu is passed as an argument and could be different from cpu where
> that is executing (e.g. CPU 0 offlining CPU 1), meaning that
> smp_processor_id() returns the id of local CPU but hotplug flow
> deals with another CPU core undergoing the transition.

This was intentional. The powermode hint is always programmed based on
the currently executing CPU core in the gs101 downstream code (for
both CPU Idle and CPU hotplug paths). See
https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/drivers/soc/google/cal-if/pmucal_powermode.c#15
and the pmu_intr_gen is done based on the actual CPU being
enabled/disabled.

It's possible Exynos850 requires something different. I suggest
checking the equivalent function in the e850 downstream kernel.

>
> This causes the pmu driver to write power down and power on configuration
> hints to the wrong hardware registers, messing up the power state of active
> cores and failing to configure the target core. Fix this by removing the
> cpuhint variable entirely and utilizing the target 'cpu' argument passed
> to the callbacks by the hotplug core infrastructure.

Unfortunately I think you're introducing the bug you describe with this patch.

regards,

Peter

>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
> Fixes: 598995027b91 ("soc: samsung: exynos-pmu: enable CPU hotplug support for gs101")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
> ---
>  drivers/soc/samsung/exynos-pmu.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index d58376c38179..6e635872247a 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -235,11 +235,10 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
>  static int __gs101_cpu_pmu_online(unsigned int cpu)
>         __must_hold(&pmu_context->cpupm_lock)
>  {
> -       unsigned int cpuhint = smp_processor_id();
>         u32 reg, mask;
>
>         /* clear cpu inform hint */
> -       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
> +       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
>                      CPU_INFORM_CLEAR);
>
>         mask = BIT(cpu);
> @@ -296,12 +295,10 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
>  static int __gs101_cpu_pmu_offline(unsigned int cpu)
>         __must_hold(&pmu_context->cpupm_lock)
>  {
> -       unsigned int cpuhint = smp_processor_id();
>         u32 reg, mask;
>
>         /* set cpu inform hint */
> -       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
> -                    CPU_INFORM_C2);
> +       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu), CPU_INFORM_C2);
>
>         mask = BIT(cpu);
>         regmap_update_bits(pmu_context->pmuintrgen, GS101_GRP2_INTR_BID_ENABLE,
>
> --
> 2.51.0
>

Re: [PATCH 2/3] soc: samsung: exynos-pmu: fix use-after-free of interrupt generator node

[Peter Griffin]

Hi Alexey,

Thanks for your patch.

On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>
> The setup_cpuhp_and_cpuidle() parses the device tree node for the
> interrupt generation block via of_parse_phandle() and decrements its
> reference count using of_node_put() immediately after fetching the resource
> address. However, later the intr_gen_node pointer is passed into
> of_syscon_register_regmap().
>
> Fix this by moving the of_node_put() invocation to after the
> of_syscon_register_regmap() call, and adding it to correct error paths.

I think  using
__free(device_node) = of_parse_phandle

would be a cleaner/simpler fix.

Peter




Peter.

>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
> Fixes: 78b72897a5c8 ("soc: samsung: exynos-pmu: Enable CPU Idle for gs101")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
> ---
>  drivers/soc/samsung/exynos-pmu.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 6e635872247a..9636287f6794 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -428,23 +428,30 @@ static int setup_cpuhp_and_cpuidle(struct device *dev)
>          * syscon provided regmap.
>          */
>         ret = of_address_to_resource(intr_gen_node, 0, &intrgen_res);
> -       of_node_put(intr_gen_node);
> +       if (ret) {
> +               of_node_put(intr_gen_node);
> +               return ret;
> +       }
>
>         virt_addr = devm_ioremap(dev, intrgen_res.start,
>                                  resource_size(&intrgen_res));
> -       if (!virt_addr)
> +       if (!virt_addr) {
> +               of_node_put(intr_gen_node);
>                 return -ENOMEM;
> +       }
>
>         pmu_context->pmuintrgen = devm_regmap_init_mmio(dev, virt_addr,
>                                                         &regmap_pmu_intr);
>         if (IS_ERR(pmu_context->pmuintrgen)) {
>                 dev_err(dev, "failed to initialize pmu-intr-gen regmap\n");
> +               of_node_put(intr_gen_node);
>                 return PTR_ERR(pmu_context->pmuintrgen);
>         }
>
>         /* register custom mmio regmap with syscon */
>         ret = of_syscon_register_regmap(intr_gen_node,
>                                         pmu_context->pmuintrgen);
> +       of_node_put(intr_gen_node);
>         if (ret)
>                 return ret;
>
>
> --
> 2.51.0
>

Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

[Peter Griffin]

Hi Alexey,

Thanks for your patch!

On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>
> The setup_cpuhp_and_cpuidle() initialisation sequence currently ignores
> the return values of cpuhp_setup_state(), cpu_pm_register_notifier(), and
> register_reboot_notifier(). If any of these registrations fail during
> probe() routine, the driver returns 0, leaving the driver partially
> configured.

I originally made the failure non-fatal because the system still boots
without the notifiers registered (and all other Arm64 Exynos SoCs
upstream don't register notifiers and AFAICT have broken cpu hotplug
and cpu idle).

In hindsight, that seems like a mistake. I think your patch to fully
unwind everything in case of failure makes more sense.  See small
comment below about destroy_cpuhp_and_cpuidle()

>
> Furthermore, if anything after setup_cpuhp_and_cpuidle() fails in probe()
> routine, for instance devm_mfd_add_devices(), the probe() lacks an error
> path and leaves notifiers and cpu hotplug states registered.
>
> Introduce variables for the cpu hotplug state IDs in exynos_pmu_context
> struct, that should be initialised to CPUHP_INVALID by default. Check all
> return codes in setup_cpuhp_and_cpuidle(), and add an error path to remove
> registered states on failure. Finally, add destroy_cpuhp_and_cpuidle()
> helper to safely tear down notifiers and cpu hotplug states.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
> Fixes: 78b72897a5c8 ("soc: samsung: exynos-pmu: Enable CPU Idle for gs101")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
> ---
>  drivers/soc/samsung/exynos-pmu.c | 57 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 9636287f6794..846313a28e9a 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -38,6 +38,8 @@ struct exynos_pmu_context {
>         unsigned long *in_cpuhp;
>         bool sys_insuspend;
>         bool sys_inreboot;
> +       int cpuhp_prepare_state;
> +       int cpuhp_online_state;
>  };
>
>  void __iomem *pmu_base_addr;
> @@ -404,6 +406,17 @@ static struct notifier_block exynos_cpupm_reboot_nb = {
>         .notifier_call = exynos_cpupm_reboot_notifier,
>  };
>
> +static void destroy_cpuhp_and_cpuidle(void)
> +{
> +       cpu_pm_unregister_notifier(&gs101_cpu_pm_notifier);
> +       unregister_reboot_notifier(&exynos_cpupm_reboot_nb);
> +
> +       if (pmu_context->cpuhp_prepare_state != CPUHP_INVALID)
> +               cpuhp_remove_state(pmu_context->cpuhp_prepare_state);
> +       if (pmu_context->cpuhp_online_state != CPUHP_INVALID)
> +               cpuhp_remove_state(pmu_context->cpuhp_online_state);
> +}
> +
>  static int setup_cpuhp_and_cpuidle(struct device *dev)
>  {
>         struct device_node *intr_gen_node;
> @@ -465,16 +478,42 @@ static int setup_cpuhp_and_cpuidle(struct device *dev)
>                 gs101_cpuhp_pmu_online(cpu);
>
>         /* register CPU hotplug callbacks */
> -       cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "soc/exynos-pmu:prepare",
> -                         gs101_cpuhp_pmu_online, NULL);
> +       pmu_context->cpuhp_prepare_state = CPUHP_INVALID;
> +       pmu_context->cpuhp_online_state = CPUHP_INVALID;
>
> -       cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
> -                         NULL, gs101_cpuhp_pmu_offline);
> +       ret = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "soc/exynos-pmu:prepare",
> +                               gs101_cpuhp_pmu_online, NULL);
> +       if (ret < 0)
> +               return ret;
> +
> +       pmu_context->cpuhp_prepare_state = ret;
> +
> +       ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
> +                               NULL, gs101_cpuhp_pmu_offline);
> +       if (ret < 0)
> +               goto clean_cpuhp_states;
> +
> +       pmu_context->cpuhp_online_state = ret;
>
>         /* register CPU PM notifiers for cpuidle */
> -       cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
> -       register_reboot_notifier(&exynos_cpupm_reboot_nb);
> -       return 0;
> +       ret = cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
> +       if (ret)
> +               goto clean_cpuhp_states;
> +
> +       ret = register_reboot_notifier(&exynos_cpupm_reboot_nb);
> +       if (!ret)
> +               /* Success */
> +               return ret;
> +
> +       cpu_pm_unregister_notifier(&gs101_cpu_pm_notifier);
> +
> +clean_cpuhp_states:
> +       if (pmu_context->cpuhp_prepare_state != CPUHP_INVALID)
> +               cpuhp_remove_state(pmu_context->cpuhp_prepare_state);
> +       if (pmu_context->cpuhp_online_state != CPUHP_INVALID)
> +               cpuhp_remove_state(pmu_context->cpuhp_online_state);
> +
> +       return ret;
>  }
>
>  static int exynos_pmu_probe(struct platform_device *pdev)
> @@ -548,8 +587,10 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>
>         ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, exynos_pmu_devs,
>                                    ARRAY_SIZE(exynos_pmu_devs), NULL, 0, NULL);
> -       if (ret)
> +       if (ret) {
> +               destroy_cpuhp_and_cpuidle();

You only want to do this if pmu_cpuhp == true, as currently only gs101
registers the notifiers.

Thanks,

Peter

Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

[Alexey Klimov]

On Wed Jun 10, 2026 at 2:34 PM BST, Peter Griffin wrote:
> Hi Alexey,

Hi Peter,

> Thanks for your patch!
>
> On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>>
>> The setup_cpuhp_and_cpuidle() initialisation sequence currently ignores
>> the return values of cpuhp_setup_state(), cpu_pm_register_notifier(), and
>> register_reboot_notifier(). If any of these registrations fail during
>> probe() routine, the driver returns 0, leaving the driver partially
>> configured.
>
> I originally made the failure non-fatal because the system still boots
> without the notifiers registered (and all other Arm64 Exynos SoCs
> upstream don't register notifiers and AFAICT have broken cpu hotplug
> and cpu idle).
>
> In hindsight, that seems like a mistake. I think your patch to fully
> unwind everything in case of failure makes more sense.  See small
> comment below about destroy_cpuhp_and_cpuidle()

Wait, setup_cpuhp_and_cpuidle() should be non-fatal and shouldn't
return any errors?
Why do we need to have notifiers (say cpu_pm_register_notifier())
registered if, for instance, cpuhp_setup_state() fails?

The other thing I didn't get is that this doesn't deal with handling
errors/return values of cpuhp_setup_state() in probe() and there
are still a lot of errors returned from setup_cpuhp_and_cpuidle().


>> Furthermore, if anything after setup_cpuhp_and_cpuidle() fails in probe()
>> routine, for instance devm_mfd_add_devices(), the probe() lacks an error
>> path and leaves notifiers and cpu hotplug states registered.
>>
>> Introduce variables for the cpu hotplug state IDs in exynos_pmu_context
>> struct, that should be initialised to CPUHP_INVALID by default. Check all
>> return codes in setup_cpuhp_and_cpuidle(), and add an error path to remove
>> registered states on failure. Finally, add destroy_cpuhp_and_cpuidle()
>> helper to safely tear down notifiers and cpu hotplug states.
>>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
>> Fixes: 78b72897a5c8 ("soc: samsung: exynos-pmu: Enable CPU Idle for gs101")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
>> ---
>>  drivers/soc/samsung/exynos-pmu.c | 57 ++++++++++++++++++++++++++++++++++------
>>  1 file changed, 49 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
>> index 9636287f6794..846313a28e9a 100644
>> --- a/drivers/soc/samsung/exynos-pmu.c
>> +++ b/drivers/soc/samsung/exynos-pmu.c
>> @@ -38,6 +38,8 @@ struct exynos_pmu_context {
>>         unsigned long *in_cpuhp;
>>         bool sys_insuspend;
>>         bool sys_inreboot;
>> +       int cpuhp_prepare_state;
>> +       int cpuhp_online_state;
>>  };
>>
>>  void __iomem *pmu_base_addr;
>> @@ -404,6 +406,17 @@ static struct notifier_block exynos_cpupm_reboot_nb = {
>>         .notifier_call = exynos_cpupm_reboot_notifier,
>>  };
>>
>> +static void destroy_cpuhp_and_cpuidle(void)
>> +{
>> +       cpu_pm_unregister_notifier(&gs101_cpu_pm_notifier);
>> +       unregister_reboot_notifier(&exynos_cpupm_reboot_nb);
>> +
>> +       if (pmu_context->cpuhp_prepare_state != CPUHP_INVALID)
>> +               cpuhp_remove_state(pmu_context->cpuhp_prepare_state);
>> +       if (pmu_context->cpuhp_online_state != CPUHP_INVALID)
>> +               cpuhp_remove_state(pmu_context->cpuhp_online_state);
>> +}
>> +
>>  static int setup_cpuhp_and_cpuidle(struct device *dev)
>>  {
>>         struct device_node *intr_gen_node;
>> @@ -465,16 +478,42 @@ static int setup_cpuhp_and_cpuidle(struct device *dev)
>>                 gs101_cpuhp_pmu_online(cpu);
>>
>>         /* register CPU hotplug callbacks */
>> -       cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "soc/exynos-pmu:prepare",
>> -                         gs101_cpuhp_pmu_online, NULL);
>> +       pmu_context->cpuhp_prepare_state = CPUHP_INVALID;
>> +       pmu_context->cpuhp_online_state = CPUHP_INVALID;
>>
>> -       cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
>> -                         NULL, gs101_cpuhp_pmu_offline);
>> +       ret = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "soc/exynos-pmu:prepare",
>> +                               gs101_cpuhp_pmu_online, NULL);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       pmu_context->cpuhp_prepare_state = ret;
>> +
>> +       ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "soc/exynos-pmu:online",
>> +                               NULL, gs101_cpuhp_pmu_offline);
>> +       if (ret < 0)
>> +               goto clean_cpuhp_states;
>> +
>> +       pmu_context->cpuhp_online_state = ret;
>>
>>         /* register CPU PM notifiers for cpuidle */
>> -       cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
>> -       register_reboot_notifier(&exynos_cpupm_reboot_nb);
>> -       return 0;
>> +       ret = cpu_pm_register_notifier(&gs101_cpu_pm_notifier);
>> +       if (ret)
>> +               goto clean_cpuhp_states;
>> +
>> +       ret = register_reboot_notifier(&exynos_cpupm_reboot_nb);
>> +       if (!ret)
>> +               /* Success */
>> +               return ret;
>> +
>> +       cpu_pm_unregister_notifier(&gs101_cpu_pm_notifier);
>> +
>> +clean_cpuhp_states:
>> +       if (pmu_context->cpuhp_prepare_state != CPUHP_INVALID)
>> +               cpuhp_remove_state(pmu_context->cpuhp_prepare_state);
>> +       if (pmu_context->cpuhp_online_state != CPUHP_INVALID)
>> +               cpuhp_remove_state(pmu_context->cpuhp_online_state);
>> +
>> +       return ret;
>>  }
>>
>>  static int exynos_pmu_probe(struct platform_device *pdev)
>> @@ -548,8 +587,10 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>>
>>         ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, exynos_pmu_devs,
>>                                    ARRAY_SIZE(exynos_pmu_devs), NULL, 0, NULL);
>> -       if (ret)
>> +       if (ret) {
>> +               destroy_cpuhp_and_cpuidle();
>
> You only want to do this if pmu_cpuhp == true, as currently only gs101
> registers the notifiers.

Thanks! That's good catch.

Best regards,
Alexey

Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

[Peter Griffin]

Hi Alexey,

On Wed, 10 Jun 2026 at 16:07, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>
> On Wed Jun 10, 2026 at 2:34 PM BST, Peter Griffin wrote:
> > Hi Alexey,
>
> Hi Peter,
>
> > Thanks for your patch!
> >
> > On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
> >>
> >> The setup_cpuhp_and_cpuidle() initialisation sequence currently ignores
> >> the return values of cpuhp_setup_state(), cpu_pm_register_notifier(), and
> >> register_reboot_notifier(). If any of these registrations fail during
> >> probe() routine, the driver returns 0, leaving the driver partially
> >> configured.
> >
> > I originally made the failure non-fatal because the system still boots
> > without the notifiers registered (and all other Arm64 Exynos SoCs
> > upstream don't register notifiers and AFAICT have broken cpu hotplug
> > and cpu idle).
> >
> > In hindsight, that seems like a mistake. I think your patch to fully
> > unwind everything in case of failure makes more sense.  See small
> > comment below about destroy_cpuhp_and_cpuidle()
>
> Wait, setup_cpuhp_and_cpuidle() should be non-fatal and shouldn't
> return any errors?

I suggest you re-read my above comment above ^^

Peter.

Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

[Alexey Klimov]

Hi Peter,

On Thu Jun 11, 2026 at 8:07 AM BST, Peter Griffin wrote:
> Hi Alexey,
>
> On Wed, 10 Jun 2026 at 16:07, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>>
>> On Wed Jun 10, 2026 at 2:34 PM BST, Peter Griffin wrote:
>> > Hi Alexey,
>>
>> Hi Peter,
>>
>> > Thanks for your patch!
>> >
>> > On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>> >>
>> >> The setup_cpuhp_and_cpuidle() initialisation sequence currently ignores
>> >> the return values of cpuhp_setup_state(), cpu_pm_register_notifier(), and
>> >> register_reboot_notifier(). If any of these registrations fail during
>> >> probe() routine, the driver returns 0, leaving the driver partially
>> >> configured.
>> >
>> > I originally made the failure non-fatal because the system still boots
>> > without the notifiers registered (and all other Arm64 Exynos SoCs
>> > upstream don't register notifiers and AFAICT have broken cpu hotplug
>> > and cpu idle).
>> >
>> > In hindsight, that seems like a mistake. I think your patch to fully
>> > unwind everything in case of failure makes more sense.  See small
>> > comment below about destroy_cpuhp_and_cpuidle()
>>
>> Wait, setup_cpuhp_and_cpuidle() should be non-fatal and shouldn't
>> return any errors?
>
> I suggest you re-read my above comment above ^^

Could you please clarify what specifically addresses my question about
notifiers?

Looking further into this, it seems that, for instance, if one of the
hotplug states fails to register then tracking of pmu_context->in_cpuhp
becomes broken.
If reboot notifier silently fails to be registered, then it is unclear how
this from gs101_cpu_pmu_offline() supposed to work:

/* Ignore CPU_PM_ENTER event in reboot or suspend sequence. */
if (pmu_context->sys_insuspend || pmu_context->sys_inreboot) {
	raw_spin_unlock(&pmu_context->cpupm_lock);
	return NOTIFY_OK;
}

If c2 idles are used during reboot/shutdown then they fail or what?

I am not saying that patch is correct and some rework is needed but I don't
get why we should completely ignore errors from hotplug states registration
and should not check registration of notifiers. At least warning should be
shown to user that pm functionality might be unreliable.

Best regards,
Alexey.

Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

[Peter Griffin]

Hi Alexey,

On Mon, 22 Jun 2026 at 19:57, Alexey Klimov <alexey.klimov@linaro.org> wrote:
[..]
> >> >
> >> > I originally made the failure non-fatal because the system still boots
> >> > without the notifiers registered (and all other Arm64 Exynos SoCs
> >> > upstream don't register notifiers and AFAICT have broken cpu hotplug
> >> > and cpu idle).
> >> >
> >> > In hindsight, that seems like a mistake. I think your patch to fully
> >> > unwind everything in case of failure makes more sense.  See small
> >> > comment below about destroy_cpuhp_and_cpuidle()
> >>
> >> Wait, setup_cpuhp_and_cpuidle() should be non-fatal and shouldn't
> >> return any errors?
> >
> > I suggest you re-read my above comment above ^^
>
> Could you please clarify what specifically addresses my question about
> notifiers?

Sure, I was referring to this part of my previous reply:

> >> > In hindsight, that seems like a mistake. I think your patch to fully
> >> > unwind everything in case of failure makes more sense.

[..]
>
> If c2 idles are used during reboot/shutdown then they fail or what?

This followed similar logic to the Samsung downstream kernel drivers.
I have no extra information about it beyond the downstream kernel
source. It seemed reasonable though that CPU's will be hotplugged
during suspend and reboot so you may wish to ignore these.

The proper solution of course is a fully PSCI compliant firmware,
which doesn't require these side channel hints.

>
> I am not saying that patch is correct and some rework is needed but I don't
> get why we should completely ignore errors from hotplug states registration
> and should not check registration of notifiers. At least warning should be
> shown to user that pm functionality might be unreliable.

As mentioned above, and in my previous reply, I think your proposed
patch is a good idea.

regards,

Peter

Re: [PATCH 2/3] soc: samsung: exynos-pmu: fix use-after-free of interrupt generator node

[Alexey Klimov]

On Wed Jun 10, 2026 at 11:58 AM BST, Peter Griffin wrote:
> Hi Alexey,

Hi Peter,

> Thanks for your patch.
>
> On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>>
>> The setup_cpuhp_and_cpuidle() parses the device tree node for the
>> interrupt generation block via of_parse_phandle() and decrements its
>> reference count using of_node_put() immediately after fetching the resource
>> address. However, later the intr_gen_node pointer is passed into
>> of_syscon_register_regmap().
>>
>> Fix this by moving the of_node_put() invocation to after the
>> of_syscon_register_regmap() call, and adding it to correct error paths.
>
> I think  using
> __free(device_node) = of_parse_phandle
>
> would be a cleaner/simpler fix.

Yep, thanks. Around time of sending this Krzysztof also adviced to use
_free() in another commit on maillist.

https://lore.kernel.org/linux-samsung-soc/c866dbcd-c211-48df-84e3-f66785ba8538@kernel.org/

I'll update it.

Best regards,
Alexey

Re: [PATCH 1/3] soc: samsung: exynos-pmu: use target cpu ID in hotplug callbacks

[Alexey Klimov]

On Wed Jun 10, 2026 at 10:55 AM BST, Peter Griffin wrote:
> Hi Alexey,
>
> Thanks for your patch.
>
> On Fri, 5 Jun 2026 at 21:19, Alexey Klimov <alexey.klimov@linaro.org> wrote:
>>
>> The CPU hotplug state callbacks __gs101_cpu_pmu_online() and
>> __gs101_cpu_pmu_offline() currently partially use smp_processor_id() to
>> determine the target register offset for the CPU inform hints. This may
>> be fine for cpuidle flow but broken for cpu hotplug where the target
>> cpu is passed as an argument and could be different from cpu where
>> that is executing (e.g. CPU 0 offlining CPU 1), meaning that
>> smp_processor_id() returns the id of local CPU but hotplug flow
>> deals with another CPU core undergoing the transition.
>
> This was intentional. The powermode hint is always programmed based on
> the currently executing CPU core in the gs101 downstream code (for
> both CPU Idle and CPU hotplug paths). See
> https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/drivers/soc/google/cal-if/pmucal_powermode.c#15
> and the pmu_intr_gen is done based on the actual CPU being
> enabled/disabled.

Beware of the Downstream bearing gifts.
Dowstream may or may not contain mistakes or maybe they have something
that workarounds potential problems. I agree that it is a good source
of reference but I doubt that we must use it as an ultimate GOAT reference
source.

Even Qualcomm downstream things have mistakes sometimes.

> It's possible Exynos850 requires something different. I suggest
> checking the equivalent function in the e850 downstream kernel.
>
>>
>> This causes the pmu driver to write power down and power on configuration
>> hints to the wrong hardware registers, messing up the power state of active
>> cores and failing to configure the target core. Fix this by removing the
>> cpuhint variable entirely and utilizing the target 'cpu' argument passed
>> to the callbacks by the hotplug core infrastructure.
>
> Unfortunately I think you're introducing the bug you describe with this patch.

I wouldn't be so quick judging this like this. I think this patch actually
does fixes a problem. IIRC I also tested this and there were no issues.

Considering that neither your team nor me have access to the implementation
details it would be easier to just drop it instead of endlessly debating
over this if there is no interest narrowing it down.
FWIW this was reported by Sashiko in the first place so I couldn't say what
justification can be provided.
https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3

Let's go over details. Idle callbacks are executed on target CPUs so
target CPU is the same as local CPU.
The hotplug callbacks are slightly different. Offline CPU callback executes
on target CPU so again the local CPU is equal to target CPU (CPU offlines
itself). The online hotplug callback stands out. It is executed on
some CPU and deal with target CPU, that some CPU is not the same as target CPU
as system assumes that CPU undergoing online transition is not ready yet.

All these callbacks write to CPUx_INFORM register (where x -- is CPU id).
The number of these regs are equal to the number of CPUs. I think these
are per-CPU registers.

Idle callbacks and hotplug offline callback write cpu power state hint
to the local CPU's cpu_inform register. On online callback, the random
CPUx_INFORM register is chosen and its state is cleared (because online
callback can be executed on random available CPU). Does it make any sense
to you? Why among 4 callbacks one of them updates random CPUx_INFORM
register?

I think these callbacks should deal with target CPU undergoing transition
and not with random CPU id.

If I consumed enough coffee and can still trust my eyes, I tested two
configurations:
-- this commit (and similar change for e850);
-- change where CPU_INFORM_CLEAR write is removed from online callback.

On e850 (with consecutive series) and gs101. All were fine.

My understanding from experiments is that:
-- clears of CPUx_INFORM are less important and maybe not needed at all;
-- important thing is the value in correct CPUx_INFORM when CPU goes
into a power state, upon executing psci call.

The potential race doesn't reproduce presumably because hotplug ops
are fully serialized.

The downstream probably uses smp_processor_id() for cluster power
down and up states. Perhaps, cluster offline callbacks don't have CPU id
for transition into CPD state and they are executed on the last CPU from
cluster undergoing power state transition.

Anyhow, I think it will less compilcated to remove this change rather
than speculate without a way to confirm the behaviour.

BR,
Alexey.

>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Closes: https://sashiko.dev/#/patchset/20260513-exynos850-cpuhotplug-v4-0-54fec5f65362@linaro.org?part=3
>> Fixes: 598995027b91 ("soc: samsung: exynos-pmu: enable CPU hotplug support for gs101")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
>> ---
>>  drivers/soc/samsung/exynos-pmu.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
>> index d58376c38179..6e635872247a 100644
>> --- a/drivers/soc/samsung/exynos-pmu.c
>> +++ b/drivers/soc/samsung/exynos-pmu.c
>> @@ -235,11 +235,10 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
>>  static int __gs101_cpu_pmu_online(unsigned int cpu)
>>         __must_hold(&pmu_context->cpupm_lock)
>>  {
>> -       unsigned int cpuhint = smp_processor_id();
>>         u32 reg, mask;
>>
>>         /* clear cpu inform hint */
>> -       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
>> +       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
>>                      CPU_INFORM_CLEAR);
>>
>>         mask = BIT(cpu);
>> @@ -296,12 +295,10 @@ static int gs101_cpuhp_pmu_online(unsigned int cpu)
>>  static int __gs101_cpu_pmu_offline(unsigned int cpu)
>>         __must_hold(&pmu_context->cpupm_lock)
>>  {
>> -       unsigned int cpuhint = smp_processor_id();
>>         u32 reg, mask;
>>
>>         /* set cpu inform hint */
>> -       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpuhint),
>> -                    CPU_INFORM_C2);
>> +       regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu), CPU_INFORM_C2);
>>
>>         mask = BIT(cpu);
>>         regmap_update_bits(pmu_context->pmuintrgen, GS101_GRP2_INTR_BID_ENABLE,
>>
>> --
>> 2.51.0
>>

Re: [PATCH 3/3] soc: samsung: exynos-pmu: fix error paths in cpuhotplug/idle states setup

[Alexey Klimov]

Hi Peter,

On Mon Jun 22, 2026 at 9:43 PM BST, Peter Griffin wrote:
> Hi Alexey,
>
> On Mon, 22 Jun 2026 at 19:57, Alexey Klimov <alexey.klimov@linaro.org> wrote:
> [..]
>> >> >
>> >> > I originally made the failure non-fatal because the system still boots
>> >> > without the notifiers registered (and all other Arm64 Exynos SoCs
>> >> > upstream don't register notifiers and AFAICT have broken cpu hotplug
>> >> > and cpu idle).
>> >> >
>> >> > In hindsight, that seems like a mistake. I think your patch to fully
>> >> > unwind everything in case of failure makes more sense.  See small
>> >> > comment below about destroy_cpuhp_and_cpuidle()
>> >>
>> >> Wait, setup_cpuhp_and_cpuidle() should be non-fatal and shouldn't
>> >> return any errors?
>> >
>> > I suggest you re-read my above comment above ^^
>>
>> Could you please clarify what specifically addresses my question about
>> notifiers?
>
> Sure, I was referring to this part of my previous reply:
>
>> >> > In hindsight, that seems like a mistake. I think your patch to fully
>> >> > unwind everything in case of failure makes more sense.
>
> [..]
>>
>> If c2 idles are used during reboot/shutdown then they fail or what?
>
> This followed similar logic to the Samsung downstream kernel drivers.
> I have no extra information about it beyond the downstream kernel
> source. It seemed reasonable though that CPU's will be hotplugged
> during suspend and reboot so you may wish to ignore these.
>
> The proper solution of course is a fully PSCI compliant firmware,
> which doesn't require these side channel hints.

It seems there is some misunderstanding, where we are not getting to the
point. I think I still do not see any comment about failed notifiers
and question about c2 idle states is kinda misdirected.
I also don't see any answers that clarify this from previous emails:

>Why do we need to have notifiers (say cpu_pm_register_notifier())
>registered if, for instance, cpuhp_setup_state() fails?

Let's jump to the details then to move it forward somewhere.

If one of the cpuhp_setup_state() fails and in the current sources
(without this patch) we don't check if it succeeds or not we may not
have properly working hotplug notifiers =>
{set,clear}_bit(cpu, pmu_context->in_cpuhp) may not be updated
correctly.

The if-check logic in cpuidle callback:

        if (test_bit(cpu, pmu_context->in_cpuhp)) {
                raw_spin_unlock(&pmu_context->cpupm_lock);
                return NOTIFY_BAD;
        }

relies on that and may not work reliably. As a result c2 idle states
may stop working or offlined CPU may be put into c2 idle state.

Going further, if register_reboot_notifier() fails and we don't
check its return value then

        /* Ignore CPU_PM_ENTER event in reboot or suspend sequence. */
        if (pmu_context->sys_insuspend || pmu_context->sys_inreboot) {
                raw_spin_unlock(&pmu_context->cpupm_lock);
                return NOTIFY_OK;
        }

a CPU can be put or at least attempted to be put into c2 idle state.
That's why there was a question:

>> If c2 idles are used during reboot/shutdown then they fail or what?

If it is fine to use c2 idle states during power off or reboot,
then why the code blocks them during power off/reboot?

>> I am not saying that patch is correct and some rework is needed but I don't
>> get why we should completely ignore errors from hotplug states registration
>> and should not check registration of notifiers. At least warning should be
>> shown to user that pm functionality might be unreliable.
>
> As mentioned above, and in my previous reply, I think your proposed
> patch is a good idea.

You mentioned, so far as I understood your replies, that _only_
destroy_cpuhp_and_cpuidle() is useful and setup_cpuhp_and_cpuidle()
should be non-fatal. I still think there are some needed error paths for
setup_cpuhp_and_cpuidle() as described above.

Best regards,
Alexey.