[PATCH 0/2] Patches for 2.6.27, dependent on the other trees

[PATCH 0/2] Patches for 2.6.27, dependent on the other trees

[Rafael J. Wysocki]

Hi Len,

The following patches are for the 2.6.27 queue, but they depend on some patches
in the other trees (most importantly, in the Greg's tree).

Thanks,
Rafael


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

[PATCH 1/2] ACPI PM: Add suspend sequence workaround

[Rafael J. Wysocki]

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

There are some systems out there that don't work correctly with
our current suspend/hibernation code ordering.  Provide a workaround
for these systems allowing them to pass the
'acpi_old_suspend_ordering' command line parameter to the kernel that
will make it use the pre-ACPI 2.0 suspend code ordering.

Unfortunately, that requires us to add a platform hook to the
resuming of devices for recovering the platform in case one of the
device drivers' .suspend() routines returns error code.  Namely,
ACPI 1.0 specifies that _PTS should be called before suspending
devices, but _WAK still should be called before resuming them in
order to undo the changes made by _PTS.  However, if there is an
error during suspending devices, they are automatically resumed
without returning control to the PM core, so the _WAK has to be
called from within device_resume() in that cases.

The patch also reorders and refactors the ACPI suspend/hibernation
code to avoid duplication as far as reasonably possible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/kernel-parameters.txt |    5 
 drivers/acpi/sleep/main.c           |  301 +++++++++++++++++++++---------------
 drivers/base/power/main.c           |   15 +
 include/linux/pm.h                  |    2 
 4 files changed, 201 insertions(+), 122 deletions(-)

Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================
--- linux-2.6.orig/Documentation/kernel-parameters.txt
+++ linux-2.6/Documentation/kernel-parameters.txt
@@ -170,6 +170,11 @@ and is between 256 and 4096 characters. 
 	acpi_irq_isa=	[HW,ACPI] If irq_balance, mark listed IRQs used by ISA
 			Format: <irq>,<irq>...
 
+       acpi_old_suspend_ordering [HW,ACPI]
+                       Enforce the ACPI 1.0 ordering of the _PTS control
+                       method wrt putting devices into low power states
+                       default: ACPI 2.0 ordering of _PTS
+
 	acpi_no_auto_ssdt	[HW,ACPI] Disable automatic loading of SSDT
 
 	acpi_os_name=	[HW,ACPI] Tell ACPI BIOS the name of the OS
Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -15,6 +15,7 @@
 #include <linux/dmi.h>
 #include <linux/device.h>
 #include <linux/suspend.h>
+#include <linux/pm.h>
 
 #include <asm/io.h>
 
@@ -24,10 +25,6 @@
 
 u8 sleep_states[ACPI_S_STATE_COUNT];
 
-#ifdef CONFIG_PM_SLEEP
-static u32 acpi_target_sleep_state = ACPI_STATE_S0;
-#endif
-
 static int acpi_sleep_prepare(u32 acpi_state)
 {
 #ifdef CONFIG_ACPI_SLEEP
@@ -50,9 +47,107 @@ static int acpi_sleep_prepare(u32 acpi_s
 	return 0;
 }
 
-#ifdef CONFIG_SUSPEND
-static struct platform_suspend_ops acpi_suspend_ops;
+#ifdef CONFIG_PM_SLEEP
+static u32 acpi_target_sleep_state = ACPI_STATE_S0;
+
+static int init_8259A_after_S1;
+
+/*
+ * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the
+ * user to request that behavior by using the 'acpi_old_suspend_ordering'
+ * kernel command line option that causes the following variable to be set.
+ */
+static bool old_suspend_ordering;
+
+static int __init acpi_old_suspend_ordering(char *str)
+{
+	old_suspend_ordering = true;
+	return 1;
+}
+__setup("acpi_old_suspend_ordering", acpi_old_suspend_ordering);
+
+/**
+ *	acpi_pm_disable_gpes - Disable the GPEs.
+ */
+static int acpi_pm_disable_gpes(void)
+{
+	acpi_hw_disable_all_gpes();
+	return 0;
+}
+
+/**
+ *	__acpi_pm_prepare - Prepare the platform to enter the target state.
+ *
+ *	If necessary, set the firmware waking vector and do arch-specific
+ *	nastiness to get the wakeup code to the waking vector.
+ */
+static int __acpi_pm_prepare(void)
+{
+	int error = acpi_sleep_prepare(acpi_target_sleep_state);
+
+	if (error)
+		acpi_target_sleep_state = ACPI_STATE_S0;
+	return error;
+}
+
+/**
+ *	acpi_pm_prepare - Prepare the platform to enter the target sleep
+ *		state and disable the GPEs.
+ */
+static int acpi_pm_prepare(void)
+{
+	int error = __acpi_pm_prepare();
+
+	if (!error)
+		acpi_hw_disable_all_gpes();
+	return error;
+}
+
+/**
+ *	acpi_pm_finish - Instruct the platform to leave a sleep state.
+ *
+ *	This is called after we wake back up (or if entering the sleep state
+ *	failed).
+ */
+static void acpi_pm_finish(void)
+{
+	u32 acpi_state = acpi_target_sleep_state;
+
+	if (acpi_state == ACPI_STATE_S0)
+		return;
+
+	printk(KERN_INFO PREFIX "Waking up from system sleep state S%d\n",
+		acpi_state);
+	acpi_disable_wakeup_device(acpi_state);
+	acpi_leave_sleep_state(acpi_state);
+
+	/* reset firmware waking vector */
+	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
+
+	acpi_target_sleep_state = ACPI_STATE_S0;
+
+#ifdef CONFIG_X86
+	if (acpi_state == ACPI_STATE_S1 && init_8259A_after_S1) {
+		printk("Broken toshiba laptop -> kicking interrupts\n");
+		init_8259A(0);
+	}
+#endif
+}
+
+/**
+ *	acpi_pm_end - Finish up suspend sequence.
+ */
+static void acpi_pm_end(void)
+{
+	/*
+	 * This is necessary in case acpi_pm_finish() is not called during a
+	 * failing transition to a sleep state.
+	 */
+	acpi_target_sleep_state = ACPI_STATE_S0;
+}
+#endif /* CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_SUSPEND
 extern void do_suspend_lowlevel(void);
 
 static u32 acpi_suspend_states[] = {
@@ -62,13 +157,10 @@ static u32 acpi_suspend_states[] = {
 	[PM_SUSPEND_MAX] = ACPI_STATE_S5
 };
 
-static int init_8259A_after_S1;
-
 /**
  *	acpi_suspend_begin - Set the target system sleep state to the state
  *		associated with given @pm_state, if supported.
  */
-
 static int acpi_suspend_begin(suspend_state_t pm_state)
 {
 	u32 acpi_state = acpi_suspend_states[pm_state];
@@ -85,25 +177,6 @@ static int acpi_suspend_begin(suspend_st
 }
 
 /**
- *	acpi_suspend_prepare - Do preliminary suspend work.
- *
- *	If necessary, set the firmware waking vector and do arch-specific
- *	nastiness to get the wakeup code to the waking vector.
- */
-
-static int acpi_suspend_prepare(void)
-{
-	int error = acpi_sleep_prepare(acpi_target_sleep_state);
-
-	if (error) {
-		acpi_target_sleep_state = ACPI_STATE_S0;
-		return error;
-	}
-
-	return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
-}
-
-/**
  *	acpi_suspend_enter - Actually enter a sleep state.
  *	@pm_state: ignored
  *
@@ -111,7 +184,6 @@ static int acpi_suspend_prepare(void)
  *	assembly, which in turn call acpi_enter_sleep_state().
  *	It's unfortunate, but it works. Please fix if you're feeling frisky.
  */
-
 static int acpi_suspend_enter(suspend_state_t pm_state)
 {
 	acpi_status status = AE_OK;
@@ -168,46 +240,6 @@ static int acpi_suspend_enter(suspend_st
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
-/**
- *	acpi_suspend_finish - Instruct the platform to leave a sleep state.
- *
- *	This is called after we wake back up (or if entering the sleep state
- *	failed). 
- */
-
-static void acpi_suspend_finish(void)
-{
-	u32 acpi_state = acpi_target_sleep_state;
-
-	acpi_disable_wakeup_device(acpi_state);
-	acpi_leave_sleep_state(acpi_state);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
-
-#ifdef CONFIG_X86
-	if (init_8259A_after_S1) {
-		printk("Broken toshiba laptop -> kicking interrupts\n");
-		init_8259A(0);
-	}
-#endif
-}
-
-/**
- *	acpi_suspend_end - Finish up suspend sequence.
- */
-
-static void acpi_suspend_end(void)
-{
-	/*
-	 * This is necessary in case acpi_suspend_finish() is not called during a
-	 * failing transition to a sleep state.
-	 */
-	acpi_target_sleep_state = ACPI_STATE_S0;
-}
-
 static int acpi_suspend_state_valid(suspend_state_t pm_state)
 {
 	u32 acpi_state;
@@ -227,10 +259,38 @@ static int acpi_suspend_state_valid(susp
 static struct platform_suspend_ops acpi_suspend_ops = {
 	.valid = acpi_suspend_state_valid,
 	.begin = acpi_suspend_begin,
-	.prepare = acpi_suspend_prepare,
+	.prepare = acpi_pm_prepare,
+	.enter = acpi_suspend_enter,
+	.finish = acpi_pm_finish,
+	.end = acpi_pm_end,
+};
+
+/**
+ *	acpi_suspend_begin_old - Set the target system sleep state to the
+ *		state associated with given @pm_state, if supported, and
+ *		execute the _PTS control method.  This function is used if the
+ *		pre-ACPI 2.0 suspend ordering has been requested.
+ */
+static int acpi_suspend_begin_old(suspend_state_t pm_state)
+{
+	int error = acpi_suspend_begin(pm_state);
+
+	if (!error)
+		error = __acpi_pm_prepare();
+	return error;
+}
+
+/*
+ * The following callbacks are used if the pre-ACPI 2.0 suspend ordering has
+ * been requested.
+ */
+static struct platform_suspend_ops acpi_suspend_ops_old = {
+	.valid = acpi_suspend_state_valid,
+	.begin = acpi_suspend_begin_old,
+	.prepare = acpi_pm_disable_gpes,
 	.enter = acpi_suspend_enter,
-	.finish = acpi_suspend_finish,
-	.end = acpi_suspend_end,
+	.finish = acpi_pm_finish,
+	.end = acpi_pm_end,
 };
 
 /*
@@ -269,22 +329,9 @@ __setup("acpi_s4_nosigcheck", acpi_s4_no
 static int acpi_hibernation_begin(void)
 {
 	acpi_target_sleep_state = ACPI_STATE_S4;
-
 	return 0;
 }
 
-static int acpi_hibernation_prepare(void)
-{
-	int error = acpi_sleep_prepare(ACPI_STATE_S4);
-
-	if (error) {
-		acpi_target_sleep_state = ACPI_STATE_S0;
-		return error;
-	}
-
-	return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
-}
-
 static int acpi_hibernation_enter(void)
 {
 	acpi_status status = AE_OK;
@@ -320,52 +367,54 @@ static void acpi_hibernation_leave(void)
 	}
 }
 
-static void acpi_hibernation_finish(void)
+static void acpi_pm_enable_gpes(void)
 {
-	acpi_disable_wakeup_device(ACPI_STATE_S4);
-	acpi_leave_sleep_state(ACPI_STATE_S4);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_hw_enable_all_runtime_gpes();
 }
 
-static void acpi_hibernation_end(void)
-{
-	/*
-	 * This is necessary in case acpi_hibernation_finish() is not called
-	 * during a failing transition to the sleep state.
-	 */
-	acpi_target_sleep_state = ACPI_STATE_S0;
-}
+static struct platform_hibernation_ops acpi_hibernation_ops = {
+	.begin = acpi_hibernation_begin,
+	.end = acpi_pm_end,
+	.pre_snapshot = acpi_pm_prepare,
+	.finish = acpi_pm_finish,
+	.prepare = acpi_pm_prepare,
+	.enter = acpi_hibernation_enter,
+	.leave = acpi_hibernation_leave,
+	.pre_restore = acpi_pm_disable_gpes,
+	.restore_cleanup = acpi_pm_enable_gpes,
+};
 
-static int acpi_hibernation_pre_restore(void)
+/**
+ *	acpi_hibernation_begin_old - Set the target system sleep state to
+ *		ACPI_STATE_S4 and execute the _PTS control method.  This
+ *		function is used if the pre-ACPI 2.0 suspend ordering has been
+ *		requested.
+ */
+static int acpi_hibernation_begin_old(void)
 {
-	acpi_status status;
-
-	status = acpi_hw_disable_all_gpes();
-
-	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
-}
+	int error = acpi_sleep_prepare(ACPI_STATE_S4);
 
-static void acpi_hibernation_restore_cleanup(void)
-{
-	acpi_hw_enable_all_runtime_gpes();
+	if (!error)
+		acpi_target_sleep_state = ACPI_STATE_S4;
+	return error;
 }
 
-static struct platform_hibernation_ops acpi_hibernation_ops = {
-	.begin = acpi_hibernation_begin,
-	.end = acpi_hibernation_end,
-	.pre_snapshot = acpi_hibernation_prepare,
-	.finish = acpi_hibernation_finish,
-	.prepare = acpi_hibernation_prepare,
+/*
+ * The following callbacks are used if the pre-ACPI 2.0 suspend ordering has
+ * been requested.
+ */
+static struct platform_hibernation_ops acpi_hibernation_ops_old = {
+	.begin = acpi_hibernation_begin_old,
+	.end = acpi_pm_end,
+	.pre_snapshot = acpi_pm_disable_gpes,
+	.finish = acpi_pm_finish,
+	.prepare = acpi_pm_disable_gpes,
 	.enter = acpi_hibernation_enter,
 	.leave = acpi_hibernation_leave,
-	.pre_restore = acpi_hibernation_pre_restore,
-	.restore_cleanup = acpi_hibernation_restore_cleanup,
+	.pre_restore = acpi_pm_disable_gpes,
+	.restore_cleanup = acpi_pm_enable_gpes,
 };
-#endif				/* CONFIG_HIBERNATION */
+#endif /* CONFIG_HIBERNATION */
 
 int acpi_suspend(u32 acpi_state)
 {
@@ -509,13 +558,15 @@ int __init acpi_sleep_init(void)
 		}
 	}
 
-	suspend_set_ops(&acpi_suspend_ops);
+	suspend_set_ops(old_suspend_ordering ?
+		&acpi_suspend_ops_old : &acpi_suspend_ops);
 #endif
 
 #ifdef CONFIG_HIBERNATION
 	status = acpi_get_sleep_type_data(ACPI_STATE_S4, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
-		hibernation_set_ops(&acpi_hibernation_ops);
+		hibernation_set_ops(old_suspend_ordering ?
+			&acpi_hibernation_ops_old : &acpi_hibernation_ops);
 		sleep_states[ACPI_STATE_S4] = 1;
 		printk(" S4");
 		if (!nosigcheck) {
@@ -527,6 +578,12 @@ int __init acpi_sleep_init(void)
 		}
 	}
 #endif
+
+#ifdef CONFIG_PM_SLEEP
+	if (old_suspend_ordering)
+		platform_pm_recover = acpi_pm_finish;
+#endif
+
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
 		sleep_states[ACPI_STATE_S5] = 1;
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -28,6 +28,20 @@
 #include "power.h"
 
 /*
+ * Some platforms (most importantly ACPI) may need to be prepared for
+ * resuming devices in case of a failure to suspend them.  They should set
+ * this pointer to the routine that will always be called just prior to resuming
+ * devices.
+ */
+void (*platform_pm_recover)(void);
+
+static inline void recover_platform(void)
+{
+	if (platform_pm_recover)
+		platform_pm_recover();
+}
+
+/*
  * The entries in the dpm_list list are in a depth first order, simply
  * because children are guaranteed to be discovered after parents, and
  * are inserted at the back of the list on discovery.
@@ -511,6 +525,7 @@ static void dpm_complete(pm_message_t st
 void device_resume(pm_message_t state)
 {
 	might_sleep();
+	recover_platform();
 	dpm_resume(state);
 	dpm_complete(state);
 }
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -445,6 +445,8 @@ struct dev_pm_info {
  */
 
 #ifdef CONFIG_PM_SLEEP
+extern void (*platform_pm_recover)(void);
+
 extern void device_pm_lock(void);
 extern void device_power_up(pm_message_t state);
 extern void device_resume(pm_message_t state);

Re: [PATCH 1/2] ACPI PM: Add suspend sequence workaround

[Carlos Corbacho]

On Tuesday 06 May 2008 22:44:30 Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> There are some systems out there that don't work correctly with
> our current suspend/hibernation code ordering.  Provide a workaround
> for these systems allowing them to pass the
> 'acpi_old_suspend_ordering' command line parameter to the kernel that
> will make it use the pre-ACPI 2.0 suspend code ordering.

Can we tack on a set of DMI matches to this somewhere to automatically apply 
this to the systems we know so far that need the old ordering?

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: [PATCH 1/2] ACPI PM: Add suspend sequence workaround

[Rafael J. Wysocki]

On Tuesday, 6 of May 2008, Carlos Corbacho wrote:
> On Tuesday 06 May 2008 22:44:30 Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > There are some systems out there that don't work correctly with
> > our current suspend/hibernation code ordering.  Provide a workaround
> > for these systems allowing them to pass the
> > 'acpi_old_suspend_ordering' command line parameter to the kernel that
> > will make it use the pre-ACPI 2.0 suspend code ordering.
> 
> Can we tack on a set of DMI matches to this somewhere to automatically apply 
> this to the systems we know so far that need the old ordering?

Yes, that will be the next step.

I think the command line argument may be useful anyway for checking if given
system is affected.

Thanks,
Rafael

Re: [PATCH 1/2] ACPI PM: Add suspend sequence workaround

[Pavel Machek]

Hi!
> ---
>  Documentation/kernel-parameters.txt |    5 
>  drivers/acpi/sleep/main.c           |  301 +++++++++++++++++++++---------------
>  drivers/base/power/main.c           |   15 +
>  include/linux/pm.h                  |    2 
>  4 files changed, 201 insertions(+), 122 deletions(-)
> 
> Index: linux-2.6/Documentation/kernel-parameters.txt
> ===================================================================
> --- linux-2.6.orig/Documentation/kernel-parameters.txt
> +++ linux-2.6/Documentation/kernel-parameters.txt
> @@ -170,6 +170,11 @@ and is between 256 and 4096 characters. 
>  	acpi_irq_isa=	[HW,ACPI] If irq_balance, mark listed IRQs used by ISA
>  			Format: <irq>,<irq>...
>  
> +       acpi_old_suspend_ordering [HW,ACPI]
> +                       Enforce the ACPI 1.0 ordering of the _PTS control
> +                       method wrt putting devices into low power states
> +                       default: ACPI 2.0 ordering of _PTS
> +

Space vs. tabs issue here, not too important...

> +#ifdef CONFIG_PM_SLEEP
> +static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> +
> +static int init_8259A_after_S1;

8259A after S1 init is workaround for bug in toshiba 4030cdt. We can
probably remove it now.

> +#ifdef CONFIG_X86
> +	if (acpi_state == ACPI_STATE_S1 && init_8259A_after_S1) {
> +		printk("Broken toshiba laptop -> kicking interrupts\n");
> +		init_8259A(0);
> +	}
> +#endif

This can die. I believe I've removed it already, but apparently not.

> +/*
> + * The following callbacks are used if the pre-ACPI 2.0 suspend ordering has
> + * been requested.
> + */
> +static struct platform_hibernation_ops acpi_hibernation_ops_old = {
> +	.begin = acpi_hibernation_begin_old,
> +	.end = acpi_pm_end,
> +	.pre_snapshot = acpi_pm_disable_gpes,
> +	.finish = acpi_pm_finish,
> +	.prepare = acpi_pm_disable_gpes,
>  	.enter = acpi_hibernation_enter,
>  	.leave = acpi_hibernation_leave,
> -	.pre_restore = acpi_hibernation_pre_restore,
> -	.restore_cleanup = acpi_hibernation_restore_cleanup,
> +	.pre_restore = acpi_pm_disable_gpes,
> +	.restore_cleanup = acpi_pm_enable_gpes,
>  };
> -#endif				/* CONFIG_HIBERNATION */
> +#endif /* CONFIG_HIBERNATION */

...

>  /*
> + * Some platforms (most importantly ACPI) may need to be prepared for
> + * resuming devices in case of a failure to suspend them.  They should set
> + * this pointer to the routine that will always be called just prior to resuming
> + * devices.
> + */
> +void (*platform_pm_recover)(void);

Should this be moved to acpi_suspend/hibernation ops? We already have
nice structures...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] ACPI PM: Add suspend sequence workaround

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Pavel Machek wrote:
> Hi!
> > ---
> >  Documentation/kernel-parameters.txt |    5 
> >  drivers/acpi/sleep/main.c           |  301 +++++++++++++++++++++---------------
> >  drivers/base/power/main.c           |   15 +
> >  include/linux/pm.h                  |    2 
> >  4 files changed, 201 insertions(+), 122 deletions(-)
> > 
> > Index: linux-2.6/Documentation/kernel-parameters.txt
> > ===================================================================
> > --- linux-2.6.orig/Documentation/kernel-parameters.txt
> > +++ linux-2.6/Documentation/kernel-parameters.txt
> > @@ -170,6 +170,11 @@ and is between 256 and 4096 characters. 
> >  	acpi_irq_isa=	[HW,ACPI] If irq_balance, mark listed IRQs used by ISA
> >  			Format: <irq>,<irq>...
> >  
> > +       acpi_old_suspend_ordering [HW,ACPI]
> > +                       Enforce the ACPI 1.0 ordering of the _PTS control
> > +                       method wrt putting devices into low power states
> > +                       default: ACPI 2.0 ordering of _PTS
> > +
> 
> Space vs. tabs issue here, not too important...
> 
> > +#ifdef CONFIG_PM_SLEEP
> > +static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> > +
> > +static int init_8259A_after_S1;
> 
> 8259A after S1 init is workaround for bug in toshiba 4030cdt. We can
> probably remove it now.
> 
> > +#ifdef CONFIG_X86
> > +	if (acpi_state == ACPI_STATE_S1 && init_8259A_after_S1) {
> > +		printk("Broken toshiba laptop -> kicking interrupts\n");
> > +		init_8259A(0);
> > +	}
> > +#endif
> 
> This can die. I believe I've removed it already, but apparently not.

Yes, but I'm going to add some analogous code for machines that are known to
need the "old" suspend ordering, so I've left that as a template. ;-)
 
> > +/*
> > + * The following callbacks are used if the pre-ACPI 2.0 suspend ordering has
> > + * been requested.
> > + */
> > +static struct platform_hibernation_ops acpi_hibernation_ops_old = {
> > +	.begin = acpi_hibernation_begin_old,
> > +	.end = acpi_pm_end,
> > +	.pre_snapshot = acpi_pm_disable_gpes,
> > +	.finish = acpi_pm_finish,
> > +	.prepare = acpi_pm_disable_gpes,
> >  	.enter = acpi_hibernation_enter,
> >  	.leave = acpi_hibernation_leave,
> > -	.pre_restore = acpi_hibernation_pre_restore,
> > -	.restore_cleanup = acpi_hibernation_restore_cleanup,
> > +	.pre_restore = acpi_pm_disable_gpes,
> > +	.restore_cleanup = acpi_pm_enable_gpes,
> >  };
> > -#endif				/* CONFIG_HIBERNATION */
> > +#endif /* CONFIG_HIBERNATION */
> 
> ...
> 
> >  /*
> > + * Some platforms (most importantly ACPI) may need to be prepared for
> > + * resuming devices in case of a failure to suspend them.  They should set
> > + * this pointer to the routine that will always be called just prior to resuming
> > + * devices.
> > + */
> > +void (*platform_pm_recover)(void);
> 
> Should this be moved to acpi_suspend/hibernation ops? We already have
> nice structures...

The catch here is that if device_suspend() fails, it doesn't return to the
caller, but invokes device_resume() directly.  While that could be changed, I'm
not sure if the final result would be much nicer (this is a workaround hack
this way or another).

Thanks,
Rafael

Re: [PATCH 1/2] ACPI PM: Add suspend sequence workaround

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Rafael J. Wysocki wrote:
> On Wednesday, 7 of May 2008, Pavel Machek wrote:
> > Hi!
> > > ---
> > >  Documentation/kernel-parameters.txt |    5 
> > >  drivers/acpi/sleep/main.c           |  301 +++++++++++++++++++++---------------
> > >  drivers/base/power/main.c           |   15 +
> > >  include/linux/pm.h                  |    2 
> > >  4 files changed, 201 insertions(+), 122 deletions(-)
> > > 
> > > Index: linux-2.6/Documentation/kernel-parameters.txt
> > > ===================================================================
> > > --- linux-2.6.orig/Documentation/kernel-parameters.txt
> > > +++ linux-2.6/Documentation/kernel-parameters.txt
> > > @@ -170,6 +170,11 @@ and is between 256 and 4096 characters. 
> > >  	acpi_irq_isa=	[HW,ACPI] If irq_balance, mark listed IRQs used by ISA
> > >  			Format: <irq>,<irq>...
> > >  
> > > +       acpi_old_suspend_ordering [HW,ACPI]
> > > +                       Enforce the ACPI 1.0 ordering of the _PTS control
> > > +                       method wrt putting devices into low power states
> > > +                       default: ACPI 2.0 ordering of _PTS
> > > +
> > 
> > Space vs. tabs issue here, not too important...
> > 
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> > > +
> > > +static int init_8259A_after_S1;
> > 
> > 8259A after S1 init is workaround for bug in toshiba 4030cdt. We can
> > probably remove it now.
> > 
> > > +#ifdef CONFIG_X86
> > > +	if (acpi_state == ACPI_STATE_S1 && init_8259A_after_S1) {
> > > +		printk("Broken toshiba laptop -> kicking interrupts\n");
> > > +		init_8259A(0);
> > > +	}
> > > +#endif
> > 
> > This can die. I believe I've removed it already, but apparently not.
> 
> Yes, but I'm going to add some analogous code for machines that are known to
> need the "old" suspend ordering, so I've left that as a template. ;-)

Okay, I guess we the $subject patch can be replaced with the two following
ones.

The first of them removes the Toshiba hook.

The second one adds a suspend sequence workaround by midifying the high-level
code and introducing new "recover" callbacks for the platform.

Then have only been compilation tested.

Please have a look.

Thanks,
Rafael

[RFC][PATCH 1/2] ACPI PM: Remove obsolete Toshiba workaround

[Rafael J. Wysocki]

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

Remove an obsolete workaround for a Toshiba Satellite 4030cdt
S1 problem from drivers/acpi/sleep/main.c .

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

Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -62,8 +62,6 @@ static u32 acpi_suspend_states[] = {
 	[PM_SUSPEND_MAX] = ACPI_STATE_S5
 };
 
-static int init_8259A_after_S1;
-
 /**
  *	acpi_suspend_begin - Set the target system sleep state to the state
  *		associated with given @pm_state, if supported.
@@ -186,13 +184,6 @@ static void acpi_suspend_finish(void)
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
 	acpi_target_sleep_state = ACPI_STATE_S0;
-
-#ifdef CONFIG_X86
-	if (init_8259A_after_S1) {
-		printk("Broken toshiba laptop -> kicking interrupts\n");
-		init_8259A(0);
-	}
-#endif
 }
 
 /**
@@ -232,26 +223,6 @@ static struct platform_suspend_ops acpi_
 	.finish = acpi_suspend_finish,
 	.end = acpi_suspend_end,
 };
-
-/*
- * Toshiba fails to preserve interrupts over S1, reinitialization
- * of 8259 is needed after S1 resume.
- */
-static int __init init_ints_after_s1(const struct dmi_system_id *d)
-{
-	printk(KERN_WARNING "%s with broken S1 detected.\n", d->ident);
-	init_8259A_after_S1 = 1;
-	return 0;
-}
-
-static struct dmi_system_id __initdata acpisleep_dmi_table[] = {
-	{
-	 .callback = init_ints_after_s1,
-	 .ident = "Toshiba Satellite 4030cdt",
-	 .matches = {DMI_MATCH(DMI_PRODUCT_NAME, "S4030CDT/4.3"),},
-	 },
-	{},
-};
 #endif /* CONFIG_SUSPEND */
 
 #ifdef CONFIG_HIBERNATION
@@ -490,8 +461,6 @@ int __init acpi_sleep_init(void)
 	u8 type_a, type_b;
 #ifdef CONFIG_SUSPEND
 	int i = 0;
-
-	dmi_check_system(acpisleep_dmi_table);
 #endif
 
 	if (acpi_disabled)

Re: [RFC][PATCH 1/2] ACPI PM: Remove obsolete Toshiba workaround

[Pavel Machek]

On Fri 2008-05-09 19:21:47, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Remove an obsolete workaround for a Toshiba Satellite 4030cdt
> S1 problem from drivers/acpi/sleep/main.c .
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.

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

[RFC][PATCH 2/2] ACPI PM: Add possibility to change suspend sequence

[Rafael J. Wysocki]

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

There are some systems out there that don't work correctly with
our current suspend/hibernation code ordering.  Provide a workaround
for these systems allowing them to pass the
'acpi_old_suspend_ordering' command line parameter to the kernel that
will make it use the pre-ACPI 2.0 suspend code ordering.

Unfortunately, that requires us to add a platform hook to the
resuming of devices for recovering the platform in case one of the
device drivers' .suspend() routines returns error code.  Namely,
ACPI 1.0 specifies that _PTS should be called before suspending
devices, but _WAK still should be called before resuming them in
order to undo the changes made by _PTS.  However, if there is an
error during suspending devices, they are automatically resumed
without returning control to the PM core, so the _WAK has to be
called from within device_resume() in that cases.

The patch also reorders and refactors the ACPI suspend/hibernation
code to avoid duplication as far as reasonably possible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/kernel-parameters.txt |    5 
 drivers/acpi/sleep/main.c           |  278 +++++++++++++++++++++---------------
 drivers/base/power/main.c           |    2 
 include/linux/suspend.h             |   14 +
 kernel/power/disk.c                 |   28 ++-
 kernel/power/main.c                 |    9 -
 6 files changed, 212 insertions(+), 124 deletions(-)

Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================
--- linux-2.6.orig/Documentation/kernel-parameters.txt
+++ linux-2.6/Documentation/kernel-parameters.txt
@@ -157,6 +157,11 @@ and is between 256 and 4096 characters. 
 			default: The hardware signature is checked during
 			         resume from S4.
 
+	acpi_old_suspend_ordering [HW,ACPI] ACPI suspend ordering
+			Enforce the ACPI 1.0 ordering of the _PTS control
+			method wrt putting devices into low power states
+			default: ACPI 2.0 ordering of _PTS
+
 	acpi_sci=	[HW,ACPI] ACPI System Control Interrupt trigger mode
 			Format: { level | edge | high | low }
 
Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -24,10 +24,6 @@
 
 u8 sleep_states[ACPI_S_STATE_COUNT];
 
-#ifdef CONFIG_PM_SLEEP
-static u32 acpi_target_sleep_state = ACPI_STATE_S0;
-#endif
-
 static int acpi_sleep_prepare(u32 acpi_state)
 {
 #ifdef CONFIG_ACPI_SLEEP
@@ -50,9 +46,98 @@ static int acpi_sleep_prepare(u32 acpi_s
 	return 0;
 }
 
-#ifdef CONFIG_SUSPEND
-static struct platform_suspend_ops acpi_suspend_ops;
+#ifdef CONFIG_PM_SLEEP
+static u32 acpi_target_sleep_state = ACPI_STATE_S0;
+
+/*
+ * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the
+ * user to request that behavior by using the 'acpi_old_suspend_ordering'
+ * kernel command line option that causes the following variable to be set.
+ */
+static bool old_suspend_ordering;
 
+static int __init acpi_old_suspend_ordering(char *str)
+{
+	old_suspend_ordering = true;
+	return 1;
+}
+__setup("acpi_old_suspend_ordering", acpi_old_suspend_ordering);
+
+/**
+ *	acpi_pm_disable_gpes - Disable the GPEs.
+ */
+static int acpi_pm_disable_gpes(void)
+{
+	acpi_hw_disable_all_gpes();
+	return 0;
+}
+
+/**
+ *	__acpi_pm_prepare - Prepare the platform to enter the target state.
+ *
+ *	If necessary, set the firmware waking vector and do arch-specific
+ *	nastiness to get the wakeup code to the waking vector.
+ */
+static int __acpi_pm_prepare(void)
+{
+	int error = acpi_sleep_prepare(acpi_target_sleep_state);
+
+	if (error)
+		acpi_target_sleep_state = ACPI_STATE_S0;
+	return error;
+}
+
+/**
+ *	acpi_pm_prepare - Prepare the platform to enter the target sleep
+ *		state and disable the GPEs.
+ */
+static int acpi_pm_prepare(void)
+{
+	int error = __acpi_pm_prepare();
+
+	if (!error)
+		acpi_hw_disable_all_gpes();
+	return error;
+}
+
+/**
+ *	acpi_pm_finish - Instruct the platform to leave a sleep state.
+ *
+ *	This is called after we wake back up (or if entering the sleep state
+ *	failed).
+ */
+static void acpi_pm_finish(void)
+{
+	u32 acpi_state = acpi_target_sleep_state;
+
+	if (acpi_state == ACPI_STATE_S0)
+		return;
+
+	printk(KERN_INFO PREFIX "Waking up from system sleep state S%d\n",
+		acpi_state);
+	acpi_disable_wakeup_device(acpi_state);
+	acpi_leave_sleep_state(acpi_state);
+
+	/* reset firmware waking vector */
+	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
+
+	acpi_target_sleep_state = ACPI_STATE_S0;
+}
+
+/**
+ *	acpi_pm_end - Finish up suspend sequence.
+ */
+static void acpi_pm_end(void)
+{
+	/*
+	 * This is necessary in case acpi_pm_finish() is not called during a
+	 * failing transition to a sleep state.
+	 */
+	acpi_target_sleep_state = ACPI_STATE_S0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_SUSPEND
 extern void do_suspend_lowlevel(void);
 
 static u32 acpi_suspend_states[] = {
@@ -66,7 +151,6 @@ static u32 acpi_suspend_states[] = {
  *	acpi_suspend_begin - Set the target system sleep state to the state
  *		associated with given @pm_state, if supported.
  */
-
 static int acpi_suspend_begin(suspend_state_t pm_state)
 {
 	u32 acpi_state = acpi_suspend_states[pm_state];
@@ -83,25 +167,6 @@ static int acpi_suspend_begin(suspend_st
 }
 
 /**
- *	acpi_suspend_prepare - Do preliminary suspend work.
- *
- *	If necessary, set the firmware waking vector and do arch-specific
- *	nastiness to get the wakeup code to the waking vector.
- */
-
-static int acpi_suspend_prepare(void)
-{
-	int error = acpi_sleep_prepare(acpi_target_sleep_state);
-
-	if (error) {
-		acpi_target_sleep_state = ACPI_STATE_S0;
-		return error;
-	}
-
-	return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
-}
-
-/**
  *	acpi_suspend_enter - Actually enter a sleep state.
  *	@pm_state: ignored
  *
@@ -109,7 +174,6 @@ static int acpi_suspend_prepare(void)
  *	assembly, which in turn call acpi_enter_sleep_state().
  *	It's unfortunate, but it works. Please fix if you're feeling frisky.
  */
-
 static int acpi_suspend_enter(suspend_state_t pm_state)
 {
 	acpi_status status = AE_OK;
@@ -166,39 +230,6 @@ static int acpi_suspend_enter(suspend_st
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
-/**
- *	acpi_suspend_finish - Instruct the platform to leave a sleep state.
- *
- *	This is called after we wake back up (or if entering the sleep state
- *	failed). 
- */
-
-static void acpi_suspend_finish(void)
-{
-	u32 acpi_state = acpi_target_sleep_state;
-
-	acpi_disable_wakeup_device(acpi_state);
-	acpi_leave_sleep_state(acpi_state);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
-}
-
-/**
- *	acpi_suspend_end - Finish up suspend sequence.
- */
-
-static void acpi_suspend_end(void)
-{
-	/*
-	 * This is necessary in case acpi_suspend_finish() is not called during a
-	 * failing transition to a sleep state.
-	 */
-	acpi_target_sleep_state = ACPI_STATE_S0;
-}
-
 static int acpi_suspend_state_valid(suspend_state_t pm_state)
 {
 	u32 acpi_state;
@@ -218,10 +249,39 @@ static int acpi_suspend_state_valid(susp
 static struct platform_suspend_ops acpi_suspend_ops = {
 	.valid = acpi_suspend_state_valid,
 	.begin = acpi_suspend_begin,
-	.prepare = acpi_suspend_prepare,
+	.prepare = acpi_pm_prepare,
+	.enter = acpi_suspend_enter,
+	.finish = acpi_pm_finish,
+	.end = acpi_pm_end,
+};
+
+/**
+ *	acpi_suspend_begin_old - Set the target system sleep state to the
+ *		state associated with given @pm_state, if supported, and
+ *		execute the _PTS control method.  This function is used if the
+ *		pre-ACPI 2.0 suspend ordering has been requested.
+ */
+static int acpi_suspend_begin_old(suspend_state_t pm_state)
+{
+	int error = acpi_suspend_begin(pm_state);
+
+	if (!error)
+		error = __acpi_pm_prepare();
+	return error;
+}
+
+/*
+ * The following callbacks are used if the pre-ACPI 2.0 suspend ordering has
+ * been requested.
+ */
+static struct platform_suspend_ops acpi_suspend_ops_old = {
+	.valid = acpi_suspend_state_valid,
+	.begin = acpi_suspend_begin_old,
+	.prepare = acpi_pm_disable_gpes,
 	.enter = acpi_suspend_enter,
-	.finish = acpi_suspend_finish,
-	.end = acpi_suspend_end,
+	.finish = acpi_pm_finish,
+	.end = acpi_pm_end,
+	.recover = acpi_pm_finish,
 };
 #endif /* CONFIG_SUSPEND */
 
@@ -240,22 +300,9 @@ __setup("acpi_s4_nosigcheck", acpi_s4_no
 static int acpi_hibernation_begin(void)
 {
 	acpi_target_sleep_state = ACPI_STATE_S4;
-
 	return 0;
 }
 
-static int acpi_hibernation_prepare(void)
-{
-	int error = acpi_sleep_prepare(ACPI_STATE_S4);
-
-	if (error) {
-		acpi_target_sleep_state = ACPI_STATE_S0;
-		return error;
-	}
-
-	return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
-}
-
 static int acpi_hibernation_enter(void)
 {
 	acpi_status status = AE_OK;
@@ -291,52 +338,55 @@ static void acpi_hibernation_leave(void)
 	}
 }
 
-static void acpi_hibernation_finish(void)
+static void acpi_pm_enable_gpes(void)
 {
-	acpi_disable_wakeup_device(ACPI_STATE_S4);
-	acpi_leave_sleep_state(ACPI_STATE_S4);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_hw_enable_all_runtime_gpes();
 }
 
-static void acpi_hibernation_end(void)
-{
-	/*
-	 * This is necessary in case acpi_hibernation_finish() is not called
-	 * during a failing transition to the sleep state.
-	 */
-	acpi_target_sleep_state = ACPI_STATE_S0;
-}
+static struct platform_hibernation_ops acpi_hibernation_ops = {
+	.begin = acpi_hibernation_begin,
+	.end = acpi_pm_end,
+	.pre_snapshot = acpi_pm_prepare,
+	.finish = acpi_pm_finish,
+	.prepare = acpi_pm_prepare,
+	.enter = acpi_hibernation_enter,
+	.leave = acpi_hibernation_leave,
+	.pre_restore = acpi_pm_disable_gpes,
+	.restore_cleanup = acpi_pm_enable_gpes,
+};
 
-static int acpi_hibernation_pre_restore(void)
+/**
+ *	acpi_hibernation_begin_old - Set the target system sleep state to
+ *		ACPI_STATE_S4 and execute the _PTS control method.  This
+ *		function is used if the pre-ACPI 2.0 suspend ordering has been
+ *		requested.
+ */
+static int acpi_hibernation_begin_old(void)
 {
-	acpi_status status;
-
-	status = acpi_hw_disable_all_gpes();
-
-	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
-}
+	int error = acpi_sleep_prepare(ACPI_STATE_S4);
 
-static void acpi_hibernation_restore_cleanup(void)
-{
-	acpi_hw_enable_all_runtime_gpes();
+	if (!error)
+		acpi_target_sleep_state = ACPI_STATE_S4;
+	return error;
 }
 
-static struct platform_hibernation_ops acpi_hibernation_ops = {
-	.begin = acpi_hibernation_begin,
-	.end = acpi_hibernation_end,
-	.pre_snapshot = acpi_hibernation_prepare,
-	.finish = acpi_hibernation_finish,
-	.prepare = acpi_hibernation_prepare,
+/*
+ * The following callbacks are used if the pre-ACPI 2.0 suspend ordering has
+ * been requested.
+ */
+static struct platform_hibernation_ops acpi_hibernation_ops_old = {
+	.begin = acpi_hibernation_begin_old,
+	.end = acpi_pm_end,
+	.pre_snapshot = acpi_pm_disable_gpes,
+	.finish = acpi_pm_finish,
+	.prepare = acpi_pm_disable_gpes,
 	.enter = acpi_hibernation_enter,
 	.leave = acpi_hibernation_leave,
-	.pre_restore = acpi_hibernation_pre_restore,
-	.restore_cleanup = acpi_hibernation_restore_cleanup,
+	.pre_restore = acpi_pm_disable_gpes,
+	.restore_cleanup = acpi_pm_enable_gpes,
+	.recover = acpi_pm_finish,
 };
-#endif				/* CONFIG_HIBERNATION */
+#endif /* CONFIG_HIBERNATION */
 
 int acpi_suspend(u32 acpi_state)
 {
@@ -478,13 +528,15 @@ int __init acpi_sleep_init(void)
 		}
 	}
 
-	suspend_set_ops(&acpi_suspend_ops);
+	suspend_set_ops(old_suspend_ordering ?
+		&acpi_suspend_ops_old : &acpi_suspend_ops);
 #endif
 
 #ifdef CONFIG_HIBERNATION
 	status = acpi_get_sleep_type_data(ACPI_STATE_S4, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
-		hibernation_set_ops(&acpi_hibernation_ops);
+		hibernation_set_ops(old_suspend_ordering ?
+			&acpi_hibernation_ops_old : &acpi_hibernation_ops);
 		sleep_states[ACPI_STATE_S4] = 1;
 		printk(" S4");
 		if (!nosigcheck) {
Index: linux-2.6/include/linux/suspend.h
===================================================================
--- linux-2.6.orig/include/linux/suspend.h
+++ linux-2.6/include/linux/suspend.h
@@ -86,6 +86,11 @@ typedef int __bitwise suspend_state_t;
  *	that implement @begin(), but platforms implementing @begin() should
  *	also provide a @end() which cleans up transitions aborted before
  *	@enter().
+ *
+ * @recover: Recover the platform from a suspend failure.
+ *	Called by the PM core if the suspending of devices fails.
+ *	This callback is optional and should only be implemented by platforms
+ *	which require special recovery actions in that situation.
  */
 struct platform_suspend_ops {
 	int (*valid)(suspend_state_t state);
@@ -94,6 +99,7 @@ struct platform_suspend_ops {
 	int (*enter)(suspend_state_t state);
 	void (*finish)(void);
 	void (*end)(void);
+	void (*recover)(void);
 };
 
 #ifdef CONFIG_SUSPEND
@@ -149,7 +155,7 @@ extern void mark_free_pages(struct zone 
  * The methods in this structure allow a platform to carry out special
  * operations required by it during a hibernation transition.
  *
- * All the methods below must be implemented.
+ * All the methods below, except for @recover(), must be implemented.
  *
  * @begin: Tell the platform driver that we're starting hibernation.
  *	Called right after shrinking memory and before freezing devices.
@@ -189,6 +195,11 @@ extern void mark_free_pages(struct zone 
  * @restore_cleanup: Clean up after a failing image restoration.
  *	Called right after the nonboot CPUs have been enabled and before
  *	thawing devices (runs with IRQs on).
+ *
+ * @recover: Recover the platform from a failure to suspend devices.
+ *	Called by the PM core if the suspending of devices during hibernation
+ *	fails.  This callback is optional and should only be implemented by
+ *	platforms which require special recovery actions in that situation.
  */
 struct platform_hibernation_ops {
 	int (*begin)(void);
@@ -200,6 +211,7 @@ struct platform_hibernation_ops {
 	void (*leave)(void);
 	int (*pre_restore)(void);
 	void (*restore_cleanup)(void);
+	void (*recover)(void);
 };
 
 #ifdef CONFIG_HIBERNATION
Index: linux-2.6/kernel/power/main.c
===================================================================
--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -269,11 +269,11 @@ int suspend_devices_and_enter(suspend_st
 	error = device_suspend(PMSG_SUSPEND);
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to suspend\n");
-		goto Resume_console;
+		goto Recover_platform;
 	}
 
 	if (suspend_test(TEST_DEVICES))
-		goto Resume_devices;
+		goto Recover_platform;
 
 	if (suspend_ops->prepare) {
 		error = suspend_ops->prepare();
@@ -300,6 +300,11 @@ int suspend_devices_and_enter(suspend_st
 	if (suspend_ops->end)
 		suspend_ops->end();
 	return error;
+
+ Recover_platform:
+	if (suspend_ops->recover)
+		suspend_ops->recover();
+	goto Resume_devices;
 }
 
 /**
Index: linux-2.6/kernel/power/disk.c
===================================================================
--- linux-2.6.orig/kernel/power/disk.c
+++ linux-2.6/kernel/power/disk.c
@@ -180,6 +180,17 @@ static void platform_restore_cleanup(int
 }
 
 /**
+ *	platform_recover - recover the platform from a failure to suspend
+ *	devices.
+ */
+
+static void platform_recover(int platform_mode)
+{
+	if (platform_mode && hibernation_ops && hibernation_ops->recover)
+		hibernation_ops->recover();
+}
+
+/**
  *	create_image - freeze devices that need to be frozen with interrupts
  *	off, create the hibernation image and thaw those devices.  Control
  *	reappears in this routine after a restore.
@@ -258,10 +269,10 @@ int hibernation_snapshot(int platform_mo
 	suspend_console();
 	error = device_suspend(PMSG_FREEZE);
 	if (error)
-		goto Resume_console;
+		goto Recover_platform;
 
 	if (hibernation_test(TEST_DEVICES))
-		goto Resume_devices;
+		goto Recover_platform;
 
 	error = platform_pre_snapshot(platform_mode);
 	if (error || hibernation_test(TEST_PLATFORM))
@@ -285,11 +296,14 @@ int hibernation_snapshot(int platform_mo
  Resume_devices:
 	device_resume(in_suspend ?
 		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
- Resume_console:
 	resume_console();
  Close:
 	platform_end(platform_mode);
 	return error;
+
+ Recover_platform:
+	platform_recover(platform_mode);
+	goto Resume_devices;
 }
 
 /**
@@ -398,8 +412,11 @@ int hibernation_platform_enter(void)
 
 	suspend_console();
 	error = device_suspend(PMSG_HIBERNATE);
-	if (error)
-		goto Resume_console;
+	if (error) {
+		if (hibernation_ops->recover)
+			hibernation_ops->recover();
+		goto Resume_devices;
+	}
 
 	error = hibernation_ops->prepare();
 	if (error)
@@ -428,7 +445,6 @@ int hibernation_platform_enter(void)
 	hibernation_ops->finish();
  Resume_devices:
 	device_resume(PMSG_RESTORE);
- Resume_console:
 	resume_console();
  Close:
 	hibernation_ops->end();
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -781,8 +781,6 @@ int device_suspend(pm_message_t state)
 	error = dpm_prepare(state);
 	if (!error)
 		error = dpm_suspend(state);
-	if (error)
-		device_resume(resume_event(state));
 	return error;
 }
 EXPORT_SYMBOL_GPL(device_suspend);

Re: [RFC][PATCH 2/2] ACPI PM: Add possibility to change suspend sequence

[Pavel Machek]

Hi!

> Index: linux-2.6/Documentation/kernel-parameters.txt
> ===================================================================
> --- linux-2.6.orig/Documentation/kernel-parameters.txt
> +++ linux-2.6/Documentation/kernel-parameters.txt
> @@ -157,6 +157,11 @@ and is between 256 and 4096 characters. 
>  			default: The hardware signature is checked during
>  			         resume from S4.
>  
> +	acpi_old_suspend_ordering [HW,ACPI] ACPI suspend ordering
> +			Enforce the ACPI 1.0 ordering of the _PTS control
> +			method wrt putting devices into low power states
> +			default: ACPI 2.0 ordering of _PTS
> +
>  	acpi_sci=	[HW,ACPI] ACPI System Control Interrupt trigger mode
>  			Format: { level | edge | high | low }
>

Hmm, I have a wild idea...

acpi_sleep=old_ordering

...and reuse the same variable we already use for
s3_bios/s3_mode. Then, we'd be able to blacklist the systems needing
old_ordering from s2ram...

I guess this whitelist should better be done in kernel, but short-term
ability to whitelist from userland could not hurt...?

									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] ACPI PM: Add possibility to change suspend sequence

[Rafael J. Wysocki]

On Monday, 12 of May 2008, Pavel Machek wrote:
> Hi!
> 
> > Index: linux-2.6/Documentation/kernel-parameters.txt
> > ===================================================================
> > --- linux-2.6.orig/Documentation/kernel-parameters.txt
> > +++ linux-2.6/Documentation/kernel-parameters.txt
> > @@ -157,6 +157,11 @@ and is between 256 and 4096 characters. 
> >  			default: The hardware signature is checked during
> >  			         resume from S4.
> >  
> > +	acpi_old_suspend_ordering [HW,ACPI] ACPI suspend ordering
> > +			Enforce the ACPI 1.0 ordering of the _PTS control
> > +			method wrt putting devices into low power states
> > +			default: ACPI 2.0 ordering of _PTS
> > +
> >  	acpi_sci=	[HW,ACPI] ACPI System Control Interrupt trigger mode
> >  			Format: { level | edge | high | low }
> >
> 
> Hmm, I have a wild idea...
> 
> acpi_sleep=old_ordering
> 
> ...and reuse the same variable we already use for
> s3_bios/s3_mode. Then, we'd be able to blacklist the systems needing
> old_ordering from s2ram...
> 
> I guess this whitelist should better be done in kernel, but short-term
> ability to whitelist from userland could not hurt...?

There's one problem with that, acpi_sleep is x86-specific and it's actually
handled in arch/x86/kernel/acpi/sleep.c .  Well, ok, I think I can handle that,
but it won't look nice.

Thanks,
Rafael

Re: [RFC][PATCH 2/2] ACPI PM: Add possibility to change suspend sequence

[Rafael J. Wysocki]

On Tuesday, 13 of May 2008, Rafael J. Wysocki wrote:
> On Monday, 12 of May 2008, Pavel Machek wrote:
> > Hi!
> > 
> > > Index: linux-2.6/Documentation/kernel-parameters.txt
> > > ===================================================================
> > > --- linux-2.6.orig/Documentation/kernel-parameters.txt
> > > +++ linux-2.6/Documentation/kernel-parameters.txt
> > > @@ -157,6 +157,11 @@ and is between 256 and 4096 characters. 
> > >  			default: The hardware signature is checked during
> > >  			         resume from S4.
> > >  
> > > +	acpi_old_suspend_ordering [HW,ACPI] ACPI suspend ordering
> > > +			Enforce the ACPI 1.0 ordering of the _PTS control
> > > +			method wrt putting devices into low power states
> > > +			default: ACPI 2.0 ordering of _PTS
> > > +
> > >  	acpi_sci=	[HW,ACPI] ACPI System Control Interrupt trigger mode
> > >  			Format: { level | edge | high | low }
> > >
> > 
> > Hmm, I have a wild idea...
> > 
> > acpi_sleep=old_ordering
> > 
> > ...and reuse the same variable we already use for
> > s3_bios/s3_mode. Then, we'd be able to blacklist the systems needing
> > old_ordering from s2ram...
> > 
> > I guess this whitelist should better be done in kernel, but short-term
> > ability to whitelist from userland could not hurt...?
> 
> There's one problem with that, acpi_sleep is x86-specific and it's actually
> handled in arch/x86/kernel/acpi/sleep.c .  Well, ok, I think I can handle that,
> but it won't look nice.

Below is the result, please have a look.

Thanks,
Rafael

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

There are some systems out there that don't work correctly with
our current suspend/hibernation code ordering.  Provide a workaround
for these systems allowing them to pass the
'acpi_sleep=old_ordering' in the kernel command line which will make
the kernel use the pre-ACPI 2.0 suspend code ordering.

Unfortunately, that requires us to rework the resuming of devices for
recovering the platform in case one of the device drivers' .suspend()
routines returns error code.  Namely, ACPI 1.0 specifies that _PTS
should be called before suspending devices, but _WAK still should be
called before resuming them in order to undo the changes made by
_PTS.  However, if there is an error during suspending devices, they
are automatically resumed without returning control to the PM core
and that has to be changed.  For this reason, the high-level suspend
and hibernation code has to be changed as well and new platform
callbacks are necessary.

The patch also reorders and refactors the ACPI suspend/hibernation
code to avoid duplication as far as reasonably possible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/kernel-parameters.txt |    6 
 arch/x86/kernel/acpi/sleep.c        |    2 
 drivers/acpi/sleep/main.c           |  276 +++++++++++++++++++++---------------
 drivers/base/power/main.c           |    2 
 include/linux/acpi.h                |    3 
 include/linux/suspend.h             |   14 +
 kernel/power/disk.c                 |   28 ++-
 kernel/power/main.c                 |   10 -
 8 files changed, 215 insertions(+), 126 deletions(-)

Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================
--- linux-2.6.orig/Documentation/kernel-parameters.txt
+++ linux-2.6/Documentation/kernel-parameters.txt
@@ -147,10 +147,14 @@ and is between 256 and 4096 characters. 
 			default: 0
 
 	acpi_sleep=	[HW,ACPI] Sleep options
-			Format: { s3_bios, s3_mode, s3_beep }
+			Format: { s3_bios, s3_mode, s3_beep, old_ordering }
 			See Documentation/power/video.txt for s3_bios and s3_mode.
 			s3_beep is for debugging; it makes the PC's speaker beep
 			as soon as the kernel's real-mode entry point is called.
+			old_ordering causes the ACPI 1.0 ordering of the _PTS
+			control method, wrt putting devices into low power
+			states, to be enforced (the ACPI 2.0 ordering of _PTS is
+			used by default).
 
 	acpi_s4_nosigcheck [HW,ACPI] S4 hardware signature
 			ACPI will ignore the S4 hardware signature
Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -24,10 +24,6 @@
 
 u8 sleep_states[ACPI_S_STATE_COUNT];
 
-#ifdef CONFIG_PM_SLEEP
-static u32 acpi_target_sleep_state = ACPI_STATE_S0;
-#endif
-
 static int acpi_sleep_prepare(u32 acpi_state)
 {
 #ifdef CONFIG_ACPI_SLEEP
@@ -50,9 +46,96 @@ static int acpi_sleep_prepare(u32 acpi_s
 	return 0;
 }
 
-#ifdef CONFIG_SUSPEND
-static struct platform_suspend_ops acpi_suspend_ops;
+#ifdef CONFIG_PM_SLEEP
+static u32 acpi_target_sleep_state = ACPI_STATE_S0;
+
+/*
+ * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the
+ * user to request that behavior by using the 'acpi_old_suspend_ordering'
+ * kernel command line option that causes the following variable to be set.
+ */
+static bool old_suspend_ordering;
+
+void __init acpi_old_suspend_ordering(void)
+{
+	old_suspend_ordering = true;
+}
+
+/**
+ *	acpi_pm_disable_gpes - Disable the GPEs.
+ */
+static int acpi_pm_disable_gpes(void)
+{
+	acpi_hw_disable_all_gpes();
+	return 0;
+}
+
+/**
+ *	__acpi_pm_prepare - Prepare the platform to enter the target state.
+ *
+ *	If necessary, set the firmware waking vector and do arch-specific
+ *	nastiness to get the wakeup code to the waking vector.
+ */
+static int __acpi_pm_prepare(void)
+{
+	int error = acpi_sleep_prepare(acpi_target_sleep_state);
+
+	if (error)
+		acpi_target_sleep_state = ACPI_STATE_S0;
+	return error;
+}
+
+/**
+ *	acpi_pm_prepare - Prepare the platform to enter the target sleep
+ *		state and disable the GPEs.
+ */
+static int acpi_pm_prepare(void)
+{
+	int error = __acpi_pm_prepare();
+
+	if (!error)
+		acpi_hw_disable_all_gpes();
+	return error;
+}
+
+/**
+ *	acpi_pm_finish - Instruct the platform to leave a sleep state.
+ *
+ *	This is called after we wake back up (or if entering the sleep state
+ *	failed).
+ */
+static void acpi_pm_finish(void)
+{
+	u32 acpi_state = acpi_target_sleep_state;
+
+	if (acpi_state == ACPI_STATE_S0)
+		return;
+
+	printk(KERN_INFO PREFIX "Waking up from system sleep state S%d\n",
+		acpi_state);
+	acpi_disable_wakeup_device(acpi_state);
+	acpi_leave_sleep_state(acpi_state);
+
+	/* reset firmware waking vector */
+	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
+
+	acpi_target_sleep_state = ACPI_STATE_S0;
+}
+
+/**
+ *	acpi_pm_end - Finish up suspend sequence.
+ */
+static void acpi_pm_end(void)
+{
+	/*
+	 * This is necessary in case acpi_pm_finish() is not called during a
+	 * failing transition to a sleep state.
+	 */
+	acpi_target_sleep_state = ACPI_STATE_S0;
+}
+#endif /* CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_SUSPEND
 extern void do_suspend_lowlevel(void);
 
 static u32 acpi_suspend_states[] = {
@@ -66,7 +149,6 @@ static u32 acpi_suspend_states[] = {
  *	acpi_suspend_begin - Set the target system sleep state to the state
  *		associated with given @pm_state, if supported.
  */
-
 static int acpi_suspend_begin(suspend_state_t pm_state)
 {
 	u32 acpi_state = acpi_suspend_states[pm_state];
@@ -83,25 +165,6 @@ static int acpi_suspend_begin(suspend_st
 }
 
 /**
- *	acpi_suspend_prepare - Do preliminary suspend work.
- *
- *	If necessary, set the firmware waking vector and do arch-specific
- *	nastiness to get the wakeup code to the waking vector.
- */
-
-static int acpi_suspend_prepare(void)
-{
-	int error = acpi_sleep_prepare(acpi_target_sleep_state);
-
-	if (error) {
-		acpi_target_sleep_state = ACPI_STATE_S0;
-		return error;
-	}
-
-	return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
-}
-
-/**
  *	acpi_suspend_enter - Actually enter a sleep state.
  *	@pm_state: ignored
  *
@@ -109,7 +172,6 @@ static int acpi_suspend_prepare(void)
  *	assembly, which in turn call acpi_enter_sleep_state().
  *	It's unfortunate, but it works. Please fix if you're feeling frisky.
  */
-
 static int acpi_suspend_enter(suspend_state_t pm_state)
 {
 	acpi_status status = AE_OK;
@@ -166,39 +228,6 @@ static int acpi_suspend_enter(suspend_st
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
-/**
- *	acpi_suspend_finish - Instruct the platform to leave a sleep state.
- *
- *	This is called after we wake back up (or if entering the sleep state
- *	failed). 
- */
-
-static void acpi_suspend_finish(void)
-{
-	u32 acpi_state = acpi_target_sleep_state;
-
-	acpi_disable_wakeup_device(acpi_state);
-	acpi_leave_sleep_state(acpi_state);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
-}
-
-/**
- *	acpi_suspend_end - Finish up suspend sequence.
- */
-
-static void acpi_suspend_end(void)
-{
-	/*
-	 * This is necessary in case acpi_suspend_finish() is not called during a
-	 * failing transition to a sleep state.
-	 */
-	acpi_target_sleep_state = ACPI_STATE_S0;
-}
-
 static int acpi_suspend_state_valid(suspend_state_t pm_state)
 {
 	u32 acpi_state;
@@ -218,10 +247,39 @@ static int acpi_suspend_state_valid(susp
 static struct platform_suspend_ops acpi_suspend_ops = {
 	.valid = acpi_suspend_state_valid,
 	.begin = acpi_suspend_begin,
-	.prepare = acpi_suspend_prepare,
+	.prepare = acpi_pm_prepare,
+	.enter = acpi_suspend_enter,
+	.finish = acpi_pm_finish,
+	.end = acpi_pm_end,
+};
+
+/**
+ *	acpi_suspend_begin_old - Set the target system sleep state to the
+ *		state associated with given @pm_state, if supported, and
+ *		execute the _PTS control method.  This function is used if the
+ *		pre-ACPI 2.0 suspend ordering has been requested.
+ */
+static int acpi_suspend_begin_old(suspend_state_t pm_state)
+{
+	int error = acpi_suspend_begin(pm_state);
+
+	if (!error)
+		error = __acpi_pm_prepare();
+	return error;
+}
+
+/*
+ * The following callbacks are used if the pre-ACPI 2.0 suspend ordering has
+ * been requested.
+ */
+static struct platform_suspend_ops acpi_suspend_ops_old = {
+	.valid = acpi_suspend_state_valid,
+	.begin = acpi_suspend_begin_old,
+	.prepare = acpi_pm_disable_gpes,
 	.enter = acpi_suspend_enter,
-	.finish = acpi_suspend_finish,
-	.end = acpi_suspend_end,
+	.finish = acpi_pm_finish,
+	.end = acpi_pm_end,
+	.recover = acpi_pm_finish,
 };
 #endif /* CONFIG_SUSPEND */
 
@@ -240,22 +298,9 @@ __setup("acpi_s4_nosigcheck", acpi_s4_no
 static int acpi_hibernation_begin(void)
 {
 	acpi_target_sleep_state = ACPI_STATE_S4;
-
 	return 0;
 }
 
-static int acpi_hibernation_prepare(void)
-{
-	int error = acpi_sleep_prepare(ACPI_STATE_S4);
-
-	if (error) {
-		acpi_target_sleep_state = ACPI_STATE_S0;
-		return error;
-	}
-
-	return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
-}
-
 static int acpi_hibernation_enter(void)
 {
 	acpi_status status = AE_OK;
@@ -291,52 +336,55 @@ static void acpi_hibernation_leave(void)
 	}
 }
 
-static void acpi_hibernation_finish(void)
+static void acpi_pm_enable_gpes(void)
 {
-	acpi_disable_wakeup_device(ACPI_STATE_S4);
-	acpi_leave_sleep_state(ACPI_STATE_S4);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_hw_enable_all_runtime_gpes();
 }
 
-static void acpi_hibernation_end(void)
-{
-	/*
-	 * This is necessary in case acpi_hibernation_finish() is not called
-	 * during a failing transition to the sleep state.
-	 */
-	acpi_target_sleep_state = ACPI_STATE_S0;
-}
+static struct platform_hibernation_ops acpi_hibernation_ops = {
+	.begin = acpi_hibernation_begin,
+	.end = acpi_pm_end,
+	.pre_snapshot = acpi_pm_prepare,
+	.finish = acpi_pm_finish,
+	.prepare = acpi_pm_prepare,
+	.enter = acpi_hibernation_enter,
+	.leave = acpi_hibernation_leave,
+	.pre_restore = acpi_pm_disable_gpes,
+	.restore_cleanup = acpi_pm_enable_gpes,
+};
 
-static int acpi_hibernation_pre_restore(void)
+/**
+ *	acpi_hibernation_begin_old - Set the target system sleep state to
+ *		ACPI_STATE_S4 and execute the _PTS control method.  This
+ *		function is used if the pre-ACPI 2.0 suspend ordering has been
+ *		requested.
+ */
+static int acpi_hibernation_begin_old(void)
 {
-	acpi_status status;
-
-	status = acpi_hw_disable_all_gpes();
-
-	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
-}
+	int error = acpi_sleep_prepare(ACPI_STATE_S4);
 
-static void acpi_hibernation_restore_cleanup(void)
-{
-	acpi_hw_enable_all_runtime_gpes();
+	if (!error)
+		acpi_target_sleep_state = ACPI_STATE_S4;
+	return error;
 }
 
-static struct platform_hibernation_ops acpi_hibernation_ops = {
-	.begin = acpi_hibernation_begin,
-	.end = acpi_hibernation_end,
-	.pre_snapshot = acpi_hibernation_prepare,
-	.finish = acpi_hibernation_finish,
-	.prepare = acpi_hibernation_prepare,
+/*
+ * The following callbacks are used if the pre-ACPI 2.0 suspend ordering has
+ * been requested.
+ */
+static struct platform_hibernation_ops acpi_hibernation_ops_old = {
+	.begin = acpi_hibernation_begin_old,
+	.end = acpi_pm_end,
+	.pre_snapshot = acpi_pm_disable_gpes,
+	.finish = acpi_pm_finish,
+	.prepare = acpi_pm_disable_gpes,
 	.enter = acpi_hibernation_enter,
 	.leave = acpi_hibernation_leave,
-	.pre_restore = acpi_hibernation_pre_restore,
-	.restore_cleanup = acpi_hibernation_restore_cleanup,
+	.pre_restore = acpi_pm_disable_gpes,
+	.restore_cleanup = acpi_pm_enable_gpes,
+	.recover = acpi_pm_finish,
 };
-#endif				/* CONFIG_HIBERNATION */
+#endif /* CONFIG_HIBERNATION */
 
 int acpi_suspend(u32 acpi_state)
 {
@@ -478,13 +526,15 @@ int __init acpi_sleep_init(void)
 		}
 	}
 
-	suspend_set_ops(&acpi_suspend_ops);
+	suspend_set_ops(old_suspend_ordering ?
+		&acpi_suspend_ops_old : &acpi_suspend_ops);
 #endif
 
 #ifdef CONFIG_HIBERNATION
 	status = acpi_get_sleep_type_data(ACPI_STATE_S4, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
-		hibernation_set_ops(&acpi_hibernation_ops);
+		hibernation_set_ops(old_suspend_ordering ?
+			&acpi_hibernation_ops_old : &acpi_hibernation_ops);
 		sleep_states[ACPI_STATE_S4] = 1;
 		printk(" S4");
 		if (!nosigcheck) {
Index: linux-2.6/include/linux/suspend.h
===================================================================
--- linux-2.6.orig/include/linux/suspend.h
+++ linux-2.6/include/linux/suspend.h
@@ -86,6 +86,11 @@ typedef int __bitwise suspend_state_t;
  *	that implement @begin(), but platforms implementing @begin() should
  *	also provide a @end() which cleans up transitions aborted before
  *	@enter().
+ *
+ * @recover: Recover the platform from a suspend failure.
+ *	Called by the PM core if the suspending of devices fails.
+ *	This callback is optional and should only be implemented by platforms
+ *	which require special recovery actions in that situation.
  */
 struct platform_suspend_ops {
 	int (*valid)(suspend_state_t state);
@@ -94,6 +99,7 @@ struct platform_suspend_ops {
 	int (*enter)(suspend_state_t state);
 	void (*finish)(void);
 	void (*end)(void);
+	void (*recover)(void);
 };
 
 #ifdef CONFIG_SUSPEND
@@ -149,7 +155,7 @@ extern void mark_free_pages(struct zone 
  * The methods in this structure allow a platform to carry out special
  * operations required by it during a hibernation transition.
  *
- * All the methods below must be implemented.
+ * All the methods below, except for @recover(), must be implemented.
  *
  * @begin: Tell the platform driver that we're starting hibernation.
  *	Called right after shrinking memory and before freezing devices.
@@ -189,6 +195,11 @@ extern void mark_free_pages(struct zone 
  * @restore_cleanup: Clean up after a failing image restoration.
  *	Called right after the nonboot CPUs have been enabled and before
  *	thawing devices (runs with IRQs on).
+ *
+ * @recover: Recover the platform from a failure to suspend devices.
+ *	Called by the PM core if the suspending of devices during hibernation
+ *	fails.  This callback is optional and should only be implemented by
+ *	platforms which require special recovery actions in that situation.
  */
 struct platform_hibernation_ops {
 	int (*begin)(void);
@@ -200,6 +211,7 @@ struct platform_hibernation_ops {
 	void (*leave)(void);
 	int (*pre_restore)(void);
 	void (*restore_cleanup)(void);
+	void (*recover)(void);
 };
 
 #ifdef CONFIG_HIBERNATION
Index: linux-2.6/kernel/power/main.c
===================================================================
--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -269,11 +269,11 @@ int suspend_devices_and_enter(suspend_st
 	error = device_suspend(PMSG_SUSPEND);
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to suspend\n");
-		goto Resume_console;
+		goto Recover_platform;
 	}
 
 	if (suspend_test(TEST_DEVICES))
-		goto Resume_devices;
+		goto Recover_platform;
 
 	if (suspend_ops->prepare) {
 		error = suspend_ops->prepare();
@@ -294,12 +294,16 @@ int suspend_devices_and_enter(suspend_st
 		suspend_ops->finish();
  Resume_devices:
 	device_resume(PMSG_RESUME);
- Resume_console:
 	resume_console();
  Close:
 	if (suspend_ops->end)
 		suspend_ops->end();
 	return error;
+
+ Recover_platform:
+	if (suspend_ops->recover)
+		suspend_ops->recover();
+	goto Resume_devices;
 }
 
 /**
Index: linux-2.6/kernel/power/disk.c
===================================================================
--- linux-2.6.orig/kernel/power/disk.c
+++ linux-2.6/kernel/power/disk.c
@@ -180,6 +180,17 @@ static void platform_restore_cleanup(int
 }
 
 /**
+ *	platform_recover - recover the platform from a failure to suspend
+ *	devices.
+ */
+
+static void platform_recover(int platform_mode)
+{
+	if (platform_mode && hibernation_ops && hibernation_ops->recover)
+		hibernation_ops->recover();
+}
+
+/**
  *	create_image - freeze devices that need to be frozen with interrupts
  *	off, create the hibernation image and thaw those devices.  Control
  *	reappears in this routine after a restore.
@@ -258,10 +269,10 @@ int hibernation_snapshot(int platform_mo
 	suspend_console();
 	error = device_suspend(PMSG_FREEZE);
 	if (error)
-		goto Resume_console;
+		goto Recover_platform;
 
 	if (hibernation_test(TEST_DEVICES))
-		goto Resume_devices;
+		goto Recover_platform;
 
 	error = platform_pre_snapshot(platform_mode);
 	if (error || hibernation_test(TEST_PLATFORM))
@@ -285,11 +296,14 @@ int hibernation_snapshot(int platform_mo
  Resume_devices:
 	device_resume(in_suspend ?
 		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
- Resume_console:
 	resume_console();
  Close:
 	platform_end(platform_mode);
 	return error;
+
+ Recover_platform:
+	platform_recover(platform_mode);
+	goto Resume_devices;
 }
 
 /**
@@ -398,8 +412,11 @@ int hibernation_platform_enter(void)
 
 	suspend_console();
 	error = device_suspend(PMSG_HIBERNATE);
-	if (error)
-		goto Resume_console;
+	if (error) {
+		if (hibernation_ops->recover)
+			hibernation_ops->recover();
+		goto Resume_devices;
+	}
 
 	error = hibernation_ops->prepare();
 	if (error)
@@ -428,7 +445,6 @@ int hibernation_platform_enter(void)
 	hibernation_ops->finish();
  Resume_devices:
 	device_resume(PMSG_RESTORE);
- Resume_console:
 	resume_console();
  Close:
 	hibernation_ops->end();
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -781,8 +781,6 @@ int device_suspend(pm_message_t state)
 	error = dpm_prepare(state);
 	if (!error)
 		error = dpm_suspend(state);
-	if (error)
-		device_resume(resume_event(state));
 	return error;
 }
 EXPORT_SYMBOL_GPL(device_suspend);
Index: linux-2.6/arch/x86/kernel/acpi/sleep.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/acpi/sleep.c
+++ linux-2.6/arch/x86/kernel/acpi/sleep.c
@@ -124,6 +124,8 @@ static int __init acpi_sleep_setup(char 
 			acpi_realmode_flags |= 2;
 		if (strncmp(str, "s3_beep", 7) == 0)
 			acpi_realmode_flags |= 4;
+		if (strncmp(str, "old_ordering", 12) == 0)
+			acpi_old_suspend_ordering();
 		str = strchr(str, ',');
 		if (str != NULL)
 			str += strspn(str, ", \t");
Index: linux-2.6/include/linux/acpi.h
===================================================================
--- linux-2.6.orig/include/linux/acpi.h
+++ linux-2.6/include/linux/acpi.h
@@ -233,6 +233,9 @@ int acpi_check_region(resource_size_t st
 		      const char *name);
 int acpi_check_mem_region(resource_size_t start, resource_size_t n,
 		      const char *name);
+#ifdef CONFIG_PM_SLEEP
+void __init acpi_old_suspend_ordering(void);
+#endif /* CONFIG_PM_SLEEP */
 
 #else	/* CONFIG_ACPI */
 

Re: [RFC][PATCH 2/2] ACPI PM: Add possibility to change suspend sequence

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> There are some systems out there that don't work correctly with
> our current suspend/hibernation code ordering.  Provide a workaround
> for these systems allowing them to pass the
> 'acpi_sleep=old_ordering' in the kernel command line which will make
> the kernel use the pre-ACPI 2.0 suspend code ordering.
> 
> Unfortunately, that requires us to rework the resuming of devices for
> recovering the platform in case one of the device drivers' .suspend()
> routines returns error code.  Namely, ACPI 1.0 specifies that _PTS
> should be called before suspending devices, but _WAK still should be
> called before resuming them in order to undo the changes made by
> _PTS.  However, if there is an error during suspending devices, they
> are automatically resumed without returning control to the PM core
> and that has to be changed.  For this reason, the high-level suspend
> and hibernation code has to be changed as well and new platform
> callbacks are necessary.
> 
> The patch also reorders and refactors the ACPI suspend/hibernation
> code to avoid duplication as far as reasonably possible.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

I like it. ACK.
								Pavel

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

[PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

[Well, this essentially is a PCI patch, but it modifies include/linux/pm.h too.]
---
From: Rafael J. Wysocki <rjw@sisk.pl>

The new suspend and hibernation callbacks introduced with
'struct pm_ops' and 'struct pm_ext_ops' do not take a
pm_message_t argument, so the drivers using them will not be able
to use pci_choose_state() in its present form.  For this reason,
introduce the new function pci_preferred_state() playing the role
of pci_choose_state(), but taking only a pointer to the device
object.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
 include/linux/pci.h |    1 +
 include/linux/pm.h  |   10 ++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
- 
+
+/**
+ * pci_preferred_state - Choose the preferred power state of a PCI device
+ * @dev: PCI device to be put into the low power state
+ * @sp: Information aboutabout what the driver would prefer to do with
+ *	the device if there were no platform-implemeted policy.
+ *
+ * Returns PCI power state suitable for given device and given suspend policy.
+ * The policy, however, is only used if platform_pci_choose_state() fails or is
+ * not present.  Otherwise, it is assumed that platform_pci_choose_state()
+ * implements the right policy.
+ */
+
+pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
+{
+	pci_power_t ret;
+
+	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+		return PCI_D0;
+
+	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
+	if (platform_pci_choose_state) {
+		pci_power_t platform_ret = platform_pci_choose_state(dev);
+
+		if (platform_ret != PCI_POWER_ERROR)
+			ret = platform_ret;
+	}
+	return ret;
+}
+
+EXPORT_SYMBOL(pci_preferred_state);
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -615,6 +615,7 @@ size_t pci_get_rom_size(void __iomem *ro
 int pci_save_state(struct pci_dev *dev);
 int pci_restore_state(struct pci_dev *dev);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp);
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable);
 
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -391,6 +391,16 @@ struct dev_pm_info {
 };
 
 /*
+ * Used to pass the information about what the driver would prefer to do with
+ * its device before the system in put into a sleep state, if there were no
+ * platform-implemeted policy.
+ */
+enum suspend_policy {
+	SP_TURN_ON,
+	SP_TURN_OFF,
+};
+
+/*
  * The PM_EVENT_ messages are also used by drivers implementing the legacy
  * suspend framework, based on the ->suspend() and ->resume() callbacks common
  * for suspend and hibernation transitions, according to the rules below.

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The new suspend and hibernation callbacks introduced with
> 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> pm_message_t argument, so the drivers using them will not be able
> to use pci_choose_state() in its present form.  For this reason,
> introduce the new function pci_preferred_state() playing the role
> of pci_choose_state(), but taking only a pointer to the device
> object.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
>  include/linux/pci.h |    1 +
>  include/linux/pm.h  |   10 ++++++++++
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
>  }
>  
>  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> - 
> +
> +/**
> + * pci_preferred_state - Choose the preferred power state of a PCI device
> + * @dev: PCI device to be put into the low power state
> + * @sp: Information aboutabout what the driver would prefer to do with
> + *	the device if there were no platform-implemeted policy.
> + *
> + * Returns PCI power state suitable for given device and given suspend policy.
> + * The policy, however, is only used if platform_pci_choose_state() fails or is
> + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> + * implements the right policy.
> + */
> +
> +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> +{
> +	pci_power_t ret;
> +
> +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> +		return PCI_D0;
> +
> +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> +	if (platform_pci_choose_state) {
> +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> +
> +		if (platform_ret != PCI_POWER_ERROR)
> +			ret = platform_ret;
> +	}
> +	return ret;
> +}
> +
> +EXPORT_SYMBOL(pci_preferred_state);

I don't get it. How is driver supposed to use this? How does the
driver decide between SP_TURN_OFF and SP_TURN_ON?

...and it seems to be clearer to just inline this in the driver... or
pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
define...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The new suspend and hibernation callbacks introduced with
> > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > pm_message_t argument, so the drivers using them will not be able
> > to use pci_choose_state() in its present form.  For this reason,
> > introduce the new function pci_preferred_state() playing the role
> > of pci_choose_state(), but taking only a pointer to the device
> > object.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
> >  include/linux/pci.h |    1 +
> >  include/linux/pm.h  |   10 ++++++++++
> >  3 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/drivers/pci/pci.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/pci.c
> > +++ linux-2.6/drivers/pci/pci.c
> > @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
> >  }
> >  
> >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > - 
> > +
> > +/**
> > + * pci_preferred_state - Choose the preferred power state of a PCI device
> > + * @dev: PCI device to be put into the low power state
> > + * @sp: Information aboutabout what the driver would prefer to do with
> > + *	the device if there were no platform-implemeted policy.
> > + *
> > + * Returns PCI power state suitable for given device and given suspend policy.
> > + * The policy, however, is only used if platform_pci_choose_state() fails or is
> > + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> > + * implements the right policy.
> > + */
> > +
> > +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> > +{
> > +	pci_power_t ret;
> > +
> > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > +		return PCI_D0;
> > +
> > +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> > +	if (platform_pci_choose_state) {
> > +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> > +
> > +		if (platform_ret != PCI_POWER_ERROR)
> > +			ret = platform_ret;
> > +	}
> > +	return ret;
> > +}
> > +
> > +EXPORT_SYMBOL(pci_preferred_state);
> 
> I don't get it. How is driver supposed to use this? How does the
> driver decide between SP_TURN_OFF and SP_TURN_ON?
> 
> ...and it seems to be clearer to just inline this in the driver... or
> pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
> define...

I thought about that too.  I'd like to know what the other people think,
though.

Thanks,
Rafael

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Alan Stern]

On Wed, 7 May 2008, Rafael J. Wysocki wrote:

> On Wednesday, 7 of May 2008, Pavel Machek wrote:
> > Hi!
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > The new suspend and hibernation callbacks introduced with
> > > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > > pm_message_t argument, so the drivers using them will not be able
> > > to use pci_choose_state() in its present form.  For this reason,
> > > introduce the new function pci_preferred_state() playing the role
> > > of pci_choose_state(), but taking only a pointer to the device
> > > object.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
> > >  include/linux/pci.h |    1 +
> > >  include/linux/pm.h  |   10 ++++++++++
> > >  3 files changed, 43 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6/drivers/pci/pci.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/pci/pci.c
> > > +++ linux-2.6/drivers/pci/pci.c
> > > @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
> > >  }
> > >  
> > >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > > - 
> > > +
> > > +/**
> > > + * pci_preferred_state - Choose the preferred power state of a PCI device
> > > + * @dev: PCI device to be put into the low power state
> > > + * @sp: Information aboutabout what the driver would prefer to do with
> > > + *	the device if there were no platform-implemeted policy.
> > > + *
> > > + * Returns PCI power state suitable for given device and given suspend policy.
> > > + * The policy, however, is only used if platform_pci_choose_state() fails or is
> > > + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> > > + * implements the right policy.
> > > + */
> > > +
> > > +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> > > +{
> > > +	pci_power_t ret;
> > > +
> > > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > +		return PCI_D0;
> > > +
> > > +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> > > +	if (platform_pci_choose_state) {
> > > +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> > > +
> > > +		if (platform_ret != PCI_POWER_ERROR)
> > > +			ret = platform_ret;
> > > +	}
> > > +	return ret;
> > > +}
> > > +
> > > +EXPORT_SYMBOL(pci_preferred_state);
> > 
> > I don't get it. How is driver supposed to use this? How does the
> > driver decide between SP_TURN_OFF and SP_TURN_ON?
> > 
> > ...and it seems to be clearer to just inline this in the driver... or
> > pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
> > define...
> 
> I thought about that too.  I'd like to know what the other people think,
> though.

The point of this isn't at all clear.

Is this routine meant to be called during a hibernation 
transition?  Or is it just for suspend?

And why would the return value ever be anything other than D3_hot?  (Or 
why would the driver ever want to put a device in a different state?)
AFAICS, the only reason would be because platform_pci_choose_state() 
suggested something else.  In which case there's no need for the 
"policy" argument.

Alan Stern

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Alan Stern wrote:
> On Wed, 7 May 2008, Rafael J. Wysocki wrote:
> 
> > On Wednesday, 7 of May 2008, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > The new suspend and hibernation callbacks introduced with
> > > > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > > > pm_message_t argument, so the drivers using them will not be able
> > > > to use pci_choose_state() in its present form.  For this reason,
> > > > introduce the new function pci_preferred_state() playing the role
> > > > of pci_choose_state(), but taking only a pointer to the device
> > > > object.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > ---
> > > >  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
> > > >  include/linux/pci.h |    1 +
> > > >  include/linux/pm.h  |   10 ++++++++++
> > > >  3 files changed, 43 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/drivers/pci/pci.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/pci/pci.c
> > > > +++ linux-2.6/drivers/pci/pci.c
> > > > @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
> > > >  }
> > > >  
> > > >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > > > - 
> > > > +
> > > > +/**
> > > > + * pci_preferred_state - Choose the preferred power state of a PCI device
> > > > + * @dev: PCI device to be put into the low power state
> > > > + * @sp: Information aboutabout what the driver would prefer to do with
> > > > + *	the device if there were no platform-implemeted policy.
> > > > + *
> > > > + * Returns PCI power state suitable for given device and given suspend policy.
> > > > + * The policy, however, is only used if platform_pci_choose_state() fails or is
> > > > + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> > > > + * implements the right policy.
> > > > + */
> > > > +
> > > > +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> > > > +{
> > > > +	pci_power_t ret;
> > > > +
> > > > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > > +		return PCI_D0;
> > > > +
> > > > +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> > > > +	if (platform_pci_choose_state) {
> > > > +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> > > > +
> > > > +		if (platform_ret != PCI_POWER_ERROR)
> > > > +			ret = platform_ret;
> > > > +	}
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +EXPORT_SYMBOL(pci_preferred_state);
> > > 
> > > I don't get it. How is driver supposed to use this? How does the
> > > driver decide between SP_TURN_OFF and SP_TURN_ON?
> > > 
> > > ...and it seems to be clearer to just inline this in the driver... or
> > > pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
> > > define...
> > 
> > I thought about that too.  I'd like to know what the other people think,
> > though.
> 
> The point of this isn't at all clear.
> 
> Is this routine meant to be called during a hibernation 
> transition?

Yes, it is.

> Or is it just for suspend? 
> 
> And why would the return value ever be anything other than D3_hot?  (Or 
> why would the driver ever want to put a device in a different state?)

In principle, the driver may want to put the device into a state having shorter
wake up latency than D3_hot.

> AFAICS, the only reason would be because platform_pci_choose_state() 
> suggested something else.  In which case there's no need for the 
> "policy" argument.

There is a need in two cases:
- if platform_pci_choose_state() is not defined (it only is defined for ACPI
  systems at the moment),
- if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it cannot
  handle the device.

I agree with Pavel that the driver could pass a "fallback state" as a second
argument to be used in case the platform cannot provide it with one.

Thanks,
Rafael

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Wednesday, 7 of May 2008, Rafael J. Wysocki wrote:
> On Wednesday, 7 of May 2008, Alan Stern wrote:
> > On Wed, 7 May 2008, Rafael J. Wysocki wrote:
> > 
> > > On Wednesday, 7 of May 2008, Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > 
> > > > > The new suspend and hibernation callbacks introduced with
> > > > > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > > > > pm_message_t argument, so the drivers using them will not be able
> > > > > to use pci_choose_state() in its present form.  For this reason,
> > > > > introduce the new function pci_preferred_state() playing the role
> > > > > of pci_choose_state(), but taking only a pointer to the device
> > > > > object.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > ---
> > > > >  drivers/pci/pci.c   |   33 ++++++++++++++++++++++++++++++++-
> > > > >  include/linux/pci.h |    1 +
> > > > >  include/linux/pm.h  |   10 ++++++++++
> > > > >  3 files changed, 43 insertions(+), 1 deletion(-)
> > > > > 
> > > > > Index: linux-2.6/drivers/pci/pci.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/pci/pci.c
> > > > > +++ linux-2.6/drivers/pci/pci.c
> > > > > @@ -509,7 +509,38 @@ pci_set_power_state(struct pci_dev *dev,
> > > > >  }
> > > > >  
> > > > >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > > > > - 
> > > > > +
> > > > > +/**
> > > > > + * pci_preferred_state - Choose the preferred power state of a PCI device
> > > > > + * @dev: PCI device to be put into the low power state
> > > > > + * @sp: Information aboutabout what the driver would prefer to do with
> > > > > + *	the device if there were no platform-implemeted policy.
> > > > > + *
> > > > > + * Returns PCI power state suitable for given device and given suspend policy.
> > > > > + * The policy, however, is only used if platform_pci_choose_state() fails or is
> > > > > + * not present.  Otherwise, it is assumed that platform_pci_choose_state()
> > > > > + * implements the right policy.
> > > > > + */
> > > > > +
> > > > > +pci_power_t pci_preferred_state(struct pci_dev *dev, enum suspend_policy sp)
> > > > > +{
> > > > > +	pci_power_t ret;
> > > > > +
> > > > > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > > > +		return PCI_D0;
> > > > > +
> > > > > +	ret = (sp == SP_TURN_OFF) ? PCI_D3hot : PCI_D0;
> > > > > +	if (platform_pci_choose_state) {
> > > > > +		pci_power_t platform_ret = platform_pci_choose_state(dev);
> > > > > +
> > > > > +		if (platform_ret != PCI_POWER_ERROR)
> > > > > +			ret = platform_ret;
> > > > > +	}
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +EXPORT_SYMBOL(pci_preferred_state);
> > > > 
> > > > I don't get it. How is driver supposed to use this? How does the
> > > > driver decide between SP_TURN_OFF and SP_TURN_ON?
> > > > 
> > > > ...and it seems to be clearer to just inline this in the driver... or
> > > > pass  PCI_D3hot/PCI_D0 to it, instead of inventing yet another
> > > > define...
> > > 
> > > I thought about that too.  I'd like to know what the other people think,
> > > though.
> > 
> > The point of this isn't at all clear.
> > 
> > Is this routine meant to be called during a hibernation 
> > transition?
> 
> Yes, it is.
> 
> > Or is it just for suspend? 
> > 
> > And why would the return value ever be anything other than D3_hot?  (Or 
> > why would the driver ever want to put a device in a different state?)
> 
> In principle, the driver may want to put the device into a state having shorter
> wake up latency than D3_hot.
> 
> > AFAICS, the only reason would be because platform_pci_choose_state() 
> > suggested something else.  In which case there's no need for the 
> > "policy" argument.
> 
> There is a need in two cases:
> - if platform_pci_choose_state() is not defined (it only is defined for ACPI
>   systems at the moment),
> - if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it cannot
>   handle the device.
> 
> I agree with Pavel that the driver could pass a "fallback state" as a second
> argument to be used in case the platform cannot provide it with one.

Modified patch follows.

Thanks,
Rafael

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

The new suspend and hibernation callbacks introduced with
'struct pm_ops' and 'struct pm_ext_ops' do not take a
pm_message_t argument, so the drivers using them will not be able
to use pci_choose_state() in its present form.  For this reason,
introduce the new function pci_preferred_state() playing the role
of pci_choose_state(), but taking only the pointer to the device
object.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c   |   28 +++++++++++++++++++++++++++-
 include/linux/pci.h |    1 +
 2 files changed, 28 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -509,7 +509,33 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
- 
+
+/**
+ * pci_preferred_state - Choose the preferred power state of a PCI device
+ * @dev: PCI device to be put into the low power state
+ * @state: State to put the device into if the platform cannot handle it
+ *
+ * Returns PCI power state suitable for given device according to the platform.
+ * However, if platform_pci_choose_state() is not defined or returns
+ * PCI_POWER_ERROR, @state is returned.
+ */
+
+pci_power_t pci_preferred_state(struct pci_dev *dev, pci_power_t state)
+{
+	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+		return state;
+
+	if (platform_pci_choose_state) {
+		pci_power_t ret = platform_pci_choose_state(dev);
+
+		if (ret != PCI_POWER_ERROR)
+			state = ret;
+	}
+	return state;
+}
+
+EXPORT_SYMBOL(pci_preferred_state);
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -615,6 +615,7 @@ size_t pci_get_rom_size(void __iomem *ro
 int pci_save_state(struct pci_dev *dev);
 int pci_restore_state(struct pci_dev *dev);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+pci_power_t pci_preferred_state(struct pci_dev *dev, pci_power_t state);
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable);
 

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Jesse Barnes]

> > > > I thought about that too.  I'd like to know what the other people
> > > > think, though.
> > >
> > > The point of this isn't at all clear.
> > >
> > > Is this routine meant to be called during a hibernation
> > > transition?
> >
> > Yes, it is.
> >
> > > Or is it just for suspend?
> > >
> > > And why would the return value ever be anything other than D3_hot?  (Or
> > > why would the driver ever want to put a device in a different state?)
> >
> > In principle, the driver may want to put the device into a state having
> > shorter wake up latency than D3_hot.
> >
> > > AFAICS, the only reason would be because platform_pci_choose_state()
> > > suggested something else.  In which case there's no need for the
> > > "policy" argument.
> >
> > There is a need in two cases:
> > - if platform_pci_choose_state() is not defined (it only is defined for
> > ACPI systems at the moment),
> > - if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it
> > cannot handle the device.
> >
> > I agree with Pavel that the driver could pass a "fallback state" as a
> > second argument to be used in case the platform cannot provide it with
> > one.
>
> Modified patch follows.

So why not make platform_pci_choose_state do:
+ pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t 
state)
+ {
+       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+               return state;
+ }

instead?  Then in the PCI core we would assign either 
platform_pci_choose_state to acpi_pci_choose_state or noacpi_pci_choose_state 
(though that's a bad name).

But really, since drivers should probably know what power state to put their 
devices in for suspend & hibernate, maybe on non-ACPI systems the function 
should just return an error and the driver can choose...

Jesse

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Friday, 9 of May 2008, Jesse Barnes wrote:
> > > > > I thought about that too.  I'd like to know what the other people
> > > > > think, though.
> > > >
> > > > The point of this isn't at all clear.
> > > >
> > > > Is this routine meant to be called during a hibernation
> > > > transition?
> > >
> > > Yes, it is.
> > >
> > > > Or is it just for suspend?
> > > >
> > > > And why would the return value ever be anything other than D3_hot?  (Or
> > > > why would the driver ever want to put a device in a different state?)
> > >
> > > In principle, the driver may want to put the device into a state having
> > > shorter wake up latency than D3_hot.
> > >
> > > > AFAICS, the only reason would be because platform_pci_choose_state()
> > > > suggested something else.  In which case there's no need for the
> > > > "policy" argument.
> > >
> > > There is a need in two cases:
> > > - if platform_pci_choose_state() is not defined (it only is defined for
> > > ACPI systems at the moment),
> > > - if platform_pci_choose_state() returns PCI_POWER_ERROR meaning that it
> > > cannot handle the device.
> > >
> > > I agree with Pavel that the driver could pass a "fallback state" as a
> > > second argument to be used in case the platform cannot provide it with
> > > one.
> >
> > Modified patch follows.
> 
> So why not make platform_pci_choose_state do:
> + pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t 
> state)
> + {
> +       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> +               return state;
> + }
> 
> instead?  Then in the PCI core we would assign either 
> platform_pci_choose_state to acpi_pci_choose_state or noacpi_pci_choose_state 

Good idea.

> (though that's a bad name).

Does generic_pci_choose_state() sound better?

> But really, since drivers should probably know what power state to put their 
> devices in for suspend & hibernate, maybe on non-ACPI systems the function 
> should just return an error and the driver can choose...

That's one possibility too, but in that case many drivers will do

state = pci_preferred_state(dev);
if (state == PCI_POWER_ERROR)
	state = something;

It's just shorter to write

state = pci_preferred_state(dev, something);

Thanks,
Rafael

Re: [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Jesse Barnes]

On Friday, May 09, 2008 10:13 am Rafael J. Wysocki wrote:
> > So why not make platform_pci_choose_state do:
> > + pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t
> > state)
> > + {
> > +       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > +               return state;
> > + }
> >
> > instead?  Then in the PCI core we would assign either
> > platform_pci_choose_state to acpi_pci_choose_state or
> > noacpi_pci_choose_state
>
> Good idea.
>
> > (though that's a bad name).
>
> Does generic_pci_choose_state() sound better?

Yeah, that's better.

> > But really, since drivers should probably know what power state to put
> > their devices in for suspend & hibernate, maybe on non-ACPI systems the
> > function should just return an error and the driver can choose...
>
> That's one possibility too, but in that case many drivers will do
>
> state = pci_preferred_state(dev);
> if (state == PCI_POWER_ERROR)
> 	state = something;
>
> It's just shorter to write
>
> state = pci_preferred_state(dev, something);

But really that's the idea, since if the core doesn't know what state your 
device should be in (and in many non-ACPI cases I'd argue that to be true) 
your driver should be picking something sensible.  After all, states other 
than D0 and D3 are really device dependent, right?

One way to avoid some ugliness like you show above would be:

device_suspend(...)
{
  ...
  state = PCI_D3hot;
  pci_choose_state(dev, pm_state, &state);
  pci_set_power_state(dev, state);
  ...
}

So in this case pci_choose_state would either change state or leave it 
untouched if it didn't have a better idea about things.  But now that I look 
at it I'm not sure it's an improvement. :)

Jesse

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Friday, 9 of May 2008, Jesse Barnes wrote:
> On Friday, May 09, 2008 10:13 am Rafael J. Wysocki wrote:
> > > So why not make platform_pci_choose_state do:
> > > + pci_power_t noacpi_pci_choose_state(struct pci_dev *dev, pci_message_t
> > > state)
> > > + {
> > > +       if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > +               return state;
> > > + }
> > >
> > > instead?  Then in the PCI core we would assign either
> > > platform_pci_choose_state to acpi_pci_choose_state or
> > > noacpi_pci_choose_state
> >
> > Good idea.
> >
> > > (though that's a bad name).
> >
> > Does generic_pci_choose_state() sound better?
> 
> Yeah, that's better.
> 
> > > But really, since drivers should probably know what power state to put
> > > their devices in for suspend & hibernate, maybe on non-ACPI systems the
> > > function should just return an error and the driver can choose...
> >
> > That's one possibility too, but in that case many drivers will do
> >
> > state = pci_preferred_state(dev);
> > if (state == PCI_POWER_ERROR)
> > 	state = something;
> >
> > It's just shorter to write
> >
> > state = pci_preferred_state(dev, something);
> 
> But really that's the idea, since if the core doesn't know what state your 
> device should be in (and in many non-ACPI cases I'd argue that to be true) 
> your driver should be picking something sensible.  After all, states other 
> than D0 and D3 are really device dependent, right?
> 
> One way to avoid some ugliness like you show above would be:
> 
> device_suspend(...)
> {
>   ...
>   state = PCI_D3hot;
>   pci_choose_state(dev, pm_state, &state);
>   pci_set_power_state(dev, state);
>   ...
> }
> 
> So in this case pci_choose_state would either change state or leave it 
> untouched if it didn't have a better idea about things.  But now that I look 
> at it I'm not sure it's an improvement. :)

Well, in principle we could go farther and introduce a wrapper around
pci_set_power_state() that will call platform_pci_choose_state() to obtain the
new state or use the driver-provided one if that fails.

What do you think?

Rafael

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Jesse Barnes]

On Friday, May 09, 2008 10:34 am Rafael J. Wysocki wrote:
> > So in this case pci_choose_state would either change state or leave it
> > untouched if it didn't have a better idea about things.  But now that I
> > look at it I'm not sure it's an improvement. :)
>
> Well, in principle we could go farther and introduce a wrapper around
> pci_set_power_state() that will call platform_pci_choose_state() to obtain
> the new state or use the driver-provided one if that fails.

Hm, yeah that sounds pretty reasonable actually.  Especially given that so 
many drivers just do:
  pci_set_power_state(pdev, pci_choose_state(pdev, state));
anyway...

Jesse

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Friday, 9 of May 2008, Jesse Barnes wrote:
> On Friday, May 09, 2008 10:34 am Rafael J. Wysocki wrote:
> > > So in this case pci_choose_state would either change state or leave it
> > > untouched if it didn't have a better idea about things.  But now that I
> > > look at it I'm not sure it's an improvement. :)
> >
> > Well, in principle we could go farther and introduce a wrapper around
> > pci_set_power_state() that will call platform_pci_choose_state() to obtain
> > the new state or use the driver-provided one if that fails.
> 
> Hm, yeah that sounds pretty reasonable actually.  Especially given that so 
> many drivers just do:
>   pci_set_power_state(pdev, pci_choose_state(pdev, state));
> anyway...

Okay, what about this:

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

The new suspend and hibernation callbacks introduced with
'struct pm_ops' and 'struct pm_ext_ops' do not take a
pm_message_t argument, so the drivers using them will not be able
to use pci_choose_state() in its present form.  For this reason,
introduce a new function, pci_choose_and_set_state(), playing the
role of pci_choose_state() combined with pci_set_power_state() and
allowing the driver to put the device into a power state chosen by
the platform.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c   |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h |    1 +
 2 files changed, 46 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -509,7 +509,51 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
- 
+
+/**
+ * pci_choose_and_set_state - Choose the power state of a PCI device and put
+ *                            the device into that state.
+ * @dev: PCI device to be put into a low power state
+ * @state: State to put the device into by default
+ *
+ * Use the platform driver to choose the preferred PCI power state of given
+ * device and put the device into that state.  If the target power state of
+ * the device cannot be chosen using the platform driver, the driver-provided
+ * @state is used.
+ *
+ * RETURN VALUE:
+ * -EINVAL if trying to enter a lower state than we're already in.
+ * 0 if we're already in the requested state.
+ * -EIO if device does not support PCI PM.
+ * 0 if we can successfully change the power state.
+ */
+
+int pci_choose_and_set_state(struct pci_dev *dev, pci_power_t state)
+{
+	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+		return -EIO;
+
+	if (platform_pci_choose_state) {
+		pci_power_t ret = platform_pci_choose_state(dev);
+
+		switch (ret) {
+		case PCI_POWER_ERROR:
+		case PCI_UNKNOWN:
+			break;
+		case PCI_D1:
+		case PCI_D2:
+			if (pci_no_d1d2(dev))
+				break;
+		default:
+			state = ret;
+		}
+	}
+
+	return pci_set_power_state(dev, state);
+}
+
+EXPORT_SYMBOL(pci_choose_and_set_state);
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -615,6 +615,7 @@ size_t pci_get_rom_size(void __iomem *ro
 int pci_save_state(struct pci_dev *dev);
 int pci_restore_state(struct pci_dev *dev);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+int pci_choose_and_set_state(struct pci_dev *dev, pci_power_t state);
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable);
 

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Jesse Barnes]

On Friday, May 09, 2008 2:44 pm Rafael J. Wysocki wrote:
> Okay, what about this:
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> The new suspend and hibernation callbacks introduced with
> 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> pm_message_t argument, so the drivers using them will not be able
> to use pci_choose_state() in its present form.  For this reason,
> introduce a new function, pci_choose_and_set_state(), playing the
> role of pci_choose_state() combined with pci_set_power_state() and
> allowing the driver to put the device into a power state chosen by
> the platform.

Yeah, that looks pretty good.  The name is long but I can't think of a better 
one offhand.  Can you also update Documentation/power/pci.txt with the latest 
best practices?  I wonder if we should do a pass through the drivers 
converting them to this interface as well...

Thanks,
Jesse

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Saturday, 10 of May 2008, Jesse Barnes wrote:
> On Friday, May 09, 2008 2:44 pm Rafael J. Wysocki wrote:
> > Okay, what about this:
> >
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > The new suspend and hibernation callbacks introduced with
> > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > pm_message_t argument, so the drivers using them will not be able
> > to use pci_choose_state() in its present form.  For this reason,
> > introduce a new function, pci_choose_and_set_state(), playing the
> > role of pci_choose_state() combined with pci_set_power_state() and
> > allowing the driver to put the device into a power state chosen by
> > the platform.
> 
> Yeah, that looks pretty good.  The name is long but I can't think of a better 
> one offhand.  Can you also update Documentation/power/pci.txt with the latest 
> best practices?

I'm going to do that, eventually, but rather in a separate patch, when
everything is ready for the new framework, while at the moment we still have
some design work to do.

For example, some drivers may want to call pci_enable_wake() for the
target state and that must be done before pci_set_power_state() in case the
target state is D3cold.  To allow them to do that, we'll need a variant of
pci_choose_and_set_state() with a 'wake_enabled' argument.

> I wonder if we should do a pass through the drivers converting them to this
> interface as well... 

Well, that would be lots of work and since we'd like the drivers to switch to
the new framework entirely, we'll need to pass through them anyway for this
purpose.  I'd prefer that to be one pass. ;-)

Thanks,
Rafael

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Saturday, 10 of May 2008, Rafael J. Wysocki wrote:
> On Saturday, 10 of May 2008, Jesse Barnes wrote:
> > On Friday, May 09, 2008 2:44 pm Rafael J. Wysocki wrote:
> > > Okay, what about this:
> > >
> > > ---
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > >
> > > The new suspend and hibernation callbacks introduced with
> > > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > > pm_message_t argument, so the drivers using them will not be able
> > > to use pci_choose_state() in its present form.  For this reason,
> > > introduce a new function, pci_choose_and_set_state(), playing the
> > > role of pci_choose_state() combined with pci_set_power_state() and
> > > allowing the driver to put the device into a power state chosen by
> > > the platform.
> > 
> > Yeah, that looks pretty good.  The name is long but I can't think of a better 
> > one offhand.  Can you also update Documentation/power/pci.txt with the latest 
> > best practices?
> 
> I'm going to do that, eventually, but rather in a separate patch, when
> everything is ready for the new framework, while at the moment we still have
> some design work to do.
> 
> For example, some drivers may want to call pci_enable_wake() for the
> target state and that must be done before pci_set_power_state() in case the
> target state is D3cold.  To allow them to do that, we'll need a variant of
> pci_choose_and_set_state() with a 'wake_enabled' argument.

Below is how I think that may look like.  Please tell me what you think.

Thanks,
Rafael

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

The new suspend and hibernation callbacks introduced with
'struct pm_ops' and 'struct pm_ext_ops' do not take a
pm_message_t argument, so the drivers using them will not be able
to use pci_choose_state() in its present form.  For this reason,
introduce a new function, raw_pci_change_state(), playing the
role of pci_choose_state() combined with pci_enable_wake() and
pci_set_power_state() and allowing the driver to put the device
into a power state chosen by the platform, optionally configuring
the device as a source of wakeup events.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c   |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/pci.h |   11 +++++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -509,7 +509,67 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
- 
+
+/**
+ * raw_pci_change_state - Choose the power state of a PCI device and put the
+ *                        device into that state.
+ * @dev: PCI device to be put into a low power state
+ * @state: State to put the device into by default
+ * @enable_wake: If set, attempt to enable device to generate wakeup events
+ * @fail_no_wake: If set, return error code if the attempt to enable device
+ *                to generate wakeup events fails
+ *
+ * Use the platform driver to choose the preferred PCI power state of given
+ * device and put the device into that state.  If the target power state of
+ * the device cannot be chosen using the platform driver, the driver-provided
+ * @state is used.  If @enable_wake is set, try to enable the device to
+ * generate wakeup events.
+ *
+ * RETURN VALUE:
+ * -EINVAL if trying to enter a lower state than we're already in.
+ * 0 if we're already in the requested state.
+ * -EIO if device does not support PCI PM.
+ * 0 if we can successfully change the power state.
+ *
+ * If both @enable_wake and @fail_no_wake are set, additionally:
+ * -EIO if the device can't ever be a wakeup event source.
+ * -EINVAL if the device can't generate wakeup events from given state.
+ */
+
+int raw_pci_change_state(struct pci_dev *dev, pci_power_t state,
+                         bool enable_wake, bool fail_no_wake)
+{
+	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+		return -EIO;
+
+	if (platform_pci_choose_state) {
+		pci_power_t ret = platform_pci_choose_state(dev);
+
+		switch (ret) {
+		case PCI_POWER_ERROR:
+		case PCI_UNKNOWN:
+			break;
+		case PCI_D1:
+		case PCI_D2:
+			if (pci_no_d1d2(dev))
+				break;
+		default:
+			state = ret;
+		}
+	}
+
+	if (enable_wake) {
+		int error = pci_enable_wake(dev, state, true);
+
+		if (error && fail_no_wake)
+			return error;
+	}
+
+	return pci_set_power_state(dev, state);
+}
+
+EXPORT_SYMBOL(raw_pci_change_state);
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -615,6 +615,17 @@ size_t pci_get_rom_size(void __iomem *ro
 int pci_save_state(struct pci_dev *dev);
 int pci_restore_state(struct pci_dev *dev);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+int raw_pci_change_state(struct pci_dev *dev, pci_power_t state,
+                         bool enable_wake, bool fail_no_wake);
+static inline int pci_change_state(struct pci_dev *dev, pci_power_t state)
+{
+	return raw_pci_change_state(dev, state, false, false);
+}
+static inline int pci_change_state_wake(struct pci_dev *dev,
+                                          pci_power_t state)
+{
+	return raw_pci_change_state(dev, state, true, false);
+}
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable);
 

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The new suspend and hibernation callbacks introduced with
> 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> pm_message_t argument, so the drivers using them will not be able
> to use pci_choose_state() in its present form.  For this reason,
> introduce a new function, pci_choose_and_set_state(), playing the
> role of pci_choose_state() combined with pci_set_power_state() and
> allowing the driver to put the device into a power state chosen by
> the platform.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/pci/pci.c   |   46 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pci.h |    1 +
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -509,7 +509,51 @@ pci_set_power_state(struct pci_dev *dev,
>  }
>  
>  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> - 
> +
> +/**
> + * pci_choose_and_set_state - Choose the power state of a PCI device and put
> + *                            the device into that state.
> + * @dev: PCI device to be put into a low power state
> + * @state: State to put the device into by default
> + *
> + * Use the platform driver to choose the preferred PCI power state of given
> + * device and put the device into that state.  If the target power state of
> + * the device cannot be chosen using the platform driver, the driver-provided
> + * @state is used.
> + *
> + * RETURN VALUE:
> + * -EINVAL if trying to enter a lower state than we're already in.
> + * 0 if we're already in the requested state.
> + * -EIO if device does not support PCI PM.
> + * 0 if we can successfully change the power state.
> + */
> +
> +int pci_choose_and_set_state(struct pci_dev *dev, pci_power_t state)
> +{

> +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> +		return -EIO;

Hmm, not sure if this is good idea. First, simple drivers will just
ignore the return value, and second, it returns -EIO even if I'm
asking it to enter state it already has -- like PCI_D0 -- no?


> +	if (platform_pci_choose_state) {
> +		pci_power_t ret = platform_pci_choose_state(dev);
> +
> +		switch (ret) {
> +		case PCI_POWER_ERROR:
> +		case PCI_UNKNOWN:
> +			break;
> +		case PCI_D1:
> +		case PCI_D2:
> +			if (pci_no_d1d2(dev))
> +				break;
> +		default:
> +			state = ret;
> +		}
> +	}
> +
> +	return pci_set_power_state(dev, state);
> +}

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

Re: [linux-pm] [PATCH 2/2] PCI PM: Introduce pci_preferred_state

[Rafael J. Wysocki]

On Monday, 12 of May 2008, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The new suspend and hibernation callbacks introduced with
> > 'struct pm_ops' and 'struct pm_ext_ops' do not take a
> > pm_message_t argument, so the drivers using them will not be able
> > to use pci_choose_state() in its present form.  For this reason,
> > introduce a new function, pci_choose_and_set_state(), playing the
> > role of pci_choose_state() combined with pci_set_power_state() and
> > allowing the driver to put the device into a power state chosen by
> > the platform.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/pci/pci.c   |   46 +++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/pci.h |    1 +
> >  2 files changed, 46 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/drivers/pci/pci.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/pci.c
> > +++ linux-2.6/drivers/pci/pci.c
> > @@ -509,7 +509,51 @@ pci_set_power_state(struct pci_dev *dev,
> >  }
> >  
> >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev);
> > - 
> > +
> > +/**
> > + * pci_choose_and_set_state - Choose the power state of a PCI device and put
> > + *                            the device into that state.
> > + * @dev: PCI device to be put into a low power state
> > + * @state: State to put the device into by default
> > + *
> > + * Use the platform driver to choose the preferred PCI power state of given
> > + * device and put the device into that state.  If the target power state of
> > + * the device cannot be chosen using the platform driver, the driver-provided
> > + * @state is used.
> > + *
> > + * RETURN VALUE:
> > + * -EINVAL if trying to enter a lower state than we're already in.
> > + * 0 if we're already in the requested state.
> > + * -EIO if device does not support PCI PM.
> > + * 0 if we can successfully change the power state.
> > + */
> > +
> > +int pci_choose_and_set_state(struct pci_dev *dev, pci_power_t state)
> > +{
> 
> > +	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > +		return -EIO;
> 
> Hmm, not sure if this is good idea. First, simple drivers will just
> ignore the return value,

That doesn't matter to us.

> and second, it returns -EIO even if I'm 
> asking it to enter state it already has -- like PCI_D0 -- no?

The -EIO means the operation could not be carried out, whatever the driver
author meant.  In particular, we wouldn't check what the current state of the
device was, so why would we return anything other than error code?

Thanks,
Rafael