From bf087ea51eb7c835873c71df042997433568a190 Mon Sep 17 00:00:00 2001 From: Tommaso Clini Date: Mon, 14 Sep 2026 17:23:14 +0200 Subject: [PATCH 1/3] use rtos semaphore as mutex --- esp-radio/src/refcount.rs | 132 ++++++++++++++++++++------------------ 1 file changed, 71 insertions(+), 61 deletions(-) diff --git a/esp-radio/src/refcount.rs b/esp-radio/src/refcount.rs index bbd02c2680b..a9e3411b875 100644 --- a/esp-radio/src/refcount.rs +++ b/esp-radio/src/refcount.rs @@ -1,83 +1,93 @@ -use core::sync::atomic::Ordering; +use core::{ptr::null_mut, sync::atomic::Ordering}; -use portable_atomic::AtomicU32; +use esp_radio_rtos_driver::semaphore::{SemaphoreHandle, SemaphoreKind, SemaphorePtr}; +use portable_atomic::{AtomicPtr, AtomicU32}; -// Refcount of 1 is special, indicating that the radio is being initialized or deinitialized. If a -// caller encounters this state, it must spin until the refcount changes. +/// Resource guard that handles initialization and deinitalization gracefully +/// using [`esp_radio_rtos_driver`]'s API. +pub(crate) struct Refcount { + counter: AtomicU32, + sem: AtomicPtr<()>, +} -/// A resource guard that uses a lock-free reference count to track usage. -pub(crate) struct Refcount(AtomicU32); +impl Drop for Refcount { + fn drop(&mut self) { + let sem = self.sem.load(Ordering::Relaxed); + if let Some(sem) = SemaphorePtr::new(sem) { + drop(unsafe { SemaphoreHandle::from_ptr(sem) }); + } + } +} impl Refcount { pub const fn new() -> Self { - Self(AtomicU32::new(0)) + Self { + counter: AtomicU32::new(0), + sem: AtomicPtr::new(null_mut()), + } } - pub fn increment(&self, on_first: impl FnOnce()) { - loop { - let op = self - .0 - .fetch_update(Ordering::Release, Ordering::Acquire, |old| { - if old == 1 { None } else { Some(old + 1) } - }); + fn use_sem_or_init(&self, f: impl FnOnce(&SemaphoreHandle) -> T) -> T { + if self.sem.load(Ordering::Relaxed).is_null() { + core::hint::cold_path(); + + let sem = SemaphoreHandle::new(SemaphoreKind::Mutex).leak(); + + if self + .sem + .compare_exchange( + null_mut(), + sem.as_ptr(), + Ordering::Release, + Ordering::Relaxed, + ) + .is_err() + { + core::hint::cold_path(); - match op { - Ok(0) => { - on_first(); - self.0.store(2, Ordering::Release); - break; - } - Ok(_) => break, - Err(_) => {} + drop(unsafe { SemaphoreHandle::from_ptr(sem) }); } } + + let sem = unsafe { SemaphorePtr::new_unchecked(self.sem.load(Ordering::Acquire)) }; + f(unsafe { SemaphoreHandle::ref_from_ptr(&sem) }) + } + + fn lock(&self, f: impl FnOnce() -> T) -> T { + self.use_sem_or_init(|sem| { + sem.take(None); + let ret = f(); + sem.give(); + ret + }) + } + + pub fn increment(&self, on_first: impl FnOnce()) { + self.lock(|| { + if self.counter.fetch_add(1, Ordering::Relaxed) == 0 { + on_first(); + } + }); } #[cfg(feature = "wifi")] pub fn try_increment(&self, on_first: impl FnOnce() -> Result<(), E>) -> Result { - loop { - let op = self - .0 - .fetch_update(Ordering::Release, Ordering::Acquire, |old| { - if old == 1 { None } else { Some(old + 1) } - }); - - match op { - Ok(0) => { - return match on_first() { - Ok(()) => { - self.0.store(2, Ordering::Release); - Ok(true) - } - Err(e) => { - self.0.store(0, Ordering::Release); - Err(e) - } - }; - } - Ok(_) => return Ok(false), - Err(_) => {} + self.lock(|| { + if self.counter.fetch_add(1, Ordering::Relaxed) == 0 { + on_first() + .inspect_err(|_| self.counter.store(0, Ordering::Relaxed)) + .map(|_| true) + } else { + Ok(false) } - } + }) } pub fn decrement(&self, on_last: impl FnOnce()) { - loop { - let op = self - .0 - .fetch_update(Ordering::Release, Ordering::Acquire, |old| { - if old == 1 { None } else { Some(old - 1) } - }); - - match op { - Ok(2) => { - on_last(); - self.0.store(0, Ordering::Release); - break; - } - Ok(_) => break, - Err(_) => {} + self.lock(|| { + if self.counter.fetch_sub(1, Ordering::Relaxed) == 1 { + on_last(); } - } + }) } } From 188fb573c2d66064c2ddcef90ad62434c21f2b18 Mon Sep 17 00:00:00 2001 From: Tommaso Clini Date: Mon, 14 Sep 2026 23:30:48 +0200 Subject: [PATCH 2/3] use unsafecell for counter as now it is locked with a mutex --- esp-radio/src/refcount.rs | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/esp-radio/src/refcount.rs b/esp-radio/src/refcount.rs index a9e3411b875..9adf14e9a95 100644 --- a/esp-radio/src/refcount.rs +++ b/esp-radio/src/refcount.rs @@ -1,15 +1,17 @@ -use core::{ptr::null_mut, sync::atomic::Ordering}; +use core::{cell::UnsafeCell, ptr::null_mut, sync::atomic::Ordering}; use esp_radio_rtos_driver::semaphore::{SemaphoreHandle, SemaphoreKind, SemaphorePtr}; -use portable_atomic::{AtomicPtr, AtomicU32}; +use portable_atomic::AtomicPtr; /// Resource guard that handles initialization and deinitalization gracefully /// using [`esp_radio_rtos_driver`]'s API. pub(crate) struct Refcount { - counter: AtomicU32, + counter: UnsafeCell, sem: AtomicPtr<()>, } +unsafe impl Sync for Refcount {} + impl Drop for Refcount { fn drop(&mut self) { let sem = self.sem.load(Ordering::Relaxed); @@ -22,7 +24,7 @@ impl Drop for Refcount { impl Refcount { pub const fn new() -> Self { Self { - counter: AtomicU32::new(0), + counter: UnsafeCell::new(0), sem: AtomicPtr::new(null_mut()), } } @@ -53,30 +55,31 @@ impl Refcount { f(unsafe { SemaphoreHandle::ref_from_ptr(&sem) }) } - fn lock(&self, f: impl FnOnce() -> T) -> T { + fn lock(&self, f: impl FnOnce(&mut u32) -> T) -> T { self.use_sem_or_init(|sem| { sem.take(None); - let ret = f(); + let ret = f(unsafe { self.counter.get().as_mut_unchecked() }); sem.give(); ret }) } pub fn increment(&self, on_first: impl FnOnce()) { - self.lock(|| { - if self.counter.fetch_add(1, Ordering::Relaxed) == 0 { + self.lock(|counter| { + if *counter == 0 { on_first(); } + *counter += 1; }); } #[cfg(feature = "wifi")] pub fn try_increment(&self, on_first: impl FnOnce() -> Result<(), E>) -> Result { - self.lock(|| { - if self.counter.fetch_add(1, Ordering::Relaxed) == 0 { - on_first() - .inspect_err(|_| self.counter.store(0, Ordering::Relaxed)) - .map(|_| true) + self.lock(|counter| { + let prev = *counter; + *counter += 1; + if prev == 0 { + on_first().inspect_err(|_| *counter = 0).map(|_| true) } else { Ok(false) } @@ -84,10 +87,11 @@ impl Refcount { } pub fn decrement(&self, on_last: impl FnOnce()) { - self.lock(|| { - if self.counter.fetch_sub(1, Ordering::Relaxed) == 1 { + self.lock(|counter| { + if *counter == 0 { on_last(); } + *counter += 1; }) } } From 828a8da0522370105e90fef19033adb2f816d519 Mon Sep 17 00:00:00 2001 From: Tommaso Clini Date: Thu, 17 Sep 2026 18:38:21 +0200 Subject: [PATCH 3/3] panic on decrement on uninitialized refcount or counter of 0 --- esp-radio/src/refcount.rs | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/esp-radio/src/refcount.rs b/esp-radio/src/refcount.rs index 9adf14e9a95..1d64411c5a1 100644 --- a/esp-radio/src/refcount.rs +++ b/esp-radio/src/refcount.rs @@ -55,6 +55,17 @@ impl Refcount { f(unsafe { SemaphoreHandle::ref_from_ptr(&sem) }) } + fn try_use_sem(&self, f: impl FnOnce(&SemaphoreHandle) -> T) -> Option { + if self.sem.load(Ordering::Relaxed).is_null() { + core::hint::cold_path(); + + None + } else { + let sem = unsafe { SemaphorePtr::new_unchecked(self.sem.load(Ordering::Acquire)) }; + Some(f(unsafe { SemaphoreHandle::ref_from_ptr(&sem) })) + } + } + fn lock(&self, f: impl FnOnce(&mut u32) -> T) -> T { self.use_sem_or_init(|sem| { sem.take(None); @@ -64,12 +75,21 @@ impl Refcount { }) } + fn try_lock(&self, f: impl FnOnce(&mut u32) -> T) -> Option { + self.try_use_sem(|sem| { + sem.take(None); + let ret = f(unsafe { self.counter.get().as_mut_unchecked() }); + sem.give(); + ret + }) + } + pub fn increment(&self, on_first: impl FnOnce()) { self.lock(|counter| { if *counter == 0 { on_first(); } - *counter += 1; + *counter = counter.checked_add(1).expect("refcount overflow"); }); } @@ -77,7 +97,8 @@ impl Refcount { pub fn try_increment(&self, on_first: impl FnOnce() -> Result<(), E>) -> Result { self.lock(|counter| { let prev = *counter; - *counter += 1; + *counter = counter.checked_add(1).expect("refcount overflow"); + if prev == 0 { on_first().inspect_err(|_| *counter = 0).map(|_| true) } else { @@ -87,11 +108,12 @@ impl Refcount { } pub fn decrement(&self, on_last: impl FnOnce()) { - self.lock(|counter| { + self.try_lock(|counter| { if *counter == 0 { on_last(); } - *counter += 1; + *counter = counter.checked_sub(1).expect("decrementing count of zero"); }) + .expect("decrementing before any successful increment") } }