[RFC][PATCH 0/2] PM: Update system power off code

[RFC][PATCH 0/2] PM: Update system power off code

[Rafael J. Wysocki]

Hi,

The following two patches update the system power off code.  They:
* remove the (unnecessary) call to acpi_sleep_prepare(ACPI_STATE_S4) from
  acpi_shutdown()
* remove acpi_sysclass and device_acpi that are only defined to register the
  ACPI power off preparation callback in acpi_shutdown() and introduce more
  general mechanism instead
The details are in the changelogs.

Comments welcome.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[RFC][PATCH 1/2] ACPI: Do not prepare for hibernation in acpi_shutdown

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Since we are now explicitly calling hibernation_ops->prepare() before
hibernation_ops->enter() in hibernation_platform_enter() (defined in
kernel/power/disk.c), ACPI should not call acpi_sleep_prepare(ACPI_STATE_S4)
from acpi_shutdown().

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep/poweroff.c |    3 ---
 1 file changed, 3 deletions(-)

Index: linux-2.6.22-rc6/drivers/acpi/sleep/poweroff.c
===================================================================
--- linux-2.6.22-rc6.orig/drivers/acpi/sleep/poweroff.c	2007-07-01 15:08:54.000000000 +0200
+++ linux-2.6.22-rc6/drivers/acpi/sleep/poweroff.c	2007-07-01 15:10:07.000000000 +0200
@@ -54,9 +54,6 @@ static int acpi_shutdown(struct sys_devi
 	case SYSTEM_POWER_OFF:
 		/* Prepare to power off the system */
 		return acpi_sleep_prepare(ACPI_STATE_S5);
-	case SYSTEM_SUSPEND_DISK:
-		/* Prepare to suspend the system to disk */
-		return acpi_sleep_prepare(ACPI_STATE_S4);
 	default:
 		return 0;
 	}

Re: [RFC][PATCH 1/2] ACPI: Do not prepare for hibernation in acpi_shutdown

[Pavel Machek]

Hi!

> Since we are now explicitly calling hibernation_ops->prepare() before
> hibernation_ops->enter() in hibernation_platform_enter() (defined in
> kernel/power/disk.c), ACPI should not call acpi_sleep_prepare(ACPI_STATE_S4)
> from acpi_shutdown().
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/sleep/poweroff.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> Index: linux-2.6.22-rc6/drivers/acpi/sleep/poweroff.c
> ===================================================================
> --- linux-2.6.22-rc6.orig/drivers/acpi/sleep/poweroff.c	2007-07-01 15:08:54.000000000 +0200
> +++ linux-2.6.22-rc6/drivers/acpi/sleep/poweroff.c	2007-07-01 15:10:07.000000000 +0200
> @@ -54,9 +54,6 @@ static int acpi_shutdown(struct sys_devi
>  	case SYSTEM_POWER_OFF:
>  		/* Prepare to power off the system */
>  		return acpi_sleep_prepare(ACPI_STATE_S5);
> -	case SYSTEM_SUSPEND_DISK:
> -		/* Prepare to suspend the system to disk */
> -		return acpi_sleep_prepare(ACPI_STATE_S4);
>  	default:
>  		return 0;

Hmm, I do not see why this is unneccessary... acpi wants to be
notified of poweroff, and hooks in a pretty reasonable
way. kernel/power does not seem to call acpi_sleep_prepare(). Hmm?

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 1/2] ACPI: Do not prepare for hibernation in acpi_shutdown

[Rafael J. Wysocki]

On Wednesday, 4 July 2007 21:49, Pavel Machek wrote:
> Hi!
> 
> > Since we are now explicitly calling hibernation_ops->prepare() before
> > hibernation_ops->enter() in hibernation_platform_enter() (defined in
> > kernel/power/disk.c), ACPI should not call acpi_sleep_prepare(ACPI_STATE_S4)
> > from acpi_shutdown().
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/acpi/sleep/poweroff.c |    3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > Index: linux-2.6.22-rc6/drivers/acpi/sleep/poweroff.c
> > ===================================================================
> > --- linux-2.6.22-rc6.orig/drivers/acpi/sleep/poweroff.c	2007-07-01 15:08:54.000000000 +0200
> > +++ linux-2.6.22-rc6/drivers/acpi/sleep/poweroff.c	2007-07-01 15:10:07.000000000 +0200
> > @@ -54,9 +54,6 @@ static int acpi_shutdown(struct sys_devi
> >  	case SYSTEM_POWER_OFF:
> >  		/* Prepare to power off the system */
> >  		return acpi_sleep_prepare(ACPI_STATE_S5);
> > -	case SYSTEM_SUSPEND_DISK:
> > -		/* Prepare to suspend the system to disk */
> > -		return acpi_sleep_prepare(ACPI_STATE_S4);
> >  	default:
> >  		return 0;
> 
> Hmm, I do not see why this is unneccessary... acpi wants to be
> notified of poweroff, and hooks in a pretty reasonable
> way. kernel/power does not seem to call acpi_sleep_prepare(). Hmm?

Please look at -mm.

We have

int hibernation_platform_enter(void)
{
	int error;

	if (hibernation_ops) {
		kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
		/*
		 * We have cancelled the power transition by running
		 * hibernation_ops->finish() before saving the image, so we
		 * should let the firmware know that we're going to enter the
		 * sleep state after all
		 */
		error = hibernation_ops->prepare();
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
		if (!error)
			error = hibernation_ops->enter();
	} else {
		error = -ENOSYS;
	}
	return error;
}

in there, which is exactly that.

The patch is necessary. :-)  Otherwise ->prepare() will be called twice in a row.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[RFC][PATCH 2/2] PM: Introduce pm_power_off_prepare

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Introduce the pm_power_off_prepare() callback that can be registered by the
interested platforms in analogy with pm_idle() and pm_power_off(), used for
preparing the system to power off (needed by ACPI).

This allows us to drop acpi_sysclass and device_acpi that are only defined in
order to register the ACPI power off preparation callback, which is needed by
pm_power_off() registered in a much different way.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep/poweroff.c |   38 +++++++++-----------------------------
 include/linux/pm.h            |    1 +
 kernel/sys.c                  |    9 +++++++++
 3 files changed, 19 insertions(+), 29 deletions(-)

Index: linux-2.6.22-rc6/include/linux/pm.h
===================================================================
--- linux-2.6.22-rc6.orig/include/linux/pm.h	2007-07-01 13:46:49.000000000 +0200
+++ linux-2.6.22-rc6/include/linux/pm.h	2007-07-01 18:37:28.000000000 +0200
@@ -101,6 +101,7 @@ struct pm_dev
  */
 extern void (*pm_idle)(void);
 extern void (*pm_power_off)(void);
+extern void (*pm_power_off_prepare)(void);
 
 /*
  * Device power management
Index: linux-2.6.22-rc6/kernel/sys.c
===================================================================
--- linux-2.6.22-rc6.orig/kernel/sys.c	2007-06-25 21:07:40.000000000 +0200
+++ linux-2.6.22-rc6/kernel/sys.c	2007-07-01 18:40:59.000000000 +0200
@@ -98,6 +98,13 @@ struct pid *cad_pid;
 EXPORT_SYMBOL(cad_pid);
 
 /*
+ * If set, this is used for preparing the system to power off.
+ */
+
+void (*pm_power_off_prepare)(void);
+EXPORT_SYMBOL(pm_power_off_prepare);
+
+/*
  *	Notifier list for kernel code which wants to be called
  *	at shutdown. This is used to stop any idling DMA operations
  *	and the like. 
@@ -865,6 +872,8 @@ 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();
 	printk(KERN_EMERG "Power down.\n");
 	machine_power_off();
 }
Index: linux-2.6.22-rc6/drivers/acpi/sleep/poweroff.c
===================================================================
--- linux-2.6.22-rc6.orig/drivers/acpi/sleep/poweroff.c	2007-07-01 15:10:07.000000000 +0200
+++ linux-2.6.22-rc6/drivers/acpi/sleep/poweroff.c	2007-07-01 18:40:09.000000000 +0200
@@ -39,7 +39,13 @@ int acpi_sleep_prepare(u32 acpi_state)
 
 #ifdef CONFIG_PM
 
-void acpi_power_off(void)
+static void acpi_power_off_prepare(void)
+{
+	/* Prepare to power off the system */
+	acpi_sleep_prepare(ACPI_STATE_S5);
+}
+
+static void acpi_power_off(void)
 {
 	/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
 	printk("%s called\n", __FUNCTION__);
@@ -48,27 +54,6 @@ void acpi_power_off(void)
 	acpi_enter_sleep_state(ACPI_STATE_S5);
 }
 
-static int acpi_shutdown(struct sys_device *x)
-{
-	switch (system_state) {
-	case SYSTEM_POWER_OFF:
-		/* Prepare to power off the system */
-		return acpi_sleep_prepare(ACPI_STATE_S5);
-	default:
-		return 0;
-	}
-}
-
-static struct sysdev_class acpi_sysclass = {
-	set_kset_name("acpi"),
-	.shutdown = acpi_shutdown
-};
-
-static struct sys_device device_acpi = {
-	.id = 0,
-	.cls = &acpi_sysclass,
-};
-
 static int acpi_poweroff_init(void)
 {
 	if (!acpi_disabled) {
@@ -78,13 +63,8 @@ static int acpi_poweroff_init(void)
 		status =
 		    acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 		if (ACPI_SUCCESS(status)) {
-			int error;
-			error = sysdev_class_register(&acpi_sysclass);
-			if (!error)
-				error = sysdev_register(&device_acpi);
-			if (!error)
-				pm_power_off = acpi_power_off;
-			return error;
+			pm_power_off_prepare = acpi_power_off_prepare;
+			pm_power_off = acpi_power_off;
 		}
 	}
 	return 0;

Re: [RFC][PATCH 2/2] PM: Introduce pm_power_off_prepare

[Pavel Machek]

On Sun 2007-07-01 20:54:31, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Introduce the pm_power_off_prepare() callback that can be registered by the
> interested platforms in analogy with pm_idle() and pm_power_off(), used for
> preparing the system to power off (needed by ACPI).
> 
> This allows us to drop acpi_sysclass and device_acpi that are only defined in
> order to register the ACPI power off preparation callback, which is needed by
> pm_power_off() registered in a much different way.

Well, the acpi way works with more than one piece of code "listening"
for powerdowns... and does not seem that ugly to me.

Okay, so accessing system_state is not _that_ nice... aha, and perhaps
I understand why 1/2 of this series is right... it called prepare for
S4 even when we had shutdown in /sys/power/disk, right?

Hmm. Okay, I think I can ACK both of these patches, but ACPI people
should have chance to comment, and it probably needs to stay in -mm
for a while.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 2/2] PM: Introduce pm_power_off_prepare

[Rafael J. Wysocki]

On Wednesday, 4 July 2007 21:52, Pavel Machek wrote:
> On Sun 2007-07-01 20:54:31, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Introduce the pm_power_off_prepare() callback that can be registered by the
> > interested platforms in analogy with pm_idle() and pm_power_off(), used for
> > preparing the system to power off (needed by ACPI).
> > 
> > This allows us to drop acpi_sysclass and device_acpi that are only defined in
> > order to register the ACPI power off preparation callback, which is needed by
> > pm_power_off() registered in a much different way.
> 
> Well, the acpi way works with more than one piece of code "listening"
> for powerdowns... and does not seem that ugly to me.
> 
> Okay, so accessing system_state is not _that_ nice... aha, and perhaps
> I understand why 1/2 of this series is right... it called prepare for
> S4 even when we had shutdown in /sys/power/disk, right?
> 
> Hmm. Okay, I think I can ACK both of these patches, but ACPI people
> should have chance to comment, and it probably needs to stay in -mm
> for a while.

Well, linux-acpi in on the CC list and I see no comments from them. :-)

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH 2/2] PM: Introduce pm_power_off_prepare

[Pavel Machek]

Hi!

> > > Introduce the pm_power_off_prepare() callback that can be registered by the
> > > interested platforms in analogy with pm_idle() and pm_power_off(), used for
> > > preparing the system to power off (needed by ACPI).
> > > 
> > > This allows us to drop acpi_sysclass and device_acpi that are only defined in
> > > order to register the ACPI power off preparation callback, which is needed by
> > > pm_power_off() registered in a much different way.
> > 
> > Well, the acpi way works with more than one piece of code "listening"
> > for powerdowns... and does not seem that ugly to me.
> > 
> > Okay, so accessing system_state is not _that_ nice... aha, and perhaps
> > I understand why 1/2 of this series is right... it called prepare for
> > S4 even when we had shutdown in /sys/power/disk, right?
> > 
> > Hmm. Okay, I think I can ACK both of these patches, but ACPI people
> > should have chance to comment, and it probably needs to stay in -mm
> > for a while.
> 
> Well, linux-acpi in on the CC list and I see no comments from them. :-)

Hmm, that's bad. We can try adding "APM rocks" into subject line to
get their attetion or something...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 2/2] PM: Introduce pm_power_off_prepare

[Rafael J. Wysocki]

On Wednesday, 4 July 2007 23:39, Pavel Machek wrote:
> Hi!
> 
> > > > Introduce the pm_power_off_prepare() callback that can be registered by the
> > > > interested platforms in analogy with pm_idle() and pm_power_off(), used for
> > > > preparing the system to power off (needed by ACPI).
> > > > 
> > > > This allows us to drop acpi_sysclass and device_acpi that are only defined in
> > > > order to register the ACPI power off preparation callback, which is needed by
> > > > pm_power_off() registered in a much different way.
> > > 
> > > Well, the acpi way works with more than one piece of code "listening"
> > > for powerdowns... and does not seem that ugly to me.
> > > 
> > > Okay, so accessing system_state is not _that_ nice... aha, and perhaps
> > > I understand why 1/2 of this series is right... it called prepare for
> > > S4 even when we had shutdown in /sys/power/disk, right?
> > > 
> > > Hmm. Okay, I think I can ACK both of these patches, but ACPI people
> > > should have chance to comment, and it probably needs to stay in -mm
> > > for a while.
> > 
> > Well, linux-acpi in on the CC list and I see no comments from them. :-)
> 
> Hmm, that's bad. We can try adding "APM rocks" into subject line to
> get their attetion or something...

Well, I take the lack of comments as the lack of objections. ;-)

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth