RE: PATCH: Call acpi_leave_sleep_state before resuming devices

RE: PATCH: Call acpi_leave_sleep_state before resuming devices

[Li, Shaohua]

>
>Here's again the resume-finish-split patch, this time CC-ed to Len.
>
>This patch splits up acpi_pm_leave. It introduces acpi_pm_leave which
calls
>acpi_leave_sleep_state and acpi_disable_wakeup_device before resuming
the
>devices. This improves S3 resume on an Acer Travelmate 800 and on the
>Samsung
>P3 notebook.
>
>Some archive references:
>http://sourceforge.net/mailarchive/message.php?msg_id=9091506
>http://sourceforge.net/mailarchive/message.php?msg_id=10315947
>
>The background is that any write access to the pci config registers of
some
>devices(sound card, usb 2.0 controler, and modem) cause a system lockup
if
>they are called before acpi_leave_sleep_state and
>acpi_disable_wakeup_device
>were called. This is on an Acer Travelmate 803 Notebook, and it seems
to be
>the same on other systems.
>
>I sent the patch to the list, but it seems that nobody noticed it and I
>forgot
>to send it again. Here is an updated patch against 2.6.10. I can't
promise
>that it doesn't break other systems. Someone should make sure that
>pm_ops->leave is set to NULL for apm. I haven't found this in the apm
code
>yet.
ACPI spec also said _PTS (in 'acpi_enter_sleep_state_prep') should be
called after all devices are suspend (ACPI sepc 3.0 P401), could you
please also change the order of device suspend and
'acpi_enter_sleep_state_prep'?

Thanks,
Shaohua


-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt

Re: RE: PATCH: Call acpi_leave_sleep_state before resuming devices

[Stefan Dösinger]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

Hello,

> ACPI spec also said _PTS (in 'acpi_enter_sleep_state_prep') should be
> called after all devices are suspend (ACPI sepc 3.0 P401), could you
> please also change the order of device suspend and
> 'acpi_enter_sleep_state_prep'?

Sorry for the delay, but I was quite busy this week. Here is the updated patch 
which also causes pm_ops->prepare to be called after device suspend.
Is this correct or should I create another new function? What does the spec 
say?

Stefan

[-- Attachment #2: patch.diff --]
[-- Type: text/x-diff, Size: 2088 bytes --]

--- linux/drivers/acpi/sleep/main.c	2005-01-15 10:28:32.224204848 +0100
+++ drivers/acpi/sleep/main.c	2004-12-27 13:11:03.000000000 +0100
@@ -130,7 +130,22 @@
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
+/**
+ *	acpi_pm_leave - Make system ready to resume devices.
+ *	@pm_state:		State we're coming out of.
+ *
+ *	This is called after we wake back up and before device
+ *	resume methods are called.
+ */
+
+static int acpi_pm_leave(suspend_state_t pm_state)
+{
+	u32 acpi_state = acpi_suspend_states[pm_state];
 
+	acpi_leave_sleep_state(acpi_state);
+	
+	return 0;
+}
 /**
  *	acpi_pm_finish - Finish up suspend sequence.
  *	@pm_state:		State we're coming out of.
@@ -142,8 +157,7 @@
 static int acpi_pm_finish(suspend_state_t pm_state)
 {
 	u32 acpi_state = acpi_suspend_states[pm_state];
-
-	acpi_leave_sleep_state(acpi_state);
+	
 	acpi_disable_wakeup_device(acpi_state);
 
 	/* reset firmware waking vector */
@@ -173,6 +187,7 @@
 static struct pm_ops acpi_pm_ops = {
 	.prepare	= acpi_pm_prepare,
 	.enter		= acpi_pm_enter,
+	.leave		= acpi_pm_leave,
 	.finish		= acpi_pm_finish,
 };
 
--- linux/kernel/power/main.c	2005-01-15 10:28:40.151999640 +0100
+++ kernel/power/main.c	2005-01-15 10:25:55.118088608 +0100
@@ -60,13 +60,14 @@
 		goto Thaw;
 	}
 
+	if ((error = device_suspend(state)))
+		goto Finish;
+	
 	if (pm_ops->prepare) {
 		if ((error = pm_ops->prepare(state)))
-			goto Thaw;
+			goto Finish;
 	}
-
-	if ((error = device_suspend(state)))
-		goto Finish;
+	
 	return 0;
  Finish:
 	if (pm_ops->finish)
@@ -104,6 +105,8 @@
 
 static void suspend_finish(suspend_state_t state)
 {
+	if (pm_ops && pm_ops->leave)
+		pm_ops->leave(state);
 	device_resume();
 	if (pm_ops && pm_ops->finish)
 		pm_ops->finish(state);
--- linux/include/linux/pm.h	2005-01-15 10:28:39.937032320 +0100
+++ include/linux/pm.h	2004-12-26 15:27:10.000000000 +0100
@@ -208,6 +208,7 @@
 	suspend_disk_method_t pm_disk_mode;
 	int (*prepare)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
+	int (*leave)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
 };
 

Re: RE: PATCH: Call acpi_leave_sleep_state before resuming devices

[Pavel Machek]

Hi!

> > ACPI spec also said _PTS (in 'acpi_enter_sleep_state_prep') should be
> > called after all devices are suspend (ACPI sepc 3.0 P401), could you
> > please also change the order of device suspend and
> > 'acpi_enter_sleep_state_prep'?
> 
> Sorry for the delay, but I was quite busy this week. Here is the updated patch 
> which also causes pm_ops->prepare to be called after device suspend.
> Is this correct or should I create another new function? What does the spec 
> say?

Creating another function seems right. Even if ACPI does not need it,
some other suspend mechanism will need it.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt

Re: RE: PATCH: Call acpi_leave_sleep_state before resuming devices

[Stefan Dösinger]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

Hello,

> > > ACPI spec also said _PTS (in 'acpi_enter_sleep_state_prep') should be
> > > called after all devices are suspend (ACPI sepc 3.0 P401), could you
> > > please also change the order of device suspend and
> > > 'acpi_enter_sleep_state_prep'?
> >
> > Sorry for the delay, but I was quite busy this week. Here is the updated
> > patch which also causes pm_ops->prepare to be called after device
> > suspend. Is this correct or should I create another new function? What
> > does the spec say?
>
> Creating another function seems right. Even if ACPI does not need it,
> some other suspend mechanism will need it.

Allright, here it is. This patch introduces two new suspend/resume functions, 
pm_ops->setup and pm_ops->leave. There are now 5 functions:

prepare: It's called before devices are suspended
setup: Called after device suspend but before finally going to sleep
enter: Last suspend function
leave: Called before devices are beeing resumed
finish: Last function, called after device resume

The code fom acpi_pm_prepare is moved to acpi_pm_setup. acpi_pm_leave is used 
to call acpi_leave_sleep_state and to disable the wakeup devices before 
resuming the devices. This is needed on some Notebooks, for example my Acer 
Travelmate 800 and Samsung P3.

Stefan Dösinger

[-- Attachment #2: suspend.diff --]
[-- Type: text/x-diff, Size: 2795 bytes --]

--- linux-2.6.9/include/linux/pm.h	2005-01-19 16:20:35.600504440 +0100
+++ include/linux/pm.h	2005-01-19 14:47:54.000000000 +0100
@@ -207,7 +207,9 @@
 struct pm_ops {
 	suspend_disk_method_t pm_disk_mode;
 	int (*prepare)(suspend_state_t state);
+	int (*setup)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
+	int (*leave)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
 };
 
--- linux-2.6.9/kernel/power/main.c	2005-01-19 16:20:35.831469328 +0100
+++ kernel/power/main.c	2005-01-19 16:30:50.407039640 +0100
@@ -64,9 +64,16 @@
 		if ((error = pm_ops->prepare(state)))
 			goto Thaw;
 	}
-
+	
 	if ((error = device_suspend(state)))
 		goto Finish;
+	
+	if (pm_ops->setup) {
+		if ((error = pm_ops->setup(state)))
+			goto Finish;
+	}
+	
+	
 	return 0;
  Finish:
 	if (pm_ops->finish)
@@ -104,6 +111,8 @@
 
 static void suspend_finish(suspend_state_t state)
 {
+	if (pm_ops && pm_ops->leave)
+		pm_ops->leave(state);
 	device_resume();
 	if (pm_ops && pm_ops->finish)
 		pm_ops->finish(state);
--- linux-2.6.9/drivers/acpi/sleep/main.c	2005-01-19 16:20:28.773542296 +0100
+++ drivers/acpi/sleep/main.c	2005-01-19 16:30:00.369646480 +0100
@@ -36,7 +36,7 @@
 static int init_8259A_after_S1;
 
 /**
- *	acpi_pm_prepare - Do preliminary suspend work.
+ *	acpi_pm_setup - Do preliminary suspend work.
  *	@pm_state:		suspend state we're entering.
  *
  *	Make sure we support the state. If we do, and we need it, set the
@@ -44,7 +44,7 @@
  *	wakeup code to the waking vector. 
  */
 
-static int acpi_pm_prepare(suspend_state_t pm_state)
+static int acpi_pm_setup(suspend_state_t pm_state)
 {
 	u32 acpi_state = acpi_suspend_states[pm_state];
 
@@ -130,7 +130,23 @@
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
+/**
+ *	acpi_pm_leave - Make system ready to resume devices.
+ *	@pm_state:		State we're coming out of.
+ *
+ *	This is called after we wake back up and before device
+ *	resume methods are called.
+ */
 
+static int acpi_pm_leave(suspend_state_t pm_state)
+{
+	u32 acpi_state = acpi_suspend_states[pm_state];
+
+	acpi_leave_sleep_state(acpi_state);
+	acpi_disable_wakeup_device(acpi_state);
+	
+	return 0;
+}
 /**
  *	acpi_pm_finish - Finish up suspend sequence.
  *	@pm_state:		State we're coming out of.
@@ -142,10 +158,7 @@
 static int acpi_pm_finish(suspend_state_t pm_state)
 {
 	u32 acpi_state = acpi_suspend_states[pm_state];
-
-	acpi_leave_sleep_state(acpi_state);
-	acpi_disable_wakeup_device(acpi_state);
-
+	
 	/* reset firmware waking vector */
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
@@ -171,8 +184,10 @@
 }
 
 static struct pm_ops acpi_pm_ops = {
-	.prepare	= acpi_pm_prepare,
+	.prepare	= NULL,
+	.setup		= acpi_pm_setup,
 	.enter		= acpi_pm_enter,
+	.leave		= acpi_pm_leave,
 	.finish		= acpi_pm_finish,
 };
 

Re: RE: PATCH: Call acpi_leave_sleep_state before resuming devices

[Pavel Machek]

Hi!

> > > > ACPI spec also said _PTS (in 'acpi_enter_sleep_state_prep') should be
> > > > called after all devices are suspend (ACPI sepc 3.0 P401), could you
> > > > please also change the order of device suspend and
> > > > 'acpi_enter_sleep_state_prep'?
> > >
> > > Sorry for the delay, but I was quite busy this week. Here is the updated
> > > patch which also causes pm_ops->prepare to be called after device
> > > suspend. Is this correct or should I create another new function? What
> > > does the spec say?
> >
> > Creating another function seems right. Even if ACPI does not need it,
> > some other suspend mechanism will need it.
> 
> Allright, here it is. This patch introduces two new suspend/resume functions, 
> pm_ops->setup and pm_ops->leave. There are now 5 functions:
> 
> prepare: It's called before devices are suspended
> setup: Called after device suspend but before finally going to sleep
> enter: Last suspend function
> leave: Called before devices are beeing resumed
> finish: Last function, called after device resume

Add this as a comment to pm.h and you have my Acknowledged-by: :-)

									Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl

Re: RE: PATCH: Call acpi_leave_sleep_state before resuming devices

[Stefan Dösinger]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

Am Mittwoch, 19. Januar 2005 22:17 schrieb Pavel Machek:
> Hi!
>
> > > > > ACPI spec also said _PTS (in 'acpi_enter_sleep_state_prep') should
> > > > > be called after all devices are suspend (ACPI sepc 3.0 P401), could
> > > > > you please also change the order of device suspend and
> > > > > 'acpi_enter_sleep_state_prep'?
> > > >
> > > > Sorry for the delay, but I was quite busy this week. Here is the
> > > > updated patch which also causes pm_ops->prepare to be called after
> > > > device suspend. Is this correct or should I create another new
> > > > function? What does the spec say?
> > >
> > > Creating another function seems right. Even if ACPI does not need it,
> > > some other suspend mechanism will need it.
> >
> > Allright, here it is. This patch introduces two new suspend/resume
> > functions, pm_ops->setup and pm_ops->leave. There are now 5 functions:
> >
> > prepare: It's called before devices are suspended
> > setup: Called after device suspend but before finally going to sleep
> > enter: Last suspend function
> > leave: Called before devices are beeing resumed
> > finish: Last function, called after device resume
>
> Add this as a comment to pm.h and you have my Acknowledged-by: :-)
Allright, here it is.

Stefan Dösinger

[-- Attachment #2: suspend.diff --]
[-- Type: text/x-diff, Size: 2994 bytes --]

--- linux-2.6.9/include/linux/pm.h	2005-01-20 14:04:54.248337144 +0100
+++ include/linux/pm.h	2005-01-20 14:03:12.000000000 +0100
@@ -206,9 +206,11 @@
 
 struct pm_ops {
 	suspend_disk_method_t pm_disk_mode;
-	int (*prepare)(suspend_state_t state);
-	int (*enter)(suspend_state_t state);
-	int (*finish)(suspend_state_t state);
+	int (*prepare)(suspend_state_t state);		/*Called before suspending devices*/
+	int (*setup)(suspend_state_t state);		/*Called after device suspend*/
+	int (*enter)(suspend_state_t state);		/*To Finally enter the sleep state*/
+	int (*leave)(suspend_state_t state);		/*Before device wakeup*/
+	int (*finish)(suspend_state_t state);		/*After device wakeup*/
 };
 
 extern void pm_set_ops(struct pm_ops *);
--- linux-2.6.9/kernel/power/main.c	2005-01-20 14:04:54.463304464 +0100
+++ kernel/power/main.c	2005-01-20 14:10:36.499307088 +0100
@@ -67,6 +67,12 @@
 
 	if ((error = device_suspend(state)))
 		goto Finish;
+
+	if (pm_ops->setup) {
+		if ((error = pm_ops->setup(state)))
+			goto Finish;
+	}
+
 	return 0;
  Finish:
 	if (pm_ops->finish)
@@ -104,6 +110,8 @@
 
 static void suspend_finish(suspend_state_t state)
 {
+	if (pm_ops && pm_ops->leave)
+		pm_ops->leave(state);
 	device_resume();
 	if (pm_ops && pm_ops->finish)
 		pm_ops->finish(state);
--- linux-2.6.9/drivers/acpi/sleep/main.c	2005-01-20 14:04:47.382380928 +0100
+++ drivers/acpi/sleep/main.c	2005-01-20 14:12:29.234168784 +0100
@@ -36,7 +36,7 @@
 static int init_8259A_after_S1;
 
 /**
- *	acpi_pm_prepare - Do preliminary suspend work.
+ *	acpi_pm_setup - Do preliminary suspend work.
  *	@pm_state:		suspend state we're entering.
  *
  *	Make sure we support the state. If we do, and we need it, set the
@@ -44,7 +44,7 @@
  *	wakeup code to the waking vector. 
  */
 
-static int acpi_pm_prepare(suspend_state_t pm_state)
+static int acpi_pm_setup(suspend_state_t pm_state)
 {
 	u32 acpi_state = acpi_suspend_states[pm_state];
 
@@ -130,7 +130,23 @@
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
+/**
+ *	acpi_pm_leave - Make system ready to resume devices.
+ *	@pm_state:		State we're coming out of.
+ *
+ *	This is called after we wake back up and before device
+ *	resume methods are called.
+ */
 
+static int acpi_pm_leave(suspend_state_t pm_state)
+{
+	u32 acpi_state = acpi_suspend_states[pm_state];
+
+	acpi_leave_sleep_state(acpi_state);
+	acpi_disable_wakeup_device(acpi_state);
+
+	return 0;
+}
 /**
  *	acpi_pm_finish - Finish up suspend sequence.
  *	@pm_state:		State we're coming out of.
@@ -143,9 +159,6 @@
 {
 	u32 acpi_state = acpi_suspend_states[pm_state];
 
-	acpi_leave_sleep_state(acpi_state);
-	acpi_disable_wakeup_device(acpi_state);
-
 	/* reset firmware waking vector */
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
@@ -171,8 +184,10 @@
 }
 
 static struct pm_ops acpi_pm_ops = {
-	.prepare	= acpi_pm_prepare,
+	.prepare	= NULL,
+	.setup		= acpi_pm_setup,
 	.enter		= acpi_pm_enter,
+	.leave		= acpi_pm_leave,
 	.finish		= acpi_pm_finish,
 };
 

Re: RE: PATCH: Call acpi_leave_sleep_state before resuming devices

[Pavel Machek]

Hi!

> > > Allright, here it is. This patch introduces two new suspend/resume
> > > functions, pm_ops->setup and pm_ops->leave. There are now 5 functions:
> > >
> > > prepare: It's called before devices are suspended
> > > setup: Called after device suspend but before finally going to sleep
> > > enter: Last suspend function
> > > leave: Called before devices are beeing resumed
> > > finish: Last function, called after device resume
> >
> > Add this as a comment to pm.h and you have my Acknowledged-by: :-)
> Allright, here it is.

Acked-by: Pavel

You probably want to mail it to len, cc: akpm to get it included.

				Pavel


-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         



-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl