From: Dmitry Osipenko <digetx@gmail.com>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: Thierry Reding <thierry.reding@gmail.com>,
Jonathan Hunter <jonathanh@nvidia.com>,
Russell King <linux@armlinux.org.uk>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>, Guo Ren <guoren@kernel.org>,
Geert Uytterhoeven <geert@linux-m68k.org>,
Greg Ungerer <gerg@linux-m68k.org>,
Joshua Thompson <funaho@jurai.org>,
Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
Sebastian Reichel <sre@kernel.org>,
Linus Walleij <linus.walleij@linaro.org>,
Philipp Zabel <p.zabel@pengutronix.de>,
Greentime Hu <green.hu@gmail.com>,
Vincent Chen <deanbo422@gmail.com>,
"James E.J. Bottomley" <James.Bottomley@hansenpartnership.com>,
Helge Deller <deller@gmx.de>,
Michael Ellerman <mpe@ellerman.id.au>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Yoshinori Sato <ysato@users.sourceforge.jp>,
Rich Felker <dalias@libc.org>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
Juergen Gross <jgross@suse.com>,
Stefano Stabellini <sstabellini@kernel.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Len Brown <lenb@kernel.org>,
Santosh Shilimkar <ssantosh@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>, Pavel Machek <pavel@ucw.cz>,
Lee Jones <lee.jones@linaro.org>,
Andrew Morton <akpm@linux-foundation.org>,
Guenter Roeck <linux@roeck-us.net>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Ulf Hansson <ulf.hansson@linaro.org>,
alankao@andestech.com,
"K . C . Kuen-Chern Lin" <kclin@andestech.com>,
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
Subject: Re: [PATCH v5 04/21] kernel: Add combined power-off+restart handler call chain API
Date: Tue, 11 Jan 2022 10:57:13 +0300 [thread overview]
Message-ID: <80f5a739-0850-a8db-7493-e8c387109ce0@gmail.com> (raw)
In-Reply-To: <Ydofs2CIfA+r5KAz@qmqm.qmqm.pl>
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.
next prev parent reply other threads:[~2022-01-11 7:57 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-12 21:02 [PATCH v5 00/21] Introduce power-off+restart call chain API Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 01/21] notifier: Add blocking_notifier_call_chain_is_empty() Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 02/21] notifier: Add atomic/blocking_notifier_chain_register_unique_prio() Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 03/21] reboot: Print error message if restart handler has duplicated priority Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 04/21] kernel: Add combined power-off+restart handler call chain API Dmitry Osipenko
2022-01-08 23:35 ` Michał Mirosław
2022-01-11 7:57 ` Dmitry Osipenko [this message]
2022-01-27 14:39 ` Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 05/21] ARM: Use do_kernel_power_off() Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 06/21] csky: " Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 07/21] riscv: " Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 08/21] arm64: " Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 09/21] parisc: " Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 10/21] xen/x86: " Dmitry Osipenko
2021-12-12 21:02 ` [PATCH v5 11/21] powerpc: " Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 12/21] m68k: Switch to new sys-off handler API Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 13/21] sh: Use do_kernel_power_off() Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 14/21] x86: " Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 15/21] ia64: " Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 16/21] mips: " Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 17/21] nds32: " Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 18/21] memory: emif: Use kernel_can_power_off() Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 19/21] ACPI: power: Switch to sys-off handler API Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 20/21] regulator: pfuze100: Use devm_register_sys_off_handler() Dmitry Osipenko
2021-12-12 21:03 ` [PATCH v5 21/21] reboot: Remove pm_power_off_prepare() Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=80f5a739-0850-a8db-7493-e8c387109ce0@gmail.com \
--to=digetx@gmail.com \
--cc=James.Bottomley@hansenpartnership.com \
--cc=akpm@linux-foundation.org \
--cc=alankao@andestech.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=aou@eecs.berkeley.edu \
--cc=benh@kernel.crashing.org \
--cc=boris.ostrovsky@oracle.com \
--cc=bp@alien8.de \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=dalias@libc.org \
--cc=daniel.lezcano@linaro.org \
--cc=dave.hansen@linux.intel.com \
--cc=deanbo422@gmail.com \
--cc=deller@gmx.de \
--cc=funaho@jurai.org \
--cc=geert@linux-m68k.org \
--cc=gerg@linux-m68k.org \
--cc=green.hu@gmail.com \
--cc=guoren@kernel.org \
--cc=hpa@zytor.com \
--cc=jgross@suse.com \
--cc=jonathanh@nvidia.com \
--cc=kclin@andestech.com \
--cc=krzysztof.kozlowski@canonical.com \
--cc=lee.jones@linaro.org \
--cc=lenb@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-csky@vger.kernel.org \
--cc=linux-ia64@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@lists.linux-m68k.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-sh@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=linux@roeck-us.net \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mingo@redhat.com \
--cc=mirq-linux@rere.qmqm.pl \
--cc=mpe@ellerman.id.au \
--cc=p.zabel@pengutronix.de \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=paulus@samba.org \
--cc=pavel@ucw.cz \
--cc=rafael@kernel.org \
--cc=sre@kernel.org \
--cc=ssantosh@kernel.org \
--cc=sstabellini@kernel.org \
--cc=tglx@linutronix.de \
--cc=thierry.reding@gmail.com \
--cc=tsbogend@alpha.franken.de \
--cc=ulf.hansson@linaro.org \
--cc=will@kernel.org \
--cc=x86@kernel.org \
--cc=xen-devel@lists.xenproject.org \
--cc=ysato@users.sourceforge.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).