Re: [PATCH v23 07/13] power: reset: Add psci-reboot-mode driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `psci-reboot-mode` platform driver is completely unreachable dead code because it lacks an `of_match_table` and is never instantiated as a child device by the parent PSCI or MFD driver.
- [High] Lockless read of `reset_cmd` in `psci_sys_reset()` can race with concurrent updates via `psci_set_reset_cmd()`.
- [Medium] The `PSCI_REBOOT_MODE` config is set to `bool`, which forces the `REBOOT_MODE` framework to be built-in.
--

--- Patch [7]: [PATCH v23 07/13] power: reset: Add psci-reboot-mode driver ---
commit 05cd2956034a357b1e7d5eab96bdce4575604285
Author: Shivendra Pratap <[email protected]>

power: reset: Add psci-reboot-mode driver

Add a psci-reboot-mode driver to handle different types of PSCI resets,
including predefined and vendor-specific resets, at one place. It registers
with the reboot-mode framework to interface these resets to userspace and
passes arguments to the PSCI driver for command-based reset.

> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 124afb99febe9..93e4029d95060 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -348,6 +348,16 @@ config NVMEM_REBOOT_MODE
>  	  then the bootloader can read it and take different
>  	  action according to the mode.
>  
> +config PSCI_REBOOT_MODE
> +	bool "PSCI reboot mode driver"
> +	depends on OF && (ARM_PSCI_FW || COMPILE_TEST)
> +	select REBOOT_MODE

[Severity: Medium]
Will this force the REBOOT_MODE framework to always be built-in?

Since PSCI_REBOOT_MODE is defined as a bool, selecting the REBOOT_MODE
tristate will forcefully upgrade the entire reboot-mode framework to built-in
(=y), which overrides user configurations that might prefer it as a module.

Also, since the driver source uses module_platform_driver and MODULE_LICENSE,
it appears designed to be built as a module. Should this be a tristate instead?

> +	help
> +	  Say y here will enable PSCI reboot mode driver. This gets
> +	  the PSCI reboot mode arguments and passes them to psci
> +	  driver. psci driver uses these arguments for issuing
> +	  device reset into different boot states.
> +
>  config POWER_MLXBF
>  	tristate "Mellanox BlueField power handling driver"
>  	depends on (GPIO_MLXBF2 || GPIO_MLXBF3) && ACPI

[ ... ]

> diff --git a/drivers/power/reset/psci-reboot-mode.c b/drivers/power/reset/psci-reboot-mode.c
> new file mode 100644
> index 0000000000000..955ed68f14026
> --- /dev/null
> +++ b/drivers/power/reset/psci-reboot-mode.c

[ ... ]

> +static int psci_reboot_mode_write(struct reboot_mode_driver *reboot,
> +				  u32 *magic, int count)
> +{
> +	if (count < 1 || count > 3)
> +		return -EINVAL;
> +
> +	return psci_set_reset_cmd(magic[0], psci_reboot_mode_get_cookie(magic, count));

[Severity: High]
Can this call to psci_set_reset_cmd() race with the atomic restart handler?

Looking at drivers/firmware/psci/psci.c, psci_sys_reset() evaluates reset_cmd
locklessly:

	if ((reset_cmd.reset_type || reset_cmd.cookie) && !panic_in_progress())
		psci_handle_reset_cmd();

And psci_set_reset_cmd() protects writes using a mutex:

	reset_cmd.reset_type = reset_type;
	reset_cmd.cookie = cookie;

If an atomic restart handler like psci_sys_reset() executes during a SysRq-B
emergency restart and interrupts a CPU executing psci_set_reset_cmd(), it could
observe a partially updated state.

Since the 64-bit cookie may be written via multiple instructions on 32-bit ARM,
could this lead to torn reads and mismatched reset types, causing the system to
hang during emergency restarts?

> +}

[ ... ]

> +static struct platform_driver psci_reboot_mode_driver = {
> +	.probe  = psci_reboot_mode_probe,
> +	.driver = {
> +		.name	= "psci-reboot-mode",
> +	},
> +};

[Severity: High]
Is this platform driver able to probe?

The driver lacks an of_match_table and is not instantiated as a child device
by any parent PSCI or MFD driver.

A platform driver without an of_match_table must be manually instantiated
(e.g., via mfd_add_devices in an MFD parent like psci-mfd.c). However,
psci_reboot_mode is not listed in psci_cells in drivers/mfd/psci-mfd.c, nor
does any code call of_platform_populate() on the PSCI node.

Does this make the newly introduced psci-reboot-mode functionality unreachable
dead code?

> +module_platform_driver(psci_reboot_mode_driver);
> +
> +MODULE_DESCRIPTION("PSCI reboot mode driver");
> +MODULE_LICENSE("GPL");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-arm-psci-system_reset2-vendor-reboots-v23-0-e7453c548c21@oss.qualcomm.com?part=7