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. > > 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). I admit it is a tough nut to crack this one - apologies for the time it is taking to reach an agreement. Lorenzo