arm64: move efi_reboot to restart handler
PATCH 1/1 arm64: move efi_reboot to restart handler
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.
--- arch/arm64/kernel/process.c | 7 ------- arch/arm64/kernel/setup.c | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-)
arch/arm64/kernel/process.c +0 -7
@@ -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);
arch/arm64/kernel/setup.c +21 -0
@@ -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 */
Signature · 1 line
2.34.1
Hi!
Quoted 8 lines · Krzysztof Adamski
On EFI enabled arm64 systems,
efi_rebootwas called beforedo_kernel_restart, completely omitting thereset_handlersfunctionality. By registeringefi_rebootas 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 likegpio_restart, restart handlers inwatchdog_core, mmc or mtds are working on those platforms.
Quoted 5 lines
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 <[email protected]>
Quoted 69 lines
--- arch/arm64/kernel/process.c | 7 ------- arch/arm64/kernel/setup.c | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-)arch/arm64/kernel/process.c +0
-7@@ -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);arch/arm64/kernel/setup.c +21
-0@@ -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 */
Signature · 2 lines
Best regards, Alexander Sverdlin.
Mark Rutland
reviewerHi,
Quoted 11 lines · Krzysztof Adamski
On EFI enabled arm64 systems,
efi_rebootwas called beforedo_kernel_restart, completely omitting thereset_handlersfunctionality. By registeringefi_rebootas 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 likegpio_restart, restart handlers inwatchdog_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?
Quoted 63 lines
Signed-off-by Krzysztof Adamski <[email protected]>--- arch/arm64/kernel/process.c | 7 ------- arch/arm64/kernel/setup.c | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-)arch/arm64/kernel/process.c +0
-7@@ -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);arch/arm64/kernel/setup.c +19
-0@@ -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);
arch/arm64/kernel/setup.c void __init __no_sanitize_address setup_arch(char **cmdline_p) · line 368
Quoted 4 lines
arch/arm64/kernel/setup.c +1
-0@@ -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.
Quoted 7 lines
arch/arm64/kernel/setup.c +1
-0+acpi_table_upgrade ();/* Parse the ACPI tables for possible boot-time configuration */ Signature · 1 line
2.34.1
Dnia Mon, Jan 17, 2022 at 01:10:56PM +0000, Mark Rutland napisał(a):
Quoted 11 lines
Hi,
Quoted 8 lines · Krzysztof Adamski
On EFI enabled arm64 systems,
efi_rebootwas called beforedo_kernel_restart, completely omitting thereset_handlersfunctionality. By registeringefi_rebootas 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 likegpio_restart, restart handlers inwatchdog_core, mmc or mtds are working on those platforms.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 65 lines
Quoted 65 lines
Signed-off-by Krzysztof Adamski <[email protected]>--- arch/arm64/kernel/process.c | 7 ------- arch/arm64/kernel/setup.c | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-)arch/arm64/kernel/process.c +0
-7@@ -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);arch/arm64/kernel/setup.c +19
-0@@ -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)
Quoted 9 lines
arch/arm64/kernel/setup.c void __init __no_sanitize_address setup_arch(char **cmdline_p) · line 368
Quoted 3 lines
arch/arm64/kernel/setup.c +1
-0paging_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