irq: Optimize interrupt save/restore

Avoid unnecessary mask and branch instructions

Cargo.toml

@@ -62,8 +62,13 @@ docsrs = ["rt", "atmega328p", "atmega32u4", "atmega2560", "attiny85", "atmega480
 bare-metal = "0.2.5"
 vcell = "0.1.2"
 cfg-if = "0.1.10"
+ufmt = "0.1.0"
 
 [dependencies.avr-device-macros]
 path = "macros/"
 version = "=0.3.4"
 optional = true
+
+[patch.crates-io]
+# XXX: Temporary fix for avr-rust/rust#148
+ufmt = { git = "https://github.com/Rahix/ufmt.git", rev = "12225dc1678e42fecb0e8635bf80f501e24817d9" }

src/interrupt.rs

@@ -30,42 +30,127 @@
 
 pub use bare_metal::{CriticalSection, Mutex, Nr};
 
+use core::marker::PhantomData;
 #[cfg(target_arch = "avr")]
 use core::arch::asm;
 
-#[inline]
-/// Disables all interrupts
+/// Opaque structure for storing the global interrupt flag status.
 ///
-/// Returns a bool, reflecting whether interrupts were enabled prior to calling this method.
-pub fn disable() -> bool {
+/// This structure does not implement `Copy` and `Clone`,
+/// because the user shall not duplicate and pass it twice to [crate::interrupt::restore].
+#[derive(Debug)]
+pub struct IrqFlag {
+    // The saved SREG.
+    sreg: u8,
+    // Use PhantomData with raw pointer to disable Send and Sync auto-trait impls.
+    // Negative impl of Send/Sync is not stabilized in rustc, yet.
+    // This is not strictly necessary for memory safety.
+    // It's just to keep the user from passing this struct between interrupt contexts,
+    // which would not be meaningful.
+    _phantom: PhantomData<*mut u8>,
+}
+
+impl IrqFlag {
+    #[inline(always)]
+    fn new(sreg: u8) -> IrqFlag {
+        IrqFlag {
+            sreg,
+            _phantom: PhantomData,
+        }
+    }
+
+    /// Check the status of the saved global interrupt flag.
+    ///
+    /// Returns true, if the saved global interrupt flag is set (IRQs enabled).
+    /// Otherwise returns false.
+    ///
+    /// This method can be used to check whether interrupts were enabled
+    /// before the [crate::interrupt::disable_save] call.
+    /// You probably shouldn't make your program behavior dependent on this state.
+    /// Consider using a different design.
+    #[inline(always)]
+    pub fn enabled(&self) -> bool {
+        self.sreg & 0x80 != 0
+    }
+}
+
+// PhantomData is not supported by uDebug. Spin our own fmt implementation.
+impl ufmt::uDebug for IrqFlag {
+    fn fmt<W: ufmt::uWrite + ?Sized>(&self, f: &mut ufmt::Formatter<'_, W>) -> Result<(), W::Error> {
+        f.debug_struct("IrqFlag")?
+            .field("sreg", &self.sreg)?
+            .finish()
+    }
+}
+
+/// Disable the global interrupt flag.
+///
+/// *Hint*: Most of the time you probably don't want to use this function directly.
+///         Consider creating a critical section with [crate::interrupt::free] instead.
+///
+/// This function is an optimization fence.
+/// That means memory accesses will not be re-ordered by the compiler across this function call.
+#[inline(always)]
+pub fn disable() {
     cfg_if::cfg_if! {
         if #[cfg(target_arch = "avr")] {
-            // Store current state
-            let sreg: u8;
+            // Disable interrupts
+            unsafe { asm!("cli") };
+        } else {
+            unimplemented!()
+        }
+    }
+}
 
+/// Disable the global interrupt flag and return an opaque representation of the previous flag status.
+///
+/// *Hint*: Most of the time you probably don't want to use this function directly.
+///         Consider creating a critical section with [crate::interrupt::free] instead.
+///
+/// This function is an optimization fence.
+/// That means memory accesses will not be re-ordered by the compiler across this function call.
+///
+/// Returns an object that contains the status of the global interrupt flag from *before* the `disable_save()` call.
+/// This object shall later be passed to the [crate::interrupt::restore] function.
+#[inline(always)]
+#[allow(unreachable_code)]
+pub fn disable_save() -> IrqFlag {
+    let sreg;
+    cfg_if::cfg_if! {
+        if #[cfg(target_arch = "avr")] {
+            // Store current state
             unsafe {
                 asm!(
                     "in {sreg}, 0x3F",
                     sreg = out(reg) sreg,
                 )
             };
-
-            // Disable interrupts
-            unsafe { asm!("cli") };
-
-            sreg & 0x80 == 0x80
         } else {
+            let _ = sreg;
             unimplemented!()
         }
     }
+    // Disable interrupts
+    disable();
+
+    IrqFlag::new(sreg)
 }
 
-#[inline]
-/// Enables all the interrupts
+/// Enable the global interrupt flag.
+///
+/// *Warning*: This function enables interrupts, no matter what the enable-state was before [crate::interrupt::disable].
+///            Especially in library code, where the previous interrupt state may be unknown,
+///            this function call shall be avoided.
+///            Most of the time you probably don't want to use this function directly.
+///            Consider creating a critical section with [crate::interrupt::free] instead.
+///
+/// This function is an optimization fence.
+/// That means memory accesses will not be re-ordered by the compiler across this function call.
 ///
 /// # Safety
 ///
 /// - Do not call this function inside an [crate::interrupt::free] critical section
+#[inline(always)]
 pub unsafe fn enable() {
     cfg_if::cfg_if! {
         if #[cfg(target_arch = "avr")] {
@@ -76,24 +161,82 @@ pub unsafe fn enable() {
     }
 }
 
+/// Restore the global interrupt flag to its previous state before [crate::interrupt::disable_save].
+///
+/// *Hint*: Most of the time you probably don't want to use this function directly.
+///         Consider creating a critical section with [crate::interrupt::free] instead.
+///
+/// This function is an optimization fence.
+/// That means memory accesses will not be re-ordered by the compiler across this function call.
+///
+/// # Safety
+///
+/// - Do not call this function inside an [crate::interrupt::free] critical section
+#[inline(always)]
+pub unsafe fn restore(irq_flag: IrqFlag) {
+    cfg_if::cfg_if! {
+        if #[cfg(target_arch = "avr")] {
+            // Restore global interrupt flag in SREG.
+            // This also clobbers all other bits in SREG.
+            asm!(
+                "out 0x3F, {sreg}",
+                sreg = in(reg) irq_flag.sreg,
+            );
+        } else {
+            let _ = irq_flag;
+            unimplemented!()
+        }
+    }
+}
+
+/// Check whether the global interrupt flag is currently enabled (in SREG).
+///
+/// *Warning*: You shouldn't use this to hand craft your own memory/interrupt safety mechanisms.
+///            This function may be used for things such as deciding whether to do
+///            expensive calculations in library code, or similar things.
+///
+/// This function is **not** an optimization fence.
+/// That means memory accesses *can* be re-ordered by the compiler across this function call.
+#[inline(always)]
+#[allow(unreachable_code)]
+pub fn is_enabled() -> bool {
+    let sreg;
+    cfg_if::cfg_if! {
+        if #[cfg(target_arch = "avr")] {
+            // Store current state
+            unsafe {
+                asm!(
+                    "in {sreg}, 0x3F",
+                    sreg = out(reg) sreg,
+                    options(readonly, preserves_flags, nostack),
+                )
+            };
+        } else {
+            let _ = sreg;
+            unimplemented!()
+        }
+    }
+
+    IrqFlag::new(sreg).enabled()
+}
+
 /// Execute closure `f` in an interrupt-free context.
 ///
 /// This as also known as a "critical section".
+#[inline(always)]
 pub fn free<F, R>(f: F) -> R
 where
     F: FnOnce(&CriticalSection) -> R,
 {
     cfg_if::cfg_if! {
         if #[cfg(target_arch = "avr")] {
-            // Disable interrupts
-            let interrupts_enabled = disable();
+            // Disable interrupts. This is an optimization fence.
+            let irq_flag = disable_save();
 
             let r = f(unsafe { &CriticalSection::new() });
 
-            // Restore interrupt state
-            if interrupts_enabled {
-                unsafe { enable(); }
-            }
+            // Restore interrupt state. This is an optimization fence.
+            unsafe { restore(irq_flag); }
 
             r
         } else {