From db344c2bda55bd0352a43720788185cc4d3a420e Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 9 Jun 2022 21:28:13 +0200 Subject: [PATCH] common/PeripheralMutex: remove unsafe API. (#802) Following the project's decision that "leak unsafe" APIs are not marked as "unsafe", update PeripheralMutex to accept non-'static state without unsafe. Fixes #801 --- embassy-hal-common/src/peripheral.rs | 26 ++-------------------- embassy-nrf/src/buffered_uarte.rs | 26 ++++++++++------------ embassy-stm32/src/eth/v1/mod.rs | 2 +- embassy-stm32/src/eth/v2/mod.rs | 2 +- embassy-stm32/src/usart/buffered.rs | 14 +++++++----- examples/stm32f4/src/bin/usart_buffered.rs | 3 +-- 6 files changed, 25 insertions(+), 48 deletions(-) diff --git a/embassy-hal-common/src/peripheral.rs b/embassy-hal-common/src/peripheral.rs index 89420a42..db2bc788 100644 --- a/embassy-hal-common/src/peripheral.rs +++ b/embassy-hal-common/src/peripheral.rs @@ -52,33 +52,11 @@ pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool { impl<'a, S: PeripheralState> PeripheralMutex<'a, S> { /// Create a new `PeripheralMutex` wrapping `irq`, with `init` initializing the initial state. /// - /// 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( 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(irq, storage, init) } - } - - /// 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( - 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"); @@ -88,10 +66,10 @@ 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(init()); + unsafe { state_ptr.write(init()) }; irq.disable(); - irq.set_handler(|p| { + irq.set_handler(|p| unsafe { // 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`, diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index e1d32a31..6972d625 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -164,22 +164,20 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { ppi_ch2.enable(); Self { - inner: unsafe { - PeripheralMutex::new_unchecked(irq, &mut state.0, move || StateInner { - phantom: PhantomData, - timer, - _ppi_ch1: ppi_ch1, - _ppi_ch2: ppi_ch2, + inner: PeripheralMutex::new(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(), + 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(), - }) - }, + tx: RingBuffer::new(tx_buffer), + tx_state: TxState::Idle, + tx_waker: WakerRegistration::new(), + }), } } diff --git a/embassy-stm32/src/eth/v1/mod.rs b/embassy-stm32/src/eth/v1/mod.rs index 465436d0..327deea2 100644 --- a/embassy-stm32/src/eth/v1/mod.rs +++ b/embassy-stm32/src/eth/v1/mod.rs @@ -146,7 +146,7 @@ impl<'d, T: Instance, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, T, config_pins!(ref_clk, mdio, mdc, crs, rx_d0, rx_d1, tx_d0, tx_d1, tx_en); // NOTE(unsafe) We are ourselves not leak-safe. - let state = PeripheralMutex::new_unchecked(interrupt, &mut state.0, || Inner::new(peri)); + let state = PeripheralMutex::new(interrupt, &mut state.0, || Inner::new(peri)); // NOTE(unsafe) We have exclusive access to the registers let dma = ETH.ethernet_dma(); diff --git a/embassy-stm32/src/eth/v2/mod.rs b/embassy-stm32/src/eth/v2/mod.rs index afde8ea9..6a49904d 100644 --- a/embassy-stm32/src/eth/v2/mod.rs +++ b/embassy-stm32/src/eth/v2/mod.rs @@ -83,7 +83,7 @@ impl<'d, T: Instance, P: PHY, const TX: usize, const RX: usize> Ethernet<'d, T, config_pins!(ref_clk, mdio, mdc, crs, rx_d0, rx_d1, tx_d0, tx_d1, tx_en); // NOTE(unsafe) We are ourselves not leak-safe. - let state = PeripheralMutex::new_unchecked(interrupt, &mut state.0, || Inner::new(peri)); + let state = PeripheralMutex::new(interrupt, &mut state.0, || Inner::new(peri)); // NOTE(unsafe) We have exclusive access to the registers let dma = ETH.ethernet_dma(); diff --git a/embassy-stm32/src/usart/buffered.rs b/embassy-stm32/src/usart/buffered.rs index 7b63638a..36d176b9 100644 --- a/embassy-stm32/src/usart/buffered.rs +++ b/embassy-stm32/src/usart/buffered.rs @@ -35,7 +35,7 @@ pub struct BufferedUart<'d, T: Instance> { impl<'d, T: Instance> Unpin for BufferedUart<'d, T> {} impl<'d, T: Instance> BufferedUart<'d, T> { - pub unsafe fn new( + pub fn new( state: &'d mut State<'d, T>, _uart: Uart<'d, T, NoDma, NoDma>, irq: impl Unborrow + 'd, @@ -45,13 +45,15 @@ impl<'d, T: Instance> BufferedUart<'d, T> { unborrow!(irq); let r = T::regs(); - r.cr1().modify(|w| { - w.set_rxneie(true); - w.set_idleie(true); - }); + unsafe { + r.cr1().modify(|w| { + w.set_rxneie(true); + w.set_idleie(true); + }); + } Self { - inner: PeripheralMutex::new_unchecked(irq, &mut state.0, move || StateInner { + inner: PeripheralMutex::new(irq, &mut state.0, move || StateInner { phantom: PhantomData, tx: RingBuffer::new(tx_buffer), tx_waker: WakerRegistration::new(), diff --git a/examples/stm32f4/src/bin/usart_buffered.rs b/examples/stm32f4/src/bin/usart_buffered.rs index 80b65f0d..2a613ee4 100644 --- a/examples/stm32f4/src/bin/usart_buffered.rs +++ b/examples/stm32f4/src/bin/usart_buffered.rs @@ -22,8 +22,7 @@ async fn main(_spawner: Spawner, p: Peripherals) { let irq = interrupt::take!(USART3); let mut tx_buf = [0u8; 32]; let mut rx_buf = [0u8; 32]; - let mut buf_usart = - unsafe { BufferedUart::new(&mut state, usart, irq, &mut tx_buf, &mut rx_buf) }; + let mut buf_usart = BufferedUart::new(&mut state, usart, irq, &mut tx_buf, &mut rx_buf); loop { let buf = buf_usart.fill_buf().await.unwrap();