Re: [PATCH 2/2] firmware: meson: sm: make power-off work on S4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] firmware: meson: sm: broken CPU offlining via freeze_secondary_cpus()
- [Medium] firmware: meson: sm: build and link failure calling freeze_secondary_cpus()
- [Medium] firmware: meson_sm: missing cleanup on error in meson_sm_register_power_off()
--

commit 520b94ab82581e32dd9c97ba0f810c53f3ecadaf
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

firmware: meson: sm: make power-off work on S4

This patch adds power-off prepare and power-off handlers for the Amlogic S4
secure monitor to offline secondary processors and set the firmware shutdown
flag prior to system power-off.

> diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
> index ab9751a59b552..5410386603c70 100644
> --- a/drivers/firmware/meson/meson_sm.c
> +++ b/drivers/firmware/meson/meson_sm.c

[ ... ]

> @@ -303,6 +306,45 @@ static struct attribute *meson_sm_sysfs_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(meson_sm_sysfs);
>  
> +/*
> + * The S4 firmware implements PSCI SYSTEM_OFF as DDR self-refresh: on the
> + * power key it jumps back to the kernel instead of booting, which hangs the
> + * board. Setting the shutdown flag first makes it cold-boot instead, and the
> + * secondary CPUs must have been taken down through PSCI CPU_OFF beforehand,
> + * otherwise the wake-up path hangs as well.
> + */
> +static int meson_sm_power_off_prepare(struct sys_off_data *data)
> +{
> +	if (freeze_secondary_cpus(0))
> +		pr_warn("meson-sm: failed to stop secondary CPUs\n");

[Severity: Medium]
Does calling freeze_secondary_cpus(0) in meson_sm_power_off_prepare() in
drivers/firmware/meson/meson_sm.c break the build when this driver is built
as a module or when sleep support is disabled?

In drivers/firmware/meson/Kconfig, the driver is tristate and does not
depend on sleep or symmetric multiprocessing support:

drivers/firmware/meson/Kconfig:
	config MESON_SM
		tristate "Amlogic Secure Monitor driver"
		depends on ARCH_MESON || COMPILE_TEST
		default ARCH_MESON
		depends on ARM64_4K_PAGES

However, include/linux/cpu.h only declares freeze_secondary_cpus() under
the sleep configuration guard and provides no stub in the else branch:

include/linux/cpu.h:
	#ifdef CONFIG_PM_SLEEP_SMP
	extern int freeze_secondary_cpus(int primary);
	extern void thaw_secondary_cpus(void);
	...
	#else /* !CONFIG_PM_SLEEP_SMP */
	static inline void thaw_secondary_cpus(void) {}
	static inline int suspend_disable_secondary_cpus(void) { return 0; }
	static inline void suspend_enable_secondary_cpus(void) { }
	#endif /* !CONFIG_PM_SLEEP_SMP */

In addition, freeze_secondary_cpus() in kernel/cpu.c is not exported for
loadable modules:

kernel/cpu.c:
	#ifdef CONFIG_PM_SLEEP_SMP
	static cpumask_var_t frozen_cpus;

	int freeze_secondary_cpus(int primary)

When sleep support is disabled, compiling drivers/firmware/meson/meson_sm.c
fails with an implicit function declaration error. When the driver is built
as a module (for example in an arm64 allmodconfig build), linking fails at
modpost with an undefined symbol error for freeze_secondary_cpus().

> +
> +	return NOTIFY_DONE;
> +}

[Severity: High]
Can calling the suspend helper freeze_secondary_cpus(0) from
meson_sm_power_off_prepare() during kernel_power_off() ->
do_kernel_power_off_prepare() fail to offline secondary processors after a
prior wakeup event?

Inside kernel/cpu.c:freeze_secondary_cpus(), the loop checks
pm_wakeup_pending() before taking down each processor and aborts if a wakeup
is pending:

kernel/cpu.c:freeze_secondary_cpus() {
	...
		if (pm_wakeup_pending()) {
			pr_info("Wakeup pending. Abort CPU freeze\n");
			error = -EBUSY;
			break;
		}

		trace_suspend_resume(TPS("CPU_OFF"), cpu, true);
		error = _cpu_down(cpu, 1, CPUHP_OFFLINE);
	...
}

In drivers/base/power/wakeup.c:pm_wakeup_pending(), the check returns true
whenever pm_abort_suspend is positive:

drivers/base/power/wakeup.c:pm_wakeup_pending() {
	...
	return ret || atomic_read(&pm_abort_suspend) > 0;
}

void pm_system_wakeup(void)
{
	atomic_inc(&pm_abort_suspend);
	s2idle_wake();
}

Because pm_abort_suspend is incremented by pm_system_wakeup() during a
suspend or s2idle wakeup event and is only cleared by pm_wakeup_clear(0) at
the start of a subsequent suspend or hibernate cycle (in pm_sleep_fs_sync()
and freeze_processes()), it remains positive after resume and is never
cleared along the kernel_power_off() path.

Furthermore, freeze_secondary_cpus(0) passes tasks_frozen = 1 to
_cpu_down(), which sets cpuhp_tasks_frozen during shutdown even though
tasks are not frozen.

Would powering off after any suspend/resume cycle or wakeup event cause
freeze_secondary_cpus(0) to abort with -EBUSY before offlining secondary
processors, leaving them parked in the kernel and reintroducing the
firmware wake-up hang?

> +
> +static int meson_sm_power_off(struct sys_off_data *data)
> +{
> +	meson_sm_call(data->cb_data, SM_SET_POWEROFF_FLAG, NULL, 1, 0, 0, 0, 0);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int meson_sm_register_power_off(struct device *dev,
> +				       struct meson_sm_firmware *fw)
> +{
> +	int ret;
> +
> +	ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
> +					    SYS_OFF_PRIO_DEFAULT,
> +					    meson_sm_power_off_prepare, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* Run before the PSCI handler, which never returns */
> +	return devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
> +					     SYS_OFF_PRIO_HIGH,
> +					     meson_sm_power_off, fw);
> +}
> +
>  static const struct of_device_id meson_sm_ids[] = {
>  	{ .compatible = "amlogic,meson-gxbb-sm", .data = &gxbb_chip },
>  	{ /* sentinel */ },
> @@ -343,6 +385,10 @@ static int __init meson_sm_probe(struct platform_device *pdev)
>  	if (devm_of_platform_populate(dev))
>  		goto unmap_out_base;
>  
> +	if (of_device_is_compatible(dev->of_node, "amlogic,meson-s4-sm") &&
> +	    meson_sm_register_power_off(dev, fw))
> +		dev_warn(dev, "failed to register power-off handlers\n");

[Severity: Medium]
If the second devm_register_sys_off_handler() call in
meson_sm_register_power_off() fails (for example, if registering at high
priority via register_sys_off_handler() ->
atomic_notifier_chain_register_unique_prio() returns -EBUSY or -ENOMEM),
meson_sm_probe() logs a warning and still returns 0.

Because meson_sm_probe() succeeds, devres cleanup does not run, leaving
meson_sm_power_off_prepare() registered on power_off_prep_handler_list
without its companion meson_sm_power_off() handler.

Should meson_sm_register_power_off() or meson_sm_probe() clean up the
prepare handler when registering the power-off handler fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192144.208954-1-christian.koever-draxl@student.uibk.ac.at?part=2