Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Tue, 15 Feb 2022 16:01:15 +0100
From:   Krzysztof Adamski <krzysztof.adamski@nokia.com>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Alexander Sverdlin <alexander.sverdlin@nokia.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Ard Biesheuvel <ardb@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Peter Collingbourne <pcc@google.com>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        Matija Glavinic-Pecotic <matija.glavinic-pecotic.ext@nokia.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-efi@vger.kernel.org
Subject: Re: [PATCH v2] arm64: move efi_reboot to restart handler
Message-ID: <YgvAO1UbZcbcj5na@localhost.localdomain>
References: <YfP0osb45uJldtM9@localhost.localdomain>
 <YfQTZTUNaeGi+8tG@FVFF77S0Q05N>
 <YfRorCpk0seVGI+5@localhost.localdomain>
 <Yfk8hQB1eon7oeYU@FVFF77S0Q05N>
 <79bcce92-abb2-4c3e-7193-d18e144da8a0@nokia.com>
 <489b76f9-fbaf-dae0-c90d-c52ee0de92a4@roeck-us.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <489b76f9-fbaf-dae0-c90d-c52ee0de92a4@roeck-us.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Tk8rdGNSMDRUZVhESFBOYWJ2bzhwVjNybndrQyttYldGZ1N2T2dNOHUwSUZx?=
 =?utf-8?B?TzhRdFdCaW1Xano3eTQ5ejJFaXdvQ3FuNDdFOXJ3TTJZZHV2ODJTUUdqZXlq?=
 =?utf-8?B?aGdSaXIzYTJGN3NOd1hhaUhPcFBvYTVjcnFSakVjcEVmQmoyR1hCUS84bWxt?=
 =?utf-8?B?RE56ZlJPeDZIRHUwOGJabHJPUE5vTWJ1ZVY5eGNpTnZ0L1ltTjJlWlBIYmMw?=
 =?utf-8?B?ejhCYUFURUZKWjZCL3c1Lzc3NzR6OGFXNVFFb2srQ1FtMi9wamg3RGxabGY2?=
 =?utf-8?B?d21WZk55L1dqbDJRdk4xakhjWWZlcmw1d0RhaStRSXU4YlQzc1BLSWhCbXp0?=
 =?utf-8?B?d0pCWDlZUExYNlcrS1JySFZ3b1R6M2tDbmErMDdZWUZid0NUQlpyYWIrMlhv?=
 =?utf-8?B?NTAyTkZ6QXgvRElSSGp5UUx2dGlqVGNKREJ2UGdEZDhpbjMxVmduV1dkenJ6?=
 =?utf-8?B?alVlNzRPUHNNcjhrMG8rVS9CZ1d3SGlzOXhzRCtKRjJJY1NWOHBjWkIwQmRW?=
 =?utf-8?B?b01tTDllWUtYUGN5UThoOW82Qi9qaWFhRzBCQ1NwVUhaRytWWEVuOE4zbW5C?=
 =?utf-8?B?SlIzL3BCeFRiU3hJTmtpdjVLUnZNVDZkNmpzdGVicGdMaGtwTVRvb2R1SjRU?=
 =?utf-8?B?aEhwTktkUjJmeVZQR0VqZlJLc3ptRERVeEEzT1k2ZmFKbVV2a2hmMm1PMGkz?=
 =?utf-8?B?NzhUY3NhemdjOVRTSGdHZVo2dWQ3aVEzNFFteUtNZmxRRmJ6TkRERlpCa3VC?=
 =?utf-8?B?R2xTbWY5TW9OUEluR2kvRXBUbTNreXFFVW5ORzdmcGNaWFQxc2pzRGhqcTd1?=
 =?utf-8?B?b3JWcHgwZDZzc2g2aitkNzRQSmxVWjhsdkxtRVpjZnlCVGhnWVRYTUhXd1gw?=
 =?utf-8?B?eGwyTzFXcGxaQXdvNUdiV01ERVJhS2R6eC9oa0ZIUC9FMmI2QkhDTVMvdjhx?=
 =?utf-8?B?MEtVV2l5Sko4bGRBYnBEaDVNc1paeC91cUJoSlM4YlBYOXZ1aE13a0pXeEsz?=
 =?utf-8?B?eWg5c0VPdWpFWVFoZHUvZm9sTnlUSTlTREpteDM3dHdIYlpWUUd1NlBlSytT?=
 =?utf-8?B?cXFweUxWclRWRUR1R3JEK0xLaHEza3dMU3ZXeG9uTTMyRWNjejRtZ25scWxh?=
 =?utf-8?B?bHhPTHpleWxJZkMxRGgvR1RRUmt5SUNEd2FtT0R0ZXVGL1I2MXVNQzRXdlND?=
 =?utf-8?B?SjdnaGs4d2dEblA2Mm1TYTY2Sytnd2Vrb0xxc3d4UWVITlQ4c0tyb1pZeCtE?=
 =?utf-8?B?SVh6WlRGUHNtSEgzcWVmQ1k5MlVsRG9xRVc3MzIwc3lHQVd0ZVBjcm1pYnlH?=
 =?utf-8?B?OHl3UllKYStnellSQUthY3FpMVZuZkVncXRXanpKL3dtTlZTTDdmb1B1Zmhr?=
 =?utf-8?B?YU1mbnRxRkYwM09PSEUxeGdYazB0ZHdYMmlYWVBLemFSb3J0SmRTU2o5aFRP?=
 =?utf-8?B?ZVZSTzRGMGZkbXFoSldESnhsZHpUOE41L3MzYXM2UXpBT3I4OVRZK1ZKdVlT?=
 =?utf-8?B?YlhOR1Bpb3c0MGVjSzBiNmpQVE5rQlBQNE9kQ2o0MnFiSjBWdHhQTXRxK3VG?=
 =?utf-8?B?elBOQXNXbmU2M042eDBnd3M4ZWNidlpxM1NkcHBLbDVQSVN5eHZORE44ZnBx?=
 =?utf-8?B?bHRSOGp1Q1lVSTF4RlZNdDhaY29Bc1pSZXloczVQMldJOVdwOTBIQm9lQlF3?=
 =?utf-8?B?VFdRRmt5eFp1M1BmU2J3dkdoZFI1VmYyRzhNK1FiL1QwcmVKWEd3L1ZFSWQ1?=
 =?utf-8?B?Ymo0eEVvMlJQL016UmpxQU5JUWhNa2NNVUpsTWdRaEZyeEtnNEdKNlRLbGNh?=
 =?utf-8?B?SDRpVEFsbWtDY0tQRk5MWkxUUHpVQWsxTEc4djR5SVEzMGU1THFiVnJHVnEw?=
 =?utf-8?B?TE5DM1JRUDVpbitVdWllMnN0S09vREpjajlPOU9MSTM2bmxvSEwyRExteWhY?=
 =?utf-8?B?VzZrYjB0MTFaRmM3NVVsZXFvR1dpblRzMENySmNNcWJWZzNyNmdFWk5zbnZ3?=
 =?utf-8?B?U212eERDbDc0b3JiNDdWelV5MkRDNDlrRUV6QzVKL3pFcXdHazh0cDRwWWRu?=
 =?utf-8?B?ZW1kamwwcTZ6Zk0wZFcvN1d6T2Z6ako3WC9mNUFuS3puYWJOYmwxay9QSGs5?=
 =?utf-8?B?ZGZVdE1nSHNVY3o4WDBENk5rSElsWnZqdXorcHpZSzdCU2RvaTVybjVmeFU2?=
 =?utf-8?B?Zm94ajhjekI2c1ZmdDQveGtYYkVqdmpNTEt2L1AxR0Q5TVcvZkY2aXBWQ3FY?=
 =?utf-8?B?QjhJUU91dG5BenkxOEZnZ1NZNlhBPT0=?=
X-OriginatorOrg: nokia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8a922f2e-6194-43d6-3148-08d9f094065a
X-MS-Exchange-CrossTenant-AuthSource: DU2PR07MB8110.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2022 15:01:20.4379
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5d471751-9675-428d-917b-70f44f9630b0
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KLpxl+WnrXmEcch16zA4VSpSqvVbIZYruRxtYwRczZb6M0taMm1jEhC5gKVJxCiDGJd3xH4NvTJ1jfp91N8H+Up3zU1ePorzLKVLXmZboVY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR07MB8149
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.

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

Krzysztof