Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 28 Jan 2022 23:32:19 +0100
From:   Krzysztof Adamski <krzysztof.adamski@nokia.com>
To:     Wolfram Sang <wsa+renesas@sang-engineering.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Peter Collingbourne <pcc@google.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Alexander Sverdlin <alexander.sverdlin@nokia.com>,
        Matija Glavinic-Pecotic <matija.glavinic-pecotic.ext@nokia.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] arm64: move efi_reboot to restart handler
Message-ID: <YfRu86YoiFpVsqyn@localhost.localdomain>
References: <YfP0osb45uJldtM9@localhost.localdomain>
 <YfREAiRMjQ9ci4LC@shikoro>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YfREAiRMjQ9ci4LC@shikoro>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SDIrSkZWRTQ3Y1JyRVU1bjVQeVg4OFgxM1BqeWxOdW12ZThWM25CNzZjWFpC?=
 =?utf-8?B?RmdzL29LQ0FJUUM0M215VkVxK2M3b1RSSDhTdnhxTC9XSnYrRFJ1SjduWWxG?=
 =?utf-8?B?ZnNGN1IzU3o0MmxuWms2SktDWHZ3ZFlUWEtZZzhxU2FOZmJwai9WdmhUbUQv?=
 =?utf-8?B?V3QwcmRha21EUzFEOHE0K2E1SXF5VzRRTmlLZTRMbEhRUmkwV0w0SFQvUEMw?=
 =?utf-8?B?S1B5ZnBGZFl1U2xua0hPUjYzWXNqaGExWlZHbmVZaEFrTnY5TnhKWEdmalhW?=
 =?utf-8?B?TGQzVG5LaklsQ0MyaHczbW5KMXlvR2VDMWRpcXl5anhpWGxqMFBwditJWkh6?=
 =?utf-8?B?c2hUdG9JajBwaFcrY01rYWprWUp5cHNwdlBEZ2thQXdDZDNzZEM5UnlHM3dX?=
 =?utf-8?B?YmhkOFphTk1kTzd3NVdHa2t0d0Q0SUxVcXU3cFRtTVJTdlFxZzh2VGNtMzJG?=
 =?utf-8?B?ejBqUldHK3Jhcnl3bWVxTjZGeWMvTGprLzVUNU0vWTIzS2JmV3hyS1dmS21I?=
 =?utf-8?B?SWtIZWROYUE2Zkd0dnA5VDcwZlZsNU5kL1lhUi9ZZC9sVUFnTE43ZDdnTmQ1?=
 =?utf-8?B?b3RCVUp3N1h2NlU3SlFORDlDMGo4eE5EQjNpTTM5Z1NkdGlDeXQwSjlZMGhK?=
 =?utf-8?B?Zmk2SS9Dc3JFTXhGNVNXeEtNRjdHTXV5NGZ5bTBvZFlsMG5iVCtDcW85MUFk?=
 =?utf-8?B?MCtZYytVaDgxbjBzT0RHVzBGbndYOFRPV0p2NWFWQ0k3bnducG13Yi9IcXFK?=
 =?utf-8?B?WVkrQTBpVktDblAvTXVObEIvM0pETEc3eURPTHZncjd3eDhiVCttTjZkVHFN?=
 =?utf-8?B?Q2MwYXp6ZU5jWlZSYisrKzNNaiswUmtkQjM0ZFlWSkgwU1oxL2VvRnNpSHVh?=
 =?utf-8?B?U24zR2tpcjFqK2FYNFlRN3J2M0NDVHIxSjdBeTF0L1BPSWJ2a2p1aTYzU1lO?=
 =?utf-8?B?dUxGY1l6cVl4bVBUVFFWd2RTRTVLbklWNWlHcmp5RTNJUHNBQUZkdSt1WFZ5?=
 =?utf-8?B?eWtWOEJDalJmYVMrcGcrMmpOWUMxOUlQaGdYMC80QzJBVWx3YVdFakxQNEov?=
 =?utf-8?B?V3pLSDZZN1VpTGg5d2VTQVh3MEJXMTVvdHlnNjFBNzcvUC9hZ1dFbWNPNVFG?=
 =?utf-8?B?MGsvak02SDdmMEc2MFhESnpNemJOUTFSOGFzTEVLa04yVTRXN29yTzJjWlRu?=
 =?utf-8?B?aE5DUnp5Rlh5RHNRTHYvOXVtcC8vUmQ1cFZ0Vk5qL1BjdDBjN1QxclFsTVdx?=
 =?utf-8?B?NVY1eE5KekNUQXFMa29yVDJFbXJvRFB3TkRTNlBudTVIT1dNTzFENGEwb2dw?=
 =?utf-8?B?Y1NQZzhVa2VkMTRZZXFFZ1Jxcm5ZR1pYcGhRU2psQU9Ta2JxSzNIbG55YXdB?=
 =?utf-8?B?dVZrY2NkRlhGSXlFZmpIeFY0cUovcldkWUdUMVpCSFZHSWR5UVBNK1QvbTZD?=
 =?utf-8?B?ZzM1RFRpd0JEUXVpUHNqYzBrakdyU3VyOVBuTWxvMmlqcHhqanNmUi9mWHpi?=
 =?utf-8?B?ZGZncFUwSXkrVHVUdUhoOG5XQ3ZqTXMyRk10MzZ2Z3NWdHg2R3VVaXM3TERj?=
 =?utf-8?B?elJ2cURhRUxIUHQwbE12RDc1aVpSMDJpZ29iSUNoZGZyY0h1VnNSeG9aUG5P?=
 =?utf-8?B?ZUNnWlZKOGtrQVZWem1ONzJya0lSZ1RyVDJsWWFNeDRCYUxIRU5XNnNIcFRD?=
 =?utf-8?B?WWRJa3JBSkRlaXJ5N3lpam5NNnZHVXVwZjY5dHNlakpSV3RQcUM3THhhTnA5?=
 =?utf-8?B?b3IyaENJQjJIRmZmWThvdUlic3h2elRibjZKT2IvR3ppWDNHNUNMQUEyNElR?=
 =?utf-8?B?aG44cENhTHBZWGJFbFhxZlNPZ3RsbTFZVGN1R2pYbXcvMld6TEJGWHJ4b1ps?=
 =?utf-8?B?bTgzY3IzS2FDeUFTM0RCV0k3QVRxQVM1RlhCQURKSWp2d1pwZGhYYTgxNGJX?=
 =?utf-8?B?RUVtS0NYZzlJYjZObk9UL1plbHU3L3NDZTBkSmV0WHJZV0VWR2UvRnFrVW9G?=
 =?utf-8?B?Q0xGdHRlcXBWUlA4VjVuZVJ5UzRybmlaa2VJNmU0Ylo3MWhzSmVyWnh2UU80?=
 =?utf-8?B?VTZCOXZIMFM1M0RCaVRlQk5wTnViVk1oOVYyK0ZXaVh6N2EwaGpXQ05BN202?=
 =?utf-8?B?MXVhcWZmY0hNbjREN29Wa2xmVmQzRy9HSXp2SmxaVFBwY3hXcDV4RFgrdTFM?=
 =?utf-8?B?MVBSODVpOGpnVXZBQjFidy9NV1ppL1FKMWNrVXBpN2VPbTdTOThpNkkxUThz?=
 =?utf-8?B?dEdCWVlLVGpOdTNIbm5zeHdHZWtRPT0=?=
X-OriginatorOrg: nokia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 61ad7cfb-a61d-494b-dbf8-08d9e2ae1176
X-MS-Exchange-CrossTenant-AuthSource: DU2PR07MB8110.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jan 2022 22:32:30.2804
 (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: em/GzGqGrD21u3ONiQ7vkwITz4nb89ZsbRBsyaILyit79gLnKjnvCAHt0lURbHrkh3xkOEULl2gkdR+jxF1PciOfbHKNwkClrbewn/iGw7c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0701MB2215
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Dnia Fri, Jan 28, 2022 at 08:29:06PM +0100, Wolfram Sang napisał(a):
>
>> +	/* We want this to take priority over PSCI which has priority of 129. */
>> +	.priority = 130,
>
>Is it an idea to add a #define for the PSCI priority somewhere and use
>here the define + 1? Hardcoded priorities look a bit fragile to me.
>

You are right. Our priority policy for restart handlers isn't really
good and it is hard to choose right priority just about every time a
handler is added.

That being said, after reading Mark's argumentation, I now thing that
just being before PSCI is not enough. I think that a much higher
priority should be used as it is not normal situation that you would
like to run something before efi_reboot().

I would really still like to move the efi_reboot() to the
restart_handler even if you consider running some code before
efi_reboot() crazy. Since 255 is max priorty but using it basically
does not make sense (as this would let to the exact same situation we
have now).

I would use something like 250, or even 254, just to indicate that we
know that most certainly nothing should be run before efi_reboot(), but
still allow those silly people like me, to do what they want in their
system, without the need to run the custom kernel. I think we could even
add a proper comment, so it woudld become something like:

/**
  * If you are running UEFI based system, you most certainly should let
  * efi_reboot() do a reset for you. If you think you know better, we
  * leave you a window of opportunity here by not using maximal priorty
  * here.
  */
  .priority = 250,

What is the downside of doing that? That we will run through atomic
notfier chain instead of calling efi_reboot directly? Sure this is
slightly more complicated but it works on all our platforms and is
battle proven and we don't worry about that there. And the upside is
that we give people possibility to use their beloved mechanism if they
really like to. Because flexibility is a good thing.

What do you think?

Krzysztof