[PATCH v7 00/20] Introduce power-off+restart call chain API

[PATCH v7 00/20] Introduce power-off+restart call chain API

[Dmitry Osipenko]

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.

Why combine restart with power-off? Because drivers often do both.
More practical to have API that provides both under the same roof.

The new API is designed with simplicity and extensibility in mind.
It's built upon the existing restart and reboot APIs. The simplicity
is in new helper functions that are convenient for drivers. The
extensibility is in the design that doesn't hardcode callback
arguments, making easy to add new parameters and remove old.

This is a third attempt to introduce the new API. First was made by
Guenter Roeck back in 2014, second was made by Thierry Reding in 2017.
In fact the work didn't stop and recently arm_pm_restart() was removed
from v5.14 kernel, which was a part of preparatory work started by
Guenter Roeck. I took into account experience and ideas from the
previous attempts, extended and polished them.

Adoption plan
-------------

This patchset introduces the new API. It also converts multiple drivers
and arch code to the new API to demonstrate how it all looks in practice.

The plan is:

1. Merge the new API and convert arch code to use do_kernel_power_off().
   For now the new API will co-exist with the older API.

2. Convert all drivers and platform code to the new API.

3. Remove obsoleted pm_power_off and pm_power_off_prepare variables.

4. Make restart-notifier API private to kernel/reboot.c once no users left.

5. Make unique-priority of the handlers' a mandatory requirement in the
   new API.

The plan is fully implemented here:

[1] https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/commits/sys-off-handler

For now I'm sending the first 20 base patches out of ~180.
Majority of drivers and platform patches depend on the base patches,
hence the rest will come later on, once base will land.

All [1] patches are compile-tested. Tegra, Rockchip and x86 ACPI patches
are tested on hardware.

Results
-------

1. Devices can be powered off properly.

2. Global variables are removed from drivers.

3. Global pm_power_off and pm_power_off_prepare callback variables are
removed once all users are converted to the new API. The latter callback
is removed by patch #25 of this series.

4. Ambiguous call chain ordering is prohibited. See patch #4 which adds
verification of restart handlers priorities, ensuring that they are unique.

Changelog:

v7: - Rebased on a recent linux-next. Dropped the recently removed
      NDS32 architecture. Only SH and x86 arches left un-acked.

    - Added acks from Thomas Bogendoerfer and Krzysztof Kozlowski
      to the MIPS and memory/emif patches respectively.

    - Made couple minor cosmetic improvements to the new API.

    - A month ago I joined Collabora and continuing to work on this series
      on the company's time, so changed my email address to collabora.com

v6: - Rebased on a recent linux-next.

    - Made minor couple cosmetic changes.

v5: - Dropped patches which cleaned up notifier/reboot headers, as was
      requested by Rafael Wysocki.

    - Dropped WARN_ON() from the code, as was requested by Rafael Wysocki.
      Replaced it with pr_err() appropriately.

    - Dropped *_notifier_has_unique_priority() functions and added
      *_notifier_chain_register_unique_prio() instead, as was suggested
      by Michał Mirosław and Rafael Wysocki.

    - Dropped export of blocking_notifier_call_chain_is_empty() symbol,
      as was suggested by Rafael Wysocki.

    - Michał Mirosław suggested that will be better to split up patch
      that adds the new API to ease reviewing, but Rafael Wysocki asked
      not add more patches, so I kept it as a single patch.

    - Added temporary "weak" stub for pm_power_off() which fixes linkage
      failure once symbol is removed from arch/* code. Previously I missed
      this problem because was only compile-testing object files.

v4: - Made a very minor improvement to doc comments, clarifying couple
      default values.

    - Corrected list of emails recipient by adding Linus, Sebastian,
      Philipp and more NDS people. Removed bouncing emails.

    - Added acks that were given to v3.

v3: - Renamed power_handler to sys_off_handler as was suggested by
      Rafael Wysocki.

    - Improved doc-comments as was suggested by Rafael Wysocki. Added more
      doc-comments.

    - Implemented full set of 180 patches which convert whole kernel in
      accordance to the plan, see link [1] above. Slightly adjusted API to
      better suit for the remaining converted drivers.

      * Added unregister_sys_off_handler() that is handy for a couple old
        platform drivers.

      * Dropped devm_register_trivial_restart_handler(), 'simple' variant
        is enough to have.

    - Improved "Add atomic/blocking_notifier_has_unique_priority()" patch,
      as was suggested by Andy Shevchenko. Also replaced down_write() with
      down_read() and factored out common notifier_has_unique_priority().

    - Added stop_chain field to struct restart_data and reboot_prep_data
      after discovering couple drivers wanting that feature.

    - Added acks that were given to v2.

v2: - Replaced standalone power-off call chain demo-API with the combined
      power-off+restart API because this is what drivers want. It's a more
      comprehensive solution.

    - Converted multiple drivers and arch code to the new API. Suggested by
      Andy Shevchenko. I skimmed through the rest of drivers, verifying that
      new API suits them. The rest of the drivers will be converted once we
      will settle on the new API, otherwise will be too many patches here.

    - v2 API doesn't expose notifier to users and require handlers to
      have unique priority. Suggested by Guenter Roeck.

    - v2 API has power-off chaining disabled by default and require
      drivers to explicitly opt-in to the chaining. This preserves old
      behaviour for existing drivers once they are converted to the new
      API.

Dmitry Osipenko (20):
  notifier: Add blocking_notifier_call_chain_is_empty()
  notifier: Add atomic/blocking_notifier_chain_register_unique_prio()
  reboot: Print error message if restart handler has duplicated priority
  kernel: Add combined power-off+restart handler call chain API
  ARM: Use do_kernel_power_off()
  csky: Use do_kernel_power_off()
  riscv: Use do_kernel_power_off()
  arm64: Use do_kernel_power_off()
  parisc: Use do_kernel_power_off()
  xen/x86: Use do_kernel_power_off()
  powerpc: Use do_kernel_power_off()
  m68k: Switch to new sys-off handler API
  sh: Use do_kernel_power_off()
  x86: Use do_kernel_power_off()
  ia64: Use do_kernel_power_off()
  mips: Use do_kernel_power_off()
  memory: emif: Use kernel_can_power_off()
  ACPI: power: Switch to sys-off handler API
  regulator: pfuze100: Use devm_register_sys_off_handler()
  reboot: Remove pm_power_off_prepare()

 arch/arm/kernel/reboot.c               |   4 +-
 arch/arm64/kernel/process.c            |   3 +-
 arch/csky/kernel/power.c               |   6 +-
 arch/ia64/kernel/process.c             |   4 +-
 arch/m68k/emu/natfeat.c                |   3 +-
 arch/m68k/include/asm/machdep.h        |   1 -
 arch/m68k/kernel/process.c             |   5 +-
 arch/m68k/kernel/setup_mm.c            |   1 -
 arch/m68k/kernel/setup_no.c            |   1 -
 arch/m68k/mac/config.c                 |   4 +-
 arch/mips/kernel/reset.c               |   3 +-
 arch/parisc/kernel/process.c           |   4 +-
 arch/powerpc/kernel/setup-common.c     |   4 +-
 arch/powerpc/xmon/xmon.c               |   3 +-
 arch/riscv/kernel/reset.c              |  12 +-
 arch/sh/kernel/reboot.c                |   3 +-
 arch/x86/kernel/reboot.c               |   4 +-
 arch/x86/xen/enlighten_pv.c            |   4 +-
 drivers/acpi/sleep.c                   |  25 +-
 drivers/memory/emif.c                  |   2 +-
 drivers/regulator/pfuze100-regulator.c |  38 +-
 include/linux/notifier.h               |   7 +
 include/linux/pm.h                     |   1 -
 include/linux/reboot.h                 | 229 ++++++++-
 kernel/notifier.c                      | 101 +++-
 kernel/power/hibernate.c               |   2 +-
 kernel/reboot.c                        | 622 ++++++++++++++++++++++++-
 27 files changed, 980 insertions(+), 116 deletions(-)

-- 
2.35.1

[PATCH v7 01/20] notifier: Add blocking_notifier_call_chain_is_empty()

[Dmitry Osipenko]

Add blocking_notifier_call_chain_is_empty() that returns true if call
chain is empty.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 include/linux/notifier.h |  2 ++
 kernel/notifier.c        | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 87069b8459af..d4717bc0ab85 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -173,6 +173,8 @@ extern int blocking_notifier_call_chain_robust(struct blocking_notifier_head *nh
 extern int raw_notifier_call_chain_robust(struct raw_notifier_head *nh,
 		unsigned long val_up, unsigned long val_down, void *v);
 
+extern bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh);
+
 #define NOTIFY_DONE		0x0000		/* Don't care */
 #define NOTIFY_OK		0x0001		/* Suits me */
 #define NOTIFY_STOP_MASK	0x8000		/* Don't call further */
diff --git a/kernel/notifier.c b/kernel/notifier.c
index ba005ebf4730..4ed6bda8f127 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -323,6 +323,19 @@ int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
 }
 EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);
 
+/**
+ *	blocking_notifier_call_chain_is_empty - Check whether notifier chain is empty
+ *	@nh: Pointer to head of the blocking notifier chain
+ *
+ *	Checks whether notifier chain is empty.
+ *
+ *	Returns true is notifier chain is empty, false otherwise.
+ */
+bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh)
+{
+	return !rcu_access_pointer(nh->head);
+}
+
 /*
  *	Raw notifier chain routines.  There is no protection;
  *	the caller must provide it.  Use at your own risk!
-- 
2.35.1

[PATCH v7 02/20] notifier: Add atomic/blocking_notifier_chain_register_unique_prio()

[Dmitry Osipenko]

Add variant of atomic/blocking_notifier_chain_register() functions that
doesn't allow to register notifier using a duplicated priority. The -EBUSY
error code is returned in this case by the new API functions.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 include/linux/notifier.h |  5 +++
 kernel/notifier.c        | 88 +++++++++++++++++++++++++++++++---------
 2 files changed, 74 insertions(+), 19 deletions(-)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index d4717bc0ab85..ccce26197dd2 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -150,6 +150,11 @@ extern int raw_notifier_chain_register(struct raw_notifier_head *nh,
 extern int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
 		struct notifier_block *nb);
 
+extern int atomic_notifier_chain_register_unique_prio(
+		struct atomic_notifier_head *nh, struct notifier_block *nb);
+extern int blocking_notifier_chain_register_unique_prio(
+		struct blocking_notifier_head *nh, struct notifier_block *nb);
+
 extern int atomic_notifier_chain_unregister(struct atomic_notifier_head *nh,
 		struct notifier_block *nb);
 extern int blocking_notifier_chain_unregister(struct blocking_notifier_head *nh,
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 4ed6bda8f127..4fc32b1e6cbb 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -20,7 +20,8 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
  */
 
 static int notifier_chain_register(struct notifier_block **nl,
-				   struct notifier_block *n)
+				   struct notifier_block *n,
+				   bool unique_priority)
 {
 	while ((*nl) != NULL) {
 		if (unlikely((*nl) == n)) {
@@ -30,6 +31,8 @@ static int notifier_chain_register(struct notifier_block **nl,
 		}
 		if (n->priority > (*nl)->priority)
 			break;
+		if (n->priority == (*nl)->priority && unique_priority)
+			return -EBUSY;
 		nl = &((*nl)->next);
 	}
 	n->next = *nl;
@@ -144,12 +147,35 @@ int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
 	int ret;
 
 	spin_lock_irqsave(&nh->lock, flags);
-	ret = notifier_chain_register(&nh->head, n);
+	ret = notifier_chain_register(&nh->head, n, false);
 	spin_unlock_irqrestore(&nh->lock, flags);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(atomic_notifier_chain_register);
 
+/**
+ *	atomic_notifier_chain_register_unique_prio - Add notifier to an atomic notifier chain
+ *	@nh: Pointer to head of the atomic notifier chain
+ *	@n: New entry in notifier chain
+ *
+ *	Adds a notifier to an atomic notifier chain if there is no other
+ *	notifier registered using the same priority.
+ *
+ *	Returns 0 on success, %-EEXIST or %-EBUSY on error.
+ */
+int atomic_notifier_chain_register_unique_prio(struct atomic_notifier_head *nh,
+					       struct notifier_block *n)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&nh->lock, flags);
+	ret = notifier_chain_register(&nh->head, n, true);
+	spin_unlock_irqrestore(&nh->lock, flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(atomic_notifier_chain_register_unique_prio);
+
 /**
  *	atomic_notifier_chain_unregister - Remove notifier from an atomic notifier chain
  *	@nh: Pointer to head of the atomic notifier chain
@@ -209,18 +235,9 @@ NOKPROBE_SYMBOL(atomic_notifier_call_chain);
  *	synchronized by an rwsem.
  */
 
-/**
- *	blocking_notifier_chain_register - Add notifier to a blocking notifier chain
- *	@nh: Pointer to head of the blocking notifier chain
- *	@n: New entry in notifier chain
- *
- *	Adds a notifier to a blocking notifier chain.
- *	Must be called in process context.
- *
- *	Returns 0 on success, %-EEXIST on error.
- */
-int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
-		struct notifier_block *n)
+static int __blocking_notifier_chain_register(struct blocking_notifier_head *nh,
+					      struct notifier_block *n,
+					      bool unique_priority)
 {
 	int ret;
 
@@ -230,15 +247,48 @@ int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
 	 * such times we must not call down_write().
 	 */
 	if (unlikely(system_state == SYSTEM_BOOTING))
-		return notifier_chain_register(&nh->head, n);
+		return notifier_chain_register(&nh->head, n, unique_priority);
 
 	down_write(&nh->rwsem);
-	ret = notifier_chain_register(&nh->head, n);
+	ret = notifier_chain_register(&nh->head, n, unique_priority);
 	up_write(&nh->rwsem);
 	return ret;
 }
+
+/**
+ *	blocking_notifier_chain_register - Add notifier to a blocking notifier chain
+ *	@nh: Pointer to head of the blocking notifier chain
+ *	@n: New entry in notifier chain
+ *
+ *	Adds a notifier to a blocking notifier chain.
+ *	Must be called in process context.
+ *
+ *	Returns 0 on success, %-EEXIST on error.
+ */
+int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
+		struct notifier_block *n)
+{
+	return __blocking_notifier_chain_register(nh, n, false);
+}
 EXPORT_SYMBOL_GPL(blocking_notifier_chain_register);
 
+/**
+ *	blocking_notifier_chain_register_unique_prio - Add notifier to a blocking notifier chain
+ *	@nh: Pointer to head of the blocking notifier chain
+ *	@n: New entry in notifier chain
+ *
+ *	Adds a notifier to an blocking notifier chain if there is no other
+ *	notifier registered using the same priority.
+ *
+ *	Returns 0 on success, %-EEXIST or %-EBUSY on error.
+ */
+int blocking_notifier_chain_register_unique_prio(struct blocking_notifier_head *nh,
+						 struct notifier_block *n)
+{
+	return __blocking_notifier_chain_register(nh, n, true);
+}
+EXPORT_SYMBOL_GPL(blocking_notifier_chain_register_unique_prio);
+
 /**
  *	blocking_notifier_chain_unregister - Remove notifier from a blocking notifier chain
  *	@nh: Pointer to head of the blocking notifier chain
@@ -354,7 +404,7 @@ bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh)
 int raw_notifier_chain_register(struct raw_notifier_head *nh,
 		struct notifier_block *n)
 {
-	return notifier_chain_register(&nh->head, n);
+	return notifier_chain_register(&nh->head, n, false);
 }
 EXPORT_SYMBOL_GPL(raw_notifier_chain_register);
 
@@ -433,10 +483,10 @@ int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
 	 * such times we must not call mutex_lock().
 	 */
 	if (unlikely(system_state == SYSTEM_BOOTING))
-		return notifier_chain_register(&nh->head, n);
+		return notifier_chain_register(&nh->head, n, false);
 
 	mutex_lock(&nh->mutex);
-	ret = notifier_chain_register(&nh->head, n);
+	ret = notifier_chain_register(&nh->head, n, false);
 	mutex_unlock(&nh->mutex);
 	return ret;
 }
-- 
2.35.1

[PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority

[Dmitry Osipenko]

Add sanity check which ensures that there are no two restart handlers
registered using the same priority. This requirement will become mandatory
once all drivers will be converted to the new API and such errors will be
fixed.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 kernel/reboot.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index ed4e6dfb7d44..acdae4e95061 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
  */
 int register_restart_handler(struct notifier_block *nb)
 {
+	int ret;
+
+	ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb);
+	if (ret != -EBUSY)
+		return ret;
+
+	/*
+	 * Handler must have unique priority. Otherwise call order is
+	 * determined by registration order, which is unreliable.
+	 *
+	 * This requirement will become mandatory once all drivers
+	 * will be converted to use new sys-off API.
+	 */
+	pr_err("failed to register restart handler using unique priority\n");
+
 	return atomic_notifier_chain_register(&restart_handler_list, nb);
 }
 EXPORT_SYMBOL(register_restart_handler);
-- 
2.35.1

[PATCH v7 04/20] kernel: Add combined power-off+restart handler call chain API

[Dmitry Osipenko]

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.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 include/linux/reboot.h   | 229 ++++++++++++++-
 kernel/power/hibernate.c |   2 +-
 kernel/reboot.c          | 604 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 827 insertions(+), 8 deletions(-)

diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index a2429648d831..ba5e5dddcfcd 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -8,10 +8,35 @@
 
 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 */
+};
+
+/*
+ * Standard restart priority levels. Intended to be set in the
+ * sys_off_handler.restart_priority field.
+ *
+ * Use `RESTART_PRIO_ABC +- prio` style for additional levels.
+ *
+ * RESTART_PRIO_RESERVED:	Falls back to RESTART_PRIO_DEFAULT.
+ *				Drivers may leave priority initialized
+ *				to zero, to auto-set it to the default level.
+ *
+ * RESTART_PRIO_LOW:		Use this for handler of last resort.
+ *
+ * RESTART_PRIO_DEFAULT:	Use this for default/generic handler.
+ *
+ * RESTART_PRIO_HIGH:		Use this if you have multiple handlers and
+ *				this handler has higher priority than the
+ *				default handler.
+ */
+#define RESTART_PRIO_RESERVED		0
+#define RESTART_PRIO_LOW		8
+#define RESTART_PRIO_DEFAULT		128
+#define RESTART_PRIO_HIGH		192
 
 enum reboot_mode {
 	REBOOT_UNDEFINED = -1,
@@ -49,6 +74,201 @@ extern int register_restart_handler(struct notifier_block *);
 extern int unregister_restart_handler(struct notifier_block *);
 extern void do_kernel_restart(char *cmd);
 
+/*
+ * System power-off and restart API.
+ */
+
+/*
+ * Standard power-off priority levels. Intended to be set in the
+ * sys_off_handler.power_off_priority field.
+ *
+ * Use `POWEROFF_PRIO_ABC +- prio` style for additional levels.
+ *
+ * POWEROFF_PRIO_RESERVED:	Falls back to POWEROFF_PRIO_DEFAULT.
+ *				Drivers may leave priority initialized
+ *				to zero, to auto-set it to the default level.
+ *
+ * POWEROFF_PRIO_PLATFORM:	Intended to be used by platform-level handler.
+ *				Has lowest priority since device drivers are
+ *				expected to take over platform handler which
+ *				doesn't allow further callback chaining.
+ *
+ * POWEROFF_PRIO_DEFAULT:	Use this for default/generic handler.
+ *
+ * POWEROFF_PRIO_FIRMWARE:	Use this if handler uses firmware call.
+ *				Has highest priority since firmware is expected
+ *				to know best how to power-off hardware properly.
+ */
+#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
+
+enum poweroff_mode {
+	POWEROFF_NORMAL = 0,
+	POWEROFF_PREPARE,
+};
+
+/**
+ * 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;
+};
+
+/**
+ * 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.
+ * @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;
+	void (*platform_power_off_cb)(void);
+	void (*simple_power_off_cb)(void *data);
+	void *simple_power_off_cb_data;
+	bool registered;
+};
+
+/**
+ * struct sys_off_handler - System power-off and restart handler
+ *
+ * @cb_data: Pointer to user's data.
+ *
+ * @power_off_cb: Callback that powers off this machine. Inactive if NULL.
+ *
+ * @power_off_prepare_cb: Power-off preparation callback. All power-off
+ * preparation callbacks are invoked after @reboot_prepare_cb and before
+ * @power_off_cb. Inactive if NULL.
+ *
+ * @power_off_priority: Power-off callback priority, must be unique.
+ * Zero value is reserved and auto-reassigned to POWEROFF_PRIO_DEFAULT.
+ * Inactive if @power_off_cb is NULL.
+ *
+ * @power_off_chaining_allowed: Set to false if callback's execution should
+ * stop when @power_off_cb fails to power off this machine. True if further
+ * lower priority power-off callback should be executed. False is default
+ * value.
+ *
+ * @restart_cb: Callback that reboots this machine. Inactive if NULL.
+ *
+ * @restart_priority: Restart callback priority, must be unique. Zero value
+ * is reserved and auto-reassigned to RESTART_PRIO_DEFAULT. Inactive if
+ * @restart_cb is NULL.
+ *
+ * @restart_chaining_disallowed: Set to true if callback's execution should
+ * stop when @restart_cb fails to restart this machine. False if further
+ * lower priority restart callback should be executed. False is default
+ * value.
+ *
+ * @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.
+ *
+ * @reboot_priority: Reboot/shutdown preparation callback priority, doesn't
+ * need to be unique. Zero is default value. Inactive if @reboot_prepare_cb
+ * is NULL.
+ *
+ * @priv: Internal data. Shouldn't be touched.
+ *
+ * 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.
+ *
+ * Struct sys_off_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_sys_off_handler().
+ */
+struct sys_off_handler {
+	void *cb_data;
+
+	void (*power_off_cb)(struct power_off_data *data);
+	void (*power_off_prepare_cb)(struct power_off_prep_data *data);
+	int power_off_priority;
+	bool power_off_chaining_allowed;
+
+	void (*restart_cb)(struct restart_data *data);
+	int restart_priority;
+	bool restart_chaining_disallowed;
+
+	void (*reboot_prepare_cb)(struct reboot_prep_data *data);
+	int reboot_priority;
+
+	const struct sys_off_handler_private_data priv;
+};
+
+int register_sys_off_handler(struct sys_off_handler *handler);
+int unregister_sys_off_handler(struct sys_off_handler *handler);
+
+int devm_register_sys_off_handler(struct device *dev,
+				  struct sys_off_handler *handler);
+
+int devm_register_prioritized_power_off_handler(struct device *dev,
+						int priority,
+						void (*callback)(void *data),
+						void *cb_data);
+
+int devm_register_simple_power_off_handler(struct device *dev,
+					   void (*callback)(void *data),
+					   void *cb_data);
+
+int register_platform_power_off(void (*power_off)(void));
+int unregister_platform_power_off(void (*power_off)(void));
+
+int devm_register_prioritized_restart_handler(struct device *dev,
+					      int priority,
+					      void (*callback)(struct restart_data *data),
+					      void *cb_data);
+
+int devm_register_simple_restart_handler(struct device *dev,
+					 void (*callback)(struct restart_data *data),
+					 void *cb_data);
+
+void do_kernel_power_off(void);
+
 /*
  * Architecture-specific implementations of sys_reboot commands.
  */
@@ -70,6 +290,7 @@ extern void kernel_restart_prepare(char *cmd);
 extern void kernel_restart(char *cmd);
 extern void kernel_halt(void);
 extern void kernel_power_off(void);
+extern bool kernel_can_power_off(void);
 
 void ctrl_alt_del(void);
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 938d5c78b421..a9464b4a3209 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -665,7 +665,7 @@ static void power_down(void)
 		hibernation_platform_enter();
 		fallthrough;
 	case HIBERNATION_SHUTDOWN:
-		if (pm_power_off)
+		if (kernel_can_power_off())
 			kernel_power_off();
 		break;
 	}
diff --git a/kernel/reboot.c b/kernel/reboot.c
index acdae4e95061..e76e2570dcf5 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -296,6 +296,595 @@ void kernel_halt(void)
 }
 EXPORT_SYMBOL_GPL(kernel_halt);
 
+/*
+ *	Notifier list for kernel code which wants to be called
+ *	to power off the system.
+ */
+static BLOCKING_NOTIFIER_HEAD(power_off_handler_list);
+
+/*
+ * Temporary stub that prevents linkage failure while we're in process
+ * of removing all uses of legacy pm_power_off() around the kernel.
+ */
+void __weak (*pm_power_off)(void);
+
+static void dummy_pm_power_off(void)
+{
+	/* temporary stub until pm_power_off() is gone, see more below */
+}
+
+static struct notifier_block *pm_power_off_nb;
+
+/**
+ *	register_power_off_handler - Register function to be called to power off
+ *				     the system
+ *	@nb: Info about handler function to be called
+ *	@nb->priority:	Handler priority. Handlers should follow the
+ *			following guidelines for setting priorities.
+ *			0:	Reserved
+ *			1:	Power-off handler of last resort,
+ *				with limited power-off capabilities
+ *			128:	Default power-off handler; use if no other
+ *				power-off handler is expected to be available,
+ *				and/or if power-off functionality is
+ *				sufficient to power-off the entire system
+ *			255:	Highest priority power-off handler, will
+ *				preempt all other power-off handlers
+ *
+ *	Registers a function with code to be called to power off the
+ *	system.
+ *
+ *	Registered functions will be called as last step of the power-off
+ *	sequence.
+ *
+ *	Registered functions are expected to power off the system immediately.
+ *	If more than one function is registered, the power-off handler priority
+ *	selects which function will be called first.
+ *
+ *	Power-off handlers are expected to be registered from non-architecture
+ *	code, typically from drivers. A typical use case would be a system
+ *	where power-off functionality is provided through a PMIC. Multiple
+ *	power-off handlers may exist; for example, one power-off handler might
+ *	turn off the entire system, while another only turns off part of
+ *	system. In such cases, the power-off handler which only disables part
+ *	of the hardware is expected to register with low priority to ensure
+ *	that it only runs if no other means to power off the system is
+ *	available.
+ *
+ *	Currently always returns zero, as blocking_notifier_chain_register()
+ *	always returns zero.
+ */
+static int register_power_off_handler(struct notifier_block *nb)
+{
+	int ret;
+
+	ret = blocking_notifier_chain_register_unique_prio(&power_off_handler_list, nb);
+	if (ret && ret != -EBUSY)
+		return ret;
+
+	if (!ret)
+		goto set_pm_power_off;
+
+	/*
+	 * Handler must have unique priority. Otherwise call order is
+	 * determined by registration order, which is unreliable.
+	 *
+	 * This requirement will become mandatory once all drivers
+	 * will be converted to use new sys-off API.
+	 */
+	pr_err("failed to register power-off handler using unique priority\n");
+
+	ret = blocking_notifier_chain_register(&power_off_handler_list, nb);
+	if (ret)
+		return ret;
+
+	/*
+	 * Some drivers check whether pm_power_off was already installed.
+	 * Install dummy callback using new API to preserve old behaviour
+	 * for those drivers during period of transition to the new API.
+	 */
+set_pm_power_off:
+	if (!pm_power_off) {
+		pm_power_off = dummy_pm_power_off;
+		pm_power_off_nb = nb;
+	}
+
+	return 0;
+}
+
+static int unregister_power_off_handler(struct notifier_block *nb)
+{
+	if (nb == pm_power_off_nb) {
+		if (pm_power_off == dummy_pm_power_off)
+			pm_power_off = NULL;
+
+		pm_power_off_nb = NULL;
+	}
+
+	return blocking_notifier_chain_unregister(&power_off_handler_list, nb);
+}
+
+static void devm_unregister_power_off_handler(void *data)
+{
+	struct notifier_block *nb = data;
+
+	unregister_power_off_handler(nb);
+}
+
+static int devm_register_power_off_handler(struct device *dev,
+					   struct notifier_block *nb)
+{
+	int err;
+
+	err = register_power_off_handler(nb);
+	if (err)
+		return err;
+
+	return devm_add_action_or_reset(dev, devm_unregister_power_off_handler,
+					nb);
+}
+
+static int sys_off_handler_power_off(struct notifier_block *nb,
+				     unsigned long mode, void *unused)
+{
+	struct power_off_prep_data prep_data = {};
+	struct sys_off_handler_private_data *priv;
+	struct power_off_data data = {};
+	struct sys_off_handler *h;
+	int ret = NOTIFY_DONE;
+
+	priv = container_of(nb, struct sys_off_handler_private_data, power_off_nb);
+	h = container_of(priv, struct sys_off_handler, priv);
+	prep_data.cb_data = h->cb_data;
+	data.cb_data = h->cb_data;
+
+	switch (mode) {
+	case POWEROFF_NORMAL:
+		if (h->power_off_cb)
+			h->power_off_cb(&data);
+
+		if (priv->simple_power_off_cb)
+			priv->simple_power_off_cb(priv->simple_power_off_cb_data);
+
+		if (priv->platform_power_off_cb)
+			priv->platform_power_off_cb();
+
+		if (!h->power_off_chaining_allowed)
+			ret = NOTIFY_STOP;
+
+		break;
+
+	case POWEROFF_PREPARE:
+		if (h->power_off_prepare_cb)
+			h->power_off_prepare_cb(&prep_data);
+
+		break;
+
+	default:
+		unreachable();
+	}
+
+	return ret;
+}
+
+static int sys_off_handler_restart(struct notifier_block *nb,
+				   unsigned long mode, void *cmd)
+{
+	struct sys_off_handler_private_data *priv;
+	struct restart_data data = {};
+	struct sys_off_handler *h;
+
+	priv = container_of(nb, struct sys_off_handler_private_data, restart_nb);
+	h = container_of(priv, struct sys_off_handler, priv);
+
+	data.stop_chain = h->restart_chaining_disallowed;
+	data.cb_data = h->cb_data;
+	data.mode = mode;
+	data.cmd = cmd;
+
+	h->restart_cb(&data);
+
+	return data.stop_chain ? NOTIFY_STOP : NOTIFY_DONE;
+}
+
+static int sys_off_handler_reboot(struct notifier_block *nb,
+				  unsigned long mode, void *cmd)
+{
+	struct sys_off_handler_private_data *priv;
+	struct reboot_prep_data data = {};
+	struct sys_off_handler *h;
+
+	priv = container_of(nb, struct sys_off_handler_private_data, reboot_nb);
+	h = container_of(priv, struct sys_off_handler, priv);
+
+	data.cb_data = h->cb_data;
+	data.stop_chain = false;
+	data.mode = mode;
+	data.cmd = cmd;
+
+	h->reboot_prepare_cb(&data);
+
+	return data.stop_chain ? NOTIFY_STOP : NOTIFY_DONE;
+}
+
+static struct sys_off_handler_private_data *
+sys_off_handler_private_data(struct sys_off_handler *handler)
+{
+	return (struct sys_off_handler_private_data *)&handler->priv;
+}
+
+/**
+ *	devm_register_sys_off_handler - Register system power-off/restart handler
+ *	@dev: Device that registers handler
+ *	@handler: System-off handler
+ *
+ *	Registers handler that will be called as last step of the power-off
+ *	and restart sequences.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int register_sys_off_handler(struct sys_off_handler *handler)
+{
+	struct sys_off_handler_private_data *priv;
+	int err, priority;
+
+	priv = sys_off_handler_private_data(handler);
+
+	/* sanity-check whether handler is registered twice */
+	if (priv->registered)
+		return -EBUSY;
+
+	if (handler->power_off_cb || handler->power_off_prepare_cb) {
+		if (handler->power_off_priority == POWEROFF_PRIO_RESERVED)
+			priority = POWEROFF_PRIO_DEFAULT;
+		else
+			priority = handler->power_off_priority;
+
+		priv->power_off_nb.notifier_call = sys_off_handler_power_off;
+		priv->power_off_nb.priority = priority;
+
+		err = register_power_off_handler(&priv->power_off_nb);
+		if (err)
+			goto reset_sys_off_handler;
+	}
+
+	if (handler->restart_cb) {
+		if (handler->restart_priority == RESTART_PRIO_RESERVED)
+			priority = RESTART_PRIO_DEFAULT;
+		else
+			priority = handler->restart_priority;
+
+		priv->restart_nb.notifier_call = sys_off_handler_restart;
+		priv->restart_nb.priority = priority;
+
+		err = register_restart_handler(&priv->restart_nb);
+		if (err)
+			goto unreg_power_off_handler;
+	}
+
+	if (handler->reboot_prepare_cb) {
+		priv->reboot_nb.notifier_call = sys_off_handler_reboot;
+		priv->reboot_nb.priority = handler->reboot_priority;
+
+		err = register_reboot_notifier(&priv->reboot_nb);
+		if (err)
+			goto unreg_restart_handler;
+	}
+
+	priv->registered = true;
+
+	return 0;
+
+unreg_restart_handler:
+	if (handler->restart_cb)
+		unregister_restart_handler(&priv->restart_nb);
+
+unreg_power_off_handler:
+	if (handler->power_off_cb)
+		unregister_power_off_handler(&priv->power_off_nb);
+
+reset_sys_off_handler:
+	memset(priv, 0, sizeof(*priv));
+
+	return err;
+}
+EXPORT_SYMBOL(register_sys_off_handler);
+
+/**
+ *	unregister_sys_off_handler - Unregister system power-off/restart handler
+ *	@handler: System-off handler
+ *
+ *	Unregisters sys-off handler. Does nothing and returns zero if handler
+ *	is NULL.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int unregister_sys_off_handler(struct sys_off_handler *handler)
+{
+	struct sys_off_handler_private_data *priv;
+
+	if (!handler)
+		return 0;
+
+	priv = sys_off_handler_private_data(handler);
+
+	/* sanity-check whether handler is unregistered twice */
+	if (!priv->registered)
+		return -EINVAL;
+
+	if (handler->reboot_prepare_cb)
+		unregister_reboot_notifier(&priv->reboot_nb);
+
+	if (handler->restart_cb)
+		unregister_restart_handler(&priv->restart_nb);
+
+	if (handler->power_off_cb)
+		unregister_power_off_handler(&priv->power_off_nb);
+
+	memset(priv, 0, sizeof(*priv));
+
+	return 0;
+}
+EXPORT_SYMBOL(unregister_sys_off_handler);
+
+static void devm_unregister_sys_off_handler(void *data)
+{
+	struct sys_off_handler *handler = data;
+
+	unregister_sys_off_handler(handler);
+}
+
+/**
+ *	devm_register_sys_off_handler - Register system power-off/restart handler
+ *	@dev: Device that registers handler
+ *	@handler: System-off handler
+ *
+ *	Resource-managed variant of register_sys_off_handler().
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int devm_register_sys_off_handler(struct device *dev,
+				  struct sys_off_handler *handler)
+{
+	int err;
+
+	err = register_sys_off_handler(handler);
+	if (err)
+		return err;
+
+	return devm_add_action_or_reset(dev, devm_unregister_sys_off_handler,
+					handler);
+}
+EXPORT_SYMBOL(devm_register_sys_off_handler);
+
+/**
+ *	devm_register_prioritized_power_off_handler - Register prioritized power-off callback
+ *	@dev: Device that registers callback
+ *	@priority: Callback's priority
+ *	@callback: Callback function
+ *	@cb_data: Callback's argument
+ *
+ *	Registers resource-managed power-off callback with a given priority.
+ *	It will be called as last step of the power-off sequence. Callbacks
+ *	chaining is disabled, i.e. further lower priority callbacks won't
+ *	be executed if this @callback will fail to execute.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int devm_register_prioritized_power_off_handler(struct device *dev,
+						int priority,
+						void (*callback)(void *data),
+						void *cb_data)
+{
+	struct sys_off_handler_private_data *priv;
+	struct sys_off_handler *handler;
+
+	handler = devm_kzalloc(dev, sizeof(*handler), GFP_KERNEL);
+	if (!handler)
+		return -ENOMEM;
+
+	if (priority == POWEROFF_PRIO_RESERVED)
+		priority = POWEROFF_PRIO_DEFAULT;
+
+	priv = sys_off_handler_private_data(handler);
+
+	priv->power_off_nb.notifier_call = sys_off_handler_power_off;
+	priv->power_off_nb.priority = priority;
+	priv->simple_power_off_cb_data = cb_data;
+	priv->simple_power_off_cb = callback;
+
+	return devm_register_power_off_handler(dev, &priv->power_off_nb);
+}
+EXPORT_SYMBOL(devm_register_prioritized_power_off_handler);
+
+int devm_register_prioritized_power_off_handler(struct device *dev,
+						int priority,
+						void (*callback)(void *data),
+						void *cb_data);
+
+/**
+ *	devm_register_simple_power_off_handler - Register simple power-off callback
+ *	@dev: Device that registers callback
+ *	@callback: Callback function
+ *	@cb_data: Callback's argument
+ *
+ *	Registers resource-managed power-off callback with default priority.
+ *	It will be invoked as last step of the power-off sequence. Further
+ *	lower priority callbacks won't be executed if this @callback fails.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int devm_register_simple_power_off_handler(struct device *dev,
+					   void (*callback)(void *data),
+					   void *cb_data)
+{
+	return devm_register_prioritized_power_off_handler(dev,
+							   POWEROFF_PRIO_DEFAULT,
+							   callback, cb_data);
+}
+EXPORT_SYMBOL(devm_register_simple_power_off_handler);
+
+/**
+ *	devm_register_prioritized_restart_handler - Register prioritized restart callback
+ *	@dev: Device that registers callback
+ *	@priority: Callback's priority
+ *	@callback: Callback function
+ *	@cb_data: Callback's argument
+ *
+ *	Registers resource-managed restart callback with a given priority.
+ *	It will be called as a part of the restart sequence. Callbacks
+ *	chaining is disabled, i.e. further lower priority callbacks won't
+ *	be executed if this @callback will fail to execute.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int devm_register_prioritized_restart_handler(struct device *dev,
+					      int priority,
+					      void (*callback)(struct restart_data *data),
+					      void *cb_data)
+{
+	struct sys_off_handler *handler;
+
+	handler = devm_kzalloc(dev, sizeof(*handler), GFP_KERNEL);
+	if (!handler)
+		return -ENOMEM;
+
+	if (priority == RESTART_PRIO_RESERVED)
+		priority = RESTART_PRIO_DEFAULT;
+
+	handler->restart_priority = priority;
+	handler->restart_cb = callback;
+	handler->cb_data = cb_data;
+
+	return devm_register_sys_off_handler(dev, handler);
+}
+EXPORT_SYMBOL(devm_register_prioritized_restart_handler);
+
+/**
+ *	devm_register_simple_restart_handler - Register simple restart callback
+ *	@dev: Device that registers callback
+ *	@callback: Callback function
+ *	@cb_data: Callback's argument
+ *
+ *	Registers resource-managed restart callback with default priority.
+ *	It will be invoked as a part of the restart sequence. Further
+ *	lower priority callback will be executed if this @callback fails.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int devm_register_simple_restart_handler(struct device *dev,
+					 void (*callback)(struct restart_data *data),
+					 void *cb_data)
+{
+	return devm_register_prioritized_restart_handler(dev,
+							 RESTART_PRIO_DEFAULT,
+							 callback, cb_data);
+}
+EXPORT_SYMBOL(devm_register_simple_restart_handler);
+
+static struct sys_off_handler platform_power_off_handler = {
+	.priv = {
+		.power_off_nb = {
+			.notifier_call = sys_off_handler_power_off,
+			.priority = POWEROFF_PRIO_PLATFORM,
+		},
+	},
+};
+
+static DEFINE_SPINLOCK(platform_power_off_lock);
+
+/**
+ *	register_platform_power_off - Register platform-level power-off callback
+ *	@power_off: Power-off callback
+ *
+ *	Registers power-off callback that will be called as last step
+ *	of the power-off sequence. This callback is expected to be invoked
+ *	for the last resort. Further lower priority callbacks won't be
+ *	executed if @power_off fails. Only one platform power-off callback
+ *	is allowed to be registered at a time.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int register_platform_power_off(void (*power_off)(void))
+{
+	struct sys_off_handler_private_data *priv;
+	int ret = 0;
+
+	priv = sys_off_handler_private_data(&platform_power_off_handler);
+
+	spin_lock(&platform_power_off_lock);
+	if (priv->platform_power_off_cb)
+		ret = -EBUSY;
+	else
+		priv->platform_power_off_cb = power_off;
+	spin_unlock(&platform_power_off_lock);
+
+	if (ret)
+		return ret;
+
+	ret = register_power_off_handler(&priv->power_off_nb);
+	if (ret)
+		priv->platform_power_off_cb = NULL;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(register_platform_power_off);
+
+/**
+ *	unregister_platform_power_off - Unregister platform-level power-off callback
+ *	@power_off: Power-off callback
+ *
+ *	Unregisters previously registered platform power-off callback.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int unregister_platform_power_off(void (*power_off)(void))
+{
+	struct sys_off_handler_private_data *priv;
+	int ret;
+
+	priv = sys_off_handler_private_data(&platform_power_off_handler);
+
+	if (priv->platform_power_off_cb != power_off)
+		return -EINVAL;
+
+	ret = unregister_power_off_handler(&priv->power_off_nb);
+	priv->platform_power_off_cb = NULL;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(unregister_platform_power_off);
+
+/**
+ *	do_kernel_power_off - Execute kernel power-off handler call chain
+ *
+ *	Calls functions registered with register_power_off_handler.
+ *
+ *	Expected to be called as last step of the power-off sequence.
+ *
+ *	Powers off the system immediately if a power-off handler function has
+ *	been registered. Otherwise does nothing.
+ */
+void do_kernel_power_off(void)
+{
+	/* legacy pm_power_off() is unchained and has highest priority */
+	if (pm_power_off && pm_power_off != dummy_pm_power_off)
+		return pm_power_off();
+
+	blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_NORMAL,
+				     NULL);
+}
+
+static void do_kernel_power_off_prepare(void)
+{
+	/* legacy pm_power_off_prepare() is unchained and has highest priority */
+	if (pm_power_off_prepare)
+		return pm_power_off_prepare();
+
+	blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_PREPARE,
+				     NULL);
+}
+
 /**
  *	kernel_power_off - power_off the system
  *
@@ -304,8 +893,7 @@ EXPORT_SYMBOL_GPL(kernel_halt);
 void kernel_power_off(void)
 {
 	kernel_shutdown_prepare(SYSTEM_POWER_OFF);
-	if (pm_power_off_prepare)
-		pm_power_off_prepare();
+	do_kernel_power_off_prepare();
 	migrate_to_reboot_cpu();
 	syscore_shutdown();
 	pr_emerg("Power down\n");
@@ -314,6 +902,16 @@ void kernel_power_off(void)
 }
 EXPORT_SYMBOL_GPL(kernel_power_off);
 
+bool kernel_can_power_off(void)
+{
+	if (!pm_power_off &&
+	    blocking_notifier_call_chain_is_empty(&power_off_handler_list))
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(kernel_can_power_off);
+
 DEFINE_MUTEX(system_transition_mutex);
 
 /*
@@ -355,7 +953,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 	/* Instead of trying to make the power_off code look like
 	 * halt when pm_power_off is not set do it the easy way.
 	 */
-	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
+	if (cmd == LINUX_REBOOT_CMD_POWER_OFF && !kernel_can_power_off())
 		cmd = LINUX_REBOOT_CMD_HALT;
 
 	mutex_lock(&system_transition_mutex);
-- 
2.35.1

[PATCH v7 05/20] ARM: Use do_kernel_power_off()

[Dmitry Osipenko]

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.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 arch/arm/kernel/reboot.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
index 3044fcb8d073..2cb943422554 100644
--- a/arch/arm/kernel/reboot.c
+++ b/arch/arm/kernel/reboot.c
@@ -116,9 +116,7 @@ void machine_power_off(void)
 {
 	local_irq_disable();
 	smp_send_stop();
-
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 }
 
 /*
-- 
2.35.1

[PATCH v7 06/20] csky: Use do_kernel_power_off()

[Dmitry Osipenko]

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.

Acked-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 arch/csky/kernel/power.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/csky/kernel/power.c b/arch/csky/kernel/power.c
index 923ee4e381b8..86ee202906f8 100644
--- a/arch/csky/kernel/power.c
+++ b/arch/csky/kernel/power.c
@@ -9,16 +9,14 @@ EXPORT_SYMBOL(pm_power_off);
 void machine_power_off(void)
 {
 	local_irq_disable();
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 	asm volatile ("bkpt");
 }
 
 void machine_halt(void)
 {
 	local_irq_disable();
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 	asm volatile ("bkpt");
 }
 
-- 
2.35.1

[PATCH v7 07/20] riscv: Use do_kernel_power_off()

[Dmitry Osipenko]

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.

Acked-by: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 arch/riscv/kernel/reset.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/reset.c b/arch/riscv/kernel/reset.c
index 9c842c41684a..912288572226 100644
--- a/arch/riscv/kernel/reset.c
+++ b/arch/riscv/kernel/reset.c
@@ -23,16 +23,12 @@ void machine_restart(char *cmd)
 
 void machine_halt(void)
 {
-	if (pm_power_off != NULL)
-		pm_power_off();
-	else
-		default_power_off();
+	do_kernel_power_off();
+	default_power_off();
 }
 
 void machine_power_off(void)
 {
-	if (pm_power_off != NULL)
-		pm_power_off();
-	else
-		default_power_off();
+	do_kernel_power_off();
+	default_power_off();
 }
-- 
2.35.1

[PATCH v7 08/20] arm64: Use do_kernel_power_off()

[Dmitry Osipenko]

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.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 arch/arm64/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 7fa97df55e3a..26d1b0a4329a 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -111,8 +111,7 @@ void machine_power_off(void)
 {
 	local_irq_disable();
 	smp_send_stop();
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 }
 
 /*
-- 
2.35.1

[PATCH v7 09/20] parisc: Use do_kernel_power_off()

[Dmitry Osipenko]

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.

Acked-by: Helge Deller <deller@gmx.de> # parisc
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 arch/parisc/kernel/process.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index 28b6a2a5574c..d145184696ea 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/personality.h>
 #include <linux/ptrace.h>
+#include <linux/reboot.h>
 #include <linux/sched.h>
 #include <linux/sched/debug.h>
 #include <linux/sched/task.h>
@@ -116,8 +117,7 @@ void machine_power_off(void)
 	pdc_chassis_send_status(PDC_CHASSIS_DIRECT_SHUTDOWN);
 
 	/* ipmi_poweroff may have been installed. */
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 		
 	/* It seems we have no way to power the system off via
 	 * software. The user has to press the button himself. */
-- 
2.35.1

[PATCH v7 10/20] xen/x86: Use do_kernel_power_off()

[Dmitry Osipenko]

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.

Acked-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 arch/x86/xen/enlighten_pv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 5038edb79ad5..af1f6e886225 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -31,6 +31,7 @@
 #include <linux/gfp.h>
 #include <linux/edd.h>
 #include <linux/objtool.h>
+#include <linux/reboot.h>
 
 #include <xen/xen.h>
 #include <xen/events.h>
@@ -1071,8 +1072,7 @@ static void xen_machine_halt(void)
 
 static void xen_machine_power_off(void)
 {
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 	xen_reboot(SHUTDOWN_poweroff);
 }
 
-- 
2.35.1

[PATCH v7 11/20] powerpc: Use do_kernel_power_off()

[Dmitry Osipenko]

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.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 arch/powerpc/kernel/setup-common.c | 4 +---
 arch/powerpc/xmon/xmon.c           | 3 +--
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 518ae5aa9410..1b586577e75b 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -161,9 +161,7 @@ void machine_restart(char *cmd)
 void machine_power_off(void)
 {
 	machine_shutdown();
-	if (pm_power_off)
-		pm_power_off();
-
+	do_kernel_power_off();
 	smp_send_stop();
 	machine_hang();
 }
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index fd72753e8ad5..c916bf250796 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1243,8 +1243,7 @@ static void bootcmds(void)
 	} else if (cmd == 'h') {
 		ppc_md.halt();
 	} else if (cmd == 'p') {
-		if (pm_power_off)
-			pm_power_off();
+		do_kernel_power_off();
 	}
 }
 
-- 
2.35.1

[PATCH v7 12/20] m68k: Switch to new sys-off handler API

[Dmitry Osipenko]

Kernel now supports chained power-off handlers. Use
register_power_off_handler() that registers power-off handlers and
do_kernel_power_off() that invokes chained power-off handlers. Legacy
pm_power_off() will be removed once all drivers will be converted to
the new power-off API.

Normally arch code should adopt only the do_kernel_power_off() at first,
but m68k is a special case because it uses pm_power_off() "inside out",
i.e. pm_power_off() invokes machine_power_off() [in fact it does nothing],
while it's machine_power_off() that should invoke the pm_power_off(), and
thus, we can't convert platforms to the new API separately. There are only
two platforms changed here, so it's not a big deal.

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 arch/m68k/emu/natfeat.c         | 3 ++-
 arch/m68k/include/asm/machdep.h | 1 -
 arch/m68k/kernel/process.c      | 5 ++---
 arch/m68k/kernel/setup_mm.c     | 1 -
 arch/m68k/kernel/setup_no.c     | 1 -
 arch/m68k/mac/config.c          | 4 +++-
 6 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/m68k/emu/natfeat.c b/arch/m68k/emu/natfeat.c
index 71b78ecee75c..b19dc00026d9 100644
--- a/arch/m68k/emu/natfeat.c
+++ b/arch/m68k/emu/natfeat.c
@@ -15,6 +15,7 @@
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/reboot.h>
 #include <linux/io.h>
 #include <asm/machdep.h>
 #include <asm/natfeat.h>
@@ -90,5 +91,5 @@ void __init nf_init(void)
 	pr_info("NatFeats found (%s, %lu.%lu)\n", buf, version >> 16,
 		version & 0xffff);
 
-	mach_power_off = nf_poweroff;
+	register_platform_power_off(nf_poweroff);
 }
diff --git a/arch/m68k/include/asm/machdep.h b/arch/m68k/include/asm/machdep.h
index 8fd80ef1b77e..8d8c3ee2069f 100644
--- a/arch/m68k/include/asm/machdep.h
+++ b/arch/m68k/include/asm/machdep.h
@@ -24,7 +24,6 @@ extern int (*mach_get_rtc_pll)(struct rtc_pll_info *);
 extern int (*mach_set_rtc_pll)(struct rtc_pll_info *);
 extern void (*mach_reset)( void );
 extern void (*mach_halt)( void );
-extern void (*mach_power_off)( void );
 extern unsigned long (*mach_hd_init) (unsigned long, unsigned long);
 extern void (*mach_hd_setup)(char *, int *);
 extern void (*mach_heartbeat) (int);
diff --git a/arch/m68k/kernel/process.c b/arch/m68k/kernel/process.c
index a6030dbaa089..e160a7c57bd3 100644
--- a/arch/m68k/kernel/process.c
+++ b/arch/m68k/kernel/process.c
@@ -67,12 +67,11 @@ void machine_halt(void)
 
 void machine_power_off(void)
 {
-	if (mach_power_off)
-		mach_power_off();
+	do_kernel_power_off();
 	for (;;);
 }
 
-void (*pm_power_off)(void) = machine_power_off;
+void (*pm_power_off)(void);
 EXPORT_SYMBOL(pm_power_off);
 
 void show_regs(struct pt_regs * regs)
diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
index 8f94feed969c..47d55541612f 100644
--- a/arch/m68k/kernel/setup_mm.c
+++ b/arch/m68k/kernel/setup_mm.c
@@ -98,7 +98,6 @@ EXPORT_SYMBOL(mach_get_rtc_pll);
 EXPORT_SYMBOL(mach_set_rtc_pll);
 void (*mach_reset)( void );
 void (*mach_halt)( void );
-void (*mach_power_off)( void );
 #ifdef CONFIG_HEARTBEAT
 void (*mach_heartbeat) (int);
 EXPORT_SYMBOL(mach_heartbeat);
diff --git a/arch/m68k/kernel/setup_no.c b/arch/m68k/kernel/setup_no.c
index 5e4104f07a44..00bf82258233 100644
--- a/arch/m68k/kernel/setup_no.c
+++ b/arch/m68k/kernel/setup_no.c
@@ -55,7 +55,6 @@ int (*mach_hwclk) (int, struct rtc_time*);
 /* machine dependent reboot functions */
 void (*mach_reset)(void);
 void (*mach_halt)(void);
-void (*mach_power_off)(void);
 
 #ifdef CONFIG_M68000
 #if defined(CONFIG_M68328)
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 65d124ec80bb..382f656c29ea 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -12,6 +12,7 @@
 
 #include <linux/errno.h>
 #include <linux/module.h>
+#include <linux/reboot.h>
 #include <linux/types.h>
 #include <linux/mm.h>
 #include <linux/tty.h>
@@ -140,7 +141,6 @@ void __init config_mac(void)
 	mach_hwclk = mac_hwclk;
 	mach_reset = mac_reset;
 	mach_halt = mac_poweroff;
-	mach_power_off = mac_poweroff;
 #if IS_ENABLED(CONFIG_INPUT_M68K_BEEP)
 	mach_beep = mac_mksound;
 #endif
@@ -160,6 +160,8 @@ void __init config_mac(void)
 
 	if (macintosh_config->ident == MAC_MODEL_IICI)
 		mach_l2_flush = via_l2_flush;
+
+	register_platform_power_off(mac_poweroff);
 }
 
 
-- 
2.35.1

[PATCH v7 13/20] sh: Use do_kernel_power_off()

[Dmitry Osipenko]

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 <dmitry.osipenko@collabora.com>
---
 arch/sh/kernel/reboot.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/sh/kernel/reboot.c b/arch/sh/kernel/reboot.c
index 5c33f036418b..e8eeedc9b182 100644
--- a/arch/sh/kernel/reboot.c
+++ b/arch/sh/kernel/reboot.c
@@ -46,8 +46,7 @@ static void native_machine_shutdown(void)
 
 static void native_machine_power_off(void)
 {
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 }
 
 static void native_machine_halt(void)
-- 
2.35.1

[PATCH v7 14/20] x86: Use do_kernel_power_off()

[Dmitry Osipenko]

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 <dmitry.osipenko@collabora.com>
---
 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 fa700b46588e..c3636ea4aa71 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -739,10 +739,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();
 	}
 	/* A fallback in case there is no PM info available */
 	tboot_shutdown(TB_SHUTDOWN_HALT);
-- 
2.35.1

[PATCH v7 15/20] ia64: Use do_kernel_power_off()

[Dmitry Osipenko]

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 <dmitry.osipenko@collabora.com>
---
 arch/ia64/kernel/process.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index d7a256bd9d6b..89025e3b3f61 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/personality.h>
+#include <linux/reboot.h>
 #include <linux/sched.h>
 #include <linux/sched/debug.h>
 #include <linux/sched/hotplug.h>
@@ -599,8 +600,7 @@ machine_halt (void)
 void
 machine_power_off (void)
 {
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 	machine_halt();
 }
 
-- 
2.35.1

[PATCH v7 16/20] mips: Use do_kernel_power_off()

[Dmitry Osipenko]

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 <dmitry.osipenko@collabora.com>
---
 arch/mips/kernel/reset.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
index 6288780b779e..e7ce07b3e79b 100644
--- a/arch/mips/kernel/reset.c
+++ b/arch/mips/kernel/reset.c
@@ -114,8 +114,7 @@ void machine_halt(void)
 
 void machine_power_off(void)
 {
-	if (pm_power_off)
-		pm_power_off();
+	do_kernel_power_off();
 
 #ifdef CONFIG_SMP
 	preempt_disable();
-- 
2.35.1

[PATCH v7 17/20] memory: emif: Use kernel_can_power_off()

[Dmitry Osipenko]

Replace legacy pm_power_off with kernel_can_power_off() helper that
is aware about chained power-off handlers.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 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 edf3ba7447ed..fa6845313a43 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");
-- 
2.35.1

[PATCH v7 18/20] ACPI: power: Switch to sys-off handler API

[Dmitry Osipenko]

Switch to sys-off API that replaces legacy pm_power_off callbacks.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/acpi/sleep.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index c992e57b2c79..426297258e26 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -47,19 +47,11 @@ static void acpi_sleep_tts_switch(u32 acpi_state)
 	}
 }
 
-static int tts_notify_reboot(struct notifier_block *this,
-			unsigned long code, void *x)
+static void tts_reboot_prepare(struct reboot_prep_data *data)
 {
 	acpi_sleep_tts_switch(ACPI_STATE_S5);
-	return NOTIFY_DONE;
 }
 
-static struct notifier_block tts_notifier = {
-	.notifier_call	= tts_notify_reboot,
-	.next		= NULL,
-	.priority	= 0,
-};
-
 static int acpi_sleep_prepare(u32 acpi_state)
 {
 #ifdef CONFIG_ACPI_SLEEP
@@ -1023,7 +1015,7 @@ static void acpi_sleep_hibernate_setup(void)
 static inline void acpi_sleep_hibernate_setup(void) {}
 #endif /* !CONFIG_HIBERNATION */
 
-static void acpi_power_off_prepare(void)
+static void acpi_power_off_prepare(struct power_off_prep_data *data)
 {
 	/* Prepare to power off the system */
 	acpi_sleep_prepare(ACPI_STATE_S5);
@@ -1031,7 +1023,7 @@ static void acpi_power_off_prepare(void)
 	acpi_os_wait_events_complete();
 }
 
-static void acpi_power_off(void)
+static void acpi_power_off(struct power_off_data *data)
 {
 	/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
 	pr_debug("%s called\n", __func__);
@@ -1039,6 +1031,11 @@ static void acpi_power_off(void)
 	acpi_enter_sleep_state(ACPI_STATE_S5);
 }
 
+static struct sys_off_handler acpi_sys_off_handler = {
+	.power_off_priority = POWEROFF_PRIO_FIRMWARE,
+	.reboot_prepare_cb = tts_reboot_prepare,
+};
+
 int __init acpi_sleep_init(void)
 {
 	char supported[ACPI_S_STATE_COUNT * 3 + 1];
@@ -1055,8 +1052,8 @@ int __init acpi_sleep_init(void)
 
 	if (acpi_sleep_state_supported(ACPI_STATE_S5)) {
 		sleep_states[ACPI_STATE_S5] = 1;
-		pm_power_off_prepare = acpi_power_off_prepare;
-		pm_power_off = acpi_power_off;
+		acpi_sys_off_handler.power_off_cb = acpi_power_off;
+		acpi_sys_off_handler.power_off_prepare_cb = acpi_power_off_prepare;
 	} else {
 		acpi_no_s5 = true;
 	}
@@ -1072,6 +1069,6 @@ int __init acpi_sleep_init(void)
 	 * Register the tts_notifier to reboot notifier list so that the _TTS
 	 * object can also be evaluated when the system enters S5.
 	 */
-	register_reboot_notifier(&tts_notifier);
+	register_sys_off_handler(&acpi_sys_off_handler);
 	return 0;
 }
-- 
2.35.1

[PATCH v7 19/20] regulator: pfuze100: Use devm_register_sys_off_handler()

[Dmitry Osipenko]

Use devm_register_sys_off_handler() that replaces global
pm_power_off_prepare variable and allows to register multiple
power-off handlers.

Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/regulator/pfuze100-regulator.c | 38 ++++++++++----------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
index d60d7d1b7fa2..2eca8d43a097 100644
--- a/drivers/regulator/pfuze100-regulator.c
+++ b/drivers/regulator/pfuze100-regulator.c
@@ -10,6 +10,7 @@
 #include <linux/of_device.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/pfuze100.h>
@@ -76,6 +77,7 @@ struct pfuze_chip {
 	struct pfuze_regulator regulator_descs[PFUZE100_MAX_REGULATOR];
 	struct regulator_dev *regulators[PFUZE100_MAX_REGULATOR];
 	struct pfuze_regulator *pfuze_regulators;
+	struct sys_off_handler sys_off;
 };
 
 static const int pfuze100_swbst[] = {
@@ -569,10 +571,10 @@ static inline struct device_node *match_of_node(int index)
 	return pfuze_matches[index].of_node;
 }
 
-static struct pfuze_chip *syspm_pfuze_chip;
-
-static void pfuze_power_off_prepare(void)
+static void pfuze_power_off_prepare(struct power_off_prep_data *data)
 {
+	struct pfuze_chip *syspm_pfuze_chip = data->cb_data;
+
 	dev_info(syspm_pfuze_chip->dev, "Configure standby mode for power off");
 
 	/* Switch from default mode: APS/APS to APS/Off */
@@ -611,24 +613,23 @@ static void pfuze_power_off_prepare(void)
 
 static int pfuze_power_off_prepare_init(struct pfuze_chip *pfuze_chip)
 {
+	int err;
+
 	if (pfuze_chip->chip_id != PFUZE100) {
 		dev_warn(pfuze_chip->dev, "Requested pm_power_off_prepare handler for not supported chip\n");
 		return -ENODEV;
 	}
 
-	if (pm_power_off_prepare) {
-		dev_warn(pfuze_chip->dev, "pm_power_off_prepare is already registered.\n");
-		return -EBUSY;
-	}
+	pfuze_chip->sys_off.power_off_prepare_cb = pfuze_power_off_prepare;
+	pfuze_chip->sys_off.cb_data = pfuze_chip;
 
-	if (syspm_pfuze_chip) {
-		dev_warn(pfuze_chip->dev, "syspm_pfuze_chip is already set.\n");
-		return -EBUSY;
+	err = devm_register_sys_off_handler(pfuze_chip->dev, &pfuze_chip->sys_off);
+	if (err) {
+		dev_err(pfuze_chip->dev,
+			"failed to register sys-off handler: %d\n", err);
+		return err;
 	}
 
-	syspm_pfuze_chip = pfuze_chip;
-	pm_power_off_prepare = pfuze_power_off_prepare;
-
 	return 0;
 }
 
@@ -837,23 +838,12 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
 	return 0;
 }
 
-static int pfuze100_regulator_remove(struct i2c_client *client)
-{
-	if (syspm_pfuze_chip) {
-		syspm_pfuze_chip = NULL;
-		pm_power_off_prepare = NULL;
-	}
-
-	return 0;
-}
-
 static struct i2c_driver pfuze_driver = {
 	.driver = {
 		.name = "pfuze100-regulator",
 		.of_match_table = pfuze_dt_ids,
 	},
 	.probe = pfuze100_regulator_probe,
-	.remove = pfuze100_regulator_remove,
 };
 module_i2c_driver(pfuze_driver);
 
-- 
2.35.1

[PATCH v7 20/20] reboot: Remove pm_power_off_prepare()

[Dmitry Osipenko]

All pm_power_off_prepare() users were converted to sys-off handler API.
Remove the obsolete callback.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 include/linux/pm.h |  1 -
 kernel/reboot.c    | 11 -----------
 2 files changed, 12 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index ffe941958501..6cdf279c7f2f 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -21,7 +21,6 @@
  * Callbacks for platform drivers to implement.
  */
 extern void (*pm_power_off)(void);
-extern void (*pm_power_off_prepare)(void);
 
 struct device; /* we have a circular dep with device.h */
 #ifdef CONFIG_VT_CONSOLE_SLEEP
diff --git a/kernel/reboot.c b/kernel/reboot.c
index e76e2570dcf5..f2f5c9d7caa0 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -48,13 +48,6 @@ int reboot_cpu;
 enum reboot_type reboot_type = BOOT_ACPI;
 int reboot_force;
 
-/*
- * If set, this is used for preparing the system to power off.
- */
-
-void (*pm_power_off_prepare)(void);
-EXPORT_SYMBOL_GPL(pm_power_off_prepare);
-
 /**
  *	emergency_restart - reboot the system
  *
@@ -877,10 +870,6 @@ void do_kernel_power_off(void)
 
 static void do_kernel_power_off_prepare(void)
 {
-	/* legacy pm_power_off_prepare() is unchained and has highest priority */
-	if (pm_power_off_prepare)
-		return pm_power_off_prepare();
-
 	blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_PREPARE,
 				     NULL);
 }
-- 
2.35.1

Re: [PATCH v7 00/20] Introduce power-off+restart call chain API

[Geert Uytterhoeven]

Hi Dmitry,

On Tue, Apr 12, 2022 at 1:38 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> 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.

> Changelog:
>
> v7: - Rebased on a recent linux-next. Dropped the recently removed
>       NDS32 architecture. Only SH and x86 arches left un-acked.
>
>     - Added acks from Thomas Bogendoerfer and Krzysztof Kozlowski
>       to the MIPS and memory/emif patches respectively.

Looks like you forgot to add the actual acks?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 00/20] Introduce power-off+restart call chain API

[Dmitry Osipenko]

On 4/12/22 10:06, Geert Uytterhoeven wrote:
> Hi Dmitry,
> 
> On Tue, Apr 12, 2022 at 1:38 AM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> 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.
> 
>> Changelog:
>>
>> v7: - Rebased on a recent linux-next. Dropped the recently removed
>>       NDS32 architecture. Only SH and x86 arches left un-acked.
>>
>>     - Added acks from Thomas Bogendoerfer and Krzysztof Kozlowski
>>       to the MIPS and memory/emif patches respectively.
> 
> Looks like you forgot to add the actual acks?

Good catch, thank you! Indeed, I sent out the version without the acks,
but luckily it's only the acks that are missing, the code is fine.

Re: [PATCH v7 16/20] mips: Use do_kernel_power_off()

[Dmitry Osipenko]

On 4/12/22 02:38, 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 <dmitry.osipenko@collabora.com>
> ---
>  arch/mips/kernel/reset.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
> index 6288780b779e..e7ce07b3e79b 100644
> --- a/arch/mips/kernel/reset.c
> +++ b/arch/mips/kernel/reset.c
> @@ -114,8 +114,7 @@ void machine_halt(void)
>  
>  void machine_power_off(void)
>  {
> -	if (pm_power_off)
> -		pm_power_off();
> +	do_kernel_power_off();
>  
>  #ifdef CONFIG_SMP
>  	preempt_disable();

Adding ack from Thomas that he gave to v6. It's missing in v7 by accident.

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

Re: [PATCH v7 17/20] memory: emif: Use kernel_can_power_off()

[Dmitry Osipenko]

On 4/12/22 02:38, 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 <dmitry.osipenko@collabora.com>
> ---
>  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 edf3ba7447ed..fa6845313a43 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");

Adding ack from Krzysztof that he gave to v6. It's missing in v7 by
accident.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Re: [PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority

[Rafael J. Wysocki]

On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> Add sanity check which ensures that there are no two restart handlers
> registered using the same priority. This requirement will become mandatory
> once all drivers will be converted to the new API and such errors will be
> fixed.
>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

The first two patches in the series are fine with me and there's only
one minor nit regarding this one (below).

> ---
>  kernel/reboot.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index ed4e6dfb7d44..acdae4e95061 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
>   */
>  int register_restart_handler(struct notifier_block *nb)
>  {
> +       int ret;
> +
> +       ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb);
> +       if (ret != -EBUSY)
> +               return ret;
> +
> +       /*
> +        * Handler must have unique priority. Otherwise call order is
> +        * determined by registration order, which is unreliable.
> +        *
> +        * This requirement will become mandatory once all drivers
> +        * will be converted to use new sys-off API.
> +        */
> +       pr_err("failed to register restart handler using unique priority\n");

I would use pr_info() here, because this is not a substantial error AFAICS.

> +
>         return atomic_notifier_chain_register(&restart_handler_list, nb);
>  }
>  EXPORT_SYMBOL(register_restart_handler);
> --

Re: [PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority

[Dmitry Osipenko]

On 4/13/22 21:48, Rafael J. Wysocki wrote:
> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>
>> Add sanity check which ensures that there are no two restart handlers
>> registered using the same priority. This requirement will become mandatory
>> once all drivers will be converted to the new API and such errors will be
>> fixed.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> The first two patches in the series are fine with me and there's only
> one minor nit regarding this one (below).
> 
>> ---
>>  kernel/reboot.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>> index ed4e6dfb7d44..acdae4e95061 100644
>> --- a/kernel/reboot.c
>> +++ b/kernel/reboot.c
>> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
>>   */
>>  int register_restart_handler(struct notifier_block *nb)
>>  {
>> +       int ret;
>> +
>> +       ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb);
>> +       if (ret != -EBUSY)
>> +               return ret;
>> +
>> +       /*
>> +        * Handler must have unique priority. Otherwise call order is
>> +        * determined by registration order, which is unreliable.
>> +        *
>> +        * This requirement will become mandatory once all drivers
>> +        * will be converted to use new sys-off API.
>> +        */
>> +       pr_err("failed to register restart handler using unique priority\n");
> 
> I would use pr_info() here, because this is not a substantial error AFAICS.

It's indeed not a substantial error so far, but it will become
substantial later on once only unique priorities will be allowed. The
pr_warn() could be a good compromise here, pr_info() is too mild, IMO.

Re: [PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority

[Rafael J. Wysocki]

On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 4/13/22 21:48, Rafael J. Wysocki wrote:
> > On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> Add sanity check which ensures that there are no two restart handlers
> >> registered using the same priority. This requirement will become mandatory
> >> once all drivers will be converted to the new API and such errors will be
> >> fixed.
> >>
> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >
> > The first two patches in the series are fine with me and there's only
> > one minor nit regarding this one (below).
> >
> >> ---
> >>  kernel/reboot.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/kernel/reboot.c b/kernel/reboot.c
> >> index ed4e6dfb7d44..acdae4e95061 100644
> >> --- a/kernel/reboot.c
> >> +++ b/kernel/reboot.c
> >> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
> >>   */
> >>  int register_restart_handler(struct notifier_block *nb)
> >>  {
> >> +       int ret;
> >> +
> >> +       ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb);
> >> +       if (ret != -EBUSY)
> >> +               return ret;
> >> +
> >> +       /*
> >> +        * Handler must have unique priority. Otherwise call order is
> >> +        * determined by registration order, which is unreliable.
> >> +        *
> >> +        * This requirement will become mandatory once all drivers
> >> +        * will be converted to use new sys-off API.
> >> +        */
> >> +       pr_err("failed to register restart handler using unique priority\n");
> >
> > I would use pr_info() here, because this is not a substantial error AFAICS.
>
> It's indeed not a substantial error so far, but it will become
> substantial later on once only unique priorities will be allowed. The
> pr_warn() could be a good compromise here, pr_info() is too mild, IMO.

Well, I'm still unconvinced about requiring all of the users of this
interface to use unique priorities.

Arguably, there are some of them who don't really care about the
ordering, so could there be an option for them to specify the lack of
care by, say, passing 0 as the priority that would be regarded as a
special case?

IOW, if you pass 0, you'll be run along the others who've also passed
0, but if you pass anything different from 0, it must be unique.  What
do you think?

Re: [PATCH v7 00/20] Introduce power-off+restart call chain API

[Michał Mirosław]

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 <mirq-linux@rere.qmqm.pl>

Re: [PATCH v7 04/20] kernel: Add combined power-off+restart handler call chain API

[Rafael J. Wysocki]

Honestly, I would prefer this to be split so as to make it easier to
review if nothing else.

On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> 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.

Which means that the existing notifier chains for system restart are
used as they are without modifications.

At least that's what follows from the code and it would be good to
mention it here.

Moreover, a new notifier chain is introduced for the power-off case
and it appears to be the counterpart of the restart_handler_list
chain, but then why is it blocking and not atomic like the latter?

> 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.

"the corresponding"

>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  include/linux/reboot.h   | 229 ++++++++++++++-
>  kernel/power/hibernate.c |   2 +-
>  kernel/reboot.c          | 604 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 827 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> index a2429648d831..ba5e5dddcfcd 100644
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -8,10 +8,35 @@
>
>  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 */
> +};
> +
> +/*
> + * Standard restart priority levels. Intended to be set in the
> + * sys_off_handler.restart_priority field.
> + *
> + * Use `RESTART_PRIO_ABC +- prio` style for additional levels.
> + *
> + * RESTART_PRIO_RESERVED:      Falls back to RESTART_PRIO_DEFAULT.
> + *                             Drivers may leave priority initialized
> + *                             to zero, to auto-set it to the default level.

What is the "default level" here?

> + *
> + * RESTART_PRIO_LOW:           Use this for handler of last resort.
> + *
> + * RESTART_PRIO_DEFAULT:       Use this for default/generic handler.
> + *
> + * RESTART_PRIO_HIGH:          Use this if you have multiple handlers and
> + *                             this handler has higher priority than the
> + *                             default handler.
> + */
> +#define RESTART_PRIO_RESERVED          0
> +#define RESTART_PRIO_LOW               8
> +#define RESTART_PRIO_DEFAULT           128
> +#define RESTART_PRIO_HIGH              192
>
>  enum reboot_mode {
>         REBOOT_UNDEFINED = -1,
> @@ -49,6 +74,201 @@ extern int register_restart_handler(struct notifier_block *);
>  extern int unregister_restart_handler(struct notifier_block *);
>  extern void do_kernel_restart(char *cmd);
>
> +/*
> + * System power-off and restart API.
> + */
> +
> +/*
> + * Standard power-off priority levels. Intended to be set in the
> + * sys_off_handler.power_off_priority field.
> + *
> + * Use `POWEROFF_PRIO_ABC +- prio` style for additional levels.

What exactly does this mean?

> + *
> + * POWEROFF_PRIO_RESERVED:     Falls back to POWEROFF_PRIO_DEFAULT.
> + *                             Drivers may leave priority initialized
> + *                             to zero, to auto-set it to the default level.
> + *
> + * POWEROFF_PRIO_PLATFORM:     Intended to be used by platform-level handler.
> + *                             Has lowest priority since device drivers are
> + *                             expected to take over platform handler which
> + *                             doesn't allow further callback chaining.
> + *
> + * POWEROFF_PRIO_DEFAULT:      Use this for default/generic handler.
> + *
> + * POWEROFF_PRIO_FIRMWARE:     Use this if handler uses firmware call.
> + *                             Has highest priority since firmware is expected
> + *                             to know best how to power-off hardware properly.
> + */
> +#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
> +
> +enum poweroff_mode {
> +       POWEROFF_NORMAL = 0,

Why not just POWEROFF?

> +       POWEROFF_PREPARE,
> +};
> +
> +/**
> + * 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 does this need to be a separate data type?

> +
> +/**
> + * 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.
> + * @mode: Preparation mode ID.
> + */
> +struct reboot_prep_data {
> +       void *cb_data;
> +       const char *cmd;
> +       bool stop_chain;
> +       enum reboot_prepare_mode mode;
> +};

There seem to be some duplicate data items between struct restart_data
and struct reboot_prep_data, so what's the reason why they are
separate?

> +
> +struct sys_off_handler_private_data {
> +       struct notifier_block power_off_nb;
> +       struct notifier_block restart_nb;
> +       struct notifier_block reboot_nb;

So restart_nb is going to be added to restart_handler_list, eboot_nb
will be added to reboot_notifier_list (which both exist already) and
power_off_nb will be added to the new power_off_handler_list, right?

Of course, this means that reboot_nb will be used in
kernel_restart_prepare() and kernel_shutdown_prepare(), so the
corresponding callback will be invoked in both the restart and
power-off cases.

It would be good to document that somehow.

> +       void (*platform_power_off_cb)(void);
> +       void (*simple_power_off_cb)(void *data);
> +       void *simple_power_off_cb_data;

Is there any particular reason to put these callbacks here and not
directly into struct sys_off_handler?

> +       bool registered;
> +};
> +
> +/**
> + * struct sys_off_handler - System power-off and restart handler
> + *
> + * @cb_data: Pointer to user's data.
> + *
> + * @power_off_cb: Callback that powers off this machine. Inactive if NULL.
> + *
> + * @power_off_prepare_cb: Power-off preparation callback. All power-off
> + * preparation callbacks are invoked after @reboot_prepare_cb and before
> + * @power_off_cb. Inactive if NULL.
> + *
> + * @power_off_priority: Power-off callback priority, must be unique.
> + * Zero value is reserved and auto-reassigned to POWEROFF_PRIO_DEFAULT.
> + * Inactive if @power_off_cb is NULL.
> + *
> + * @power_off_chaining_allowed: Set to false if callback's execution should
> + * stop when @power_off_cb fails to power off this machine. True if further
> + * lower priority power-off callback should be executed. False is default
> + * value.
> + *
> + * @restart_cb: Callback that reboots this machine. Inactive if NULL.
> + *
> + * @restart_priority: Restart callback priority, must be unique. Zero value
> + * is reserved and auto-reassigned to RESTART_PRIO_DEFAULT. Inactive if
> + * @restart_cb is NULL.
> + *
> + * @restart_chaining_disallowed: Set to true if callback's execution should
> + * stop when @restart_cb fails to restart this machine. False if further
> + * lower priority restart callback should be executed. False is default
> + * value.
> + *
> + * @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.
> + *
> + * @reboot_priority: Reboot/shutdown preparation callback priority, doesn't
> + * need to be unique. Zero is default value. Inactive if @reboot_prepare_cb
> + * is NULL.

It is unclear that the ->reboot_prepare_cb() callback is going to be
used for both restart and power-off and reboot_priority is about the
preparation phase only.

And in the preparation phase the priority may not matter that much,
because there are users who don't care about the ordering as long as
their stuff is called at all.

Honestly, I would change the naming here, because what it is is quite
confusing at least to me.  Especially that "restart" and "reboot" seem
to be used interchangeably in the comments.

> + *
> + * @priv: Internal data. Shouldn't be touched.
> + *
> + * 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.
> + *
> + * Struct sys_off_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_sys_off_handler().
> + */
> +struct sys_off_handler {
> +       void *cb_data;
> +
> +       void (*power_off_cb)(struct power_off_data *data);
> +       void (*power_off_prepare_cb)(struct power_off_prep_data *data);
> +       int power_off_priority;
> +       bool power_off_chaining_allowed;
> +
> +       void (*restart_cb)(struct restart_data *data);
> +       int restart_priority;
> +       bool restart_chaining_disallowed;
> +
> +       void (*reboot_prepare_cb)(struct reboot_prep_data *data);
> +       int reboot_priority;
> +
> +       const struct sys_off_handler_private_data priv;

Why is it const?  Because of the callbacks in there?

Doesn't this mean that all struct sys_off_handler need to be static?

> +};
> +
> +int register_sys_off_handler(struct sys_off_handler *handler);
> +int unregister_sys_off_handler(struct sys_off_handler *handler);
> +
> +int devm_register_sys_off_handler(struct device *dev,
> +                                 struct sys_off_handler *handler);
> +
> +int devm_register_prioritized_power_off_handler(struct device *dev,
> +                                               int priority,
> +                                               void (*callback)(void *data),
> +                                               void *cb_data);
> +
> +int devm_register_simple_power_off_handler(struct device *dev,
> +                                          void (*callback)(void *data),
> +                                          void *cb_data);
> +
> +int register_platform_power_off(void (*power_off)(void));
> +int unregister_platform_power_off(void (*power_off)(void));
> +
> +int devm_register_prioritized_restart_handler(struct device *dev,
> +                                             int priority,
> +                                             void (*callback)(struct restart_data *data),
> +                                             void *cb_data);
> +
> +int devm_register_simple_restart_handler(struct device *dev,
> +                                        void (*callback)(struct restart_data *data),
> +                                        void *cb_data);
> +
> +void do_kernel_power_off(void);
> +
>  /*
>   * Architecture-specific implementations of sys_reboot commands.
>   */
> @@ -70,6 +290,7 @@ extern void kernel_restart_prepare(char *cmd);
>  extern void kernel_restart(char *cmd);
>  extern void kernel_halt(void);
>  extern void kernel_power_off(void);
> +extern bool kernel_can_power_off(void);
>
>  void ctrl_alt_del(void);
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 938d5c78b421..a9464b4a3209 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -665,7 +665,7 @@ static void power_down(void)
>                 hibernation_platform_enter();
>                 fallthrough;
>         case HIBERNATION_SHUTDOWN:
> -               if (pm_power_off)
> +               if (kernel_can_power_off())
>                         kernel_power_off();
>                 break;
>         }
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index acdae4e95061..e76e2570dcf5 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -296,6 +296,595 @@ void kernel_halt(void)
>  }
>  EXPORT_SYMBOL_GPL(kernel_halt);
>
> +/*
> + *     Notifier list for kernel code which wants to be called
> + *     to power off the system.
> + */
> +static BLOCKING_NOTIFIER_HEAD(power_off_handler_list);
> +
> +/*
> + * Temporary stub that prevents linkage failure while we're in process
> + * of removing all uses of legacy pm_power_off() around the kernel.

Nit: inconsistent comment formatting.

> + */
> +void __weak (*pm_power_off)(void);
> +
> +static void dummy_pm_power_off(void)
> +{
> +       /* temporary stub until pm_power_off() is gone, see more below */
> +}
> +
> +static struct notifier_block *pm_power_off_nb;
> +
> +/**
> + *     register_power_off_handler - Register function to be called to power off
> + *                                  the system
> + *     @nb: Info about handler function to be called
> + *     @nb->priority:  Handler priority. Handlers should follow the
> + *                     following guidelines for setting priorities.
> + *                     0:      Reserved
> + *                     1:      Power-off handler of last resort,
> + *                             with limited power-off capabilities
> + *                     128:    Default power-off handler; use if no other
> + *                             power-off handler is expected to be available,
> + *                             and/or if power-off functionality is
> + *                             sufficient to power-off the entire system
> + *                     255:    Highest priority power-off handler, will
> + *                             preempt all other power-off handlers
> + *
> + *     Registers a function with code to be called to power off the
> + *     system.

Because this is only used internally in this file, I'd say what it
does directly, that is "Add a notifier to the power-off chain used for
powering off the system".

> + *
> + *     Registered functions will be called as last step of the power-off
> + *     sequence.
> + *
> + *     Registered functions are expected to power off the system immediately.
> + *     If more than one function is registered, the power-off handler priority
> + *     selects which function will be called first.
> + *
> + *     Power-off handlers are expected to be registered from non-architecture
> + *     code, typically from drivers. A typical use case would be a system
> + *     where power-off functionality is provided through a PMIC. Multiple
> + *     power-off handlers may exist; for example, one power-off handler might
> + *     turn off the entire system, while another only turns off part of
> + *     system. In such cases, the power-off handler which only disables part
> + *     of the hardware is expected to register with low priority to ensure
> + *     that it only runs if no other means to power off the system is
> + *     available.

I would move the above 3 paragraphs to the description comment for
power_off_handler_list.

> + *
> + *     Currently always returns zero, as blocking_notifier_chain_register()
> + *     always returns zero.
> + */
> +static int register_power_off_handler(struct notifier_block *nb)
> +{
> +       int ret;
> +
> +       ret = blocking_notifier_chain_register_unique_prio(&power_off_handler_list, nb);
> +       if (ret && ret != -EBUSY)
> +               return ret;
> +
> +       if (!ret)
> +               goto set_pm_power_off;
> +
> +       /*
> +        * Handler must have unique priority. Otherwise call order is
> +        * determined by registration order, which is unreliable.
> +        *
> +        * This requirement will become mandatory once all drivers
> +        * will be converted to use new sys-off API.
> +        */
> +       pr_err("failed to register power-off handler using unique priority\n");
> +
> +       ret = blocking_notifier_chain_register(&power_off_handler_list, nb);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * Some drivers check whether pm_power_off was already installed.
> +        * Install dummy callback using new API to preserve old behaviour
> +        * for those drivers during period of transition to the new API.
> +        */
> +set_pm_power_off:
> +       if (!pm_power_off) {
> +               pm_power_off = dummy_pm_power_off;
> +               pm_power_off_nb = nb;
> +       }
> +
> +       return 0;
> +}
> +
> +static int unregister_power_off_handler(struct notifier_block *nb)
> +{
> +       if (nb == pm_power_off_nb) {
> +               if (pm_power_off == dummy_pm_power_off)
> +                       pm_power_off = NULL;
> +
> +               pm_power_off_nb = NULL;
> +       }
> +
> +       return blocking_notifier_chain_unregister(&power_off_handler_list, nb);
> +}
> +
> +static void devm_unregister_power_off_handler(void *data)
> +{
> +       struct notifier_block *nb = data;
> +
> +       unregister_power_off_handler(nb);
> +}
> +
> +static int devm_register_power_off_handler(struct device *dev,
> +                                          struct notifier_block *nb)
> +{
> +       int err;
> +
> +       err = register_power_off_handler(nb);
> +       if (err)
> +               return err;
> +
> +       return devm_add_action_or_reset(dev, devm_unregister_power_off_handler,
> +                                       nb);
> +}
> +
> +static int sys_off_handler_power_off(struct notifier_block *nb,
> +                                    unsigned long mode, void *unused)
> +{
> +       struct power_off_prep_data prep_data = {};
> +       struct sys_off_handler_private_data *priv;
> +       struct power_off_data data = {};
> +       struct sys_off_handler *h;
> +       int ret = NOTIFY_DONE;
> +
> +       priv = container_of(nb, struct sys_off_handler_private_data, power_off_nb);
> +       h = container_of(priv, struct sys_off_handler, priv);
> +       prep_data.cb_data = h->cb_data;
> +       data.cb_data = h->cb_data;
> +
> +       switch (mode) {
> +       case POWEROFF_NORMAL:
> +               if (h->power_off_cb)
> +                       h->power_off_cb(&data);
> +
> +               if (priv->simple_power_off_cb)
> +                       priv->simple_power_off_cb(priv->simple_power_off_cb_data);
> +
> +               if (priv->platform_power_off_cb)
> +                       priv->platform_power_off_cb();

The invocation of the priv callbacks here confuses me quite a bit.

Can you please at least add a comment explaining this?

> +
> +               if (!h->power_off_chaining_allowed)
> +                       ret = NOTIFY_STOP;
> +
> +               break;
> +
> +       case POWEROFF_PREPARE:
> +               if (h->power_off_prepare_cb)
> +                       h->power_off_prepare_cb(&prep_data);
> +
> +               break;
> +
> +       default:
> +               unreachable();
> +       }
> +
> +       return ret;
> +}
> +
> +static int sys_off_handler_restart(struct notifier_block *nb,
> +                                  unsigned long mode, void *cmd)
> +{
> +       struct sys_off_handler_private_data *priv;
> +       struct restart_data data = {};
> +       struct sys_off_handler *h;
> +
> +       priv = container_of(nb, struct sys_off_handler_private_data, restart_nb);
> +       h = container_of(priv, struct sys_off_handler, priv);
> +
> +       data.stop_chain = h->restart_chaining_disallowed;
> +       data.cb_data = h->cb_data;
> +       data.mode = mode;
> +       data.cmd = cmd;
> +
> +       h->restart_cb(&data);

Wouldn't it be a bit more straightforward to allow ->restart_cb() to
return a value being either NOTIFY_STOP or NOTIFY_DONE?

> +
> +       return data.stop_chain ? NOTIFY_STOP : NOTIFY_DONE;

And I would prefer

if (data.stop_chain)
        return NOTIFY_STOP;

return NOTIFY_DONE;

> +}
> +
> +static int sys_off_handler_reboot(struct notifier_block *nb,
> +                                 unsigned long mode, void *cmd)
> +{
> +       struct sys_off_handler_private_data *priv;
> +       struct reboot_prep_data data = {};
> +       struct sys_off_handler *h;
> +
> +       priv = container_of(nb, struct sys_off_handler_private_data, reboot_nb);
> +       h = container_of(priv, struct sys_off_handler, priv);
> +
> +       data.cb_data = h->cb_data;
> +       data.stop_chain = false;
> +       data.mode = mode;
> +       data.cmd = cmd;
> +
> +       h->reboot_prepare_cb(&data);
> +
> +       return data.stop_chain ? NOTIFY_STOP : NOTIFY_DONE;

And analogously here.

> +}
> +
> +static struct sys_off_handler_private_data *
> +sys_off_handler_private_data(struct sys_off_handler *handler)
> +{
> +       return (struct sys_off_handler_private_data *)&handler->priv;

Is the cast needed to avoid a warning about "const"?

> +}
> +
> +/**
> + *     devm_register_sys_off_handler - Register system power-off/restart handler

register_sys_off_handler

> + *     @dev: Device that registers handler
> + *     @handler: System-off handler
> + *
> + *     Registers handler that will be called as last step of the power-off
> + *     and restart sequences.

Not necessarily as the last step, because there may be other system
power-off/restart handlers called after it.

I would just say "at the end of the power-off and restart sequences".

Moreover, it registers the "reboot_cb" part that is not called "at the
end" even.

Also, because this is the function that will be used by drivers etc to
register handlers, I would give some more information on how the
object registered by it is going to be used to the prospective users.

> + *
> + *     Returns zero on success, or error code on failure.
> + */
> +int register_sys_off_handler(struct sys_off_handler *handler)
> +{
> +       struct sys_off_handler_private_data *priv;
> +       int err, priority;
> +
> +       priv = sys_off_handler_private_data(handler);
> +
> +       /* sanity-check whether handler is registered twice */
> +       if (priv->registered)
> +               return -EBUSY;
> +
> +       if (handler->power_off_cb || handler->power_off_prepare_cb) {
> +               if (handler->power_off_priority == POWEROFF_PRIO_RESERVED)
> +                       priority = POWEROFF_PRIO_DEFAULT;

I'm not sure that this helps.

I mean, why can't the users of this new API pass POWEROFF_PRIO_DEFAULT
directly if they want "default"?

> +               else
> +                       priority = handler->power_off_priority;
> +
> +               priv->power_off_nb.notifier_call = sys_off_handler_power_off;
> +               priv->power_off_nb.priority = priority;
> +
> +               err = register_power_off_handler(&priv->power_off_nb);
> +               if (err)
> +                       goto reset_sys_off_handler;
> +       }
> +
> +       if (handler->restart_cb) {
> +               if (handler->restart_priority == RESTART_PRIO_RESERVED)
> +                       priority = RESTART_PRIO_DEFAULT;
> +               else
> +                       priority = handler->restart_priority;
> +
> +               priv->restart_nb.notifier_call = sys_off_handler_restart;
> +               priv->restart_nb.priority = priority;
> +
> +               err = register_restart_handler(&priv->restart_nb);
> +               if (err)
> +                       goto unreg_power_off_handler;
> +       }
> +
> +       if (handler->reboot_prepare_cb) {
> +               priv->reboot_nb.notifier_call = sys_off_handler_reboot;
> +               priv->reboot_nb.priority = handler->reboot_priority;
> +
> +               err = register_reboot_notifier(&priv->reboot_nb);
> +               if (err)
> +                       goto unreg_restart_handler;
> +       }
> +
> +       priv->registered = true;
> +
> +       return 0;
> +
> +unreg_restart_handler:
> +       if (handler->restart_cb)
> +               unregister_restart_handler(&priv->restart_nb);
> +
> +unreg_power_off_handler:
> +       if (handler->power_off_cb)
> +               unregister_power_off_handler(&priv->power_off_nb);
> +
> +reset_sys_off_handler:
> +       memset(priv, 0, sizeof(*priv));
> +
> +       return err;
> +}
> +EXPORT_SYMBOL(register_sys_off_handler);
> +
> +/**
> + *     unregister_sys_off_handler - Unregister system power-off/restart handler
> + *     @handler: System-off handler
> + *
> + *     Unregisters sys-off handler. Does nothing and returns zero if handler
> + *     is NULL.
> + *
> + *     Returns zero on success, or error code on failure.
> + */
> +int unregister_sys_off_handler(struct sys_off_handler *handler)
> +{
> +       struct sys_off_handler_private_data *priv;
> +
> +       if (!handler)
> +               return 0;
> +
> +       priv = sys_off_handler_private_data(handler);
> +
> +       /* sanity-check whether handler is unregistered twice */
> +       if (!priv->registered)
> +               return -EINVAL;
> +
> +       if (handler->reboot_prepare_cb)
> +               unregister_reboot_notifier(&priv->reboot_nb);
> +
> +       if (handler->restart_cb)
> +               unregister_restart_handler(&priv->restart_nb);
> +
> +       if (handler->power_off_cb)
> +               unregister_power_off_handler(&priv->power_off_nb);
> +
> +       memset(priv, 0, sizeof(*priv));
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(unregister_sys_off_handler);
> +
> +static void devm_unregister_sys_off_handler(void *data)
> +{
> +       struct sys_off_handler *handler = data;
> +
> +       unregister_sys_off_handler(handler);
> +}
> +
> +/**
> + *     devm_register_sys_off_handler - Register system power-off/restart handler
> + *     @dev: Device that registers handler
> + *     @handler: System-off handler
> + *
> + *     Resource-managed variant of register_sys_off_handler().
> + *
> + *     Returns zero on success, or error code on failure.
> + */
> +int devm_register_sys_off_handler(struct device *dev,
> +                                 struct sys_off_handler *handler)
> +{
> +       int err;
> +
> +       err = register_sys_off_handler(handler);
> +       if (err)
> +               return err;
> +
> +       return devm_add_action_or_reset(dev, devm_unregister_sys_off_handler,
> +                                       handler);
> +}
> +EXPORT_SYMBOL(devm_register_sys_off_handler);
> +
> +/**
> + *     devm_register_prioritized_power_off_handler - Register prioritized power-off callback
> + *     @dev: Device that registers callback
> + *     @priority: Callback's priority
> + *     @callback: Callback function
> + *     @cb_data: Callback's argument
> + *
> + *     Registers resource-managed power-off callback with a given priority.
> + *     It will be called as last step of the power-off sequence. Callbacks
> + *     chaining is disabled, i.e. further lower priority callbacks won't
> + *     be executed if this @callback will fail to execute.
> + *
> + *     Returns zero on success, or error code on failure.

What's the case in which this should be used instead of registering a
full sys_off handler?

> + */
> +int devm_register_prioritized_power_off_handler(struct device *dev,
> +                                               int priority,
> +                                               void (*callback)(void *data),
> +                                               void *cb_data)
> +{
> +       struct sys_off_handler_private_data *priv;
> +       struct sys_off_handler *handler;
> +
> +       handler = devm_kzalloc(dev, sizeof(*handler), GFP_KERNEL);
> +       if (!handler)
> +               return -ENOMEM;
> +
> +       if (priority == POWEROFF_PRIO_RESERVED)
> +               priority = POWEROFF_PRIO_DEFAULT;
> +
> +       priv = sys_off_handler_private_data(handler);
> +
> +       priv->power_off_nb.notifier_call = sys_off_handler_power_off;
> +       priv->power_off_nb.priority = priority;
> +       priv->simple_power_off_cb_data = cb_data;
> +       priv->simple_power_off_cb = callback;
> +
> +       return devm_register_power_off_handler(dev, &priv->power_off_nb);
> +}
> +EXPORT_SYMBOL(devm_register_prioritized_power_off_handler);
> +
> +int devm_register_prioritized_power_off_handler(struct device *dev,
> +                                               int priority,
> +                                               void (*callback)(void *data),
> +                                               void *cb_data);
> +
> +/**
> + *     devm_register_simple_power_off_handler - Register simple power-off callback
> + *     @dev: Device that registers callback
> + *     @callback: Callback function
> + *     @cb_data: Callback's argument
> + *
> + *     Registers resource-managed power-off callback with default priority.
> + *     It will be invoked as last step of the power-off sequence. Further
> + *     lower priority callbacks won't be executed if this @callback fails.
> + *
> + *     Returns zero on success, or error code on failure.

And here?

> + */
> +int devm_register_simple_power_off_handler(struct device *dev,
> +                                          void (*callback)(void *data),
> +                                          void *cb_data)
> +{
> +       return devm_register_prioritized_power_off_handler(dev,
> +                                                          POWEROFF_PRIO_DEFAULT,
> +                                                          callback, cb_data);
> +}
> +EXPORT_SYMBOL(devm_register_simple_power_off_handler);
> +
> +/**
> + *     devm_register_prioritized_restart_handler - Register prioritized restart callback
> + *     @dev: Device that registers callback
> + *     @priority: Callback's priority
> + *     @callback: Callback function
> + *     @cb_data: Callback's argument
> + *
> + *     Registers resource-managed restart callback with a given priority.
> + *     It will be called as a part of the restart sequence. Callbacks
> + *     chaining is disabled, i.e. further lower priority callbacks won't
> + *     be executed if this @callback will fail to execute.
> + *
> + *     Returns zero on success, or error code on failure.

And here?

> + */
> +int devm_register_prioritized_restart_handler(struct device *dev,
> +                                             int priority,
> +                                             void (*callback)(struct restart_data *data),
> +                                             void *cb_data)
> +{
> +       struct sys_off_handler *handler;
> +
> +       handler = devm_kzalloc(dev, sizeof(*handler), GFP_KERNEL);
> +       if (!handler)
> +               return -ENOMEM;
> +
> +       if (priority == RESTART_PRIO_RESERVED)
> +               priority = RESTART_PRIO_DEFAULT;
> +
> +       handler->restart_priority = priority;
> +       handler->restart_cb = callback;
> +       handler->cb_data = cb_data;
> +
> +       return devm_register_sys_off_handler(dev, handler);
> +}
> +EXPORT_SYMBOL(devm_register_prioritized_restart_handler);
> +
> +/**
> + *     devm_register_simple_restart_handler - Register simple restart callback
> + *     @dev: Device that registers callback
> + *     @callback: Callback function
> + *     @cb_data: Callback's argument
> + *
> + *     Registers resource-managed restart callback with default priority.
> + *     It will be invoked as a part of the restart sequence. Further
> + *     lower priority callback will be executed if this @callback fails.
> + *
> + *     Returns zero on success, or error code on failure.

And here?

> + */
> +int devm_register_simple_restart_handler(struct device *dev,
> +                                        void (*callback)(struct restart_data *data),
> +                                        void *cb_data)
> +{
> +       return devm_register_prioritized_restart_handler(dev,
> +                                                        RESTART_PRIO_DEFAULT,
> +                                                        callback, cb_data);
> +}
> +EXPORT_SYMBOL(devm_register_simple_restart_handler);
> +
> +static struct sys_off_handler platform_power_off_handler = {
> +       .priv = {
> +               .power_off_nb = {
> +                       .notifier_call = sys_off_handler_power_off,
> +                       .priority = POWEROFF_PRIO_PLATFORM,
> +               },
> +       },
> +};
> +
> +static DEFINE_SPINLOCK(platform_power_off_lock);
> +
> +/**
> + *     register_platform_power_off - Register platform-level power-off callback
> + *     @power_off: Power-off callback
> + *
> + *     Registers power-off callback that will be called as last step
> + *     of the power-off sequence. This callback is expected to be invoked
> + *     for the last resort. Further lower priority callbacks won't be
> + *     executed if @power_off fails. Only one platform power-off callback
> + *     is allowed to be registered at a time.
> + *
> + *     Returns zero on success, or error code on failure.

What's the use case for this?

> + */
> +int register_platform_power_off(void (*power_off)(void))
> +{
> +       struct sys_off_handler_private_data *priv;
> +       int ret = 0;
> +
> +       priv = sys_off_handler_private_data(&platform_power_off_handler);
> +
> +       spin_lock(&platform_power_off_lock);
> +       if (priv->platform_power_off_cb)
> +               ret = -EBUSY;
> +       else
> +               priv->platform_power_off_cb = power_off;

Wasn't priv supposed to be const?

> +       spin_unlock(&platform_power_off_lock);
> +
> +       if (ret)
> +               return ret;
> +
> +       ret = register_power_off_handler(&priv->power_off_nb);
> +       if (ret)
> +               priv->platform_power_off_cb = NULL;
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(register_platform_power_off);
> +
> +/**
> + *     unregister_platform_power_off - Unregister platform-level power-off callback
> + *     @power_off: Power-off callback
> + *
> + *     Unregisters previously registered platform power-off callback.
> + *
> + *     Returns zero on success, or error code on failure.
> + */
> +int unregister_platform_power_off(void (*power_off)(void))
> +{
> +       struct sys_off_handler_private_data *priv;
> +       int ret;
> +
> +       priv = sys_off_handler_private_data(&platform_power_off_handler);
> +
> +       if (priv->platform_power_off_cb != power_off)
> +               return -EINVAL;
> +
> +       ret = unregister_power_off_handler(&priv->power_off_nb);
> +       priv->platform_power_off_cb = NULL;
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(unregister_platform_power_off);
> +
> +/**
> + *     do_kernel_power_off - Execute kernel power-off handler call chain
> + *
> + *     Calls functions registered with register_power_off_handler.
> + *
> + *     Expected to be called as last step of the power-off sequence.
> + *
> + *     Powers off the system immediately if a power-off handler function has
> + *     been registered. Otherwise does nothing.
> + */
> +void do_kernel_power_off(void)
> +{
> +       /* legacy pm_power_off() is unchained and has highest priority */
> +       if (pm_power_off && pm_power_off != dummy_pm_power_off)
> +               return pm_power_off();
> +
> +       blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_NORMAL,
> +                                    NULL);
> +}
> +
> +static void do_kernel_power_off_prepare(void)
> +{
> +       /* legacy pm_power_off_prepare() is unchained and has highest priority */
> +       if (pm_power_off_prepare)
> +               return pm_power_off_prepare();
> +
> +       blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_PREPARE,
> +                                    NULL);
> +}
> +
>  /**
>   *     kernel_power_off - power_off the system
>   *
> @@ -304,8 +893,7 @@ EXPORT_SYMBOL_GPL(kernel_halt);
>  void kernel_power_off(void)
>  {
>         kernel_shutdown_prepare(SYSTEM_POWER_OFF);
> -       if (pm_power_off_prepare)
> -               pm_power_off_prepare();
> +       do_kernel_power_off_prepare();
>         migrate_to_reboot_cpu();
>         syscore_shutdown();
>         pr_emerg("Power down\n");
> @@ -314,6 +902,16 @@ void kernel_power_off(void)
>  }
>  EXPORT_SYMBOL_GPL(kernel_power_off);
>
> +bool kernel_can_power_off(void)
> +{
> +       if (!pm_power_off &&
> +           blocking_notifier_call_chain_is_empty(&power_off_handler_list))
> +               return false;
> +
> +       return true;

return pm_power_off ||
blocking_notifier_call_chain_is_empty(&power_off_handler_list);

> +}
> +EXPORT_SYMBOL_GPL(kernel_can_power_off);
> +
>  DEFINE_MUTEX(system_transition_mutex);
>
>  /*
> @@ -355,7 +953,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
>         /* Instead of trying to make the power_off code look like
>          * halt when pm_power_off is not set do it the easy way.
>          */
> -       if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
> +       if (cmd == LINUX_REBOOT_CMD_POWER_OFF && !kernel_can_power_off())
>                 cmd = LINUX_REBOOT_CMD_HALT;
>
>         mutex_lock(&system_transition_mutex);
> --

Re: [PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority

[Dmitry Osipenko]

On 4/14/22 14:19, Rafael J. Wysocki wrote:
> On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>
>> On 4/13/22 21:48, Rafael J. Wysocki wrote:
>>> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko
>>> <dmitry.osipenko@collabora.com> wrote:
>>>>
>>>> Add sanity check which ensures that there are no two restart handlers
>>>> registered using the same priority. This requirement will become mandatory
>>>> once all drivers will be converted to the new API and such errors will be
>>>> fixed.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>
>>> The first two patches in the series are fine with me and there's only
>>> one minor nit regarding this one (below).
>>>
>>>> ---
>>>>  kernel/reboot.c | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>>>> index ed4e6dfb7d44..acdae4e95061 100644
>>>> --- a/kernel/reboot.c
>>>> +++ b/kernel/reboot.c
>>>> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
>>>>   */
>>>>  int register_restart_handler(struct notifier_block *nb)
>>>>  {
>>>> +       int ret;
>>>> +
>>>> +       ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb);
>>>> +       if (ret != -EBUSY)
>>>> +               return ret;
>>>> +
>>>> +       /*
>>>> +        * Handler must have unique priority. Otherwise call order is
>>>> +        * determined by registration order, which is unreliable.
>>>> +        *
>>>> +        * This requirement will become mandatory once all drivers
>>>> +        * will be converted to use new sys-off API.
>>>> +        */
>>>> +       pr_err("failed to register restart handler using unique priority\n");
>>>
>>> I would use pr_info() here, because this is not a substantial error AFAICS.
>>
>> It's indeed not a substantial error so far, but it will become
>> substantial later on once only unique priorities will be allowed. The
>> pr_warn() could be a good compromise here, pr_info() is too mild, IMO.
> 
> Well, I'm still unconvinced about requiring all of the users of this
> interface to use unique priorities.
> 
> Arguably, there are some of them who don't really care about the
> ordering, so could there be an option for them to specify the lack of
> care by, say, passing 0 as the priority that would be regarded as a
> special case?
> 
> IOW, if you pass 0, you'll be run along the others who've also passed
> 0, but if you pass anything different from 0, it must be unique.  What
> do you think?

There are indeed cases where ordering is unimportant. Like a case of
PMIC and watchdog restart handlers for example, both handlers will
produce equal effect from a user's perspective. Perhaps indeed it's more
practical to have at least one shared level.

In this patchset the level 0 is specified as an alias to the default
level 128. If one user registers handler using unique level 128 and the
other user uses non-unique level 0, then we have ambiguity.

One potential option is to make the whole default level 128 non-unique.
This will allow users to not care about the uniqueness by default like
they always did it previously, but it will hide potential problems for
users who actually need unique level and don't know about it yet due to
a lucky registration ordering that they have today. Are you okay with
this option?

Re: [PATCH v7 04/20] kernel: Add combined power-off+restart handler call chain API

[Dmitry Osipenko]

On 4/15/22 21:14, Rafael J. Wysocki wrote:
> Honestly, I would prefer this to be split so as to make it easier to
> review if nothing else.

I'll try to split it in v8.

> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> 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.
> 
> Which means that the existing notifier chains for system restart are
> used as they are without modifications.
> 
> At least that's what follows from the code and it would be good to
> mention it here.

Will improve the commit message.

> Moreover, a new notifier chain is introduced for the power-off case
> and it appears to be the counterpart of the restart_handler_list
> chain, but then why is it blocking and not atomic like the latter?

Good catch, it probably indeed should be atomic because shutting down
could run with a disabled interrupts. I'll invistigate this more for v8,
at least right now I don't recall any particular reason for using the
blocking notifier.

>> 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.
> 
> "the corresponding"

Thanks

>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>  include/linux/reboot.h   | 229 ++++++++++++++-
>>  kernel/power/hibernate.c |   2 +-
>>  kernel/reboot.c          | 604 ++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 827 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
>> index a2429648d831..ba5e5dddcfcd 100644
>> --- a/include/linux/reboot.h
>> +++ b/include/linux/reboot.h
>> @@ -8,10 +8,35 @@
>>
>>  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 */
>> +};
>> +
>> +/*
>> + * Standard restart priority levels. Intended to be set in the
>> + * sys_off_handler.restart_priority field.
>> + *
>> + * Use `RESTART_PRIO_ABC +- prio` style for additional levels.
>> + *
>> + * RESTART_PRIO_RESERVED:      Falls back to RESTART_PRIO_DEFAULT.
>> + *                             Drivers may leave priority initialized
>> + *                             to zero, to auto-set it to the default level.
> 
> What is the "default level" here?

"default level" = RESTART_PRIO_DEFAULT = 128

I'll remove the second sentence about the "default level", for clarity.

>> + *
>> + * RESTART_PRIO_LOW:           Use this for handler of last resort.
>> + *
>> + * RESTART_PRIO_DEFAULT:       Use this for default/generic handler.
>> + *
>> + * RESTART_PRIO_HIGH:          Use this if you have multiple handlers and
>> + *                             this handler has higher priority than the
>> + *                             default handler.
>> + */
>> +#define RESTART_PRIO_RESERVED          0
>> +#define RESTART_PRIO_LOW               8
>> +#define RESTART_PRIO_DEFAULT           128
>> +#define RESTART_PRIO_HIGH              192
>>
>>  enum reboot_mode {
>>         REBOOT_UNDEFINED = -1,
>> @@ -49,6 +74,201 @@ extern int register_restart_handler(struct notifier_block *);
>>  extern int unregister_restart_handler(struct notifier_block *);
>>  extern void do_kernel_restart(char *cmd);
>>
>> +/*
>> + * System power-off and restart API.
>> + */
>> +
>> +/*
>> + * Standard power-off priority levels. Intended to be set in the
>> + * sys_off_handler.power_off_priority field.
>> + *
>> + * Use `POWEROFF_PRIO_ABC +- prio` style for additional levels.
> 
> What exactly does this mean?

"POWEROFF_PRIO_DEFAULT+1 or POWEROFF_PRIO_DEFAULT-1" for example, I'll
improve it in v8.

>> + *
>> + * POWEROFF_PRIO_RESERVED:     Falls back to POWEROFF_PRIO_DEFAULT.
>> + *                             Drivers may leave priority initialized
>> + *                             to zero, to auto-set it to the default level.
>> + *
>> + * POWEROFF_PRIO_PLATFORM:     Intended to be used by platform-level handler.
>> + *                             Has lowest priority since device drivers are
>> + *                             expected to take over platform handler which
>> + *                             doesn't allow further callback chaining.
>> + *
>> + * POWEROFF_PRIO_DEFAULT:      Use this for default/generic handler.
>> + *
>> + * POWEROFF_PRIO_FIRMWARE:     Use this if handler uses firmware call.
>> + *                             Has highest priority since firmware is expected
>> + *                             to know best how to power-off hardware properly.
>> + */
>> +#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
>> +
>> +enum poweroff_mode {
>> +       POWEROFF_NORMAL = 0,
> 
> Why not just POWEROFF?

This is a bit too generic name to me.

I can rename it as MODE_POWEROFF and MODE_POWEROFF_PREPARE.

>> +       POWEROFF_PREPARE,
>> +};
>> +
>> +/**
>> + * 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 does this need to be a separate data type?

To allow us extend the "struct power_off_prep_data" with more parameters
later on without a need to update each driver with the new arguments.

>> +
>> +/**
>> + * 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.
>> + * @mode: Preparation mode ID.
>> + */
>> +struct reboot_prep_data {
>> +       void *cb_data;
>> +       const char *cmd;
>> +       bool stop_chain;
>> +       enum reboot_prepare_mode mode;
>> +};
> 
> There seem to be some duplicate data items between struct restart_data
> and struct reboot_prep_data, so what's the reason why they are
> separate?

They indeed look similar, but have different purposes. We shouldn't use
reboot handler for restarting, hence why should we mix them?

>> +struct sys_off_handler_private_data {
>> +       struct notifier_block power_off_nb;
>> +       struct notifier_block restart_nb;
>> +       struct notifier_block reboot_nb;
> 
> So restart_nb is going to be added to restart_handler_list, eboot_nb
> will be added to reboot_notifier_list (which both exist already) and
> power_off_nb will be added to the new power_off_handler_list, right?
> 
> Of course, this means that reboot_nb will be used in
> kernel_restart_prepare() and kernel_shutdown_prepare(), so the
> corresponding callback will be invoked in both the restart and
> power-off cases.
> 
> It would be good to document that somehow.

This is documented in the doc-comment to struct sys_off_handler later in
this patch. You actually found it below.

>> +       void (*platform_power_off_cb)(void);
>> +       void (*simple_power_off_cb)(void *data);
>> +       void *simple_power_off_cb_data;
> 
> Is there any particular reason to put these callbacks here and not
> directly into struct sys_off_handler?

They are used internally by reboot.c and shouldn't be touched by the
kernel API users, so I wanted to factor out and protect all the private
data.

>> +       bool registered;
>> +};
>> +
>> +/**
>> + * struct sys_off_handler - System power-off and restart handler
>> + *
>> + * @cb_data: Pointer to user's data.
>> + *
>> + * @power_off_cb: Callback that powers off this machine. Inactive if NULL.
>> + *
>> + * @power_off_prepare_cb: Power-off preparation callback. All power-off
>> + * preparation callbacks are invoked after @reboot_prepare_cb and before
>> + * @power_off_cb. Inactive if NULL.
>> + *
>> + * @power_off_priority: Power-off callback priority, must be unique.
>> + * Zero value is reserved and auto-reassigned to POWEROFF_PRIO_DEFAULT.
>> + * Inactive if @power_off_cb is NULL.
>> + *
>> + * @power_off_chaining_allowed: Set to false if callback's execution should
>> + * stop when @power_off_cb fails to power off this machine. True if further
>> + * lower priority power-off callback should be executed. False is default
>> + * value.
>> + *
>> + * @restart_cb: Callback that reboots this machine. Inactive if NULL.
>> + *
>> + * @restart_priority: Restart callback priority, must be unique. Zero value
>> + * is reserved and auto-reassigned to RESTART_PRIO_DEFAULT. Inactive if
>> + * @restart_cb is NULL.
>> + *
>> + * @restart_chaining_disallowed: Set to true if callback's execution should
>> + * stop when @restart_cb fails to restart this machine. False if further
>> + * lower priority restart callback should be executed. False is default
>> + * value.
>> + *
>> + * @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.
>> + *
>> + * @reboot_priority: Reboot/shutdown preparation callback priority, doesn't
>> + * need to be unique. Zero is default value. Inactive if @reboot_prepare_cb
>> + * is NULL.
> 
> It is unclear that the ->reboot_prepare_cb() callback is going to be
> used for both restart and power-off and reboot_priority is about the
> preparation phase only.
> 
> And in the preparation phase the priority may not matter that much,
> because there are users who don't care about the ordering as long as
> their stuff is called at all.
> 
> Honestly, I would change the naming here, because what it is is quite
> confusing at least to me.  Especially that "restart" and "reboot" seem
> to be used interchangeably in the comments.

These are the legacy names and they are indeed very confusing. Do you
have suggestion for a better names?

>> + *
>> + * @priv: Internal data. Shouldn't be touched.
>> + *
>> + * 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.
>> + *
>> + * Struct sys_off_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_sys_off_handler().
>> + */
>> +struct sys_off_handler {
>> +       void *cb_data;
>> +
>> +       void (*power_off_cb)(struct power_off_data *data);
>> +       void (*power_off_prepare_cb)(struct power_off_prep_data *data);
>> +       int power_off_priority;
>> +       bool power_off_chaining_allowed;
>> +
>> +       void (*restart_cb)(struct restart_data *data);
>> +       int restart_priority;
>> +       bool restart_chaining_disallowed;
>> +
>> +       void (*reboot_prepare_cb)(struct reboot_prep_data *data);
>> +       int reboot_priority;
>> +
>> +       const struct sys_off_handler_private_data priv;
> 
> Why is it const?  Because of the callbacks in there?

It's const to prevent changing/abusing of the internal data by kernel
API users. Those callbacks are private to kernel/reboot.c

> Doesn't this mean that all struct sys_off_handler need to be static?

The reboot.c is allowed to modify the private data, sys_off_handler can
be static and dynamic. There is no limitaion, I made the 'priv' member
constant only for the API users.

>> +};
>> +
>> +int register_sys_off_handler(struct sys_off_handler *handler);
>> +int unregister_sys_off_handler(struct sys_off_handler *handler);
>> +
>> +int devm_register_sys_off_handler(struct device *dev,
>> +                                 struct sys_off_handler *handler);
>> +
>> +int devm_register_prioritized_power_off_handler(struct device *dev,
>> +                                               int priority,
>> +                                               void (*callback)(void *data),
>> +                                               void *cb_data);
>> +
>> +int devm_register_simple_power_off_handler(struct device *dev,
>> +                                          void (*callback)(void *data),
>> +                                          void *cb_data);
>> +
>> +int register_platform_power_off(void (*power_off)(void));
>> +int unregister_platform_power_off(void (*power_off)(void));
>> +
>> +int devm_register_prioritized_restart_handler(struct device *dev,
>> +                                             int priority,
>> +                                             void (*callback)(struct restart_data *data),
>> +                                             void *cb_data);
>> +
>> +int devm_register_simple_restart_handler(struct device *dev,
>> +                                        void (*callback)(struct restart_data *data),
>> +                                        void *cb_data);
>> +
>> +void do_kernel_power_off(void);
>> +
>>  /*
>>   * Architecture-specific implementations of sys_reboot commands.
>>   */
>> @@ -70,6 +290,7 @@ extern void kernel_restart_prepare(char *cmd);
>>  extern void kernel_restart(char *cmd);
>>  extern void kernel_halt(void);
>>  extern void kernel_power_off(void);
>> +extern bool kernel_can_power_off(void);
>>
>>  void ctrl_alt_del(void);
>>
>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> index 938d5c78b421..a9464b4a3209 100644
>> --- a/kernel/power/hibernate.c
>> +++ b/kernel/power/hibernate.c
>> @@ -665,7 +665,7 @@ static void power_down(void)
>>                 hibernation_platform_enter();
>>                 fallthrough;
>>         case HIBERNATION_SHUTDOWN:
>> -               if (pm_power_off)
>> +               if (kernel_can_power_off())
>>                         kernel_power_off();
>>                 break;
>>         }
>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>> index acdae4e95061..e76e2570dcf5 100644
>> --- a/kernel/reboot.c
>> +++ b/kernel/reboot.c
>> @@ -296,6 +296,595 @@ void kernel_halt(void)
>>  }
>>  EXPORT_SYMBOL_GPL(kernel_halt);
>>
>> +/*
>> + *     Notifier list for kernel code which wants to be called
>> + *     to power off the system.
>> + */
>> +static BLOCKING_NOTIFIER_HEAD(power_off_handler_list);
>> +
>> +/*
>> + * Temporary stub that prevents linkage failure while we're in process
>> + * of removing all uses of legacy pm_power_off() around the kernel.
> 
> Nit: inconsistent comment formatting.
> 
>> + */
>> +void __weak (*pm_power_off)(void);
>> +
>> +static void dummy_pm_power_off(void)
>> +{
>> +       /* temporary stub until pm_power_off() is gone, see more below */
>> +}
>> +
>> +static struct notifier_block *pm_power_off_nb;
>> +
>> +/**
>> + *     register_power_off_handler - Register function to be called to power off
>> + *                                  the system
>> + *     @nb: Info about handler function to be called
>> + *     @nb->priority:  Handler priority. Handlers should follow the
>> + *                     following guidelines for setting priorities.
>> + *                     0:      Reserved
>> + *                     1:      Power-off handler of last resort,
>> + *                             with limited power-off capabilities
>> + *                     128:    Default power-off handler; use if no other
>> + *                             power-off handler is expected to be available,
>> + *                             and/or if power-off functionality is
>> + *                             sufficient to power-off the entire system
>> + *                     255:    Highest priority power-off handler, will
>> + *                             preempt all other power-off handlers
>> + *
>> + *     Registers a function with code to be called to power off the
>> + *     system.
> 
> Because this is only used internally in this file, I'd say what it
> does directly, that is "Add a notifier to the power-off chain used for
> powering off the system".

I'll improve it, thanks.

>> + *
>> + *     Registered functions will be called as last step of the power-off
>> + *     sequence.
>> + *
>> + *     Registered functions are expected to power off the system immediately.
>> + *     If more than one function is registered, the power-off handler priority
>> + *     selects which function will be called first.
>> + *
>> + *     Power-off handlers are expected to be registered from non-architecture
>> + *     code, typically from drivers. A typical use case would be a system
>> + *     where power-off functionality is provided through a PMIC. Multiple
>> + *     power-off handlers may exist; for example, one power-off handler might
>> + *     turn off the entire system, while another only turns off part of
>> + *     system. In such cases, the power-off handler which only disables part
>> + *     of the hardware is expected to register with low priority to ensure
>> + *     that it only runs if no other means to power off the system is
>> + *     available.
> 
> I would move the above 3 paragraphs to the description comment for
> power_off_handler_list.

Noted

>> + *
>> + *     Currently always returns zero, as blocking_notifier_chain_register()
>> + *     always returns zero.
>> + */
>> +static int register_power_off_handler(struct notifier_block *nb)
>> +{
>> +       int ret;
>> +
>> +       ret = blocking_notifier_chain_register_unique_prio(&power_off_handler_list, nb);
>> +       if (ret && ret != -EBUSY)
>> +               return ret;
>> +
>> +       if (!ret)
>> +               goto set_pm_power_off;
>> +
>> +       /*
>> +        * Handler must have unique priority. Otherwise call order is
>> +        * determined by registration order, which is unreliable.
>> +        *
>> +        * This requirement will become mandatory once all drivers
>> +        * will be converted to use new sys-off API.
>> +        */
>> +       pr_err("failed to register power-off handler using unique priority\n");
>> +
>> +       ret = blocking_notifier_chain_register(&power_off_handler_list, nb);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /*
>> +        * Some drivers check whether pm_power_off was already installed.
>> +        * Install dummy callback using new API to preserve old behaviour
>> +        * for those drivers during period of transition to the new API.
>> +        */
>> +set_pm_power_off:
>> +       if (!pm_power_off) {
>> +               pm_power_off = dummy_pm_power_off;
>> +               pm_power_off_nb = nb;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int unregister_power_off_handler(struct notifier_block *nb)
>> +{
>> +       if (nb == pm_power_off_nb) {
>> +               if (pm_power_off == dummy_pm_power_off)
>> +                       pm_power_off = NULL;
>> +
>> +               pm_power_off_nb = NULL;
>> +       }
>> +
>> +       return blocking_notifier_chain_unregister(&power_off_handler_list, nb);
>> +}
>> +
>> +static void devm_unregister_power_off_handler(void *data)
>> +{
>> +       struct notifier_block *nb = data;
>> +
>> +       unregister_power_off_handler(nb);
>> +}
>> +
>> +static int devm_register_power_off_handler(struct device *dev,
>> +                                          struct notifier_block *nb)
>> +{
>> +       int err;
>> +
>> +       err = register_power_off_handler(nb);
>> +       if (err)
>> +               return err;
>> +
>> +       return devm_add_action_or_reset(dev, devm_unregister_power_off_handler,
>> +                                       nb);
>> +}
>> +
>> +static int sys_off_handler_power_off(struct notifier_block *nb,
>> +                                    unsigned long mode, void *unused)
>> +{
>> +       struct power_off_prep_data prep_data = {};
>> +       struct sys_off_handler_private_data *priv;
>> +       struct power_off_data data = {};
>> +       struct sys_off_handler *h;
>> +       int ret = NOTIFY_DONE;
>> +
>> +       priv = container_of(nb, struct sys_off_handler_private_data, power_off_nb);
>> +       h = container_of(priv, struct sys_off_handler, priv);
>> +       prep_data.cb_data = h->cb_data;
>> +       data.cb_data = h->cb_data;
>> +
>> +       switch (mode) {
>> +       case POWEROFF_NORMAL:
>> +               if (h->power_off_cb)
>> +                       h->power_off_cb(&data);
>> +
>> +               if (priv->simple_power_off_cb)
>> +                       priv->simple_power_off_cb(priv->simple_power_off_cb_data);
>> +
>> +               if (priv->platform_power_off_cb)
>> +                       priv->platform_power_off_cb();
> 
> The invocation of the priv callbacks here confuses me quite a bit.
> 
> Can you please at least add a comment explaining this?

Sure

>> +
>> +               if (!h->power_off_chaining_allowed)
>> +                       ret = NOTIFY_STOP;
>> +
>> +               break;
>> +
>> +       case POWEROFF_PREPARE:
>> +               if (h->power_off_prepare_cb)
>> +                       h->power_off_prepare_cb(&prep_data);
>> +
>> +               break;
>> +
>> +       default:
>> +               unreachable();
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static int sys_off_handler_restart(struct notifier_block *nb,
>> +                                  unsigned long mode, void *cmd)
>> +{
>> +       struct sys_off_handler_private_data *priv;
>> +       struct restart_data data = {};
>> +       struct sys_off_handler *h;
>> +
>> +       priv = container_of(nb, struct sys_off_handler_private_data, restart_nb);
>> +       h = container_of(priv, struct sys_off_handler, priv);
>> +
>> +       data.stop_chain = h->restart_chaining_disallowed;
>> +       data.cb_data = h->cb_data;
>> +       data.mode = mode;
>> +       data.cmd = cmd;
>> +
>> +       h->restart_cb(&data);
> 
> Wouldn't it be a bit more straightforward to allow ->restart_cb() to
> return a value being either NOTIFY_STOP or NOTIFY_DONE?

Vast majority of drivers don't use the 'stop_chain', hence it's
unnecessary boilerplate code for drivers. The code looks nicer without
the boilerplate "return NOTIFY_DONE", IMO.

>> +       return data.stop_chain ? NOTIFY_STOP : NOTIFY_DONE;
> 
> And I would prefer
> 
> if (data.stop_chain)
>         return NOTIFY_STOP;
> 
> return NOTIFY_DONE;

Okay

>> +}
>> +
>> +static int sys_off_handler_reboot(struct notifier_block *nb,
>> +                                 unsigned long mode, void *cmd)
>> +{
>> +       struct sys_off_handler_private_data *priv;
>> +       struct reboot_prep_data data = {};
>> +       struct sys_off_handler *h;
>> +
>> +       priv = container_of(nb, struct sys_off_handler_private_data, reboot_nb);
>> +       h = container_of(priv, struct sys_off_handler, priv);
>> +
>> +       data.cb_data = h->cb_data;
>> +       data.stop_chain = false;
>> +       data.mode = mode;
>> +       data.cmd = cmd;
>> +
>> +       h->reboot_prepare_cb(&data);
>> +
>> +       return data.stop_chain ? NOTIFY_STOP : NOTIFY_DONE;
> 
> And analogously here.
> 
>> +}
>> +
>> +static struct sys_off_handler_private_data *
>> +sys_off_handler_private_data(struct sys_off_handler *handler)
>> +{
>> +       return (struct sys_off_handler_private_data *)&handler->priv;
> 
> Is the cast needed to avoid a warning about "const"?

The private data is constant to prevent the API users from using it. For
internal use we need to cast out the "const". It's a compilation error
to change the constant members.

>> +}
>> +
>> +/**
>> + *     devm_register_sys_off_handler - Register system power-off/restart handler
> 
> register_sys_off_handler

Good catch!

>> + *     @dev: Device that registers handler
>> + *     @handler: System-off handler
>> + *
>> + *     Registers handler that will be called as last step of the power-off
>> + *     and restart sequences.
> 
> Not necessarily as the last step, because there may be other system
> power-off/restart handlers called after it.
> 
> I would just say "at the end of the power-off and restart sequences".
> 
> Moreover, it registers the "reboot_cb" part that is not called "at the
> end" even.
> 
> Also, because this is the function that will be used by drivers etc to
> register handlers, I would give some more information on how the
> object registered by it is going to be used to the prospective users.

I'll improve the description, thank you.

>> + *
>> + *     Returns zero on success, or error code on failure.
>> + */
>> +int register_sys_off_handler(struct sys_off_handler *handler)
>> +{
>> +       struct sys_off_handler_private_data *priv;
>> +       int err, priority;
>> +
>> +       priv = sys_off_handler_private_data(handler);
>> +
>> +       /* sanity-check whether handler is registered twice */
>> +       if (priv->registered)
>> +               return -EBUSY;
>> +
>> +       if (handler->power_off_cb || handler->power_off_prepare_cb) {
>> +               if (handler->power_off_priority == POWEROFF_PRIO_RESERVED)
>> +                       priority = POWEROFF_PRIO_DEFAULT;
> 
> I'm not sure that this helps.
> 
> I mean, why can't the users of this new API pass POWEROFF_PRIO_DEFAULT
> directly if they want "default"?

Users could pass POWEROFF_PRIO_DEFAULT, but it's a boilerplate code
which I wanted to avoid by reserving 0 for the alias to
POWEROFF_PRIO_DEFAULT=128.

This removes a need from drivers to explicitly specify the default
priorities in the code, assuming that sys_off_handler is initialized to
zero.

>> +               else
>> +                       priority = handler->power_off_priority;
>> +
>> +               priv->power_off_nb.notifier_call = sys_off_handler_power_off;
>> +               priv->power_off_nb.priority = priority;
>> +
>> +               err = register_power_off_handler(&priv->power_off_nb);
>> +               if (err)
>> +                       goto reset_sys_off_handler;
>> +       }
>> +
>> +       if (handler->restart_cb) {
>> +               if (handler->restart_priority == RESTART_PRIO_RESERVED)
>> +                       priority = RESTART_PRIO_DEFAULT;
>> +               else
>> +                       priority = handler->restart_priority;
>> +
>> +               priv->restart_nb.notifier_call = sys_off_handler_restart;
>> +               priv->restart_nb.priority = priority;
>> +
>> +               err = register_restart_handler(&priv->restart_nb);
>> +               if (err)
>> +                       goto unreg_power_off_handler;
>> +       }
>> +
>> +       if (handler->reboot_prepare_cb) {
>> +               priv->reboot_nb.notifier_call = sys_off_handler_reboot;
>> +               priv->reboot_nb.priority = handler->reboot_priority;
>> +
>> +               err = register_reboot_notifier(&priv->reboot_nb);
>> +               if (err)
>> +                       goto unreg_restart_handler;
>> +       }
>> +
>> +       priv->registered = true;
>> +
>> +       return 0;
>> +
>> +unreg_restart_handler:
>> +       if (handler->restart_cb)
>> +               unregister_restart_handler(&priv->restart_nb);
>> +
>> +unreg_power_off_handler:
>> +       if (handler->power_off_cb)
>> +               unregister_power_off_handler(&priv->power_off_nb);
>> +
>> +reset_sys_off_handler:
>> +       memset(priv, 0, sizeof(*priv));
>> +
>> +       return err;
>> +}
>> +EXPORT_SYMBOL(register_sys_off_handler);
>> +
>> +/**
>> + *     unregister_sys_off_handler - Unregister system power-off/restart handler
>> + *     @handler: System-off handler
>> + *
>> + *     Unregisters sys-off handler. Does nothing and returns zero if handler
>> + *     is NULL.
>> + *
>> + *     Returns zero on success, or error code on failure.
>> + */
>> +int unregister_sys_off_handler(struct sys_off_handler *handler)
>> +{
>> +       struct sys_off_handler_private_data *priv;
>> +
>> +       if (!handler)
>> +               return 0;
>> +
>> +       priv = sys_off_handler_private_data(handler);
>> +
>> +       /* sanity-check whether handler is unregistered twice */
>> +       if (!priv->registered)
>> +               return -EINVAL;
>> +
>> +       if (handler->reboot_prepare_cb)
>> +               unregister_reboot_notifier(&priv->reboot_nb);
>> +
>> +       if (handler->restart_cb)
>> +               unregister_restart_handler(&priv->restart_nb);
>> +
>> +       if (handler->power_off_cb)
>> +               unregister_power_off_handler(&priv->power_off_nb);
>> +
>> +       memset(priv, 0, sizeof(*priv));
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(unregister_sys_off_handler);
>> +
>> +static void devm_unregister_sys_off_handler(void *data)
>> +{
>> +       struct sys_off_handler *handler = data;
>> +
>> +       unregister_sys_off_handler(handler);
>> +}
>> +
>> +/**
>> + *     devm_register_sys_off_handler - Register system power-off/restart handler
>> + *     @dev: Device that registers handler
>> + *     @handler: System-off handler
>> + *
>> + *     Resource-managed variant of register_sys_off_handler().
>> + *
>> + *     Returns zero on success, or error code on failure.
>> + */
>> +int devm_register_sys_off_handler(struct device *dev,
>> +                                 struct sys_off_handler *handler)
>> +{
>> +       int err;
>> +
>> +       err = register_sys_off_handler(handler);
>> +       if (err)
>> +               return err;
>> +
>> +       return devm_add_action_or_reset(dev, devm_unregister_sys_off_handler,
>> +                                       handler);
>> +}
>> +EXPORT_SYMBOL(devm_register_sys_off_handler);
>> +
>> +/**
>> + *     devm_register_prioritized_power_off_handler - Register prioritized power-off callback
>> + *     @dev: Device that registers callback
>> + *     @priority: Callback's priority
>> + *     @callback: Callback function
>> + *     @cb_data: Callback's argument
>> + *
>> + *     Registers resource-managed power-off callback with a given priority.
>> + *     It will be called as last step of the power-off sequence. Callbacks
>> + *     chaining is disabled, i.e. further lower priority callbacks won't
>> + *     be executed if this @callback will fail to execute.
>> + *
>> + *     Returns zero on success, or error code on failure.
> 
> What's the case in which this should be used instead of registering a
> full sys_off handler?

There are a lot of drivers that just want to register power-off handler
with a non-default priority and don't need to stop the chain or do
anything else special. This is a convinient helper for them.

Please note that the callback here takes only the cb_data for the
argument, while sys_off uses "struct power_off_data".

Similar for the reset of the convinient helpers.

...
>> +/**
>> + *     register_platform_power_off - Register platform-level power-off callback
>> + *     @power_off: Power-off callback
>> + *
>> + *     Registers power-off callback that will be called as last step
>> + *     of the power-off sequence. This callback is expected to be invoked
>> + *     for the last resort. Further lower priority callbacks won't be
>> + *     executed if @power_off fails. Only one platform power-off callback
>> + *     is allowed to be registered at a time.
>> + *
>> + *     Returns zero on success, or error code on failure.
> 
> What's the use case for this?

There are dozens of trivial platform/board-specific power-off handlers
in the arch/ code. Those handlers don't take any arguments, hence it's a
convinient helper that transits them to the new API neatly.

>> + */
>> +int register_platform_power_off(void (*power_off)(void))
>> +{
>> +       struct sys_off_handler_private_data *priv;
>> +       int ret = 0;
>> +
>> +       priv = sys_off_handler_private_data(&platform_power_off_handler);
>> +
>> +       spin_lock(&platform_power_off_lock);
>> +       if (priv->platform_power_off_cb)
>> +               ret = -EBUSY;
>> +       else
>> +               priv->platform_power_off_cb = power_off;
> 
> Wasn't priv supposed to be const?

It's const only for the external API users to prevent them from abusing
the internal data.

>> +       spin_unlock(&platform_power_off_lock);
>> +
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = register_power_off_handler(&priv->power_off_nb);
>> +       if (ret)
>> +               priv->platform_power_off_cb = NULL;
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(register_platform_power_off);
>> +
>> +/**
>> + *     unregister_platform_power_off - Unregister platform-level power-off callback
>> + *     @power_off: Power-off callback
>> + *
>> + *     Unregisters previously registered platform power-off callback.
>> + *
>> + *     Returns zero on success, or error code on failure.
>> + */
>> +int unregister_platform_power_off(void (*power_off)(void))
>> +{
>> +       struct sys_off_handler_private_data *priv;
>> +       int ret;
>> +
>> +       priv = sys_off_handler_private_data(&platform_power_off_handler);
>> +
>> +       if (priv->platform_power_off_cb != power_off)
>> +               return -EINVAL;
>> +
>> +       ret = unregister_power_off_handler(&priv->power_off_nb);
>> +       priv->platform_power_off_cb = NULL;
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(unregister_platform_power_off);
>> +
>> +/**
>> + *     do_kernel_power_off - Execute kernel power-off handler call chain
>> + *
>> + *     Calls functions registered with register_power_off_handler.
>> + *
>> + *     Expected to be called as last step of the power-off sequence.
>> + *
>> + *     Powers off the system immediately if a power-off handler function has
>> + *     been registered. Otherwise does nothing.
>> + */
>> +void do_kernel_power_off(void)
>> +{
>> +       /* legacy pm_power_off() is unchained and has highest priority */
>> +       if (pm_power_off && pm_power_off != dummy_pm_power_off)
>> +               return pm_power_off();
>> +
>> +       blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_NORMAL,
>> +                                    NULL);
>> +}
>> +
>> +static void do_kernel_power_off_prepare(void)
>> +{
>> +       /* legacy pm_power_off_prepare() is unchained and has highest priority */
>> +       if (pm_power_off_prepare)
>> +               return pm_power_off_prepare();
>> +
>> +       blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_PREPARE,
>> +                                    NULL);
>> +}
>> +
>>  /**
>>   *     kernel_power_off - power_off the system
>>   *
>> @@ -304,8 +893,7 @@ EXPORT_SYMBOL_GPL(kernel_halt);
>>  void kernel_power_off(void)
>>  {
>>         kernel_shutdown_prepare(SYSTEM_POWER_OFF);
>> -       if (pm_power_off_prepare)
>> -               pm_power_off_prepare();
>> +       do_kernel_power_off_prepare();
>>         migrate_to_reboot_cpu();
>>         syscore_shutdown();
>>         pr_emerg("Power down\n");
>> @@ -314,6 +902,16 @@ void kernel_power_off(void)
>>  }
>>  EXPORT_SYMBOL_GPL(kernel_power_off);
>>
>> +bool kernel_can_power_off(void)
>> +{
>> +       if (!pm_power_off &&
>> +           blocking_notifier_call_chain_is_empty(&power_off_handler_list))
>> +               return false;
>> +
>> +       return true;
> 
> return pm_power_off ||
> blocking_notifier_call_chain_is_empty(&power_off_handler_list);

Thank you for the thorough review!

Re: [PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority

[Rafael J. Wysocki]

On Mon, Apr 18, 2022 at 3:29 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 4/14/22 14:19, Rafael J. Wysocki wrote:
> > On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> On 4/13/22 21:48, Rafael J. Wysocki wrote:
> >>> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko
> >>> <dmitry.osipenko@collabora.com> wrote:
> >>>>
> >>>> Add sanity check which ensures that there are no two restart handlers
> >>>> registered using the same priority. This requirement will become mandatory
> >>>> once all drivers will be converted to the new API and such errors will be
> >>>> fixed.
> >>>>
> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>>
> >>> The first two patches in the series are fine with me and there's only
> >>> one minor nit regarding this one (below).
> >>>
> >>>> ---
> >>>>  kernel/reboot.c | 15 +++++++++++++++
> >>>>  1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/kernel/reboot.c b/kernel/reboot.c
> >>>> index ed4e6dfb7d44..acdae4e95061 100644
> >>>> --- a/kernel/reboot.c
> >>>> +++ b/kernel/reboot.c
> >>>> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
> >>>>   */
> >>>>  int register_restart_handler(struct notifier_block *nb)
> >>>>  {
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb);
> >>>> +       if (ret != -EBUSY)
> >>>> +               return ret;
> >>>> +
> >>>> +       /*
> >>>> +        * Handler must have unique priority. Otherwise call order is
> >>>> +        * determined by registration order, which is unreliable.
> >>>> +        *
> >>>> +        * This requirement will become mandatory once all drivers
> >>>> +        * will be converted to use new sys-off API.
> >>>> +        */
> >>>> +       pr_err("failed to register restart handler using unique priority\n");
> >>>
> >>> I would use pr_info() here, because this is not a substantial error AFAICS.
> >>
> >> It's indeed not a substantial error so far, but it will become
> >> substantial later on once only unique priorities will be allowed. The
> >> pr_warn() could be a good compromise here, pr_info() is too mild, IMO.
> >
> > Well, I'm still unconvinced about requiring all of the users of this
> > interface to use unique priorities.
> >
> > Arguably, there are some of them who don't really care about the
> > ordering, so could there be an option for them to specify the lack of
> > care by, say, passing 0 as the priority that would be regarded as a
> > special case?
> >
> > IOW, if you pass 0, you'll be run along the others who've also passed
> > 0, but if you pass anything different from 0, it must be unique.  What
> > do you think?
>
> There are indeed cases where ordering is unimportant. Like a case of
> PMIC and watchdog restart handlers for example, both handlers will
> produce equal effect from a user's perspective. Perhaps indeed it's more
> practical to have at least one shared level.
>
> In this patchset the level 0 is specified as an alias to the default
> level 128. If one user registers handler using unique level 128 and the
> other user uses non-unique level 0, then we have ambiguity.
>
> One potential option is to make the whole default level 128 non-unique.
> This will allow users to not care about the uniqueness by default like
> they always did it previously, but it will hide potential problems for
> users who actually need unique level and don't know about it yet due to
> a lucky registration ordering that they have today. Are you okay with
> this option?

Yes, I am.

Re: [PATCH v7 04/20] kernel: Add combined power-off+restart handler call chain API

[Rafael J. Wysocki]

On Mon, Apr 18, 2022 at 3:44 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 4/15/22 21:14, Rafael J. Wysocki wrote:
> > Honestly, I would prefer this to be split so as to make it easier to
> > review if nothing else.
>
> I'll try to split it in v8.
>
> > On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> 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.
> >
> > Which means that the existing notifier chains for system restart are
> > used as they are without modifications.
> >
> > At least that's what follows from the code and it would be good to
> > mention it here.
>
> Will improve the commit message.
>
> > Moreover, a new notifier chain is introduced for the power-off case
> > and it appears to be the counterpart of the restart_handler_list
> > chain, but then why is it blocking and not atomic like the latter?
>
> Good catch, it probably indeed should be atomic because shutting down
> could run with a disabled interrupts. I'll invistigate this more for v8,
> at least right now I don't recall any particular reason for using the
> blocking notifier.
>
> >> 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.
> >
> > "the corresponding"
>
> Thanks
>
> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >> ---
> >>  include/linux/reboot.h   | 229 ++++++++++++++-
> >>  kernel/power/hibernate.c |   2 +-
> >>  kernel/reboot.c          | 604 ++++++++++++++++++++++++++++++++++++++-
> >>  3 files changed, 827 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> >> index a2429648d831..ba5e5dddcfcd 100644
> >> --- a/include/linux/reboot.h
> >> +++ b/include/linux/reboot.h
> >> @@ -8,10 +8,35 @@
> >>
> >>  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 */
> >> +};
> >> +
> >> +/*
> >> + * Standard restart priority levels. Intended to be set in the
> >> + * sys_off_handler.restart_priority field.
> >> + *
> >> + * Use `RESTART_PRIO_ABC +- prio` style for additional levels.
> >> + *
> >> + * RESTART_PRIO_RESERVED:      Falls back to RESTART_PRIO_DEFAULT.
> >> + *                             Drivers may leave priority initialized
> >> + *                             to zero, to auto-set it to the default level.
> >
> > What is the "default level" here?
>
> "default level" = RESTART_PRIO_DEFAULT = 128
>
> I'll remove the second sentence about the "default level", for clarity.
>
> >> + *
> >> + * RESTART_PRIO_LOW:           Use this for handler of last resort.
> >> + *
> >> + * RESTART_PRIO_DEFAULT:       Use this for default/generic handler.
> >> + *
> >> + * RESTART_PRIO_HIGH:          Use this if you have multiple handlers and
> >> + *                             this handler has higher priority than the
> >> + *                             default handler.
> >> + */
> >> +#define RESTART_PRIO_RESERVED          0
> >> +#define RESTART_PRIO_LOW               8
> >> +#define RESTART_PRIO_DEFAULT           128
> >> +#define RESTART_PRIO_HIGH              192
> >>
> >>  enum reboot_mode {
> >>         REBOOT_UNDEFINED = -1,
> >> @@ -49,6 +74,201 @@ extern int register_restart_handler(struct notifier_block *);
> >>  extern int unregister_restart_handler(struct notifier_block *);
> >>  extern void do_kernel_restart(char *cmd);
> >>
> >> +/*
> >> + * System power-off and restart API.
> >> + */
> >> +
> >> +/*
> >> + * Standard power-off priority levels. Intended to be set in the
> >> + * sys_off_handler.power_off_priority field.
> >> + *
> >> + * Use `POWEROFF_PRIO_ABC +- prio` style for additional levels.
> >
> > What exactly does this mean?
>
> "POWEROFF_PRIO_DEFAULT+1 or POWEROFF_PRIO_DEFAULT-1" for example, I'll
> improve it in v8.

So the users of this interface are expected to set priorities relative
to the "standard" levels.  That wasn't clear.

> >> + *
> >> + * POWEROFF_PRIO_RESERVED:     Falls back to POWEROFF_PRIO_DEFAULT.
> >> + *                             Drivers may leave priority initialized
> >> + *                             to zero, to auto-set it to the default level.
> >> + *
> >> + * POWEROFF_PRIO_PLATFORM:     Intended to be used by platform-level handler.
> >> + *                             Has lowest priority since device drivers are
> >> + *                             expected to take over platform handler which
> >> + *                             doesn't allow further callback chaining.
> >> + *
> >> + * POWEROFF_PRIO_DEFAULT:      Use this for default/generic handler.
> >> + *
> >> + * POWEROFF_PRIO_FIRMWARE:     Use this if handler uses firmware call.
> >> + *                             Has highest priority since firmware is expected
> >> + *                             to know best how to power-off hardware properly.
> >> + */
> >> +#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
> >> +
> >> +enum poweroff_mode {
> >> +       POWEROFF_NORMAL = 0,
> >
> > Why not just POWEROFF?
>
> This is a bit too generic name to me.
>
> I can rename it as MODE_POWEROFF and MODE_POWEROFF_PREPARE.

In that case I would leave POWEROFF_PREPARE as is and rename the other
one to POWEROFF_FINAL.  And maybe put them in the reverse order in the
enum definition, because _PREPARE will be done before _FINAL.

> >> +       POWEROFF_PREPARE,
> >> +};
> >> +
> >> +/**
> >> + * 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 does this need to be a separate data type?
>
> To allow us extend the "struct power_off_prep_data" with more parameters
> later on without a need to update each driver with the new arguments.

I'm not really sure what you mean here.  Can you give an example?

> >> +
> >> +/**
> >> + * 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.
> >> + * @mode: Preparation mode ID.
> >> + */
> >> +struct reboot_prep_data {
> >> +       void *cb_data;
> >> +       const char *cmd;
> >> +       bool stop_chain;
> >> +       enum reboot_prepare_mode mode;
> >> +};
> >
> > There seem to be some duplicate data items between struct restart_data
> > and struct reboot_prep_data, so what's the reason why they are
> > separate?
>
> They indeed look similar, but have different purposes. We shouldn't use
> reboot handler for restarting, hence why should we mix them?

Well, I got confused by the naming.

The "reboot prep" thing really should be a "system is going down"
notification.  Since "sysoff" is already used in the names below, why
not rename it to "sysoff_prepare"?

> >> +struct sys_off_handler_private_data {
> >> +       struct notifier_block power_off_nb;
> >> +       struct notifier_block restart_nb;
> >> +       struct notifier_block reboot_nb;
> >
> > So restart_nb is going to be added to restart_handler_list, eboot_nb
> > will be added to reboot_notifier_list (which both exist already) and
> > power_off_nb will be added to the new power_off_handler_list, right?
> >
> > Of course, this means that reboot_nb will be used in
> > kernel_restart_prepare() and kernel_shutdown_prepare(), so the
> > corresponding callback will be invoked in both the restart and
> > power-off cases.
> >
> > It would be good to document that somehow.
>
> This is documented in the doc-comment to struct sys_off_handler later in
> this patch. You actually found it below.

IMO, it would be better to document it here, because that's where
people will look first, as a rule.

> >> +       void (*platform_power_off_cb)(void);
> >> +       void (*simple_power_off_cb)(void *data);
> >> +       void *simple_power_off_cb_data;
> >
> > Is there any particular reason to put these callbacks here and not
> > directly into struct sys_off_handler?
>
> They are used internally by reboot.c and shouldn't be touched by the
> kernel API users, so I wanted to factor out and protect all the private
> data.

But they come from these users anyway, don't they?

> >> +       bool registered;
> >> +};
> >> +
> >> +/**
> >> + * struct sys_off_handler - System power-off and restart handler
> >> + *
> >> + * @cb_data: Pointer to user's data.
> >> + *
> >> + * @power_off_cb: Callback that powers off this machine. Inactive if NULL.
> >> + *
> >> + * @power_off_prepare_cb: Power-off preparation callback. All power-off
> >> + * preparation callbacks are invoked after @reboot_prepare_cb and before
> >> + * @power_off_cb. Inactive if NULL.
> >> + *
> >> + * @power_off_priority: Power-off callback priority, must be unique.
> >> + * Zero value is reserved and auto-reassigned to POWEROFF_PRIO_DEFAULT.
> >> + * Inactive if @power_off_cb is NULL.
> >> + *
> >> + * @power_off_chaining_allowed: Set to false if callback's execution should
> >> + * stop when @power_off_cb fails to power off this machine. True if further
> >> + * lower priority power-off callback should be executed. False is default
> >> + * value.
> >> + *
> >> + * @restart_cb: Callback that reboots this machine. Inactive if NULL.
> >> + *
> >> + * @restart_priority: Restart callback priority, must be unique. Zero value
> >> + * is reserved and auto-reassigned to RESTART_PRIO_DEFAULT. Inactive if
> >> + * @restart_cb is NULL.
> >> + *
> >> + * @restart_chaining_disallowed: Set to true if callback's execution should
> >> + * stop when @restart_cb fails to restart this machine. False if further
> >> + * lower priority restart callback should be executed. False is default
> >> + * value.
> >> + *
> >> + * @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.
> >> + *
> >> + * @reboot_priority: Reboot/shutdown preparation callback priority, doesn't
> >> + * need to be unique. Zero is default value. Inactive if @reboot_prepare_cb
> >> + * is NULL.
> >
> > It is unclear that the ->reboot_prepare_cb() callback is going to be
> > used for both restart and power-off and reboot_priority is about the
> > preparation phase only.
> >
> > And in the preparation phase the priority may not matter that much,
> > because there are users who don't care about the ordering as long as
> > their stuff is called at all.
> >
> > Honestly, I would change the naming here, because what it is is quite
> > confusing at least to me.  Especially that "restart" and "reboot" seem
> > to be used interchangeably in the comments.
>
> These are the legacy names and they are indeed very confusing. Do you
> have suggestion for a better names?

Well, as I said above, I would rename the "reboot" thing to
"sysoff_prepare" covering all of the cases.  And then either "restart"
or "poweroff" would be called after that, depending on the specific
case.

On a slightly related note, I started to wonder about the actual value
provided by struct sys_off_handler.  Namely, the existing "reboot" and
"restart" notifier chains will be used anyway and there are ways to
use them directly.  Also, there will be ways to use the new "poweroff"
chain directly, so struct sys_off_handler appears to be an extra
complication and it is not particularly easier to use than a
combination of "raw" notifiers.

It looks to me that it might be sufficient to have a way to register a
callback for "sysoff_prepare" (aka "reboot") and "restart" and a pair
of callbacks for "poweroff", with a given priority with all of the
necessary internals taken care of by the framework.

> >> + *
> >> + * @priv: Internal data. Shouldn't be touched.
> >> + *
> >> + * 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.
> >> + *
> >> + * Struct sys_off_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_sys_off_handler().
> >> + */
> >> +struct sys_off_handler {
> >> +       void *cb_data;
> >> +
> >> +       void (*power_off_cb)(struct power_off_data *data);
> >> +       void (*power_off_prepare_cb)(struct power_off_prep_data *data);
> >> +       int power_off_priority;
> >> +       bool power_off_chaining_allowed;
> >> +
> >> +       void (*restart_cb)(struct restart_data *data);
> >> +       int restart_priority;
> >> +       bool restart_chaining_disallowed;
> >> +
> >> +       void (*reboot_prepare_cb)(struct reboot_prep_data *data);
> >> +       int reboot_priority;
> >> +
> >> +       const struct sys_off_handler_private_data priv;
> >
> > Why is it const?  Because of the callbacks in there?
>
> It's const to prevent changing/abusing of the internal data by kernel
> API users. Those callbacks are private to kernel/reboot.c
>
> > Doesn't this mean that all struct sys_off_handler need to be static?
>
> The reboot.c is allowed to modify the private data, sys_off_handler can
> be static and dynamic. There is no limitaion, I made the 'priv' member
> constant only for the API users.

Well, to me it is confusing and it doesn't really make the intention clear.

Personally, I'd call it something like "internal_leave_alone" or similar.

> >> +};
> >> +
> >> +int register_sys_off_handler(struct sys_off_handler *handler);
> >> +int unregister_sys_off_handler(struct sys_off_handler *handler);
> >> +
> >> +int devm_register_sys_off_handler(struct device *dev,
> >> +                                 struct sys_off_handler *handler);
> >> +
> >> +int devm_register_prioritized_power_off_handler(struct device *dev,
> >> +                                               int priority,
> >> +                                               void (*callback)(void *data),
> >> +                                               void *cb_data);
> >> +
> >> +int devm_register_simple_power_off_handler(struct device *dev,
> >> +                                          void (*callback)(void *data),
> >> +                                          void *cb_data);
> >> +
> >> +int register_platform_power_off(void (*power_off)(void));
> >> +int unregister_platform_power_off(void (*power_off)(void));
> >> +
> >> +int devm_register_prioritized_restart_handler(struct device *dev,
> >> +                                             int priority,
> >> +                                             void (*callback)(struct restart_data *data),
> >> +                                             void *cb_data);
> >> +
> >> +int devm_register_simple_restart_handler(struct device *dev,
> >> +                                        void (*callback)(struct restart_data *data),
> >> +                                        void *cb_data);
> >> +
> >> +void do_kernel_power_off(void);
> >> +
> >>  /*
> >>   * Architecture-specific implementations of sys_reboot commands.
> >>   */
> >> @@ -70,6 +290,7 @@ extern void kernel_restart_prepare(char *cmd);
> >>  extern void kernel_restart(char *cmd);
> >>  extern void kernel_halt(void);
> >>  extern void kernel_power_off(void);
> >> +extern bool kernel_can_power_off(void);
> >>
> >>  void ctrl_alt_del(void);
> >>
> >> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> >> index 938d5c78b421..a9464b4a3209 100644
> >> --- a/kernel/power/hibernate.c
> >> +++ b/kernel/power/hibernate.c
> >> @@ -665,7 +665,7 @@ static void power_down(void)
> >>                 hibernation_platform_enter();
> >>                 fallthrough;
> >>         case HIBERNATION_SHUTDOWN:
> >> -               if (pm_power_off)
> >> +               if (kernel_can_power_off())
> >>                         kernel_power_off();
> >>                 break;
> >>         }
> >> diff --git a/kernel/reboot.c b/kernel/reboot.c
> >> index acdae4e95061..e76e2570dcf5 100644
> >> --- a/kernel/reboot.c
> >> +++ b/kernel/reboot.c
> >> @@ -296,6 +296,595 @@ void kernel_halt(void)
> >>  }
> >>  EXPORT_SYMBOL_GPL(kernel_halt);
> >>
> >> +/*
> >> + *     Notifier list for kernel code which wants to be called
> >> + *     to power off the system.
> >> + */
> >> +static BLOCKING_NOTIFIER_HEAD(power_off_handler_list);
> >> +
> >> +/*
> >> + * Temporary stub that prevents linkage failure while we're in process
> >> + * of removing all uses of legacy pm_power_off() around the kernel.
> >
> > Nit: inconsistent comment formatting.
> >
> >> + */
> >> +void __weak (*pm_power_off)(void);
> >> +
> >> +static void dummy_pm_power_off(void)
> >> +{
> >> +       /* temporary stub until pm_power_off() is gone, see more below */
> >> +}
> >> +
> >> +static struct notifier_block *pm_power_off_nb;
> >> +
> >> +/**
> >> + *     register_power_off_handler - Register function to be called to power off
> >> + *                                  the system
> >> + *     @nb: Info about handler function to be called
> >> + *     @nb->priority:  Handler priority. Handlers should follow the
> >> + *                     following guidelines for setting priorities.
> >> + *                     0:      Reserved
> >> + *                     1:      Power-off handler of last resort,
> >> + *                             with limited power-off capabilities
> >> + *                     128:    Default power-off handler; use if no other
> >> + *                             power-off handler is expected to be available,
> >> + *                             and/or if power-off functionality is
> >> + *                             sufficient to power-off the entire system
> >> + *                     255:    Highest priority power-off handler, will
> >> + *                             preempt all other power-off handlers
> >> + *
> >> + *     Registers a function with code to be called to power off the
> >> + *     system.
> >
> > Because this is only used internally in this file, I'd say what it
> > does directly, that is "Add a notifier to the power-off chain used for
> > powering off the system".
>
> I'll improve it, thanks.
>
> >> + *
> >> + *     Registered functions will be called as last step of the power-off
> >> + *     sequence.
> >> + *
> >> + *     Registered functions are expected to power off the system immediately.
> >> + *     If more than one function is registered, the power-off handler priority
> >> + *     selects which function will be called first.
> >> + *
> >> + *     Power-off handlers are expected to be registered from non-architecture
> >> + *     code, typically from drivers. A typical use case would be a system
> >> + *     where power-off functionality is provided through a PMIC. Multiple
> >> + *     power-off handlers may exist; for example, one power-off handler might
> >> + *     turn off the entire system, while another only turns off part of
> >> + *     system. In such cases, the power-off handler which only disables part
> >> + *     of the hardware is expected to register with low priority to ensure
> >> + *     that it only runs if no other means to power off the system is
> >> + *     available.
> >
> > I would move the above 3 paragraphs to the description comment for
> > power_off_handler_list.
>
> Noted
>
> >> + *
> >> + *     Currently always returns zero, as blocking_notifier_chain_register()
> >> + *     always returns zero.
> >> + */
> >> +static int register_power_off_handler(struct notifier_block *nb)
> >> +{
> >> +       int ret;
> >> +
> >> +       ret = blocking_notifier_chain_register_unique_prio(&power_off_handler_list, nb);
> >> +       if (ret && ret != -EBUSY)
> >> +               return ret;
> >> +
> >> +       if (!ret)
> >> +               goto set_pm_power_off;
> >> +
> >> +       /*
> >> +        * Handler must have unique priority. Otherwise call order is
> >> +        * determined by registration order, which is unreliable.
> >> +        *
> >> +        * This requirement will become mandatory once all drivers
> >> +        * will be converted to use new sys-off API.
> >> +        */
> >> +       pr_err("failed to register power-off handler using unique priority\n");
> >> +
> >> +       ret = blocking_notifier_chain_register(&power_off_handler_list, nb);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       /*
> >> +        * Some drivers check whether pm_power_off was already installed.
> >> +        * Install dummy callback using new API to preserve old behaviour
> >> +        * for those drivers during period of transition to the new API.
> >> +        */
> >> +set_pm_power_off:
> >> +       if (!pm_power_off) {
> >> +               pm_power_off = dummy_pm_power_off;
> >> +               pm_power_off_nb = nb;
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int unregister_power_off_handler(struct notifier_block *nb)
> >> +{
> >> +       if (nb == pm_power_off_nb) {
> >> +               if (pm_power_off == dummy_pm_power_off)
> >> +                       pm_power_off = NULL;
> >> +
> >> +               pm_power_off_nb = NULL;
> >> +       }
> >> +
> >> +       return blocking_notifier_chain_unregister(&power_off_handler_list, nb);
> >> +}
> >> +
> >> +static void devm_unregister_power_off_handler(void *data)
> >> +{
> >> +       struct notifier_block *nb = data;
> >> +
> >> +       unregister_power_off_handler(nb);
> >> +}
> >> +
> >> +static int devm_register_power_off_handler(struct device *dev,
> >> +                                          struct notifier_block *nb)
> >> +{
> >> +       int err;
> >> +
> >> +       err = register_power_off_handler(nb);
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       return devm_add_action_or_reset(dev, devm_unregister_power_off_handler,
> >> +                                       nb);
> >> +}
> >> +
> >> +static int sys_off_handler_power_off(struct notifier_block *nb,
> >> +                                    unsigned long mode, void *unused)
> >> +{
> >> +       struct power_off_prep_data prep_data = {};
> >> +       struct sys_off_handler_private_data *priv;
> >> +       struct power_off_data data = {};
> >> +       struct sys_off_handler *h;
> >> +       int ret = NOTIFY_DONE;
> >> +
> >> +       priv = container_of(nb, struct sys_off_handler_private_data, power_off_nb);
> >> +       h = container_of(priv, struct sys_off_handler, priv);
> >> +       prep_data.cb_data = h->cb_data;
> >> +       data.cb_data = h->cb_data;
> >> +
> >> +       switch (mode) {
> >> +       case POWEROFF_NORMAL:
> >> +               if (h->power_off_cb)
> >> +                       h->power_off_cb(&data);
> >> +
> >> +               if (priv->simple_power_off_cb)
> >> +                       priv->simple_power_off_cb(priv->simple_power_off_cb_data);
> >> +
> >> +               if (priv->platform_power_off_cb)
> >> +                       priv->platform_power_off_cb();
> >
> > The invocation of the priv callbacks here confuses me quite a bit.
> >
> > Can you please at least add a comment explaining this?
>
> Sure
>
> >> +
> >> +               if (!h->power_off_chaining_allowed)
> >> +                       ret = NOTIFY_STOP;
> >> +
> >> +               break;
> >> +
> >> +       case POWEROFF_PREPARE:
> >> +               if (h->power_off_prepare_cb)
> >> +                       h->power_off_prepare_cb(&prep_data);
> >> +
> >> +               break;
> >> +
> >> +       default:
> >> +               unreachable();
> >> +       }
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +static int sys_off_handler_restart(struct notifier_block *nb,
> >> +                                  unsigned long mode, void *cmd)
> >> +{
> >> +       struct sys_off_handler_private_data *priv;
> >> +       struct restart_data data = {};
> >> +       struct sys_off_handler *h;
> >> +
> >> +       priv = container_of(nb, struct sys_off_handler_private_data, restart_nb);
> >> +       h = container_of(priv, struct sys_off_handler, priv);
> >> +
> >> +       data.stop_chain = h->restart_chaining_disallowed;
> >> +       data.cb_data = h->cb_data;
> >> +       data.mode = mode;
> >> +       data.cmd = cmd;
> >> +
> >> +       h->restart_cb(&data);
> >
> > Wouldn't it be a bit more straightforward to allow ->restart_cb() to
> > return a value being either NOTIFY_STOP or NOTIFY_DONE?
>
> Vast majority of drivers don't use the 'stop_chain', hence it's
> unnecessary boilerplate code for drivers. The code looks nicer without
> the boilerplate "return NOTIFY_DONE", IMO.

I don't really think that making the callback non-void would be a problem.

> >> +       return data.stop_chain ? NOTIFY_STOP : NOTIFY_DONE;
> >
> > And I would prefer
> >
> > if (data.stop_chain)
> >         return NOTIFY_STOP;
> >
> > return NOTIFY_DONE;
>
> Okay
>
> >> +}
> >> +
> >> +static int sys_off_handler_reboot(struct notifier_block *nb,
> >> +                                 unsigned long mode, void *cmd)
> >> +{
> >> +       struct sys_off_handler_private_data *priv;
> >> +       struct reboot_prep_data data = {};
> >> +       struct sys_off_handler *h;
> >> +
> >> +       priv = container_of(nb, struct sys_off_handler_private_data, reboot_nb);
> >> +       h = container_of(priv, struct sys_off_handler, priv);
> >> +
> >> +       data.cb_data = h->cb_data;
> >> +       data.stop_chain = false;
> >> +       data.mode = mode;
> >> +       data.cmd = cmd;
> >> +
> >> +       h->reboot_prepare_cb(&data);
> >> +
> >> +       return data.stop_chain ? NOTIFY_STOP : NOTIFY_DONE;
> >
> > And analogously here.
> >
> >> +}
> >> +
> >> +static struct sys_off_handler_private_data *
> >> +sys_off_handler_private_data(struct sys_off_handler *handler)
> >> +{
> >> +       return (struct sys_off_handler_private_data *)&handler->priv;
> >
> > Is the cast needed to avoid a warning about "const"?
>
> The private data is constant to prevent the API users from using it. For
> internal use we need to cast out the "const". It's a compilation error
> to change the constant members.

Precisely.  And so it shouldn't be done in any way.

> >> +}
> >> +
> >> +/**
> >> + *     devm_register_sys_off_handler - Register system power-off/restart handler
> >
> > register_sys_off_handler
>
> Good catch!
>
> >> + *     @dev: Device that registers handler
> >> + *     @handler: System-off handler
> >> + *
> >> + *     Registers handler that will be called as last step of the power-off
> >> + *     and restart sequences.
> >
> > Not necessarily as the last step, because there may be other system
> > power-off/restart handlers called after it.
> >
> > I would just say "at the end of the power-off and restart sequences".
> >
> > Moreover, it registers the "reboot_cb" part that is not called "at the
> > end" even.
> >
> > Also, because this is the function that will be used by drivers etc to
> > register handlers, I would give some more information on how the
> > object registered by it is going to be used to the prospective users.
>
> I'll improve the description, thank you.
>
> >> + *
> >> + *     Returns zero on success, or error code on failure.
> >> + */
> >> +int register_sys_off_handler(struct sys_off_handler *handler)
> >> +{
> >> +       struct sys_off_handler_private_data *priv;
> >> +       int err, priority;
> >> +
> >> +       priv = sys_off_handler_private_data(handler);
> >> +
> >> +       /* sanity-check whether handler is registered twice */
> >> +       if (priv->registered)
> >> +               return -EBUSY;
> >> +
> >> +       if (handler->power_off_cb || handler->power_off_prepare_cb) {
> >> +               if (handler->power_off_priority == POWEROFF_PRIO_RESERVED)
> >> +                       priority = POWEROFF_PRIO_DEFAULT;
> >
> > I'm not sure that this helps.
> >
> > I mean, why can't the users of this new API pass POWEROFF_PRIO_DEFAULT
> > directly if they want "default"?
>
> Users could pass POWEROFF_PRIO_DEFAULT, but it's a boilerplate code
> which I wanted to avoid by reserving 0 for the alias to
> POWEROFF_PRIO_DEFAULT=128.
>
> This removes a need from drivers to explicitly specify the default
> priorities in the code, assuming that sys_off_handler is initialized to
> zero.

But the developers need to check whether or not they do the right
thing anyway and so they will need to look at the code here to see
whether or not it is OK to pass 0 and what happens then.  I'm not sure
if I prefer this to the explicit "default".

Or define POWEROFF_PRIO_DEFAULT as 0 and there will be nothing to worry about.

> >> +               else
> >> +                       priority = handler->power_off_priority;
> >> +
> >> +               priv->power_off_nb.notifier_call = sys_off_handler_power_off;
> >> +               priv->power_off_nb.priority = priority;
> >> +
> >> +               err = register_power_off_handler(&priv->power_off_nb);
> >> +               if (err)
> >> +                       goto reset_sys_off_handler;
> >> +       }
> >> +
> >> +       if (handler->restart_cb) {
> >> +               if (handler->restart_priority == RESTART_PRIO_RESERVED)
> >> +                       priority = RESTART_PRIO_DEFAULT;
> >> +               else
> >> +                       priority = handler->restart_priority;
> >> +
> >> +               priv->restart_nb.notifier_call = sys_off_handler_restart;
> >> +               priv->restart_nb.priority = priority;
> >> +
> >> +               err = register_restart_handler(&priv->restart_nb);
> >> +               if (err)
> >> +                       goto unreg_power_off_handler;
> >> +       }
> >> +
> >> +       if (handler->reboot_prepare_cb) {
> >> +               priv->reboot_nb.notifier_call = sys_off_handler_reboot;
> >> +               priv->reboot_nb.priority = handler->reboot_priority;
> >> +
> >> +               err = register_reboot_notifier(&priv->reboot_nb);
> >> +               if (err)
> >> +                       goto unreg_restart_handler;
> >> +       }
> >> +
> >> +       priv->registered = true;
> >> +
> >> +       return 0;
> >> +
> >> +unreg_restart_handler:
> >> +       if (handler->restart_cb)
> >> +               unregister_restart_handler(&priv->restart_nb);
> >> +
> >> +unreg_power_off_handler:
> >> +       if (handler->power_off_cb)
> >> +               unregister_power_off_handler(&priv->power_off_nb);
> >> +
> >> +reset_sys_off_handler:
> >> +       memset(priv, 0, sizeof(*priv));
> >> +
> >> +       return err;
> >> +}
> >> +EXPORT_SYMBOL(register_sys_off_handler);
> >> +
> >> +/**
> >> + *     unregister_sys_off_handler - Unregister system power-off/restart handler
> >> + *     @handler: System-off handler
> >> + *
> >> + *     Unregisters sys-off handler. Does nothing and returns zero if handler
> >> + *     is NULL.
> >> + *
> >> + *     Returns zero on success, or error code on failure.
> >> + */
> >> +int unregister_sys_off_handler(struct sys_off_handler *handler)
> >> +{
> >> +       struct sys_off_handler_private_data *priv;
> >> +
> >> +       if (!handler)
> >> +               return 0;
> >> +
> >> +       priv = sys_off_handler_private_data(handler);
> >> +
> >> +       /* sanity-check whether handler is unregistered twice */
> >> +       if (!priv->registered)
> >> +               return -EINVAL;
> >> +
> >> +       if (handler->reboot_prepare_cb)
> >> +               unregister_reboot_notifier(&priv->reboot_nb);
> >> +
> >> +       if (handler->restart_cb)
> >> +               unregister_restart_handler(&priv->restart_nb);
> >> +
> >> +       if (handler->power_off_cb)
> >> +               unregister_power_off_handler(&priv->power_off_nb);
> >> +
> >> +       memset(priv, 0, sizeof(*priv));
> >> +
> >> +       return 0;
> >> +}
> >> +EXPORT_SYMBOL(unregister_sys_off_handler);
> >> +
> >> +static void devm_unregister_sys_off_handler(void *data)
> >> +{
> >> +       struct sys_off_handler *handler = data;
> >> +
> >> +       unregister_sys_off_handler(handler);
> >> +}
> >> +
> >> +/**
> >> + *     devm_register_sys_off_handler - Register system power-off/restart handler
> >> + *     @dev: Device that registers handler
> >> + *     @handler: System-off handler
> >> + *
> >> + *     Resource-managed variant of register_sys_off_handler().
> >> + *
> >> + *     Returns zero on success, or error code on failure.
> >> + */
> >> +int devm_register_sys_off_handler(struct device *dev,
> >> +                                 struct sys_off_handler *handler)
> >> +{
> >> +       int err;
> >> +
> >> +       err = register_sys_off_handler(handler);
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       return devm_add_action_or_reset(dev, devm_unregister_sys_off_handler,
> >> +                                       handler);
> >> +}
> >> +EXPORT_SYMBOL(devm_register_sys_off_handler);
> >> +
> >> +/**
> >> + *     devm_register_prioritized_power_off_handler - Register prioritized power-off callback
> >> + *     @dev: Device that registers callback
> >> + *     @priority: Callback's priority
> >> + *     @callback: Callback function
> >> + *     @cb_data: Callback's argument
> >> + *
> >> + *     Registers resource-managed power-off callback with a given priority.
> >> + *     It will be called as last step of the power-off sequence. Callbacks
> >> + *     chaining is disabled, i.e. further lower priority callbacks won't
> >> + *     be executed if this @callback will fail to execute.
> >> + *
> >> + *     Returns zero on success, or error code on failure.
> >
> > What's the case in which this should be used instead of registering a
> > full sys_off handler?
>
> There are a lot of drivers that just want to register power-off handler
> with a non-default priority and don't need to stop the chain or do
> anything else special. This is a convinient helper for them.
>
> Please note that the callback here takes only the cb_data for the
> argument, while sys_off uses "struct power_off_data".
>
> Similar for the reset of the convinient helpers.

So if there is a way to do this, why would anyone prefer to use the
full sys_off handler at all?

> ...
> >> +/**
> >> + *     register_platform_power_off - Register platform-level power-off callback
> >> + *     @power_off: Power-off callback
> >> + *
> >> + *     Registers power-off callback that will be called as last step
> >> + *     of the power-off sequence. This callback is expected to be invoked
> >> + *     for the last resort. Further lower priority callbacks won't be
> >> + *     executed if @power_off fails. Only one platform power-off callback
> >> + *     is allowed to be registered at a time.
> >> + *
> >> + *     Returns zero on success, or error code on failure.
> >
> > What's the use case for this?
>
> There are dozens of trivial platform/board-specific power-off handlers
> in the arch/ code. Those handlers don't take any arguments, hence it's a
> convinient helper that transits them to the new API neatly.
>
> >> + */
> >> +int register_platform_power_off(void (*power_off)(void))
> >> +{
> >> +       struct sys_off_handler_private_data *priv;
> >> +       int ret = 0;
> >> +
> >> +       priv = sys_off_handler_private_data(&platform_power_off_handler);
> >> +
> >> +       spin_lock(&platform_power_off_lock);
> >> +       if (priv->platform_power_off_cb)
> >> +               ret = -EBUSY;
> >> +       else
> >> +               priv->platform_power_off_cb = power_off;
> >
> > Wasn't priv supposed to be const?
>
> It's const only for the external API users to prevent them from abusing
> the internal data.

So if this is not clear yet, I'm not going to let this go.

> >> +       spin_unlock(&platform_power_off_lock);
> >> +
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       ret = register_power_off_handler(&priv->power_off_nb);
> >> +       if (ret)
> >> +               priv->platform_power_off_cb = NULL;
> >> +
> >> +       return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(register_platform_power_off);
> >> +
> >> +/**
> >> + *     unregister_platform_power_off - Unregister platform-level power-off callback
> >> + *     @power_off: Power-off callback
> >> + *
> >> + *     Unregisters previously registered platform power-off callback.
> >> + *
> >> + *     Returns zero on success, or error code on failure.
> >> + */
> >> +int unregister_platform_power_off(void (*power_off)(void))
> >> +{
> >> +       struct sys_off_handler_private_data *priv;
> >> +       int ret;
> >> +
> >> +       priv = sys_off_handler_private_data(&platform_power_off_handler);
> >> +
> >> +       if (priv->platform_power_off_cb != power_off)
> >> +               return -EINVAL;
> >> +
> >> +       ret = unregister_power_off_handler(&priv->power_off_nb);
> >> +       priv->platform_power_off_cb = NULL;
> >> +
> >> +       return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(unregister_platform_power_off);
> >> +
> >> +/**
> >> + *     do_kernel_power_off - Execute kernel power-off handler call chain
> >> + *
> >> + *     Calls functions registered with register_power_off_handler.
> >> + *
> >> + *     Expected to be called as last step of the power-off sequence.
> >> + *
> >> + *     Powers off the system immediately if a power-off handler function has
> >> + *     been registered. Otherwise does nothing.
> >> + */
> >> +void do_kernel_power_off(void)
> >> +{
> >> +       /* legacy pm_power_off() is unchained and has highest priority */
> >> +       if (pm_power_off && pm_power_off != dummy_pm_power_off)
> >> +               return pm_power_off();
> >> +
> >> +       blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_NORMAL,
> >> +                                    NULL);
> >> +}
> >> +
> >> +static void do_kernel_power_off_prepare(void)
> >> +{
> >> +       /* legacy pm_power_off_prepare() is unchained and has highest priority */
> >> +       if (pm_power_off_prepare)
> >> +               return pm_power_off_prepare();
> >> +
> >> +       blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_PREPARE,
> >> +                                    NULL);
> >> +}
> >> +
> >>  /**
> >>   *     kernel_power_off - power_off the system
> >>   *
> >> @@ -304,8 +893,7 @@ EXPORT_SYMBOL_GPL(kernel_halt);
> >>  void kernel_power_off(void)
> >>  {
> >>         kernel_shutdown_prepare(SYSTEM_POWER_OFF);
> >> -       if (pm_power_off_prepare)
> >> -               pm_power_off_prepare();
> >> +       do_kernel_power_off_prepare();
> >>         migrate_to_reboot_cpu();
> >>         syscore_shutdown();
> >>         pr_emerg("Power down\n");
> >> @@ -314,6 +902,16 @@ void kernel_power_off(void)
> >>  }
> >>  EXPORT_SYMBOL_GPL(kernel_power_off);
> >>
> >> +bool kernel_can_power_off(void)
> >> +{
> >> +       if (!pm_power_off &&
> >> +           blocking_notifier_call_chain_is_empty(&power_off_handler_list))
> >> +               return false;
> >> +
> >> +       return true;
> >
> > return pm_power_off ||
> > blocking_notifier_call_chain_is_empty(&power_off_handler_list);
>
> Thank you for the thorough review!

You're very welcome!

Re: [PATCH v7 04/20] kernel: Add combined power-off+restart handler call chain API

[Dmitry Osipenko]

On 4/20/22 21:47, Rafael J. Wysocki wrote:
>>>> +       POWEROFF_PREPARE,
>>>> +};
>>>> +
>>>> +/**
>>>> + * 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 does this need to be a separate data type?
>> To allow us extend the "struct power_off_prep_data" with more parameters
>> later on without a need to update each driver with the new arguments.

> I'm not really sure what you mean here.  Can you give an example?
> 

The restart callbacks use more than the cb_data and we have:

struct restart_data {
	void *cb_data;
	const char *cmd;
	bool stop_chain;
	enum reboot_mode mode;
};

If we'll ever need to extended struct power_off_data similarly to the
restart_data, then we will need to update all the power-off callbacks
instead of adding a new field to the power_off_data.

Hence, for example, if you'll want to extend power_off_data with "enum
poweroff_mode mode", then for each driver you'll need to do this change:

-power_off(void *cb_data)
+power_off(void *cb_data, enum poweroff_mode mode)

and you won't need to do that using struct power_off_data.

Why do we need this? Because I saw in the past people changing kernel
APIs that way when they wanted to add new arguments and then needed to
update every call site around the kernel.

-- 
Best regards,
Dmitry

Re: [PATCH v7 04/20] kernel: Add combined power-off+restart handler call chain API

[Dmitry Osipenko]

On 4/20/22 21:47, Rafael J. Wysocki wrote:
>>>> +/**
>>>> + *     devm_register_prioritized_power_off_handler - Register prioritized power-off callback
>>>> + *     @dev: Device that registers callback
>>>> + *     @priority: Callback's priority
>>>> + *     @callback: Callback function
>>>> + *     @cb_data: Callback's argument
>>>> + *
>>>> + *     Registers resource-managed power-off callback with a given priority.
>>>> + *     It will be called as last step of the power-off sequence. Callbacks
>>>> + *     chaining is disabled, i.e. further lower priority callbacks won't
>>>> + *     be executed if this @callback will fail to execute.
>>>> + *
>>>> + *     Returns zero on success, or error code on failure.
>>> What's the case in which this should be used instead of registering a
>>> full sys_off handler?
>> There are a lot of drivers that just want to register power-off handler
>> with a non-default priority and don't need to stop the chain or do
>> anything else special. This is a convinient helper for them.
>>
>> Please note that the callback here takes only the cb_data for the
>> argument, while sys_off uses "struct power_off_data".
>>
>> Similar for the reset of the convinient helpers.
> So if there is a way to do this, why would anyone prefer to use the
> full sys_off handler at all?

There are couple occurrences around kernel where there is no device
available for the devm_ functions, like this for example [1].

[1]
https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/commit/184dfd3983e774d3cf9050dc5b4ec23a662a9551

-- 
Best regards,
Dmitry

Re: [PATCH v7 04/20] kernel: Add combined power-off+restart handler call chain API

[Dmitry Osipenko]

On 4/20/22 21:47, Rafael J. Wysocki wrote:
>>>> +       void (*platform_power_off_cb)(void);
>>>> +       void (*simple_power_off_cb)(void *data);
>>>> +       void *simple_power_off_cb_data;
>>> Is there any particular reason to put these callbacks here and not
>>> directly into struct sys_off_handler?
>> They are used internally by reboot.c and shouldn't be touched by the
>> kernel API users, so I wanted to factor out and protect all the private
>> data.
> But they come from these users anyway, don't they?
> 

Yes, the point is that these callback pointers are owned by the kernel
core while callbacks are registered.

-- 
Best regards,
Dmitry

Re: [PATCH v7 04/20] kernel: Add combined power-off+restart handler call chain API

[Dmitry Osipenko]

On 4/20/22 21:47, Rafael J. Wysocki wrote:
>>>> +       spin_unlock(&platform_power_off_lock);
>>>> +
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       ret = register_power_off_handler(&priv->power_off_nb);
>>>> +       if (ret)
>>>> +               priv->platform_power_off_cb = NULL;
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(register_platform_power_off);
>>>> +
>>>> +/**
>>>> + *     unregister_platform_power_off - Unregister platform-level power-off callback
>>>> + *     @power_off: Power-off callback
>>>> + *
>>>> + *     Unregisters previously registered platform power-off callback.
>>>> + *
>>>> + *     Returns zero on success, or error code on failure.
>>>> + */
>>>> +int unregister_platform_power_off(void (*power_off)(void))
>>>> +{
>>>> +       struct sys_off_handler_private_data *priv;
>>>> +       int ret;
>>>> +
>>>> +       priv = sys_off_handler_private_data(&platform_power_off_handler);
>>>> +
>>>> +       if (priv->platform_power_off_cb != power_off)
>>>> +               return -EINVAL;
>>>> +
>>>> +       ret = unregister_power_off_handler(&priv->power_off_nb);
>>>> +       priv->platform_power_off_cb = NULL;
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(unregister_platform_power_off);
>>>> +
>>>> +/**
>>>> + *     do_kernel_power_off - Execute kernel power-off handler call chain
>>>> + *
>>>> + *     Calls functions registered with register_power_off_handler.
>>>> + *
>>>> + *     Expected to be called as last step of the power-off sequence.
>>>> + *
>>>> + *     Powers off the system immediately if a power-off handler function has
>>>> + *     been registered. Otherwise does nothing.
>>>> + */
>>>> +void do_kernel_power_off(void)
>>>> +{
>>>> +       /* legacy pm_power_off() is unchained and has highest priority */
>>>> +       if (pm_power_off && pm_power_off != dummy_pm_power_off)
>>>> +               return pm_power_off();
>>>> +
>>>> +       blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_NORMAL,
>>>> +                                    NULL);
>>>> +}
>>>> +
>>>> +static void do_kernel_power_off_prepare(void)
>>>> +{
>>>> +       /* legacy pm_power_off_prepare() is unchained and has highest priority */
>>>> +       if (pm_power_off_prepare)
>>>> +               return pm_power_off_prepare();
>>>> +
>>>> +       blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_PREPARE,
>>>> +                                    NULL);
>>>> +}
>>>> +
>>>>  /**
>>>>   *     kernel_power_off - power_off the system
>>>>   *
>>>> @@ -304,8 +893,7 @@ EXPORT_SYMBOL_GPL(kernel_halt);
>>>>  void kernel_power_off(void)
>>>>  {
>>>>         kernel_shutdown_prepare(SYSTEM_POWER_OFF);
>>>> -       if (pm_power_off_prepare)
>>>> -               pm_power_off_prepare();
>>>> +       do_kernel_power_off_prepare();
>>>>         migrate_to_reboot_cpu();
>>>>         syscore_shutdown();
>>>>         pr_emerg("Power down\n");
>>>> @@ -314,6 +902,16 @@ void kernel_power_off(void)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(kernel_power_off);
>>>>
>>>> +bool kernel_can_power_off(void)
>>>> +{
>>>> +       if (!pm_power_off &&
>>>> +           blocking_notifier_call_chain_is_empty(&power_off_handler_list))
>>>> +               return false;
>>>> +
>>>> +       return true;
>>> return pm_power_off ||
>>> blocking_notifier_call_chain_is_empty(&power_off_handler_list);
>> Thank you for the thorough review!
> You're very welcome!

Thanks again for taking a look at the patches. I don't have strong
preferences about the names and etc, so I'll update it all in v8 like
you suggested.

-- 
Best regards,
Dmitry