| Message ID | 20241018-arm-psci-system_reset2-vendor-reboots-v6-3-50cbe88b0a24@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Implement vendor resets for PSCI SYSTEM_RESET2 | expand |
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? > + } > + } > +} > + > 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? > + 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 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 12:02:19PM -0700, Stephen Boyd wrote: > 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. > Ack. Thanks for the feedback! - Elliot
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
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-" + /* * 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. @@ -79,6 +81,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void) static u32 psci_cpu_suspend_feature; static bool psci_system_reset2_supported; +struct psci_reset_param { + const char *mode; + u32 reset_type; + u32 cookie; +}; +static struct psci_reset_param *psci_reset_params; +static size_t num_psci_reset_params; + static inline bool psci_has_ext_power_state(void) { return psci_cpu_suspend_feature & @@ -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); + } + } +} + 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])); + 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); + if (num < 0) { + 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; + param++; + num_psci_reset_params++; + } + + return 0; +} +arch_initcall(psci_init_system_reset2_modes); + int __init psci_dt_init(void) { struct device_node *np;
SoC vendors have different types of resets and are controlled through various registers. For instance, Qualcomm chipsets can reboot to a "download mode" that allows a RAM dump to be collected. Another example is they also support writing a cookie that can be read by bootloader during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these vendor reset types to be implemented without requiring drivers for every register/cookie. Add support in PSCI to statically map reboot mode commands from userspace to a vendor reset and cookie value using the device tree. A separate initcall is needed to parse the devicetree, instead of using psci_dt_init because mm isn't sufficiently set up to allocate memory. Reboot mode framework is close but doesn't quite fit with the design and requirements for PSCI SYSTEM_RESET2. Some of these issues can be solved but doesn't seem reasonable in sum: 1. reboot mode registers against the reboot_notifier_list, which is too early to call SYSTEM_RESET2. PSCI would need to remember the reset type from the reboot-mode framework callback and use it psci_sys_reset. 2. reboot mode assumes only one cookie/parameter is described in the device tree. SYSTEM_RESET2 uses 2: one for the type and one for cookie. 3. psci cpuidle driver already registers a driver against the arm,psci-1.0 compatible. Refactoring would be needed to have both a cpuidle and reboot-mode driver. Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> --- drivers/firmware/psci/psci.c | 92 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+)