Skip to contentSkip to conversation

RFC: Common machine reset handling

PATCH In review Domenico Andreoli 6 participants 21 messages Oct 31, 2013 – Nov 1, 2013

Subsystems (2)
MIPS Generic include/asm header files
lore
Share & tools
lore

Domenico Andreoli

author
#0 lore raw

cover RFC: Common machine reset handling

Hi,

I've been looking for a solution to my bcm4760 watchdog based restart
hook when I noticed that the kernel reboot/shutdown mechanism is having a few unaddressed issues.

Those I identified are:

1) context pointer often needed by the reset hook (currently local static data is used for this pourpose)
2) unclear ownership/policy in case of multiple reset hooks (currently almost nobody seems to care much)

I put together this patchset to explore some possible interface and improve the status, essentially moving away from the function-pointer-assignament kind of reset hook registration.

It is meant to be platform agnostic and sits in kernel/machine_reset.c. It opts in only when needed and when supported.

A common usage, as reset hook provider (drivers, boardfiles):

void restart_hook(void *dev, enum reboot_mode mode, const char *cmd)

{
	/* reboot the board */
}

void release_hook(void *dev)

{
	/* release the hook */
}

void init_restart_hook(void *dev)

{
	struct reset_hook hook;
	reset_hook_init(&hook);
	hook.restart = restart_hook;
	hook.release = release_hook; /* optional */
	set_machine_reset(RESET_RESTART, &hook, dev);
}

A common usage, as reset hook consumer (arch specific hooks, kernel):

void machine_restart(char *command)

{
	if (_machine_restart)
		_machine_restart(command);
	else
		default_restart(reboot_mode, command);
}

void machine_halt(void)

{
	if (_machine_halt)
		_machine_halt();
	else
		default_halt();
}

void machine_power_off(void)

{
	if (pm_power_off)
		pm_power_off();
	else
		default_power_off();
}

I don't see these impressive diffstats but at least it's possible to free some static data along the way. Maybe there are some other desiderata that could be satisfied?

Comments are welcome.

Thanks,
Domenico

#5 lore raw

Re: [PATCH 04/11] MIPS: use the common machine reset handling

Quoted 53 lines · Domenico Andreoli

From: Domenico Andreoli <[email protected]>

Proof of concept: MIPS as a consumer of the machine reset hooks.

Cc Ralf Baechle <[email protected]>
Cc linux-arch at vger.kernel.org
Cc linux-mips at vger.kernel.org
Signed-off-by Domenico Andreoli <[email protected]>
---
 arch/mips/kernel/reset.c |    7 +++++++
 kernel/power/Kconfig     |    2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

Index: b/arch/mips/kernel/reset.c
===================================================================

arch/mips/kernel/reset.c +7 -0
@@ -11,6 +11,7 @@
 #include <linux/pm.h>
 #include <linux/types.h>
 #include <linux/reboot.h>
+#include <linux/machine_reset.h>
 
 #include <asm/reboot.h>
 

@@ -29,16 +30,22 @@ void machine_restart(char *command)
 {
 	if (_machine_restart)
 		_machine_restart(command);
+	else
+		default_restart(reboot_mode, command);
 }
 
 void machine_halt(void)
 {
 	if (_machine_halt)
 		_machine_halt();
+	else
+		default_halt();
 }
 
 void machine_power_off(void)
 {
 	if (pm_power_off)
 		pm_power_off();
+	else
+		default_power_off();
 }

Index: b/kernel/power/Kconfig
===================================================================

kernel/power/Kconfig config CPU_PM · line 300

Quoted 5 lines
kernel/power/Kconfig +1 -1
@@ -297,4 +297,4 @@ config CPU_PM
 config MACHINE_RESET
 	bool
 	default n
-	depends on ARM || ARM64
+	depends on ARM || ARM64 || MIPS

This particular idiom is frowned upon for new code. As new arches get added this list keeps getting bigger and then those who don't need the feature need to add the anti dependency. Also in this particular case the dependency is trivial so you can just "select" it in arch/*/Kconfig

#6 lore raw

Re: [PATCH 04/11] MIPS: use the common machine reset handling

Quoted 56 lines · Vineet Gupta
Quoted 55 lines · Domenico Andreoli

From: Domenico Andreoli <[email protected]>

Proof of concept: MIPS as a consumer of the machine reset hooks.

Cc Ralf Baechle <[email protected]>
Cc linux-arch at vger.kernel.org
Cc linux-mips at vger.kernel.org
Signed-off-by Domenico Andreoli <[email protected]>
---
 arch/mips/kernel/reset.c |    7 +++++++
 kernel/power/Kconfig     |    2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

Index: b/arch/mips/kernel/reset.c
===================================================================

arch/mips/kernel/reset.c +7 -0
@@ -11,6 +11,7 @@
 #include <linux/pm.h>
 #include <linux/types.h>
 #include <linux/reboot.h>
+#include <linux/machine_reset.h>
 
 #include <asm/reboot.h>
 

@@ -29,16 +30,22 @@ void machine_restart(char *command)
 {
 	if (_machine_restart)
 		_machine_restart(command);
+	else
+		default_restart(reboot_mode, command);
 }
 
 void machine_halt(void)
 {
 	if (_machine_halt)
 		_machine_halt();
+	else
+		default_halt();
 }
 
 void machine_power_off(void)
 {
 	if (pm_power_off)
 		pm_power_off();
+	else
+		default_power_off();
 }

Index: b/kernel/power/Kconfig
===================================================================

kernel/power/Kconfig +0 -0
@@ -297,4 +297,4 @@ config CPU_PM
 config MACHINE_RESET
 	bool
Quoted 8 lines

kernel/power/Kconfig config CPU_PM · line 300

Quoted 3 lines
kernel/power/Kconfig +1 -1
 	default n
-	depends on ARM || ARM64
+	depends on ARM || ARM64 || MIPS

This particular idiom is frowned upon for new code. As new arches get added this list keeps getting bigger and then those who don't need the feature need to add the anti dependency. Also in this particular case the dependency is trivial so you can just "select" it in arch/*/Kconfig

this dependency guarantees only that the option is available only on the supported arches.

anyway I've not a strong opinion, I only picked the least invasive solution I was able to think.

thanks,
Domenico

#11 lore raw

Re: [PATCH 08/11] ARM: u300: use the common machine reset handling

Quoted 3 lines · Domenico Andreoli

From: Domenico Andreoli <[email protected]>

Proof of concept: u300 as provider of reset hooks.

Do you have a u300 to test this on or are you just guessing?

Anyway, concept looks correct, so Acked-by given that the overall idea is accepted.

Yours,
Linus Walleij

#12 lore raw

Re: [PATCH 08/11] ARM: u300: use the common machine reset handling

Quoted 8 lines · Linus Walleij
Quoted 3 lines · Domenico Andreoli

From: Domenico Andreoli <[email protected]>

Proof of concept: u300 as provider of reset hooks.

Do you have a u300 to test this on or are you just guessing?

I don't have any u300, I picked it randomly. It's just to show some usage and possibly discuss it. There are quite a lot of these hooks all around.

Quoted 2 lines

Anyway, concept looks correct, so Acked-by given that the overall idea is accepted.

uhm.. don't see any general acceptance right now. maybe later..

thanks,
Domenico

#16 lore raw
In reply to Domenico Andreoli
Quoted 10 lines · Domenico Andreoli

Hi,

I've been looking for a solution to my bcm4760 watchdog based restart
hook when I noticed that the kernel reboot/shutdown mechanism is having a few unaddressed issues.

Those I identified are:

1) context pointer often needed by the reset hook (currently local static data is used for this pourpose)

I don't think that's a problem at all.

Quoted 3 lines

I don't see these impressive diffstats but at least it's possible to free some static data along the way. Maybe there are some other desiderata that could be satisfied?

If that's the only benefit, then I don't think this is worth the churn, sorry.

#17 lore raw
In reply to Domenico Andreoli
Quoted 6 lines · Domenico Andreoli

Hi,

I've been looking for a solution to my bcm4760 watchdog based restart
hook when I noticed that the kernel reboot/shutdown mechanism is having a few unaddressed issues.

Quoted 6 lines

Those I identified are:

1) context pointer often needed by the reset hook (currently local static data is used for this pourpose)
2) unclear ownership/policy in case of multiple reset hooks (currently almost nobody seems to care much)

I'm not sure how this patchset solves (2); even with the new API, it's still the case that whichever code calls set_machine_reset() last wins, just like before where whichever code wrote to pm_power_off won. I'm not sure what this series attempts to solve.

#18 lore raw
Quoted 14 lines · Stephen Warren
Quoted 6 lines · Domenico Andreoli

Hi,

I've been looking for a solution to my bcm4760 watchdog based restart
hook when I noticed that the kernel reboot/shutdown mechanism is having a few unaddressed issues.

Quoted 6 lines

Those I identified are:

1) context pointer often needed by the reset hook (currently local static data is used for this pourpose)
2) unclear ownership/policy in case of multiple reset hooks (currently almost nobody seems to care much)

Quoted 4 lines

I'm not sure how this patchset solves (2); even with the new API, it's still the case that whichever code calls set_machine_reset() last wins, just like before where whichever code wrote to pm_power_off won. I'm not sure what this series attempts to solve.

That's right, the last wins. But the previous has a chance to know.

I only supposed there is somebody in charge of selecting the best handler for the machine. Don't know how fancy this decision is but at least for the vexpress there is also a sysfs way to configure different reset methods from user-space.

So cleaning up things after the handler is replaced seemed a sensible thing to do.

Another "problem" this patch would solve is the registration of the reset handler in a architecture independent way. Now an otherwise platform generic gpio HW reset driver would need to do different things on different architectures.

thanks,
Domenico

#19 lore raw
Quoted 20 lines · Domenico Andreoli
Quoted 14 lines · Stephen Warren
Quoted 6 lines · Domenico Andreoli

Hi,

I've been looking for a solution to my bcm4760 watchdog based restart
hook when I noticed that the kernel reboot/shutdown mechanism is having a few unaddressed issues.

Quoted 6 lines

Those I identified are:

1) context pointer often needed by the reset hook (currently local static data is used for this pourpose)
2) unclear ownership/policy in case of multiple reset hooks (currently almost nobody seems to care much)

Quoted 4 lines

I'm not sure how this patchset solves (2); even with the new API, it's still the case that whichever code calls set_machine_reset() last wins, just like before where whichever code wrote to pm_power_off won. I'm not sure what this series attempts to solve.

Quoted 6 lines

That's right, the last wins. But the previous has a chance to know.

I only supposed there is somebody in charge of selecting the best handler for the machine. Don't know how fancy this decision is but at least for the vexpress there is also a sysfs way to configure different reset methods from user-space.

For PMICs that provide power off, we've been adding a property to DT to indicate whether the PMIC is *the* system power off controller or not. If the property is present, the PMIC registers itself in the poweroff hook. If not, it doesn't. So, there really isn't an algorithm for selecting the power off mechanism, but rather we designate one mechanism ahead of time, and that's the only one that's relevant. We could probably do the same for reset mechanisms.

I guess the vexpress situation is actually the same; there's a single concept of a custom vexpress reset, it's just that sysfs is used to select exactly what that does?

Quoted 2 lines

So cleaning up things after the handler is replaced seemed a sensible thing to do.

Can't we avoid replacing handlers, but only registering a single handler?

Quoted 4 lines

Another "problem" this patch would solve is the registration of the reset handler in a architecture independent way. Now an otherwise platform generic gpio HW reset driver would need to do different things on different architectures.

OK, if there are architecture differences in how the hooks are registered, that seems like a good thing to fix.

#20 lore raw
Quoted 11 lines · Stephen Warren

For PMICs that provide power off, we've been adding a property to DT to indicate whether the PMIC is *the* system power off controller or not. If the property is present, the PMIC registers itself in the poweroff hook. If not, it doesn't. So, there really isn't an algorithm for selecting the power off mechanism, but rather we designate one mechanism ahead of time, and that's the only one that's relevant. We could probably do the same for reset mechanisms.

I guess the vexpress situation is actually the same; there's a single concept of a custom vexpress reset, it's just that sysfs is used to select exactly what that does?

I'm not aware of that. Vexpress has the following mechanisms:

- reset - this causes the system to be restarted without powering off.
- restart - this causes the system to be powered off and back on.
- poweroff - this causes the system to power off.

Obviously, poweroff is what needs to happen when someone issues the poweroff command (or, when we get hibernate support, the power off hook will also be called to power the system off after saving all system state.) So, a power off callback really better power the system off and not reboot it.

reset vs restart is a choice, and one of those should happen as a result of the reboot command, or other similar event which ends up requesting a system restart. That may be configurable.

Ultimately though, this should have no bearing on the hooking of poweroff and restart callbacks; the only difference there is on Vexpress is the function code passed to the system controller.