embassy-sync, lock-api mutex compat

I spent a bunch of time today trying to figure out why embassy (an embedded async runtime in Rust) forked the RawMutex and Mutex code from lock_api. This makes it incompatible with foreign types implementing RawMutex, such as flipperzero's FuriMutex.

I wrote up a draft GitHub issue as I figured this out, thinking I had solved the issue. That appears below — at the end of this post, I explain what I actually figured out. Honestly, I'm mostly making this a blog post to preserve this issue that I spent several hours researching.

Original (draft) post

I'm doing a bit of work on the Flipper Zero, the rust crate for which provides a Mutex that implements lock_api::RawMutex. I have some previous firmware functionality written against embassy that I'm porting to the Flipper, and I started looking into using that Mutex type to back an embassy_sync::Mutex for async support.

I noticed that embassy_sync::blocking is a fork of lock_api with a scoped lock() (rather than lock + unlock). I patched my embassy fork to replace this with lock_api and haven't noticed any adverse effects -- all the embassy_sync tests pass and empirically, embassy_sync::Mutex works as expected.

Looking at #3175 and this gist that was linked there, the problem they're solving appears to be essentially about reentrancy wrt. critical sections. For the sake of argument, let's consider an impl of lock_api::RawMutex for CriticalSectionRawMutex that disallows reentrancy (this is essentially what I have in my patch):

pub struct CriticalSectionRawMutex {
    inner: UnsafeCell<Option<critical_section::RestoreState>>,
}

unsafe impl lock_api::RawMutex for CriticalSectionRawMutex {
    // ...
    fn lock(&self) {
        while !self.try_lock() {}
    }

    fn try_lock(&self) -> bool {
        let state = unsafe { critical_section::acquire() };

        // SAFETY: inner is only accessed within a critical section, which means single,
        // non-concurrent-access is guaranteed
        let inner = unsafe { &mut *self.inner.get() };

        if inner.is_some() {
            unsafe { critical_section::release(state) };

            return false;
        }

        *inner = Some(state);
        true
    }

    unsafe fn unlock(&self) {
        unsafe {
            let inner = &mut *self.inner.get();
            let state = inner.take().unwrap_unchecked();
            critical_section::release(state);
        }
    }
}

(Notably, this design differs from the existing CriticalSectionRawMutex in that I'm tracking the acquire/release state explicitly rather than using the closure form in critical_section::with.)

As far as I can tell, this has the properties we want — a single CriticalSectionRawMutex isn't reentrant (i.e. we can't re-lock the same mutex without first unlocking it), but the "global reentrancy" of critical_section is preserved (multiple mutexes can be taken at once). I.e., this doesn't work (as desired):

let mutex = lock_api::Mutex::<CriticalSectionRawMutex, _>::new(());

mutex.lock();
mutex.lock(); // spins forever

But this does:

let mutex1 = lock_api::Mutex::<CriticalSectionRawMutex, _>::new(());
let mutex2 = lock_api::Mutex::<CriticalSectionRawMutex, _>::new(());

mutex1.lock();
mutex2.lock();

Resolution

This actually doesn't satisfy the requirements for critical_section, specifically because it has the potential to violate nesting. While the above example will work because the underlying RestoreStates will be released in the correct nesting order, this is trivially violated:

let mutex1 = lock_api::Mutex::<CriticalSectionRawMutex, _>::new(());
let mutex2 = lock_api::Mutex::<CriticalSectionRawMutex, _>::new(());

mutex1.lock();
mutex2.lock();

// Wrong order!
mutex1.unlock();
mutex2.unlock();

This is the critical issue with this approach that requires scoped_mutex.