Re: commit 9630bdd9 changes behavior of the poweroff - bug?

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Thursday 08 April 2010, Michal Hocko wrote:
> On Wed 07-04-10 21:49:23, Rafael J. Wysocki wrote:
> > On Wednesday 07 April 2010, Michal Hocko wrote:
> > > Sorry for late reply, but I was AFK during holiday.
> > > 
> > > On Fri 02-04-10 21:31:47, Rafael J. Wysocki wrote:
> > > > On Thursday 01 April 2010, Michal Hocko wrote:
> > > > > On Thu 01-04-10 14:45:35, Matthew Garrett wrote:
> > > > > > On Thu, Apr 01, 2010 at 03:39:23PM +0200, Michal Hocko wrote:
> > > > > > > Hi Rafael, Matthew,
> > > > > > > my laptop changed behavior during poweroff recently (after upgrading
> > > > > > > from .33 to .34-rc1). The system seems to be powered off (status display
> > > > > > > is off) at first glance but when I close the lid then I can hear a noise
> > > > > > > which sounds like HDD parking and when I open lid again it starts
> > > > > > > booting without poweroff button (same like when I suspend to RAM).
> > > > > > 
> > > > > > Hm. At a guess, we're somehow managing to leave the lid GPE enabled at 
> > > > > > poweroff. Can you send the output of the acpidump command?
> > > > > 
> > > > > Sure, see attached.
> > > > 
> > > > Thanks.
> > > > 
> > > > Please also send the contents of /proc/acpi/wakeup.
> > > 
> > > See attached. I have checked both kernels (with and without reverted
> > > 9630bdd) and the output looks pretty much same. 2.6.34-rc3, however,
> > > looks different a bit, so I am attaching it as well.
> > 
> > Well, I fail to see the possible failure scenario, so let's first try a blind
> > shot.
> > 
> > Please check if this patch helps:
> > https://patchwork.kernel.org/patch/86238/
> 
> No change, sorry. I have tried that on top of 0fdf867 which is not the
> most recent one (does it make sense to retest on the most recent one
> f5284e7?)

There were no other changes in that area I know of, but the current git
contains the above patch, so I'd recommend to use it for further testing.

I'll try to prepare a debug patch for this issue.

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Tony Vroon]

[-- Attachment #1.1: Type: text/plain, Size: 510 bytes --]

Rafael,

I have the exact same "lid close = powering back on" regression on a
Fujitsu-Siemens Lifebook S6420. Totally missed this thread before, but
I'm glad that I'm not alone with this one.
Happy to test any patches you may come up with; will an acpidump do for
you or do you need additional data?

Regards,
-- 
Tony Vroon
UNIX systems administrator
London Internet Exchange Ltd, Trinity Court, Trinity Street,
Peterborough, PE1 1DA
Registered in England number 3137929
E-Mail: tony@linx.net

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Friday 09 April 2010, Tony Vroon wrote:
> Rafael,
> 
> I have the exact same "lid close = powering back on" regression on a
> Fujitsu-Siemens Lifebook S6420. Totally missed this thread before, but
> I'm glad that I'm not alone with this one.
> Happy to test any patches you may come up with; will an acpidump do for
> you or do you need additional data?

I need to figure out what actually happens and I'll need you to try a debug
patch, but it's not ready yet.

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Thursday 08 April 2010, Rafael J. Wysocki wrote:
> On Thursday 08 April 2010, Michal Hocko wrote:
> > On Wed 07-04-10 21:49:23, Rafael J. Wysocki wrote:
> > > On Wednesday 07 April 2010, Michal Hocko wrote:
> > > > Sorry for late reply, but I was AFK during holiday.
> > > > 
> > > > On Fri 02-04-10 21:31:47, Rafael J. Wysocki wrote:
> > > > > On Thursday 01 April 2010, Michal Hocko wrote:
> > > > > > On Thu 01-04-10 14:45:35, Matthew Garrett wrote:
> > > > > > > On Thu, Apr 01, 2010 at 03:39:23PM +0200, Michal Hocko wrote:
> > > > > > > > Hi Rafael, Matthew,
> > > > > > > > my laptop changed behavior during poweroff recently (after upgrading
> > > > > > > > from .33 to .34-rc1). The system seems to be powered off (status display
> > > > > > > > is off) at first glance but when I close the lid then I can hear a noise
> > > > > > > > which sounds like HDD parking and when I open lid again it starts
> > > > > > > > booting without poweroff button (same like when I suspend to RAM).
> > > > > > > 
> > > > > > > Hm. At a guess, we're somehow managing to leave the lid GPE enabled at 
> > > > > > > poweroff. Can you send the output of the acpidump command?
> > > > > > 
> > > > > > Sure, see attached.
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > > Please also send the contents of /proc/acpi/wakeup.
> > > > 
> > > > See attached. I have checked both kernels (with and without reverted
> > > > 9630bdd) and the output looks pretty much same. 2.6.34-rc3, however,
> > > > looks different a bit, so I am attaching it as well.
> > > 
> > > Well, I fail to see the possible failure scenario, so let's first try a blind
> > > shot.
> > > 
> > > Please check if this patch helps:
> > > https://patchwork.kernel.org/patch/86238/
> > 
> > No change, sorry. I have tried that on top of 0fdf867 which is not the
> > most recent one (does it make sense to retest on the most recent one
> > f5284e7?)
> 
> There were no other changes in that area I know of, but the current git
> contains the above patch, so I'd recommend to use it for further testing.
> 
> I'll try to prepare a debug patch for this issue.

Please check if the patch below changes anything.

Rafael

---
 drivers/acpi/wakeup.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/wakeup.c
===================================================================
--- linux-2.6.orig/drivers/acpi/wakeup.c
+++ linux-2.6/drivers/acpi/wakeup.c
@@ -71,9 +71,9 @@ void acpi_enable_wakeup_device(u8 sleep_
 		    || sleep_state > (u32) dev->wakeup.sleep_state)
 			continue;
 
-		/* The wake-up power should have been enabled already. */
+		/* The wake-up power should have been enabled already.
 		acpi_set_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
-				ACPI_GPE_ENABLE);
+				ACPI_GPE_ENABLE);*/
 	}
 }
 

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Tony Vroon]

[-- Attachment #1.1: Type: text/plain, Size: 547 bytes --]

On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> Please check if the patch below changes anything.
>  drivers/acpi/wakeup.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

That didn't change the behaviour for me, sorry.
(I made sure to go through a full power down session before trying the
patched kernel)

Regards,
-- 
Tony Vroon
UNIX systems administrator
London Internet Exchange Ltd, Trinity Court, Trinity Street,
Peterborough, PE1 1DA
Registered in England number 3137929
E-Mail: tony@linx.net

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Friday 09 April 2010, Tony Vroon wrote:
> On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > Please check if the patch below changes anything.
> >  drivers/acpi/wakeup.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> That didn't change the behaviour for me, sorry.

Well, I would be sorry if it did, because the patch removed some useful code. :-)

> (I made sure to go through a full power down session before trying the
> patched kernel)

Thanks for testing.  So it looks like we don't disable the GPE during power off.

I'll try to figure out what's going on, please stay tuned.

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> On Friday 09 April 2010, Tony Vroon wrote:
> > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > Please check if the patch below changes anything.
> > >  drivers/acpi/wakeup.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > That didn't change the behaviour for me, sorry.
> 
> Well, I would be sorry if it did, because the patch removed some useful code. :-)
> 
> > (I made sure to go through a full power down session before trying the
> > patched kernel)
> 
> Thanks for testing.  So it looks like we don't disable the GPE during power off.
> 
> I'll try to figure out what's going on, please stay tuned.

Can you please check if the patch below changes the behavior?

Rafael

---
 drivers/acpi/wakeup.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/acpi/wakeup.c
===================================================================
--- linux-2.6.orig/drivers/acpi/wakeup.c
+++ linux-2.6/drivers/acpi/wakeup.c
@@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev =
 			container_of(node, struct acpi_device, wakeup_list);
+		u8 action = ACPI_GPE_ENABLE;
 
 		if (!dev->wakeup.flags.valid)
 			continue;
 
 		if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count)
 		    || sleep_state > (u32) dev->wakeup.sleep_state)
-			continue;
+			action = ACPI_GPE_DISABLE;
 
-		/* The wake-up power should have been enabled already. */
 		acpi_set_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
-				ACPI_GPE_ENABLE);
+				action);
 	}
 }
 

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> On Friday 09 April 2010, Tony Vroon wrote:
> > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > Please check if the patch below changes anything.
> > >  drivers/acpi/wakeup.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > That didn't change the behaviour for me, sorry.
> 
> Well, I would be sorry if it did, because the patch removed some useful code. :-)
> 
> > (I made sure to go through a full power down session before trying the
> > patched kernel)
> 
> Thanks for testing.  So it looks like we don't disable the GPE during power off.
> 
> I'll try to figure out what's going on, please stay tuned.

Can you please check if the patch below changes the behavior?

Rafael

---
 drivers/acpi/wakeup.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/acpi/wakeup.c
===================================================================
--- linux-2.6.orig/drivers/acpi/wakeup.c
+++ linux-2.6/drivers/acpi/wakeup.c
@@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev =
 			container_of(node, struct acpi_device, wakeup_list);
+		u8 action = ACPI_GPE_ENABLE;
 
 		if (!dev->wakeup.flags.valid)
 			continue;
 
 		if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count)
 		    || sleep_state > (u32) dev->wakeup.sleep_state)
-			continue;
+			action = ACPI_GPE_DISABLE;
 
-		/* The wake-up power should have been enabled already. */
 		acpi_set_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
-				ACPI_GPE_ENABLE);
+				action);
 	}
 }
 

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Michal Hocko]

On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > On Friday 09 April 2010, Tony Vroon wrote:
> > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > Please check if the patch below changes anything.
> > > >  drivers/acpi/wakeup.c |    4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > That didn't change the behaviour for me, sorry.
> > 
> > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > 
> > > (I made sure to go through a full power down session before trying the
> > > patched kernel)
> > 
> > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > 
> > I'll try to figure out what's going on, please stay tuned.
> 
> Can you please check if the patch below changes the behavior?

Unfortunately, it didn't help either (I have tried on top of the fresh
rc4).

> 
> Rafael
> 
> ---
>  drivers/acpi/wakeup.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/wakeup.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/wakeup.c
> +++ linux-2.6/drivers/acpi/wakeup.c
> @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
>  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
>  		struct acpi_device *dev =
>  			container_of(node, struct acpi_device, wakeup_list);
> +		u8 action = ACPI_GPE_ENABLE;
>  
>  		if (!dev->wakeup.flags.valid)
>  			continue;
>  
>  		if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count)
>  		    || sleep_state > (u32) dev->wakeup.sleep_state)
> -			continue;
> +			action = ACPI_GPE_DISABLE;
>  
> -		/* The wake-up power should have been enabled already. */
>  		acpi_set_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> -				ACPI_GPE_ENABLE);
> +				action);
>  	}
>  }
>  

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Tuesday 13 April 2010, Michal Hocko wrote:
> On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > Please check if the patch below changes anything.
> > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > That didn't change the behaviour for me, sorry.
> > > 
> > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > 
> > > > (I made sure to go through a full power down session before trying the
> > > > patched kernel)
> > > 
> > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > 
> > > I'll try to figure out what's going on, please stay tuned.
> > 
> > Can you please check if the patch below changes the behavior?
> 
> Unfortunately, it didn't help either (I have tried on top of the fresh
> rc4).

That gets really weird.

> > ---
> >  drivers/acpi/wakeup.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > Index: linux-2.6/drivers/acpi/wakeup.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > +++ linux-2.6/drivers/acpi/wakeup.c
> > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> >  		struct acpi_device *dev =
> >  			container_of(node, struct acpi_device, wakeup_list);
> > +		u8 action = ACPI_GPE_ENABLE;

Can you try to change the above to ACPI_GPE_DISABLE and retest, please?

> >  		if (!dev->wakeup.flags.valid)
> >  			continue;
> >  
> >  		if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count)
> >  		    || sleep_state > (u32) dev->wakeup.sleep_state)
> > -			continue;
> > +			action = ACPI_GPE_DISABLE;
> >  
> > -		/* The wake-up power should have been enabled already. */
> >  		acpi_set_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> > -				ACPI_GPE_ENABLE);
> > +				action);
> >  	}
> >  }

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Michal Hocko]

On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> On Tuesday 13 April 2010, Michal Hocko wrote:
> > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > Please check if the patch below changes anything.
> > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > That didn't change the behaviour for me, sorry.
> > > > 
> > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > 
> > > > > (I made sure to go through a full power down session before trying the
> > > > > patched kernel)
> > > > 
> > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > 
> > > > I'll try to figure out what's going on, please stay tuned.
> > > 
> > > Can you please check if the patch below changes the behavior?
> > 
> > Unfortunately, it didn't help either (I have tried on top of the fresh
> > rc4).
> 
> That gets really weird.
> 
> > > ---
> > >  drivers/acpi/wakeup.c |    6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > >  		struct acpi_device *dev =
> > >  			container_of(node, struct acpi_device, wakeup_list);
> > > +		u8 action = ACPI_GPE_ENABLE;
> 
> Can you try to change the above to ACPI_GPE_DISABLE and retest, please?

Unfortunately didn't help as well...
Just for reference:

diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
index 248b473..f23c08f 100644
--- a/drivers/acpi/wakeup.c
+++ b/drivers/acpi/wakeup.c
@@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev =
 			container_of(node, struct acpi_device, wakeup_list);
-		u8 action = ACPI_GPE_ENABLE;
+		u8 action = ACPI_GPE_DISABLE;
 
 		if (!dev->wakeup.flags.valid)
 			continue;
> 
> > >  		if (!dev->wakeup.flags.valid)
> > >  			continue;
> > >  
> > >  		if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count)
> > >  		    || sleep_state > (u32) dev->wakeup.sleep_state)
> > > -			continue;
> > > +			action = ACPI_GPE_DISABLE;
> > >  
> > > -		/* The wake-up power should have been enabled already. */
> > >  		acpi_set_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> > > -				ACPI_GPE_ENABLE);
> > > +				action);
> > >  	}
> > >  }
> 
> Rafael

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Wednesday 14 April 2010, Michal Hocko wrote:
> On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > Please check if the patch below changes anything.
> > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > That didn't change the behaviour for me, sorry.
> > > > > 
> > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > 
> > > > > > (I made sure to go through a full power down session before trying the
> > > > > > patched kernel)
> > > > > 
> > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > 
> > > > > I'll try to figure out what's going on, please stay tuned.
> > > > 
> > > > Can you please check if the patch below changes the behavior?
> > > 
> > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > rc4).
> > 
> > That gets really weird.
> > 
> > > > ---
> > > >  drivers/acpi/wakeup.c |    6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > >  		struct acpi_device *dev =
> > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > +		u8 action = ACPI_GPE_ENABLE;
> > 
> > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> 
> Unfortunately didn't help as well...
> Just for reference:
> 
> diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> index 248b473..f23c08f 100644
> --- a/drivers/acpi/wakeup.c
> +++ b/drivers/acpi/wakeup.c
> @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
>  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
>  		struct acpi_device *dev =
>  			container_of(node, struct acpi_device, wakeup_list);
> -		u8 action = ACPI_GPE_ENABLE;
> +		u8 action = ACPI_GPE_DISABLE;

That probably means the chipset enables the GPEs by itself _after_ we've
disabled them in acpi_enable_wakeup_device().

Unfortunately, I can't reproduce the issue on any of my test boxes and it's
hard to find the source of the problem staring at the code.

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Michal Hocko]

On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> On Wednesday 14 April 2010, Michal Hocko wrote:
> > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > Please check if the patch below changes anything.
> > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > 
> > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > 
> > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > patched kernel)
> > > > > > 
> > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > 
> > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > 
> > > > > Can you please check if the patch below changes the behavior?
> > > > 
> > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > rc4).
> > > 
> > > That gets really weird.
> > > 
> > > > > ---
> > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > >  		struct acpi_device *dev =
> > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > 
> > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > 
> > Unfortunately didn't help as well...
> > Just for reference:
> > 
> > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > index 248b473..f23c08f 100644
> > --- a/drivers/acpi/wakeup.c
> > +++ b/drivers/acpi/wakeup.c
> > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> >  		struct acpi_device *dev =
> >  			container_of(node, struct acpi_device, wakeup_list);
> > -		u8 action = ACPI_GPE_ENABLE;
> > +		u8 action = ACPI_GPE_DISABLE;
> 
> That probably means the chipset enables the GPEs by itself _after_ we've
> disabled them in acpi_enable_wakeup_device().

Is this something BIOS specific?

> 
> Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> hard to find the source of the problem staring at the code.

Are there any debug options I can turn on to provide some information?

Btw. what exactly does this mean? In what state is the laptop while it
is turned off and GPE is enabled?

Thanks!

> 
> Rafael

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Monday 19 April 2010, Michal Hocko wrote:
> On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> > On Wednesday 14 April 2010, Michal Hocko wrote:
> > > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > Please check if the patch below changes anything.
> > > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > > 
> > > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > > 
> > > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > > patched kernel)
> > > > > > > 
> > > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > > 
> > > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > > 
> > > > > > Can you please check if the patch below changes the behavior?
> > > > > 
> > > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > > rc4).
> > > > 
> > > > That gets really weird.
> > > > 
> > > > > > ---
> > > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > >  		struct acpi_device *dev =
> > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > > 
> > > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > > 
> > > Unfortunately didn't help as well...
> > > Just for reference:
> > > 
> > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > > index 248b473..f23c08f 100644
> > > --- a/drivers/acpi/wakeup.c
> > > +++ b/drivers/acpi/wakeup.c
> > > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > >  		struct acpi_device *dev =
> > >  			container_of(node, struct acpi_device, wakeup_list);
> > > -		u8 action = ACPI_GPE_ENABLE;
> > > +		u8 action = ACPI_GPE_DISABLE;
> > 
> > That probably means the chipset enables the GPEs by itself _after_ we've
> > disabled them in acpi_enable_wakeup_device().
> 
> Is this something BIOS specific?
> 
> > 
> > Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> > hard to find the source of the problem staring at the code.
> 
> Are there any debug options I can turn on to provide some information?

We can only check what the kernel tells us before power off, but all that seems
correct.

> Btw. what exactly does this mean? In what state is the laptop while it
> is turned off and GPE is enabled?

If a GPE is enabled, then some part of the chipset has power provided so that
it can signal wakeup.

I'll look into it a bit more later today.

Thanks,
Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Monday 19 April 2010, Rafael J. Wysocki wrote:
> On Monday 19 April 2010, Michal Hocko wrote:
> > On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> > > On Wednesday 14 April 2010, Michal Hocko wrote:
> > > > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > Please check if the patch below changes anything.
> > > > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > 
> > > > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > > > 
> > > > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > > > 
> > > > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > > > patched kernel)
> > > > > > > > 
> > > > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > > > 
> > > > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > > > 
> > > > > > > Can you please check if the patch below changes the behavior?
> > > > > > 
> > > > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > > > rc4).
> > > > > 
> > > > > That gets really weird.
> > > > > 
> > > > > > > ---
> > > > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > > > ===================================================================
> > > > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > >  		struct acpi_device *dev =
> > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > > > 
> > > > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > > > 
> > > > Unfortunately didn't help as well...
> > > > Just for reference:
> > > > 
> > > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > > > index 248b473..f23c08f 100644
> > > > --- a/drivers/acpi/wakeup.c
> > > > +++ b/drivers/acpi/wakeup.c
> > > > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > >  		struct acpi_device *dev =
> > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > -		u8 action = ACPI_GPE_ENABLE;
> > > > +		u8 action = ACPI_GPE_DISABLE;
> > > 
> > > That probably means the chipset enables the GPEs by itself _after_ we've
> > > disabled them in acpi_enable_wakeup_device().
> > 
> > Is this something BIOS specific?
> > 
> > > 
> > > Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> > > hard to find the source of the problem staring at the code.
> > 
> > Are there any debug options I can turn on to provide some information?
> 
> We can only check what the kernel tells us before power off, but all that seems
> correct.
> 
> > Btw. what exactly does this mean? In what state is the laptop while it
> > is turned off and GPE is enabled?
> 
> If a GPE is enabled, then some part of the chipset has power provided so that
> it can signal wakeup.
> 
> I'll look into it a bit more later today.

Please try the patch below.  It kind of restores the previous behavior,
let's see if it changes anything.

Rafael

---
 drivers/acpi/acpica/evgpeblk.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/acpi/acpica/evgpeblk.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evgpeblk.c
+++ linux-2.6/drivers/acpi/acpica/evgpeblk.c
@@ -364,7 +364,7 @@ acpi_ev_match_prw_and_gpe(acpi_handle ob
 	union acpi_operand_object *pkg_desc;
 	union acpi_operand_object *obj_desc;
 	u32 gpe_number;
-	acpi_status status;
+	acpi_status status = AE_OK;
 
 	ACPI_FUNCTION_TRACE(ev_match_prw_and_gpe);
 
@@ -439,13 +439,15 @@ acpi_ev_match_prw_and_gpe(acpi_handle ob
 	if (gpe_device == target_gpe_device) {
 		gpe_event_info = acpi_ev_gpeblk_event_info(gpe_block,
 							   gpe_number);
-		if (gpe_event_info)
+		if (gpe_event_info) {
+			status = acpi_ev_disable_gpe(gpe_event_info);
 			gpe_event_info->flags |= ACPI_GPE_CAN_WAKE;
+		}
 	}
 
       cleanup:
 	acpi_ut_remove_reference(pkg_desc);
-	return_ACPI_STATUS(AE_OK);
+	return_ACPI_STATUS(status);
 }
 
 /*******************************************************************************

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Sunday 25 April 2010, Rafael J. Wysocki wrote:
> On Monday 19 April 2010, Rafael J. Wysocki wrote:
> > On Monday 19 April 2010, Michal Hocko wrote:
> > > On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> > > > On Wednesday 14 April 2010, Michal Hocko wrote:
> > > > > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > > > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > > Please check if the patch below changes anything.
> > > > > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > > > > 
> > > > > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > > > > 
> > > > > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > > > > patched kernel)
> > > > > > > > > 
> > > > > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > > > > 
> > > > > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > > > > 
> > > > > > > > Can you please check if the patch below changes the behavior?
> > > > > > > 
> > > > > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > > > > rc4).
> > > > > > 
> > > > > > That gets really weird.
> > > > > > 
> > > > > > > > ---
> > > > > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > ===================================================================
> > > > > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > > >  		struct acpi_device *dev =
> > > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > > > > 
> > > > > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > > > > 
> > > > > Unfortunately didn't help as well...
> > > > > Just for reference:
> > > > > 
> > > > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > > > > index 248b473..f23c08f 100644
> > > > > --- a/drivers/acpi/wakeup.c
> > > > > +++ b/drivers/acpi/wakeup.c
> > > > > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > >  		struct acpi_device *dev =
> > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > -		u8 action = ACPI_GPE_ENABLE;
> > > > > +		u8 action = ACPI_GPE_DISABLE;
> > > > 
> > > > That probably means the chipset enables the GPEs by itself _after_ we've
> > > > disabled them in acpi_enable_wakeup_device().
> > > 
> > > Is this something BIOS specific?
> > > 
> > > > 
> > > > Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> > > > hard to find the source of the problem staring at the code.
> > > 
> > > Are there any debug options I can turn on to provide some information?
> > 
> > We can only check what the kernel tells us before power off, but all that seems
> > correct.
> > 
> > > Btw. what exactly does this mean? In what state is the laptop while it
> > > is turned off and GPE is enabled?
> > 
> > If a GPE is enabled, then some part of the chipset has power provided so that
> > it can signal wakeup.
> > 
> > I'll look into it a bit more later today.
> 
> Please try the patch below.  It kind of restores the previous behavior,
> let's see if it changes anything.

If it doesn't help, please try to comment out the acpi_enable_gpe() in
drivers/acpi/wakeup.c:acpi_wakeup_device_init() and retest (if you haven't
done that already).

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Michal Hocko]

On Sun 25-04-10 04:35:42, Rafael J. Wysocki wrote:
> On Monday 19 April 2010, Rafael J. Wysocki wrote:
> > On Monday 19 April 2010, Michal Hocko wrote:
> > > On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> > > > On Wednesday 14 April 2010, Michal Hocko wrote:
> > > > > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > > > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > > Please check if the patch below changes anything.
> > > > > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > > > > 
> > > > > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > > > > 
> > > > > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > > > > patched kernel)
> > > > > > > > > 
> > > > > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > > > > 
> > > > > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > > > > 
> > > > > > > > Can you please check if the patch below changes the behavior?
> > > > > > > 
> > > > > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > > > > rc4).
> > > > > > 
> > > > > > That gets really weird.
> > > > > > 
> > > > > > > > ---
> > > > > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > ===================================================================
> > > > > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > > >  		struct acpi_device *dev =
> > > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > > > > 
> > > > > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > > > > 
> > > > > Unfortunately didn't help as well...
> > > > > Just for reference:
> > > > > 
> > > > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > > > > index 248b473..f23c08f 100644
> > > > > --- a/drivers/acpi/wakeup.c
> > > > > +++ b/drivers/acpi/wakeup.c
> > > > > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > >  		struct acpi_device *dev =
> > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > -		u8 action = ACPI_GPE_ENABLE;
> > > > > +		u8 action = ACPI_GPE_DISABLE;
> > > > 
> > > > That probably means the chipset enables the GPEs by itself _after_ we've
> > > > disabled them in acpi_enable_wakeup_device().
> > > 
> > > Is this something BIOS specific?
> > > 
> > > > 
> > > > Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> > > > hard to find the source of the problem staring at the code.
> > > 
> > > Are there any debug options I can turn on to provide some information?
> > 
> > We can only check what the kernel tells us before power off, but all that seems
> > correct.
> > 
> > > Btw. what exactly does this mean? In what state is the laptop while it
> > > is turned off and GPE is enabled?
> > 
> > If a GPE is enabled, then some part of the chipset has power provided so that
> > it can signal wakeup.
> > 
> > I'll look into it a bit more later today.
> 
> Please try the patch below.  It kind of restores the previous behavior,
> let's see if it changes anything.

Again, no success. Just to make sure that I didn't screw anything. I
have used just the following patch on top of the clean rc5 (your patch
has failed with some rejects but I guess that the following should do
the same):

commit 65eafe4a504e3bb2c13b4feb8590dc52e7439baa
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date:   Sun Apr 25 04:35:42 2010 +0200

diff --git a/drivers/acpi/acpica/evgpeblk.c b/drivers/acpi/acpica/evgpeblk.c
index fef7219..b6e1e0c 100644
--- a/drivers/acpi/acpica/evgpeblk.c
+++ b/drivers/acpi/acpica/evgpeblk.c
@@ -367,7 +367,7 @@ acpi_ev_match_prw_and_gpe(acpi_handle obj_handle,
 	union acpi_operand_object *pkg_desc;
 	union acpi_operand_object *obj_desc;
 	u32 gpe_number;
-	acpi_status status;
+	acpi_status status = AE_OK;
 
 	ACPI_FUNCTION_TRACE(ev_match_prw_and_gpe);
 
@@ -447,12 +447,13 @@ acpi_ev_match_prw_and_gpe(acpi_handle obj_handle,
 							gpe_block->
 							block_base_number];
 
+		status = acpi_ev_disable_gpe(gpe_event_info);
 		gpe_event_info->flags |= ACPI_GPE_CAN_WAKE;
 	}
 
       cleanup:
 	acpi_ut_remove_reference(pkg_desc);
-	return_ACPI_STATUS(AE_OK);
+	return_ACPI_STATUS(status);
 }
 
 /*******************************************************************************

> 
> Rafael
> 
> ---
>  drivers/acpi/acpica/evgpeblk.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/acpica/evgpeblk.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/acpica/evgpeblk.c
> +++ linux-2.6/drivers/acpi/acpica/evgpeblk.c
> @@ -364,7 +364,7 @@ acpi_ev_match_prw_and_gpe(acpi_handle ob
>  	union acpi_operand_object *pkg_desc;
>  	union acpi_operand_object *obj_desc;
>  	u32 gpe_number;
> -	acpi_status status;
> +	acpi_status status = AE_OK;
>  
>  	ACPI_FUNCTION_TRACE(ev_match_prw_and_gpe);
>  
> @@ -439,13 +439,15 @@ acpi_ev_match_prw_and_gpe(acpi_handle ob
>  	if (gpe_device == target_gpe_device) {
>  		gpe_event_info = acpi_ev_gpeblk_event_info(gpe_block,
>  							   gpe_number);
> -		if (gpe_event_info)
> +		if (gpe_event_info) {
> +			status = acpi_ev_disable_gpe(gpe_event_info);
>  			gpe_event_info->flags |= ACPI_GPE_CAN_WAKE;
> +		}
>  	}
>  
>        cleanup:
>  	acpi_ut_remove_reference(pkg_desc);
> -	return_ACPI_STATUS(AE_OK);
> +	return_ACPI_STATUS(status);
>  }
>  
>  /*******************************************************************************
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Len Brown]

Michal,

> my laptop changed behavior during poweroff recently (after upgrading
> from .33 to .34-rc1). The system seems to be powered off (status display
> is off) at first glance but when I close the lid then I can hear a noise
> which sounds like HDD parking and when I open lid again it starts
> booting without poweroff button (same like when I suspend to RAM).

Tony,

> I have the exact same "lid close = powering back on" regression on a
> Fujitsu-Siemens Lifebook S6420.

Perhaps you can review this description and help me
better understand the symptoms?

Michal,
Re: poweroff

Before the regression you could hear the HDD park as a result of the
"poweroff" command without touching the lid, and after the regression
you don't hear that sound until after you close the lid -- no matter
how long you wait to close the lid?

During this period after poweroff and LCD goes out, but before
lid close, are there any other signs of life?  eg. does caps
lock button light the caps LED?

How about during this period if you press the power button and
hold it for 5 seconds, does that cause the HDD parking sound?

If yes, then the regression is that poweroff never completes,
though perhaps a lid event allows it to complete.

When you say 'when I open the lid again i starts booting'.
Is that a cold boot from scratch, or a resume from suspend?

Does the new boot start when the lid is closed, or when it is
subsequently opened?

Thanks for sending your /proc/acpi/wakeup, it shows that

LID       S4    *enabled  

which means that the LID is not an S5 wakeup device.
So if you really do get down to S5, the LID should
not be able to wake the system.  So if it is, then
we are never getting to S5.

Tony,
Please send the /proc/acpi/wakeup file before and after
the regression.  Does the system reach a complete poweroff
before the lid is closed?  If yes, what happens when
you press the power button w/o closing the lid?
If you reached S5, it should give you a cold boot.
If you did not reach S5, it will have no effect.

thanks,
Len Brown, Intel Open Source Technology Center

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Monday 26 April 2010, Michal Hocko wrote:
> On Sun 25-04-10 04:35:42, Rafael J. Wysocki wrote:
> > On Monday 19 April 2010, Rafael J. Wysocki wrote:
> > > On Monday 19 April 2010, Michal Hocko wrote:
> > > > On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> > > > > On Wednesday 14 April 2010, Michal Hocko wrote:
> > > > > > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > > > > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > > > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > > > Please check if the patch below changes anything.
> > > > > > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > > > > > 
> > > > > > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > > > > > 
> > > > > > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > > > > > patched kernel)
> > > > > > > > > > 
> > > > > > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > > > > > 
> > > > > > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > > > > > 
> > > > > > > > > Can you please check if the patch below changes the behavior?
> > > > > > > > 
> > > > > > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > > > > > rc4).
> > > > > > > 
> > > > > > > That gets really weird.
> > > > > > > 
> > > > > > > > > ---
> > > > > > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > > 
> > > > > > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > > ===================================================================
> > > > > > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > > > >  		struct acpi_device *dev =
> > > > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > > > > > 
> > > > > > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > > > > > 
> > > > > > Unfortunately didn't help as well...
> > > > > > Just for reference:
> > > > > > 
> > > > > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > > > > > index 248b473..f23c08f 100644
> > > > > > --- a/drivers/acpi/wakeup.c
> > > > > > +++ b/drivers/acpi/wakeup.c
> > > > > > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > >  		struct acpi_device *dev =
> > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > -		u8 action = ACPI_GPE_ENABLE;
> > > > > > +		u8 action = ACPI_GPE_DISABLE;
> > > > > 
> > > > > That probably means the chipset enables the GPEs by itself _after_ we've
> > > > > disabled them in acpi_enable_wakeup_device().
> > > > 
> > > > Is this something BIOS specific?
> > > > 
> > > > > 
> > > > > Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> > > > > hard to find the source of the problem staring at the code.
> > > > 
> > > > Are there any debug options I can turn on to provide some information?
> > > 
> > > We can only check what the kernel tells us before power off, but all that seems
> > > correct.
> > > 
> > > > Btw. what exactly does this mean? In what state is the laptop while it
> > > > is turned off and GPE is enabled?
> > > 
> > > If a GPE is enabled, then some part of the chipset has power provided so that
> > > it can signal wakeup.
> > > 
> > > I'll look into it a bit more later today.
> > 
> > Please try the patch below.  It kind of restores the previous behavior,
> > let's see if it changes anything.
> 
> Again, no success. Just to make sure that I didn't screw anything. I
> have used just the following patch on top of the clean rc5 (your patch
> has failed with some rejects but I guess that the following should do
> the same):

Thanks for testing.  Did you try the second thing, ie. try to comment out the
acpi_enable_gpe() in drivers/acpi/wakeup.c:acpi_wakeup_device_init() and
retest (if you haven't done that already)?

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Michal Hocko]

On Mon 26-04-10 20:51:09, Rafael J. Wysocki wrote:
> On Monday 26 April 2010, Michal Hocko wrote:
> > On Sun 25-04-10 04:35:42, Rafael J. Wysocki wrote:
> > > On Monday 19 April 2010, Rafael J. Wysocki wrote:
> > > > On Monday 19 April 2010, Michal Hocko wrote:
> > > > > On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> > > > > > On Wednesday 14 April 2010, Michal Hocko wrote:
> > > > > > > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > > > > > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > > > > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > > > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > > > > Please check if the patch below changes anything.
> > > > > > > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > > > > > > 
> > > > > > > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > > > > > > 
> > > > > > > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > > > > > > patched kernel)
> > > > > > > > > > > 
> > > > > > > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > > > > > > 
> > > > > > > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > > > > > > 
> > > > > > > > > > Can you please check if the patch below changes the behavior?
> > > > > > > > > 
> > > > > > > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > > > > > > rc4).
> > > > > > > > 
> > > > > > > > That gets really weird.
> > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > > > ===================================================================
> > > > > > > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > > > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > > > > >  		struct acpi_device *dev =
> > > > > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > > > > > > 
> > > > > > > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > > > > > > 
> > > > > > > Unfortunately didn't help as well...
> > > > > > > Just for reference:
> > > > > > > 
> > > > > > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > > > > > > index 248b473..f23c08f 100644
> > > > > > > --- a/drivers/acpi/wakeup.c
> > > > > > > +++ b/drivers/acpi/wakeup.c
> > > > > > > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > >  		struct acpi_device *dev =
> > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > -		u8 action = ACPI_GPE_ENABLE;
> > > > > > > +		u8 action = ACPI_GPE_DISABLE;
> > > > > > 
> > > > > > That probably means the chipset enables the GPEs by itself _after_ we've
> > > > > > disabled them in acpi_enable_wakeup_device().
> > > > > 
> > > > > Is this something BIOS specific?
> > > > > 
> > > > > > 
> > > > > > Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> > > > > > hard to find the source of the problem staring at the code.
> > > > > 
> > > > > Are there any debug options I can turn on to provide some information?
> > > > 
> > > > We can only check what the kernel tells us before power off, but all that seems
> > > > correct.
> > > > 
> > > > > Btw. what exactly does this mean? In what state is the laptop while it
> > > > > is turned off and GPE is enabled?
> > > > 
> > > > If a GPE is enabled, then some part of the chipset has power provided so that
> > > > it can signal wakeup.
> > > > 
> > > > I'll look into it a bit more later today.
> > > 
> > > Please try the patch below.  It kind of restores the previous behavior,
> > > let's see if it changes anything.
> > 
> > Again, no success. Just to make sure that I didn't screw anything. I
> > have used just the following patch on top of the clean rc5 (your patch
> > has failed with some rejects but I guess that the following should do
> > the same):
> 
> Thanks for testing.  Did you try the second thing, ie. try to comment out the
> acpi_enable_gpe() in drivers/acpi/wakeup.c:acpi_wakeup_device_init() and
> retest (if you haven't done that already)?

I have tried it just now and still no success.


commit 1a7e7c9ac82aa2de185fa2f686417a5eb7765420
Author: Michal Hocko <mhocko@suse.cz>
Date:   Tue Apr 27 08:22:23 2010 +0200

    disable acpi_enable_gpe in acpi_wakeup_device_init
    
    as suggested by Rafael.

diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
index 4b9d339..a0f4859 100644
--- a/drivers/acpi/wakeup.c
+++ b/drivers/acpi/wakeup.c
@@ -113,8 +113,9 @@ int __init acpi_wakeup_device_init(void)
 		if (!dev->wakeup.flags.always_enabled ||
 		    dev->wakeup.state.enabled)
 			continue;
- 		acpi_enable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
+ 		/*acpi_enable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
  				ACPI_GPE_TYPE_WAKE);
+		 */
 		dev->wakeup.state.enabled = 1;
 	}
 	mutex_unlock(&acpi_device_lock);

On top of the previous patch. If I understand that correctly then this
is really strange because this patch disables GPE completely, doesn't
it?

> 
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Tuesday 27 April 2010, Michal Hocko wrote:
> On Mon 26-04-10 20:51:09, Rafael J. Wysocki wrote:
> > On Monday 26 April 2010, Michal Hocko wrote:
> > > On Sun 25-04-10 04:35:42, Rafael J. Wysocki wrote:
> > > > On Monday 19 April 2010, Rafael J. Wysocki wrote:
> > > > > On Monday 19 April 2010, Michal Hocko wrote:
> > > > > > On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> > > > > > > On Wednesday 14 April 2010, Michal Hocko wrote:
> > > > > > > > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > > > > > > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > > > > > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > > > > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > > > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > > > > > Please check if the patch below changes anything.
> > > > > > > > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > > > > > > > 
> > > > > > > > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > > > > > > > 
> > > > > > > > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > > > > > > > patched kernel)
> > > > > > > > > > > > 
> > > > > > > > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > > > > > > > 
> > > > > > > > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > > > > > > > 
> > > > > > > > > > > Can you please check if the patch below changes the behavior?
> > > > > > > > > > 
> > > > > > > > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > > > > > > > rc4).
> > > > > > > > > 
> > > > > > > > > That gets really weird.
> > > > > > > > > 
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > > > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > > > > ===================================================================
> > > > > > > > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > > > > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > > > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > > > > > >  		struct acpi_device *dev =
> > > > > > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > > > > > > > 
> > > > > > > > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > > > > > > > 
> > > > > > > > Unfortunately didn't help as well...
> > > > > > > > Just for reference:
> > > > > > > > 
> > > > > > > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > > > > > > > index 248b473..f23c08f 100644
> > > > > > > > --- a/drivers/acpi/wakeup.c
> > > > > > > > +++ b/drivers/acpi/wakeup.c
> > > > > > > > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> > > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > > >  		struct acpi_device *dev =
> > > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > > -		u8 action = ACPI_GPE_ENABLE;
> > > > > > > > +		u8 action = ACPI_GPE_DISABLE;
> > > > > > > 
> > > > > > > That probably means the chipset enables the GPEs by itself _after_ we've
> > > > > > > disabled them in acpi_enable_wakeup_device().
> > > > > > 
> > > > > > Is this something BIOS specific?
> > > > > > 
> > > > > > > 
> > > > > > > Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> > > > > > > hard to find the source of the problem staring at the code.
> > > > > > 
> > > > > > Are there any debug options I can turn on to provide some information?
> > > > > 
> > > > > We can only check what the kernel tells us before power off, but all that seems
> > > > > correct.
> > > > > 
> > > > > > Btw. what exactly does this mean? In what state is the laptop while it
> > > > > > is turned off and GPE is enabled?
> > > > > 
> > > > > If a GPE is enabled, then some part of the chipset has power provided so that
> > > > > it can signal wakeup.
> > > > > 
> > > > > I'll look into it a bit more later today.
> > > > 
> > > > Please try the patch below.  It kind of restores the previous behavior,
> > > > let's see if it changes anything.
> > > 
> > > Again, no success. Just to make sure that I didn't screw anything. I
> > > have used just the following patch on top of the clean rc5 (your patch
> > > has failed with some rejects but I guess that the following should do
> > > the same):
> > 
> > Thanks for testing.  Did you try the second thing, ie. try to comment out the
> > acpi_enable_gpe() in drivers/acpi/wakeup.c:acpi_wakeup_device_init() and
> > retest (if you haven't done that already)?
> 
> I have tried it just now and still no success.
> 
> 
> commit 1a7e7c9ac82aa2de185fa2f686417a5eb7765420
> Author: Michal Hocko <mhocko@suse.cz>
> Date:   Tue Apr 27 08:22:23 2010 +0200
> 
>     disable acpi_enable_gpe in acpi_wakeup_device_init
>     
>     as suggested by Rafael.
> 
> diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> index 4b9d339..a0f4859 100644
> --- a/drivers/acpi/wakeup.c
> +++ b/drivers/acpi/wakeup.c
> @@ -113,8 +113,9 @@ int __init acpi_wakeup_device_init(void)
>  		if (!dev->wakeup.flags.always_enabled ||
>  		    dev->wakeup.state.enabled)
>  			continue;
> - 		acpi_enable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> + 		/*acpi_enable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
>   				ACPI_GPE_TYPE_WAKE);
> +		 */
>  		dev->wakeup.state.enabled = 1;
>  	}
>  	mutex_unlock(&acpi_device_lock);
> 
> On top of the previous patch. If I understand that correctly then this
> is really strange because this patch disables GPE completely, doesn't
> it?

Kind of, but this only happens for devices that are not handled by the button
driver which now has become our primary suspect.

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Rafael J. Wysocki]

On Wednesday 28 April 2010, Rafael J. Wysocki wrote:
> On Tuesday 27 April 2010, Michal Hocko wrote:
> > On Mon 26-04-10 20:51:09, Rafael J. Wysocki wrote:
> > > On Monday 26 April 2010, Michal Hocko wrote:
> > > > On Sun 25-04-10 04:35:42, Rafael J. Wysocki wrote:
> > > > > On Monday 19 April 2010, Rafael J. Wysocki wrote:
> > > > > > On Monday 19 April 2010, Michal Hocko wrote:
> > > > > > > On Fri 16-04-10 20:00:29, Rafael J. Wysocki wrote:
> > > > > > > > On Wednesday 14 April 2010, Michal Hocko wrote:
> > > > > > > > > On Tue 13-04-10 22:53:37, Rafael J. Wysocki wrote:
> > > > > > > > > > On Tuesday 13 April 2010, Michal Hocko wrote:
> > > > > > > > > > > On Tue 13-04-10 01:01:54, Rafael J. Wysocki wrote:
> > > > > > > > > > > > On Saturday 10 April 2010, Rafael J. Wysocki wrote:
> > > > > > > > > > > > > On Friday 09 April 2010, Tony Vroon wrote:
> > > > > > > > > > > > > > On Fri, 2010-04-09 at 22:42 +0200, Rafael J. Wysocki wrote:
> > > > > > > > > > > > > > > Please check if the patch below changes anything.
> > > > > > > > > > > > > > >  drivers/acpi/wakeup.c |    4 ++--
> > > > > > > > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > That didn't change the behaviour for me, sorry.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Well, I would be sorry if it did, because the patch removed some useful code. :-)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > (I made sure to go through a full power down session before trying the
> > > > > > > > > > > > > > patched kernel)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Thanks for testing.  So it looks like we don't disable the GPE during power off.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I'll try to figure out what's going on, please stay tuned.
> > > > > > > > > > > > 
> > > > > > > > > > > > Can you please check if the patch below changes the behavior?
> > > > > > > > > > > 
> > > > > > > > > > > Unfortunately, it didn't help either (I have tried on top of the fresh
> > > > > > > > > > > rc4).
> > > > > > > > > > 
> > > > > > > > > > That gets really weird.
> > > > > > > > > > 
> > > > > > > > > > > > ---
> > > > > > > > > > > >  drivers/acpi/wakeup.c |    6 +++---
> > > > > > > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > Index: linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > > > > > ===================================================================
> > > > > > > > > > > > --- linux-2.6.orig/drivers/acpi/wakeup.c
> > > > > > > > > > > > +++ linux-2.6/drivers/acpi/wakeup.c
> > > > > > > > > > > > @@ -63,17 +63,17 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > > > > > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > > > > > > >  		struct acpi_device *dev =
> > > > > > > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > > > > > > +		u8 action = ACPI_GPE_ENABLE;
> > > > > > > > > > 
> > > > > > > > > > Can you try to change the above to ACPI_GPE_DISABLE and retest, please?
> > > > > > > > > 
> > > > > > > > > Unfortunately didn't help as well...
> > > > > > > > > Just for reference:
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > > > > > > > > index 248b473..f23c08f 100644
> > > > > > > > > --- a/drivers/acpi/wakeup.c
> > > > > > > > > +++ b/drivers/acpi/wakeup.c
> > > > > > > > > @@ -63,7 +63,7 @@ void acpi_enable_wakeup_device(u8 sleep_state)
> > > > > > > > >  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
> > > > > > > > >  		struct acpi_device *dev =
> > > > > > > > >  			container_of(node, struct acpi_device, wakeup_list);
> > > > > > > > > -		u8 action = ACPI_GPE_ENABLE;
> > > > > > > > > +		u8 action = ACPI_GPE_DISABLE;
> > > > > > > > 
> > > > > > > > That probably means the chipset enables the GPEs by itself _after_ we've
> > > > > > > > disabled them in acpi_enable_wakeup_device().
> > > > > > > 
> > > > > > > Is this something BIOS specific?
> > > > > > > 
> > > > > > > > 
> > > > > > > > Unfortunately, I can't reproduce the issue on any of my test boxes and it's
> > > > > > > > hard to find the source of the problem staring at the code.
> > > > > > > 
> > > > > > > Are there any debug options I can turn on to provide some information?
> > > > > > 
> > > > > > We can only check what the kernel tells us before power off, but all that seems
> > > > > > correct.
> > > > > > 
> > > > > > > Btw. what exactly does this mean? In what state is the laptop while it
> > > > > > > is turned off and GPE is enabled?
> > > > > > 
> > > > > > If a GPE is enabled, then some part of the chipset has power provided so that
> > > > > > it can signal wakeup.
> > > > > > 
> > > > > > I'll look into it a bit more later today.
> > > > > 
> > > > > Please try the patch below.  It kind of restores the previous behavior,
> > > > > let's see if it changes anything.
> > > > 
> > > > Again, no success. Just to make sure that I didn't screw anything. I
> > > > have used just the following patch on top of the clean rc5 (your patch
> > > > has failed with some rejects but I guess that the following should do
> > > > the same):
> > > 
> > > Thanks for testing.  Did you try the second thing, ie. try to comment out the
> > > acpi_enable_gpe() in drivers/acpi/wakeup.c:acpi_wakeup_device_init() and
> > > retest (if you haven't done that already)?
> > 
> > I have tried it just now and still no success.
> > 
> > 
> > commit 1a7e7c9ac82aa2de185fa2f686417a5eb7765420
> > Author: Michal Hocko <mhocko@suse.cz>
> > Date:   Tue Apr 27 08:22:23 2010 +0200
> > 
> >     disable acpi_enable_gpe in acpi_wakeup_device_init
> >     
> >     as suggested by Rafael.
> > 
> > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
> > index 4b9d339..a0f4859 100644
> > --- a/drivers/acpi/wakeup.c
> > +++ b/drivers/acpi/wakeup.c
> > @@ -113,8 +113,9 @@ int __init acpi_wakeup_device_init(void)
> >  		if (!dev->wakeup.flags.always_enabled ||
> >  		    dev->wakeup.state.enabled)
> >  			continue;
> > - 		acpi_enable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> > + 		/*acpi_enable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
> >   				ACPI_GPE_TYPE_WAKE);
> > +		 */
> >  		dev->wakeup.state.enabled = 1;
> >  	}
> >  	mutex_unlock(&acpi_device_lock);
> > 
> > On top of the previous patch. If I understand that correctly then this
> > is really strange because this patch disables GPE completely, doesn't
> > it?
> 
> Kind of, but this only happens for devices that are not handled by the button
> driver which now has become our primary suspect.

I'm thinking at the moment that we probably enable something at the hardware
level that we didn't before.

Why it is not _disabled_ before power off although there are all of the
necessary things in the code remains to be a mystery to me.

Rafael

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Michal Hocko]

Hi Len,

On Mon 26-04-10 12:22:58, Len Brown wrote:
> Michal,
> 
> > my laptop changed behavior during poweroff recently (after upgrading
> > from .33 to .34-rc1). The system seems to be powered off (status display
> > is off) at first glance but when I close the lid then I can hear a noise
> > which sounds like HDD parking and when I open lid again it starts
> > booting without poweroff button (same like when I suspend to RAM).
> 
> Tony,
> 
> > I have the exact same "lid close = powering back on" regression on a
> > Fujitsu-Siemens Lifebook S6420.
> 
> Perhaps you can review this description and help me
> better understand the symptoms?
> 
> Michal,
> Re: poweroff
> 
> Before the regression you could hear the HDD park as a result of the

It is hard to tell whether this sound comes from HDD and I think that it
is actually something different as I've realized that I've heard this
kind of sound only when system was suspended to RAM and lid was closed.
Never during poweroff.

> "poweroff" command without touching the lid, and after the regression
> you don't hear that sound until after you close the lid -- no matter
> how long you wait to close the lid?

Yes. Also the main difference is that the laptop turns on automatically
after lid is open which did not happen before.

> 
> During this period after poweroff and LCD goes out, but before
> lid close, are there any other signs of life?  eg. does caps
> lock button light the caps LED?

No signs of life at all. The small display which shows the status
(charging, battery status, caps lock, num lock, etc.) is turned off.
No reaction to any key press.

> 
> How about during this period if you press the power button and
> hold it for 5 seconds, does that cause the HDD parking sound?

No sound. Even when I closed the lid. Nevertheless, laptop doesn't
start automatically after lid is open.

> 
> If yes, then the regression is that poweroff never completes,
> though perhaps a lid event allows it to complete.
> 
> When you say 'when I open the lid again i starts booting'.
> Is that a cold boot from scratch, or a resume from suspend?

Cold boot.

> 
> Does the new boot start when the lid is closed, or when it is
> subsequently opened?

I have not seen start during lid being closed. As I already said, the
system starts up automatically when I open the lid.

> 
> Thanks for sending your /proc/acpi/wakeup, it shows that
> 
> LID       S4    *enabled  
> 
> which means that the LID is not an S5 wakeup device.
> So if you really do get down to S5, the LID should
> not be able to wake the system.  So if it is, then
> we are never getting to S5.
> 
> Tony,
> Please send the /proc/acpi/wakeup file before and after
> the regression.  Does the system reach a complete poweroff
> before the lid is closed?  If yes, what happens when
> you press the power button w/o closing the lid?
> If you reached S5, it should give you a cold boot.
> If you did not reach S5, it will have no effect.
> 
> thanks,
> Len Brown, Intel Open Source Technology Center
> 

Thanks!
-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: commit 9630bdd9 changes behavior of the poweroff - bug?

[Tony Vroon]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 26/04/10 17:22, Len Brown wrote:
> During this period after poweroff and LCD goes out, but before
> lid close, are there any other signs of life?  eg. does caps
> lock button light the caps LED?

The caps lock button does not respond, no.

> How about during this period if you press the power button and
> hold it for 5 seconds, does that cause the HDD parking sound?

Holding down the power button for 5 seconds has no audible or visible
effects, but does indeed properly shut it down. Closing or opening the
lid afterwards does not cause a phantom power-up.
A short push of the power button seems to start the machine up normally
(from a cold boot; just like when the lid is closed/opened after a linux
"shutdown").

> Please send the /proc/acpi/wakeup file before and after
> the regression.

I don't have this file in my /proc filesystem, sorry.

Regards,
- --
Tony Vroon
UNIX systems administrator
London Internet Exchange Ltd, Trinity Court, Trinity Street,
Peterborough, PE1 1DA
Registered in England number 3137929
E-Mail: tony@linx.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.15 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkvZmdYACgkQp5vW4rUFj5oUxgCdGPnfQsSE9jhjSzzar/kQbw95
Er4AmgLGStZlrvGZltqsU/REs2srO95A
=6VLH
-----END PGP SIGNATURE-----