Fix mutex references and the global lock

Cargo.toml

@@ -24,7 +24,10 @@ byteorder = { version = "1.5.0", default-features = false }
 
 [dev-dependencies]
 aml_test_tools = { path = "tools/aml_test_tools" }
+lock_api = "0.4.14"
+parking_lot = "0.12.5"
 pretty_env_logger = "0.5.0"
+serial_test = "4.0.1"
 
 [features]
 default = ["alloc", "aml"]

src/aml/mod.rs

@@ -109,6 +109,11 @@ where
     region_handlers: Spinlock<BTreeMap<RegionSpace, Box<dyn RegionHandler>>>,
 
     global_lock_mutex: Handle,
+
+    /// How many times has the thread that owns the global lock acquired it? Zero should correspond
+    /// to [`global_lock_mutex`] being unlocked.
+    global_lock_acquisition_count: AtomicU64,
+
     registers: Arc<FixedRegisters<H>>,
     facs: Option<PhysicalMapping<H, Facs>>,
 }
@@ -148,6 +153,7 @@ where
             integer_size: IntegerSize::from_revision(dsdt_revision),
             region_handlers: Spinlock::new(BTreeMap::new()),
             global_lock_mutex,
+            global_lock_acquisition_count: AtomicU64::new(0),
             registers,
             facs,
         }
@@ -327,25 +333,34 @@ where
     }
 
     pub fn acquire_global_lock(&self, timeout: u16) -> Result<(), AmlError> {
+        // This lock is released by `release_global_lock`. Acquire unconditionally since AML
+        // mutexes are reentrant, and the [`Handler`] might want to take action on each acquisition.
         self.handler.acquire(self.global_lock_mutex, timeout)?;
 
-        // Now we've acquired the AML-side mutex, acquire the hardware side
-        // TODO: count the number of times we have to go round this loop / enforce a timeout?
-        loop {
-            if self.try_do_acquire_firmware_lock() {
-                break Ok(());
-            } else {
-                /*
-                 * The lock is owned by the firmware. We have set the pending bit - we now need to
-                 * wait for the firmware to signal it has released the lock.
-                 *
-                 * TODO: this should wait for an interrupt from the firmware. That needs more infra
-                 * so for now let's just spin round and try and acquire it again...
-                 */
-                self.handler.release(self.global_lock_mutex);
-                continue;
+        let last = self.global_lock_acquisition_count.fetch_add(1, Ordering::Relaxed);
+
+        // The firmware lock does not have an acquisition counter, so don't try and acquire a
+        // firmware lock we already own.
+        if last == 0 {
+            // Now we've acquired the AML-side mutex, acquire the hardware side
+            // TODO: count the number of times we have to go round this loop / enforce a timeout?
+            loop {
+                if self.try_do_acquire_firmware_lock() {
+                    break;
+                } else {
+                    /*
+                     * The lock is owned by the firmware. We have set the pending bit - we now need
+                     * to wait for the firmware to signal it has released the lock.
+                     *
+                     * TODO: this should wait for an interrupt from the firmware. That needs more
+                     * infra so for now let's just spin round and try and acquire it again...
+                     */
+                    continue;
+                }
             }
         }
+
+        Ok(())
     }
 
     /// Attempt to acquire the firmware lock, setting the owned bit if the lock is free. If the
@@ -379,10 +394,22 @@ where
     }
 
     pub fn release_global_lock(&self) -> Result<(), AmlError> {
-        let is_pending = self.do_release_firmware_lock();
-        if is_pending {
-            self.registers.pm1_control_registers.set_bit(Pm1ControlBit::GlobalLockRelease, true).unwrap();
+        let c = self.global_lock_acquisition_count.fetch_sub(1, Ordering::Relaxed);
+
+        // Only release the firmware lock if that was the last global lock acquisition that this
+        // thread was holding.
+        //
+        // There is no risk of `c` changing - this thread still holds the global lock mutex, so only
+        // this thread can change `self.global_lock_acquisition_count`.
+        if c == 1 {
+            let is_pending = self.do_release_firmware_lock();
+            if is_pending {
+                self.registers.pm1_control_registers.set_bit(Pm1ControlBit::GlobalLockRelease, true).unwrap();
+            }
         }
+
+        // This mutex was locked in `acquire_global_mutex`.
+        self.handler.release(self.global_lock_mutex);
         Ok(())
     }
 
@@ -884,7 +911,8 @@ where
                     }
                     Opcode::Acquire => {
                         extract_args!(op => [Argument::Object(mutex)]);
-                        let Object::Mutex { mutex, sync_level: _ } = **mutex else {
+                        let mutex = mutex.clone().unwrap_reference();
+                        let Object::Mutex { mutex, sync_level: _ } = *mutex else {
                             Err(AmlError::InvalidOperationOnObject { op: Operation::Acquire, typ: mutex.typ() })?
                         };
                         let timeout = context.next_u16()?;
@@ -900,7 +928,8 @@ where
                     }
                     Opcode::Release => {
                         extract_args!(op => [Argument::Object(mutex)]);
-                        let Object::Mutex { mutex, sync_level: _ } = **mutex else {
+                        let mutex = mutex.clone().unwrap_reference();
+                        let Object::Mutex { mutex, sync_level: _ } = *mutex else {
                             Err(AmlError::InvalidOperationOnObject { op: Operation::Release, typ: mutex.typ() })?
                         };
 

src/lib.rs

@@ -507,8 +507,23 @@ pub trait Handler: Clone {
     ///
     /// AML mutexes are **reentrant** - that is, a thread may acquire the same mutex more than once
     /// without causing a deadlock.
+    ///
+    /// Note: The ACPI specification says: "A Mutex must be totally released before an invocation
+    /// completes." - this crate does not enforce that, and it also does not forcibly release
+    /// mutexes when the top-level called method exits.
     #[cfg(feature = "aml")]
     fn acquire(&self, mutex: Handle, timeout: u16) -> Result<(), aml::AmlError>;
+
+    /// Release the mutex referred to by the given handle.
+    ///
+    /// AML mutexes are reentrant - this function should release only one acquisition of the mutex.
+    ///
+    /// In correctly written AML code, the number of Acquire statements (which cause the interpreter
+    /// to call [`acquire`](Self::acquire) must equal the number of Release statements (which causes
+    /// this function to be called).
+    ///
+    /// According to the ACPI spec: "It is fatal to release ownership on a Mutex unless it is
+    /// currently owned" - this crate does not specify what a Handler should do in this case.
     #[cfg(feature = "aml")]
     fn release(&self, mutex: Handle);