[PATCH v4 0/7] kernel: Add support for restart notifier call chain

[PATCH v4 0/7] kernel: Add support for restart notifier call chain

[Guenter Roeck]

Various drivers implement architecture and/or device specific means
to restart (reset) the system. Various mechanisms have been implemented
to support those schemes. The best known mechanism is arm_pm_restart,
which is a function pointer to be set either from platform specific code
or from drivers. Another mechanism is to use hardware watchdogs to issue
a reset; this mechanism is used if there is no other method available
to reset a board or system. Two examples are alim7101_wdt, which currently
uses the reboot notifier to trigger a reset, and moxart_wdt, which registers
the arm_pm_restart function. Several other restart drivers for arm, all
directly calling arm_pm_restart, are in the process of being integrated
into the kernel. All those drivers would benefit from the new API.

The existing mechanisms have a number of drawbacks. Typically only one scheme
to restart the system is supported (at least if arm_pm_restart is used).
At least in theory there can be multiple means to restart the system, some of
which may be less desirable (for example one mechanism may only reset the CPU,
while another may reset the entire system). Using arm_pm_restart can also be
racy if the function pointer is set from a driver, as the driver may be in
the process of being unloaded when arm_pm_restart is called.
Using the reboot notifier is always racy, as it is unknown if and when
other functions using the reboot notifier have completed execution
by the time the watchdog fires.

Introduce a system restart notifier to solve the described problems.
This notifier is expected to be called from the architecture specific
machine_restart() function. Drivers providing system restart functionality
(such as the watchdog drivers mentioned above) are expected to register
with this notifier. By using the priority field in the notifier block,
callers can control notifier execution sequence and thus ensure that the
notifier with the optimal restart capabilities for a given system
is called first.

Since the first revision of this patchset, a number of separate patch
submissions have been made which either depend on it or could make use of it.

http://www.spinics.net/linux/lists/arm-kernel/msg344796.html
	registers three notifiers.
https://lkml.org/lkml/2014/7/8/962
	would benefit from it.

Patch 1 of this series implements the notifier function. Patches 2 and 3
implement calling the notifier chain from arm and arm64 restart code.

Patch 4 modifies the restart-poweroff driver to no longer call arm_pm_restart
directly but machine_restart. This is done to avoid calling arm_pm_restart
from more than one place. The change makes the driver architecture independent,
so it would be possible to drop the arm dependency from its Kconfig entry.

Patch 5 and 6 convert existing restart handlers in the watchdog subsystem
to use the restart notifier. Patch 7 unexports arm_pm_restart to ensure
that no one gets the idea to implement a restart handler as module.

---
v4: Document restart notifier priorities
    Select 128 as default priority for newly introduced notifiers
    Fix checkpatch warning (line too long) in moxart patch
v3: Drop RFC.
    Add kernel_restart_notify wrapper function to execute notifier
    Improve documentation.
    Move restart_notifier_list into kernel/reboot.c and make it static.
v2: Add patch 4.
    Only call blocking notifier call chain if arm_pm_restart was not set.

[PATCH v4 1/7] kernel: Add support for restart notifier call chain

[Guenter Roeck]

Various drivers implement architecture and/or device specific means
to restart (reset) the system. Various mechanisms have been implemented
to support those schemes. The best known mechanism is arm_pm_restart,
which is a function pointer to be set either from platform specific code
or from drivers. Another mechanism is to use hardware watchdogs to issue
a reset; this mechanism is used if there is no other method available
to reset a board or system. Two examples are alim7101_wdt, which currently
uses the reboot notifier to trigger a reset, and moxart_wdt, which registers
the arm_pm_restart function.

The existing mechanisms have a number of drawbacks. Typically only one scheme
to restart the system is supported (at least if arm_pm_restart is used).
At least in theory there can be multiple means to restart the system, some of
which may be less desirable (for example one mechanism may only reset the CPU,
while another may reset the entire system). Using arm_pm_restart can also be
racy if the function pointer is set from a driver, as the driver may be in
the process of being unloaded when arm_pm_restart is called.
Using the reboot notifier is always racy, as it is unknown if and when
other functions using the reboot notifier have completed execution
by the time the watchdog fires.

Introduce a system restart notifier to solve the described problems.
This notifier is expected to be called from the architecture specific
machine_restart() function. Drivers providing system restart functionality
(such as the watchdog drivers mentioned above) are expected to register
with this notifier. By using the priority field in the notifier block,
callers can control notifier execution sequence and thus ensure that the
notifier with the optimal restart capabilities for a given system
is called first.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v4: Document and suggest values for notifier priorities
v3: Add kernel_restart_notify wrapper function to execute notifier.
    Improve documentation.
    Move restart_notifier_list into kernel/reboot.c and make it static.
v2: No change.

 include/linux/reboot.h |  3 ++
 kernel/reboot.c        | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index 48bf152..120db73 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -38,6 +38,9 @@ extern int reboot_force;
 extern int register_reboot_notifier(struct notifier_block *);
 extern int unregister_reboot_notifier(struct notifier_block *);
 
+extern int register_restart_notifier(struct notifier_block *);
+extern int unregister_restart_notifier(struct notifier_block *);
+extern void kernel_restart_notify(char *cmd);
 
 /*
  * Architecture-specific implementations of sys_reboot commands.
diff --git a/kernel/reboot.c b/kernel/reboot.c
index a3a9e24..1bc9bf2 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -104,6 +104,87 @@ int unregister_reboot_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_reboot_notifier);
 
+/*
+ *	Notifier list for kernel code which wants to be called
+ *	to restart the system.
+ */
+static BLOCKING_NOTIFIER_HEAD(restart_notifier_list);
+
+/**
+ *	register_restart_notifier - Register function to be called to reset
+ *				    the system
+ *	@nb: Info about notifier function to be called
+ *	@nb->priority:	Notifier priority. Notifiers should follow the
+ *			following guidelines for setting priorities.
+ *			0:	Restart notifier of last resort,
+ *				with limited restart capabilities
+ *			128:	Default notifier; use if no other
+ *				notifier is expected to be available,
+ *				and/or if restart functionality is
+ *				sufficient to restart the entire system
+ *			255:	Highest priority notifier, will preempt
+ *				all other notifier functions
+ *
+ *	Registers a function with the list of functions
+ *	to be called to restart the system.
+ *
+ *	Registered functions will be called from machine_restart as last
+ *	step of the restart sequence (if the architecture specific
+ *	machine_restart function calls kernel_restart_notify - see below
+ *	for details).
+ *	Registered functions are expected to restart the system immediately.
+ *	If more than one function is registered, the notifier priority
+ *	selects which function will be called first.
+ *
+ *	Restart notifiers are expected to be registered from non-architecture
+ *	code, typically from drivers. A typical use case would be a system
+ *	where restart functionality is provided through a watchdog. Multiple
+ *	restart handlers may exist; for example, one restart handler might
+ *	restart the entire system, while another only restarts the CPU.
+ *	In such cases, the restart handler which only restarts part of the
+ *	hardware is expected to register with low priority to ensure that
+ *	it only runs if no other means to restart the system is available.
+ *
+ *	Currently always returns zero, as blocking_notifier_chain_register()
+ *	always returns zero.
+ */
+int register_restart_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&restart_notifier_list, nb);
+}
+EXPORT_SYMBOL(register_restart_notifier);
+
+/**
+ *	unregister_restart_notifier - Unregister previously registered
+ *				      restart notifier
+ *	@nb: Hook to be unregistered
+ *
+ *	Unregisters a previously registered restart notifier function.
+ *
+ *	Returns zero on success, or %-ENOENT on failure.
+ */
+int unregister_restart_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&restart_notifier_list, nb);
+}
+EXPORT_SYMBOL(unregister_restart_notifier);
+
+/**
+ *	kernel_restart_notify - Execute kernel restart handler call chain
+ *
+ *	Calls functions registered with register_restart_notifier.
+ *
+ *	Expected to be called from machine_restart as last step of the restart
+ *	sequence.
+ *
+ *	Restarts the system immediately if a restart notifier function has been
+ *	registered. Otherwise does nothing.
+ */
+void kernel_restart_notify(char *cmd)
+{
+	blocking_notifier_call_chain(&restart_notifier_list, reboot_mode, cmd);
+}
+
 void migrate_to_reboot_cpu(void)
 {
 	/* The boot cpu is always logical cpu 0 */
-- 
1.9.1

[PATCH v4 2/7] arm64: Support restart through restart notifier call chain

[Guenter Roeck]

The kernel core now supports a notifier call chain to restart
the system. Call it if arm_pm_restart is not set.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v4: No change.
v3: Use wrapper function to execute notifier call chain.
v2: Only call notifier call chain if arm_pm_restart is not set.
    Do not include linux/watchdog.h.

 arch/arm64/kernel/process.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 43b7c34..b2da6d5 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -174,6 +174,8 @@ void machine_restart(char *cmd)
 	/* Now call the architecture specific reboot code. */
 	if (arm_pm_restart)
 		arm_pm_restart(reboot_mode, cmd);
+	else
+		kernel_restart_notify(cmd);
 
 	/*
 	 * Whoops - the architecture was unable to reboot.
-- 
1.9.1

[PATCH v4 3/7] arm: Support restart through restart notifier call chain

[Guenter Roeck]

The kernel core now supports a notifier call chain for system
restart functions.

With this change, the arm_pm_restart callback is now optional,
so check if it is set before calling it. Only call the notifier
functions if arm_pm_restart is not set.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v4: No change.
v3: Use wrapper function to execute notifier call chain.
v2: Only call notifier call chain if arm_pm_restart is not set.
    Do not include linux/watchdog.h.

 arch/arm/kernel/process.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 81ef686..5d191e3 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -230,7 +230,10 @@ void machine_restart(char *cmd)
 	local_irq_disable();
 	smp_send_stop();
 
-	arm_pm_restart(reboot_mode, cmd);
+	if (arm_pm_restart)
+		arm_pm_restart(reboot_mode, cmd);
+	else
+		kernel_restart_notify(cmd);
 
 	/* Give a grace period for failure to restart of 1s */
 	mdelay(1000);
-- 
1.9.1

[PATCH v4 4/7] power/restart: Call machine_restart instead of arm_pm_restart

[Guenter Roeck]

machine_restart is supported on non-ARM platforms, and and ultimately calls
arm_pm_restart, so dont call arm_pm_restart directly but use the more
generic function.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v4: No change.
v3: No change.
v2: Added patch.

 drivers/power/reset/restart-poweroff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/power/reset/restart-poweroff.c b/drivers/power/reset/restart-poweroff.c
index 5758033..47f10eb 100644
--- a/drivers/power/reset/restart-poweroff.c
+++ b/drivers/power/reset/restart-poweroff.c
@@ -20,7 +20,8 @@
 
 static void restart_poweroff_do_poweroff(void)
 {
-	arm_pm_restart(REBOOT_HARD, NULL);
+	reboot_mode = REBOOT_HARD;
+	machine_restart(NULL);
 }
 
 static int restart_poweroff_probe(struct platform_device *pdev)
-- 
1.9.1

[PATCH v4 5/7] watchdog: moxart: Register restart handler with restart notifier

[Guenter Roeck]

The kernel now provides an API to trigger a system restart.
Register with it instead of setting arm_pm_restart.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v4: Set notifier priority to 128.
v3: Move struct notifier_block into struct moxart_wdt_dev.
    Drop static variable previously needed to access struct moxart_wdt_dev
    from notifier function; use container_of instead.
v2: No change.

 drivers/watchdog/moxart_wdt.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/moxart_wdt.c b/drivers/watchdog/moxart_wdt.c
index 4aa3a8a..5080b50 100644
--- a/drivers/watchdog/moxart_wdt.c
+++ b/drivers/watchdog/moxart_wdt.c
@@ -15,12 +15,12 @@
 #include <linux/module.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
+#include <linux/notifier.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/watchdog.h>
 #include <linux/moduleparam.h>
 
-#include <asm/system_misc.h>
-
 #define REG_COUNT			0x4
 #define REG_MODE			0x8
 #define REG_ENABLE			0xC
@@ -29,17 +29,22 @@ struct moxart_wdt_dev {
 	struct watchdog_device dev;
 	void __iomem *base;
 	unsigned int clock_frequency;
+	struct notifier_block restart_notifier;
 };
 
-static struct moxart_wdt_dev *moxart_restart_ctx;
-
 static int heartbeat;
 
-static void moxart_wdt_restart(enum reboot_mode reboot_mode, const char *cmd)
+static int moxart_restart_notify(struct notifier_block *this,
+				 unsigned long mode, void *cmd)
 {
-	writel(1, moxart_restart_ctx->base + REG_COUNT);
-	writel(0x5ab9, moxart_restart_ctx->base + REG_MODE);
-	writel(0x03, moxart_restart_ctx->base + REG_ENABLE);
+	struct moxart_wdt_dev *moxart_wdt = container_of(this,
+							 struct moxart_wdt_dev,
+							 restart_notifier);
+	writel(1, moxart_wdt->base + REG_COUNT);
+	writel(0x5ab9, moxart_wdt->base + REG_MODE);
+	writel(0x03, moxart_wdt->base + REG_ENABLE);
+
+	return NOTIFY_DONE;
 }
 
 static int moxart_wdt_stop(struct watchdog_device *wdt_dev)
@@ -136,8 +141,12 @@ static int moxart_wdt_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	moxart_restart_ctx = moxart_wdt;
-	arm_pm_restart = moxart_wdt_restart;
+	moxart_wdt->restart_notifier.notifier_call = moxart_restart_notify;
+	moxart_wdt->restart_notifier.priority = 128;
+	err = register_restart_notifier(&moxart_wdt->restart_notifier);
+	if (err)
+		dev_err(dev, "cannot register restart notifier (err=%d)\n",
+			err);
 
 	dev_dbg(dev, "Watchdog enabled (heartbeat=%d sec, nowayout=%d)\n",
 		moxart_wdt->dev.timeout, nowayout);
@@ -149,9 +158,8 @@ static int moxart_wdt_remove(struct platform_device *pdev)
 {
 	struct moxart_wdt_dev *moxart_wdt = platform_get_drvdata(pdev);
 
-	arm_pm_restart = NULL;
+	unregister_restart_notifier(&moxart_wdt->restart_notifier);
 	moxart_wdt_stop(&moxart_wdt->dev);
-	watchdog_unregister_device(&moxart_wdt->dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH v4 6/7] watchdog: alim7101: Register restart handler with restart notifier

[Guenter Roeck]

The kernel core now provides an API to trigger a system restart.
Register with it to restart the system instead of misusing the
reboot notifier.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v4: Set restart notifier priority to 128.
v3: No change
v2: No change

 drivers/watchdog/alim7101_wdt.c | 42 +++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/alim7101_wdt.c b/drivers/watchdog/alim7101_wdt.c
index 996b2f7..069c2e2 100644
--- a/drivers/watchdog/alim7101_wdt.c
+++ b/drivers/watchdog/alim7101_wdt.c
@@ -301,6 +301,28 @@ static struct miscdevice wdt_miscdev = {
 	.fops	=	&wdt_fops,
 };
 
+static int wdt_restart_notify(struct notifier_block *this, unsigned long mode,
+			      void *cmd)
+{
+	/*
+	 * Cobalt devices have no way of rebooting themselves other
+	 * than getting the watchdog to pull reset, so we restart the
+	 * watchdog on reboot with no heartbeat.
+	 */
+	wdt_change(WDT_ENABLE);
+
+	/* loop until the watchdog fires */
+	while (true)
+		;
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block wdt_restart_notifier = {
+	.notifier_call = wdt_restart_notify,
+	.priority = 128,
+};
+
 /*
  *	Notifier for system down
  */
@@ -311,15 +333,6 @@ static int wdt_notify_sys(struct notifier_block *this,
 	if (code == SYS_DOWN || code == SYS_HALT)
 		wdt_turnoff();
 
-	if (code == SYS_RESTART) {
-		/*
-		 * Cobalt devices have no way of rebooting themselves other
-		 * than getting the watchdog to pull reset, so we restart the
-		 * watchdog on reboot with no heartbeat
-		 */
-		wdt_change(WDT_ENABLE);
-		pr_info("Watchdog timer is now enabled with no heartbeat - should reboot in ~1 second\n");
-	}
 	return NOTIFY_DONE;
 }
 
@@ -338,6 +351,7 @@ static void __exit alim7101_wdt_unload(void)
 	/* Deregister */
 	misc_deregister(&wdt_miscdev);
 	unregister_reboot_notifier(&wdt_notifier);
+	unregister_restart_notifier(&wdt_restart_notifier);
 	pci_dev_put(alim7101_pmu);
 }
 
@@ -390,11 +404,17 @@ static int __init alim7101_wdt_init(void)
 		goto err_out;
 	}
 
+	rc = register_restart_notifier(&wdt_restart_notifier);
+	if (rc) {
+		pr_err("cannot register reboot notifier (err=%d)\n", rc);
+		goto err_out_reboot;
+	}
+
 	rc = misc_register(&wdt_miscdev);
 	if (rc) {
 		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
 		       wdt_miscdev.minor, rc);
-		goto err_out_reboot;
+		goto err_out_restart;
 	}
 
 	if (nowayout)
@@ -404,6 +424,8 @@ static int __init alim7101_wdt_init(void)
 		timeout, nowayout);
 	return 0;
 
+err_out_restart:
+	unregister_restart_notifier(&wdt_restart_notifier);
 err_out_reboot:
 	unregister_reboot_notifier(&wdt_notifier);
 err_out:
-- 
1.9.1

[PATCH v4 7/7] arm/arm64: Unexport restart handlers

[Guenter Roeck]

Implementing a restart handler in a module don't make sense
as there would be no guarantee that the module is loaded when
a restart is needed. Unexport arm_pm_restart to ensure that
no one gets the idea to do it anyway.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v4: No change
v3: No change
v2: No change

 arch/arm/kernel/process.c   | 1 -
 arch/arm64/kernel/process.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 5d191e3..25c7f00 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -125,7 +125,6 @@ void (*pm_power_off)(void);
 EXPORT_SYMBOL(pm_power_off);
 
 void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd) = null_restart;
-EXPORT_SYMBOL_GPL(arm_pm_restart);
 
 /*
  * This is our default idle handler.
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index b2da6d5..5b07396 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -92,7 +92,6 @@ void (*pm_power_off)(void);
 EXPORT_SYMBOL_GPL(pm_power_off);
 
 void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
-EXPORT_SYMBOL_GPL(arm_pm_restart);
 
 /*
  * This is our default idle handler.
-- 
1.9.1

[PATCH v4 5/7] watchdog: moxart: Register restart handler with restart notifier

[Catalin Marinas]

On Sun, Jul 13, 2014 at 04:30:29PM +0100, Guenter Roeck wrote:
> diff --git a/drivers/watchdog/moxart_wdt.c b/drivers/watchdog/moxart_wdt.c
> index 4aa3a8a..5080b50 100644
> --- a/drivers/watchdog/moxart_wdt.c
> +++ b/drivers/watchdog/moxart_wdt.c
[...]
> -static void moxart_wdt_restart(enum reboot_mode reboot_mode, const char *cmd)
> +static int moxart_restart_notify(struct notifier_block *this,
> +				 unsigned long mode, void *cmd)
>  {
> -	writel(1, moxart_restart_ctx->base + REG_COUNT);
> -	writel(0x5ab9, moxart_restart_ctx->base + REG_MODE);
> -	writel(0x03, moxart_restart_ctx->base + REG_ENABLE);
> +	struct moxart_wdt_dev *moxart_wdt = container_of(this,
> +							 struct moxart_wdt_dev,
> +							 restart_notifier);
> +	writel(1, moxart_wdt->base + REG_COUNT);
> +	writel(0x5ab9, moxart_wdt->base + REG_MODE);
> +	writel(0x03, moxart_wdt->base + REG_ENABLE);
> +
> +	return NOTIFY_DONE;
>  }

Wondering whether NOTIFY_OK or even NOTIFY_STOP_MASK is better here.

-- 
Catalin

[PATCH v4 7/7] arm/arm64: Unexport restart handlers

[Catalin Marinas]

On Sun, Jul 13, 2014 at 04:30:31PM +0100, Guenter Roeck wrote:
> Implementing a restart handler in a module don't make sense
> as there would be no guarantee that the module is loaded when
> a restart is needed. Unexport arm_pm_restart to ensure that
> no one gets the idea to do it anyway.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v4: No change
> v3: No change
> v2: No change
> 
>  arch/arm/kernel/process.c   | 1 -
>  arch/arm64/kernel/process.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
> index 5d191e3..25c7f00 100644
> --- a/arch/arm/kernel/process.c
> +++ b/arch/arm/kernel/process.c
> @@ -125,7 +125,6 @@ void (*pm_power_off)(void);
>  EXPORT_SYMBOL(pm_power_off);
>  
>  void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd) = null_restart;
> -EXPORT_SYMBOL_GPL(arm_pm_restart);

Unless I miss something, how is this different from registering a
restart notifier from a module (blocking_notifier_chain_register is
exported)?

-- 
Catalin

[PATCH v4 7/7] arm/arm64: Unexport restart handlers

[Guenter Roeck]

On 07/14/2014 07:22 AM, Catalin Marinas wrote:
> On Sun, Jul 13, 2014 at 04:30:31PM +0100, Guenter Roeck wrote:
>> Implementing a restart handler in a module don't make sense
>> as there would be no guarantee that the module is loaded when
>> a restart is needed. Unexport arm_pm_restart to ensure that
>> no one gets the idea to do it anyway.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v4: No change
>> v3: No change
>> v2: No change
>>
>>   arch/arm/kernel/process.c   | 1 -
>>   arch/arm64/kernel/process.c | 1 -
>>   2 files changed, 2 deletions(-)
>>
>> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
>> index 5d191e3..25c7f00 100644
>> --- a/arch/arm/kernel/process.c
>> +++ b/arch/arm/kernel/process.c
>> @@ -125,7 +125,6 @@ void (*pm_power_off)(void);
>>   EXPORT_SYMBOL(pm_power_off);
>>
>>   void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd) = null_restart;
>> -EXPORT_SYMBOL_GPL(arm_pm_restart);
>
> Unless I miss something, how is this different from registering a
> restart notifier from a module (blocking_notifier_chain_register is
> exported)?
>

A notifier can be unregistered safely, and more than one notifier call
is supported. If there is more than one driver setting arm_pm_restart,
the first one to unregister will clear the pointer. Using the notifier
is cleaner and not architecture dependent. One might argue that setting
module external function pointers from module code isn't exactly clean
coding.

Anyway, this patch is not relevant for the series. If you prefer to have
the function exported, and keep using it for arm drivers loaded as modules,
be my guest, and I'll be more than happy drop it. I'll take your comment
as a hint _not_ to convert existing code to use the notifier after the
series is accepted. Cool, as I hate wasting my time.

Guenter

[PATCH v4 5/7] watchdog: moxart: Register restart handler with restart notifier

[Guenter Roeck]

On 07/14/2014 07:19 AM, Catalin Marinas wrote:
> On Sun, Jul 13, 2014 at 04:30:29PM +0100, Guenter Roeck wrote:
>> diff --git a/drivers/watchdog/moxart_wdt.c b/drivers/watchdog/moxart_wdt.c
>> index 4aa3a8a..5080b50 100644
>> --- a/drivers/watchdog/moxart_wdt.c
>> +++ b/drivers/watchdog/moxart_wdt.c
> [...]
>> -static void moxart_wdt_restart(enum reboot_mode reboot_mode, const char *cmd)
>> +static int moxart_restart_notify(struct notifier_block *this,
>> +				 unsigned long mode, void *cmd)
>>   {
>> -	writel(1, moxart_restart_ctx->base + REG_COUNT);
>> -	writel(0x5ab9, moxart_restart_ctx->base + REG_MODE);
>> -	writel(0x03, moxart_restart_ctx->base + REG_ENABLE);
>> +	struct moxart_wdt_dev *moxart_wdt = container_of(this,
>> +							 struct moxart_wdt_dev,
>> +							 restart_notifier);
>> +	writel(1, moxart_wdt->base + REG_COUNT);
>> +	writel(0x5ab9, moxart_wdt->base + REG_MODE);
>> +	writel(0x03, moxart_wdt->base + REG_ENABLE);
>> +
>> +	return NOTIFY_DONE;
>>   }
>
> Wondering whether NOTIFY_OK or even NOTIFY_STOP_MASK is better here.
>

Personally I would rather call all of them if more than one notifier is
registered to ensure that (at least) one does what it is intended to do,
but I am open to suggestions.

Guenter

[PATCH v4 1/7] kernel: Add support for restart notifier call chain

[Catalin Marinas]

On Sun, Jul 13, 2014 at 04:30:25PM +0100, Guenter Roeck wrote:
> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> index 48bf152..120db73 100644
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -38,6 +38,9 @@ extern int reboot_force;
>  extern int register_reboot_notifier(struct notifier_block *);
>  extern int unregister_reboot_notifier(struct notifier_block *);
>  
> +extern int register_restart_notifier(struct notifier_block *);
> +extern int unregister_restart_notifier(struct notifier_block *);
> +extern void kernel_restart_notify(char *cmd);
>  
>  /*
>   * Architecture-specific implementations of sys_reboot commands.
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index a3a9e24..1bc9bf2 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -104,6 +104,87 @@ int unregister_reboot_notifier(struct notifier_block *nb)
>  }
>  EXPORT_SYMBOL(unregister_reboot_notifier);
>  
> +/*
> + *	Notifier list for kernel code which wants to be called
> + *	to restart the system.
> + */
> +static BLOCKING_NOTIFIER_HEAD(restart_notifier_list);
> +
> +/**
> + *	register_restart_notifier - Register function to be called to reset
> + *				    the system
> + *	@nb: Info about notifier function to be called
> + *	@nb->priority:	Notifier priority. Notifiers should follow the
> + *			following guidelines for setting priorities.
> + *			0:	Restart notifier of last resort,
> + *				with limited restart capabilities
> + *			128:	Default notifier; use if no other
> + *				notifier is expected to be available,
> + *				and/or if restart functionality is
> + *				sufficient to restart the entire system
> + *			255:	Highest priority notifier, will preempt
> + *				all other notifier functions

I'm not fully convinced implying a 'notifier' is the right approach
here. By analogy with the reboot notifier, this is something drivers
would want to know about and do some work before the actual system
restart (e.g. disable watchdogs as in the reboot notifier case). The
restart notifier here is meant to perform the actual system restart.
Arguably, the actual restart should be handled by priority 0 with some
preparation before but we have reboot notifier already, so I don't think
it's worth another notifier.

While re-using the notifier mechanism behind the scene is fine, I think
we should at least rename the functions to something like
(un)register_restart_handler().

-- 
Catalin

[PATCH v4 2/7] arm64: Support restart through restart notifier call chain

[Catalin Marinas]

On Sun, Jul 13, 2014 at 04:30:26PM +0100, Guenter Roeck wrote:
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 43b7c34..b2da6d5 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -174,6 +174,8 @@ void machine_restart(char *cmd)
>  	/* Now call the architecture specific reboot code. */
>  	if (arm_pm_restart)
>  		arm_pm_restart(reboot_mode, cmd);
> +	else
> +		kernel_restart_notify(cmd);

There are couple of drivers specific to arm64, once they are converted
we can get rid of arm_pm_restart entirely here.

-- 
Catalin

[PATCH v4 1/7] kernel: Add support for restart notifier call chain

[Guenter Roeck]

On 07/14/2014 07:53 AM, Catalin Marinas wrote:
> On Sun, Jul 13, 2014 at 04:30:25PM +0100, Guenter Roeck wrote:
>> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
>> index 48bf152..120db73 100644
>> --- a/include/linux/reboot.h
>> +++ b/include/linux/reboot.h
>> @@ -38,6 +38,9 @@ extern int reboot_force;
>>   extern int register_reboot_notifier(struct notifier_block *);
>>   extern int unregister_reboot_notifier(struct notifier_block *);
>>
>> +extern int register_restart_notifier(struct notifier_block *);
>> +extern int unregister_restart_notifier(struct notifier_block *);
>> +extern void kernel_restart_notify(char *cmd);
>>
>>   /*
>>    * Architecture-specific implementations of sys_reboot commands.
>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>> index a3a9e24..1bc9bf2 100644
>> --- a/kernel/reboot.c
>> +++ b/kernel/reboot.c
>> @@ -104,6 +104,87 @@ int unregister_reboot_notifier(struct notifier_block *nb)
>>   }
>>   EXPORT_SYMBOL(unregister_reboot_notifier);
>>
>> +/*
>> + *	Notifier list for kernel code which wants to be called
>> + *	to restart the system.
>> + */
>> +static BLOCKING_NOTIFIER_HEAD(restart_notifier_list);
>> +
>> +/**
>> + *	register_restart_notifier - Register function to be called to reset
>> + *				    the system
>> + *	@nb: Info about notifier function to be called
>> + *	@nb->priority:	Notifier priority. Notifiers should follow the
>> + *			following guidelines for setting priorities.
>> + *			0:	Restart notifier of last resort,
>> + *				with limited restart capabilities
>> + *			128:	Default notifier; use if no other
>> + *				notifier is expected to be available,
>> + *				and/or if restart functionality is
>> + *				sufficient to restart the entire system
>> + *			255:	Highest priority notifier, will preempt
>> + *				all other notifier functions
>
> I'm not fully convinced implying a 'notifier' is the right approach
> here. By analogy with the reboot notifier, this is something drivers
> would want to know about and do some work before the actual system
> restart (e.g. disable watchdogs as in the reboot notifier case). The
> restart notifier here is meant to perform the actual system restart.
> Arguably, the actual restart should be handled by priority 0 with some
> preparation before but we have reboot notifier already, so I don't think
> it's worth another notifier.
>
> While re-using the notifier mechanism behind the scene is fine, I think
> we should at least rename the functions to something like
> (un)register_restart_handler().
>

Fine with me. Any other comments / suggestions on the name of the function ?

Guenter

[PATCH v4 2/7] arm64: Support restart through restart notifier call chain

[Guenter Roeck]

On 07/14/2014 07:55 AM, Catalin Marinas wrote:
> On Sun, Jul 13, 2014 at 04:30:26PM +0100, Guenter Roeck wrote:
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 43b7c34..b2da6d5 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -174,6 +174,8 @@ void machine_restart(char *cmd)
>>   	/* Now call the architecture specific reboot code. */
>>   	if (arm_pm_restart)
>>   		arm_pm_restart(reboot_mode, cmd);
>> +	else
>> +		kernel_restart_notify(cmd);
>
> There are couple of drivers specific to arm64, once they are converted
> we can get rid of arm_pm_restart entirely here.
>

I thought you wanted to keep exporting arm_pm_restart. That logically implies
that you want to have the ability to write new drivers which use it, which
in turn implies that converting existing drivers would not make much sense.

Am I missing something ?

Guenter

[PATCH v4 2/7] arm64: Support restart through restart notifier call chain

[Catalin Marinas]

On Mon, Jul 14, 2014 at 04:01:14PM +0100, Guenter Roeck wrote:
> On 07/14/2014 07:55 AM, Catalin Marinas wrote:
> > On Sun, Jul 13, 2014 at 04:30:26PM +0100, Guenter Roeck wrote:
> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> >> index 43b7c34..b2da6d5 100644
> >> --- a/arch/arm64/kernel/process.c
> >> +++ b/arch/arm64/kernel/process.c
> >> @@ -174,6 +174,8 @@ void machine_restart(char *cmd)
> >>   	/* Now call the architecture specific reboot code. */
> >>   	if (arm_pm_restart)
> >>   		arm_pm_restart(reboot_mode, cmd);
> >> +	else
> >> +		kernel_restart_notify(cmd);
> >
> > There are couple of drivers specific to arm64, once they are converted
> > we can get rid of arm_pm_restart entirely here.
> 
> I thought you wanted to keep exporting arm_pm_restart.

No, I just thought you don't want the restart mechanism implemented in
modules but the notifier registration still allows that, so I didn't
fully get the reasoning. But I agree on the race aspect.

> That logically implies
> that you want to have the ability to write new drivers which use it, which
> in turn implies that converting existing drivers would not make much sense.

If the new handler registration mechanism gives the functionality, I
don't see why we should keep arm_pm_restart around (for arm64 it's
easier since there aren't many drivers setting it).

-- 
Catalin

[PATCH v4 7/7] arm/arm64: Unexport restart handlers

[Catalin Marinas]

On Mon, Jul 14, 2014 at 03:39:38PM +0100, Guenter Roeck wrote:
> On 07/14/2014 07:22 AM, Catalin Marinas wrote:
> > On Sun, Jul 13, 2014 at 04:30:31PM +0100, Guenter Roeck wrote:
> >> Implementing a restart handler in a module don't make sense
> >> as there would be no guarantee that the module is loaded when
> >> a restart is needed. Unexport arm_pm_restart to ensure that
> >> no one gets the idea to do it anyway.
> >>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >> v4: No change
> >> v3: No change
> >> v2: No change
> >>
> >>   arch/arm/kernel/process.c   | 1 -
> >>   arch/arm64/kernel/process.c | 1 -
> >>   2 files changed, 2 deletions(-)
> >>
> >> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
> >> index 5d191e3..25c7f00 100644
> >> --- a/arch/arm/kernel/process.c
> >> +++ b/arch/arm/kernel/process.c
> >> @@ -125,7 +125,6 @@ void (*pm_power_off)(void);
> >>   EXPORT_SYMBOL(pm_power_off);
> >>
> >>   void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd) = null_restart;
> >> -EXPORT_SYMBOL_GPL(arm_pm_restart);
> >
> > Unless I miss something, how is this different from registering a
> > restart notifier from a module (blocking_notifier_chain_register is
> > exported)?
> 
> A notifier can be unregistered safely, and more than one notifier call
> is supported. If there is more than one driver setting arm_pm_restart,
> the first one to unregister will clear the pointer. Using the notifier
> is cleaner and not architecture dependent. One might argue that setting
> module external function pointers from module code isn't exactly clean
> coding.

I agree.

> Anyway, this patch is not relevant for the series. If you prefer to have
> the function exported, and keep using it for arm drivers loaded as modules,
> be my guest, and I'll be more than happy drop it. I'll take your comment
> as a hint _not_ to convert existing code to use the notifier after the
> series is accepted. Cool, as I hate wasting my time.

No, it's actually the opposite ;) (and I would go further and remove
arm_pm_restart entirely for arm64).

-- 
Catalin

[PATCH v4 5/7] watchdog: moxart: Register restart handler with restart notifier

[Catalin Marinas]

On Mon, Jul 14, 2014 at 03:41:15PM +0100, Guenter Roeck wrote:
> On 07/14/2014 07:19 AM, Catalin Marinas wrote:
> > On Sun, Jul 13, 2014 at 04:30:29PM +0100, Guenter Roeck wrote:
> >> diff --git a/drivers/watchdog/moxart_wdt.c b/drivers/watchdog/moxart_wdt.c
> >> index 4aa3a8a..5080b50 100644
> >> --- a/drivers/watchdog/moxart_wdt.c
> >> +++ b/drivers/watchdog/moxart_wdt.c
> > [...]
> >> -static void moxart_wdt_restart(enum reboot_mode reboot_mode, const char *cmd)
> >> +static int moxart_restart_notify(struct notifier_block *this,
> >> +				 unsigned long mode, void *cmd)
> >>   {
> >> -	writel(1, moxart_restart_ctx->base + REG_COUNT);
> >> -	writel(0x5ab9, moxart_restart_ctx->base + REG_MODE);
> >> -	writel(0x03, moxart_restart_ctx->base + REG_ENABLE);
> >> +	struct moxart_wdt_dev *moxart_wdt = container_of(this,
> >> +							 struct moxart_wdt_dev,
> >> +							 restart_notifier);
> >> +	writel(1, moxart_wdt->base + REG_COUNT);
> >> +	writel(0x5ab9, moxart_wdt->base + REG_MODE);
> >> +	writel(0x03, moxart_wdt->base + REG_ENABLE);
> >> +
> >> +	return NOTIFY_DONE;
> >>   }
> >
> > Wondering whether NOTIFY_OK or even NOTIFY_STOP_MASK is better here.
> 
> Personally I would rather call all of them if more than one notifier is
> registered to ensure that (at least) one does what it is intended to do,
> but I am open to suggestions.

Probably fine. I guess there won't be more than one handler registered
in most cases anyway.

-- 
Catalin

[PATCH v4 7/7] arm/arm64: Unexport restart handlers

[Guenter Roeck]

On Mon, Jul 14, 2014 at 04:09:09PM +0100, Catalin Marinas wrote:
> On Mon, Jul 14, 2014 at 03:39:38PM +0100, Guenter Roeck wrote:
> > On 07/14/2014 07:22 AM, Catalin Marinas wrote:
> > > On Sun, Jul 13, 2014 at 04:30:31PM +0100, Guenter Roeck wrote:
> > >> Implementing a restart handler in a module don't make sense
> > >> as there would be no guarantee that the module is loaded when
> > >> a restart is needed. Unexport arm_pm_restart to ensure that
> > >> no one gets the idea to do it anyway.
> > >>
> > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > >> ---
> > >> v4: No change
> > >> v3: No change
> > >> v2: No change
> > >>
> > >>   arch/arm/kernel/process.c   | 1 -
> > >>   arch/arm64/kernel/process.c | 1 -
> > >>   2 files changed, 2 deletions(-)
> > >>
> > >> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
> > >> index 5d191e3..25c7f00 100644
> > >> --- a/arch/arm/kernel/process.c
> > >> +++ b/arch/arm/kernel/process.c
> > >> @@ -125,7 +125,6 @@ void (*pm_power_off)(void);
> > >>   EXPORT_SYMBOL(pm_power_off);
> > >>
> > >>   void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd) = null_restart;
> > >> -EXPORT_SYMBOL_GPL(arm_pm_restart);
> > >
> > > Unless I miss something, how is this different from registering a
> > > restart notifier from a module (blocking_notifier_chain_register is
> > > exported)?
> > 
> > A notifier can be unregistered safely, and more than one notifier call
> > is supported. If there is more than one driver setting arm_pm_restart,
> > the first one to unregister will clear the pointer. Using the notifier
> > is cleaner and not architecture dependent. One might argue that setting
> > module external function pointers from module code isn't exactly clean
> > coding.
> 
> I agree.
> 
> > Anyway, this patch is not relevant for the series. If you prefer to have
> > the function exported, and keep using it for arm drivers loaded as modules,
> > be my guest, and I'll be more than happy drop it. I'll take your comment
> > as a hint _not_ to convert existing code to use the notifier after the
> > series is accepted. Cool, as I hate wasting my time.
> 
> No, it's actually the opposite ;) (and I would go further and remove
> arm_pm_restart entirely for arm64).
> 
Ah, sorry, I misunderstood. As mentioned above, I had planned to submit
patches to do just that after the initial series has been accepted.
Guess I can start with arm64 to keep things simple.

Thanks,
Guenter

[PATCH v4 1/7] kernel: Add support for restart notifier call chain

[Guenter Roeck]

On Mon, Jul 14, 2014 at 07:58:59AM -0700, Guenter Roeck wrote:
> On 07/14/2014 07:53 AM, Catalin Marinas wrote:
[ ... ]
> >
> >I'm not fully convinced implying a 'notifier' is the right approach
> >here. By analogy with the reboot notifier, this is something drivers
> >would want to know about and do some work before the actual system
> >restart (e.g. disable watchdogs as in the reboot notifier case). The
> >restart notifier here is meant to perform the actual system restart.
> >Arguably, the actual restart should be handled by priority 0 with some
> >preparation before but we have reboot notifier already, so I don't think
> >it's worth another notifier.
> >
> >While re-using the notifier mechanism behind the scene is fine, I think
> >we should at least rename the functions to something like
> >(un)register_restart_handler().
> >
> 
> Fine with me. Any other comments / suggestions on the name of the function ?
> 
Any further feedback on the function names ? If not I'll resubmit the series
tonight or tomorrow morning with the functions renamed as suggested above.

Guenter