From 7a2d76d6b881bface640a269faf7758aba38eea0 Mon Sep 17 00:00:00 2001 From: Justin Beaurivage Date: Mon, 11 Nov 2024 21:44:38 -0500 Subject: [PATCH] feat(spi): Unlock DMA transfers for SpiBus::transfer_in_place (#780) * feat(spi): Unlock DMA transfers for SpiBus::transfer_in_place Also fix potential UB caused by mishandled memory barriers in `dmac * typo fix --------- Co-authored-by: Paul Sajna --- hal/src/dmac/channel/mod.rs | 62 +++++++++---------- hal/src/dmac/channel/reg.rs | 19 ++++++ hal/src/dmac/mod.rs | 6 +- hal/src/dmac/transfer.rs | 13 +--- hal/src/sercom/i2c.rs | 8 +-- hal/src/sercom/i2c/config.rs | 2 +- hal/src/sercom/i2c/impl_ehal.rs | 8 +-- hal/src/sercom/spi.rs | 8 --- hal/src/sercom/spi/impl_ehal/dma.rs | 17 +++-- .../spi/{impl_ehal.rs => impl_ehal/mod.rs} | 6 +- hal/src/sercom/spi/impl_ehal/panic_on.rs | 6 +- .../thumbv6m.rs} | 0 .../thumbv7em.rs} | 0 13 files changed, 82 insertions(+), 73 deletions(-) rename hal/src/sercom/spi/{impl_ehal.rs => impl_ehal/mod.rs} (97%) rename hal/src/sercom/spi/{impl_ehal_thumbv6m.rs => impl_ehal/thumbv6m.rs} (100%) rename hal/src/sercom/spi/{impl_ehal_thumbv7em.rs => impl_ehal/thumbv7em.rs} (100%) diff --git a/hal/src/dmac/channel/mod.rs b/hal/src/dmac/channel/mod.rs index 98f3d3a6144..d501de79c0a 100644 --- a/hal/src/dmac/channel/mod.rs +++ b/hal/src/dmac/channel/mod.rs @@ -160,11 +160,7 @@ impl Channel { #[hal_cfg("dmac-d5x")] self.regs.chprilvl.modify(|_, w| w.prilvl().variant(lvl)); - - Channel { - regs: self.regs, - _status: PhantomData, - } + self.change_status() } /// Selectively enable interrupts @@ -219,8 +215,13 @@ impl Channel { self.regs.swtrigctrl.set_bit(); } + /// Enable the transfer, and emit a compiler fence. #[inline] fn _enable_private(&mut self) { + // Prevent the compiler from re-ordering read/write + // operations beyond this fence. + // (see https://docs.rust-embedded.org/embedonomicon/dma.html#compiler-misoptimizations) + atomic::fence(atomic::Ordering::Release); // ▲ self.regs.chctrla.modify(|_, w| w.enable().set_bit()); } @@ -228,12 +229,22 @@ impl Channel { #[inline] pub(crate) fn stop(&mut self) { self.regs.chctrla.modify(|_, w| w.enable().clear_bit()); + + // Wait for the burst to finish + while !self.xfer_complete() { + core::hint::spin_loop(); + } + + // Prevent the compiler from re-ordering read/write + // operations beyond this fence. + // (see https://docs.rust-embedded.org/embedonomicon/dma.html#compiler-misoptimizations) + atomic::fence(atomic::Ordering::Acquire); // ▼ } /// Returns whether or not the transfer is complete. #[inline] pub(crate) fn xfer_complete(&mut self) -> bool { - !self.regs.chctrla.read().enable().bit_is_set() + self.regs.chctrla.read().enable().bit_is_clear() } /// Returns the transfer's success status. @@ -320,11 +331,7 @@ impl Channel { #[inline] pub fn reset(mut self) -> Channel { self._reset_private(); - - Channel { - regs: self.regs, - _status: PhantomData, - } + self.change_status() } /// Set the FIFO threshold length. The channel will wait until it has @@ -365,6 +372,7 @@ impl Channel { // Configure the trigger source and trigger action self.configure_trigger(trig_src, trig_act); self._enable_private(); + // If trigger source is DISABLE, manually trigger transfer if trig_src == TriggerSource::Disable { self._trigger_private(); @@ -486,8 +494,6 @@ impl Channel { } self.configure_trigger(trig_src, trig_act); - - atomic::fence(atomic::Ordering::Release); self._enable_private(); if trig_src == TriggerSource::Disable { @@ -512,12 +518,8 @@ impl Channel { /// [`Transfer`](super::transfer::Transfer) #[inline] pub(crate) fn free(mut self) -> Channel { - self.regs.chctrla.modify(|_, w| w.enable().clear_bit()); - while !self.xfer_complete() {} - Channel { - regs: self.regs, - _status: PhantomData, - } + self.stop(); + self.change_status() } #[inline] @@ -545,16 +547,14 @@ impl Channel { /// Restart transfer using previously-configured trigger source and action #[inline] pub(crate) fn restart(&mut self) { - self.regs.chctrla.modify(|_, w| w.enable().set_bit()); + self._enable_private(); } } impl From> for Channel { - fn from(item: Channel) -> Self { - Channel { - regs: item.regs, - _status: PhantomData, - } + fn from(mut item: Channel) -> Self { + item._reset_private(); + item.change_status() } } @@ -569,12 +569,6 @@ pub enum CallbackStatus { TransferSuspended, } -impl Default for InterruptFlags { - fn default() -> Self { - Self::new() - } -} - /// Interrupt sources available to a DMA channel #[bitfield] #[repr(u8)] @@ -590,6 +584,12 @@ pub struct InterruptFlags { _reserved: B5, } +impl Default for InterruptFlags { + fn default() -> Self { + Self::new() + } +} + /// Generate a [`DmacDescriptor`], and write it to the provided descriptor /// reference. /// diff --git a/hal/src/dmac/channel/reg.rs b/hal/src/dmac/channel/reg.rs index b8662775c28..803b306d058 100644 --- a/hal/src/dmac/channel/reg.rs +++ b/hal/src/dmac/channel/reg.rs @@ -276,6 +276,9 @@ reg_proxy!(swtrigctrl, bit, rw); /// Acts as a proxy to the PAC DMAC object. Only registers and bits /// within registers that should be readable/writable by specific /// [`Channel`]s are exposed. +/// +/// This struct implements [`Drop`]. Dropping this struct will stop +/// any ongoing transfers for the channel which it represents. #[allow(dead_code)] #[hal_macro_helper] pub(super) struct RegisterBlock { @@ -312,3 +315,19 @@ impl RegisterBlock { } } } + +impl Drop for RegisterBlock { + fn drop(&mut self) { + // Stop any potentially ongoing transfers + self.chctrla.modify(|_, w| w.enable().clear_bit()); + + while self.chctrla.read().enable().bit_is_set() { + core::hint::spin_loop(); + } + + // Prevent the compiler from re-ordering read/write + // operations beyond this fence. + // (see https://docs.rust-embedded.org/embedonomicon/dma.html#compiler-misoptimizations) + core::sync::atomic::fence(core::sync::atomic::Ordering::Acquire); // ▼ + } +} diff --git a/hal/src/dmac/mod.rs b/hal/src/dmac/mod.rs index 6b1e75aef22..6d0a70b156e 100644 --- a/hal/src/dmac/mod.rs +++ b/hal/src/dmac/mod.rs @@ -99,9 +99,9 @@ //! stack-allocated buffers for reuse while the DMAC is still writing to/reading //! from them! Needless to say that is very unsafe. //! Refer [here](https://docs.rust-embedded.org/embedonomicon/dma.html#memforget) -//! or [here](https://blog.japaric.io/safe-dma/) for more information. You may choose to forego -//! the `'static` lifetimes by using the unsafe API and the -//! [`Transfer::new_unchecked`](transfer::Transfer::new_unchecked) method. +//! or [here](https://blog.japaric.io/safe-dma/#leakpocalypse) for more information. +//! You may choose to forgo the `'static` lifetimes by using the unsafe API and +//! the [`Transfer::new_unchecked`](transfer::Transfer::new_unchecked) method. //! //! # Unsafe API //! diff --git a/hal/src/dmac/transfer.rs b/hal/src/dmac/transfer.rs index ad6a9415a38..f8e4fdf1dbd 100644 --- a/hal/src/dmac/transfer.rs +++ b/hal/src/dmac/transfer.rs @@ -88,7 +88,6 @@ use super::{ Error, Result, }; use crate::typelevel::{Is, Sealed}; -use core::sync::atomic; use modular_bitfield::prelude::*; //============================================================================== @@ -453,10 +452,6 @@ where // before this function returns. self.complete = false; - // Memory barrier to prevent the compiler/CPU from re-ordering read/write - // operations beyond this fence. - // (see https://docs.rust-embedded.org/embedonomicon/dma.html#compiler-misoptimizations) - atomic::fence(atomic::Ordering::Release); // ▲ let chan = self.chan.into().start(trig_src, trig_act); Transfer { @@ -649,13 +644,9 @@ where /// resources #[inline] pub fn stop(self) -> (Channel, Ready>, S, D) { + // `free()` stops the transfer, waits for the burst to finish, and emits a + // compiler fence. let chan = self.chan.into().free(); - - // Memory barrier to prevent the compiler/CPU from re-ordering read/write - // operations beyond this fence. - // (see https://docs.rust-embedded.org/embedonomicon/dma.html#compiler-misoptimizations) - atomic::fence(atomic::Ordering::Acquire); // ▼ - (chan, self.buffers.source, self.buffers.destination) } } diff --git a/hal/src/sercom/i2c.rs b/hal/src/sercom/i2c.rs index f2abffad520..c33878abe05 100644 --- a/hal/src/sercom/i2c.rs +++ b/hal/src/sercom/i2c.rs @@ -281,7 +281,7 @@ pub enum InactiveTimeout { /// Abstraction over a I2C peripheral, allowing to perform I2C transactions. pub struct I2c { config: C, - dma_channel: D, + _dma_channel: D, } impl I2c { @@ -430,7 +430,7 @@ impl I2c { ) -> I2c { I2c { config: self.config, - dma_channel: channel, + _dma_channel: channel, } } } @@ -442,9 +442,9 @@ impl> I2c< ( I2c { config: self.config, - dma_channel: crate::typelevel::NoneT, + _dma_channel: crate::typelevel::NoneT, }, - self.dma_channel, + self._dma_channel, ) } } diff --git a/hal/src/sercom/i2c/config.rs b/hal/src/sercom/i2c/config.rs index 220d10f0904..39f684564b3 100644 --- a/hal/src/sercom/i2c/config.rs +++ b/hal/src/sercom/i2c/config.rs @@ -232,7 +232,7 @@ impl Config

{ I2c { config: self, - dma_channel: NoneT, + _dma_channel: NoneT, } } } diff --git a/hal/src/sercom/i2c/impl_ehal.rs b/hal/src/sercom/i2c/impl_ehal.rs index 873655093e6..6c7838d7804 100644 --- a/hal/src/sercom/i2c/impl_ehal.rs +++ b/hal/src/sercom/i2c/impl_ehal.rs @@ -179,7 +179,7 @@ mod dma { ); self.start_dma_read(address, transfer_len as u8); - let channel = self.dma_channel.as_mut(); + let channel = self._dma_channel.as_mut(); // SAFETY: We must make sure that any DMA transfer is complete or stopped before // returning. @@ -193,7 +193,7 @@ mod dma { channel.stop(); self.read_status().check_bus_error()?; - self.dma_channel.as_mut().xfer_success()?; + self._dma_channel.as_mut().xfer_success()?; Ok(()) } @@ -233,7 +233,7 @@ mod dma { self.start_dma_write(address, transfer_len as u8); let mut bytes = SharedSliceBuffer::from_slice(source); - let channel = self.dma_channel.as_mut(); + let channel = self._dma_channel.as_mut(); // SAFETY: We must make sure that any DMA transfer is complete or stopped before // returning. @@ -251,7 +251,7 @@ mod dma { } self.read_status().check_bus_error()?; - self.dma_channel.as_mut().xfer_success()?; + self._dma_channel.as_mut().xfer_success()?; Ok(()) } } diff --git a/hal/src/sercom/spi.rs b/hal/src/sercom/spi.rs index 99e5dcef9d7..fc6370e8f66 100644 --- a/hal/src/sercom/spi.rs +++ b/hal/src/sercom/spi.rs @@ -302,14 +302,6 @@ //! spi.write(&mut buffer)?; //! ``` //! -//! ### Considerations when using SPI DMA transfers -//! -//! * The implemenatation for the `transfer_in_place` method provided by -//! [`embedded_hal::spi::SpiBus`] cannot make use of DMA transfers, because -//! two DMA transfers may not operate on the same buffer at the same time -//! without introducing undefined behaviour. The method falls back to using -//! word-by-word transfers. -//! //! [`enable`]: Config::enable //! [`gpio`]: crate::gpio //! [`Pin`]: crate::gpio::pin::Pin diff --git a/hal/src/sercom/spi/impl_ehal/dma.rs b/hal/src/sercom/spi/impl_ehal/dma.rs index 021657dae51..a94fb275086 100644 --- a/hal/src/sercom/spi/impl_ehal/dma.rs +++ b/hal/src/sercom/spi/impl_ehal/dma.rs @@ -307,13 +307,18 @@ where #[inline] fn transfer_in_place(&mut self, words: &mut [C::Word]) -> Result<(), Self::Error> { - for word in words { - let read = self.transfer_word_in_place(*word)?; - *word = read; + // Safety: Aliasing the buffer is only safe because the DMA read will always be + // lagging one word behind the write, so they don't overlap on the same memory. + // It's preferable to use two `SharedSliceBuffer`s here; using the `words` slice + // directly as a buffer could potentially cause UB issues if not careful when + // aliasing, as it could be easy to create two `&mut` references pointing to the + // same buffer. `read_buf` and `write_buf` may only be read/written to by the + // DMAC, otherwise an `UnsafeCell` would be necessary. + unsafe { + let mut read_buf = SharedSliceBuffer::from_slice_unchecked(words); + let mut write_buf = SharedSliceBuffer::from_slice(words); + self.transfer_blocking(&mut read_buf, &mut write_buf) } - - self.flush_tx(); - Ok(()) } #[inline] diff --git a/hal/src/sercom/spi/impl_ehal.rs b/hal/src/sercom/spi/impl_ehal/mod.rs similarity index 97% rename from hal/src/sercom/spi/impl_ehal.rs rename to hal/src/sercom/spi/impl_ehal/mod.rs index 6ba0cdda861..b1ebaee830c 100644 --- a/hal/src/sercom/spi/impl_ehal.rs +++ b/hal/src/sercom/spi/impl_ehal/mod.rs @@ -10,9 +10,8 @@ mod dma; mod panic_on; #[hal_module( - any("sercom0-d11", "sercom0-d21") => "impl_ehal_thumbv6m.rs", - "sercom0-d5x" => "impl_ehal_thumbv7em.rs" -)] + any("sercom0-d11", "sercom0-d21") => "thumbv6m.rs", + "sercom0-d5x" => "thumbv7em.rs")] pub mod impl_ehal_02 {} impl spi::Error for Error { @@ -133,6 +132,7 @@ where /// Wait on TXC and RXC flags #[inline] + #[cfg(feature = "dma")] fn flush_tx_rx(&mut self) -> Result<(), Error> { self.block_on_flags(Flags::TXC | Flags::RXC) } diff --git a/hal/src/sercom/spi/impl_ehal/panic_on.rs b/hal/src/sercom/spi/impl_ehal/panic_on.rs index 1aa2bc808fb..6cbaa4c4391 100644 --- a/hal/src/sercom/spi/impl_ehal/panic_on.rs +++ b/hal/src/sercom/spi/impl_ehal/panic_on.rs @@ -5,8 +5,8 @@ use num_traits::{AsPrimitive, PrimInt}; use crate::ehal::spi::{ErrorType, SpiBus}; use super::{ - Config, DataWidth, MasterMode, NoneT, PanicOnRead, PanicOnWrite, Rx, Sercom, Size, Spi, Tx, - ValidConfig, ValidPads, Word, + Config, DataWidth, MasterMode, PanicOnRead, PanicOnWrite, Rx, Size, Spi, Tx, ValidConfig, + ValidPads, Word, }; impl ErrorType for PanicOnRead { @@ -94,6 +94,8 @@ where mod dma { use super::*; use crate::dmac::{AnyChannel, Beat, Ready}; + use crate::sercom::Sercom; + use crate::typelevel::NoneT; /// [`SpiBus`] implementation for [`PanicOnRead`] using DMA transfers impl SpiBus> for PanicOnRead, Tx, NoneT, T>> diff --git a/hal/src/sercom/spi/impl_ehal_thumbv6m.rs b/hal/src/sercom/spi/impl_ehal/thumbv6m.rs similarity index 100% rename from hal/src/sercom/spi/impl_ehal_thumbv6m.rs rename to hal/src/sercom/spi/impl_ehal/thumbv6m.rs diff --git a/hal/src/sercom/spi/impl_ehal_thumbv7em.rs b/hal/src/sercom/spi/impl_ehal/thumbv7em.rs similarity index 100% rename from hal/src/sercom/spi/impl_ehal_thumbv7em.rs rename to hal/src/sercom/spi/impl_ehal/thumbv7em.rs