Refresh lid state on resume

PATCH In review Richard Hughes 4 participants 11 messages Jul 17, 2007 – Jul 31, 2007

Subsystems (2)

ACPI HID

lore

Share & tools

lore

Richard Hughes

author

2007-07-17 11:15 UTC #0 lore raw

PATCH 1/1 Refresh lid state on resume

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 <[email protected]>

---

drivers/acpi/button.c +25 -0

index cb4110b..fd3473b 100644
@@ -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;

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 <[email protected]>

---

drivers/acpi/button.c +25 -0

index cb4110b..fd3473b 100644
@@ -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;

Thomas Renninger

2007-07-30 15:21 UTC #1 lore raw

Quoted 7 lines · Richard Hughes

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

Quoted 53 lines

Signed-off-by Richard Hughes <[email protected]>

---

drivers/acpi/button.c +25 -0

index cb4110b..fd3473b 100644
@@ -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;

Richard Hughes

author

2007-07-30 15:33 UTC #2 lore raw

Quoted 9 lines · Thomas Renninger

Quoted 7 lines · Richard Hughes
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.

Quoted 1 line

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

No, I don't believe so.

Quoted 2 lines

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.

Rafael J. Wysocki

2007-07-31 08:59 UTC #3 lore raw

Quoted 16 lines · Richard Hughes

Quoted 9 lines · Thomas Renninger
Quoted 7 lines · Richard Hughes
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.
Quoted 1 line
Or does this only happen if "shutdown" suspend mode is used?

Quoted 8 lines

No, I don't believe so.
Quoted 2 lines
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

Signature · 1 line

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

Richard Hughes

author

2007-07-31 09:07 UTC #4 lore raw

Quoted 19 lines · Rafael J. Wysocki

Quoted 18 lines · Richard Hughes
Quoted 9 lines · Thomas Renninger
Quoted 7 lines · Richard Hughes
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.
Quoted 1 line
Or does this only happen if "shutdown" suspend mode is used?
No, I don't believe so.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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.

Quoted 1 line

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.

Thomas Renninger

2007-07-31 10:42 UTC #5 lore raw

Quoted 27 lines · Richard Hughes

Quoted 26 lines · Rafael J. Wysocki
Quoted 24 lines · Richard Hughes
Quoted 9 lines · Thomas Renninger
Quoted 7 lines · Richard Hughes
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.
Quoted 1 line
Or does this only happen if "shutdown" suspend mode is used?
No, I don't believe so.
Quoted 2 lines
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.

Quoted 8 lines

Quoted 1 line
AFAICS, the notify doesn't seem to work very well on some machines.
Agree.
Quoted 1 line
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 <[email protected]>

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

drivers/acpi/hardware/hwsleep.c +17 ~~-14~~

Index: 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)

Richard Hughes

author

2007-07-31 13:18 UTC #6 lore raw

Quoted 3 lines · Thomas Renninger

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.

Rafael J. Wysocki

2007-07-31 13:42 UTC #7 lore raw

In reply to Thomas Renninger

Quoted 34 lines · Thomas Renninger

Quoted 33 lines · Richard Hughes
Quoted 19 lines · Rafael J. Wysocki
Quoted 18 lines · Richard Hughes
Quoted 9 lines · Thomas Renninger
Quoted 7 lines · Richard Hughes
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.
Quoted 1 line
Or does this only happen if "shutdown" suspend mode is used?
No, I don't believe so.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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.
Quoted 1 line
Are there any downsides of the $subject patch?

Quoted 7 lines

Quoted 2 lines
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?

Quoted 22 lines

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.

Quoted 7 lines

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. :-)

Quoted 1 line

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

I don't know ...

Quoted 33 lines

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 <[email protected]>
---
 drivers/acpi/hardware/hwsleep.c |   31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)
Index: linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
===================================================================
drivers/acpi/hardware/hwsleep.c +7 -0
@@ -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

drivers/acpi/hardware/hwsleep.c acpi_status acpi_leave_sleep_state(u8 sl · line 581

Quoted 10 lines

drivers/acpi/hardware/hwsleep.c +10 -0

+	 */
+	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.

Quoted 16 lines

drivers/acpi/hardware/hwsleep.c +0 -9

 	/* 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);
-	}

drivers/acpi/hardware/hwsleep.c acpi_status acpi_leave_sleep_state(u8 sl · line 606

Quoted 10 lines

drivers/acpi/hardware/hwsleep.c +0 -5

 	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

Signature · 1 line

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

Thomas Renninger

2007-07-31 14:26 UTC #8 lore raw

PATCH 1/1 Enable GPEs before _WAK is called

Quoted 121 lines · Rafael J. Wysocki

Quoted 34 lines · Thomas Renninger
Quoted 33 lines · Richard Hughes
Quoted 19 lines · Rafael J. Wysocki
Quoted 18 lines · Richard Hughes
Quoted 9 lines · Thomas Renninger
Quoted 7 lines · Richard Hughes
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.
Quoted 1 line
Or does this only happen if "shutdown" suspend mode is used?
No, I don't believe so.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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.
Quoted 1 line
Are there any downsides of the $subject patch?
Quoted 7 lines
Quoted 2 lines
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?
Quoted 22 lines
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.
Quoted 7 lines
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. :-)
Quoted 1 line
(and is the way it should be done or at least the way M$ is doing it?).
I don't know ...
Quoted 37 lines
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 <[email protected]>
---
 drivers/acpi/hardware/hwsleep.c |   31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)
Index: linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
===================================================================
drivers/acpi/hardware/hwsleep.c +11 -0
@@ -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);

Quoted 8 lines

drivers/acpi/hardware/hwsleep.c acpi_status acpi_leave_sleep_state(u8 sl · line 581
Quoted 6 lines
drivers/acpi/hardware/hwsleep.c +6 -0
+	}
+	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 <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

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

drivers/acpi/hardware/hwsleep.c +11 -8

Index: 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)

Rafael J. Wysocki

2007-07-31 15:00 UTC #9 lore raw

Re: [PATCH] Enable GPEs before _WAK is called

Quoted 7 lines · Thomas Renninger

Quoted 6 lines · Rafael J. Wysocki
Quoted 5 lines · Thomas Renninger
Quoted 4 lines · Richard Hughes
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Richard Hughes
Quoted 1 line · Thomas Renninger
On Tue, 2007-07-17 at 12:15 +0100, Richard Hughes wrote:

[--snip--]

Quoted 18 lines

Quoted 1 line
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 :)

Quoted 3 lines

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

Quoted 68 lines

------------
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 <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/hardware/hwsleep.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)
Index: linux-2.6.22.1/drivers/acpi/hardware/hwsleep.c
===================================================================
drivers/acpi/hardware/hwsleep.c +11 -9
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

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

Henrique de Moraes Holschuh

2007-07-31 12:53 UTC #10 lore raw

In reply to Rafael J. Wysocki

Quoted 5 lines · Rafael J. Wysocki

Quoted 3 lines
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.

Quoted 1 line

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.

Signature · 4 lines

  "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