Re: [PATCH v2 08/45] kernel: Add combined power-off+restart handler call chain API

From: Rafael J. Wysocki
Date: Thu Oct 28 2021 - 05:59:45 EST


On Wed, Oct 27, 2021 at 11:18 PM Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> SoC platforms often have multiple options 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 power-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 executed, this is what 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.
>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
> include/linux/reboot.h | 176 +++++++++++-
> kernel/power/hibernate.c | 2 +-
> kernel/reboot.c | 601 ++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 768 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> index b7fa25726323..0ec835338c27 100644
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -8,10 +8,16 @@
>
> struct device;
>
> -#define SYS_DOWN 0x0001 /* Notify of system down */
> -#define SYS_RESTART SYS_DOWN
> -#define SYS_HALT 0x0002 /* Notify of system halt */
> -#define SYS_POWER_OFF 0x0003 /* Notify of system power off */
> +enum reboot_prepare_mode {
> + SYS_DOWN = 1, /* Notify of system down */
> + SYS_RESTART = SYS_DOWN,
> + SYS_HALT, /* Notify of system halt */
> + SYS_POWER_OFF, /* Notify of system power off */
> +};
> +
> +#define RESTART_PRIO_RESERVED 0
> +#define RESTART_PRIO_DEFAULT 128
> +#define RESTART_PRIO_HIGH 192
>
> enum reboot_mode {
> REBOOT_UNDEFINED = -1,
> @@ -49,6 +55,167 @@ int register_restart_handler(struct notifier_block *);
> int unregister_restart_handler(struct notifier_block *);
> void do_kernel_restart(char *cmd);
>
> +/*
> + * Unified poweroff + restart API.
> + */
> +
> +#define POWEROFF_PRIO_RESERVED 0
> +#define POWEROFF_PRIO_PLATFORM 1
> +#define POWEROFF_PRIO_DEFAULT 128
> +#define POWEROFF_PRIO_HIGH 192
> +#define POWEROFF_PRIO_FIRMWARE 224

Also I'm wondering why these particular numbers were chosen, here and above?

> +
> +enum poweroff_mode {
> + POWEROFF_NORMAL = 0,
> + POWEROFF_PREPARE,
> +};
> +
> +struct power_off_data {
> + void *cb_data;
> +};
> +
> +struct power_off_prep_data {
> + void *cb_data;
> +};
> +
> +struct restart_data {
> + void *cb_data;
> + const char *cmd;
> + enum reboot_mode mode;
> +};
> +
> +struct reboot_prep_data {
> + void *cb_data;
> + const char *cmd;
> + enum reboot_prepare_mode mode;
> +};
> +
> +struct power_handler_private_data {
> + struct notifier_block reboot_prep_nb;
> + struct notifier_block power_off_nb;
> + struct notifier_block restart_nb;
> + void (*trivial_power_off_cb)(void);
> + void (*simple_power_off_cb)(void *data);
> + void *simple_power_off_cb_data;
> + bool registered;
> +};
> +
> +/**
> + * struct power_handler - Machine power-off + restart handler
> + *
> + * Describes power-off and restart handlers which are invoked by kernel
> + * to power off or restart this machine. Supports prioritized chaining for
> + * both restart and power-off handlers. Callback's priority must be unique.
> + * Intended to be used by device drivers that are responsible for restarting
> + * and powering off hardware which kernel is running on.
> + *
> + * Struct power_handler can be static. Members of this structure must not be
> + * altered while handler is registered.
> + *
> + * Fill the structure members and pass it to register_power_handler().
> + */
> +struct power_handler {
> + /**
> + * @cb_data:
> + *
> + * User data included in callback's argument.
> + */

And here I would document the structure fields in the main kerneldoc
comment above.

As is, it is a bit hard to grasp the whole definition.

> + void *cb_data;
> +
> + /**
> + * @power_off_cb:
> + *
> + * Callback that should turn off machine. Inactive if NULL.
> + */
> + void (*power_off_cb)(struct power_off_data *data);
> +
> + /**
> + * @power_off_prepare_cb:
> + *
> + * Power-off preparation callback. All power-off preparation callbacks
> + * are invoked before @restart_cb. Inactive if NULL.
> + */
> + void (*power_off_prepare_cb)(struct power_off_prep_data *data);
> +
> + /**
> + * @power_off_priority:
> + *
> + * Power-off callback priority, must be unique. Zero value is
> + * reassigned to default priority. Inactive if @power_off_cb is NULL.
> + */
> + int power_off_priority;
> +
> + /**
> + * @power_off_chaining_allowed:
> + *
> + * False if callbacks execution should stop when @power_off_cb fails
> + * to power off machine. True if further lower priority power-off
> + * callback should be executed.
> + */
> + bool power_off_chaining_allowed;
> +
> + /**
> + * @restart_cb:
> + *
> + * Callback that should reboot machine. Inactive if NULL.
> + */
> + void (*restart_cb)(struct restart_data *data);
> +
> + /**
> + * @restart_priority:
> + *
> + * Restart callback priority, must be unique. Zero value is reassigned
> + * to default priority. Inactive if @restart_cb is NULL.
> + */
> + int restart_priority;
> +
> + /**
> + * @reboot_prepare_cb:
> + *
> + * Reboot preparation callback. All reboot preparation callbacks are
> + * invoked before @restart_cb. Inactive if NULL.
> + */
> + void (*reboot_prepare_cb)(struct reboot_prep_data *data);
> +
> + /**
> + * @priv:
> + *
> + * Internal data. Shouldn't be touched.
> + */
> + const struct power_handler_private_data priv;
> +};