also for interrupt::free; revert register::primask API changes

Author: sw

cortex-m/src/critical_section.rs

@@ -1,4 +1,3 @@
-use core::sync::atomic::{compiler_fence, Ordering};
 use critical_section::{set_impl, Impl, RawRestoreState};
 
 use crate::interrupt;
@@ -9,15 +8,17 @@ set_impl!(SingleCoreCriticalSection);
 
 unsafe impl Impl for SingleCoreCriticalSection {
     unsafe fn acquire() -> RawRestoreState {
-        let restore_state = primask::read();
+        // Backup previous state of PRIMASK register. We access the entire register directly as a
+        // u32 instead of using the primask::read() function to minimize the number of processor
+        // cycles during which interrupts are disabled.
+        let restore_state = primask::read_raw();
         // NOTE: Fence guarantees are provided by interrupt::disable(), which performs a `compiler_fence(SeqCst)`.
         interrupt::disable();
-        restore_state.0
+        restore_state
     }
 
     unsafe fn release(restore_state: RawRestoreState) {
-        // Ensure no preceeding memory accesses are reordered to after interrupts are enabled.
-        compiler_fence(Ordering::SeqCst);
-        primask::write(restore_state);
+        // NOTE: Fence guarantees are provided by primask::write_raw(), which performs a `compiler_fence(SeqCst)`.
+        primask::write_raw(restore_state);
     }
 }

cortex-m/src/interrupt.rs

@@ -66,18 +66,17 @@ pub fn free<F, R>(f: F) -> R
 where
     F: FnOnce() -> R,
 {
-    let primask = crate::register::primask::read();
+    // Backup previous state of PRIMASK register. We access the entire register directly as a
+    // u32 instead of using the primask::read() function to minimize the number of processor
+    // cycles during which interrupts are disabled.
+    let primask = crate::register::primask::read_raw();
 
     // disable interrupts
     disable();
 
     let r = f();
 
-    // If the interrupts were active before our `disable` call, then re-enable
-    // them. Otherwise, keep them disabled
-    if primask.is_active() {
-        unsafe { enable() }
-    }
+    crate::register::primask::write_raw(primask);
 
     r
 }

cortex-m/src/register/primask.rs

@@ -2,36 +2,60 @@
 
 #[cfg(cortex_m)]
 use core::arch::asm;
+use core::sync::atomic::{compiler_fence, Ordering};
 
-/// Priority mask register
-pub struct Primask(pub u32);
+/// All exceptions with configurable priority are ...
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum Primask {
+    /// Active
+    Active,
+    /// Inactive
+    Inactive,
+}
 
 impl Primask {
     /// All exceptions with configurable priority are active
     #[inline]
     pub fn is_active(self) -> bool {
-        !self.is_inactive()
+        self == Primask::Active
     }
 
     /// All exceptions with configurable priority are inactive
     #[inline]
     pub fn is_inactive(self) -> bool {
-        self.0 & (1 << 0) == (1 << 0)
+        self == Primask::Inactive
     }
 }
 
-/// Reads the CPU register
+/// Reads the prioritizable interrupt mask
 #[cfg(cortex_m)]
 #[inline]
 pub fn read() -> Primask {
+    if read_raw() & (1 << 0) == (1 << 0) {
+        Primask::Inactive
+    } else {
+        Primask::Active
+    }
+}
+
+/// Reads the entire PRIMASK register
+/// Note that bits [31:1] are reserved and UNK (Unknown)
+#[cfg(cortex_m)]
+#[inline]
+pub fn read_raw() -> u32 {
     let r: u32;
     unsafe { asm!("mrs {}, PRIMASK", out(reg) r, options(nomem, nostack, preserves_flags)) };
-    Primask(r)
+    r
 }
 
-/// Writes the CPU register
+/// Writes the entire PRIMASK register
+/// Note that bits [31:1] are reserved and SBZP (Should-Be-Zero-or-Preserved)
 #[cfg(cortex_m)]
 #[inline]
-pub fn write(r: u32) {
+pub fn write_raw(r: u32) {
+    // Ensure no preceeding memory accesses are reordered to after interrupts are possibly enabled.
+    compiler_fence(Ordering::SeqCst);
     unsafe { asm!("msr PRIMASK, {}", in(reg) r, options(nomem, nostack, preserves_flags)) };
+    // Ensure no subsequent memory accesses are reordered to before interrupts are possibly disabled.
+    compiler_fence(Ordering::SeqCst);
 }

testsuite/src/main.rs

@@ -12,16 +12,16 @@ fn panic(info: &core::panic::PanicInfo) -> ! {
     minitest::fail()
 }
 
-static CRITICAL_SECTION_FLAG: AtomicBool = AtomicBool::new(false);
+static EXCEPTION_FLAG: AtomicBool = AtomicBool::new(false);
 
 #[cortex_m_rt::exception]
 fn PendSV() {
-    CRITICAL_SECTION_FLAG.store(true, Ordering::SeqCst);
+    EXCEPTION_FLAG.store(true, Ordering::SeqCst);
 }
 
 #[minitest::tests]
 mod tests {
-    use crate::{Ordering, CRITICAL_SECTION_FLAG};
+    use crate::{Ordering, EXCEPTION_FLAG};
     use minitest::log;
 
     #[init]
@@ -63,13 +63,27 @@ mod tests {
 
     #[test]
     fn critical_section_nesting() {
+        EXCEPTION_FLAG.store(false, Ordering::SeqCst);
         critical_section::with(|_| {
             critical_section::with(|_| {
                 cortex_m::peripheral::SCB::set_pendsv();
-                assert!(!CRITICAL_SECTION_FLAG.load(Ordering::SeqCst));
+                assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst));
             });
-            assert!(!CRITICAL_SECTION_FLAG.load(Ordering::SeqCst));
+            assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst));
         });
-        assert!(CRITICAL_SECTION_FLAG.load(Ordering::SeqCst));
+        assert!(EXCEPTION_FLAG.load(Ordering::SeqCst));
+    }
+
+    #[test]
+    fn interrupt_free_nesting() {
+        EXCEPTION_FLAG.store(false, Ordering::SeqCst);
+        cortex_m::interrupt::free(|| {
+            cortex_m::interrupt::free(|| {
+                cortex_m::peripheral::SCB::set_pendsv();
+                assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst));
+            });
+            assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst));
+        });
+        assert!(EXCEPTION_FLAG.load(Ordering::SeqCst));
     }
 }