| Message ID | 20241018-arm-psci-system_reset2-vendor-reboots-v6-0-50cbe88b0a24@quicinc.com |
|---|---|
| Headers | show |
| Series | Implement vendor resets for PSCI SYSTEM_RESET2 | expand |
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. > > + } > > + } > > +} > > + > > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > > void *data) > > { > > + if (data && num_psci_reset_params) > > + psci_vendor_sys_reset2(action, data); > > + > > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > psci_system_reset2_supported) { > > /* > > @@ -750,6 +780,68 @@ static const struct of_device_id psci_of_match[] __initconst = { > > {}, > > }; > > > > +static int __init psci_init_system_reset2_modes(void) > > +{ > > + const size_t len = strlen(REBOOT_PREFIX); > > + struct psci_reset_param *param; > > + struct device_node *psci_np __free(device_node) = NULL; > > + struct device_node *np __free(device_node) = NULL; > > + struct property *prop; > > + size_t count = 0; > > + u32 magic[2]; > > + int num; > > + > > + if (!psci_system_reset2_supported) > > + return 0; > > + > > + psci_np = of_find_matching_node(NULL, psci_of_match); > > + if (!psci_np) > > + return 0; > > + > > + np = of_find_node_by_name(psci_np, "reset-types"); > > + if (!np) > > + return 0; > > + > > + for_each_property_of_node(np, prop) { > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > + continue; > > + num = of_property_count_elems_of_size(np, prop->name, sizeof(magic[0])); > > Use of_property_count_u32_elems()? > > > + if (num != 1 && num != 2) > > + continue; > > + > > + count++; > > + } > > + > > + param = psci_reset_params = kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL); > > + if (!psci_reset_params) > > + return -ENOMEM; > > + > > + for_each_property_of_node(np, prop) { > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > + continue; > > + > > + param->mode = kstrdup_const(prop->name + len, GFP_KERNEL); > > + if (!param->mode) > > + continue; > > + > > + num = of_property_read_variable_u32_array(np, prop->name, magic, 1, 2); > > ARRAY_SIZE(magic)? > > > + if (num < 0) { > > Should this be less than 1? > of_property_read_variable_u32_array should return -EOVERFLOW (or maybe -ENODATA) if the array is empty. I don't see it's possible for of_property_read_variable_u32_array() to return a non-negative value that's not 1 or 2. > > + pr_warn("Failed to parse vendor reboot mode %s\n", param->mode); > > + kfree_const(param->mode); > > + continue; > > + } > > + > > + /* Force reset type to be in vendor space */ > > + param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0]; > > + param->cookie = num == 2 ? magic[1] : 0; > > ARRAY_SIZE(magic)? > > > + param++; > > + num_psci_reset_params++; > > + } > > + > > + return 0;
Quoting Elliot Berman (2024-10-23 09:30:21) > 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 > > > @@ -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. Ok. Please add a comment in the code so we know that it's intentional. > > > > + } > > > + } > > > +} > > > + > > > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > > > void *data) > > > { > > > + if (data && num_psci_reset_params) > > > + psci_vendor_sys_reset2(action, data); > > > + I'd add a comment here as well indicating that a fallback is used. > > > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > > psci_system_reset2_supported) { > > > /* > > > @@ -750,6 +780,68 @@ static const struct of_device_id psci_of_match[] __initconst = { > > > {}, [...] > > > + continue; > > > + > > > + num = of_property_read_variable_u32_array(np, prop->name, magic, 1, 2); > > > > ARRAY_SIZE(magic)? > > > > > + if (num < 0) { > > > > Should this be less than 1? > > > > of_property_read_variable_u32_array should return -EOVERFLOW (or maybe > -ENODATA) if the array is empty. I don't see it's possible for > of_property_read_variable_u32_array() to return a non-negative value > that's not 1 or 2. I think you're saying a return value of 0 is impossible? Ok. I was mostly looking at the usage of the return value later on in this patch and trying to understand why 0 would be allowed as a possible return value without looking at the details of of_property_read_variable_u32_array(). I guess the 1, 2 are the min/max though so it's fine.
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 ? Are these "reset types" exported to user space ? Lorenzo > > > + } > > > + } > > > +} > > > + > > > static int psci_sys_reset(struct notifier_block *nb, unsigned long action, > > > void *data) > > > { > > > + if (data && num_psci_reset_params) > > > + psci_vendor_sys_reset2(action, data); > > > + > > > if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) && > > > psci_system_reset2_supported) { > > > /* > > > @@ -750,6 +780,68 @@ static const struct of_device_id psci_of_match[] __initconst = { > > > {}, > > > }; > > > > > > +static int __init psci_init_system_reset2_modes(void) > > > +{ > > > + const size_t len = strlen(REBOOT_PREFIX); > > > + struct psci_reset_param *param; > > > + struct device_node *psci_np __free(device_node) = NULL; > > > + struct device_node *np __free(device_node) = NULL; > > > + struct property *prop; > > > + size_t count = 0; > > > + u32 magic[2]; > > > + int num; > > > + > > > + if (!psci_system_reset2_supported) > > > + return 0; > > > + > > > + psci_np = of_find_matching_node(NULL, psci_of_match); > > > + if (!psci_np) > > > + return 0; > > > + > > > + np = of_find_node_by_name(psci_np, "reset-types"); > > > + if (!np) > > > + return 0; > > > + > > > + for_each_property_of_node(np, prop) { > > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > > + continue; > > > + num = of_property_count_elems_of_size(np, prop->name, sizeof(magic[0])); > > > > Use of_property_count_u32_elems()? > > > > > + if (num != 1 && num != 2) > > > + continue; > > > + > > > + count++; > > > + } > > > + > > > + param = psci_reset_params = kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL); > > > + if (!psci_reset_params) > > > + return -ENOMEM; > > > + > > > + for_each_property_of_node(np, prop) { > > > + if (strncmp(prop->name, REBOOT_PREFIX, len)) > > > + continue; > > > + > > > + param->mode = kstrdup_const(prop->name + len, GFP_KERNEL); > > > + if (!param->mode) > > > + continue; > > > + > > > + num = of_property_read_variable_u32_array(np, prop->name, magic, 1, 2); > > > > ARRAY_SIZE(magic)? > > > > > + if (num < 0) { > > > > Should this be less than 1? > > > > of_property_read_variable_u32_array should return -EOVERFLOW (or maybe > -ENODATA) if the array is empty. I don't see it's possible for > of_property_read_variable_u32_array() to return a non-negative value > that's not 1 or 2. > > > > + pr_warn("Failed to parse vendor reboot mode %s\n", param->mode); > > > + kfree_const(param->mode); > > > + continue; > > > + } > > > + > > > + /* Force reset type to be in vendor space */ > > > + param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0]; > > > + param->cookie = num == 2 ? magic[1] : 0; > > > > ARRAY_SIZE(magic)? > > > > > + param++; > > > + num_psci_reset_params++; > > > + } > > > + > > > + return 0;
On 11/15/24 05:35, 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 ? > > Are these "reset types" exported to user space ? AFAICT, they are not, but arguably you already need custom user space which is capable of doing: syscall(SYS_reboot, LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, LINUX_REBOOT_CMD_RESTART2, reboot_cmd); in order to utilize the custom reboot mode. I could imagine that with a discovery mechanism, a wrapper could be written to check that the specified command is actually supported before issuing the system call, or even have the system call do that under the hood. I don't personally feel like this is very important in the sense that as long as a fallback exists for an unsupported reboot command specified, the system does reboot.
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. > > 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. - Elliot
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
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
The PSCI SYSTEM_RESET2 call allows vendor firmware to define additional reset types which could be mapped to the reboot argument. Setting up reboot on Qualcomm devices can be inconsistent from chipset to chipset. Generally, there is a PMIC register that gets written to decide the reboot type. There is also sometimes a cookie that can be written to indicate that the bootloader should behave differently than a regular boot. These knobs evolve over product generations and require more drivers. Qualcomm firmwares are beginning to expose vendor SYSTEM_RESET2 types to simplify driver requirements from Linux. Add support in PSCI to statically wire reboot mode commands from userspace to a vendor reset and cookie value using the device tree. The DT bindings are similar to reboot mode framework except that 2 integers are accepted (the type and cookie). Also, reboot mode framework is intended to program the cookies, but not actually reboot the host. PSCI SYSTEM_RESET2 does both. I've not added support for reading ACPI tables since I don't have any device which provides them + firmware that supports vendor SYSTEM_RESET2 types. --- Lorenzo and I are also looking for some feedback on whether it is safe to perform a vendor SYSTEM_RESET2 irrespective of the enum reboot_mode: https://lore.kernel.org/all/Zw5ffeYW5uRpsaG3@lpieralisi/ --- Previous discussions around SYSTEM_RESET2: - https://lore.kernel.org/lkml/20230724223057.1208122-2-quic_eberman@quicinc.com/T/ - https://lore.kernel.org/all/4a679542-b48d-7e11-f33a-63535a5c68cb@quicinc.com/ To: Bjorn Andersson <andersson@kernel.org> To: Konrad Dybcio <konrad.dybcio@linaro.org> To: Sebastian Reichel <sre@kernel.org> To: Rob Herring <robh@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Conor Dooley <conor+dt@kernel.org> To: Vinod Koul <vkoul@kernel.org> To: Andy Yan <andy.yan@rock-chips.com> To: Lorenzo Pieralisi <lpieralisi@kernel.org> To: Mark Rutland <mark.rutland@arm.com> To: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> To: Arnd Bergmann <arnd@arndb.de> To: Olof Johansson <olof@lixom.net>A To: Catalin Marinas <catalin.marinas@arm.com> To: Will Deacon <will@kernel.org> To: cros-qcom-dts-watchers@chromium.org Cc: Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com> Cc: Melody Olvera <quic_molvera@quicinc.com> Cc: Shivendra Pratap <quic_spratap@quicinc.com> Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: Florian Fainelli <florian.fainelli@broadcom.com> Cc: linux-pm@vger.kernel.org Cc: linux-arm-msm@vger.kernel.org Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> Changes in v6: - Rebase to v6.11 and fix trivial conflicts in qcm6490-idp - Add sa8775p-ride support (same as qcm6490-idp) - Link to v5: https://lore.kernel.org/r/20240617-arm-psci-system_reset2-vendor-reboots-v5-0-086950f650c8@quicinc.com Changes in v5: - Drop the nested "items" in prep for future dtschema tools - Link to v4: https://lore.kernel.org/r/20240611-arm-psci-system_reset2-vendor-reboots-v4-0-98f55aa74ae8@quicinc.com Changes in v4: - Change mode- properties from uint32-matrix to uint32-array - Restructure the reset-types node so only the restriction is in the if/then schemas and not the entire definition - Link to v3: https://lore.kernel.org/r/20240515-arm-psci-system_reset2-vendor-reboots-v3-0-16dd4f9c0ab4@quicinc.com Changes in v3: - Limit outer number of items to 1 for mode-* properties - Move the reboot-mode for psci under a subnode "reset-types" - Fix the DT node in qcm6490-idp so it doesn't overwrite the one from sc7820.dtsi - Link to v2: https://lore.kernel.org/r/20240414-arm-psci-system_reset2-vendor-reboots-v2-0-da9a055a648f@quicinc.com Changes in v2: - Fixes to schema as suggested by Rob and Krzysztof - Add qcm6490 idp as first Qualcomm device to support - Link to v1: https://lore.kernel.org/r/20231117-arm-psci-system_reset2-vendor-reboots-v1-0-03c4612153e2@quicinc.com Changes in v1: - Reference reboot-mode bindings as suggeted by Rob. - Link to RFC: https://lore.kernel.org/r/20231030-arm-psci-system_reset2-vendor-reboots-v1-0-dcdd63352ad1@quicinc.com To: Sebastian Reichel <sre@kernel.org> To: Rob Herring <robh@kernel.org> To: Krzysztof Kozlowski <krzk+dt@kernel.org> To: Conor Dooley <conor+dt@kernel.org> To: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> To: Vinod Koul <vkoul@kernel.org> To: Andy Yan <andy.yan@rock-chips.com> To: Lorenzo Pieralisi <lpieralisi@kernel.org> To: Mark Rutland <mark.rutland@arm.com> To: Bjorn Andersson <andersson@kernel.org> To: Konrad Dybcio <konradybcio@kernel.org> To: cros-qcom-dts-watchers@chromium.org Cc: linux-pm@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-arm-msm@vger.kernel.org --- Elliot Berman (5): dt-bindings: power: reset: Convert mode-.* properties to array dt-bindings: arm: Document reboot mode magic firmware: psci: Read and use vendor reset types arm64: dts: qcom: Add PSCI SYSTEM_RESET2 types for qcm6490-idp arm64: dts: qcom: Add PSCI SYSTEM_RESET2 types for sa8775p-ride Documentation/devicetree/bindings/arm/psci.yaml | 43 ++++++++++ .../bindings/power/reset/nvmem-reboot-mode.yaml | 4 + .../devicetree/bindings/power/reset/qcom,pon.yaml | 7 ++ .../bindings/power/reset/reboot-mode.yaml | 4 +- .../bindings/power/reset/syscon-reboot-mode.yaml | 4 + arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 7 ++ arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 7 ++ arch/arm64/boot/dts/qcom/sa8775p.dtsi | 2 +- arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +- drivers/firmware/psci/psci.c | 92 ++++++++++++++++++++++ include/linux/arm-smccc.h | 5 ++ 11 files changed, 173 insertions(+), 4 deletions(-) --- base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652 change-id: 20231016-arm-psci-system_reset2-vendor-reboots-cc3ad456c070 Best regards,