Runtime GPEs not saved over suspend/resume if type != "platform"

Runtime GPEs not saved over suspend/resume if type != "platform"

[Alexey Starikovskiy]

Hi,

There is a problem with swsuspend and GPE events.
Now they are restored from acpi_pm_finish(), which is called only if we
suspend with "platform" option, what means that if we suspended to disk
with the default "shutdown" option after resume all the GPEs will be disabled.
This presents us with the overheating problem after resume at very least.
I see two solutions: one is to make "platform" to be default option if ACPI is enabled,
and second -- enable GPE events from some other function, which will be called in any case.

What do you think?

Regards,
	Alex.

Re: Runtime GPEs not saved over suspend/resume if type != "platform"

[Rafael J. Wysocki]

Hi,

On Wednesday, 25 October 2006 16:42, Alexey Starikovskiy wrote:
> Hi,
> 
> There is a problem with swsuspend and GPE events.
> Now they are restored from acpi_pm_finish(), which is called only if we
> suspend with "platform" option, what means that if we suspended to disk
> with the default "shutdown" option after resume all the GPEs will be disabled.
> This presents us with the overheating problem after resume at very least.
> I see two solutions: one is to make "platform" to be default option if ACPI is enabled,
> and second -- enable GPE events from some other function, which will be called in any case.
> 
> What do you think?

Obviously we can "fix" that in the short run by causing "platform" to be the
default.  Still, I think swsusp should work regardless of whether "platform"
or "shutdown" is used.

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

Re: Runtime GPEs not saved over suspend/resume if type != "platform"

[Len Brown]

On Wednesday 25 October 2006 10:42, Alexey Starikovskiy wrote:
> Hi,
> 
> There is a problem with swsuspend and GPE events.
> Now they are restored from acpi_pm_finish(), which is called only if we
> suspend with "platform" option, what means that if we suspended to disk
> with the default "shutdown" option after resume all the GPEs will be disabled.
> This presents us with the overheating problem after resume at very least.

> I see two solutions: one is to make "platform" to be default option if ACPI is enabled,
> and second -- enable GPE events from some other function, which will be called in any case.

We need to do both.

"platform" needs to be the default on ACPI-enabled platforms because
when "shutdown" is used, wakeup devices that function from S4
but fail to function from S5 are unavailable.

Further, if somebody chooses to use "shutdown", certainly GPEs
need to work when they resume.

-Len

Re: Runtime GPEs not saved over suspend/resume if type != "platform"

[Pavel Machek]

Hi!

> > There is a problem with swsuspend and GPE events.
> > Now they are restored from acpi_pm_finish(), which is called only if we
> > suspend with "platform" option, what means that if we suspended to disk
> > with the default "shutdown" option after resume all the GPEs will be disabled.
> > This presents us with the overheating problem after resume at very least.
> 
> > I see two solutions: one is to make "platform" to be default option if ACPI is enabled,
> > and second -- enable GPE events from some other function, which will be called in any case.
> 
> We need to do both.
> 
> "platform" needs to be the default on ACPI-enabled platforms because
> when "shutdown" is used, wakeup devices that function from S4
> but fail to function from S5 are unavailable.
> 
> Further, if somebody chooses to use "shutdown", certainly GPEs
> need to work when they resume.

Agreed. And rafael's patch is good step forward (in making "platform"
default). Time to push it to akpm?
								Pavel

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

RE: Runtime GPEs not saved over suspend/resume if type != "platform"

[Yu, Luming]

>
>There is a problem with swsuspend and GPE events.
>Now they are restored from acpi_pm_finish(), which is called only if we
>suspend with "platform" option, what means that if we suspended to disk
>with the default "shutdown" option after resume all the GPEs 
>will be disabled.
>This presents us with the overheating problem after resume at 
>very least.
>I see two solutions: one is to make "platform" to be default 
>option if ACPI is enabled,
>and second -- enable GPE events from some other function, 
>which will be called in any case.
>
>What do you think?
>

I prefer platform S4 as default...

--Luming

Re: Runtime GPEs not saved over suspend/resume if type != "platform"

[Alexey Starikovskiy]

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

Here is a least intrusive patch by my opinion...
Just copy pm_disk_mode from pm_ops supplied by platform.
ACPI sets it to PM_DISK_PLATFORM if it has S4,
and it should be zero otherwise, patch does not copy it then.

Regards,
	Alex.

Yu, Luming wrote:
>> There is a problem with swsuspend and GPE events.
>> Now they are restored from acpi_pm_finish(), which is called only if we
>> suspend with "platform" option, what means that if we suspended to disk
>> with the default "shutdown" option after resume all the GPEs 
>> will be disabled.
>> This presents us with the overheating problem after resume at 
>> very least.
>> I see two solutions: one is to make "platform" to be default 
>> option if ACPI is enabled,
>> and second -- enable GPE events from some other function, 
>> which will be called in any case.
>>
>> What do you think?
>>
> 
> I prefer platform S4 as default...
> 
> --Luming
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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

Copy non-null pm_disk_mode option from pm_ops.

From:  Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>


---

 kernel/power/main.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/power/main.c b/kernel/power/main.c
index 873228c..2085611 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -38,6 +38,8 @@ void pm_set_ops(struct pm_ops * ops)
 {
 	down(&pm_sem);
 	pm_ops = ops;
+	if (pm_ops && pm_ops->pm_disk_mode)
+		pm_disk_mode = pm_ops->pm_disk_mode;
 	up(&pm_sem);
 }
 

Re: Runtime GPEs not saved over suspend/resume if type != "platform"

[Rafael J. Wysocki]

On Wednesday, 25 October 2006 18:35, Alexey Starikovskiy wrote:
> Here is a least intrusive patch by my opinion...
> Just copy pm_disk_mode from pm_ops supplied by platform.
> ACPI sets it to PM_DISK_PLATFORM if it has S4,
> and it should be zero otherwise, patch does not copy it then.

Well, I think the appended patch is sufficient.

If "platform" is unavailable it has no effect and works like "shutdown".

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/disk.c |    8 +++++---
 kernel/power/main.c |    2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

Index: linux-2.6.19-rc2-mm2/kernel/power/main.c
===================================================================
--- linux-2.6.19-rc2-mm2.orig/kernel/power/main.c
+++ linux-2.6.19-rc2-mm2/kernel/power/main.c
@@ -28,7 +28,7 @@
 DECLARE_MUTEX(pm_sem);
 
 struct pm_ops *pm_ops;
-suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
+suspend_disk_method_t pm_disk_mode = PM_DISK_PLATFORM;
 
 /**
  *	pm_set_ops - Set the global power method table. 
Index: linux-2.6.19-rc2-mm2/kernel/power/disk.c
===================================================================
--- linux-2.6.19-rc2-mm2.orig/kernel/power/disk.c
+++ linux-2.6.19-rc2-mm2/kernel/power/disk.c
@@ -61,9 +61,11 @@ static void power_down(suspend_disk_meth
 
 	switch(mode) {
 	case PM_DISK_PLATFORM:
-		kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-		error = pm_ops->enter(PM_SUSPEND_DISK);
-		break;
+		if (pm_ops && pm_ops->enter) {
+			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
+			error = pm_ops->enter(PM_SUSPEND_DISK);
+			break;
+		}
 	case PM_DISK_SHUTDOWN:
 		kernel_power_off();
 		break;

Re: Runtime GPEs not saved over suspend/resume if type != "platform"

[Alexey Starikovskiy]

Both yours and mine patches suffer from the same problem -- it's not possible to write
to "/sys/power/disk" any longer, as all values other than PM_DISK_PLATFORM would be rejected :)
Still want to hear a word from subsystem maintainers on this issue... :(

Regards,
	Alex.

Rafael J. Wysocki wrote:
> On Wednesday, 25 October 2006 18:35, Alexey Starikovskiy wrote:
>> Here is a least intrusive patch by my opinion...
>> Just copy pm_disk_mode from pm_ops supplied by platform.
>> ACPI sets it to PM_DISK_PLATFORM if it has S4,
>> and it should be zero otherwise, patch does not copy it then.
> 
> Well, I think the appended patch is sufficient.
> 
> If "platform" is unavailable it has no effect and works like "shutdown".
> 
> Greetings,
> Rafael
> 
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/power/disk.c |    8 +++++---
>  kernel/power/main.c |    2 +-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> Index: linux-2.6.19-rc2-mm2/kernel/power/main.c
> ===================================================================
> --- linux-2.6.19-rc2-mm2.orig/kernel/power/main.c
> +++ linux-2.6.19-rc2-mm2/kernel/power/main.c
> @@ -28,7 +28,7 @@
>  DECLARE_MUTEX(pm_sem);
>  
>  struct pm_ops *pm_ops;
> -suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
> +suspend_disk_method_t pm_disk_mode = PM_DISK_PLATFORM;
>  
>  /**
>   *	pm_set_ops - Set the global power method table. 
> Index: linux-2.6.19-rc2-mm2/kernel/power/disk.c
> ===================================================================
> --- linux-2.6.19-rc2-mm2.orig/kernel/power/disk.c
> +++ linux-2.6.19-rc2-mm2/kernel/power/disk.c
> @@ -61,9 +61,11 @@ static void power_down(suspend_disk_meth
>  
>  	switch(mode) {
>  	case PM_DISK_PLATFORM:
> -		kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> -		error = pm_ops->enter(PM_SUSPEND_DISK);
> -		break;
> +		if (pm_ops && pm_ops->enter) {
> +			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
> +			error = pm_ops->enter(PM_SUSPEND_DISK);
> +			break;
> +		}
>  	case PM_DISK_SHUTDOWN:
>  		kernel_power_off();
>  		break;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Runtime GPEs not saved over suspend/resume if type != "platform"

[Rafael J. Wysocki]

On Thursday, 26 October 2006 15:21, Alexey Starikovskiy wrote:
> Both yours and mine patches suffer from the same problem -- it's not possible to write
> to "/sys/power/disk" any longer, as all values other than PM_DISK_PLATFORM would be rejected :)

Well, I'm running with my patch applied now:

albercik:~ # cat /sys/power/disk
platform
albercik:~ # echo shutdown > /sys/power/disk
albercik:~ # cat /sys/power/disk
shutdown
albercik:~ #

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

Re: Runtime GPEs not saved over suspend/resume if type != "platform"

[Alexey Starikovskiy]

Rafael J. Wysocki wrote:
> On Thursday, 26 October 2006 15:21, Alexey Starikovskiy wrote:
>> Both yours and mine patches suffer from the same problem -- it's not possible to write
>> to "/sys/power/disk" any longer, as all values other than PM_DISK_PLATFORM would be rejected :)
> 
> Well, I'm running with my patch applied now:
Great, then we are ready to push it out :)
> 
> albercik:~ # cat /sys/power/disk
> platform
> albercik:~ # echo shutdown > /sys/power/disk
> albercik:~ # cat /sys/power/disk
> shutdown
> albercik:~ #
> 
> Greetings,
> Rafael
> 
> 

RE: Runtime GPEs not saved over suspend/resume if type != "platform"

[Yu, Luming]

>Obviously we can "fix" that in the short run by causing 
>"platform" to be the
>default.  Still, I think swsusp should work regardless of 
>whether "platform"
>or "shutdown" is used.

If a platform support ACPI, I guess, we'd better enable
platform S4 by default. Otherwise, sooner or later,
it will ran into some issues...

Thanks,
Luming