[patch] Refresh lid state on resume

[patch] Refresh lid state on resume

[Richard Hughes]

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

On resume we need to refresh the lid status as we will not get an event if
the lid opening was what triggered the suspend.
This manifests itself in users never getting a "lid open" event when a
suspend happens because of lid close on hardware that supports wake on
lid open. This makes userspace gets very confused indeed.
Patch inline (and also attached) forces a check of the lid status in the
resume handler.

Signed-off-by: Richard Hughes <richard@hughsie.com>
---

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index cb4110b..fd3473b 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -68,6 +68,7 @@ MODULE_LICENSE("GPL");
 
 static int acpi_button_add(struct acpi_device *device);
 static int acpi_button_remove(struct acpi_device *device, int type);
+static int acpi_button_resume(struct acpi_device *device);
 static int acpi_button_info_open_fs(struct inode *inode, struct file *file);
 static int acpi_button_state_open_fs(struct inode *inode, struct file *file);
 
@@ -77,6 +78,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ids = "button_power,button_sleep,PNP0C0D,PNP0C0C,PNP0C0E",
 	.ops = {
 		.add = acpi_button_add,
+		.resume = acpi_button_resume,
 		.remove = acpi_button_remove,
 	},
 };
@@ -487,6 +489,29 @@ static int acpi_button_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+/* this is needed to learn about changes made in suspended state */
+static int acpi_button_resume(struct acpi_device *device)
+{
+	struct acpi_button *button;
+	struct acpi_handle *handle;
+	struct input_dev *input;
+	unsigned long state;
+
+	button = device->driver_data;
+	handle = button->device->handle;
+	input = button->input;
+
+	/*
+	 * On resume we send the state; if it matches to what input layer
+	 * thinks then the event will not even reach userspace.
+	 */
+	if (!ACPI_FAILURE(acpi_evaluate_integer(handle, "_LID",
+						NULL, &state)))
+		input_report_switch(input, SW_LID, !state);
+
+	return 0;
+}
+
 static int __init acpi_button_init(void)
 {
 	int result;

[-- Attachment #2: button-resume.patch --]
[-- Type: message/rfc822, Size: 2209 bytes --]

From: Richard Hughes <richard@hughsie.com>
Subject: No Subject
Date: Fri, 15 Jun 2007 15:53:11 +0100
Message-ID: <1181919191.2681.5.camel@work>

On resume we need to refresh the lid status as we will not get an event if
the lid opening was what triggered the suspend.
This manifests itself in users never getting a "lid open" event when a
suspend happens because of lid close on hardware that supports wake on
lid open. This makes userspace gets very confused indeed.
Patch attached forces a check of the lid status in the resume handler.

Signed-off-by: Richard Hughes <richard@hughsie.com>
---

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index cb4110b..fd3473b 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -68,6 +68,7 @@ MODULE_LICENSE("GPL");
 
 static int acpi_button_add(struct acpi_device *device);
 static int acpi_button_remove(struct acpi_device *device, int type);
+static int acpi_button_resume(struct acpi_device *device);
 static int acpi_button_info_open_fs(struct inode *inode, struct file *file);
 static int acpi_button_state_open_fs(struct inode *inode, struct file *file);
 
@@ -77,6 +78,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ids = "button_power,button_sleep,PNP0C0D,PNP0C0C,PNP0C0E",
 	.ops = {
 		.add = acpi_button_add,
+		.resume = acpi_button_resume,
 		.remove = acpi_button_remove,
 	},
 };
@@ -487,6 +489,29 @@ static int acpi_button_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+/* this is needed to learn about changes made in suspended state */
+static int acpi_button_resume(struct acpi_device *device)
+{
+	struct acpi_button *button;
+	struct acpi_handle *handle;
+	struct input_dev *input;
+	unsigned long state;
+
+	button = device->driver_data;
+	handle = button->device->handle;
+	input = button->input;
+
+	/*
+	 * On resume we send the state; if it matches to what input layer
+	 * thinks then the event will not even reach userspace.
+	 */
+	if (!ACPI_FAILURE(acpi_evaluate_integer(handle, "_LID",
+						NULL, &state)))
+		input_report_switch(input, SW_LID, !state);
+
+	return 0;
+}
+
 static int __init acpi_button_init(void)
 {
 	int result;

Re: [patch] Refresh lid state on resume

[Thomas Renninger]

On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:
> On resume we need to refresh the lid status as we will not get an event if
> the lid opening was what triggered the suspend.
> This manifests itself in users never getting a "lid open" event when a
> suspend happens because of lid close on hardware that supports wake on
> lid open. This makes userspace gets very confused indeed.
> Patch inline (and also attached) forces a check of the lid status in the
> resume handler.
Is this a general problem on all machines?
Or does this only happen if "shutdown" suspend mode is used?
AFAIK AML code (only on some machines? or only in suspend functions
which are not invoked in shutdown mode?) checks for LID state change and
invokes a notify(LID0,0x80) (or similar).
If this only happens in shutdown mode then IMO this is not needed ->
working around a workaround makes not much sense.
I could imagine a lot machines let it up to OS to check for LID state
change, then this one should be added.

Thanks,

    Thomas

> Signed-off-by: Richard Hughes <richard@hughsie.com>
> ---
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index cb4110b..fd3473b 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -68,6 +68,7 @@ MODULE_LICENSE("GPL");
>  
>  static int acpi_button_add(struct acpi_device *device);
>  static int acpi_button_remove(struct acpi_device *device, int type);
> +static int acpi_button_resume(struct acpi_device *device);
>  static int acpi_button_info_open_fs(struct inode *inode, struct file *file);
>  static int acpi_button_state_open_fs(struct inode *inode, struct file *file);
>  
> @@ -77,6 +78,7 @@ static struct acpi_driver acpi_button_driver = {
>  	.ids = "button_power,button_sleep,PNP0C0D,PNP0C0C,PNP0C0E",
>  	.ops = {
>  		.add = acpi_button_add,
> +		.resume = acpi_button_resume,
>  		.remove = acpi_button_remove,
>  	},
>  };
> @@ -487,6 +489,29 @@ static int acpi_button_remove(struct acpi_device *device, int type)
>  	return 0;
>  }
>  
> +/* this is needed to learn about changes made in suspended state */
> +static int acpi_button_resume(struct acpi_device *device)
> +{
> +	struct acpi_button *button;
> +	struct acpi_handle *handle;
> +	struct input_dev *input;
> +	unsigned long state;
> +
> +	button = device->driver_data;
> +	handle = button->device->handle;
> +	input = button->input;
> +
> +	/*
> +	 * On resume we send the state; if it matches to what input layer
> +	 * thinks then the event will not even reach userspace.
> +	 */
> +	if (!ACPI_FAILURE(acpi_evaluate_integer(handle, "_LID",
> +						NULL, &state)))
> +		input_report_switch(input, SW_LID, !state);
> +
> +	return 0;
> +}
> +
>  static int __init acpi_button_init(void)
>  {
>  	int result;

Re: [patch] Refresh lid state on resume

[Richard Hughes]

On Mon, 2007-07-30 at 17:21 +0200, Thomas Renninger wrote:
> On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:
> > On resume we need to refresh the lid status as we will not get an event if
> > the lid opening was what triggered the suspend.
> > This manifests itself in users never getting a "lid open" event when a
> > suspend happens because of lid close on hardware that supports wake on
> > lid open. This makes userspace gets very confused indeed.
> > Patch inline (and also attached) forces a check of the lid status in the
> > resume handler.
> Is this a general problem on all machines?

I've only seen myself it on new ThinkPads such as the T61 and X60,
although I've been getting a few bug reports about other IBM laptops.

> Or does this only happen if "shutdown" suspend mode is used?

No, I don't believe so.

> I could imagine a lot machines let it up to OS to check for LID state
> change, then this one should be added.

I guess it's up to the BIOS, and I don't think this refresh hurts any
machines that implement a notify on resume, and fixes a fair few
machines that don't.

Thanks,

Richard.

Re: [patch] Refresh lid state on resume

[Richard Hughes]

On Tue, 2007-07-31 at 11:08 +0200, Rafael J. Wysocki wrote:
> On Monday, 30 July 2007 17:33, Richard Hughes wrote:
> > On Mon, 2007-07-30 at 17:21 +0200, Thomas Renninger wrote:
> > > On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:
> > > > On resume we need to refresh the lid status as we will not get an event if
> > > > the lid opening was what triggered the suspend.
> > > > This manifests itself in users never getting a "lid open" event when a
> > > > suspend happens because of lid close on hardware that supports wake on
> > > > lid open. This makes userspace gets very confused indeed.
> > > > Patch inline (and also attached) forces a check of the lid status in the
> > > > resume handler.
> > > Is this a general problem on all machines?
> > 
> > I've only seen myself it on new ThinkPads such as the T61 and X60,
> > although I've been getting a few bug reports about other IBM laptops.
> > 
> > > Or does this only happen if "shutdown" suspend mode is used?
> > 
> > No, I don't believe so.
> > 
> > > I could imagine a lot machines let it up to OS to check for LID state
> > > change, then this one should be added.
> > 
> > I guess it's up to the BIOS, and I don't think this refresh hurts any
> > machines that implement a notify on resume, and fixes a fair few
> > machines that don't.
> 
> AFAICS, the notify doesn't seem to work very well on some machines.

Agree.

> Are there any downsides of the $subject patch?

Not that I've found. I've been testing it on ~6 IBM and non-IBM machines
with no bad effects so far.

Richard.

Re: [patch] Refresh lid state on resume

[Rafael J. Wysocki]

On Monday, 30 July 2007 17:33, Richard Hughes wrote:
> On Mon, 2007-07-30 at 17:21 +0200, Thomas Renninger wrote:
> > On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:
> > > On resume we need to refresh the lid status as we will not get an event if
> > > the lid opening was what triggered the suspend.
> > > This manifests itself in users never getting a "lid open" event when a
> > > suspend happens because of lid close on hardware that supports wake on
> > > lid open. This makes userspace gets very confused indeed.
> > > Patch inline (and also attached) forces a check of the lid status in the
> > > resume handler.
> > Is this a general problem on all machines?
> 
> I've only seen myself it on new ThinkPads such as the T61 and X60,
> although I've been getting a few bug reports about other IBM laptops.
> 
> > Or does this only happen if "shutdown" suspend mode is used?
> 
> No, I don't believe so.
> 
> > I could imagine a lot machines let it up to OS to check for LID state
> > change, then this one should be added.
> 
> I guess it's up to the BIOS, and I don't think this refresh hurts any
> machines that implement a notify on resume, and fixes a fair few
> machines that don't.

AFAICS, the notify doesn't seem to work very well on some machines.

Are there any downsides of the $subject patch?

Greetings,
Rafael


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

Re: [patch] Refresh lid state on resume

[Thomas Renninger]

On Tue, 2007-07-31 at 10:07 +0100, Richard Hughes wrote:
> On Tue, 2007-07-31 at 11:08 +0200, Rafael J. Wysocki wrote:
> > On Monday, 30 July 2007 17:33, Richard Hughes wrote:
> > > On Mon, 2007-07-30 at 17:21 +0200, Thomas Renninger wrote:
> > > > On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:
> > > > > On resume we need to refresh the lid status as we will not get an event if
> > > > > the lid opening was what triggered the suspend.
> > > > > This manifests itself in users never getting a "lid open" event when a
> > > > > suspend happens because of lid close on hardware that supports wake on
> > > > > lid open. This makes userspace gets very confused indeed.
> > > > > Patch inline (and also attached) forces a check of the lid status in the
> > > > > resume handler.
> > > > Is this a general problem on all machines?
> > > 
> > > I've only seen myself it on new ThinkPads such as the T61 and X60,
> > > although I've been getting a few bug reports about other IBM laptops.
> > > 
> > > > Or does this only happen if "shutdown" suspend mode is used?
> > > 
> > > No, I don't believe so.
> > > 
> > > > I could imagine a lot machines let it up to OS to check for LID state
> > > > change, then this one should be added.
> > > 
> > > I guess it's up to the BIOS, and I don't think this refresh hurts any
> > > machines that implement a notify on resume, and fixes a fair few
> > > machines that don't.
> > 
> > AFAICS, the notify doesn't seem to work very well on some machines.
> 
> Agree.
> 
> > Are there any downsides of the $subject patch?
> 
> Not that I've found. I've been testing it on ~6 IBM and non-IBM machines
> with no bad effects so far.

I just checked a X60 DSDT (couldn't check the SSDTs, but I doubt there
is anything related):
There are two Notify(\_SB.LID,0x80), both are in GPE handlers.
AFAIK there should be one in the _WAK function.
Maybe they try to raise the GPE after wakeup in _WAK by something like
this:
\VSLD (\_SB.LID._LID ())
....
Method (VSLD, 1, NotSerialized)
    {
        SMI (0x01, 0x07, Arg0, 0x00, 0x00)
    }
:)


Related ACPI Spec parts:

6.3 Device Insertion, Removal, and Status Objects:
The Notify command can also be used from the _WAK control method (for
more information about _WAK, see section 7.3.7 “\_WAK (System Wake)”) to
indicate device changes that may have occurred while the computer was
sleeping. For more information about the Notify command,
see section 5.6.3 “Device Object Notification.”.”

The X60 is definitely not doing this.

The transition from Working to Sleep state is described very detailed,
but I couldn't find (just overseen?) a detailed description about the
transition from Sleep State to working state.
In detail I searched for whether first the GPEs should get enabled and
then _WAK is called or the other way around (the latter is currently
implemented).
Maybe enabling GPEs before calling _WAK will also fix this (and is the
way it should be done or at least the way M$ is doing it?).

Richard, could you give attached patch a try, pls.
Also check that platform suspend mode is used. AFAIK this isn't called
at all in suspend mode.

Thanks,

    Thomas

-----------------------

Enable GPEs before calling _WAK on resume

Signed-off-by: Thomas Renninger <trenn@suse.de>

---
 drivers/acpi/hardware/hwsleep.c |   31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Index: linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.22.1.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
@@ -562,6 +562,23 @@ acpi_status acpi_leave_sleep_state(u8 sl
 	arg_list.pointer = &arg;
 	arg.type = ACPI_TYPE_INTEGER;
 
+	/*
+	 * GPEs must be enabled before _WAK is called as GPEs
+	 * might get fired there
+	 *
+	 * Restore the GPEs:
+	 * 1) Disable/Clear all GPEs
+	 * 2) Enable all runtime GPEs
+	 */
+	status = acpi_hw_disable_all_gpes();
+	if (ACPI_FAILURE(status)) {
+		return_ACPI_STATUS(status);
+	}
+	status = acpi_hw_enable_all_runtime_gpes();
+	if (ACPI_FAILURE(status)) {
+		return_ACPI_STATUS(status);
+	}
+
 	/* Ignore any errors from these methods */
 
 	arg.integer.value = ACPI_SST_WAKING;
@@ -582,22 +599,8 @@ acpi_status acpi_leave_sleep_state(u8 sl
 	}
 	/* TBD: _WAK "sometimes" returns stuff - do we want to look at it? */
 
-	/*
-	 * Restore the GPEs:
-	 * 1) Disable/Clear all GPEs
-	 * 2) Enable all runtime GPEs
-	 */
-	status = acpi_hw_disable_all_gpes();
-	if (ACPI_FAILURE(status)) {
-		return_ACPI_STATUS(status);
-	}
 	acpi_gbl_system_awake_and_running = TRUE;
 
-	status = acpi_hw_enable_all_runtime_gpes();
-	if (ACPI_FAILURE(status)) {
-		return_ACPI_STATUS(status);
-	}
-
 	/* Enable power button */
 
 	(void)

Re: [patch] Refresh lid state on resume

[Henrique de Moraes Holschuh]

On Tue, 31 Jul 2007, Rafael J. Wysocki wrote:
> > I guess it's up to the BIOS, and I don't think this refresh hurts any
> > machines that implement a notify on resume, and fixes a fair few
> > machines that don't.
> 
> AFAICS, the notify doesn't seem to work very well on some machines.

I have noticed thinkpad-acpi gets notifications during *early resume*
sometimes, and certainly *well* before its own resume handlers are called.

That might be part of the problem.

> Are there any downsides of the $subject patch?

None that I know.  I am doing something like that in thinkpad-acpi for
input-layer switches, and it helps a great deal.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch] Refresh lid state on resume

[Richard Hughes]

On Tue, 2007-07-31 at 12:42 +0200, Thomas Renninger wrote:
> Richard, could you give attached patch a try, pls.
> Also check that platform suspend mode is used. AFAIK this isn't called
> at all in suspend mode.

The attached patch works for me with a git kernel on my X60. I'm not in
the office today so I can't test it on other machines.

The suspend was called using echo mem > /sys/power/state

Thanks,

Richard.

Re: [patch] Refresh lid state on resume

[Rafael J. Wysocki]

On Tuesday, 31 July 2007 12:42, Thomas Renninger wrote:
> On Tue, 2007-07-31 at 10:07 +0100, Richard Hughes wrote:
> > On Tue, 2007-07-31 at 11:08 +0200, Rafael J. Wysocki wrote:
> > > On Monday, 30 July 2007 17:33, Richard Hughes wrote:
> > > > On Mon, 2007-07-30 at 17:21 +0200, Thomas Renninger wrote:
> > > > > On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:
> > > > > > On resume we need to refresh the lid status as we will not get an event if
> > > > > > the lid opening was what triggered the suspend.
> > > > > > This manifests itself in users never getting a "lid open" event when a
> > > > > > suspend happens because of lid close on hardware that supports wake on
> > > > > > lid open. This makes userspace gets very confused indeed.
> > > > > > Patch inline (and also attached) forces a check of the lid status in the
> > > > > > resume handler.
> > > > > Is this a general problem on all machines?
> > > > 
> > > > I've only seen myself it on new ThinkPads such as the T61 and X60,
> > > > although I've been getting a few bug reports about other IBM laptops.
> > > > 
> > > > > Or does this only happen if "shutdown" suspend mode is used?
> > > > 
> > > > No, I don't believe so.
> > > > 
> > > > > I could imagine a lot machines let it up to OS to check for LID state
> > > > > change, then this one should be added.
> > > > 
> > > > I guess it's up to the BIOS, and I don't think this refresh hurts any
> > > > machines that implement a notify on resume, and fixes a fair few
> > > > machines that don't.
> > > 
> > > AFAICS, the notify doesn't seem to work very well on some machines.
> > 
> > Agree.
> > 
> > > Are there any downsides of the $subject patch?
> > 
> > Not that I've found. I've been testing it on ~6 IBM and non-IBM machines
> > with no bad effects so far.
> 
> I just checked a X60 DSDT (couldn't check the SSDTs, but I doubt there
> is anything related):
> There are two Notify(\_SB.LID,0x80), both are in GPE handlers.
> AFAIK there should be one in the _WAK function.

Well, we only enable GPEs after calling _WAK, so this one won't trigger.

Perhaps we should change the code ordering in acpi_leave_sleep_state() to
enable GPEs before executing _WAK?

> Maybe they try to raise the GPE after wakeup in _WAK by something like
> this:
> \VSLD (\_SB.LID._LID ())
> ....
> Method (VSLD, 1, NotSerialized)
>     {
>         SMI (0x01, 0x07, Arg0, 0x00, 0x00)
>     }
> :)
> 
> 
> Related ACPI Spec parts:
> 
> 6.3 Device Insertion, Removal, and Status Objects:
> The Notify command can also be used from the _WAK control method (for
> more information about _WAK, see section 7.3.7 “\_WAK (System Wake)”) to
> indicate device changes that may have occurred while the computer was
> sleeping. For more information about the Notify command,
> see section 5.6.3 “Device Object Notification.”.”
> 
> The X60 is definitely not doing this.
> 
> The transition from Working to Sleep state is described very detailed,
> but I couldn't find (just overseen?) a detailed description about the
> transition from Sleep State to working state.
> In detail I searched for whether first the GPEs should get enabled and
> then _WAK is called or the other way around (the latter is currently
> implemented).
> Maybe enabling GPEs before calling _WAK will also fix this

Well, my thought above. :-)

> (and is the way it should be done or at least the way M$ is doing it?).

I don't know ...

> Richard, could you give attached patch a try, pls.
> Also check that platform suspend mode is used. AFAIK this isn't called
> at all in suspend mode.
> 
> Thanks,
> 
>     Thomas
> 
> -----------------------
> 
> Enable GPEs before calling _WAK on resume
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> 
> ---
>  drivers/acpi/hardware/hwsleep.c |   31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> Index: linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
> ===================================================================
> --- linux-2.6.22.1.orig/drivers/acpi/hardware/hwsleep.c
> +++ linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
> @@ -562,6 +562,23 @@ acpi_status acpi_leave_sleep_state(u8 sl
>  	arg_list.pointer = &arg;
>  	arg.type = ACPI_TYPE_INTEGER;
>  
> +	/*
> +	 * GPEs must be enabled before _WAK is called as GPEs
> +	 * might get fired there
> +	 *
> +	 * Restore the GPEs:
> +	 * 1) Disable/Clear all GPEs
> +	 * 2) Enable all runtime GPEs
> +	 */
> +	status = acpi_hw_disable_all_gpes();
> +	if (ACPI_FAILURE(status)) {
> +		return_ACPI_STATUS(status);
> +	}
> +	status = acpi_hw_enable_all_runtime_gpes();
> +	if (ACPI_FAILURE(status)) {
> +		return_ACPI_STATUS(status);
> +	}
> +

I wouldn't move that before _BFS, just in case someone actually implements it.

Enabling GPEs just prior to calling _WAK should be safe, IMO.

>  	/* Ignore any errors from these methods */
>  
>  	arg.integer.value = ACPI_SST_WAKING;
> @@ -582,22 +599,8 @@ acpi_status acpi_leave_sleep_state(u8 sl
>  	}
>  	/* TBD: _WAK "sometimes" returns stuff - do we want to look at it? */
>  
> -	/*
> -	 * Restore the GPEs:
> -	 * 1) Disable/Clear all GPEs
> -	 * 2) Enable all runtime GPEs
> -	 */
> -	status = acpi_hw_disable_all_gpes();
> -	if (ACPI_FAILURE(status)) {
> -		return_ACPI_STATUS(status);
> -	}
>  	acpi_gbl_system_awake_and_running = TRUE;
>  
> -	status = acpi_hw_enable_all_runtime_gpes();
> -	if (ACPI_FAILURE(status)) {
> -		return_ACPI_STATUS(status);
> -	}
> -
>  	/* Enable power button */
>  
>  	(void)

Greetings,
Rafael


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

[PATCH] Enable GPEs before _WAK is called

[Thomas Renninger]

On Tue, 2007-07-31 at 15:42 +0200, Rafael J. Wysocki wrote:
> On Tuesday, 31 July 2007 12:42, Thomas Renninger wrote:
> > On Tue, 2007-07-31 at 10:07 +0100, Richard Hughes wrote:
> > > On Tue, 2007-07-31 at 11:08 +0200, Rafael J. Wysocki wrote:
> > > > On Monday, 30 July 2007 17:33, Richard Hughes wrote:
> > > > > On Mon, 2007-07-30 at 17:21 +0200, Thomas Renninger wrote:
> > > > > > On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:
> > > > > > > On resume we need to refresh the lid status as we will not get an event if
> > > > > > > the lid opening was what triggered the suspend.
> > > > > > > This manifests itself in users never getting a "lid open" event when a
> > > > > > > suspend happens because of lid close on hardware that supports wake on
> > > > > > > lid open. This makes userspace gets very confused indeed.
> > > > > > > Patch inline (and also attached) forces a check of the lid status in the
> > > > > > > resume handler.
> > > > > > Is this a general problem on all machines?
> > > > > 
> > > > > I've only seen myself it on new ThinkPads such as the T61 and X60,
> > > > > although I've been getting a few bug reports about other IBM laptops.
> > > > > 
> > > > > > Or does this only happen if "shutdown" suspend mode is used?
> > > > > 
> > > > > No, I don't believe so.
> > > > > 
> > > > > > I could imagine a lot machines let it up to OS to check for LID state
> > > > > > change, then this one should be added.
> > > > > 
> > > > > I guess it's up to the BIOS, and I don't think this refresh hurts any
> > > > > machines that implement a notify on resume, and fixes a fair few
> > > > > machines that don't.
> > > > 
> > > > AFAICS, the notify doesn't seem to work very well on some machines.
> > > 
> > > Agree.
> > > 
> > > > Are there any downsides of the $subject patch?
> > > 
> > > Not that I've found. I've been testing it on ~6 IBM and non-IBM machines
> > > with no bad effects so far.
> > 
> > I just checked a X60 DSDT (couldn't check the SSDTs, but I doubt there
> > is anything related):
> > There are two Notify(\_SB.LID,0x80), both are in GPE handlers.
> > AFAIK there should be one in the _WAK function.
> 
> Well, we only enable GPEs after calling _WAK, so this one won't trigger.
> 
> Perhaps we should change the code ordering in acpi_leave_sleep_state() to
> enable GPEs before executing _WAK?
> 
> > Maybe they try to raise the GPE after wakeup in _WAK by something like
> > this:
> > \VSLD (\_SB.LID._LID ())
> > ....
> > Method (VSLD, 1, NotSerialized)
> >     {
> >         SMI (0x01, 0x07, Arg0, 0x00, 0x00)
> >     }
> > :)
> > 
> > 
> > Related ACPI Spec parts:
> > 
> > 6.3 Device Insertion, Removal, and Status Objects:
> > The Notify command can also be used from the _WAK control method (for
> > more information about _WAK, see section 7.3.7 “\_WAK (System Wake)”) to
> > indicate device changes that may have occurred while the computer was
> > sleeping. For more information about the Notify command,
> > see section 5.6.3 “Device Object Notification.”.”
> > 
> > The X60 is definitely not doing this.
> > 
> > The transition from Working to Sleep state is described very detailed,
> > but I couldn't find (just overseen?) a detailed description about the
> > transition from Sleep State to working state.
> > In detail I searched for whether first the GPEs should get enabled and
> > then _WAK is called or the other way around (the latter is currently
> > implemented).
> > Maybe enabling GPEs before calling _WAK will also fix this
> 
> Well, my thought above. :-)
> 
> > (and is the way it should be done or at least the way M$ is doing it?).
> 
> I don't know ...
> 
> > Richard, could you give attached patch a try, pls.
> > Also check that platform suspend mode is used. AFAIK this isn't called
> > at all in suspend mode.
> > 
> > Thanks,
> > 
> >     Thomas
> > 
> > -----------------------
> > 
> > Enable GPEs before calling _WAK on resume
> > 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > 
> > ---
> >  drivers/acpi/hardware/hwsleep.c |   31 +++++++++++++++++--------------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> > 
> > Index: linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
> > ===================================================================
> > --- linux-2.6.22.1.orig/drivers/acpi/hardware/hwsleep.c
> > +++ linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
> > @@ -562,6 +562,23 @@ acpi_status acpi_leave_sleep_state(u8 sl
> >  	arg_list.pointer = &arg;
> >  	arg.type = ACPI_TYPE_INTEGER;
> >  
> > +	/*
> > +	 * GPEs must be enabled before _WAK is called as GPEs
> > +	 * might get fired there
> > +	 *
> > +	 * Restore the GPEs:
> > +	 * 1) Disable/Clear all GPEs
> > +	 * 2) Enable all runtime GPEs
> > +	 */
> > +	status = acpi_hw_disable_all_gpes();
> > +	if (ACPI_FAILURE(status)) {
> > +		return_ACPI_STATUS(status);
> > +	}
> > +	status = acpi_hw_enable_all_runtime_gpes();
> > +	if (ACPI_FAILURE(status)) {
> > +		return_ACPI_STATUS(status);
> > +	}
> > +
> 
> I wouldn't move that before _BFS, just in case someone actually implements it.
Yes you are right, thanks.
(ACPI spec):
-----------
OSPM will execute the _BFS control method before performing any other
physical I/O or enabling any interrupt servicing upon returning
from a sleeping state.
-----------

Now it makes sense to have _BFS and _WAK, before it had not made a
difference from BIOS programmer point of view to use _BFS or _WAK.

With some luck this fixes some other things, I remember a weird bug on
(X60?) thinkpad:
If you suspend to RAM you can wakeup with the blue FN key, after doing a
suspend to disk and then doing a suspend to RAM the blue FN key does not
wake the machine anymore from STR :)

Attached an updated patch (Rafael, I added your Acked from comments
above. I just moved GPE enabling between _BFS and _WAK as you
suggested, pls scream if you still find something bad).

Len, can you commit this one, pls.

Thanks,

    Thomas

------------


Enable GPEs before calling _WAK on resume

It seems it's required to enable GPEs before _WAK.
E.g. X60 triggers a LID related GPE instead of doing a Notify in WAK.
Now the GPE reaches the kernel and the Notify for LID status
change gets thrown from there.

Signed-off-by: Thomas Renninger <trenn@suse.de>
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

---
 drivers/acpi/hardware/hwsleep.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Index: linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.22.1.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
@@ -576,13 +576,10 @@ acpi_status acpi_leave_sleep_state(u8 sl
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
 	}
 
-	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));
-	}
-	/* TBD: _WAK "sometimes" returns stuff - do we want to look at it? */
-
 	/*
+	 * GPEs must be enabled before _WAK is called as GPEs
+	 * might get fired there
+	 *
 	 * Restore the GPEs:
 	 * 1) Disable/Clear all GPEs
 	 * 2) Enable all runtime GPEs
@@ -591,13 +588,19 @@ acpi_status acpi_leave_sleep_state(u8 sl
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
 	}
-	acpi_gbl_system_awake_and_running = TRUE;
-
 	status = acpi_hw_enable_all_runtime_gpes();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
 	}
 
+	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));
+	}
+	/* TBD: _WAK "sometimes" returns stuff - do we want to look at it? */
+
+	acpi_gbl_system_awake_and_running = TRUE;
+
 	/* Enable power button */
 
 	(void)

Re: [PATCH] Enable GPEs before _WAK is called

[Rafael J. Wysocki]

On Tuesday, 31 July 2007 16:26, Thomas Renninger wrote:
> On Tue, 2007-07-31 at 15:42 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, 31 July 2007 12:42, Thomas Renninger wrote:
> > > On Tue, 2007-07-31 at 10:07 +0100, Richard Hughes wrote:
> > > > On Tue, 2007-07-31 at 11:08 +0200, Rafael J. Wysocki wrote:
> > > > > On Monday, 30 July 2007 17:33, Richard Hughes wrote:
> > > > > > On Mon, 2007-07-30 at 17:21 +0200, Thomas Renninger wrote:
> > > > > > > On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:
[--snip--]
> > I wouldn't move that before _BFS, just in case someone actually implements it.
> Yes you are right, thanks.
> (ACPI spec):
> -----------
> OSPM will execute the _BFS control method before performing any other
> physical I/O or enabling any interrupt servicing upon returning
> from a sleeping state.
> -----------
> 
> Now it makes sense to have _BFS and _WAK, before it had not made a
> difference from BIOS programmer point of view to use _BFS or _WAK.
> 
> With some luck this fixes some other things, I remember a weird bug on
> (X60?) thinkpad:
> If you suspend to RAM you can wakeup with the blue FN key, after doing a
> suspend to disk and then doing a suspend to RAM the blue FN key does not
> wake the machine anymore from STR :)
> 
> Attached an updated patch (Rafael, I added your Acked from comments
> above. I just moved GPE enabling between _BFS and _WAK as you
> suggested, pls scream if you still find something bad).

I think the patch (below) is correct.

Greetings,
Rafael


> ------------
> 
> 
> Enable GPEs before calling _WAK on resume
> 
> It seems it's required to enable GPEs before _WAK.
> E.g. X60 triggers a LID related GPE instead of doing a Notify in WAK.
> Now the GPE reaches the kernel and the Notify for LID status
> change gets thrown from there.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> ---
>  drivers/acpi/hardware/hwsleep.c |   19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
> ===================================================================
> --- linux-2.6.22.1.orig/drivers/acpi/hardware/hwsleep.c
> +++ linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
> @@ -576,13 +576,10 @@ acpi_status acpi_leave_sleep_state(u8 sl
>  		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
>  	}
>  
> -	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
> -	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> -		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));
> -	}
> -	/* TBD: _WAK "sometimes" returns stuff - do we want to look at it? */
> -
>  	/*
> +	 * GPEs must be enabled before _WAK is called as GPEs
> +	 * might get fired there
> +	 *
>  	 * Restore the GPEs:
>  	 * 1) Disable/Clear all GPEs
>  	 * 2) Enable all runtime GPEs
> @@ -591,13 +588,19 @@ acpi_status acpi_leave_sleep_state(u8 sl
>  	if (ACPI_FAILURE(status)) {
>  		return_ACPI_STATUS(status);
>  	}
> -	acpi_gbl_system_awake_and_running = TRUE;
> -
>  	status = acpi_hw_enable_all_runtime_gpes();
>  	if (ACPI_FAILURE(status)) {
>  		return_ACPI_STATUS(status);
>  	}
>  
> +	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
> +	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> +		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));
> +	}
> +	/* TBD: _WAK "sometimes" returns stuff - do we want to look at it? */
> +
> +	acpi_gbl_system_awake_and_running = TRUE;
> +
>  	/* Enable power button */
>  
>  	(void)
> 
> 
> -
> 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
> 
> 

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