Re: [PATCH v20 06/10] power: reset: Add psci-reboot-mode driver

[Lorenzo Pieralisi]

On Fri, Apr 03, 2026 at 12:05:27AM +0530, Shivendra Pratap wrote:
> 
> 
> On 01-04-2026 20:07, Lorenzo Pieralisi wrote:
> > On Tue, Mar 31, 2026 at 11:30:09PM +0530, Shivendra Pratap wrote:
> > > 
> > > 
> > > On 27-03-2026 19:25, Lorenzo Pieralisi wrote:
> > > > On Wed, Mar 04, 2026 at 11:33:06PM +0530, Shivendra Pratap wrote:
> > > > > PSCI supports different types of resets like COLD reset, ARCH WARM
> 
> [snip..]
> 
> > > > > + * Predefined reboot-modes are defined as per the values
> > > > > + * of enum reboot_mode defined in the kernel: reboot.c.
> > > > > + */
> > > > > +static struct mode_info psci_resets[] = {
> > > > > +	{ .mode = "warm", .magic = REBOOT_WARM},
> > > > > +	{ .mode = "soft", .magic = REBOOT_SOFT},
> > > > > +	{ .mode = "cold", .magic = REBOOT_COLD},
> > 
> > These strings match the command userspace issue right ? I think that we
> > should make them match the corresponding PSCI reset types, the list above
> > maps command to reboot_mode values and those can belong to any reboot
> > mode driver to be honest they don't make much sense in a PSCI reboot
> > mode driver only.
> > 
> > It is a question for everyone here: would it make sense to make these
> > predefined resets a set of strings, eg:
> > 
> > psci-system-reset
> > psci-system-reset2-arch-warm-reset
> > 
> > and then vendor resets:
> > 
> > psci-system-reset2-vendor-reset
> 
> Can you share bit more details on this? We are already defining the string
> from userspace in the struct - eg: ".mode = "warm".

"warm","soft","cold" are not strictly speaking PSCI concepts and mean nothing
well defined to user space and even if they did, they would not belong in
the PSCI reboot mode driver but in generic code.

Spelling out what a reset is might help instead, again, this is just my
opinion, I don't know how the semantics of resets have been handled thus
far.

If userspace issues a LINUX_REBOOT_CMD_RESTART2 with arg, say,
"psci-system-reset2-arch-warm-reset" it is pretty clear what it wants
to do in PSCI.

Again, it is a suggestion, comments welcome.

> yes we can move away from enum reboot_mode and use custom psci defines one -
> Ack.
> 
> > 
> 
> [snip ..]
> 
> > > > > +
> > > > > +/*
> > > > > + * arg1 is reset_type(Low 32 bit of magic).
> > > > > + * arg2 is cookie(High 32 bit of magic).
> > > > > + * If reset_type is 0, cookie will be used to decide the reset command.
> > > > > + */
> > > > > +static int psci_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
> > > > > +{
> > > > > +	u32 reset_type = REBOOT_MODE_ARG1(magic);
> > > > > +	u32 cookie = REBOOT_MODE_ARG2(magic);
> > > > > +
> > > > > +	if (reset_type == 0) {
> > > > > +		if (cookie == REBOOT_WARM || cookie == REBOOT_SOFT)
> > > > > +			psci_set_reset_cmd(true, 0, 0);
> > > > > +		else
> > > > > +			psci_set_reset_cmd(false, 0, 0);
> > > > > +	} else {
> > > > > +		psci_set_reset_cmd(true, reset_type, cookie);
> > > > > +	}
> > > > 
> > > > I don't think that psci_set_reset_cmd() has the right interface (and this
> > > > nested if is too complicated for my taste). All we need to pass is reset-type
> > > > and cookie (and if the reset is one of the predefined ones, reset-type is 0
> > > > and cookie is the REBOOT_* cookie).
> > > > 
> > > > Then the PSCI firmware driver will take the action according to what
> > > > resets are available.
> > > > 
> > > > How does it sound ?
> > > 
> > > So we mean these checks will move to the psci driver? Sorry for re-iterating
> > > the question.
> > 
> > Given what I say above, I believe that something we can do is mapping the magic
> > to an enum like:
> > 
> > PSCI_SYSTEM_RESET
> > PSCI_SYSTEM_RESET2_ARCH_SYSTEM_WARM_RESET
> > PSCI_SYSTEM_RESET2_VENDOR_RESET
> > 
> > and can add a probe function into PSCI driver similar to psci_has_osi_support() but
> > to probe for SYSTEM_RESET2 and initialize the predefined strings accordingly,
> > depending on its presence.
> 
> Not able to get it cleanly.
> 
> 1. Will move away from reboot_mode enum for pre-defined modes and define new
> enum defining these modes- fine.
> 2. get SYSTEM_RESET2 is supported from psci exported function -- fine, but
> how we use it here now, as we do not want to send the reset_cmd from
> psci_set_reset_cmd now?

You do keep psci_set_reset_cmd() but all it is used for is setting a struct
shared with the PSCI driver where you initialize the enum above, possibly
with a cookie if it is a vendor reset.

> 3. For pre-defined modes, warm/soft or cold - reset_type and cookie, both
> are zero, sys_reset2 or sys_reset2 decides the ARCH reset vs cold reset.
> 4. For vendor-rest , we use sys_reset2 with reset_type and cookie.

Yes.

> All above is done in reboot_notifier call at psci-reboot-mode.
> --
> 
> Now in the final restart_notifier->psci_sys_reset --
> 
> If panic is in progress, we do not use any of the cmd based reset params and
> go with the legacy reset. So we need to preserve the values that were set
> from psci-reboot-mode.
> 
> Did not understand the proposed suggestion in above usecase. Need more input
> on this.

I explained above. The reboot mode driver sets the command to carry out
depending on the string coming from user space and whether PSCI supports
SYSTEM_RESET2 or not.

> --
> 
> One other option is to have a restart_notifier in psci-reboot-mode, with
> lesser priority than psci_sys_rest and then handle all the case including
> panic and sys_reset2.

No.

Thanks,
Lorenzo