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

On 14-07-2026 23:00, [email protected] wrote:
> 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?

kept it boot by design.

> 
>> +	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?

Will evaluate this.

> 
>> +}
> 
> [ ... ]
> 
>> +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?

This driver probes via psci-mfd.

thanks,
Shivendra