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

[Krzysztof Adamski]

Hi Mark,

Thank you for your feedback. I don't know UEFI specification that much,
it is quite complicated so maybe I am misunderstnading something. Let's
clarify.

Dnia Fri, Jan 28, 2022 at 04:01:41PM +0000, Mark Rutland napisał(a):
> On Fri, Jan 28, 2022 at 02:50:26PM +0100, Krzysztof Adamski wrote:
> > On EFI enabled arm64 systems, efi_reboot was called before
> > do_kernel_restart, completely omitting the reset_handlers functionality.
>
> As I pointed out before, per the EFI spec we *need* to do this before any other
> restart mechanism so that anything which EFI ties to the restart actually
> occurs as expected -- e.g. UpdateCapsule(), as the comment says.
> AFAICT, either:
>
> * The other restart handlers have lower priority, in which case they'll be
>  called after this anyway, and in such cases this patch is not necessary.

But efi_reboot calls ResetSystem(), which according to the spec:
"Resets all processors and devices and reboots the system."

So nothing should be able to run *after* this call. Thus, none of the
reset handlers will ever run on a system where efi_reboot() is used
(with current, unmodified kernel code) so this case is invalid.

> * At least one other restart handler has higher priority, and the EFI restart
>  isn't actually used, and so any functionaltiy tied to the EFI restart will
>  not work on that machine.

If we use the restart handlers only to reset the system, this is indeed
true. But technically, restart handlers support the scenario where the
handler does some action that does not do reset of the whole system and
passes the control further down the chain, eventually reaching a handler
that will reset the whole system.
This can be done on non-uefi systems without problems but it doesn't
work on UEFI bases arm64 systems and this is a problem for us.

In other words, I would like to be able to run a restart handler on EFI
based ARM64 systems, just like I can on other systems, just for its
"side effects", not to do the actual reboot. Current code disables this
possibility on an ARM64 EFI system.

> > 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.
>
> On which platforms is it necessary that these are used in preference to the EFI
> restart? Can you please give a specific example?

> If there's a specific platform that needs this, then we should call that out
> and explain why the EFI restart isn't actually required on that (or if it is,
> and functionality is broken, why that's acceptable).

Again, I'm not saying EFI restart is not required. I would just like to
have the flexibility I have on other architectures, and run some code
just before restart. My understanding is that pwrseq_emmc.c driver does
exactly that, for example, but there are also other possibilities:
 - using gpio-restart to notify external components about reset of the
   CPU
 - starting an external watchdog that makes sure we are not stuck in the
   reset procedure, etc.

> Otherwise this patch is making this logic more complicated *and* making it
> possible to have problems which we avoid by construction today, without any
> actual benefit.

The benefit for me is this added flexibility and unification between
architectures. On other systems, like ARM32 or non-EFI ARM64 I can
insert a custom reset handler to do some actions just before restart and
on EFI based ARM64, I can't.

You could argue that restart handlers were not created for that but they
suit this purpose ideally and it wouldn't make much sense (in my
opinion) to add yet another notifier chain that would run before reset
notifiers, for code that is not supposed to reset the whole system and
this is exacly what I would have to do if efi_reboot() is forced to be
called before all handlers.

Best regards,
Krzysztof Adamski