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

[Shivendra Pratap]

On 27-03-2026 19:37, Lorenzo Pieralisi wrote:
> 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.

Ack.

> > Signed-off-by: Shivendra Pratap <shivendra.pratap@xxxxxxxxxxxxxxxx>
> > ---
> >   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.

Need bit more clarification. The issue is that at some point we need to 
decide - sys_rst2 or the reboot_mode based reset. Are you suggesting 
that this check should be in psci driver instead of a pre-check in 
psci_reboot_mode driver?

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

Ack. Will restructure it as suggested.

thanks,
Shivendra