Skip to contentSkip to conversation

kernel: Drop pm_poweroff_prepare

RFC PATCH In review Guenter Roeck 2 participants 7 messages Oct 11, 2014 – Oct 12, 2014

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover kernel: Drop pm_poweroff_prepare

While working on the poweroff handler implementation, I also started looking into pm_power_off_prepare. Ultimately, I concluded that it is not really necessary, and that it would be easy to remove.

The first patch in this series converts acpi to use a syscore callback instead of setting pm_power_off_prepare. Since acpi is the only user of pm_power_off_prepare, it is no longer used and removed with the second patch.

Removing pm_power_off_prepare streamlines poweroff handling and makes it more uniform, so I think its removal would be worthwhile to consider.

I am sending the series as RFC since I am not sure if I may be missing something. Compile tested only at this time; I'll do more testing if this sounds like a reasonable thing to do.

On a side note, I don't see any callers of acpi_suspend(). I actually removed it as an experiment, and at least x86_64:allmodconfig still compiles. Can it be removed ?

#1 lore raw

RFC PATCH 1/2 acpi: Use syscore instead of pm_power_off_prepare to prepare for poweroff

The syscore shutdown callback seems to be perfectly suited to prepare for system poweroff. Use it instead of pm_power_off_prepare.

Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/sleep.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
drivers/acpi/sleep.c +16 -5
index 05a31b5..e03c74d 100644
@@ -17,6 +17,7 @@
 #include <linux/interrupt.h>
 #include <linux/suspend.h>
 #include <linux/reboot.h>
+#include <linux/syscore_ops.h>
 #include <linux/acpi.h>
 #include <linux/module.h>
 #include <asm/io.h>
@@ -820,13 +821,23 @@ int acpi_suspend(u32 acpi_state)
 	return -EINVAL;
 }
 
-static void acpi_power_off_prepare(void)
+static void acpi_shutdown(void)
 {
-	/* Prepare to power off the system */
-	acpi_sleep_prepare(ACPI_STATE_S5);
-	acpi_disable_all_gpes();
+	switch (system_state) {
+	case SYSTEM_POWER_OFF:
+		/* Prepare to power off the system */
+		acpi_sleep_prepare(ACPI_STATE_S5);
+		acpi_disable_all_gpes();
+		break;
+	default:
+		break;
+	}
 }
 
+static struct syscore_ops acpi_syscore_ops = {
+	.shutdown = acpi_shutdown,
+};
+
 static void acpi_power_off(void)
 {
 	/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
@@ -850,7 +861,7 @@ 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;
+		register_syscore_ops(&acpi_syscore_ops);
 		pm_power_off = acpi_power_off;
 	}
 
Signature · 1 line
1.9.1
#2 lore raw

Re: [RFC PATCH 1/2] acpi: Use syscore instead of pm_power_off_prepare to prepare for poweroff

Quoted 2 lines · Guenter Roeck

The syscore shutdown callback seems to be perfectly suited to prepare for system poweroff. Use it instead of pm_power_off_prepare.

How much testing did that receive?

Quoted 57 lines
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/sleep.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
drivers/acpi/sleep.c +16 -5
index 05a31b5..e03c74d 100644
@@ -17,6 +17,7 @@
 #include <linux/interrupt.h>
 #include <linux/suspend.h>
 #include <linux/reboot.h>
+#include <linux/syscore_ops.h>
 #include <linux/acpi.h>
 #include <linux/module.h>
 #include <asm/io.h>
@@ -820,13 +821,23 @@ int acpi_suspend(u32 acpi_state)
 	return -EINVAL;
 }
 
-static void acpi_power_off_prepare(void)
+static void acpi_shutdown(void)
 {
-	/* Prepare to power off the system */
-	acpi_sleep_prepare(ACPI_STATE_S5);
-	acpi_disable_all_gpes();
+	switch (system_state) {
+	case SYSTEM_POWER_OFF:
+		/* Prepare to power off the system */
+		acpi_sleep_prepare(ACPI_STATE_S5);
+		acpi_disable_all_gpes();
+		break;
+	default:
+		break;
+	}
 }
 
+static struct syscore_ops acpi_syscore_ops = {
+	.shutdown = acpi_shutdown,
+};
+
 static void acpi_power_off(void)
 {
 	/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
@@ -850,7 +861,7 @@ 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;
+		register_syscore_ops(&acpi_syscore_ops);
 		pm_power_off = acpi_power_off;
 	}
 
Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
#3 lore raw

Re: [RFC PATCH 1/2] acpi: Use syscore instead of pm_power_off_prepare to prepare for poweroff

Quoted 6 lines · Rafael J. Wysocki
Quoted 2 lines · Guenter Roeck

The syscore shutdown callback seems to be perfectly suited to prepare for system poweroff. Use it instead of pm_power_off_prepare.

How much testing did that receive?

As I mentioned in patch 0/2, compile tested so far only. Before I start playing with my servers, I wanted to get some feedback if the idea is worth pursuing further or if I am missing something essential. _If_ it should be worth pursuing I'll test it with some real systems (servers + laptops) before resubmitting, and also add log messages for that testing to ensure that acpi_shutdown is actually called.

Guenter

Quoted 58 lines
Quoted 57 lines
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
  drivers/acpi/sleep.c | 21 ++++++++++++++++-----
  1 file changed, 16 insertions(+), 5 deletions(-)
drivers/acpi/sleep.c +16 -5
index 05a31b5..e03c74d 100644
@@ -17,6 +17,7 @@
  #include <linux/interrupt.h>
  #include <linux/suspend.h>
  #include <linux/reboot.h>
+#include <linux/syscore_ops.h>
  #include <linux/acpi.h>
  #include <linux/module.h>
  #include <asm/io.h>
@@ -820,13 +821,23 @@ int acpi_suspend(u32 acpi_state)
  	return -EINVAL;
  }
 
-static void acpi_power_off_prepare(void)
+static void acpi_shutdown(void)
  {
-	/* Prepare to power off the system */
-	acpi_sleep_prepare(ACPI_STATE_S5);
-	acpi_disable_all_gpes();
+	switch (system_state) {
+	case SYSTEM_POWER_OFF:
+		/* Prepare to power off the system */
+		acpi_sleep_prepare(ACPI_STATE_S5);
+		acpi_disable_all_gpes();
+		break;
+	default:
+		break;
+	}
  }
 
+static struct syscore_ops acpi_syscore_ops = {
+	.shutdown = acpi_shutdown,
+};
+
  static void acpi_power_off(void)
  {
  	/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
@@ -850,7 +861,7 @@ 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;
+		register_syscore_ops(&acpi_syscore_ops);
  		pm_power_off = acpi_power_off;
  	}
 
#4 lore raw

Re: [RFC PATCH 1/2] acpi: Use syscore instead of pm_power_off_prepare to prepare for poweroff

Quoted 11 lines · Guenter Roeck
Quoted 6 lines · Rafael J. Wysocki
Quoted 2 lines · Guenter Roeck

The syscore shutdown callback seems to be perfectly suited to prepare for system poweroff. Use it instead of pm_power_off_prepare.

How much testing did that receive?

As I mentioned in patch 0/2, compile tested so far only. Before I start playing with my servers, I wanted to get some feedback if the idea is worth pursuing further or if I am missing something essential.

Well, it makes sense in principle, but the ordering with respect to the other syscore shutdown things and the migrate_to_reboot_cpu() may be a problem.

I always get nervous when the ordering of ACPI-related code changes like that and the reason is quite weak this time to be honest.

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
#5 lore raw

Re: [RFC PATCH 1/2] acpi: Use syscore instead of pm_power_off_prepare to prepare for poweroff

Quoted 13 lines · Rafael J. Wysocki
Quoted 11 lines · Guenter Roeck
Quoted 6 lines · Rafael J. Wysocki
Quoted 2 lines · Guenter Roeck

The syscore shutdown callback seems to be perfectly suited to prepare for system poweroff. Use it instead of pm_power_off_prepare.

How much testing did that receive?

As I mentioned in patch 0/2, compile tested so far only. Before I start playing with my servers, I wanted to get some feedback if the idea is worth pursuing further or if I am missing something essential.

Quoted 6 lines

Well, it makes sense in principle, but the ordering with respect to the other syscore shutdown things and the migrate_to_reboot_cpu() may be a problem.

I always get nervous when the ordering of ACPI-related code changes like that and the reason is quite weak this time to be honest.

Ok, no problem; I'll drop it.

Guenter

#6 lore raw

RFC PATCH 2/2 kernel: power: Drop pm_poweroff_prepare

In reply to Guenter Roeck

The only user of pm_poweroff_prepare was acpi, which has been converted to use a syscore callback. Drop pm_poweroff_prepare as it is no longer necessary.

Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
Cc Pavel Machek <[email protected]>
---
 include/linux/pm.h | 1 -
 kernel/reboot.c    | 8 --------
 2 files changed, 9 deletions(-)
include/linux/pm.h +0 -1
index 383fd68..333506e 100644
@@ -32,7 +32,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
kernel/reboot.c +0 -8
index 5925f5a..5ad4f0b 100644
@@ -44,12 +44,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);
-
 /**
  *	emergency_restart - reboot the system
  *
@@ -257,8 +251,6 @@ 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();
 	migrate_to_reboot_cpu();
 	syscore_shutdown();
 	pr_emerg("Power down\n");
Signature · 1 line
1.9.1