Re: [PATCH v22 04/13] 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] Unsupported or malformed reset commands cause the reboot process to silently fail and hang the system.
- [High] The API illegally truncates the 64-bit PSCI SYSTEM_RESET2 cookie parameter to 32 bits, violating the ARM PSCI specification.
- [Medium] Meaningful code mismatch: Missing EXPORT_SYMBOL for functions intended for external drivers.
- [Medium] Confusing and contradictory API semantics between `enum psci_reset_type` and the `PSCI_RESET_TYPE` macro.
--

commit bb624cd844c2478ffbac61bf8098b194ebc91887
Author: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>

firmware: psci: Introduce command-based resets

This commit introduces a command-based reset mechanism for PSCI, allowing
external drivers to specify the reset command. It adds predefined reset types
and maps them to standard PSCI resets, handling vendor-specific resets if
supported. The command-based reset is checked during the device reset flow,
falling back to the standard reboot mode on panic.

> firmware: psci: Introduce command-based resets
> 
> PSCI currently supports only two resets - SYSTEM_RESET and SYSTEM_RESET2
> ARCH WARM reset. The reset patch is selected based on the Linux
> reboot_mode variable. The PSCI specification now includes SYSTEM_RESET2
> for vendor-specific resets but there's no mechanism to issue these
> through psci_sys_reset().
> 
> Add a command-based reset mechanism that allows external drivers to set
> the psci reset command via a exported psci_set_reset_cmd() function.

The commit message mentions an exported function, but the implementation
does not include EXPORT_SYMBOL_GPL() for psci_set_reset_cmd(). Could this
cause module link failures if external drivers are built as modules?

[ ... ]

> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c

[ ... ]

> @@ -51,6 +51,16 @@ static int resident_cpu = -1;
>  struct psci_operations psci_ops;
>  static enum arm_smccc_conduit psci_conduit = SMCCC_CONDUIT_NONE;
>  
> +/*
> + * Encoded reset command:
> + * bits[63:32] = cookie
> + * bits[31:0]  = reset_type
> + */
> +static u64 reset_cmd;
> +
> +#define PSCI_RESET_TYPE(reset_cmd)	((u32)(reset_cmd))
> +#define PSCI_RESET_COOKIE(reset_cmd)	((u32)((reset_cmd) >> 32))

The ARM PSCI v1.1 specification defines the cookie parameter for SYSTEM_RESET2
as a 64-bit value on SMC64/HVC64 platforms. Does cramming the cookie into
the upper 32 bits and casting it to a u32 silently truncate this value?

This might break vendor-specific reset implementations that require a 64-bit
cookie, as the function will receive truncated parameters.

[ ... ]

> @@ -306,8 +345,24 @@ static int get_set_conduit_method(const struct device_node *np)
>  	return 0;
>  }
>  
> -static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> -			  void *data)
> +static void psci_handle_reset_cmd(void)
> +{
> +	u32 psci_sys_reset_fn;
> +
> +	if ((reset_cmd & BIT_ULL(31)) && psci_system_reset2_supported) {
> +		/* PSCI SYSTEM_RESET2 Vendor-specific reset */
> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> +			       PSCI_RESET_TYPE(reset_cmd),
> +			       PSCI_RESET_COOKIE(reset_cmd), 0);
> +	} else {
> +		/* cookie part of the reset_cmd decides ARCH WARM RESET vs SYSTEM_RESET */
> +		psci_sys_reset_fn = psci_fn_from_cookie(PSCI_RESET_COOKIE(reset_cmd));
> +		if (!PSCI_RESET_TYPE(reset_cmd) && psci_sys_reset_fn)
> +			invoke_psci_fn(psci_sys_reset_fn, 0, 0, 0);
> +	}
> +}

What happens if an external driver provides an unsupported or malformed reset
command? 

If bit 31 is set but psci_system_reset2_supported is false, or if an
unrecognized cookie results in a null psci_sys_reset_fn, neither branch
invokes the PSCI function.

psci_handle_reset_cmd() will return without doing anything, and
psci_sys_reset() will return NOTIFY_DONE without falling back to the default
reboot behavior.

Could this leave the system in a hung state instead of rebooting gracefully?

[ ... ]

> --- a/include/linux/psci.h
> +++ b/include/linux/psci.h

[ ... ]

> +/**
> + * enum psci_reset_type	-	PSCI_RESET_TYPE for SYSTEM_RESET.
> + * @PSCI_RESET_TYPE_SYSTEM_RESET: Standard SYSTEM_RESET command.
> + * @PSCI_RESET_TYPE_SYSTEM_RESET2_ARCH_WARM: SYSTEM_RESET2 architectural warm reset.
> + *
> + * These enum values map PSCI_RESET_TYPE_SYSTEM_* constants to reset strings
> + * issued from user space. When user space requests a reset, the cookie
> + * carries one of these values, and the PSCI reset path translates it into
> + * the appropriate PSCI system reset call.
> + */
> +enum psci_reset_type {
> +	PSCI_RESET_TYPE_SYSTEM_RESET = 1,
> +	PSCI_RESET_TYPE_SYSTEM_RESET2_ARCH_WARM,
> +};

Is the naming of this enum potentially confusing? The PSCI_RESET_TYPE macro
defined earlier extracts the lower 32 bits, but these enum values are mapped
to the upper 32 bits (the cookie) as handled by psci_fn_from_cookie().

Naming the cookie enum values as reset_type contradicts the macro
design and might lead to developers passing these enum values into the
reset_type field by mistake.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-arm-psci-system_reset2-vendor-reboots-v22-0-28a5bde07483@oss.qualcomm.com?part=4