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

[Ard Biesheuvel]

On Tue, 15 Feb 2022 at 17:57, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 2/15/22 07:01, Krzysztof Adamski wrote:
> > Dnia Tue, Feb 15, 2022 at 06:30:26AM -0800, Guenter Roeck napisał(a):
> >> On 2/15/22 00:44, Alexander Sverdlin wrote:
> >>> Hello Mark, Ard,
> >>>
> >>> On 01/02/2022 14:58, Mark Rutland wrote:
> >>>>> 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.
> >>>>
> >>>> As above, I think that's just using the wrong interface, and the reboot
> >>>> notifier mechanism *already* exists, so I'm really confused here.
> >>>>
> >>>> Have I misunderstood what you're trying to achieve?
> >>>>
> >>>> Is there some problem with the reboot notifier mechanism that I am unaware of?
> >>>> e.g. do we bypass them in some case where you think they're needed?
> >>>>
> >>>> Are you simply unaware of reboot notifiers?
> >>>
> >>> Could you please check the simple case of pwrseq_emmc.c?
> >>>
> >>> While that's currently the only example of this kind upstream I can imagine
> >>> further use-cases, especially in storage area like above.
> >>>
> >>> Would you suggest it's illegal usage of register_restart_handler()?
> >>> Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain
> >>> which will be called before restart handlers, so that it works on
> >>> emergency_restart()?
> >>>
> >>
> >> Abuse isn't just about using an API for something it isn't originally intended
> >> for, abuse is also to intentionally _not_ use an API, as it is currently done
> >> by the EFI restart code for arm64. Also, keep in mind that the same argument
> >> (our restart handler _must_ be executed under all circumstances and does therefore
> >> not use the restart API) is likely going to be used again in the future. All
> >> it takes is for some other standard (or chip vendor, for that matter) to declare
> >> their restart handler mandatory if present.
> >
> > Wait, but it is up to us to decide if we want to follow such standard or
> > not. If we want to have code that is more flexible, nobody can refuse us
> > to do so, right? None of the standards says that we can't support
> > restart handlers in case of EFI on ARM64, it was decided by kernel
> > developers, not some vendors. We can change that.
> >
>
> Of course it was decided by kernel developers. Point is that they use
> the EFI standard as argument for bypassing the API. What I am saying is
> that others can (and likely will, since the flood doors have been opened)
> do the same in the future, using the same line of arguments.
>

I don't think anyone was doing what you describe here. My primary
point was that platforms should not implement and expose EFI reset if
that implementation fails to deal with the platform's peripherals
adequately.

> >> Given that, I'd suggest to cut your losses and try to find another
> >> solution for your problem. That may be to introduce yet another API,
> >> one that is called before the EFI restart handling but that is always
> >> called, unlike reboot notifiers, or simply stick with out-of-tree code.
> >
> > Sure I could create yet another API like you suggest but in practice it
> > would be a copy of existing API as i would have to work exactly the
> > same - would be called at the same time in the same situations. The only
> > thing that would be different would be separate chain.
>
> Correct.
>
> > But if we want to prevent registering some custom code to be run before > efi_reboot(), that new API would have to be rejected as well, for the
> > same reason. So what is the point?
> >
>
> Ah, yes, you are right. The emmc example does reset the emmc, after all,
> which one could use as argument that it "violates" the EFI mandate.
> Sorry, I guess you'll be stuck with out-of-tree code (and, realistically,
> so is everyone using emmc in an arm64 based system with an EFI restart
> handler which does not implement emmc reset). Actually, turns out that
> the emmc restart handling code is not reliable anyway, since for example
> x86 doesn't use/support the restart handler call chain, and neither
> do several other architectures.
>

Rich firmware like EFI and ACPI implies that the firmware knows how to
manage the hardware.

> Interestingly, in this context, x86 isn't as inflexible as arm64 and does
> support other means to reset the system even in the presence of EFI
> (and actually seems to prefer ACPI reset over EFI reset unless I
> misunderstand the code).
>

No, correct. This is primarily for historical reasons and for parity
with Windows/x86 which does the same (which means that on many cheap
Wintel laptops, ACPI reset is the only thing that works)

> Other options for you might be to disable EFI restart handling in your
> system (assuming that is possible), or to implement the necessary code
> as part of the EFI restart handler, ie outside Linux, again if that is
> possible.
>

Either implement EFI reset properly, or not at all. Adding code to the
OS that forces it to reason about whether or not EFI reset can be
called safely simply means that the EFI implementation is broken and
should probably be avoided entirely.