Skip to content

Commit

Permalink
Fix interrupt::free() unsoundness on multicore systems.
Browse files Browse the repository at this point in the history
This is unsound on multicore systems because it only disables interrupts in the
current core. For multicore chips, a chip-specific critical section implementation
is needed instead.

Unsoundness is fixed by not returning the `CriticalSection` token.

This is a breaking change.
  • Loading branch information
Dirbaio committed Oct 12, 2022
1 parent bc50767 commit 6d2ddc9
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 11 deletions.
1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ targets = [
critical-section-single-core = ["critical-section/restore-state-bool"]

[dependencies]
bare-metal = "1.0.0"
bit_field = "0.10.0"
critical-section = "1.1.0"
embedded-hal = "0.2.6"
20 changes: 12 additions & 8 deletions src/interrupt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@

// NOTE: Adapted from cortex-m/src/interrupt.rs
use crate::register::mstatus;
pub use bare_metal::{CriticalSection, Mutex};

/// Disables all interrupts
/// Disables all interrupts in the current core.
#[inline]
pub unsafe fn disable() {
match () {
Expand All @@ -15,11 +14,11 @@ pub unsafe fn disable() {
}
}

/// Enables all the interrupts
/// Enables all the interrupts in the current core.
///
/// # Safety
///
/// - Do not call this function inside an `interrupt::free` critical section
/// - Do not call this function inside a critical section.
#[inline]
pub unsafe fn enable() {
match () {
Expand All @@ -30,13 +29,18 @@ pub unsafe fn enable() {
}
}

/// Execute closure `f` in an interrupt-free context.
/// Execute closure `f` with interrupts disabled in the current core.
///
/// This as also known as a "critical section".
/// This method does not synchronise multiple cores, so it is not suitable for
/// using as a critical section. See the `critical-section` crate for a cross-platform
/// way to enter a critical section which provides a `CriticalSection` token.
///
/// This crate provides an implementation for `critical-section` suitable for single-core systems,
/// based on disabling all interrupts. It can be enabled with the `critical-section-single-core` feature.
#[inline]
pub fn free<F, R>(f: F) -> R
where
F: FnOnce(&CriticalSection) -> R,
F: FnOnce() -> R,
{
let mstatus = mstatus::read();

Expand All @@ -45,7 +49,7 @@ where
disable();
}

let r = f(unsafe { &CriticalSection::new() });
let r = f();

// If the interrupts were active before our `disable` call, then re-enable
// them. Otherwise, keep them disabled
Expand Down
7 changes: 7 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,10 @@ mod macros;

#[cfg(all(riscv, feature = "critical-section-single-core"))]
mod critical_section;

/// Used to reexport items for use in macros. Do not use directly.
/// Not covered by semver guarantees.
#[doc(hidden)]
pub mod _export {
pub use critical_section;
}
7 changes: 5 additions & 2 deletions src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
/// at most once in the whole lifetime of the program.
///
/// # Note
/// this macro is unsound on multi-core systems
///
/// This macro requires a `critical-section` implementation to be set. For most single core systems,
/// you can enable the `critical-section-single-core` feature for this crate. For other systems, you
/// have to provide one from elsewhere, typically your chip's HAL crate.
///
/// # Example
///
Expand All @@ -29,7 +32,7 @@
#[macro_export]
macro_rules! singleton {
(: $ty:ty = $expr:expr) => {
$crate::interrupt::free(|_| {
$crate::_export::critical_section::with(|_| {
static mut VAR: Option<$ty> = None;

#[allow(unsafe_code)]
Expand Down

0 comments on commit 6d2ddc9

Please sign in to comment.