ACPICA: Do not hang at mutex forever if BIOS tries to release global lock

PATCH In review Thomas Renninger 3 participants 4 messages Oct 8, 2010 – Oct 18, 2010

Subsystems (2)

ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Thomas Renninger

author

2010-10-08 10:40 UTC #0 lore raw

PATCH 1/1 ACPICA: Do not hang at mutex forever if BIOS tries to release global lock

Hi,

find the code against the Linux kernel pasted at the end of the mail for review.
Find the code adjusted for ACPICA inclusion in the attachement.

Bob: If you are ok with the patch, it would be great if you could push it. Be aware that I did not compile tested it in ACPICA environment.

Thanks,

Thomas

From: Stuart Hayes <stuart_hayes(a)dell.com>

ACPICA: Do not hang at mutex forever if BIOS tries to release global lock

and OS did not request the lock (means the pending bit was not set). Which in fact is a BIOS/HW bug (compare with ACPI Spec 3.0b):

5.2.10.1 Global Lock:
..
This signal only occurs when the other environment attempted to acquire ownership while the lock was owned.
..
or
4.7.3.1.1 PM1 Status Registers
GBL_STS:
..
This bit is set in response to the BIOS releasing control of the Global Lock and having seen the pending bit set.
..

There seem to be chipsets which can get this wrong (especially after a kexec boot on Linux).

Also one could theoretically run into this if a new kernel got booted without proper reboot and BIOS re-initialization (like kexec) while:
1) The OS hold the global lock
2) The BIOS hold the global lock
This patch would handle condition 1.

To make sure condition 2. cannot happen, it should get stated in the ACPI spec that the global lock needs to be zeroed by BIOS everytime the OS tries to (re-)enable ACPI mode.

Modifications by trenn:
- Add a message -> This should not happen and if it happens it should not be ignored/handled silently
- Adjusted from Linux kernel to acpica sources

Signed-off-by Thomas Renninger <trenn(a)suse.de>

drivers/acpi/acpica/acglobal.h +1 -0

diff -purN '--exclude=*orig' linux-2.6.36-rc6_orig/drivers/acpi/acpica/acglobal.h linux-2.6.36-rc6/drivers/acpi/acpica/acglobal.h
@@ -213,6 +213,7 @@ ACPI_EXTERN acpi_semaphore acpi_gbl_glob
 ACPI_EXTERN u16 acpi_gbl_global_lock_handle;
 ACPI_EXTERN u8 acpi_gbl_global_lock_acquired;
 ACPI_EXTERN u8 acpi_gbl_global_lock_present;
+ACPI_EXTERN u32 acpi_gbl_global_lock_pending;
 
 /*
  * Spinlocks are used for interfaces that can be possibly called at

drivers/acpi/acpica/evmisc.c +14 -0

diff -purN '--exclude=*orig' linux-2.6.36-rc6_orig/drivers/acpi/acpica/evmisc.c linux-2.6.36-rc6/drivers/acpi/acpica/evmisc.c
@@ -297,6 +297,17 @@ static u32 acpi_ev_global_lock_handler(v
 {
 	u8 acquired = FALSE;
 
+	/* This must not happen: BIOS must only release the lock if OS has set
+	 * it pending. Such inconsistancies could happen if the global lock bits
+	 * survived through a kexec boot. */
+
+	if (!acpi_gbl_global_lock_pending) {
+		u8 pending;
+		/* Clear lock bits. BIOS released lock and OS isn't waiting. */
+		ACPI_RELEASE_GLOBAL_LOCK(acpi_gbl_FACS, pending);
+		return (ACPI_INTERRUPT_HANDLED);
+	}
+
 	/*
 	 * Attempt to get the lock.
 	 *

@@ -309,6 +320,7 @@ static u32 acpi_ev_global_lock_handler(v
 		/* Got the lock, now wake all threads waiting for it */
 
 		acpi_gbl_global_lock_acquired = TRUE;
+		acpi_gbl_global_lock_pending--;
 		/* Send a unit to the semaphore */
 
 		if (ACPI_FAILURE

@@ -479,6 +491,8 @@ acpi_status acpi_ev_acquire_global_lock(
 
 		acpi_gbl_global_lock_acquired = TRUE;
 		return_ACPI_STATUS(AE_OK);
+	} else {
+		acpi_gbl_global_lock_pending++;
 	}
 
 	/*

drivers/acpi/acpica/utglobal.c +1 -0

diff -purN '--exclude=*orig' linux-2.6.36-rc6_orig/drivers/acpi/acpica/utglobal.c linux-2.6.36-rc6/drivers/acpi/acpica/utglobal.c
@@ -782,6 +782,7 @@ acpi_status acpi_ut_init_globals(void)
 	acpi_gbl_global_lock_acquired = FALSE;
 	acpi_gbl_global_lock_handle = 0;
 	acpi_gbl_global_lock_present = FALSE;
+	acpi_gbl_global_lock_pending = 0;
 
 	/* Miscellaneous variables */

From: Stuart Hayes <[email protected]>

ACPICA: Do not hang at mutex forever if BIOS tries to release global lock

and OS did not request the lock (means the pending bit was not set). Which in fact is a BIOS/HW bug (compare with ACPI Spec 3.0b):

There seem to be chipsets which can get this wrong (especially after a kexec boot on Linux).

To make sure condition 2. cannot happen, it should get stated in the ACPI spec that the global lock needs to be zeroed by BIOS everytime the OS tries to (re-)enable ACPI mode.

Modifications by trenn:
- Add a message -> This should not happen and if it happens it should not be ignored/handled silently
- Adjusted from Linux kernel to acpica sources

Signed-off-by Thomas Renninger <[email protected]>

---
 source/components/events/evmisc.c      |   22 ++++++++++++++++++++--
 source/components/utilities/utglobal.c |    1 +
 2 files changed, 21 insertions(+), 2 deletions(-)

source/components/events/evmisc.c +20 -2

index b0e1cac..8c0f9b1 100644
@@ -399,6 +399,23 @@ AcpiEvGlobalLockHandler (
 
 
     /*
+     * This must not happen: BIOS must only release the lock if OS has set
+     * it pending. Such inconsistancies could happen if another kernel is booted
+     * without a proper reboot and BIOS re-initialization and the global lock bits
+     * did not get cleared. But in general it's a BIOS bug if a gbl lock release
+     * GPE is triggered and OS has not tried to acquire the lock.
+     */
+    
+    if (!AcpiGbl_GlobalLockPending) {
+	u8 pending;
+	/* Clear lock bits. BIOS released lock and OS isn't waiting. */
+	ACPI_RELEASE_GLOBAL_LOCK(acpi_gbl_FACS, pending)
+            ACPI_WARNING ((AE_INFO, "Global lock release GPE, even OS did not try"
+			   "to acquire the lock"));
+	return (ACPI_INTERRUPT_HANDLED);
+    }
+
+    /*
      * Attempt to get the lock.
      *
      * If we don't get it now, it will be marked pending and we will

@@ -410,7 +427,7 @@ AcpiEvGlobalLockHandler (
         /* Got the lock, now wake the thread waiting for it */
 
         AcpiGbl_GlobalLockAcquired = TRUE;
-
+	AcpiGbl_GlobalLockPending--;
         /* Send a unit to the semaphore */
 
         Status = AcpiOsSignalSemaphore (AcpiGbl_GlobalLockSemaphore, 1);

@@ -579,7 +596,8 @@ AcpiEvAcquireGlobalLock (
 
         AcpiGbl_GlobalLockAcquired = TRUE;
         return_ACPI_STATUS (AE_OK);
-    }
+    } else
+	AcpiGbl_GlobalLockPending++;
 
     /*
      * Did not get the lock. The pending bit was set above, and we must now

source/components/utilities/utglobal.c +1 -0

index a7e1baa..5f0f29f 100644
@@ -916,6 +916,7 @@ AcpiUtInitGlobals (
     AcpiGbl_GlobalLockAcquired          = FALSE;
     AcpiGbl_GlobalLockHandle            = 0;
     AcpiGbl_GlobalLockPresent           = FALSE;
+    AcpiGbl_GlobalLockPending           = 0;
 
     /* Miscellaneous variables */

Moore

2010-10-08 14:54 UTC #1 lore raw

We will take a look at this, and probably have a few more questions.

+ /* This must not happen: BIOS must only release the lock if OS has set
+ * it pending. Such inconsistancies could happen if the global lock bits
+ * survived through a kexec boot. */

We're going to need a non-OS-specific comment here.

Quoted 142 lines

-----Original Message-----
From: Thomas Renninger [mailto:trenn(a)suse.de]
Sent: Friday, October 08, 2010 3:40 AM
To: Moore, Robert; ACPI Devel Maling List
Cc: devel(a)acpica.org; stuart_hayes(a)dell.com
Subject: [PATCH] ACPICA: Do not hang at mutex forever if BIOS tries to release global lock
Hi,
find the code against the Linux kernel pasted at the end of the mail for review.
Find the code adjusted for ACPICA inclusion in the attachement.
Bob: If you are ok with the patch, it would be great if you could push it. Be aware that I did not compile tested it in ACPICA environment.
Thanks,
Thomas
From: Stuart Hayes <stuart_hayes(a)dell.com>
ACPICA: Do not hang at mutex forever if BIOS tries to release global lock
and OS did not request the lock (means the pending bit was not set). Which in fact is a BIOS/HW bug (compare with ACPI Spec 3.0b):
5.2.10.1 Global Lock:
..
This signal only occurs when the other environment attempted to acquire ownership while the lock was owned.
..
or
4.7.3.1.1 PM1 Status Registers
GBL_STS:
..
This bit is set in response to the BIOS releasing control of the Global Lock
and having seen the pending bit set.
..
There seem to be chipsets which can get this wrong (especially after a kexec
boot on Linux).
Also one could theoretically run into this if a new kernel got booted without
proper reboot and BIOS re-initialization (like kexec) while:
1) The OS hold the global lock
2) The BIOS hold the global lock
This patch would handle condition 1.
To make sure condition 2. cannot happen, it should get stated in the ACPI spec
that the global lock needs to be zeroed by BIOS everytime the OS tries to (re-)enable ACPI mode.
Modifications by trenn:
- Add a message -> This should not happen and if it happens it should not be ignored/handled silently
- Adjusted from Linux kernel to acpica sources
Signed-off-by Thomas Renninger <trenn(a)suse.de>
diff -purN '--exclude=*orig' linux-2.6.36- rc6_orig/drivers/acpi/acpica/acglobal.h linux-2.6.36- rc6/drivers/acpi/acpica/acglobal.h
--- linux-2.6.36-rc6_orig/drivers/acpi/acpica/acglobal.h 2010-06-02
09:16:14.000000000 -0500
+++ linux-2.6.36-rc6/drivers/acpi/acpica/acglobal.h 2010-06-02
09:24:44.000000000 -0500
drivers/acpi/acpica/acglobal.h +1 -0
@@ -213,6 +213,7 @@ ACPI_EXTERN acpi_semaphore acpi_gbl_glob
 ACPI_EXTERN u16 acpi_gbl_global_lock_handle;
 ACPI_EXTERN u8 acpi_gbl_global_lock_acquired;
 ACPI_EXTERN u8 acpi_gbl_global_lock_present;
+ACPI_EXTERN u32 acpi_gbl_global_lock_pending;
 
 /*
 * Spinlocks are used for interfaces that can be possibly called at
diff -purN '--exclude=*orig' linux-2.6.36- rc6_orig/drivers/acpi/acpica/evmisc.c linux-2.6.36- rc6/drivers/acpi/acpica/evmisc.c
--- linux-2.6.36-rc6_orig/drivers/acpi/acpica/evmisc.c 2010-06-02
09:16:11.000000000 -0500
+++ linux-2.6.36-rc6/drivers/acpi/acpica/evmisc.c 2010-06-02
09:24:45.000000000 -0500
drivers/acpi/acpica/evmisc.c +1 -0
@@ -297,6 +297,17 @@ static u32 acpi_ev_global_lock_handler(v
 {
 	u8 acquired = FALSE;
 
+	/* This must not happen: BIOS must only release the lock if OS has
set
(quoted diff) +1 -0
+	 * it pending. Such inconsistancies could happen if the global lock
bits
drivers/acpi/acpica/evmisc.c +9 -0
+	 * survived through a kexec boot. */
+
+	if (!acpi_gbl_global_lock_pending) {
+		u8 pending;
+		/* Clear lock bits. BIOS released lock and OS isn't waiting. */
+		ACPI_RELEASE_GLOBAL_LOCK(acpi_gbl_FACS, pending);
+		return (ACPI_INTERRUPT_HANDLED);
+	}
+
	/*
	 * Attempt to get the lock.
	 *
drivers/acpi/acpica/evmisc.c +3 -0
@@ -309,6 +320,7 @@ static u32 acpi_ev_global_lock_handler(v
 		/* Got the lock, now wake all threads waiting for it */
 
 		acpi_gbl_global_lock_acquired = TRUE;
+		acpi_gbl_global_lock_pending--;
 		/* Send a unit to the semaphore */
 
 		if (ACPI_FAILURE
@@ -479,6 +491,8 @@ acpi_status acpi_ev_acquire_global_lock(
 
 		acpi_gbl_global_lock_acquired = TRUE;
 		return_ACPI_STATUS(AE_OK);
+	} else {
+		acpi_gbl_global_lock_pending++;
 	}
 
 	/*
diff -purN '--exclude=*orig' linux-2.6.36- rc6_orig/drivers/acpi/acpica/utglobal.c linux-2.6.36- rc6/drivers/acpi/acpica/utglobal.c
--- linux-2.6.36-rc6_orig/drivers/acpi/acpica/utglobal.c 2010-06-02
09:16:14.000000000 -0500
+++ linux-2.6.36-rc6/drivers/acpi/acpica/utglobal.c 2010-06-02
09:24:45.000000000 -0500
drivers/acpi/acpica/utglobal.c +1 -0
@@ -782,6 +782,7 @@ acpi_status acpi_ut_init_globals(void)
 	acpi_gbl_global_lock_acquired = FALSE;
 	acpi_gbl_global_lock_handle = 0;
 	acpi_gbl_global_lock_present = FALSE;
+	acpi_gbl_global_lock_pending = 0;
 
 	/* Miscellaneous variables */
 

Len Brown

2010-10-16 02:25 UTC #2 lore raw

In reply to Thomas Renninger

Quoted 2 lines

There seem to be chipsets which can get this wrong (especially after a kexec boot on Linux).

As kexec is fundamentally unreliable by design, why should reliable software be modified for its beneift?

</recording>

-Len Brown, Intel Open Source Technology Center

Stuart_Hayes at Dell.com

2010-10-18 15:37 UTC #3 lore raw

Quoted 12 lines

Quoted 2 lines
There seem to be chipsets which can get this wrong (especially after
a kexec
Quoted 1 line
boot on Linux).
<recording>
As kexec is fundamentally unreliable by design, why should reliable software be modified for its beneift?
</recording>

The problem isn't just with kexec. The lock is acquired by acpi_ev_global_lock_handler() when a global lock release SCI occurs, even when the OS doesn't want it, which sets the "owned" bit on the first SCI and then the "pending" bit on the second SCI. This prevents the global lock from working correctly, even though we're only seeing the OS hang waiting for the semaphore because of this when a kexec boot is attempted.