From d5ba35424d7eef2cc0c501758d214ce3a6febfc1 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Thu, 29 Jul 2021 15:11:26 +1000 Subject: [PATCH] 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) {