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

[Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>]

On 8/3/2026 3:31 PM, sashiko-bot@kernel.org wrote:
> 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.
> --

[snip..]

>> + *
>> + * 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.

Setting proper command is caller responsibility. In case a unsupported 
command is sent, it will be ignored in final reset.

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

This call is expected to be called only by psci-reboot-mode for now.

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

will review if this needs to be taken care.

> 
>> +	} 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.

Yes. If a custom command is set in reboot notifier and data is NULL 
psci_sys_reset, then we are either in panic or in a emergency_restart. 
In both cases, custom reboots are skipped.

thanks,
Shivendra