* [PATCH] arm64: move efi_reboot to restart handler @ 2022-01-17 12:31 Krzysztof Adamski 2022-01-17 12:44 ` Alexander Sverdlin 2022-01-17 13:10 ` Mark Rutland 0 siblings, 2 replies; 4+ messages in thread From: Krzysztof Adamski @ 2022-01-17 12:31 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Mark Rutland, Peter Collingbourne, Guenter Roeck, Wolfram Sang Cc: Alexander Sverdlin, Matija Glavinic-Pecotic, linux-arm-kernel, linux-kernel 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(-) 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 */ -- 2.34.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] arm64: move efi_reboot to restart handler 2022-01-17 12:31 [PATCH] arm64: move efi_reboot to restart handler Krzysztof Adamski @ 2022-01-17 12:44 ` Alexander Sverdlin 2022-01-17 13:10 ` Mark Rutland 1 sibling, 0 replies; 4+ messages in thread From: Alexander Sverdlin @ 2022-01-17 12:44 UTC (permalink / raw) To: Krzysztof Adamski, Catalin Marinas, Will Deacon, Mark Rutland, Peter Collingbourne, Guenter Roeck, Wolfram Sang Cc: Matija Glavinic-Pecotic, linux-arm-kernel, linux-kernel 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 */ -- Best regards, Alexander Sverdlin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] arm64: move efi_reboot to restart handler 2022-01-17 12:31 [PATCH] arm64: move efi_reboot to restart handler Krzysztof Adamski 2022-01-17 12:44 ` Alexander Sverdlin @ 2022-01-17 13:10 ` Mark Rutland 2022-01-17 17:58 ` Krzysztof Adamski 1 sibling, 1 reply; 4+ messages in thread From: Mark Rutland @ 2022-01-17 13:10 UTC (permalink / raw) To: Krzysztof Adamski Cc: Catalin Marinas, Will Deacon, Peter Collingbourne, Guenter Roeck, Wolfram Sang, Alexander Sverdlin, Matija Glavinic-Pecotic, linux-arm-kernel, linux-kernel 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] arm64: move efi_reboot to restart handler 2022-01-17 13:10 ` Mark Rutland @ 2022-01-17 17:58 ` Krzysztof Adamski 0 siblings, 0 replies; 4+ messages in thread From: Krzysztof Adamski @ 2022-01-17 17:58 UTC (permalink / raw) To: Mark Rutland Cc: Catalin Marinas, Will Deacon, Peter Collingbourne, Guenter Roeck, Wolfram Sang, Alexander Sverdlin, Matija Glavinic-Pecotic, linux-arm-kernel, linux-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-17 18:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-17 12:31 [PATCH] arm64: move efi_reboot to restart handler Krzysztof Adamski 2022-01-17 12:44 ` Alexander Sverdlin 2022-01-17 13:10 ` Mark Rutland 2022-01-17 17:58 ` Krzysztof Adamski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).