On Mon, Nov 18, 2024 at 04:13:47PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Nov 15, 2024 at 11:08:22AM -0800, Elliot Berman wrote:
> > On Fri, Nov 15, 2024 at 02:35:52PM +0100, Lorenzo Pieralisi wrote:
> > > On Wed, Oct 23, 2024 at 09:30:21AM -0700, Elliot Berman wrote:
> > > > On Fri, Oct 18, 2024 at 10:42:46PM -0700, Stephen Boyd wrote:
> > > > > Quoting Elliot Berman (2024-10-18 12:39:48)
> > > > > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > > > > > index 2328ca58bba6..60bc285622ce 100644
> > > > > > --- a/drivers/firmware/psci/psci.c
> > > > > > +++ b/drivers/firmware/psci/psci.c
> > > > > > @@ -29,6 +29,8 @@
> > > > > > #include <asm/smp_plat.h>
> > > > > > #include <asm/suspend.h>
> > > > > >
> > > > > > +#define REBOOT_PREFIX "mode-"
> > > > >
> > > > > Maybe move this near the function that uses it.
> > > > >
> > > > > > +
> > > > > > /*
> > > > > > * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> > > > > > * calls it is necessary to use SMC64 to pass or return 64-bit values.
> > > > > > @@ -305,9 +315,29 @@ static int get_set_conduit_method(const struct device_node *np)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +static void psci_vendor_sys_reset2(unsigned long action, void *data)
> > > > > > +{
> > > > > > + const char *cmd = data;
> > > > > > + unsigned long ret;
> > > > > > + size_t i;
> > > > > > +
> > > > > > + for (i = 0; i < num_psci_reset_params; i++) {
> > > > > > + if (!strcmp(psci_reset_params[i].mode, cmd)) {
> > > > > > + ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> > > > > > + psci_reset_params[i].reset_type,
> > > > > > + psci_reset_params[i].cookie, 0);
> > > > > > + pr_err("failed to perform reset \"%s\": %ld\n",
> > > > > > + cmd, (long)ret);
> > > > >
> > > > > Do this intentionally return? Should it be some other function that's
> > > > > __noreturn instead and a while (1) if the firmware returns back to the
> > > > > kernel?
> > > > >
> > > >
> > > > Yes, I think it's best to make sure we fall back to the architectural
> > > > reset (whether it's the SYSTEM_RESET or architectural SYSTEM_RESET2)
> > > > since device would reboot then.
> > >
> > > Well, that's one of the doubts I have about enabling this code. From
> > > userspace we are requesting a reboot (I don't even think that user
> > > space knows which reboot modes are actually implemented (?)) and we may
> > > end up issuing one with completely different semantics ?
> >
> > You're right here, userspace issue a "reboot bootloader" and if kernel
> > doesn't have the support to set up the right cookie, the device would do
> > a normal reboot and not stop at the bootloader. This problem exists
> > today and I think whether this is an issue to solve is out of scope here.
>
> That's true. It is the same issue we have with reboot_mode anyway.
>
> Is it a fair statement to say that currently when we request a reboot,
> the reboot mode is the one set through /sys/kernel/reboot/mode ?
>
> Does user space use that file today ?
>
> I guess userspace does not take specific actions according to the
> reset it thinks it issues - it is a question.
>
Yes, user space can write to that file. User space has to configure both
the mode and command to get the desired reboot configuration. I view the
vendor reset types as replacing "both", in the sense userspace may not
need to configure the reboot mode anymore. If "reboot bootloader" or
"reboot edl" requires a warm reset, the firmware knows that's how the
PMIC needs to be configured. I don't currently see any need for Linux to
be aware that a particular vendor reset type is "like a soft" or "like a
warm" or "like a cold" reset.
> > > Are these "reset types" exported to user space ?
> > >
> >
> > No mechanism exists to do that. We could do something specific for PSCI
> > or do something generic for everybody. I don't think something specific
> > for PSCI is the right approach because it's a general problem. I don't
> > think there's enough interest to change reboot command plumbing to
> > advertise valid reset types to userspace.
>
> That's for sure. I suppose the most important bit is making sure that
> all resets comply with the kernel semantics expected from a *reset*;
> I appreciate that's a vague statement (and I have no idea how to enforce
> it) but that's the gist of this discussion.
>
> Another thing I am worried about is device drivers restart handlers
> (ie having to parse a command that might be platform specific in a
> generic driver to grok what reset was actually issued and what action
> should be taken).
Right, I got your point! I haven't seen any drivers that care about it,
besides the ones that actually do the resetting.
I'm okay to say that all vendor SYSTEM_RESET2 need to be treated like a
REBOOT_COLD, but we might run into issue in future where we might want
some vendor SYSTEM_RESET2 to act be closer to some other mode. I suppose
then a device-specific driver is needed there.
In the hypothetical situation where we need reboot_mode to be a specific
value for a vendor SYSTEM_RESET2, I like my current approach. Userspace
already needs to align the mode and command without the kernel enforcing
it, so it's possible we could still use the generic PSCI driver without
needing to write a device-specific driver to issue the vendor
SYSTEM_RESET2. If we hard-code that vendor SYSTEM_RESET2 must be like a
cold reset, then we definitely need a device-specific driver if we'd
rather it be like a warm or soft mode.
> I admit it is a tough nut to crack this one - apologies for the time
> it is taking to reach an agreement.
>
This is a weird one :) It seems simple at first but it flexes the design
of reboot mode and command. I appreciate the time you've taken to look
at this!
- Elliot
