Re: [PATCH v7 00/20] Introduce power-off+restart call chain API
On Tue, Apr 12, 2022 at 02:38:12AM +0300, Dmitry Osipenko wrote: > Problem > --- > > SoC devices require power-off call chaining functionality from kernel. > We have a widely used restart chaining provided by restart notifier API, > but nothing for power-off. > > Solution > > > Introduce new API that provides both restart and power-off call chains. [...] For the series: Reviewed-by: Michał Mirosław
Re: [PATCH v5 04/21] kernel: Add combined power-off+restart handler call chain API
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.
> +
> +/**
> + * 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.
> + * @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?
> + 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.
Best Regards
Michał Mirosław
Re: [PATCH v4 08/25] kernel: Add combined power-off+restart handler call chain API
On Mon, Nov 29, 2021 at 12:53:51AM +0300, Dmitry Osipenko wrote:
> 29.11.2021 00:17, Michał Mirosław пишет:
> >> I'm having trouble with parsing this comment. Could you please try to
> >> rephrase it? I don't see how you could check whether power-off handler
> >> is available if you'll mix all handlers together.
> > If notify_call_chain() would be fixed to return NOTIFY_OK if any call
> > returned NOTIFY_OK, then this would be a clear way to gather the
> > answer if any of the handlers will attempt the final action (reboot or
> > power off).
> Could you please show a code snippet that implements your suggestion?
A rough idea is this:
static int notifier_call_chain(struct notifier_block **nl,
unsigned long val, void *v,
int nr_to_call, int *nr_calls)
{
- int ret = NOTIFY_DONE;
+ int ret, result = NOTIFY_DONE;
struct notifier_block *nb, *next_nb;
nb = rcu_dereference_raw(*nl);
while (nb && nr_to_call) {
...
ret = nb->notifier_call(nb, val, v);
+
+ /* Assuming NOTIFY_STOP-carrying return is always greater than
non-stopping one. */
+ if (result < ret)
+ result = ret;
...
}
- return ret;
+ return result;
}
Then:
bool prepare_reboot()
{
int ret = xx_notifier_call_chain(&shutdown_notifier, PREPARE_REBOOT,
...);
return ret == NOTIFY_OK;
}
And the return value would signify whether the reboot will be attempted
when calling the chain for the REBOOT action. (Analogously for powering off.)
Best Regards
Michał Mirosław
Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> 28.11.2021 03:28, Michał Mirosław пишет:
> > On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> >> Add sanity check which ensures that there are no two restart handlers
> >> registered with the same priority. Normally it's a direct sign of a
> >> problem if two handlers use the same priority.
> >
> > The patch doesn't ensure the property that there are no duplicated-priority
> > entries on the chain.
>
> It's not the exact point of this patch.
>
> > I'd rather see a atomic_notifier_chain_register_unique() that returns
> > -EBUSY or something istead of adding an entry with duplicate priority.
> > That way it would need only one list traversal unless you want to
> > register the duplicate anyway (then you would call the older
> > atomic_notifier_chain_register() after reporting the error).
>
> The point of this patch is to warn developers about the problem that
> needs to be fixed. We already have such troubling drivers in mainline.
>
> It's not critical to register different handlers with a duplicated
> priorities, but such cases really need to be corrected. We shouldn't
> break users' machines during transition to the new API, meanwhile
> developers should take action of fixing theirs drivers.
>
> > (Or you could return > 0 when a duplicate is registered in
> > atomic_notifier_chain_register() if the callers are prepared
> > for that. I don't really like this way, though.)
>
> I had a similar thought at some point before and decided that I'm not in
> favor of this approach. It's nicer to have a dedicated function that
> verifies the uniqueness, IMO.
I don't like the part that it traverses the list second time to check
the uniqueness. But actually you could avoid that if
notifier_chain_register() would always add equal-priority entries in
reverse order:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
if (unlikely((*nl) == n)) {
WARN(1, "double register detected");
return 0;
}
- if (n->priority > (*nl)->priority)
+ if (n->priority >= (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl;
rcu_assign_pointer(*nl, n);
return 0;
}
Then the check for uniqueness after adding would be:
WARN(nb->next && nb->priority == nb->next->priority);
Best Regards
Michał Mirosław
Re: [PATCH v4 08/25] kernel: Add combined power-off+restart handler call chain API
On Mon, Nov 29, 2021 at 12:04:01AM +0300, Dmitry Osipenko wrote: > 28.11.2021 03:43, Michał Mirosław пишет: > > On Fri, Nov 26, 2021 at 09:00:44PM +0300, Dmitry Osipenko wrote: > >> SoC platforms often have multiple ways of how to perform system's > >> power-off and restart operations. Meanwhile today's kernel is limited to > >> a single option. Add combined power-off+restart handler call chain API, > >> which is inspired by the restart API. The new API provides both power-off > >> and restart functionality. > >> > >> The old pm_power_off method will be kept around till all users are > >> converted to the new API. > >> > >> Current restart API will be replaced by the new unified API since > >> new API is its superset. The restart functionality of the sys-off handler > >> API is built upon the existing restart-notifier APIs. > >> > >> In order to ease conversion to the new API, convenient helpers are added > >> for the common use-cases. They will reduce amount of boilerplate code and > >> remove global variables. These helpers preserve old behaviour for cases > >> where only one power-off handler is expected, this is what all existing > >> drivers want, and thus, they could be easily converted to the new API. > >> Users of the new API should explicitly enable power-off chaining by > >> setting corresponding flag of the power_handler structure. > > [...] > > > > Hi, > > > > A general question: do we really need three distinct chains for this? > > Hello Michał, > > At minimum this makes code easier to follow. > > > Can't there be only one that chain of callbacks that get a stage > > (RESTART_PREPARE, RESTART, POWER_OFF_PREPARE, POWER_OFF) and can ignore > > them at will? Calling through POWER_OFF_PREPARE would also return > > whether that POWER_OFF is possible (for kernel_can_power_off()). > > I'm having trouble with parsing this comment. Could you please try to > rephrase it? I don't see how you could check whether power-off handler > is available if you'll mix all handlers together. If notify_call_chain() would be fixed to return NOTIFY_OK if any call returned NOTIFY_OK, then this would be a clear way to gather the answer if any of the handlers will attempt the final action (reboot or power off). > > > I would also split this patch into preparation cleanups (like wrapping > > pm_power_off call with a function) and adding the notifier-based > > implementation. > > What's the benefit of this split up will be? Are you suggesting that it > will ease reviewing of this patch or something else? Mainly to ease review, as the wrapping will be a no-op, but the addition of notifier chain changes semantics a bit. Best Regards Michał Mirosław
Re: [PATCH v4 22/25] memory: emif: Use kernel_can_power_off()
On Fri, Nov 26, 2021 at 09:00:58PM +0300, Dmitry Osipenko wrote:
> Replace legacy pm_power_off with kernel_can_power_off() helper that
> is aware about chained power-off handlers.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/memory/emif.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/memory/emif.c b/drivers/memory/emif.c
> index 762d0c0f0716..cab10d5274a0 100644
> --- a/drivers/memory/emif.c
> +++ b/drivers/memory/emif.c
> @@ -630,7 +630,7 @@ static irqreturn_t emif_threaded_isr(int irq, void
> *dev_id)
> dev_emerg(emif->dev, "SDRAM temperature exceeds operating
> limit.. Needs shut down!!!\n");
>
> /* If we have Power OFF ability, use it, else try restarting */
> - if (pm_power_off) {
> + if (kernel_can_power_off()) {
> kernel_power_off();
> } else {
> WARN(1, "FIXME: NO pm_power_off!!! trying restart\n");
BTW, this part of the code seems to be better moved to generic code that
could replace POWER_OFF request with REBOOT like it is done for reboot()
syscall.
Best Regards
Michał Mirosław
Re: [PATCH v4 18/25] x86: Use do_kernel_power_off()
On Fri, Nov 26, 2021 at 09:00:54PM +0300, Dmitry Osipenko wrote:
> Kernel now supports chained power-off handlers. Use do_kernel_power_off()
> that invokes chained power-off handlers. It also invokes legacy
> pm_power_off() for now, which will be removed once all drivers will
> be converted to the new power-off API.
>
> Signed-off-by: Dmitry Osipenko
> ---
> arch/x86/kernel/reboot.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 0a40df66a40d..cd7d9416d81a 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -747,10 +747,10 @@ static void native_machine_halt(void)
>
> static void native_machine_power_off(void)
> {
> - if (pm_power_off) {
> + if (kernel_can_power_off()) {
> if (!reboot_force)
> machine_shutdown();
> - pm_power_off();
> + do_kernel_power_off();
> }
Judging from an old commit from 2006 [1], this can be rewritten as:
if (!reboot_force && kernel_can_power_off())
machine_shutdown();
do_kernel_power_off();
And maybe later reworked so it doesn't need kernel_can_power_off().
[1] http://lkml.iu.edu/hypermail//linux/kernel/0511.3/0681.html
Best Regards
Michał Mirosław
Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote: > Add sanity check which ensures that there are no two restart handlers > registered with the same priority. Normally it's a direct sign of a > problem if two handlers use the same priority. The patch doesn't ensure the property that there are no duplicated-priority entries on the chain. I'd rather see a atomic_notifier_chain_register_unique() that returns -EBUSY or something istead of adding an entry with duplicate priority. That way it would need only one list traversal unless you want to register the duplicate anyway (then you would call the older atomic_notifier_chain_register() after reporting the error). (Or you could return > 0 when a duplicate is registered in atomic_notifier_chain_register() if the callers are prepared for that. I don't really like this way, though.) Best Regards Michał Mirosław
Re: [PATCH v4 08/25] kernel: Add combined power-off+restart handler call chain API
On Fri, Nov 26, 2021 at 09:00:44PM +0300, Dmitry Osipenko wrote: > SoC platforms often have multiple ways of how to perform system's > power-off and restart operations. Meanwhile today's kernel is limited to > a single option. Add combined power-off+restart handler call chain API, > which is inspired by the restart API. The new API provides both power-off > and restart functionality. > > The old pm_power_off method will be kept around till all users are > converted to the new API. > > Current restart API will be replaced by the new unified API since > new API is its superset. The restart functionality of the sys-off handler > API is built upon the existing restart-notifier APIs. > > In order to ease conversion to the new API, convenient helpers are added > for the common use-cases. They will reduce amount of boilerplate code and > remove global variables. These helpers preserve old behaviour for cases > where only one power-off handler is expected, this is what all existing > drivers want, and thus, they could be easily converted to the new API. > Users of the new API should explicitly enable power-off chaining by > setting corresponding flag of the power_handler structure. [...] Hi, A general question: do we really need three distinct chains for this? Can't there be only one that chain of callbacks that get a stage (RESTART_PREPARE, RESTART, POWER_OFF_PREPARE, POWER_OFF) and can ignore them at will? Calling through POWER_OFF_PREPARE would also return whether that POWER_OFF is possible (for kernel_can_power_off()). I would also split this patch into preparation cleanups (like wrapping pm_power_off call with a function) and adding the notifier-based implementation. Best Regards Michał Mirosław
