Re: [PATCH v20 04/10] firmware: psci: Introduce command-based reset in psci_sys_reset

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Wed, Mar 04, 2026 at 11:33:04PM +0530, Shivendra Pratap wrote:
> PSCI currently supports only COLD reset and ARCH WARM reset 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 new psci_set_reset_cmd() function.
> 
> The psci command-based reset is disabled by default and the
> psci_sys_reset follows its original flow until a psci_reset command is
> set. In kernel panic path, psci_reset command is ignored.

If it is function calls you should add parenthesis (eg psci_sys_reset ->
psci_sys_reset()).

You must explain why the kernel panic path requires separate handling
here AND in the code - think about looking at this years down the line
and figure out why kernel panics are special here.

> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  drivers/firmware/psci/psci.c | 45 ++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/psci.h         |  2 ++
>  2 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 38ca190d4a22d6e7e0f06420e8478a2b0ec2fe6f..ae6f7a0aead913d740070080d4b2a3da15b29485 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -51,6 +51,15 @@ static int resident_cpu = -1;
>  struct psci_operations psci_ops;
>  static enum arm_smccc_conduit psci_conduit = SMCCC_CONDUIT_NONE;
>  
> +struct psci_sys_reset_params {
> +	u32 system_reset;
> +	u32 reset_type;
> +	u32 cookie;
> +	bool cmd;
> +};
> +
> +static struct psci_sys_reset_params psci_reset;
> +
>  bool psci_tos_resident_on(int cpu)
>  {
>  	return cpu == resident_cpu;
> @@ -80,6 +89,28 @@ static u32 psci_cpu_suspend_feature;
>  static bool psci_system_reset2_supported;
>  static bool psci_system_off2_hibernate_supported;
>  
> +/**
> + * psci_set_reset_cmd - Sets the psci_reset_cmd for command-based
> + * reset which will be used in psci_sys_reset call.
> + *
> + * @cmd_sys_rst2: Set to true for SYSTEM_RESET2 based resets.
> + * @cmd_reset_type: Set the reset_type argument for psci_sys_reset.
> + * @cmd_cookie: Set the cookie argument for psci_sys_reset.
> + */
> +void psci_set_reset_cmd(bool cmd_sys_rst2, u32 cmd_reset_type, u32 cmd_cookie)
> +{

I don't think cmd_sys_rst2 is needed, as a replied in a different thread.

> +	if (cmd_sys_rst2 && psci_system_reset2_supported) {
> +		psci_reset.system_reset = PSCI_FN_NATIVE(1_1, SYSTEM_RESET2);
> +		psci_reset.reset_type = cmd_reset_type;
> +		psci_reset.cookie = cmd_cookie;
> +	} else {
> +		psci_reset.system_reset = PSCI_0_2_FN_SYSTEM_RESET;
> +		psci_reset.reset_type = 0;
> +		psci_reset.cookie = 0;
> +	}
> +	psci_reset.cmd = true;
> +}
> +
>  static inline bool psci_has_ext_power_state(void)
>  {
>  	return psci_cpu_suspend_feature &
> @@ -309,14 +340,24 @@ static int get_set_conduit_method(const struct device_node *np)
>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
>  			  void *data)
>  {
> -	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
> -	    psci_system_reset2_supported) {
> +	if (((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
> +	     psci_system_reset2_supported) && (panic_in_progress() || !psci_reset.cmd)) {
>  		/*
>  		 * reset_type[31] = 0 (architectural)
>  		 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
>  		 * cookie = 0 (ignored by the implementation)
>  		 */
>  		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);
> +	} else if (!panic_in_progress() && psci_reset.cmd) {
> +		/*
> +		 * Commands are being set in psci_set_reset_cmd
> +		 * This issues, SYSTEM_RESET2 arch warm reset or
> +		 * SYSTEM_RESET2 vendor-specific reset or
> +		 * a SYSTEM_RESET cold reset in accordance with
> +		 * the reboot-mode command.
> +		 */
> +		invoke_psci_fn(psci_reset.system_reset, psci_reset.reset_type,
> +			       psci_reset.cookie, 0);
>  	} else {
>  		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);

This is very hard to parse. IMO, what you should do is:

- Split this into two different paths: reboot_mode vs psci_reset.cmd == true.
- Document very clearly why a panic needs separate handling.

Something like:

if (psci_reset.cmd)
	handle_reset_cmd();
else
	handle_reboot_mode();

I don't think we are far from converging but I want to be able to maintain
this code going forward.

Thanks,
Lorenzo

>  	}
> diff --git a/include/linux/psci.h b/include/linux/psci.h
> index 4ca0060a3fc42ba1ca751c7862fb4ad8dda35a4c..d13ceca88eab8932894051e7c86e806c2ad8a73a 100644
> --- a/include/linux/psci.h
> +++ b/include/linux/psci.h
> @@ -45,8 +45,10 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void);
>  
>  #if defined(CONFIG_ARM_PSCI_FW)
>  int __init psci_dt_init(void);
> +void psci_set_reset_cmd(bool cmd_sys_rst2, u32 cmd_reset_type, u32 cmd_cookie);
>  #else
>  static inline int psci_dt_init(void) { return 0; }
> +static inline void psci_set_reset_cmd(bool cmd_sys_rst2, u32 cmd_reset_type, u32 cmd_cookie) { }
>  #endif
>  
>  #if defined(CONFIG_ARM_PSCI_FW) && defined(CONFIG_ACPI)
> 
> -- 
> 2.34.1
>