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

["Rafael J. Wysocki" <rafael@xxxxxxxxxx>]

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;
> +};