Re: [PATCH] arm64: move efi_reboot to restart handler

[Krzysztof Adamski]

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@xxxxxxxxx>
> > ---
> >  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