Make mutexes reentrant by default (#3913)

* Make mutexes reentrant by default

* Remove redundant function

esp-hal/CHANGELOG.md

@@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - WDT now allows configuring longer timeouts (#3816)
 - `ADC2` now cannot be used simultaneously with `radio` on ESP32 (#3876)
 - Switched GPIO32 and GPIO33 ADC channel numbers (#3908, #3911)
+- Calling `Input::unlisten` in a GPIO interrupt handler no longer panics (#3913)
 
 ### Removed
 

esp-hal/src/sync.rs

@@ -321,8 +321,14 @@ impl<L: single_core::RawLock> GenericRawMutex<L> {
     ///
     /// Note that this function is not reentrant, calling it reentrantly will
     /// panic.
+    pub fn lock_non_reentrant<R>(&self, f: impl FnOnce() -> R) -> R {
+        let _token = LockGuard::new_non_reentrant(self);
+        f()
+    }
+
+    /// Runs the callback with this lock locked.
     pub fn lock<R>(&self, f: impl FnOnce() -> R) -> R {
-        let _token = LockGuard::new(self);
+        let _token = LockGuard::new_reentrant(self);
         f()
     }
 }
@@ -381,6 +387,11 @@ impl RawMutex {
     ///
     /// Note that this function is not reentrant, calling it reentrantly will
     /// panic.
+    pub fn lock_non_reentrant<R>(&self, f: impl FnOnce() -> R) -> R {
+        self.inner.lock_non_reentrant(f)
+    }
+
+    /// Runs the callback with this lock locked.
     pub fn lock<R>(&self, f: impl FnOnce() -> R) -> R {
         self.inner.lock(f)
     }
@@ -391,9 +402,7 @@ unsafe impl embassy_sync::blocking_mutex::raw::RawMutex for RawMutex {
     const INIT: Self = Self::new();
 
     fn lock<R>(&self, f: impl FnOnce() -> R) -> R {
-        // embassy_sync semantics allow reentrancy.
+        self.inner.lock(f)
-        let _token = LockGuard::new_reentrant(&self.inner);
-        f()
     }
 }
 
@@ -414,9 +423,6 @@ impl RawPriorityLimitedMutex {
     }
 
     /// Runs the callback with this lock locked.
-    ///
-    /// Note that this function is not reentrant, calling it reentrantly will
-    /// panic.
     pub fn lock<R>(&self, f: impl FnOnce() -> R) -> R {
         self.inner.lock(f)
     }
@@ -427,19 +433,10 @@ unsafe impl embassy_sync::blocking_mutex::raw::RawMutex for RawPriorityLimitedMu
     const INIT: Self = Self::new(Priority::max());
 
     fn lock<R>(&self, f: impl FnOnce() -> R) -> R {
-        // embassy_sync semantics allow reentrancy.
+        self.inner.lock(f)
-        let _token = LockGuard::new_reentrant(&self.inner);
-        f()
     }
 }
 
-// Prefer this over a critical-section as this allows you to have multiple
-// locks active at the same time rather than using the global mutex that is
-// critical-section.
-pub(crate) fn lock<T>(lock: &RawMutex, f: impl FnOnce() -> T) -> T {
-    lock.lock(f)
-}
-
 /// Data protected by a [RawMutex].
 ///
 /// This is largely equivalent to a `Mutex<RefCell<T>>`, but accessing the inner
@@ -462,7 +459,8 @@ impl<T> Locked<T> {
     ///
     /// Calling this reentrantly will panic.
     pub fn with<R>(&self, f: impl FnOnce(&mut T) -> R) -> R {
-        lock(&self.lock_state, || f(unsafe { &mut *self.data.get() }))
+        self.lock_state
+            .lock_non_reentrant(|| f(unsafe { &mut *self.data.get() }))
     }
 }
 
@@ -474,7 +472,7 @@ struct LockGuard<'a, L: single_core::RawLock> {
 }
 
 impl<'a, L: single_core::RawLock> LockGuard<'a, L> {
-    fn new(lock: &'a GenericRawMutex<L>) -> Self {
+    fn new_non_reentrant(lock: &'a GenericRawMutex<L>) -> Self {
         let this = Self::new_reentrant(lock);
         assert!(!this.token.is_reentry(), "lock is not reentrant");
         this

esp-hal/src/timer/systimer.rs

@@ -24,7 +24,7 @@ use crate::{
     asynch::AtomicWaker,
     interrupt::{self, InterruptHandler},
     peripherals::{Interrupt, SYSTIMER},
-    sync::{RawMutex, lock},
+    sync::RawMutex,
     system::{Cpu, Peripheral as PeripheralEnable, PeripheralClockControl},
     time::{Duration, Instant},
 };
@@ -168,7 +168,7 @@ impl Unit {
 
     #[cfg(not(esp32s2))]
     fn configure(&self, config: UnitConfig) {
-        lock(&CONF_LOCK, || {
+        CONF_LOCK.lock(|| {
             SYSTIMER::regs().conf().modify(|_, w| match config {
                 UnitConfig::Disabled => match self.channel() {
                     0 => w.timer_unit0_work_en().clear_bit(),
@@ -313,7 +313,7 @@ impl Alarm<'_> {
     /// Enables/disables the comparator. If enabled, this means
     /// it will generate interrupt based on its configuration.
     fn set_enable(&self, enable: bool) {
-        lock(&CONF_LOCK, || {
+        CONF_LOCK.lock(|| {
             #[cfg(not(esp32s2))]
             SYSTIMER::regs().conf().modify(|_, w| match self.channel() {
                 0 => w.target0_work_en().bit(enable),
@@ -566,7 +566,7 @@ impl super::Timer for Alarm<'_> {
     }
 
     fn enable_interrupt(&self, state: bool) {
-        lock(&INT_ENA_LOCK, || {
+        INT_ENA_LOCK.lock(|| {
             SYSTIMER::regs()
                 .int_ena()
                 .modify(|_, w| w.target(self.channel()).bit(state));

esp-hal/src/timer/timg.rs

@@ -93,7 +93,7 @@ cfg_if::cfg_if! {
     // and S2 where the effective interrupt enable register (config) is not shared between
     // the timers.
     if #[cfg(all(timergroup_timg_has_timer1, not(any(esp32, esp32s2))))] {
-        use crate::sync::{lock, RawMutex};
+        use crate::sync::RawMutex;
         static INT_ENA_LOCK: [RawMutex; NUM_TIMG] = [const { RawMutex::new() }; NUM_TIMG];
     }
 }
@@ -582,7 +582,7 @@ impl Timer<'_> {
                     .config()
                     .modify(|_, w| w.level_int_en().bit(state));
             } else if #[cfg(timergroup_timg_has_timer1)] {
-                lock(&INT_ENA_LOCK[self.timer_group() as usize], || {
+                INT_ENA_LOCK[self.timer_group() as usize].lock(|| {
                     self.register_block()
                         .int_ena()
                         .modify(|_, w| w.t(self.timer_number()).bit(state));

hil-test/tests/gpio.rs

@@ -58,6 +58,18 @@ pub fn interrupt_handler() {
     });
 }
 
+#[cfg_attr(feature = "unstable", handler)]
+#[cfg(feature = "unstable")]
+pub fn interrupt_handler_unlisten() {
+    critical_section::with(|cs| {
+        INPUT_PIN
+            .borrow_ref_mut(cs)
+            .as_mut()
+            .map(|pin| pin.unlisten());
+        *COUNTER.borrow_ref_mut(cs) += 1;
+    });
+}
+
 // Compile-time test to check that GPIOs can be passed by reference.
 fn _gpios_can_be_reused() {
     let p = esp_hal::init(esp_hal::Config::default());
@@ -328,6 +340,25 @@ mod tests {
         test_gpio1.unlisten();
     }
 
+    #[test]
+    #[cfg(feature = "unstable")] // Interrupts are unstable
+    async fn unlisten_in_interrupt_handler_does_not_panic(mut ctx: Context) {
+        ctx.io.set_interrupt_handler(interrupt_handler_unlisten);
+
+        let mut test_gpio1 =
+            Input::new(ctx.test_gpio1, InputConfig::default().with_pull(Pull::Down));
+        let mut test_gpio2 = Output::new(ctx.test_gpio2, Level::Low, OutputConfig::default());
+
+        critical_section::with(|cs| {
+            *COUNTER.borrow_ref_mut(cs) = 0;
+            test_gpio1.listen(Event::AnyEdge);
+            INPUT_PIN.borrow_ref_mut(cs).replace(test_gpio1);
+        });
+        test_gpio2.set_high();
+
+        while critical_section::with(|cs| *COUNTER.borrow_ref(cs)) == 0 {}
+    }
+
     #[test]
     #[cfg(feature = "unstable")] // delay is unstable
     fn gpio_od(ctx: Context) {