From 280d685517656cbec867ab5f1190c05ae50650b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20B=C3=BCsch?= Date: Wed, 7 Sep 2022 08:30:03 +0200 Subject: [PATCH] irq: Optimize interrupt save/restore Avoid unnecessary mask and branch instructions. The basic reasoning behind this is that all other flags in the SREG can be clobbered without ill effects. The restore() function is an optimization fence and the compiler is not allowed to make assumptions about memory or SREG state after execution. This avoids an `and` and a `breq` instruction or similar in every critical section. While at it, also introduce a better API for manual IRQ-flag management. --- Cargo.toml | 4 ++ src/interrupt.rs | 170 +++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 155 insertions(+), 19 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 88a1285..8551d53 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,10 +58,14 @@ rt = ["avr-device-macros"] # feel free to add it here. docsrs = ["rt", "atmega328p", "atmega32u4", "atmega2560", "attiny85", "atmega4809"] +# Enable ufmt::uDebug impls for certain types to aid efficient debugging. +udebug = ["dep:ufmt"] + [dependencies] bare-metal = "0.2.5" vcell = "0.1.2" cfg-if = "0.1.10" +ufmt = { version = "0.2.0", optional = true } [dependencies.avr-device-macros] path = "macros/" diff --git a/src/interrupt.rs b/src/interrupt.rs index 65b8c83..2ff0041 100644 --- a/src/interrupt.rs +++ b/src/interrupt.rs @@ -33,39 +33,108 @@ pub use bare_metal::{CriticalSection, Mutex, Nr}; #[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)] +#[cfg_attr(feature = "udebug", derive(ufmt::derive::uDebug))] +pub struct IrqFlag { + // The saved SREG. + sreg: u8, +} + +impl IrqFlag { + #[inline(always)] + fn new(sreg: u8) -> IrqFlag { + IrqFlag { + sreg, + } + } + + /// 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 + } +} + +/// 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 +145,87 @@ 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 +/// +/// - If you call this function inside of a [crate::interrupt::free] critical section, the +/// corresponding [crate::interrupt::disable_save] must also be in the same critical section. +/// - If you nest multiple [crate::interrupt::disable_save] + [crate::interrupt::restore] +/// sequences, the [crate::interrupt::restore] must be called in the reverse order of the +/// [crate::interrupt::disable_save] call order. +/// That means the first saved IrqFlag must be restored last. +#[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: 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 { -- 2.49.0