| Message ID | 20241018-arm-psci-system_reset2-vendor-reboots-v6-3-50cbe88b0a24@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
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
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(+)