| Message ID | YeVhtL2gCLkhTPdv@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: move efi_reboot to restart handler | expand |
Hi! On 17/01/2022 13:31, Krzysztof Adamski wrote: > On EFI enabled arm64 systems, efi_reboot was called before > do_kernel_restart, completely omitting the reset_handlers functionality. > By registering efi_reboot as part of the chain with slightly elevated > priority, we make it run before the default handler but still allow > plugging in other handlers. > Thanks to that, things like gpio_restart, restart handlers in > watchdog_core, mmc or mtds are working on those platforms. > > The priority 129 should be high enough as we will likely be the first > one to register on this prio so we will be called before others, like > PSCI handler. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > arch/arm64/kernel/process.c | 7 ------- > arch/arm64/kernel/setup.c | 21 +++++++++++++++++++++ > 2 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 5369e649fa79..b86ef77bb0c8 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -130,13 +130,6 @@ void machine_restart(char *cmd) > local_irq_disable(); > smp_send_stop(); > > - /* > - * UpdateCapsule() depends on the system being reset via > - * ResetSystem(). > - */ > - if (efi_enabled(EFI_RUNTIME_SERVICES)) > - efi_reboot(reboot_mode, NULL); > - > /* Now call the architecture specific reboot code. */ > do_kernel_restart(cmd); > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index f70573928f1b..5fa95980ba73 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -12,6 +12,7 @@ > #include <linux/stddef.h> > #include <linux/ioport.h> > #include <linux/delay.h> > +#include <linux/reboot.h> > #include <linux/initrd.h> > #include <linux/console.h> > #include <linux/cache.h> > @@ -298,6 +299,24 @@ u64 cpu_logical_map(unsigned int cpu) > return __cpu_logical_map[cpu]; > } > > +static int efi_restart(struct notifier_block *nb, unsigned long action, > + void *data) > +{ > + /* > + * UpdateCapsule() depends on the system being reset via > + * ResetSystem(). > + */ > + if (efi_enabled(EFI_RUNTIME_SERVICES)) > + efi_reboot(reboot_mode, NULL); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block efi_restart_nb = { > + .notifier_call = efi_restart, > + .priority = 129, > +}; > + > void __init __no_sanitize_address setup_arch(char **cmdline_p) > { > setup_initial_init_mm(_stext, _etext, _edata, _end); > @@ -346,6 +365,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p) > > paging_init(); > > + register_restart_handler(&efi_restart_nb); > + > acpi_table_upgrade(); > > /* Parse the ACPI tables for possible boot-time configuration */
Hi, On Mon, Jan 17, 2022 at 01:31:48PM +0100, Krzysztof Adamski wrote: > On EFI enabled arm64 systems, efi_reboot was called before > do_kernel_restart, completely omitting the reset_handlers functionality. > By registering efi_reboot as part of the chain with slightly elevated > priority, we make it run before the default handler but still allow > plugging in other handlers. > Thanks to that, things like gpio_restart, restart handlers in > watchdog_core, mmc or mtds are working on those platforms. > > The priority 129 should be high enough as we will likely be the first > one to register on this prio so we will be called before others, like > PSCI handler. I apprecaiate that this is kinda nice for consistency, but if adds more lines and reduces certainty down to "likely", neither of which seem ideal. What do we gain by changing this? e.g. does this enable some further rework? Do we actually need to change this? > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > --- > arch/arm64/kernel/process.c | 7 ------- > arch/arm64/kernel/setup.c | 21 +++++++++++++++++++++ > 2 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 5369e649fa79..b86ef77bb0c8 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -130,13 +130,6 @@ void machine_restart(char *cmd) > local_irq_disable(); > smp_send_stop(); > > - /* > - * UpdateCapsule() depends on the system being reset via > - * ResetSystem(). > - */ > - if (efi_enabled(EFI_RUNTIME_SERVICES)) > - efi_reboot(reboot_mode, NULL); > - > /* Now call the architecture specific reboot code. */ > do_kernel_restart(cmd); > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index f70573928f1b..5fa95980ba73 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -12,6 +12,7 @@ > #include <linux/stddef.h> > #include <linux/ioport.h> > #include <linux/delay.h> > +#include <linux/reboot.h> > #include <linux/initrd.h> > #include <linux/console.h> > #include <linux/cache.h> > @@ -298,6 +299,24 @@ u64 cpu_logical_map(unsigned int cpu) > return __cpu_logical_map[cpu]; > } > > +static int efi_restart(struct notifier_block *nb, unsigned long action, > + void *data) > +{ > + /* > + * UpdateCapsule() depends on the system being reset via > + * ResetSystem(). > + */ > + if (efi_enabled(EFI_RUNTIME_SERVICES)) > + efi_reboot(reboot_mode, NULL); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block efi_restart_nb = { > + .notifier_call = efi_restart, > + .priority = 129, > +}; > + > void __init __no_sanitize_address setup_arch(char **cmdline_p) > { > setup_initial_init_mm(_stext, _etext, _edata, _end); > @@ -346,6 +365,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p) > > paging_init(); > > + register_restart_handler(&efi_restart_nb); If we're going to register this, it'd be nicer to register it conditionally in the EFI code when we probe EFI, rather than having the arch setup code unconditionally register a notifier that conditionally does something. Thanks, Mark. > + > acpi_table_upgrade(); > > /* Parse the ACPI tables for possible boot-time configuration */ > -- > 2.34.1 >
Dnia Mon, Jan 17, 2022 at 01:10:56PM +0000, Mark Rutland napisał(a): >Hi, > >On Mon, Jan 17, 2022 at 01:31:48PM +0100, Krzysztof Adamski wrote: >> On EFI enabled arm64 systems, efi_reboot was called before >> do_kernel_restart, completely omitting the reset_handlers functionality. >> By registering efi_reboot as part of the chain with slightly elevated >> priority, we make it run before the default handler but still allow >> plugging in other handlers. >> Thanks to that, things like gpio_restart, restart handlers in >> watchdog_core, mmc or mtds are working on those platforms. >> >> The priority 129 should be high enough as we will likely be the first >> one to register on this prio so we will be called before others, like >> PSCI handler. > >I apprecaiate that this is kinda nice for consistency, but if adds more >lines and reduces certainty down to "likely", neither of which seem >ideal. Well, my choosing of the word "likely" might not be ideal. What I meant is that it is unlikely that anybody would ever add another restart handler with priority 129 in earlier code, as it would also have to be done in arch setup. We can, however, bump the priority to a larger value, of course. I just wanted to use tha smallest sensible value. Choosing the right priority is the hardest part of using reset_handlers mechanism, though. The direct mapping of the previous code to the restart handlers would use the maximal priority of 255, but this wouldn't make sense as the whole point of using restart_handler is to be able to register something with higer priority than default (in this case efi) reset mechanism. > >What do we gain by changing this? e.g. does this enable some further >rework? > >Do we actually need to change this? Well, it is not just nice, it is very useful. Without this change, the whole mechanism of restart_handlers does not work on a whole class of systems. We use this mechanism to inject our custom handler that should run just before restart but this is also used by a handful of mainline drivers that I mentioned in my commit message - none of them is gonna work in EFI based ARM64 systems right now - this includes the MTD/MMC drivers trying to do some work just before restart, gpio_restart driver that is used in many systems to trigger some external events just before restart, etc. So, for everybody who uses restart_handlers mechanism, this change is needed. > >> >> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >> --- >> arch/arm64/kernel/process.c | 7 ------- >> arch/arm64/kernel/setup.c | 21 +++++++++++++++++++++ >> 2 files changed, 21 insertions(+), 7 deletions(-) >> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >> index 5369e649fa79..b86ef77bb0c8 100644 >> --- a/arch/arm64/kernel/process.c >> +++ b/arch/arm64/kernel/process.c >> @@ -130,13 +130,6 @@ void machine_restart(char *cmd) >> local_irq_disable(); >> smp_send_stop(); >> >> - /* >> - * UpdateCapsule() depends on the system being reset via >> - * ResetSystem(). >> - */ >> - if (efi_enabled(EFI_RUNTIME_SERVICES)) >> - efi_reboot(reboot_mode, NULL); >> - >> /* Now call the architecture specific reboot code. */ >> do_kernel_restart(cmd); >> >> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c >> index f70573928f1b..5fa95980ba73 100644 >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -12,6 +12,7 @@ >> #include <linux/stddef.h> >> #include <linux/ioport.h> >> #include <linux/delay.h> >> +#include <linux/reboot.h> >> #include <linux/initrd.h> >> #include <linux/console.h> >> #include <linux/cache.h> >> @@ -298,6 +299,24 @@ u64 cpu_logical_map(unsigned int cpu) >> return __cpu_logical_map[cpu]; >> } >> >> +static int efi_restart(struct notifier_block *nb, unsigned long action, >> + void *data) >> +{ >> + /* >> + * UpdateCapsule() depends on the system being reset via >> + * ResetSystem(). >> + */ >> + if (efi_enabled(EFI_RUNTIME_SERVICES)) >> + efi_reboot(reboot_mode, NULL); >> + >> + return NOTIFY_DONE; >> +} >> + >> +static struct notifier_block efi_restart_nb = { >> + .notifier_call = efi_restart, >> + .priority = 129, >> +}; >> + >> void __init __no_sanitize_address setup_arch(char **cmdline_p) >> { >> setup_initial_init_mm(_stext, _etext, _edata, _end); >> @@ -346,6 +365,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p) >> >> paging_init(); >> >> + register_restart_handler(&efi_restart_nb); > >If we're going to register this, it'd be nicer to register it >conditionally in the EFI code when we probe EFI, rather than having the >arch setup code unconditionally register a notifier that conditionally >does something. > You might be right, I did a 1:1 translation of the code, so to say. I will look at the alternative approach for registering. Krzysztof
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 5369e649fa79..b86ef77bb0c8 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -130,13 +130,6 @@ void machine_restart(char *cmd) local_irq_disable(); smp_send_stop(); - /* - * UpdateCapsule() depends on the system being reset via - * ResetSystem(). - */ - if (efi_enabled(EFI_RUNTIME_SERVICES)) - efi_reboot(reboot_mode, NULL); - /* Now call the architecture specific reboot code. */ do_kernel_restart(cmd); diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index f70573928f1b..5fa95980ba73 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -12,6 +12,7 @@ #include <linux/stddef.h> #include <linux/ioport.h> #include <linux/delay.h> +#include <linux/reboot.h> #include <linux/initrd.h> #include <linux/console.h> #include <linux/cache.h> @@ -298,6 +299,24 @@ u64 cpu_logical_map(unsigned int cpu) return __cpu_logical_map[cpu]; } +static int efi_restart(struct notifier_block *nb, unsigned long action, + void *data) +{ + /* + * UpdateCapsule() depends on the system being reset via + * ResetSystem(). + */ + if (efi_enabled(EFI_RUNTIME_SERVICES)) + efi_reboot(reboot_mode, NULL); + + return NOTIFY_DONE; +} + +static struct notifier_block efi_restart_nb = { + .notifier_call = efi_restart, + .priority = 129, +}; + void __init __no_sanitize_address setup_arch(char **cmdline_p) { setup_initial_init_mm(_stext, _etext, _edata, _end); @@ -346,6 +365,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p) paging_init(); + register_restart_handler(&efi_restart_nb); + acpi_table_upgrade(); /* Parse the ACPI tables for possible boot-time configuration */
On EFI enabled arm64 systems, efi_reboot was called before do_kernel_restart, completely omitting the reset_handlers functionality. By registering efi_reboot as part of the chain with slightly elevated priority, we make it run before the default handler but still allow plugging in other handlers. Thanks to that, things like gpio_restart, restart handlers in watchdog_core, mmc or mtds are working on those platforms. The priority 129 should be high enough as we will likely be the first one to register on this prio so we will be called before others, like PSCI handler. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> --- arch/arm64/kernel/process.c | 7 ------- arch/arm64/kernel/setup.c | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-)