From af87031d62ca9ee5e7dd44cba297f3d171ec0708 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 29 Jul 2021 14:08:32 +0200 Subject: [PATCH 1/4] hal-common: remove Pin in PeripheralMutex --- embassy-hal-common/src/peripheral.rs | 130 +++++++++++------------ embassy-hal-common/src/usb/mod.rs | 49 +++++---- embassy-hal-common/src/usb/usb_serial.rs | 10 +- embassy-nrf/src/buffered_uarte.rs | 94 ++++++++-------- embassy-stm32/src/eth/v2/mod.rs | 64 ++++------- examples/nrf/src/bin/buffered_uart.rs | 3 + examples/stm32h7/src/bin/eth.rs | 18 ++-- 7 files changed, 171 insertions(+), 197 deletions(-) diff --git a/embassy-hal-common/src/peripheral.rs b/embassy-hal-common/src/peripheral.rs index 92512a0f..d3df06e8 100644 --- a/embassy-hal-common/src/peripheral.rs +++ b/embassy-hal-common/src/peripheral.rs @@ -1,6 +1,5 @@ -use core::cell::UnsafeCell; -use core::marker::{PhantomData, PhantomPinned}; -use core::pin::Pin; +use core::marker::PhantomData; +use core::mem::MaybeUninit; use cortex_m::peripheral::scb::VectActive; use cortex_m::peripheral::{NVIC, SCB}; @@ -10,23 +9,23 @@ use embassy::interrupt::{Interrupt, InterruptExt}; /// /// 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` 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 { type Interrupt: Interrupt; fn on_interrupt(&mut self); } -pub struct PeripheralMutex { - state: UnsafeCell, +pub struct StateStorage(MaybeUninit); - irq_setup_done: bool, +impl StateStorage { + pub fn new() -> Self { + Self(MaybeUninit::uninit()) + } +} + +pub struct PeripheralMutex<'a, S: PeripheralState> { + state: *mut S, + _phantom: PhantomData<&'a mut S>, irq: S::Interrupt, - - _not_send: PhantomData<*mut ()>, - _pinned: PhantomPinned, } /// Whether `irq` can be preempted by the current interrupt. @@ -50,58 +49,45 @@ pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool { } } -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 { +impl<'a, S: PeripheralState> PeripheralMutex<'a, S> { /// Create a new `PeripheralMutex` wrapping `irq`, with the initial state `state`. - pub fn new(state: S, irq: S::Interrupt) -> Self { + /// + /// self requires `state` to live for `'static`, because if the `PeripheralMutex` is leaked, the + /// interrupt won't be disabled, which may try accessing the state at any time. To use non-`'static` + /// state, see [`Self::new_unchecked`]. + /// + /// Registers `on_interrupt` as the `irq`'s handler, and enables it. + pub fn new(storage: &'a mut StateStorage, state: S, irq: S::Interrupt) -> Self + where + 'a: 'static, + { + // safety: safe because state is `'static`. + unsafe { Self::new_unchecked(storage, state, irq) } + } + + /// Create a `PeripheralMutex` without requiring the state is `'static`. + /// + /// See also [`Self::new`]. + /// + /// # Safety + /// The created instance must not be leaked (its `drop` must run). + pub unsafe fn new_unchecked( + storage: &'a mut StateStorage, + 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, + let state_ptr = storage.0.as_mut_ptr(); - state: UnsafeCell::new(state), - _not_send: PhantomData, - _pinned: PhantomPinned, - } - } + // Safety: The pointer is valid and not used by anyone else + // because we have the `&mut StateStorage`. + state_ptr.write(state); - /// 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; - } - - this.irq.disable(); - this.irq.set_handler(|p| { + irq.disable(); + irq.set_handler(|p| { // 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`, @@ -110,23 +96,24 @@ impl PeripheralMutex { let state = unsafe { &mut *(p as *mut S) }; state.on_interrupt(); }); - this.irq - .set_handler_context((&mut this.state) as *mut _ as *mut ()); - this.irq.enable(); + irq.set_handler_context(state_ptr as *mut ()); + irq.enable(); - this.irq_setup_done = true; + Self { + irq, + state: state_ptr, + _phantom: PhantomData, + } } - pub fn with(self: Pin<&mut Self>, f: impl FnOnce(&mut S) -> R) -> R { - let this = unsafe { self.get_unchecked_mut() }; - - this.irq.disable(); + pub fn with(&mut self, f: impl FnOnce(&mut S) -> R) -> R { + self.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 state = unsafe { &mut *self.state }; let r = f(state); - this.irq.enable(); + self.irq.enable(); r } @@ -152,9 +139,14 @@ impl PeripheralMutex { } } -impl Drop for PeripheralMutex { +impl<'a, S: PeripheralState> Drop for PeripheralMutex<'a, S> { fn drop(&mut self) { self.irq.disable(); self.irq.remove_handler(); + + // safety: + // - we initialized the state in `new`, so we know it's initialized. + // - the irq is disabled, so it won't preempt us while dropping. + unsafe { self.state.drop_in_place() } } } diff --git a/embassy-hal-common/src/usb/mod.rs b/embassy-hal-common/src/usb/mod.rs index 1fb501d7..0940e6b0 100644 --- a/embassy-hal-common/src/usb/mod.rs +++ b/embassy-hal-common/src/usb/mod.rs @@ -9,14 +9,31 @@ use usb_device::device::UsbDevice; mod cdc_acm; pub mod usb_serial; -use crate::peripheral::{PeripheralMutex, PeripheralState}; +use crate::peripheral::{PeripheralMutex, PeripheralState, StateStorage}; 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 + Send {} -pub(crate) struct State<'bus, B, T, I> +pub struct State<'bus, B, T, I>(StateStorage>) +where + B: UsbBus, + T: ClassSet, + I: USBInterrupt; + +impl<'bus, B, T, I> State<'bus, B, T, I> +where + B: UsbBus, + T: ClassSet, + I: USBInterrupt, +{ + pub fn new() -> Self { + Self(StateStorage::new()) + } +} + +pub(crate) struct StateInner<'bus, B, T, I> where B: UsbBus, T: ClassSet, @@ -34,7 +51,7 @@ where I: USBInterrupt, { // Don't you dare moving out `PeripheralMutex` - inner: RefCell>>, + inner: RefCell>>, } impl<'bus, B, T, I> Usb<'bus, B, T, I> @@ -43,30 +60,22 @@ where T: ClassSet, I: USBInterrupt, { - pub fn new>(device: UsbDevice<'bus, B>, class_set: S, irq: I) -> Self { - let state = State { + pub fn new>( + state: &'bus mut State<'bus, B, T, I>, + device: UsbDevice<'bus, B>, + class_set: S, + irq: I, + ) -> Self { + let initial_state = StateInner { device, classes: class_set.into_class_set(), _interrupt: PhantomData, }; - let mutex = PeripheralMutex::new(state, irq); + let mutex = unsafe { PeripheralMutex::new_unchecked(&mut state.0, initial_state, irq) }; Self { inner: RefCell::new(mutex), } } - - /// # 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 = Pin::new_unchecked(&mut *mutex); - - // Use inner to register the irq - // SAFETY: the safety contract of this function makes sure the `UsbDevice` won't be invalidated - // without the `PeripheralMutex` being dropped. - mutex.register_interrupt_unchecked(); - } } impl<'bus, 'c, B, T, I> Usb<'bus, B, T, I> @@ -129,7 +138,7 @@ where } } -impl<'bus, B, T, I> PeripheralState for State<'bus, B, T, I> +impl<'bus, B, T, I> PeripheralState for StateInner<'bus, B, T, I> where B: UsbBus, T: ClassSet, diff --git a/embassy-hal-common/src/usb/usb_serial.rs b/embassy-hal-common/src/usb/usb_serial.rs index a229b200..8b27152b 100644 --- a/embassy-hal-common/src/usb/usb_serial.rs +++ b/embassy-hal-common/src/usb/usb_serial.rs @@ -10,9 +10,10 @@ use usb_device::class_prelude::*; use usb_device::UsbError; use super::cdc_acm::CdcAcmClass; +use super::StateInner; use crate::peripheral::PeripheralMutex; use crate::ring_buffer::RingBuffer; -use crate::usb::{ClassSet, SerialState, State, USBInterrupt}; +use crate::usb::{ClassSet, SerialState, USBInterrupt}; pub struct ReadInterface<'a, 'bus, 'c, I, B, T, INT> where @@ -22,7 +23,7 @@ where INT: USBInterrupt, { // Don't you dare moving out `PeripheralMutex` - pub(crate) inner: &'a RefCell>>, + pub(crate) inner: &'a RefCell>>, pub(crate) _buf_lifetime: PhantomData<&'c T>, pub(crate) _index: PhantomData, } @@ -39,7 +40,7 @@ where INT: USBInterrupt, { // Don't you dare moving out `PeripheralMutex` - pub(crate) inner: &'a RefCell>>, + pub(crate) inner: &'a RefCell>>, pub(crate) _buf_lifetime: PhantomData<&'c T>, pub(crate) _index: PhantomData, } @@ -54,7 +55,6 @@ where fn poll_fill_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { let this = self.get_mut(); let mut mutex = this.inner.borrow_mut(); - let mutex = unsafe { Pin::new_unchecked(&mut *mutex) }; mutex.with(|state| { let serial = state.classes.get_serial(); let serial = Pin::new(serial); @@ -76,7 +76,6 @@ where fn consume(self: Pin<&mut Self>, amt: usize) { let this = self.get_mut(); let mut mutex = this.inner.borrow_mut(); - let mutex = unsafe { Pin::new_unchecked(&mut *mutex) }; mutex.with(|state| { let serial = state.classes.get_serial(); let serial = Pin::new(serial); @@ -100,7 +99,6 @@ where ) -> Poll> { let this = self.get_mut(); let mut mutex = this.inner.borrow_mut(); - let mutex = unsafe { Pin::new_unchecked(&mut *mutex) }; 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 d6120bd0..642c3018 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_hal_common::peripheral::{PeripheralMutex, PeripheralState}; +use embassy_hal_common::peripheral::{PeripheralMutex, PeripheralState, StateStorage}; use embassy_hal_common::ring_buffer::RingBuffer; use embassy_hal_common::{low_power_wait_until, unborrow}; @@ -35,7 +35,14 @@ enum TxState { Transmitting(usize), } -struct State<'d, U: UarteInstance, T: TimerInstance> { +pub struct State<'d, U: UarteInstance, T: TimerInstance>(StateStorage>); +impl<'d, U: UarteInstance, T: TimerInstance> State<'d, U, T> { + pub fn new() -> Self { + Self(StateStorage::new()) + } +} + +struct StateInner<'d, U: UarteInstance, T: TimerInstance> { phantom: PhantomData<&'d mut U>, timer: Timer<'d, T>, _ppi_ch1: Ppi<'d, AnyConfigurableChannel>, @@ -51,20 +58,16 @@ struct State<'d, U: UarteInstance, T: TimerInstance> { } /// Interface to a UARTE instance -/// -/// This is a very basic interface that comes with the following limitations: -/// - The UARTE instances share the same address space with instances of UART. -/// You need to make sure that conflicting instances -/// are disabled before using `Uarte`. See product specification: -/// - nrf52832: Section 15.2 -/// - nrf52840: Section 6.1.2 pub struct BufferedUarte<'d, U: UarteInstance, T: TimerInstance> { - inner: PeripheralMutex>, + inner: PeripheralMutex<'d, StateInner<'d, U, T>>, } +impl<'d, U: UarteInstance, T: TimerInstance> Unpin for BufferedUarte<'d, U, T> {} + impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { /// unsafe: may not leak self or futures pub unsafe fn new( + state: &'d mut State<'d, U, T>, _uarte: impl Unborrow + 'd, timer: impl Unborrow + 'd, ppi_ch1: impl Unborrow + 'd, @@ -152,31 +155,28 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { ppi_ch2.set_task(Task::from_reg(&r.tasks_stoprx)); ppi_ch2.enable(); - BufferedUarte { - inner: PeripheralMutex::new( - State { - phantom: PhantomData, - timer, - _ppi_ch1: ppi_ch1, - _ppi_ch2: ppi_ch2, + let initial_state = StateInner { + phantom: PhantomData, + timer, + _ppi_ch1: ppi_ch1, + _ppi_ch2: ppi_ch2, - rx: RingBuffer::new(rx_buffer), - rx_state: RxState::Idle, - rx_waker: WakerRegistration::new(), + rx: RingBuffer::new(rx_buffer), + rx_state: RxState::Idle, + rx_waker: WakerRegistration::new(), - tx: RingBuffer::new(tx_buffer), - tx_state: TxState::Idle, - tx_waker: WakerRegistration::new(), - }, - irq, - ), + tx: RingBuffer::new(tx_buffer), + tx_state: TxState::Idle, + tx_waker: WakerRegistration::new(), + }; + + Self { + inner: PeripheralMutex::new_unchecked(&mut state.0, initial_state, irq), } } - pub fn set_baudrate(self: Pin<&mut Self>, baudrate: Baudrate) { - let mut inner = self.inner(); - unsafe { inner.as_mut().register_interrupt_unchecked() } - inner.with(|state| { + pub fn set_baudrate(&mut self, baudrate: Baudrate) { + self.inner.with(|state| { let r = U::regs(); let timeout = 0x8000_0000 / (baudrate as u32 / 40); @@ -186,17 +186,11 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { r.baudrate.write(|w| w.baudrate().variant(baudrate)); }); } - - fn inner(self: Pin<&mut Self>) -> Pin<&mut PeripheralMutex>> { - unsafe { Pin::new_unchecked(&mut self.get_unchecked_mut().inner) } - } } 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(); - unsafe { inner.as_mut().register_interrupt_unchecked() } - inner.with(|state| { + fn poll_fill_buf(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + self.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 @@ -218,22 +212,22 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, }) } - fn consume(self: Pin<&mut Self>, amt: usize) { - let mut inner = self.inner(); - unsafe { inner.as_mut().register_interrupt_unchecked() } - inner.as_mut().with(|state| { + fn consume(mut self: Pin<&mut Self>, amt: usize) { + self.inner.with(|state| { trace!("consume {:?}", amt); state.rx.pop(amt); }); - inner.pend(); + self.inner.pend(); } } 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(); - unsafe { inner.as_mut().register_interrupt_unchecked() } - let poll = inner.as_mut().with(|state| { + fn poll_write( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + let poll = self.inner.with(|state| { trace!("poll_write: {:?}", buf.len()); let tx_buf = state.tx.push_buf(); @@ -257,13 +251,13 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncWrite for BufferedUarte<'d, U, Poll::Ready(Ok(n)) }); - inner.pend(); + self.inner.pend(); poll } } -impl<'a, U: UarteInstance, T: TimerInstance> Drop for State<'a, U, T> { +impl<'a, U: UarteInstance, T: TimerInstance> Drop for StateInner<'a, U, T> { fn drop(&mut self) { let r = U::regs(); @@ -285,7 +279,7 @@ impl<'a, U: UarteInstance, T: TimerInstance> Drop for State<'a, U, T> { } } -impl<'a, U: UarteInstance, T: TimerInstance> PeripheralState for State<'a, U, T> { +impl<'a, U: UarteInstance, T: TimerInstance> PeripheralState for StateInner<'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 3f72fb35..2c73e0d0 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -1,10 +1,9 @@ use core::marker::PhantomData; -use core::pin::Pin; use core::sync::atomic::{fence, Ordering}; use core::task::Waker; use embassy::util::{AtomicWaker, Unborrow}; -use embassy_hal_common::peripheral::{PeripheralMutex, PeripheralState}; +use embassy_hal_common::peripheral::{PeripheralMutex, PeripheralState, StateStorage}; use embassy_hal_common::unborrow; use embassy_net::{Device, DeviceCapabilities, LinkState, PacketBuf, MTU}; @@ -19,8 +18,14 @@ mod descriptors; use super::{StationManagement, PHY}; use descriptors::DescriptorRing; +pub struct State<'d, const TX: usize, const RX: usize>(StateStorage>); +impl<'d, const TX: usize, const RX: usize> State<'d, TX, RX> { + pub fn new() -> Self { + Self(StateStorage::new()) + } +} pub struct Ethernet<'d, P: PHY, const TX: usize, const RX: usize> { - state: PeripheralMutex>, + state: PeripheralMutex<'d, Inner<'d, TX, RX>>, pins: [AnyPin; 9], _phy: P, clock_range: u8, @@ -30,6 +35,7 @@ pub struct Ethernet<'d, P: PHY, const TX: usize, const RX: usize> { impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { pub fn new( + state: &'d mut State<'d, TX, RX>, peri: impl Unborrow + 'd, interrupt: impl Unborrow + 'd, ref_clk: impl Unborrow + 'd, @@ -72,7 +78,7 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { tx_en.configure(); let inner = Inner::new(peri); - let state = PeripheralMutex::new(inner, interrupt); + let state = unsafe { PeripheralMutex::new_unchecked(&mut state.0, inner, interrupt) }; // NOTE(unsafe) We have exclusive access to the registers unsafe { @@ -145,24 +151,16 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { tx_en.degrade(), ]; - Self { + let mut this = Self { state, pins, _phy: phy, clock_range, phy_addr, mac_addr, - } - } + }; - pub fn init(self: Pin<&mut Self>) { - // NOTE(unsafe) We won't move this - let this = unsafe { self.get_unchecked_mut() }; - let mut mutex = unsafe { Pin::new_unchecked(&mut this.state) }; - // 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| { + this.state.with(|s| { s.desc_ring.init(); fence(Ordering::SeqCst); @@ -189,8 +187,10 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { }); } }); - P::phy_reset(this); - P::phy_init(this); + P::phy_reset(&mut this); + P::phy_init(&mut this); + + this } } @@ -232,29 +232,17 @@ unsafe impl<'d, P: PHY, const TX: usize, const RX: usize> StationManagement } } -impl<'d, P: PHY, const TX: usize, const RX: usize> Device for Pin<&mut Ethernet<'d, P, TX, RX>> { +impl<'d, P: PHY, const TX: usize, const RX: usize> Device for Ethernet<'d, P, TX, RX> { fn is_transmit_ready(&mut self) -> bool { - // NOTE(unsafe) We won't move out of self - 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()) + self.state.with(|s| s.desc_ring.tx.available()) } fn transmit(&mut self, pkt: PacketBuf) { - // NOTE(unsafe) We won't move out of self - 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))); + self.state.with(|s| unwrap!(s.desc_ring.tx.transmit(pkt))); } fn receive(&mut self) -> Option { - // NOTE(unsafe) We won't move out of self - 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()) + self.state.with(|s| s.desc_ring.rx.pop_packet()) } fn register_waker(&mut self, waker: &Waker) { @@ -269,10 +257,7 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Device for Pin<&mut Ethernet< } fn link_state(&mut self) -> LinkState { - // NOTE(unsafe) We won't move out of self - let this = unsafe { self.as_mut().get_unchecked_mut() }; - - if P::poll_link(this) { + if P::poll_link(self) { LinkState::Up } else { LinkState::Down @@ -280,10 +265,7 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Device for Pin<&mut Ethernet< } fn ethernet_address(&mut self) -> [u8; 6] { - // NOTE(unsafe) We won't move out of self - let this = unsafe { self.as_mut().get_unchecked_mut() }; - - this.mac_addr + self.mac_addr } } diff --git a/examples/nrf/src/bin/buffered_uart.rs b/examples/nrf/src/bin/buffered_uart.rs index c800e64f..a78d2df4 100644 --- a/examples/nrf/src/bin/buffered_uart.rs +++ b/examples/nrf/src/bin/buffered_uart.rs @@ -11,6 +11,7 @@ mod example_common; use defmt::panic; use embassy::executor::Spawner; use embassy::io::{AsyncBufReadExt, AsyncWriteExt}; +use embassy_nrf::buffered_uarte::State; use embassy_nrf::gpio::NoPin; use embassy_nrf::{buffered_uarte::BufferedUarte, interrupt, uarte, Peripherals}; use example_common::*; @@ -26,8 +27,10 @@ async fn main(_spawner: Spawner, p: Peripherals) { let mut rx_buffer = [0u8; 4096]; let irq = interrupt::take!(UARTE0_UART0); + let mut state = State::new(); let u = unsafe { BufferedUarte::new( + &mut state, p.UARTE0, p.TIMER0, p.PPI_CH0, diff --git a/examples/stm32h7/src/bin/eth.rs b/examples/stm32h7/src/bin/eth.rs index 7ae80d6e..5cf49e82 100644 --- a/examples/stm32h7/src/bin/eth.rs +++ b/examples/stm32h7/src/bin/eth.rs @@ -6,7 +6,6 @@ #![feature(impl_trait_in_bindings)] #![feature(type_alias_impl_trait)] -use core::pin::Pin; use core::sync::atomic::{AtomicUsize, Ordering}; use cortex_m_rt::entry; @@ -22,7 +21,7 @@ use embassy_net::{ }; use embassy_stm32::clock::{Alarm, Clock}; use embassy_stm32::eth::lan8742a::LAN8742A; -use embassy_stm32::eth::Ethernet; +use embassy_stm32::eth::{Ethernet, State}; use embassy_stm32::rcc::{Config as RccConfig, Rcc}; use embassy_stm32::rng::Random; use embassy_stm32::time::Hertz; @@ -42,7 +41,7 @@ defmt::timestamp! {"{=u64}", { #[embassy::task] async fn main_task( - device: &'static mut Pin<&'static mut Ethernet<'static, LAN8742A, 4, 4>>, + device: &'static mut Ethernet<'static, LAN8742A, 4, 4>, config: &'static mut StaticConfigurator, spawner: Spawner, ) { @@ -99,8 +98,8 @@ static mut RNG_INST: Option> = None; static EXECUTOR: Forever = Forever::new(); static TIMER_RTC: Forever> = Forever::new(); static ALARM: Forever> = Forever::new(); +static STATE: Forever> = Forever::new(); static ETH: Forever> = Forever::new(); -static DEVICE: Forever>> = Forever::new(); static CONFIG: Forever = Forever::new(); static NET_RESOURCES: Forever> = Forever::new(); @@ -135,15 +134,12 @@ fn main() -> ! { let eth_int = interrupt_take!(ETH); let mac_addr = [0x10; 6]; + let state = STATE.put(State::new()); let eth = ETH.put(Ethernet::new( - p.ETH, eth_int, p.PA1, p.PA2, p.PC1, p.PA7, p.PC4, p.PC5, p.PB12, p.PB13, p.PB11, LAN8742A, - mac_addr, 1, + state, p.ETH, eth_int, p.PA1, p.PA2, p.PC1, p.PA7, p.PC4, p.PC5, p.PB12, p.PB13, p.PB11, + LAN8742A, mac_addr, 1, )); - // NOTE(unsafe) This thing is a &'static - let net_device = DEVICE.put(unsafe { Pin::new_unchecked(eth) }); - net_device.as_mut().init(); - let config = StaticConfigurator::new(NetConfig { address: Ipv4Cidr::new(Ipv4Address::new(192, 168, 0, 61), 24), dns_servers: Vec::new(), @@ -156,6 +152,6 @@ fn main() -> ! { executor.set_alarm(alarm); executor.run(move |spawner| { - unwrap!(spawner.spawn(main_task(net_device, config, spawner))); + unwrap!(spawner.spawn(main_task(eth, config, spawner))); }) } From 63ac7ac799b07c18a9a621b4d20ead2b39982ef5 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Mon, 2 Aug 2021 12:40:01 +0200 Subject: [PATCH 2/4] Mark `new`s as unsafe due to not being leak-safe. --- embassy-hal-common/src/usb/mod.rs | 5 +- embassy-stm32/src/eth/v2/mod.rs | 111 +++++++++++++++--------------- examples/stm32h7/src/bin/eth.rs | 10 +-- 3 files changed, 64 insertions(+), 62 deletions(-) diff --git a/embassy-hal-common/src/usb/mod.rs b/embassy-hal-common/src/usb/mod.rs index 0940e6b0..ae9f2607 100644 --- a/embassy-hal-common/src/usb/mod.rs +++ b/embassy-hal-common/src/usb/mod.rs @@ -60,7 +60,8 @@ where T: ClassSet, I: USBInterrupt, { - pub fn new>( + /// safety: the returned instance is not leak-safe + pub unsafe fn new>( state: &'bus mut State<'bus, B, T, I>, device: UsbDevice<'bus, B>, class_set: S, @@ -71,7 +72,7 @@ where classes: class_set.into_class_set(), _interrupt: PhantomData, }; - let mutex = unsafe { PeripheralMutex::new_unchecked(&mut state.0, initial_state, irq) }; + let mutex = PeripheralMutex::new_unchecked(&mut state.0, initial_state, irq); Self { inner: RefCell::new(mutex), } diff --git a/embassy-stm32/src/eth/v2/mod.rs b/embassy-stm32/src/eth/v2/mod.rs index 2c73e0d0..37bc9715 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -34,7 +34,8 @@ pub struct Ethernet<'d, P: PHY, const TX: usize, const RX: usize> { } impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { - pub fn new( + /// safety: the returned instance is not leak-safe + pub unsafe fn new( state: &'d mut State<'d, TX, RX>, peri: impl Unborrow + 'd, interrupt: impl Unborrow + 'd, @@ -55,7 +56,7 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { // Enable the necessary Clocks // NOTE(unsafe) We have exclusive access to the registers - critical_section::with(|_| unsafe { + critical_section::with(|_| { RCC.apb4enr().modify(|w| w.set_syscfgen(true)); RCC.ahb1enr().modify(|w| { w.set_eth1macen(true); @@ -78,52 +79,52 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { tx_en.configure(); let inner = Inner::new(peri); - let state = unsafe { PeripheralMutex::new_unchecked(&mut state.0, inner, interrupt) }; + + // NOTE(unsafe) We are ourselves not leak-safe. + let state = PeripheralMutex::new_unchecked(&mut state.0, inner, interrupt); // NOTE(unsafe) We have exclusive access to the registers - unsafe { - let dma = ETH.ethernet_dma(); - let mac = ETH.ethernet_mac(); - let mtl = ETH.ethernet_mtl(); + let dma = ETH.ethernet_dma(); + let mac = ETH.ethernet_mac(); + let mtl = ETH.ethernet_mtl(); - // Reset and wait - dma.dmamr().modify(|w| w.set_swr(true)); - while dma.dmamr().read().swr() {} + // Reset and wait + dma.dmamr().modify(|w| w.set_swr(true)); + while dma.dmamr().read().swr() {} - mac.maccr().modify(|w| { - w.set_ipg(0b000); // 96 bit times - w.set_acs(true); - w.set_fes(true); - w.set_dm(true); - // TODO: Carrier sense ? ECRSFD - }); + mac.maccr().modify(|w| { + w.set_ipg(0b000); // 96 bit times + w.set_acs(true); + w.set_fes(true); + w.set_dm(true); + // TODO: Carrier sense ? ECRSFD + }); - mac.maca0lr().write(|w| { - w.set_addrlo( - u32::from(mac_addr[0]) - | (u32::from(mac_addr[1]) << 8) - | (u32::from(mac_addr[2]) << 16) - | (u32::from(mac_addr[3]) << 24), - ) - }); - mac.maca0hr() - .modify(|w| w.set_addrhi(u16::from(mac_addr[4]) | (u16::from(mac_addr[5]) << 8))); + mac.maca0lr().write(|w| { + w.set_addrlo( + u32::from(mac_addr[0]) + | (u32::from(mac_addr[1]) << 8) + | (u32::from(mac_addr[2]) << 16) + | (u32::from(mac_addr[3]) << 24), + ) + }); + mac.maca0hr() + .modify(|w| w.set_addrhi(u16::from(mac_addr[4]) | (u16::from(mac_addr[5]) << 8))); - mac.macpfr().modify(|w| w.set_saf(true)); - mac.macqtx_fcr().modify(|w| w.set_pt(0x100)); + mac.macpfr().modify(|w| w.set_saf(true)); + mac.macqtx_fcr().modify(|w| w.set_pt(0x100)); - mtl.mtlrx_qomr().modify(|w| w.set_rsf(true)); - mtl.mtltx_qomr().modify(|w| w.set_tsf(true)); + mtl.mtlrx_qomr().modify(|w| w.set_rsf(true)); + mtl.mtltx_qomr().modify(|w| w.set_tsf(true)); - dma.dmactx_cr().modify(|w| w.set_txpbl(1)); // 32 ? - dma.dmacrx_cr().modify(|w| { - w.set_rxpbl(1); // 32 ? - w.set_rbsz(MTU as u16); - }); - } + dma.dmactx_cr().modify(|w| w.set_txpbl(1)); // 32 ? + dma.dmacrx_cr().modify(|w| { + w.set_rxpbl(1); // 32 ? + w.set_rbsz(MTU as u16); + }); // NOTE(unsafe) We got the peripheral singleton, which means that `rcc::init` was called - let hclk = unsafe { crate::rcc::get_freqs().ahb1 }; + let hclk = crate::rcc::get_freqs().ahb1; let hclk_mhz = hclk.0 / 1_000_000; // Set the MDC clock frequency in the range 1MHz - 2.5MHz @@ -165,27 +166,25 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { fence(Ordering::SeqCst); - unsafe { - let mac = ETH.ethernet_mac(); - let mtl = ETH.ethernet_mtl(); - let dma = ETH.ethernet_dma(); + let mac = ETH.ethernet_mac(); + let mtl = ETH.ethernet_mtl(); + let dma = ETH.ethernet_dma(); - mac.maccr().modify(|w| { - w.set_re(true); - w.set_te(true); - }); - mtl.mtltx_qomr().modify(|w| w.set_ftq(true)); + mac.maccr().modify(|w| { + w.set_re(true); + w.set_te(true); + }); + mtl.mtltx_qomr().modify(|w| w.set_ftq(true)); - dma.dmactx_cr().modify(|w| w.set_st(true)); - dma.dmacrx_cr().modify(|w| w.set_sr(true)); + dma.dmactx_cr().modify(|w| w.set_st(true)); + dma.dmacrx_cr().modify(|w| w.set_sr(true)); - // Enable interrupts - dma.dmacier().modify(|w| { - w.set_nie(true); - w.set_rie(true); - w.set_tie(true); - }); - } + // Enable interrupts + dma.dmacier().modify(|w| { + w.set_nie(true); + w.set_rie(true); + w.set_tie(true); + }); }); P::phy_reset(&mut this); P::phy_init(&mut this); diff --git a/examples/stm32h7/src/bin/eth.rs b/examples/stm32h7/src/bin/eth.rs index 5cf49e82..e49a101b 100644 --- a/examples/stm32h7/src/bin/eth.rs +++ b/examples/stm32h7/src/bin/eth.rs @@ -135,10 +135,12 @@ fn main() -> ! { let eth_int = interrupt_take!(ETH); let mac_addr = [0x10; 6]; let state = STATE.put(State::new()); - let eth = ETH.put(Ethernet::new( - state, p.ETH, eth_int, p.PA1, p.PA2, p.PC1, p.PA7, p.PC4, p.PC5, p.PB12, p.PB13, p.PB11, - LAN8742A, mac_addr, 1, - )); + let eth = unsafe { + ETH.put(Ethernet::new( + state, p.ETH, eth_int, p.PA1, p.PA2, p.PC1, p.PA7, p.PC4, p.PC5, p.PB12, p.PB13, + p.PB11, LAN8742A, mac_addr, 1, + )) + }; let config = StaticConfigurator::new(NetConfig { address: Ipv4Cidr::new(Ipv4Address::new(192, 168, 0, 61), 24), From e238079d7d921cab589eb6106059d4fb0b12ce1c Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Mon, 2 Aug 2021 19:50:07 +0200 Subject: [PATCH 3/4] Make const the states when able. --- embassy-hal-common/src/peripheral.rs | 2 +- embassy-stm32/src/eth/v2/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/embassy-hal-common/src/peripheral.rs b/embassy-hal-common/src/peripheral.rs index d3df06e8..46c6ebee 100644 --- a/embassy-hal-common/src/peripheral.rs +++ b/embassy-hal-common/src/peripheral.rs @@ -17,7 +17,7 @@ pub trait PeripheralState: Send { pub struct StateStorage(MaybeUninit); impl StateStorage { - pub fn new() -> Self { + pub const fn new() -> Self { Self(MaybeUninit::uninit()) } } diff --git a/embassy-stm32/src/eth/v2/mod.rs b/embassy-stm32/src/eth/v2/mod.rs index 37bc9715..cfb461ab 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -20,7 +20,7 @@ use descriptors::DescriptorRing; pub struct State<'d, const TX: usize, const RX: usize>(StateStorage>); impl<'d, const TX: usize, const RX: usize> State<'d, TX, RX> { - pub fn new() -> Self { + pub const fn new() -> Self { Self(StateStorage::new()) } } From 3f28bb6c77de3d8ecbb6d401f107586f24e416a4 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Mon, 2 Aug 2021 20:13:41 +0200 Subject: [PATCH 4/4] common: Initialize PeripheralMutex state with closure to ensure it's done in-place. --- embassy-hal-common/src/peripheral.rs | 18 ++++++++++------- embassy-hal-common/src/usb/mod.rs | 5 ++--- embassy-nrf/src/buffered_uarte.rs | 30 +++++++++++++--------------- embassy-stm32/src/eth/v2/mod.rs | 4 +--- 4 files changed, 28 insertions(+), 29 deletions(-) diff --git a/embassy-hal-common/src/peripheral.rs b/embassy-hal-common/src/peripheral.rs index 46c6ebee..dcf9d3a2 100644 --- a/embassy-hal-common/src/peripheral.rs +++ b/embassy-hal-common/src/peripheral.rs @@ -50,19 +50,23 @@ pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool { } impl<'a, S: PeripheralState> PeripheralMutex<'a, S> { - /// Create a new `PeripheralMutex` wrapping `irq`, with the initial state `state`. + /// Create a new `PeripheralMutex` wrapping `irq`, with `init` initializing the initial state. /// - /// self requires `state` to live for `'static`, because if the `PeripheralMutex` is leaked, the + /// self requires `S` to live for `'static`, because if the `PeripheralMutex` is leaked, the /// interrupt won't be disabled, which may try accessing the state at any time. To use non-`'static` /// state, see [`Self::new_unchecked`]. /// /// Registers `on_interrupt` as the `irq`'s handler, and enables it. - pub fn new(storage: &'a mut StateStorage, state: S, irq: S::Interrupt) -> Self + pub fn new( + irq: S::Interrupt, + storage: &'a mut StateStorage, + init: impl FnOnce() -> S, + ) -> Self where 'a: 'static, { // safety: safe because state is `'static`. - unsafe { Self::new_unchecked(storage, state, irq) } + unsafe { Self::new_unchecked(irq, storage, init) } } /// Create a `PeripheralMutex` without requiring the state is `'static`. @@ -72,9 +76,9 @@ impl<'a, S: PeripheralState> PeripheralMutex<'a, S> { /// # Safety /// The created instance must not be leaked (its `drop` must run). pub unsafe fn new_unchecked( - storage: &'a mut StateStorage, - state: S, irq: S::Interrupt, + storage: &'a mut StateStorage, + init: impl FnOnce() -> S, ) -> Self { if can_be_preempted(&irq) { panic!("`PeripheralMutex` cannot be created in an interrupt with higher priority than the interrupt it wraps"); @@ -84,7 +88,7 @@ impl<'a, S: PeripheralState> PeripheralMutex<'a, S> { // Safety: The pointer is valid and not used by anyone else // because we have the `&mut StateStorage`. - state_ptr.write(state); + state_ptr.write(init()); irq.disable(); irq.set_handler(|p| { diff --git a/embassy-hal-common/src/usb/mod.rs b/embassy-hal-common/src/usb/mod.rs index ae9f2607..70a74bd5 100644 --- a/embassy-hal-common/src/usb/mod.rs +++ b/embassy-hal-common/src/usb/mod.rs @@ -67,12 +67,11 @@ where class_set: S, irq: I, ) -> Self { - let initial_state = StateInner { + let mutex = PeripheralMutex::new_unchecked(irq, &mut state.0, || StateInner { device, classes: class_set.into_class_set(), _interrupt: PhantomData, - }; - let mutex = PeripheralMutex::new_unchecked(&mut state.0, initial_state, irq); + }); Self { inner: RefCell::new(mutex), } diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index 642c3018..048c36d3 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -155,23 +155,21 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { ppi_ch2.set_task(Task::from_reg(&r.tasks_stoprx)); ppi_ch2.enable(); - let initial_state = StateInner { - phantom: PhantomData, - timer, - _ppi_ch1: ppi_ch1, - _ppi_ch2: ppi_ch2, - - rx: RingBuffer::new(rx_buffer), - rx_state: RxState::Idle, - rx_waker: WakerRegistration::new(), - - tx: RingBuffer::new(tx_buffer), - tx_state: TxState::Idle, - tx_waker: WakerRegistration::new(), - }; - Self { - inner: PeripheralMutex::new_unchecked(&mut state.0, initial_state, irq), + inner: PeripheralMutex::new_unchecked(irq, &mut state.0, move || StateInner { + phantom: PhantomData, + timer, + _ppi_ch1: ppi_ch1, + _ppi_ch2: ppi_ch2, + + rx: RingBuffer::new(rx_buffer), + rx_state: RxState::Idle, + rx_waker: WakerRegistration::new(), + + tx: RingBuffer::new(tx_buffer), + tx_state: TxState::Idle, + tx_waker: WakerRegistration::new(), + }), } } diff --git a/embassy-stm32/src/eth/v2/mod.rs b/embassy-stm32/src/eth/v2/mod.rs index cfb461ab..c1956aa1 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -78,10 +78,8 @@ impl<'d, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, P, TX, RX> { tx_d1.configure(); tx_en.configure(); - let inner = Inner::new(peri); - // NOTE(unsafe) We are ourselves not leak-safe. - let state = PeripheralMutex::new_unchecked(&mut state.0, inner, interrupt); + let state = PeripheralMutex::new_unchecked(interrupt, &mut state.0, || Inner::new(peri)); // NOTE(unsafe) We have exclusive access to the registers let dma = ETH.ethernet_dma();