Re: [PATCH v24 06/12] power: reset: Add psci-reboot-mode driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `psci-reboot-mode` driver is completely unreachable and non-functional because it is never instantiated.
- [High] Standard 'normal' reboot mode commands are entirely ignored by PSCI due to an incorrect `data` check in the restart handler.
--

--- Patch [6]: [PATCH v24 06/12] power: reset: Add psci-reboot-mode driver ---
commit 90394d93558537c03c13e84cb541a725b02ad7e4
Author: Shivendra Pratap <[email protected]>
Subject: power: reset: Add psci-reboot-mode driver

This commit adds a psci-reboot-mode driver to handle predefined and
vendor-specific PSCI resets. It registers the driver with the reboot-mode
framework to interface these resets to userspace. When userspace initiates a
supported command, the reset arguments are passed to the PSCI driver to
enable a command-based reset.

> diff --git a/drivers/power/reset/psci-reboot-mode.c b/drivers/power/reset/psci-reboot-mode.c
> new file mode 100644
> index 0000000000000..2f960fda8229f
> --- /dev/null
> +++ b/drivers/power/reset/psci-reboot-mode.c
[ ... ]
> +static int psci_reboot_mode_write(struct reboot_mode_driver *reboot,
> +				  const u32 *magic, u32 count)
> +{
> +	(void)reboot;
> +
> +	if (count < 1 || count > 3)
> +		return -EINVAL;
> +
> +	return psci_set_reset_cmd(magic[0], psci_reboot_mode_get_cookie(magic, count));

[Severity: High]
Does this work for standard "normal" reboots?

When a standard reboot is triggered without a specific command string, the
reboot_mode framework translates the NULL command to "normal" mode and calls
this write function to configure the reset command.

However, in drivers/firmware/psci/psci.c, psci_sys_reset() skips applying
the command because it checks for a non-NULL data argument:

drivers/firmware/psci/psci.c:psci_sys_reset() {
	...
	cmd = smp_load_acquire(&reset_cmd);
	if (data && cmd && !panic_in_progress())
		psci_handle_reset_cmd(cmd);

	psci_handle_reboot_mode();
	...
}

Will this cause standard reboots mapped to vendor-specific commands to be
silently ignored?

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

[Severity: High]
Is this driver ever instantiated?

It appears to be missing an .of_match_table in the driver structure.

Additionally, looking at drivers/mfd/psci-mfd.c, psci-reboot-mode is not
included in the psci_cells array:

drivers/mfd/psci-mfd.c:psci_cells[] = {
	{
		.name = "psci-cpuidle-domain",
	},
};

Without an OF match table and being omitted from the MFD cells, how does this
driver get probed to parse the vendor-specific reset modes from the device
tree?

> +module_platform_driver(psci_reboot_mode_driver);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-arm-psci-system_reset2-vendor-reboots-v24-0-889281373870@oss.qualcomm.com?part=6