[RFC 0/3] Add system power and restart framework

[RFC 0/3] Add system power and restart framework

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi everyone,

This series of patches proposes a small framework targetted at system
power and restart drivers. Restart drivers currently use a notifier
chain and there was an attempt by Guenter Roeck a while ago to move
power off drivers to similar infrastructure[0]. That attempt had met
with some pushback, with the main criticism being that there was no
formal definition of the priorities of these handlers.

The system power and restart framework tries to solve this by adding
a more explicit framework that power and restart drivers can register
with. This is currently very simple, but it is meant primarily as a
basis for discussion so that we can reach concensus on what we want
such a framework to look like (and if we need one at all).

There was a bit of discussion on this two weeks ago[1], and this set
is an attempt at implementing my proposal from that discussion[2]. A
formal mechanism to implement priorities on top of this could easily
be added (see linked discussion).

One very big advantage of this method is that we have a very easy way
of keeping backwards compatibility with the restart notifier chain and
the pm_power_off() mechanism, which means we can convert drivers one
by one and evolve the framework incrementally without having to have a
flag day where everyone needs to convert.

So the goal of this series is to get some feedback on whether or not the
people involved in earlier discussions around this think this is sound.
If so I've got a couple of patches on top that convert architectures
over to using the new function calls and a couple of drivers that I have
converted as a proof of concept.

Thanks,
Thierry

[0]: https://lkml.org/lkml/2014/11/6/505
[1]: https://lkml.org/lkml/2017/1/12/470
[2]: https://lkml.org/lkml/2017/1/20/89

Thierry Reding (3):
  system-power: Add system power and restart framework
  kernel: Wire up system power framework
  PM / hibernate: Wire up system-power framework

 drivers/base/Makefile        |   3 +-
 drivers/base/system-power.c  | 110 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/system-power.h |  38 +++++++++++++++
 kernel/power/hibernate.c     |   3 +-
 kernel/reboot.c              |  11 +++--
 5 files changed, 158 insertions(+), 7 deletions(-)
 create mode 100644 drivers/base/system-power.c
 create mode 100644 include/linux/system-power.h

-- 
2.11.0

[RFC 1/3] system-power: Add system power and restart framework

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This adds a very simple framework that allows drivers to register system
power and restart controllers. The goal of this framework is to replace
the current notifier based mechanism for restart handlers and the power
off equivalent that is the global pm_power_off() function.

Both of these approaches currently lack any means of locking against
concurrently registering handlers or formal definitions on what proper
priorities are to order handlers.

The system-power framework attempts to remedy this by adding a system
power chip object that drivers can embed in their driver-specific data.
A chip contains a description of capabilities that the framework uses
to determine a good sequence of handlers to use for restart and power
off.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/base/Makefile        |   3 +-
 drivers/base/system-power.c  | 110 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/system-power.h |  38 +++++++++++++++
 3 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/system-power.c
 create mode 100644 include/linux/system-power.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index f2816f6ff76a..eef165221d9d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -4,7 +4,8 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o property.o cacheinfo.o
+			   topology.o container.o property.o cacheinfo.o \
+			   system-power.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/system-power.c b/drivers/base/system-power.c
new file mode 100644
index 000000000000..96c0cb457933
--- /dev/null
+++ b/drivers/base/system-power.c
@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2017 NVIDIA Corporation
+ *
+ * This file is released under the GPL v2
+ */
+
+#define pr_fmt(fmt) "system-power: " fmt
+
+#include <linux/system-power.h>
+
+static DEFINE_MUTEX(system_power_lock);
+static LIST_HEAD(system_power_chips);
+
+int system_power_chip_add(struct system_power_chip *chip)
+{
+	if (!chip->ops || (!chip->ops->restart && !chip->ops->power_off)) {
+		WARN(1, pr_fmt("must implement restart or power off\n"));
+		return -EINVAL;
+	}
+
+	mutex_lock(&system_power_lock);
+
+	INIT_LIST_HEAD(&chip->list);
+	list_add_tail(&chip->list, &system_power_chips);
+
+	mutex_unlock(&system_power_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(system_power_chip_add);
+
+int system_power_chip_remove(struct system_power_chip *chip)
+{
+	mutex_lock(&system_power_lock);
+
+	list_del_init(&chip->list);
+
+	mutex_unlock(&system_power_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(system_power_chip_remove);
+
+bool system_can_power_off(void)
+{
+	/* XXX for backwards compatibility */
+	return pm_power_off != NULL;
+}
+
+int system_restart(char *cmd)
+{
+	struct system_power_chip *chip;
+	int err;
+
+	mutex_lock(&system_power_lock);
+
+	list_for_each_entry(chip, &system_power_chips, list) {
+		if (!chip->ops->restart)
+			continue;
+
+		pr_debug("trying to restart using %ps\n", chip);
+
+		err = chip->ops->restart(chip, reboot_mode, cmd);
+		if (err < 0)
+			dev_warn(chip->dev, "failed to restart: %d\n", err);
+	}
+
+	mutex_unlock(&system_power_lock);
+
+	/* XXX for backwards compatibility */
+	do_kernel_restart(cmd);
+
+	return 0;
+}
+
+int system_power_off_prepare(void)
+{
+	/* XXX for backwards compatibility */
+	if (pm_power_off_prepare)
+		pm_power_off_prepare();
+
+	return 0;
+}
+
+int system_power_off(void)
+{
+	struct system_power_chip *chip;
+	int err;
+
+	mutex_lock(&system_power_lock);
+
+	list_for_each_entry(chip, &system_power_chips, list) {
+		if (!chip->ops->power_off)
+			continue;
+
+		pr_debug("trying to power off using %ps\n", chip);
+
+		err = chip->ops->power_off(chip);
+		if (err < 0)
+			dev_warn(chip->dev, "failed to power off: %d\n", err);
+	}
+
+	mutex_unlock(&system_power_lock);
+
+	/* XXX for backwards compatibility */
+	if (pm_power_off)
+		pm_power_off();
+
+	return 0;
+}
diff --git a/include/linux/system-power.h b/include/linux/system-power.h
new file mode 100644
index 000000000000..f709c14c1552
--- /dev/null
+++ b/include/linux/system-power.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2017 NVIDIA Corporation
+ *
+ * This file is released under the GPL v2
+ */
+
+#ifndef SYSTEM_POWER_H
+#define SYSTEM_POWER_H
+
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/reboot.h>
+
+struct system_power_chip;
+
+struct system_power_ops {
+	int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
+		       char *cmd);
+	int (*power_off_prepare)(struct system_power_chip *chip);
+	int (*power_off)(struct system_power_chip *chip);
+};
+
+struct system_power_chip {
+	const struct system_power_ops *ops;
+	struct list_head list;
+	struct device *dev;
+};
+
+int system_power_chip_add(struct system_power_chip *chip);
+int system_power_chip_remove(struct system_power_chip *chip);
+
+bool system_can_power_off(void);
+
+int system_restart(char *cmd);
+int system_power_off_prepare(void);
+int system_power_off(void);
+
+#endif
-- 
2.11.0

[RFC 2/3] kernel: Wire up system power framework

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Instead of relying on the globally defined pm_power_off_prepare() and
pm_power_off() function pointers, use the equivalent global functions
from the system-power framework.

The system-power framework implements a fallback that relies on these
global functions in case no system power chips have been registered.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 kernel/reboot.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index bd30a973fe94..ea075e285832 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -15,6 +15,7 @@
 #include <linux/suspend.h>
 #include <linux/syscalls.h>
 #include <linux/syscore_ops.h>
+#include <linux/system-power.h>
 #include <linux/uaccess.h>
 
 /*
@@ -257,8 +258,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();
+	system_power_off_prepare();
 	migrate_to_reboot_cpu();
 	syscore_shutdown();
 	pr_emerg("Power down\n");
@@ -305,10 +305,11 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 	if (ret)
 		return ret;
 
-	/* Instead of trying to make the power_off code look like
-	 * halt when pm_power_off is not set do it the easy way.
+	/*
+	 * Instead of trying to make the power_off code look like halt when
+	 * power off is not supported do it the easy way.
 	 */
-	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
+	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !system_can_power_off())
 		cmd = LINUX_REBOOT_CMD_HALT;
 
 	mutex_lock(&reboot_mutex);
-- 
2.11.0

[RFC 3/3] PM / hibernate: Wire up system-power framework

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use the system-power framework's equivalent to test for power off
capability instead of relying on the globally defined pm_power_off()
function pointer.

The system-power framework implements a fallback that relies on this
global function in case no system power chips have been registered.

Moving this to the system-power framework allows us to eventually
remove any traces of pm_power_off() once all handlers have moved over
to the new framework.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 kernel/power/hibernate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index b26dbc48c75b..e7429ea11e9a 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -30,6 +30,7 @@
 #include <linux/genhd.h>
 #include <linux/ktime.h>
 #include <trace/events/power.h>
+#include <linux/system-power.h>
 
 #include "power.h"
 
@@ -617,7 +618,7 @@ static void power_down(void)
 	case HIBERNATION_PLATFORM:
 		hibernation_platform_enter();
 	case HIBERNATION_SHUTDOWN:
-		if (pm_power_off)
+		if (system_can_power_off())
 			kernel_power_off();
 		break;
 #ifdef CONFIG_SUSPEND
-- 
2.11.0

Re: [RFC 1/3] system-power: Add system power and restart framework

[Pavel Machek]

Hi!


> +struct system_power_chip;
> +
> +struct system_power_ops {
> +	int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> +		       char *cmd);
> +	int (*power_off_prepare)(struct system_power_chip *chip);
> +	int (*power_off)(struct system_power_chip *chip);
> +};
> +
> +struct system_power_chip {
> +	const struct system_power_ops *ops;
> +	struct list_head list;
> +	struct device *dev;
> +};

Is it useful to have two structures? AFAICT one would do.

Do we always have struct device * to work with? IMO we have nothing
suitable for example in the ACPI case. Would void * be more suitable?

Could you convert someting (acpi?) to the new framework as
demonstration?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 1/3] system-power: Add system power and restart framework

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2953 bytes --]

On Mon, Jan 30, 2017 at 10:53:01PM +0100, Pavel Machek wrote:
> Hi!
> 
> 
> > +struct system_power_chip;
> > +
> > +struct system_power_ops {
> > +	int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> > +		       char *cmd);
> > +	int (*power_off_prepare)(struct system_power_chip *chip);
> > +	int (*power_off)(struct system_power_chip *chip);
> > +};
> > +
> > +struct system_power_chip {
> > +	const struct system_power_ops *ops;
> > +	struct list_head list;
> > +	struct device *dev;
> > +};
> 
> Is it useful to have two structures? AFAICT one would do.

Yeah, one structure works fine. I was drawing inspiration from other
subsystems that have a separate structure for these. I've merged the
operations into the struct system_power_chip now because that gives
us some more flexiblity, for example in cases where a chip can be a
power controller and a reset controller, but sometimes we may want
it to be only one of them.

> Do we always have struct device * to work with? IMO we have nothing
> suitable for example in the ACPI case. Would void * be more suitable?

The struct device * was meant to be purely optional, but working with
the code some more today and doing some more conversions, I've resorted
to adding a separate field (const char *name) that takes precedence. So
if a chip specifies both a .dev and .name field, then .name will be the
user visible string, otherwise dev_name(.dev) will be used in messages.

> Could you convert someting (acpi?) to the new framework as
> demonstration?

I had originally only converted architecture code to call into system
power instead of the notifier chain and added a driver for a chip that
I want to get this to work on. I've now converted a couple of other
drivers from drivers/power/reset as well as ACPI. I've also added a
very rudimentary prioritization mechanism that I've validated on the
specific setup that I'm working on.

On the Jetson TX1 that I'm testing this on, the SoC has a way of
resetting itself. This has the advantage that some of the registers are
kept intact over the reset, and this in turn is used to control early
boot, so that specific recovery modes can be used. However, the board
has to be powered off using the PMIC (via I2C). The patches achieve this
by splitting up restart and power off into two steps, prepare and
restart/power-off, as well as levels to prioritize. On Jetson TX1 the
PMIC will be higher priority than the SoC (determined by the level) and
therefore be able to override the SoC restart mechanism if we want to.
If we don't we simply instruct the MAX77620 driver not to register the
restart callback, in which case the SoC implementation will be used.

I've uploaded all of it to a branch on github:

	https://github.com/thierryreding/linux/tree/system-power

It's rather lengthy, so I'm not sure if it makes sense to send to the
lists right away.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 1/3] system-power: Add system power and restart framework

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3355 bytes --]

On Tue 2017-01-31 18:46:58, Thierry Reding wrote:
> On Mon, Jan 30, 2017 at 10:53:01PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > 
> > > +struct system_power_chip;
> > > +
> > > +struct system_power_ops {
> > > +	int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> > > +		       char *cmd);
> > > +	int (*power_off_prepare)(struct system_power_chip *chip);
> > > +	int (*power_off)(struct system_power_chip *chip);
> > > +};
> > > +
> > > +struct system_power_chip {
> > > +	const struct system_power_ops *ops;
> > > +	struct list_head list;
> > > +	struct device *dev;
> > > +};
> > 
> > Is it useful to have two structures? AFAICT one would do.
> 
> Yeah, one structure works fine. I was drawing inspiration from other
> subsystems that have a separate structure for these. I've merged the
> operations into the struct system_power_chip now because that gives
> us some more flexiblity, for example in cases where a chip can be a
> power controller and a reset controller, but sometimes we may want
> it to be only one of them.
> 
> > Do we always have struct device * to work with? IMO we have nothing
> > suitable for example in the ACPI case. Would void * be more suitable?
> 
> The struct device * was meant to be purely optional, but working with
> the code some more today and doing some more conversions, I've resorted
> to adding a separate field (const char *name) that takes precedence. So
> if a chip specifies both a .dev and .name field, then .name will be the
> user visible string, otherwise dev_name(.dev) will be used in
> messages.

Thanks!

> > Could you convert someting (acpi?) to the new framework as
> > demonstration?
> 
> I had originally only converted architecture code to call into system
> power instead of the notifier chain and added a driver for a chip that
> I want to get this to work on. I've now converted a couple of other
> drivers from drivers/power/reset as well as ACPI. I've also added a
> very rudimentary prioritization mechanism that I've validated on the
> specific setup that I'm working on.
> 
> On the Jetson TX1 that I'm testing this on, the SoC has a way of
> resetting itself. This has the advantage that some of the registers are
> kept intact over the reset, and this in turn is used to control early
> boot, so that specific recovery modes can be used. However, the board
> has to be powered off using the PMIC (via I2C). The patches achieve this
> by splitting up restart and power off into two steps, prepare and
> restart/power-off, as well as levels to prioritize. On Jetson TX1 the
> PMIC will be higher priority than the SoC (determined by the level) and
> therefore be able to override the SoC restart mechanism if we want to.
> If we don't we simply instruct the MAX77620 driver not to register the
> restart callback, in which case the SoC implementation will be used.
> 
> I've uploaded all of it to a branch on github:
> 
> 	https://github.com/thierryreding/linux/tree/system-power
> 
> It's rather lengthy, so I'm not sure if it makes sense to send to the
> lists right away.

It is easier to review on lists, but no reasons to do it now.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]