From 744e2cbb8a0b58eccf3a9708c8c28f1ef17e4771 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Mon, 5 Jul 2021 17:42:43 +1000 Subject: [PATCH 01/12] extras: Fix UB in `Peripheral` `Peripheral` assumed that interrupts can't be preempted, when they can be preempted by higher priority interrupts. So I put the interrupt handler inside a critical section, and also added checks for whether the state had been dropped before the critical section was entered. I also added a `'static` bound to `PeripheralState`, since `Pin` only guarantees that the memory it directly references will not be invalidated. It doesn't guarantee that memory its pointee references also won't be invalidated. There were already some implementations of `PeripheralState` that weren't `'static`, though, so I added an unsafe `PeripheralStateUnchecked` trait and forwarded the `unsafe` to the constructors of the implementors. --- embassy-extras/Cargo.toml | 1 + embassy-extras/src/peripheral.rs | 50 ++++++++++++++++++++----- embassy-extras/src/peripheral_shared.rs | 45 ++++++++++++++++------ embassy-extras/src/usb/mod.rs | 13 ++++--- embassy-nrf/src/buffered_uarte.rs | 5 ++- 5 files changed, 86 insertions(+), 28 deletions(-) diff --git a/embassy-extras/Cargo.toml b/embassy-extras/Cargo.toml index 5d07901a9..8415e32ef 100644 --- a/embassy-extras/Cargo.toml +++ b/embassy-extras/Cargo.toml @@ -17,4 +17,5 @@ embassy = { version = "0.1.0", path = "../embassy" } defmt = { version = "0.2.0", optional = true } log = { version = "0.4.11", optional = true } cortex-m = "0.7.1" +critical-section = "0.2.1" usb-device = "0.2.7" diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index 68972c543..e3e06d692 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -1,15 +1,38 @@ use core::cell::UnsafeCell; use core::marker::{PhantomData, PhantomPinned}; use core::pin::Pin; +use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; -pub trait PeripheralState { +/// # Safety +/// When types implementing this trait are used with `Peripheral` or `PeripheralMutex`, +/// their lifetime must not end without first calling `Drop` on the `Peripheral` or `PeripheralMutex`. +pub unsafe trait PeripheralStateUnchecked { type Interrupt: Interrupt; fn on_interrupt(&mut self); } -pub struct PeripheralMutex { +// `PeripheralMutex` is safe because `Pin` guarantees that the memory it references will not be invalidated or reused +// without calling `Drop`. However, it provides no guarantees about references contained within the state still being valid, +// so this `'static` bound is necessary. +pub trait PeripheralState: 'static { + type Interrupt: Interrupt; + fn on_interrupt(&mut self); +} + +// SAFETY: `T` has to live for `'static` to implement `PeripheralState`, thus its lifetime cannot end. +unsafe impl PeripheralStateUnchecked for T +where + T: PeripheralState, +{ + type Interrupt = T::Interrupt; + fn on_interrupt(&mut self) { + self.on_interrupt() + } +} + +pub struct PeripheralMutex { state: UnsafeCell, irq_setup_done: bool, @@ -19,7 +42,7 @@ pub struct PeripheralMutex { _pinned: PhantomPinned, } -impl PeripheralMutex { +impl PeripheralMutex { pub fn new(state: S, irq: S::Interrupt) -> Self { Self { irq, @@ -39,11 +62,17 @@ impl PeripheralMutex { this.irq.disable(); this.irq.set_handler(|p| { - // Safety: it's OK to get a &mut to the state, since - // - We're in the IRQ, no one else can't preempt us - // - We can't have preempted a with() call because the irq is disabled during it. - let state = unsafe { &mut *(p as *mut S) }; - state.on_interrupt(); + critical_section::with(|_| { + if p.is_null() { + // The state was dropped, so we can't operate on it. + return; + } + // Safety: it's OK to get a &mut to the state, since + // - We're in a critical section, no one can preempt us (and call with()) + // - We can't have preempted a with() call because the irq is disabled during it. + let state = unsafe { &mut *(p as *mut S) }; + state.on_interrupt(); + }) }); this.irq .set_handler_context((&mut this.state) as *mut _ as *mut ()); @@ -67,9 +96,12 @@ impl PeripheralMutex { } } -impl Drop for PeripheralMutex { +impl Drop for PeripheralMutex { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); + // Set the context to null so that the interrupt will know we're dropped + // if we pre-empted it before it entered a critical section. + self.irq.set_handler_context(ptr::null_mut()); } } diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index c62113396..a9fca8ca6 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -1,16 +1,27 @@ -use core::cell::UnsafeCell; use core::marker::{PhantomData, PhantomPinned}; use core::pin::Pin; +use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; -pub trait PeripheralState { +/// # Safety +/// When types implementing this trait are used with `Peripheral` or `PeripheralMutex`, +/// their lifetime must not end without first calling `Drop` on the `Peripheral` or `PeripheralMutex`. +pub unsafe trait PeripheralStateUnchecked { type Interrupt: Interrupt; fn on_interrupt(&self); } -pub struct Peripheral { - state: UnsafeCell, +// `Peripheral` is safe because `Pin` guarantees that the memory it references will not be invalidated or reused +// without calling `Drop`. However, it provides no guarantees about references contained within the state still being valid, +// so this `'static` bound is necessary. +pub trait PeripheralState: 'static { + type Interrupt: Interrupt; + fn on_interrupt(&self); +} + +pub struct Peripheral { + state: S, irq_setup_done: bool, irq: S::Interrupt, @@ -19,13 +30,13 @@ pub struct Peripheral { _pinned: PhantomPinned, } -impl Peripheral { +impl Peripheral { pub fn new(irq: S::Interrupt, state: S) -> Self { Self { irq, irq_setup_done: false, - state: UnsafeCell::new(state), + state, _not_send: PhantomData, _pinned: PhantomPinned, } @@ -39,8 +50,16 @@ impl Peripheral { this.irq.disable(); this.irq.set_handler(|p| { - let state = unsafe { &*(p as *const S) }; - state.on_interrupt(); + // We need to be in a critical section so that no one can preempt us + // and drop the state after we check whether `p.is_null()`. + critical_section::with(|_| { + if p.is_null() { + // The state was dropped, so we can't operate on it. + return; + } + let state = unsafe { &*(p as *const S) }; + state.on_interrupt(); + }); }); this.irq .set_handler_context((&this.state) as *const _ as *mut ()); @@ -49,15 +68,17 @@ impl Peripheral { this.irq_setup_done = true; } - pub fn state(self: Pin<&mut Self>) -> &S { - let this = unsafe { self.get_unchecked_mut() }; - unsafe { &*this.state.get() } + pub fn state<'a>(self: Pin<&'a mut Self>) -> &'a S { + &self.into_ref().get_ref().state } } -impl Drop for Peripheral { +impl Drop for Peripheral { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); + // Set the context to null so that the interrupt will know we're dropped + // if we pre-empted it before it entered a critical section. + self.irq.set_handler_context(ptr::null_mut()); } } diff --git a/embassy-extras/src/usb/mod.rs b/embassy-extras/src/usb/mod.rs index 182cd87d0..330eb9220 100644 --- a/embassy-extras/src/usb/mod.rs +++ b/embassy-extras/src/usb/mod.rs @@ -9,7 +9,7 @@ use usb_device::device::UsbDevice; mod cdc_acm; pub mod usb_serial; -use crate::peripheral::{PeripheralMutex, PeripheralState}; +use crate::peripheral::{PeripheralMutex, PeripheralStateUnchecked}; use embassy::interrupt::Interrupt; use usb_serial::{ReadInterface, UsbSerial, WriteInterface}; @@ -55,10 +55,12 @@ where } } - pub fn start(self: Pin<&mut Self>) { - let this = unsafe { self.get_unchecked_mut() }; + /// # Safety + /// The `UsbDevice` passed to `Self::new` must not be dropped without calling `Drop` on this `Usb` first. + pub unsafe fn start(self: Pin<&mut Self>) { + let this = self.get_unchecked_mut(); let mut mutex = this.inner.borrow_mut(); - let mutex = unsafe { Pin::new_unchecked(&mut *mutex) }; + let mutex = Pin::new_unchecked(&mut *mutex); // Use inner to register the irq mutex.register_interrupt(); @@ -125,7 +127,8 @@ where } } -impl<'bus, B, T, I> PeripheralState for State<'bus, B, T, I> +// SAFETY: The safety contract of `PeripheralStateUnchecked` is forwarded to `Usb::start`. +unsafe impl<'bus, B, T, I> PeripheralStateUnchecked for State<'bus, B, T, I> where B: UsbBus, T: ClassSet, diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index a5a37b982..1fa98a6b2 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -7,7 +7,7 @@ use core::task::{Context, Poll}; use embassy::interrupt::InterruptExt; use embassy::io::{AsyncBufRead, AsyncWrite, Result}; use embassy::util::{Unborrow, WakerRegistration}; -use embassy_extras::peripheral::{PeripheralMutex, PeripheralState}; +use embassy_extras::peripheral::{PeripheralMutex, PeripheralStateUnchecked}; use embassy_extras::ring_buffer::RingBuffer; use embassy_extras::{low_power_wait_until, unborrow}; @@ -283,7 +283,8 @@ impl<'a, U: UarteInstance, T: TimerInstance> Drop for State<'a, U, T> { } } -impl<'a, U: UarteInstance, T: TimerInstance> PeripheralState for State<'a, U, T> { +// SAFETY: the safety contract of `PeripheralStateUnchecked` is forwarded to `BufferedUarte::new`. +unsafe impl<'a, U: UarteInstance, T: TimerInstance> PeripheralStateUnchecked for State<'a, U, T> { type Interrupt = U::Interrupt; fn on_interrupt(&mut self) { trace!("irq: start"); From 3d96b10b0cd0066d4269094852e458bbf80a9807 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Mon, 5 Jul 2021 17:47:55 +1000 Subject: [PATCH 02/12] Elide lifetimes on `Peripheral::state` --- embassy-extras/src/peripheral_shared.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index a9fca8ca6..d05ae0307 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -68,7 +68,7 @@ impl Peripheral { this.irq_setup_done = true; } - pub fn state<'a>(self: Pin<&'a mut Self>) -> &'a S { + pub fn state(self: Pin<&mut Self>) -> &S { &self.into_ref().get_ref().state } } From fc1ef4947d463495ccd3da1d7763dcbe95a2588f Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Mon, 5 Jul 2021 18:18:05 +1000 Subject: [PATCH 03/12] Fix stm32 ethernet --- embassy-extras/src/peripheral.rs | 4 ++-- embassy-extras/src/peripheral_shared.rs | 4 ++-- embassy-stm32/src/eth/v2/mod.rs | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index e3e06d692..edc5fe955 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -6,8 +6,8 @@ use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; /// # Safety -/// When types implementing this trait are used with `Peripheral` or `PeripheralMutex`, -/// their lifetime must not end without first calling `Drop` on the `Peripheral` or `PeripheralMutex`. +/// When types implementing this trait are used with `PeripheralMutex`, +/// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `PeripheralMutex`. pub unsafe trait PeripheralStateUnchecked { type Interrupt: Interrupt; fn on_interrupt(&mut self); diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index d05ae0307..820622bb9 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -5,8 +5,8 @@ use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; /// # Safety -/// When types implementing this trait are used with `Peripheral` or `PeripheralMutex`, -/// their lifetime must not end without first calling `Drop` on the `Peripheral` or `PeripheralMutex`. +/// When types implementing this trait are used with `Peripheral`, +/// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `Peripheral`. pub unsafe trait PeripheralStateUnchecked { type Interrupt: Interrupt; fn on_interrupt(&self); diff --git a/embassy-stm32/src/eth/v2/mod.rs b/embassy-stm32/src/eth/v2/mod.rs index a8a361dfe..c4be2bd2c 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -343,7 +343,8 @@ impl<'d, const TX: usize, const RX: usize> Inner<'d, TX, RX> { } } -impl<'d, const TX: usize, const RX: usize> PeripheralState for Inner<'d, TX, RX> { +// SAFETY: The lifetime of `Inner` is only due to `PhantomData`; it isn't actually referencing any data with that lifetime. +unsafe impl<'d, const TX: usize, const RX: usize> PeripheralStateUnchecked for Inner<'d, TX, RX> { type Interrupt = crate::interrupt::ETH; fn on_interrupt(&mut self) { From ff9ff5e43a357cd54f25e83dd3e6e9628266f60b Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Mon, 5 Jul 2021 18:31:54 +1000 Subject: [PATCH 04/12] Update the import --- embassy-stm32/src/eth/v2/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/embassy-stm32/src/eth/v2/mod.rs b/embassy-stm32/src/eth/v2/mod.rs index c4be2bd2c..567088e8d 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -4,7 +4,7 @@ use core::sync::atomic::{fence, Ordering}; use core::task::Waker; use embassy::util::{AtomicWaker, Unborrow}; -use embassy_extras::peripheral::{PeripheralMutex, PeripheralState}; +use embassy_extras::peripheral::{PeripheralMutex, PeripheralStateUnchecked}; use embassy_extras::unborrow; use embassy_net::{Device, DeviceCapabilities, LinkState, PacketBuf, MTU}; From 1b7ad7080e6a8c96f2622d75b99a4d3bdbc7c394 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Sat, 24 Jul 2021 12:53:57 +1000 Subject: [PATCH 05/12] Add `Send/Sync` bounds to `PeripheralState` --- embassy-extras/src/peripheral.rs | 18 +++++++++++++----- embassy-extras/src/peripheral_shared.rs | 17 ++++++++++++----- embassy-extras/src/usb/mod.rs | 24 ++++++++++++------------ embassy-nrf/src/timer.rs | 2 +- embassy-nrf/src/uarte.rs | 2 +- embassy/src/util/waker.rs | 5 +++++ 6 files changed, 44 insertions(+), 24 deletions(-) diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index edc5fe955..2358ab96f 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -5,18 +5,26 @@ use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; +/// A version of `PeripheralState` without the `'static` bound, +/// for cases where the compiler can't statically make sure +/// that `on_interrupt` doesn't reference anything which might be invalidated. +/// /// # Safety /// When types implementing this trait are used with `PeripheralMutex`, /// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `PeripheralMutex`. -pub unsafe trait PeripheralStateUnchecked { +pub unsafe trait PeripheralStateUnchecked: Send { type Interrupt: Interrupt; fn on_interrupt(&mut self); } -// `PeripheralMutex` is safe because `Pin` guarantees that the memory it references will not be invalidated or reused -// without calling `Drop`. However, it provides no guarantees about references contained within the state still being valid, -// so this `'static` bound is necessary. -pub trait PeripheralState: 'static { +/// A type which can be used as state with `PeripheralMutex`. +/// +/// It needs to be `Send` because `&mut` references are sent back and forth between the 'thread' which owns the `PeripheralMutex` and the interrupt, +/// and `&mut T` is `Send` where `T: Send`. +/// +/// It also requires `'static`, because although `Pin` guarantees that the memory of the state won't be invalidated, +/// it doesn't guarantee that the lifetime will last. +pub trait PeripheralState: Send + 'static { type Interrupt: Interrupt; fn on_interrupt(&mut self); } diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index 820622bb9..5961441e0 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -4,18 +4,25 @@ use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; +/// A version of `PeripheralState` without the `'static` bound, +/// for cases where the compiler can't statically make sure +/// that `on_interrupt` doesn't reference anything which might be invalidated. +/// /// # Safety /// When types implementing this trait are used with `Peripheral`, /// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `Peripheral`. -pub unsafe trait PeripheralStateUnchecked { +pub unsafe trait PeripheralStateUnchecked: Sync { type Interrupt: Interrupt; fn on_interrupt(&self); } -// `Peripheral` is safe because `Pin` guarantees that the memory it references will not be invalidated or reused -// without calling `Drop`. However, it provides no guarantees about references contained within the state still being valid, -// so this `'static` bound is necessary. -pub trait PeripheralState: 'static { +/// A type which can be used as state with `Peripheral`. +/// +/// It needs to be `Sync` because references are shared between the 'thread' which owns the `Peripheral` and the interrupt. +/// +/// It also requires `'static`, because although `Pin` guarantees that the memory of the state won't be invalidated, +/// it doesn't guarantee that the lifetime will last. +pub trait PeripheralState: Sync + 'static { type Interrupt: Interrupt; fn on_interrupt(&self); } diff --git a/embassy-extras/src/usb/mod.rs b/embassy-extras/src/usb/mod.rs index 330eb9220..481987a6c 100644 --- a/embassy-extras/src/usb/mod.rs +++ b/embassy-extras/src/usb/mod.rs @@ -14,7 +14,7 @@ use embassy::interrupt::Interrupt; use usb_serial::{ReadInterface, UsbSerial, WriteInterface}; /// Marker trait to mark an interrupt to be used with the [`Usb`] abstraction. -pub unsafe trait USBInterrupt: Interrupt {} +pub unsafe trait USBInterrupt: Interrupt + Send {} pub(crate) struct State<'bus, B, T, I> where @@ -140,7 +140,7 @@ where } } -pub trait ClassSet { +pub trait ClassSet: Send { fn poll_all(&mut self, device: &mut UsbDevice<'_, B>) -> bool; } @@ -176,8 +176,8 @@ pub struct Index1; impl ClassSet for ClassSet1 where - B: UsbBus, - C1: UsbClass, + B: UsbBus + Send, + C1: UsbClass + Send, { fn poll_all(&mut self, device: &mut UsbDevice<'_, B>) -> bool { device.poll(&mut [&mut self.class]) @@ -186,9 +186,9 @@ where impl ClassSet for ClassSet2 where - B: UsbBus, - C1: UsbClass, - C2: UsbClass, + B: UsbBus + Send, + C1: UsbClass + Send, + C2: UsbClass + Send, { fn poll_all(&mut self, device: &mut UsbDevice<'_, B>) -> bool { device.poll(&mut [&mut self.class1, &mut self.class2]) @@ -197,8 +197,8 @@ where impl IntoClassSet> for C1 where - B: UsbBus, - C1: UsbClass, + B: UsbBus + Send, + C1: UsbClass + Send, { fn into_class_set(self) -> ClassSet1 { ClassSet1 { @@ -210,9 +210,9 @@ where impl IntoClassSet> for (C1, C2) where - B: UsbBus, - C1: UsbClass, - C2: UsbClass, + B: UsbBus + Send, + C1: UsbClass + Send, + C2: UsbClass + Send, { fn into_class_set(self) -> ClassSet2 { ClassSet2 { diff --git a/embassy-nrf/src/timer.rs b/embassy-nrf/src/timer.rs index a6e91f228..7ff35c320 100644 --- a/embassy-nrf/src/timer.rs +++ b/embassy-nrf/src/timer.rs @@ -29,7 +29,7 @@ pub(crate) mod sealed { pub trait ExtendedInstance {} } -pub trait Instance: Unborrow + sealed::Instance + 'static { +pub trait Instance: Unborrow + sealed::Instance + 'static + Send { type Interrupt: Interrupt; } pub trait ExtendedInstance: Instance + sealed::ExtendedInstance {} diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs index 67ec5d73f..985854a5f 100644 --- a/embassy-nrf/src/uarte.rs +++ b/embassy-nrf/src/uarte.rs @@ -461,7 +461,7 @@ pub(crate) mod sealed { } } -pub trait Instance: Unborrow + sealed::Instance + 'static { +pub trait Instance: Unborrow + sealed::Instance + 'static + Send { type Interrupt: Interrupt; } diff --git a/embassy/src/util/waker.rs b/embassy/src/util/waker.rs index 393155099..1ac6054f9 100644 --- a/embassy/src/util/waker.rs +++ b/embassy/src/util/waker.rs @@ -48,6 +48,11 @@ impl WakerRegistration { } } +// SAFETY: `WakerRegistration` effectively contains an `Option`, +// which is `Send` and `Sync`. +unsafe impl Send for WakerRegistration {} +unsafe impl Sync for WakerRegistration {} + pub struct AtomicWaker { waker: AtomicPtr, } From 079526559f44a0822574205e8798136f11f207d3 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Tue, 27 Jul 2021 17:28:52 +1000 Subject: [PATCH 06/12] Remove critical sections from `PeripheralMutex` interrupt handler by checking the interrupt's priority on startup. Since `PeripheralMutex` is the only way to safely maintain state across interrupts, and it no longer allows setting the interrupt's priority, the priority changing isn't a concern. This also prevents other causes of UB due to the interrupt being exposed during `with`, and allowing enabling the interrupt and setting its context to a bogus pointer. --- embassy-extras/src/peripheral.rs | 140 +++++++++++++++++++++--- embassy-extras/src/peripheral_shared.rs | 61 ++++++++--- embassy-extras/src/usb/usb_serial.rs | 6 +- embassy-nrf/src/buffered_uarte.rs | 20 ++-- embassy-stm32/src/eth/v2/mod.rs | 8 +- 5 files changed, 187 insertions(+), 48 deletions(-) diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index 2358ab96f..2402ba9eb 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -1,8 +1,9 @@ use core::cell::UnsafeCell; use core::marker::{PhantomData, PhantomPinned}; use core::pin::Pin; -use core::ptr; +use cortex_m::peripheral::scb::{Exception, SystemHandler, VectActive}; +use cortex_m::peripheral::{NVIC, SCB}; use embassy::interrupt::{Interrupt, InterruptExt}; /// A version of `PeripheralState` without the `'static` bound, @@ -50,8 +51,79 @@ pub struct PeripheralMutex { _pinned: PhantomPinned, } +fn exception_to_system_handler(exception: Exception) -> Option { + match exception { + Exception::NonMaskableInt | Exception::HardFault => None, + #[cfg(not(armv6m))] + Exception::MemoryManagement => Some(SystemHandler::MemoryManagement), + #[cfg(not(armv6m))] + Exception::BusFault => Some(SystemHandler::BusFault), + #[cfg(not(armv6m))] + Exception::UsageFault => Some(SystemHandler::UsageFault), + #[cfg(any(armv8m, target_arch = "x86_64"))] + Exception::SecureFault => Some(SystemHandler::SecureFault), + Exception::SVCall => Some(SystemHandler::SVCall), + #[cfg(not(armv6m))] + Exception::DebugMonitor => Some(SystemHandler::DebugMonitor), + Exception::PendSV => Some(SystemHandler::PendSV), + Exception::SysTick => Some(SystemHandler::SysTick), + } +} + +/// Whether `irq` can be preempted by the current interrupt. +pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool { + match SCB::vect_active() { + // Thread mode can't preempt each other + VectActive::ThreadMode => false, + VectActive::Exception(exception) => { + // `SystemHandler` is a subset of `Exception` for those with configurable priority. + // There's no built in way to convert between them, so `exception_to_system_handler` was necessary. + if let Some(system_handler) = exception_to_system_handler(exception) { + let current_prio = SCB::get_priority(system_handler); + let irq_prio = irq.get_priority().into(); + if current_prio < irq_prio { + true + } else if current_prio == irq_prio { + // When multiple interrupts have the same priority number, + // the pending interrupt with the lowest interrupt number takes precedence. + (exception.irqn() as i16) < irq.number() as i16 + } else { + false + } + } else { + // There's no safe way I know of to maintain `!Send` state across invocations of HardFault or NMI, so that should be fine. + false + } + } + VectActive::Interrupt { irqn } => { + #[derive(Clone, Copy)] + struct NrWrap(u16); + unsafe impl cortex_m::interrupt::InterruptNumber for NrWrap { + fn number(self) -> u16 { + self.0 + } + } + let current_prio = NVIC::get_priority(NrWrap(irqn.into())); + let irq_prio = irq.get_priority().into(); + if current_prio < irq_prio { + true + } else if current_prio == irq_prio { + // When multiple interrupts have the same priority number, + // the pending interrupt with the lowest interrupt number takes precedence. + (irqn as u16) < irq.number() + } else { + false + } + } + } +} + impl PeripheralMutex { pub fn new(state: S, irq: S::Interrupt) -> Self { + if can_be_preempted(&irq) { + panic!("`PeripheralMutex` cannot be created in an interrupt with higher priority than the interrupt it wraps"); + } + Self { irq, irq_setup_done: false, @@ -70,17 +142,13 @@ impl PeripheralMutex { this.irq.disable(); this.irq.set_handler(|p| { - critical_section::with(|_| { - if p.is_null() { - // The state was dropped, so we can't operate on it. - return; - } - // Safety: it's OK to get a &mut to the state, since - // - We're in a critical section, no one can preempt us (and call with()) - // - We can't have preempted a with() call because the irq is disabled during it. - let state = unsafe { &mut *(p as *mut S) }; - state.on_interrupt(); - }) + // Safety: it's OK to get a &mut to the state, since + // - We checked that the thread owning the `PeripheralMutex` can't preempt us in `new`. + // Interrupts' priorities can only be changed with raw embassy `Interrupts`, + // which can't safely store a `PeripheralMutex` across invocations. + // - We can't have preempted a with() call because the irq is disabled during it. + let state = unsafe { &mut *(p as *mut S) }; + state.on_interrupt(); }); this.irq .set_handler_context((&mut this.state) as *mut _ as *mut ()); @@ -89,27 +157,63 @@ impl PeripheralMutex { this.irq_setup_done = true; } - pub fn with(self: Pin<&mut Self>, f: impl FnOnce(&mut S, &mut S::Interrupt) -> R) -> R { + pub fn with(self: Pin<&mut Self>, f: impl FnOnce(&mut S) -> R) -> R { let this = unsafe { self.get_unchecked_mut() }; + let was_enabled = this.irq.is_enabled(); this.irq.disable(); // Safety: it's OK to get a &mut to the state, since the irq is disabled. let state = unsafe { &mut *this.state.get() }; - let r = f(state, &mut this.irq); + let r = f(state); - this.irq.enable(); + if was_enabled { + this.irq.enable(); + } r } + + /// Enables the wrapped interrupt. + pub fn enable(&self) { + // This is fine to do before initialization, because we haven't set the handler yet. + self.irq.enable() + } + + /// Disables the wrapped interrupt. + pub fn disable(&self) { + self.irq.disable() + } + + /// Returns whether the wrapped interrupt is enabled. + pub fn is_enabled(&self) -> bool { + self.irq.is_enabled() + } + + /// Returns whether the wrapped interrupt is currently in a pending state. + pub fn is_pending(&self) -> bool { + self.irq.is_pending() + } + + /// Forces the wrapped interrupt into a pending state. + pub fn pend(&self) { + self.irq.pend() + } + + /// Forces the wrapped interrupt out of a pending state. + pub fn unpend(&self) { + self.irq.unpend() + } + + /// Gets the priority of the wrapped interrupt. + pub fn priority(&self) -> ::Priority { + self.irq.get_priority() + } } impl Drop for PeripheralMutex { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); - // Set the context to null so that the interrupt will know we're dropped - // if we pre-empted it before it entered a critical section. - self.irq.set_handler_context(ptr::null_mut()); } } diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index 5961441e0..ae9ae6935 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -1,9 +1,10 @@ use core::marker::{PhantomData, PhantomPinned}; use core::pin::Pin; -use core::ptr; use embassy::interrupt::{Interrupt, InterruptExt}; +use crate::peripheral::can_be_preempted; + /// A version of `PeripheralState` without the `'static` bound, /// for cases where the compiler can't statically make sure /// that `on_interrupt` doesn't reference anything which might be invalidated. @@ -39,6 +40,10 @@ pub struct Peripheral { impl Peripheral { pub fn new(irq: S::Interrupt, state: S) -> Self { + if can_be_preempted(&irq) { + panic!("`Peripheral` cannot be created in an interrupt with higher priority than the interrupt it wraps"); + } + Self { irq, irq_setup_done: false, @@ -57,16 +62,11 @@ impl Peripheral { this.irq.disable(); this.irq.set_handler(|p| { - // We need to be in a critical section so that no one can preempt us - // and drop the state after we check whether `p.is_null()`. - critical_section::with(|_| { - if p.is_null() { - // The state was dropped, so we can't operate on it. - return; - } - let state = unsafe { &*(p as *const S) }; - state.on_interrupt(); - }); + // The state can't have been dropped, otherwise the interrupt would have been disabled. + // We checked in `new` that the thread owning the `Peripheral` can't preempt the interrupt, + // so someone can't have preempted us before this point and dropped the `Peripheral`. + let state = unsafe { &*(p as *const S) }; + state.on_interrupt(); }); this.irq .set_handler_context((&this.state) as *const _ as *mut ()); @@ -78,14 +78,47 @@ impl Peripheral { pub fn state(self: Pin<&mut Self>) -> &S { &self.into_ref().get_ref().state } + + /// Enables the wrapped interrupt. + pub fn enable(&self) { + // This is fine to do before initialization, because we haven't set the handler yet. + self.irq.enable() + } + + /// Disables the wrapped interrupt. + pub fn disable(&self) { + self.irq.disable() + } + + /// Returns whether the wrapped interrupt is enabled. + pub fn is_enabled(&self) -> bool { + self.irq.is_enabled() + } + + /// Returns whether the wrapped interrupt is currently in a pending state. + pub fn is_pending(&self) -> bool { + self.irq.is_pending() + } + + /// Forces the wrapped interrupt into a pending state. + pub fn pend(&self) { + self.irq.pend() + } + + /// Forces the wrapped interrupt out of a pending state. + pub fn unpend(&self) { + self.irq.unpend() + } + + /// Gets the priority of the wrapped interrupt. + pub fn priority(&self) -> ::Priority { + self.irq.get_priority() + } } impl Drop for Peripheral { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); - // Set the context to null so that the interrupt will know we're dropped - // if we pre-empted it before it entered a critical section. - self.irq.set_handler_context(ptr::null_mut()); } } diff --git a/embassy-extras/src/usb/usb_serial.rs b/embassy-extras/src/usb/usb_serial.rs index 9cbfb2da4..a229b2000 100644 --- a/embassy-extras/src/usb/usb_serial.rs +++ b/embassy-extras/src/usb/usb_serial.rs @@ -55,7 +55,7 @@ where let this = self.get_mut(); let mut mutex = this.inner.borrow_mut(); let mutex = unsafe { Pin::new_unchecked(&mut *mutex) }; - mutex.with(|state, _irq| { + mutex.with(|state| { let serial = state.classes.get_serial(); let serial = Pin::new(serial); @@ -77,7 +77,7 @@ where let this = self.get_mut(); let mut mutex = this.inner.borrow_mut(); let mutex = unsafe { Pin::new_unchecked(&mut *mutex) }; - mutex.with(|state, _irq| { + mutex.with(|state| { let serial = state.classes.get_serial(); let serial = Pin::new(serial); @@ -101,7 +101,7 @@ where let this = self.get_mut(); let mut mutex = this.inner.borrow_mut(); let mutex = unsafe { Pin::new_unchecked(&mut *mutex) }; - mutex.with(|state, _irq| { + mutex.with(|state| { let serial = state.classes.get_serial(); let serial = Pin::new(serial); diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index 1fa98a6b2..2cce122bc 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -176,7 +176,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { pub fn set_baudrate(self: Pin<&mut Self>, baudrate: Baudrate) { let mut inner = self.inner(); inner.as_mut().register_interrupt(); - inner.with(|state, _irq| { + inner.with(|state| { let r = U::regs(); let timeout = 0x8000_0000 / (baudrate as u32 / 40); @@ -196,7 +196,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, fn poll_fill_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { let mut inner = self.inner(); inner.as_mut().register_interrupt(); - inner.with(|state, _irq| { + inner.with(|state| { // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here, // before any DMA action has started @@ -221,11 +221,11 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, fn consume(self: Pin<&mut Self>, amt: usize) { let mut inner = self.inner(); inner.as_mut().register_interrupt(); - inner.with(|state, irq| { + inner.as_mut().with(|state| { trace!("consume {:?}", amt); state.rx.pop(amt); - irq.pend(); - }) + }); + inner.pend(); } } @@ -233,7 +233,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncWrite for BufferedUarte<'d, U, fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { let mut inner = self.inner(); inner.as_mut().register_interrupt(); - inner.with(|state, irq| { + let poll = inner.as_mut().with(|state| { trace!("poll_write: {:?}", buf.len()); let tx_buf = state.tx.push_buf(); @@ -254,10 +254,12 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncWrite for BufferedUarte<'d, U, // before any DMA action has started compiler_fence(Ordering::SeqCst); - irq.pend(); - Poll::Ready(Ok(n)) - }) + }); + + inner.pend(); + + poll } } diff --git a/embassy-stm32/src/eth/v2/mod.rs b/embassy-stm32/src/eth/v2/mod.rs index 567088e8d..3ac09f944 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -161,7 +161,7 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { let mut mutex = unsafe { Pin::new_unchecked(&mut this.state) }; mutex.as_mut().register_interrupt(); - mutex.with(|s, _| { + mutex.with(|s| { s.desc_ring.init(); fence(Ordering::SeqCst); @@ -237,7 +237,7 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Device for Pin<&mut Ethernet< let this = unsafe { self.as_mut().get_unchecked_mut() }; let mutex = unsafe { Pin::new_unchecked(&mut this.state) }; - mutex.with(|s, _| s.desc_ring.tx.available()) + mutex.with(|s| s.desc_ring.tx.available()) } fn transmit(&mut self, pkt: PacketBuf) { @@ -245,7 +245,7 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Device for Pin<&mut Ethernet< let this = unsafe { self.as_mut().get_unchecked_mut() }; let mutex = unsafe { Pin::new_unchecked(&mut this.state) }; - mutex.with(|s, _| unwrap!(s.desc_ring.tx.transmit(pkt))); + mutex.with(|s| unwrap!(s.desc_ring.tx.transmit(pkt))); } fn receive(&mut self) -> Option { @@ -253,7 +253,7 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Device for Pin<&mut Ethernet< let this = unsafe { self.as_mut().get_unchecked_mut() }; let mutex = unsafe { Pin::new_unchecked(&mut this.state) }; - mutex.with(|s, _| s.desc_ring.rx.pop_packet()) + mutex.with(|s| s.desc_ring.rx.pop_packet()) } fn register_waker(&mut self, waker: &Waker) { From e57ca5f7dbd4ea5d3d5209e4b674841a19fe022d Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Tue, 27 Jul 2021 17:30:10 +1000 Subject: [PATCH 07/12] Remove `critical-section` dependency --- embassy-extras/Cargo.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/embassy-extras/Cargo.toml b/embassy-extras/Cargo.toml index 8415e32ef..5d07901a9 100644 --- a/embassy-extras/Cargo.toml +++ b/embassy-extras/Cargo.toml @@ -17,5 +17,4 @@ embassy = { version = "0.1.0", path = "../embassy" } defmt = { version = "0.2.0", optional = true } log = { version = "0.4.11", optional = true } cortex-m = "0.7.1" -critical-section = "0.2.1" usb-device = "0.2.7" From a6fea3cb28f717627bd8858016622270c3e76721 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Tue, 27 Jul 2021 17:40:13 +1000 Subject: [PATCH 08/12] Fix `#[cfg]`s in `exception_to_system_handler` --- embassy-extras/build.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 embassy-extras/build.rs diff --git a/embassy-extras/build.rs b/embassy-extras/build.rs new file mode 100644 index 000000000..e3388da26 --- /dev/null +++ b/embassy-extras/build.rs @@ -0,0 +1,11 @@ +use std::env; + +fn main() { + let target = env::var("TARGET").unwrap(); + + if target.starts_with("thumbv6m-") { + println!("cargo:rustc-cfg=armv6m"); + } else if target.starts_with("thumbv8m.") { + println!("cargo:rustc-cfg=armv8m"); + } +} From 68c93256bcab8dfe0f65c694fa5fadb890fd3f00 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Wed, 28 Jul 2021 21:31:31 +1000 Subject: [PATCH 09/12] fix: interrupts with equal priority can't preempt each other --- embassy-extras/src/peripheral.rs | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index 2402ba9eb..396ab5432 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -21,7 +21,7 @@ pub unsafe trait PeripheralStateUnchecked: Send { /// A type which can be used as state with `PeripheralMutex`. /// /// It needs to be `Send` because `&mut` references are sent back and forth between the 'thread' which owns the `PeripheralMutex` and the interrupt, -/// and `&mut T` is `Send` where `T: Send`. +/// and `&mut T` is only `Send` where `T: Send`. /// /// It also requires `'static`, because although `Pin` guarantees that the memory of the state won't be invalidated, /// it doesn't guarantee that the lifetime will last. @@ -73,23 +73,13 @@ fn exception_to_system_handler(exception: Exception) -> Option { /// Whether `irq` can be preempted by the current interrupt. pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool { match SCB::vect_active() { - // Thread mode can't preempt each other + // Thread mode can't preempt anything VectActive::ThreadMode => false, VectActive::Exception(exception) => { // `SystemHandler` is a subset of `Exception` for those with configurable priority. // There's no built in way to convert between them, so `exception_to_system_handler` was necessary. if let Some(system_handler) = exception_to_system_handler(exception) { - let current_prio = SCB::get_priority(system_handler); - let irq_prio = irq.get_priority().into(); - if current_prio < irq_prio { - true - } else if current_prio == irq_prio { - // When multiple interrupts have the same priority number, - // the pending interrupt with the lowest interrupt number takes precedence. - (exception.irqn() as i16) < irq.number() as i16 - } else { - false - } + SCB::get_priority(system_handler) < irq.get_priority().into() } else { // There's no safe way I know of to maintain `!Send` state across invocations of HardFault or NMI, so that should be fine. false @@ -103,17 +93,7 @@ pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool { self.0 } } - let current_prio = NVIC::get_priority(NrWrap(irqn.into())); - let irq_prio = irq.get_priority().into(); - if current_prio < irq_prio { - true - } else if current_prio == irq_prio { - // When multiple interrupts have the same priority number, - // the pending interrupt with the lowest interrupt number takes precedence. - (irqn as u16) < irq.number() - } else { - false - } + NVIC::get_priority(NrWrap(irqn.into())) < irq.get_priority().into() } } } From 4d9514cbcb97343c3a75bfa565d753c44c2a0e27 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Wed, 28 Jul 2021 21:39:31 +1000 Subject: [PATCH 10/12] Don't allow disabling interrupts wrapped by `PeripheralMutex` --- embassy-extras/src/peripheral.rs | 21 +-------------------- embassy-extras/src/peripheral_shared.rs | 16 ---------------- 2 files changed, 1 insertion(+), 36 deletions(-) diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index 396ab5432..725a58a46 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -140,36 +140,17 @@ impl PeripheralMutex { pub fn with(self: Pin<&mut Self>, f: impl FnOnce(&mut S) -> R) -> R { let this = unsafe { self.get_unchecked_mut() }; - let was_enabled = this.irq.is_enabled(); this.irq.disable(); // Safety: it's OK to get a &mut to the state, since the irq is disabled. let state = unsafe { &mut *this.state.get() }; let r = f(state); - if was_enabled { - this.irq.enable(); - } + this.irq.enable(); r } - /// Enables the wrapped interrupt. - pub fn enable(&self) { - // This is fine to do before initialization, because we haven't set the handler yet. - self.irq.enable() - } - - /// Disables the wrapped interrupt. - pub fn disable(&self) { - self.irq.disable() - } - - /// Returns whether the wrapped interrupt is enabled. - pub fn is_enabled(&self) -> bool { - self.irq.is_enabled() - } - /// Returns whether the wrapped interrupt is currently in a pending state. pub fn is_pending(&self) -> bool { self.irq.is_pending() diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index ae9ae6935..788ac3f96 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -79,22 +79,6 @@ impl Peripheral { &self.into_ref().get_ref().state } - /// Enables the wrapped interrupt. - pub fn enable(&self) { - // This is fine to do before initialization, because we haven't set the handler yet. - self.irq.enable() - } - - /// Disables the wrapped interrupt. - pub fn disable(&self) { - self.irq.disable() - } - - /// Returns whether the wrapped interrupt is enabled. - pub fn is_enabled(&self) -> bool { - self.irq.is_enabled() - } - /// Returns whether the wrapped interrupt is currently in a pending state. pub fn is_pending(&self) -> bool { self.irq.is_pending() From d5ba35424d7eef2cc0c501758d214ce3a6febfc1 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Thu, 29 Jul 2021 15:11:26 +1000 Subject: [PATCH 11/12] Replace `PeripheralStateUnchecked` with `register_interrupt_unchecked` --- embassy-extras/src/peripheral.rs | 66 ++++++++++++++----------- embassy-extras/src/peripheral_shared.rs | 52 ++++++++++++------- embassy-extras/src/usb/mod.rs | 9 ++-- embassy-nrf/src/buffered_uarte.rs | 13 +++-- embassy-stm32/src/eth/v2/mod.rs | 8 +-- 5 files changed, 84 insertions(+), 64 deletions(-) diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index 725a58a46..1868edd7d 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -6,42 +6,20 @@ use cortex_m::peripheral::scb::{Exception, SystemHandler, VectActive}; use cortex_m::peripheral::{NVIC, SCB}; use embassy::interrupt::{Interrupt, InterruptExt}; -/// A version of `PeripheralState` without the `'static` bound, -/// for cases where the compiler can't statically make sure -/// that `on_interrupt` doesn't reference anything which might be invalidated. -/// -/// # Safety -/// When types implementing this trait are used with `PeripheralMutex`, -/// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `PeripheralMutex`. -pub unsafe trait PeripheralStateUnchecked: Send { - type Interrupt: Interrupt; - fn on_interrupt(&mut self); -} - /// A type which can be used as state with `PeripheralMutex`. /// /// It needs to be `Send` because `&mut` references are sent back and forth between the 'thread' which owns the `PeripheralMutex` and the interrupt, /// and `&mut T` is only `Send` where `T: Send`. /// -/// It also requires `'static`, because although `Pin` guarantees that the memory of the state won't be invalidated, +/// It also requires `'static` to be used safely with `PeripheralMutex::register_interrupt`, +/// because although `Pin` guarantees that the memory of the state won't be invalidated, /// it doesn't guarantee that the lifetime will last. -pub trait PeripheralState: Send + 'static { +pub trait PeripheralState: Send { type Interrupt: Interrupt; fn on_interrupt(&mut self); } -// SAFETY: `T` has to live for `'static` to implement `PeripheralState`, thus its lifetime cannot end. -unsafe impl PeripheralStateUnchecked for T -where - T: PeripheralState, -{ - type Interrupt = T::Interrupt; - fn on_interrupt(&mut self) { - self.on_interrupt() - } -} - -pub struct PeripheralMutex { +pub struct PeripheralMutex { state: UnsafeCell, irq_setup_done: bool, @@ -98,7 +76,25 @@ pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool { } } -impl PeripheralMutex { +impl PeripheralMutex { + /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it. + /// + /// This requires this `PeripheralMutex`'s `PeripheralState` to live for `'static`, + /// because `Pin` only guarantees that it's memory won't be repurposed, + /// not that it's lifetime will last. + /// + /// To use non-`'static` `PeripheralState`, use the unsafe `register_interrupt_unchecked`. + /// + /// Note: `'static` doesn't mean it _has_ to live for the entire program, like an `&'static T`; + /// it just means it _can_ live for the entire program - for example, `u8` lives for `'static`. + pub fn register_interrupt(self: Pin<&mut Self>) { + // SAFETY: `S: 'static`, so there's no way it's lifetime can expire. + unsafe { self.register_interrupt_unchecked() } + } +} + +impl PeripheralMutex { + /// Create a new `PeripheralMutex` wrapping `irq`, with the initial state `state`. pub fn new(state: S, irq: S::Interrupt) -> Self { if can_be_preempted(&irq) { panic!("`PeripheralMutex` cannot be created in an interrupt with higher priority than the interrupt it wraps"); @@ -114,8 +110,18 @@ impl PeripheralMutex { } } - pub fn register_interrupt(self: Pin<&mut Self>) { - let this = unsafe { self.get_unchecked_mut() }; + /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it. + /// + /// # Safety + /// The lifetime of any data in `PeripheralState` that is accessed by the interrupt handler + /// must not end without `Drop` being called on this `PeripheralMutex`. + /// + /// This can be accomplished by either not accessing any data with a lifetime in `on_interrupt`, + /// or making sure that nothing like `mem::forget` is used on the `PeripheralMutex`. + + // TODO: this name isn't the best. + pub unsafe fn register_interrupt_unchecked(self: Pin<&mut Self>) { + let this = self.get_unchecked_mut(); if this.irq_setup_done { return; } @@ -172,7 +178,7 @@ impl PeripheralMutex { } } -impl Drop for PeripheralMutex { +impl Drop for PeripheralMutex { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs index 788ac3f96..71d746341 100644 --- a/embassy-extras/src/peripheral_shared.rs +++ b/embassy-extras/src/peripheral_shared.rs @@ -5,30 +5,19 @@ use embassy::interrupt::{Interrupt, InterruptExt}; use crate::peripheral::can_be_preempted; -/// A version of `PeripheralState` without the `'static` bound, -/// for cases where the compiler can't statically make sure -/// that `on_interrupt` doesn't reference anything which might be invalidated. -/// -/// # Safety -/// When types implementing this trait are used with `Peripheral`, -/// no fields referenced by `on_interrupt`'s lifetimes must end without first calling `Drop` on the `Peripheral`. -pub unsafe trait PeripheralStateUnchecked: Sync { - type Interrupt: Interrupt; - fn on_interrupt(&self); -} - /// A type which can be used as state with `Peripheral`. /// /// It needs to be `Sync` because references are shared between the 'thread' which owns the `Peripheral` and the interrupt. /// -/// It also requires `'static`, because although `Pin` guarantees that the memory of the state won't be invalidated, +/// It also requires `'static` to be used safely with `Peripheral::register_interrupt`, +/// because although `Pin` guarantees that the memory of the state won't be invalidated, /// it doesn't guarantee that the lifetime will last. -pub trait PeripheralState: Sync + 'static { +pub trait PeripheralState: Sync { type Interrupt: Interrupt; fn on_interrupt(&self); } -pub struct Peripheral { +pub struct Peripheral { state: S, irq_setup_done: bool, @@ -38,7 +27,24 @@ pub struct Peripheral { _pinned: PhantomPinned, } -impl Peripheral { +impl Peripheral { + /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it. + /// + /// This requires this `Peripheral`'s `PeripheralState` to live for `'static`, + /// because `Pin` only guarantees that it's memory won't be repurposed, + /// not that it's lifetime will last. + /// + /// To use non-`'static` `PeripheralState`, use the unsafe `register_interrupt_unchecked`. + /// + /// Note: `'static` doesn't mean it _has_ to live for the entire program, like an `&'static T`; + /// it just means it _can_ live for the entire program - for example, `u8` lives for `'static`. + pub fn register_interrupt(self: Pin<&mut Self>) { + // SAFETY: `S: 'static`, so there's no way it's lifetime can expire. + unsafe { self.register_interrupt_unchecked() } + } +} + +impl Peripheral { pub fn new(irq: S::Interrupt, state: S) -> Self { if can_be_preempted(&irq) { panic!("`Peripheral` cannot be created in an interrupt with higher priority than the interrupt it wraps"); @@ -54,8 +60,16 @@ impl Peripheral { } } - pub fn register_interrupt(self: Pin<&mut Self>) { - let this = unsafe { self.get_unchecked_mut() }; + /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it. + /// + /// # Safety + /// The lifetime of any data in `PeripheralState` that is accessed by the interrupt handler + /// must not end without `Drop` being called on this `Peripheral`. + /// + /// This can be accomplished by either not accessing any data with a lifetime in `on_interrupt`, + /// or making sure that nothing like `mem::forget` is used on the `Peripheral`. + pub unsafe fn register_interrupt_unchecked(self: Pin<&mut Self>) { + let this = self.get_unchecked_mut(); if this.irq_setup_done { return; } @@ -100,7 +114,7 @@ impl Peripheral { } } -impl Drop for Peripheral { +impl Drop for Peripheral { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); diff --git a/embassy-extras/src/usb/mod.rs b/embassy-extras/src/usb/mod.rs index 481987a6c..1fb501d7f 100644 --- a/embassy-extras/src/usb/mod.rs +++ b/embassy-extras/src/usb/mod.rs @@ -9,7 +9,7 @@ use usb_device::device::UsbDevice; mod cdc_acm; pub mod usb_serial; -use crate::peripheral::{PeripheralMutex, PeripheralStateUnchecked}; +use crate::peripheral::{PeripheralMutex, PeripheralState}; use embassy::interrupt::Interrupt; use usb_serial::{ReadInterface, UsbSerial, WriteInterface}; @@ -63,7 +63,9 @@ where let mutex = Pin::new_unchecked(&mut *mutex); // Use inner to register the irq - mutex.register_interrupt(); + // SAFETY: the safety contract of this function makes sure the `UsbDevice` won't be invalidated + // without the `PeripheralMutex` being dropped. + mutex.register_interrupt_unchecked(); } } @@ -127,8 +129,7 @@ where } } -// SAFETY: The safety contract of `PeripheralStateUnchecked` is forwarded to `Usb::start`. -unsafe impl<'bus, B, T, I> PeripheralStateUnchecked for State<'bus, B, T, I> +impl<'bus, B, T, I> PeripheralState for State<'bus, B, T, I> where B: UsbBus, T: ClassSet, diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index 2cce122bc..9be4d4d54 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -7,7 +7,7 @@ use core::task::{Context, Poll}; use embassy::interrupt::InterruptExt; use embassy::io::{AsyncBufRead, AsyncWrite, Result}; use embassy::util::{Unborrow, WakerRegistration}; -use embassy_extras::peripheral::{PeripheralMutex, PeripheralStateUnchecked}; +use embassy_extras::peripheral::{PeripheralMutex, PeripheralState}; use embassy_extras::ring_buffer::RingBuffer; use embassy_extras::{low_power_wait_until, unborrow}; @@ -175,7 +175,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { pub fn set_baudrate(self: Pin<&mut Self>, baudrate: Baudrate) { let mut inner = self.inner(); - inner.as_mut().register_interrupt(); + unsafe { inner.as_mut().register_interrupt_unchecked() } inner.with(|state| { let r = U::regs(); @@ -195,7 +195,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, U, T> { fn poll_fill_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { let mut inner = self.inner(); - inner.as_mut().register_interrupt(); + unsafe { inner.as_mut().register_interrupt_unchecked() } inner.with(|state| { // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here, @@ -220,7 +220,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, fn consume(self: Pin<&mut Self>, amt: usize) { let mut inner = self.inner(); - inner.as_mut().register_interrupt(); + unsafe { inner.as_mut().register_interrupt_unchecked() } inner.as_mut().with(|state| { trace!("consume {:?}", amt); state.rx.pop(amt); @@ -232,7 +232,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, impl<'d, U: UarteInstance, T: TimerInstance> AsyncWrite for BufferedUarte<'d, U, T> { fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { let mut inner = self.inner(); - inner.as_mut().register_interrupt(); + unsafe { inner.as_mut().register_interrupt_unchecked() } let poll = inner.as_mut().with(|state| { trace!("poll_write: {:?}", buf.len()); @@ -285,8 +285,7 @@ impl<'a, U: UarteInstance, T: TimerInstance> Drop for State<'a, U, T> { } } -// SAFETY: the safety contract of `PeripheralStateUnchecked` is forwarded to `BufferedUarte::new`. -unsafe impl<'a, U: UarteInstance, T: TimerInstance> PeripheralStateUnchecked for State<'a, U, T> { +impl<'a, U: UarteInstance, T: TimerInstance> PeripheralState for State<'a, U, T> { type Interrupt = U::Interrupt; fn on_interrupt(&mut self) { trace!("irq: start"); diff --git a/embassy-stm32/src/eth/v2/mod.rs b/embassy-stm32/src/eth/v2/mod.rs index 3ac09f944..129d2d02c 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -4,7 +4,7 @@ use core::sync::atomic::{fence, Ordering}; use core::task::Waker; use embassy::util::{AtomicWaker, Unborrow}; -use embassy_extras::peripheral::{PeripheralMutex, PeripheralStateUnchecked}; +use embassy_extras::peripheral::{PeripheralMutex, PeripheralState}; use embassy_extras::unborrow; use embassy_net::{Device, DeviceCapabilities, LinkState, PacketBuf, MTU}; @@ -159,7 +159,8 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { // NOTE(unsafe) We won't move this let this = unsafe { self.get_unchecked_mut() }; let mut mutex = unsafe { Pin::new_unchecked(&mut this.state) }; - mutex.as_mut().register_interrupt(); + // SAFETY: The lifetime of `Inner` is only due to `PhantomData`; it isn't actually referencing any data with that lifetime. + unsafe { mutex.as_mut().register_interrupt_unchecked() } mutex.with(|s| { s.desc_ring.init(); @@ -343,8 +344,7 @@ impl<'d, const TX: usize, const RX: usize> Inner<'d, TX, RX> { } } -// SAFETY: The lifetime of `Inner` is only due to `PhantomData`; it isn't actually referencing any data with that lifetime. -unsafe impl<'d, const TX: usize, const RX: usize> PeripheralStateUnchecked for Inner<'d, TX, RX> { +impl<'d, const TX: usize, const RX: usize> PeripheralState for Inner<'d, TX, RX> { type Interrupt = crate::interrupt::ETH; fn on_interrupt(&mut self) { From cd1a3fcff34943117f446e1afeb9e6d531ee577b Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Thu, 29 Jul 2021 15:19:57 +1000 Subject: [PATCH 12/12] Don't bother supporting creating a `PeripheralMutex` in an exception handler --- embassy-extras/build.rs | 11 ---------- embassy-extras/src/peripheral.rs | 36 +++++--------------------------- 2 files changed, 5 insertions(+), 42 deletions(-) delete mode 100644 embassy-extras/build.rs diff --git a/embassy-extras/build.rs b/embassy-extras/build.rs deleted file mode 100644 index e3388da26..000000000 --- a/embassy-extras/build.rs +++ /dev/null @@ -1,11 +0,0 @@ -use std::env; - -fn main() { - let target = env::var("TARGET").unwrap(); - - if target.starts_with("thumbv6m-") { - println!("cargo:rustc-cfg=armv6m"); - } else if target.starts_with("thumbv8m.") { - println!("cargo:rustc-cfg=armv8m"); - } -} diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index 1868edd7d..92512a0f6 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -2,7 +2,7 @@ use core::cell::UnsafeCell; use core::marker::{PhantomData, PhantomPinned}; use core::pin::Pin; -use cortex_m::peripheral::scb::{Exception, SystemHandler, VectActive}; +use cortex_m::peripheral::scb::VectActive; use cortex_m::peripheral::{NVIC, SCB}; use embassy::interrupt::{Interrupt, InterruptExt}; @@ -29,40 +29,14 @@ pub struct PeripheralMutex { _pinned: PhantomPinned, } -fn exception_to_system_handler(exception: Exception) -> Option { - match exception { - Exception::NonMaskableInt | Exception::HardFault => None, - #[cfg(not(armv6m))] - Exception::MemoryManagement => Some(SystemHandler::MemoryManagement), - #[cfg(not(armv6m))] - Exception::BusFault => Some(SystemHandler::BusFault), - #[cfg(not(armv6m))] - Exception::UsageFault => Some(SystemHandler::UsageFault), - #[cfg(any(armv8m, target_arch = "x86_64"))] - Exception::SecureFault => Some(SystemHandler::SecureFault), - Exception::SVCall => Some(SystemHandler::SVCall), - #[cfg(not(armv6m))] - Exception::DebugMonitor => Some(SystemHandler::DebugMonitor), - Exception::PendSV => Some(SystemHandler::PendSV), - Exception::SysTick => Some(SystemHandler::SysTick), - } -} - /// Whether `irq` can be preempted by the current interrupt. pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool { match SCB::vect_active() { - // Thread mode can't preempt anything + // Thread mode can't preempt anything. VectActive::ThreadMode => false, - VectActive::Exception(exception) => { - // `SystemHandler` is a subset of `Exception` for those with configurable priority. - // There's no built in way to convert between them, so `exception_to_system_handler` was necessary. - if let Some(system_handler) = exception_to_system_handler(exception) { - SCB::get_priority(system_handler) < irq.get_priority().into() - } else { - // There's no safe way I know of to maintain `!Send` state across invocations of HardFault or NMI, so that should be fine. - false - } - } + // Exceptions don't always preempt interrupts, + // but there isn't much of a good reason to be keeping a `PeripheralMutex` in an exception anyway. + VectActive::Exception(_) => true, VectActive::Interrupt { irqn } => { #[derive(Clone, Copy)] struct NrWrap(u16);