Re: [patch] Refresh lid state on resume

[Thomas Renninger <trenn@suse.de>]

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)