[PATCH][2.4] acpi S5 poweroff fix

[PATCH][2.4] acpi S5 poweroff fix

[willy tarreau]

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

Hello Andy,

since I have this new laptop (sony vaio pcg-fx705), ACPI has never allowed
me to shutdown correctly. It always hangs and I have to maintain the power
button pressed to power off, whatever the version of your acpi patch I was
using. Today, I had a bit of free time, so I investigated into the code and
finally fixed the problem :-)

Basically, two unrelated things :
1) I noticed that "echo 5 >/proc/acpi/sleep" would oops, so I manually copied
   the oops and discovered that acpi_restore_state_mem() crashed, trying to
   affet 'pmd' a NULL value. Then I realized that it was because on this path,
   acpi_save_state_mem() was never called in S5. I first thought this was
   intentional because S5 is not expected to return, but reading through the
   code, I found a conflict such as "if (state < 5) { ... if (state != 5) ..."
   so I was sure that the first one should have included 5, and I changed the
   '<' to '<=' so that acpi_save_state_mem() is now called.
   It still didn't work, and I noticed that in acpi_suspend(), the call to
   acpi_system_suspend() is expected to do the right job and certainly not to
   return. But acpi_system_suspend() does nothing if called with state=5 !
   So I added the check for ACPI_STATE_S5 at the same level as _S1 because it
   semt right to me.
   => with these 2 changes, echoing 5 in /proc/acpi/sleep now turns my notebook
   OFF !!!
   => I diffed original (20030424) and my changes and put them in the first
      attached patch.

2) The sad thing is that even after that, neither Alt-SysRq-O nor halt would
   power off. I saw that acpi_poweroff() was a simpler than the code path
   executed from acpi_suspend(5). I tried to duplicate part of the code, but
   no lock. I think I missed some bits. So I did it far simpler : now
   acpi_poweroff() only calls acpi_suspend(ACPI_STATE_S5) and everything works
   fine !  => hence the second patch, still against 20030424.

Although I'm not sure that the second case affects everybody, because it might
be caused by side-effects, but from what I've understood from the code, I'm
pretty sure that nobody can power off by echoing 5 to /proc/acpi/sleep !

Here are the two patches, please review them and apply them if you agree !

Cheers,
Willy (happy with a notebook that powers down for the first time in months !)


___________________________________________________________
Do You Yahoo!? -- Une adresse @yahoo.fr gratuite et en français !
Yahoo! Mail : http://fr.mail.yahoo.com

[-- Attachment #2: acpi-20030424-S5-fix.diff --]
[-- Type: application/octet-stream, Size: 531 bytes --]

--- ./drivers/acpi/system.c-orig	Tue Apr 29 17:39:34 2003
+++ ./drivers/acpi/system.c	Tue Apr 29 19:08:09 2003
@@ -180,7 +180,7 @@
 			return AE_ERROR;
 	}
 
-	if (state < ACPI_STATE_S5) {
+	if (state <= ACPI_STATE_S5) {
 		/* Tell devices to stop I/O and actually save their state.
 		 * It is theoretically possible that something could fail,
 		 * so handle that gracefully..
@@ -277,6 +277,7 @@
 
 	switch (state) {
 	case ACPI_STATE_S1:
+	case ACPI_STATE_S5:
 		barrier();
 		status = acpi_enter_sleep_state(state);
 		break;

[-- Attachment #3: acpi-20030424-pwoff-use-suspend.diff --]
[-- Type: application/octet-stream, Size: 339 bytes --]

--- ./drivers/acpi/system.c-working	Tue Apr 29 19:09:19 2003
+++ ./drivers/acpi/system.c	Tue Apr 29 19:36:08 2003
@@ -90,9 +90,7 @@
 static void
 acpi_power_off (void)
 {
-	acpi_enter_sleep_state_prep(ACPI_STATE_S5);
-	ACPI_DISABLE_IRQS();
-	acpi_enter_sleep_state(ACPI_STATE_S5);
+	acpi_suspend(ACPI_STATE_S5);
 }
 
 #endif /*CONFIG_PM*/

Re: [PATCH][2.4] acpi S5 poweroff fix

[Derek Broughton]

From: "willy tarreau" <wtarreau-Qt13gs6zZMY@public.gmane.org>

> Although I'm not sure that the second case affects everybody, because it might
> be caused by side-effects, but from what I've understood from the code, I'm
> pretty sure that nobody can power off by echoing 5 to /proc/acpi/sleep !

Mostly we wouldn't want to.  iirc, there was some discussion here about not even
permitting "echo 5 >/proc/acpi/sleep".  If I am forced to recover from the (too
frequent) situation where my laptop freezes and ignores the keyboard, I have to
hold down the power button for 4 or 5 seconds, as you mention.  But that, like
S5, is an abrupt power-off.  Generally, I'd want to go through init and do it
gracefully.  Pushing the power button once is enough to generate a power button
event in acpi, and then acpid shuts the machine down using '/sbin/init 0'



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH][2.4] acpi S5 poweroff fix

[Ducrot Bruno]

On Wed, Apr 30, 2003 at 09:09:19AM -0300, Derek Broughton wrote:
> From: "willy tarreau" <wtarreau-Qt13gs6zZMY@public.gmane.org>
> 
> > Although I'm not sure that the second case affects everybody, because it might
> > be caused by side-effects, but from what I've understood from the code, I'm
> > pretty sure that nobody can power off by echoing 5 to /proc/acpi/sleep !
> 
> Mostly we wouldn't want to.  iirc, there was some discussion here about not even
> permitting "echo 5 >/proc/acpi/sleep".

This is done afaik in 2.5 and I supposed it was done in 2.4 as well.

> If I am forced to recover from the (too
> frequent) situation where my laptop freezes and ignores the keyboard, I have to
> hold down the power button for 4 or 5 seconds, as you mention.  But that, like
> S5, is an abrupt power-off.  Generally, I'd want to go through init and do it
> gracefully.  Pushing the power button once is enough to generate a power button
> event in acpi, and then acpid shuts the machine down using '/sbin/init 0'
> 

I must admit that I don't have
a real point-of-view about allowing 'echo 5 > /proc/acpi/sleep'
or not.

One 'pro', for example, is to permit ospmd (or acpid)
to enter this state if a critical thermal event occur.

For me, that is more a debate of what have to be done in
userspace vs in-kernel in ACPI in general.

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH][2.4] acpi S5 poweroff fix

[Markus Gaugusch]

I think that the prevention of S5 is nonsense.
echo 5 > /dev/hda will screw up your system much more than echo 5 > sleep.
Root must know what he's doing. I think that the patch should be applied.

Markus
-- 
__________________    /"\
Markus Gaugusch       \ /    ASCII Ribbon Campaign
markus-z+rTbpWsRgbk7+2FdBfRIA@public.gmane.org     X     Against HTML Mail
                      / \


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH][2.4] acpi S5 poweroff fix

[Derek Broughton]

From: "Markus Gaugusch" <markus-z+rTbpWsRgbk7+2FdBfRIA@public.gmane.org>

> I think that the prevention of S5 is nonsense.
> echo 5 > /dev/hda will screw up your system much more than echo 5 > sleep.
> Root must know what he's doing. I think that the patch should be applied.

Neither Bruno nor I suggested that there's anything wrong with the patch - I
haven't looked at it, and if I had I doubt I would know any more than I learned
from having read Willy's description.  It made sense.  However, the reason Willy
gave for actually wanting to _use_ S5 seemed flawed.

derek
(ps.  I didn't even need to know that I _could_ echo to /dev/hda :-) Ack!!)



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH][2.4] acpi S5 poweroff fix

[Karol Kozimor]

Thus wrote Markus Gaugusch:
> I think that the prevention of S5 is nonsense.
> echo 5 > /dev/hda will screw up your system much more than echo 5 > sleep.
> Root must know what he's doing. I think that the patch should be applied.

Some time ago (at least with acpi-20021212) this feature worked perfectly.
It was then judged as dangerous and explicitly disabled in further releases.
What you are trying to do, is to reverse this change, so basically, you'll
probably not have much luck succeeding. Check the archives of the list (I 
believe the topic was discussed around January).
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH][2.4] acpi S5 poweroff fix

[Ducrot Bruno]

On Wed, Apr 30, 2003 at 09:38:56PM +0200, Karol Kozimor wrote:
> Thus wrote Markus Gaugusch:
> > I think that the prevention of S5 is nonsense.
> > echo 5 > /dev/hda will screw up your system much more than echo 5 > sleep.
> > Root must know what he's doing. I think that the patch should be applied.
> 
> Some time ago (at least with acpi-20021212) this feature worked perfectly.
> It was then judged as dangerous and explicitly disabled in further releases.
> What you are trying to do, is to reverse this change, so basically, you'll
> probably not have much luck succeeding. Check the archives of the list (I 
> believe the topic was discussed around January).
> Best regards,

This was my fault(c)(TM) with s4bios.  A fix is the first
patch send by the original poster of this thread (or
one I have send to Andy some times ago which does the
same fix anyway).
But I have more concern with the second patch.
It break the CONFIG_ACPI_SLEEP option.

At this time, this option is always 'yes', but this may change
in the future, and we want to be able to enter S5 even if this
option is off (as in 2.5).

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [PATCH][2.4] acpi S5 poweroff fix

[Yu, Luming]

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

To keep the consistency of the code,  below patch could be a better solution.

Thanks,
Luming

-----Original Message-----
From: willy tarreau [mailto:wtarreau-Qt13gs6zZMY@public.gmane.org]
Sent: 2003?4?30? 2:15
To: Grover, Andrew
Cc: Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Subject: [ACPI] [PATCH][2.4] acpi S5 poweroff fix


Hello Andy,

since I have this new laptop (sony vaio pcg-fx705), ACPI has never allowed
me to shutdown correctly. It always hangs and I have to maintain the power
button pressed to power off, whatever the version of your acpi patch I was
using. Today, I had a bit of free time, so I investigated into the code and
finally fixed the problem :-)

Basically, two unrelated things :
1) I noticed that "echo 5 >/proc/acpi/sleep" would oops, so I manually copied
   the oops and discovered that acpi_restore_state_mem() crashed, trying to
   affet 'pmd' a NULL value. Then I realized that it was because on this path,
   acpi_save_state_mem() was never called in S5. I first thought this was
   intentional because S5 is not expected to return, but reading through the
   code, I found a conflict such as "if (state < 5) { ... if (state != 5) ..."
   so I was sure that the first one should have included 5, and I changed the
   '<' to '<=' so that acpi_save_state_mem() is now called.
   It still didn't work, and I noticed that in acpi_suspend(), the call to
   acpi_system_suspend() is expected to do the right job and certainly not to
   return. But acpi_system_suspend() does nothing if called with state=5 !
   So I added the check for ACPI_STATE_S5 at the same level as _S1 because it
   semt right to me.
   => with these 2 changes, echoing 5 in /proc/acpi/sleep now turns my notebook
   OFF !!!
   => I diffed original (20030424) and my changes and put them in the first
      attached patch.

2) The sad thing is that even after that, neither Alt-SysRq-O nor halt would
   power off. I saw that acpi_poweroff() was a simpler than the code path
   executed from acpi_suspend(5). I tried to duplicate part of the code, but
   no lock. I think I missed some bits. So I did it far simpler : now
   acpi_poweroff() only calls acpi_suspend(ACPI_STATE_S5) and everything works
   fine !  => hence the second patch, still against 20030424.

Although I'm not sure that the second case affects everybody, because it might
be caused by side-effects, but from what I've understood from the code, I'm
pretty sure that nobody can power off by echoing 5 to /proc/acpi/sleep !

Here are the two patches, please review them and apply them if you agree !

Cheers,
Willy (happy with a notebook that powers down for the first time in months !)


___________________________________________________________
Do You Yahoo!? -- Une adresse @yahoo.fr gratuite et en français !
Yahoo! Mail : http://fr.mail.yahoo.com

[-- Attachment #2: s5oops.diff --]
[-- Type: application/octet-stream, Size: 449 bytes --]

diff -Bru 1/drivers/acpi/system.c 2/drivers/acpi/system.c
--- 1/drivers/acpi/system.c	2003-03-28 14:44:35.000000000 +0800
+++ 2/drivers/acpi/system.c	2003-03-31 09:47:38.000000000 +0800
@@ -113,6 +128,10 @@
 	if (state != ACPI_STATE_S1 && state != ACPI_STATE_S5)
 		return AE_ERROR;
 
+	if(state == ACPI_STATE_S5){
+		acpi_power_off();
+		return;
+	}
 	acpi_enter_sleep_state_prep(state);
 
 	/* disable interrupts and flush caches */

Re: [PATCH][2.4] acpi S5 poweroff fix

[Ducrot Bruno]

On Mon, May 12, 2003 at 04:35:13PM +0800, Yu, Luming wrote:
> To keep the consistency of the code,  below patch could be a better solution.
> 
> Thanks,
> Luming
> 

Please do not use '\r\n' in a patch, or you will probably be spanked
by Linus or Marcello.

Thanks,

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
Enterprise Linux Forum Conference & Expo, June 4-6, 2003, Santa Clara
The only event dedicated to issues related to Linux enterprise solutions
www.enterpriselinuxforum.com