Re: [PATCH v24 03/12] firmware: psci: Introduce command-based resets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Custom PSCI reset commands are completely ignored on standard normal reboots due to an incorrect NULL check on the restart command string (`data`).
- [High] The 64-bit vendor reset `cookie` is silently truncated to 32 bits on 32-bit ARM architectures, violating the PSCI SMC32 calling convention.
- [Medium] The `psci_set_reset_cmd` API cannot be used by modular drivers because it is not exported.
- [Low] `psci_set_reset_cmd()` does not validate that a non-zero `reset_type` has the vendor-specific bit set.
--

--- Patch [3]: [PATCH v24 03/12] firmware: psci: Introduce command-based resets ---
commit f75a0ef9e572ce4de306eab6e925457653c70d6f
Author: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>

firmware: psci: Introduce command-based resets

This commit introduces a mechanism to support command-based resets via
PSCI, allowing reboot-mode drivers to specify a reset_type and cookie.
It falls back to the normal reboot_mode-based reset path if the command
is unsupported or a panic occurs.

> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
[ ... ]
> @@ -80,6 +92,61 @@ static u32 psci_cpu_suspend_feature;
> +/**
> + * psci_set_reset_cmd() - Configure the PSCI reset command
> + * @reset_type: SYSTEM_RESET2 reset type, or 0 for a standard reset
> + * @cookie: Vendor-defined SYSTEM_RESET2 cookie, or a value from
> + *	    enum psci_standard_resets when @reset_type is 0
> + *
> + * For vendor-specific SYSTEM_RESET2 resets, @reset_type and @cookie
> + * must contain platform-defined values.
> + *
> + * For standard resets, @reset_type must be 0 and @cookie must be a
> + * value from enum psci_standard_resets.
> + *
> + * The reset command may be configured only once per boot cycle.
> + */
> +int psci_set_reset_cmd(u32 reset_type, u64 cookie)
> +{
> +	if (!reset_type && !cookie)
> +		return -EINVAL;

[Severity: Low]
Does this function need to validate that a non-zero reset_type has the
vendor-specific bit (PSCI_1_1_RESET_TYPE_VENDOR_START) set? 

If a caller erroneously passes a reset_type that is greater than 0 but
lacks this bit, it is stored successfully but will be silently ignored
during reboot in psci_handle_reset_cmd(). Rejecting it here with -EINVAL
might prevent silent runtime failures.

> +
> +	scoped_guard(mutex, &reset_cmd_mutex) {
> +		if (reset_cmd)
> +			return -EBUSY;
> +
> +		reset_cmd_data.reset_type = reset_type;
> +		reset_cmd_data.cookie = cookie;
> +		/*
> +		 * Publish the command only after both fields are fully initialized.
> +		 * Readers run from the atomic restart notifier path and must not block.
> +		 */
> +		smp_store_release(&reset_cmd, &reset_cmd_data);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Is this missing an EXPORT_SYMBOL_GPL? 

Reboot-mode drivers are often configured as loadable kernel modules (e.g.,
CONFIG_REBOOT_MODE=m), and without an export, they will fail to link against
this new API.

[ ... ]
> +static void psci_handle_reset_cmd(const struct psci_system_reset_cmd *cmd)
> +{
> +	u32 psci_sys_reset_fn;
> +
> +	/* PSCI_1_1_RESET_TYPE_VENDOR_START identifies vendor reset types. */
> +	if ((cmd->reset_type & PSCI_1_1_RESET_TYPE_VENDOR_START) &&
> +	    psci_system_reset2_supported) {
> +		/* PSCI SYSTEM_RESET2 Vendor-specific reset */
> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> +			       cmd->reset_type, cmd->cookie, 0);

[Severity: High]
Will this silently truncate the 64-bit cmd->cookie on 32-bit ARM
architectures?

Because invoke_psci_fn() takes arguments of type unsigned long, the C
compiler will cast the u64 cookie to 32 bits. Under the SMC32 calling
convention for SYSTEM_RESET2, the secure monitor expects the 64-bit cookie
passed across two 32-bit registers (r2 and r3). 

Here, it seems r3 will receive 0 because it is passed as the fourth argument,
corrupting the hardware communication for vendor-specific resets that rely on
the upper 32 bits.

> +	} else {
> +		/* Retrieve the psci reset function from reset_cmd */
> +		psci_sys_reset_fn = psci_get_sys_reset_fn(cmd);
> +		if (!cmd->reset_type && psci_sys_reset_fn)
> +			invoke_psci_fn(psci_sys_reset_fn, 0, 0, 0);
> +	}
> +}
[ ... ]
> +static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> +			  void *data)
> +{
> +	const struct psci_system_reset_cmd *cmd;
> +
> +	/* The function psci_handle_reboot_mode follows reboot_mode based
> +	 * reset flow and psci_handle_reset_cmd uses reset_cmd based reset flow.
> +	 *
> +	 * The reset_cmd is configured at the reboot_notifier phase.
> +	 * If panic() or emergency_restart() occurs between the reboot_notifier
> +	 * and this final reset, skip command-based reset and let reboot_mode drive
> +	 * the reset flow.
> +	 *
> +	 * The function psci_handle_reset_cmd invokes non-returning PSCI SYSTEM_RESET
> +	 * calls to reset the device. If it returns, either the reset failed, or the
> +	 * command was unsupported. Fallback to reboot_mode based reset flow.
> +	 */
> +	cmd = smp_load_acquire(&reset_cmd);
> +	if (data && cmd && !panic_in_progress())
> +		psci_handle_reset_cmd(cmd);

[Severity: High]
Are custom reset commands completely ignored on standard normal reboots
due to the "data &&" check?

A standard user-initiated reboot (e.g., executing the reboot command without
arguments) invokes sys_reboot(LINUX_REBOOT_CMD_RESTART), which ultimately
calls kernel_restart(NULL). This passes NULL down the notifier chain, resulting
in data == NULL in psci_sys_reset().

Because of this check, the custom command path appears to be skipped entirely,
forcing a fallback to the default behavior and defeating the purpose of
setting a custom command for standard reboots.

> +
> +	psci_handle_reboot_mode();
>  
>  	return NOTIFY_DONE;
>  }

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