From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Osipenko Date: Tue, 11 Jan 2022 07:57:13 +0000 Subject: Re: [PATCH v5 04/21] kernel: Add combined power-off+restart handler call chain API Message-Id: <80f5a739-0850-a8db-7493-e8c387109ce0@gmail.com> List-Id: References: <20211212210309.9851-1-digetx@gmail.com> <20211212210309.9851-5-digetx@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= Cc: Thierry Reding , Jonathan Hunter , Russell King , Catalin Marinas , Will Deacon , Guo Ren , Geert Uytterhoeven , Greg Ungerer , Joshua Thompson , Thomas Bogendoerfer , Sebastian Reichel , Linus Walleij , Philipp Zabel , Greentime Hu , Vincent Chen , "James E.J. Bottomley" , Helge Deller , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Paul Walmsley , Palmer Dabbelt , Albert Ou , Yoshinori Sato , Rich Felker , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , Boris Ostrovsky , Juergen Gross , Stefano Stabellini , "Rafael J. Wysocki" , Len Brown , Santosh Shilimkar , Krzysztof Kozlowski , Liam Girdwood , Mark Brown , Pavel Machek , Lee Jones , Andrew Morton , Guenter Roeck , Daniel Lezcano , Andy Shevchenko , Ulf Hansson , alankao@andestech.com, "K . C . Kuen-Chern Lin" , linux-kernel@vger.kernel.org, linux-csky@vger.kernel.org, linux-ia64@vger.kernel.org, linux-m68k@lists.linux-m68k.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-riscv@lists.infradead.org, linux-sh@vger.kernel.org, xen-devel@lists.xenproject.org, linux-acpi@vger.kernel.org, linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org 09.01.2022 02:35, Michał Mirosław пишет: > On Mon, Dec 13, 2021 at 12:02:52AM +0300, Dmitry Osipenko wrote: > [...] >> +/** >> + * struct power_off_data - Power-off callback argument >> + * >> + * @cb_data: Callback data. >> + */ >> +struct power_off_data { >> + void *cb_data; >> +}; >> + >> +/** >> + * struct power_off_prep_data - Power-off preparation callback argument >> + * >> + * @cb_data: Callback data. >> + */ >> +struct power_off_prep_data { >> + void *cb_data; >> +}; > > Why two exactly same structures? Why only a single pointer instead? If > it just to enable type-checking callbacks, then thouse could be opaque > or zero-sized structs that would be embedded or casted away in > respective callbacks. Preparation and final execution are two different operations, it's much cleaner from a user's perspective to have same separated, IMO. In the future we may would want to extend one of the structs, and not the other. Type-checking is another benefit, of course. The single callback pointer is what is utilized by all current kernel users. This may change in the future and then it won't be a problem to extend the power-off API without disrupting whole kernel. >> + >> +/** >> + * struct restart_data - Restart callback argument >> + * >> + * @cb_data: Callback data. >> + * @cmd: Restart command string. >> + * @stop_chain: Further lower priority callbacks won't be executed if set to >> + * true. Can be changed within callback. Default is false. >> + * @mode: Reboot mode ID. >> + */ >> +struct restart_data { >> + void *cb_data; >> + const char *cmd; >> + bool stop_chain; >> + enum reboot_mode mode; >> +}; >> + >> +/** >> + * struct reboot_prep_data - Reboot and shutdown preparation callback argument >> + * >> + * @cb_data: Callback data. >> + * @cmd: Restart command string. >> + * @stop_chain: Further lower priority callbacks won't be executed if set to >> + * true. Can be changed within callback. Default is false. > > Why would we want to stop power-off or erboot chain? If the callback > succeded, then further calls won't be made. If it doesn't succeed, but > possibly breaks the system somehow, it shouldn't return. Then the only > case left would be to just try the next method of shutting down. This is what some of the API users were doing for years. I don't know for sure why they want to stop the chain, it indeed looks like an incorrect behaviour, but that's up to developers to decide. I only retained the old behaviour for those users. >> + * @mode: Preparation mode ID. >> + */ >> +struct reboot_prep_data { >> + void *cb_data; >> + const char *cmd; >> + bool stop_chain; >> + enum reboot_prepare_mode mode; >> +}; >> + >> +struct sys_off_handler_private_data { >> + struct notifier_block power_off_nb; >> + struct notifier_block restart_nb; >> + struct notifier_block reboot_nb; > > What's the difference between restart and reboot? Reboot is always executed before restart and power-off callbacks. This is explained in the doc-comment of the struct sys_off_handler. + * @reboot_prepare_cb: Reboot/shutdown preparation callback. All reboot + * preparation callbacks are invoked before @restart_cb or @power_off_cb, + * depending on the mode. It's registered with register_reboot_notifier(). + * The point is to remove boilerplate code from drivers which use this + * callback in conjunction with the restart/power-off callbacks. + * This reboot callback usually performs early preparations that are need to be done before machine is placed into reset state, please see [1] for the examples. [1] https://elixir.bootlin.com/linux/v5.16/A/ident/register_reboot_notifier I agree that "reboot" sounds like a misnomer. This name was coined long time ago, perhaps not worth to rename it at this point. I'm also not sure what could be a better name. >> + void (*platform_power_off_cb)(void); >> + void (*simple_power_off_cb)(void *data); >> + void *simple_power_off_cb_data; >> + bool registered; >> +}; > > BTW, I couldn't find a right description of my idea of unifying the > chains before, so let me sketch it now. > > The idea is to have a single system-off chain in which the callback > gets a mode ({QUERY_*, PREP_*, DO_*} for each of {*_REBOOT, *_POWEROFF, ...?). > The QUERY_* calls would be made in can_kernel_reboot/poweroff(): all > would be called, and if at least one returned true, then the shutdown > mode would continue. All of PREP_* would be called then. After that > all DO_* would be tried until one doesn't return (succeeded or broke > the system hard). Classic for(;;); could be a final fallback for the > case where arch/machine (lowest priority) call would return instead > of halting the system in machine-dependent way. The QUERY and PREP > stages could be combined, but I haven't thought about it enough to > see what conditions would need to be imposed on the callbacks in > that case (maybe it's not worth the trouble, since it isn't a fast > path anyway?). The goal here is to have less (duplicated) code in > kernel, but otherwise it seems equivalent to your API proposal. Michał, thank you for the review and suggestions! I'll take another look at yours proposal during this merge window, in a preparation to v6.