Message-ID: <be9cc279-1d62-3d57-9347-6cfaffc00cb4@roeck-us.net>
Date:   Tue, 15 Feb 2022 08:57:17 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Krzysztof Adamski <krzysztof.adamski@...ia.com>
Cc:     Alexander Sverdlin <alexander.sverdlin@...ia.com>,
        Mark Rutland <mark.rutland@....com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Peter Collingbourne <pcc@...gle.com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Matija Glavinic-Pecotic <matija.glavinic-pecotic.ext@...ia.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-efi@...r.kernel.org
Subject: Re: [PATCH v2] arm64: move efi_reboot to restart handler

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.

>> 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.

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).

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.

Guenter

Powered by blists - more mailing lists