[RFC PATCH 0/2] kernel: Drop pm_poweroff_prepare

[RFC PATCH 0/2] kernel: Drop pm_poweroff_prepare

[Guenter Roeck]

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 ?

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

[Guenter Roeck]

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 <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/acpi/sleep.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 05a31b5..e03c74d 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -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;
 	}
 
-- 
1.9.1

[RFC PATCH 2/2] kernel: power: Drop pm_poweroff_prepare

[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 <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 include/linux/pm.h | 1 -
 kernel/reboot.c    | 8 --------
 2 files changed, 9 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 383fd68..333506e 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -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
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 5925f5a..5ad4f0b 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -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");
-- 
1.9.1

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

[Guenter Roeck]

On 10/12/2014 12:45 PM, Rafael J. Wysocki wrote:
> On Saturday, October 11, 2014 02:14:16 PM Guenter Roeck wrote:
>> 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

>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Len Brown <len.brown@intel.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/acpi/sleep.c | 21 ++++++++++++++++-----
>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
>> index 05a31b5..e03c74d 100644
>> --- a/drivers/acpi/sleep.c
>> +++ b/drivers/acpi/sleep.c
>> @@ -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;
>>   	}
>>
>>
>

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

[Rafael J. Wysocki]

On Saturday, October 11, 2014 02:14:16 PM Guenter Roeck wrote:
> 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?

> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <len.brown@intel.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/acpi/sleep.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 05a31b5..e03c74d 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -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;
>  	}
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Guenter Roeck]

On 10/12/2014 01:16 PM, Rafael J. Wysocki wrote:
> On Sunday, October 12, 2014 12:35:22 PM Guenter Roeck wrote:
>> On 10/12/2014 12:45 PM, Rafael J. Wysocki wrote:
>>> On Saturday, October 11, 2014 02:14:16 PM Guenter Roeck wrote:
>>>> 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.
>

Ok, no problem; I'll drop it.

Guenter

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

[Rafael J. Wysocki]

On Sunday, October 12, 2014 12:35:22 PM Guenter Roeck wrote:
> On 10/12/2014 12:45 PM, Rafael J. Wysocki wrote:
> > On Saturday, October 11, 2014 02:14:16 PM Guenter Roeck wrote:
> >> 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.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.