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 <[email protected]>

hal/src/dmac/channel/mod.rs

@@ -160,11 +160,7 @@ impl<Id: ChId, S: Status> Channel<Id, S> {
 
         #[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,21 +215,36 @@ impl<Id: ChId, S: Status> Channel<Id, S> {
         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());
     }
 
     /// Stop transfer on channel whether or not the transfer has completed
     #[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<Id: ChId> Channel<Id, Ready> {
     #[inline]
     pub fn reset(mut self) -> Channel<Id, Uninitialized> {
         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<Id: ChId> Channel<Id, Ready> {
         // 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<Id: ChId> Channel<Id, Ready> {
         }
 
         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<Id: ChId> Channel<Id, Busy> {
     /// [`Transfer`](super::transfer::Transfer)
     #[inline]
     pub(crate) fn free(mut self) -> Channel<Id, Ready> {
-        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<Id: ChId> Channel<Id, Busy> {
     /// 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<Id: ChId> From<Channel<Id, Ready>> for Channel<Id, Uninitialized> {
-    fn from(item: Channel<Id, Ready>) -> Self {
-        Channel {
-            regs: item.regs,
-            _status: PhantomData,
-        }
+    fn from(mut item: Channel<Id, Ready>) -> 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.
 ///

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<Id: ChId> {
@@ -312,3 +315,19 @@ impl<Id: ChId> RegisterBlock<Id> {
         }
     }
 }
+
+impl<Id: ChId> Drop for RegisterBlock<Id> {
+    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); // ▼
+    }
+}

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
 //!

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<ChannelId<C>, 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)
     }
 }

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<C: AnyConfig, D = crate::typelevel::NoneT> {
     config: C,
-    dma_channel: D,
+    _dma_channel: D,
 }
 
 impl<C: AnyConfig, D> I2c<C, D> {
@@ -430,7 +430,7 @@ impl<C: AnyConfig> I2c<C> {
     ) -> I2c<C, Chan> {
         I2c {
             config: self.config,
-            dma_channel: channel,
+            _dma_channel: channel,
         }
     }
 }
@@ -442,9 +442,9 @@ impl<C: AnyConfig, D: crate::dmac::AnyChannel<Status = crate::dmac::Ready>> I2c<
         (
             I2c {
                 config: self.config,
-                dma_channel: crate::typelevel::NoneT,
+                _dma_channel: crate::typelevel::NoneT,
             },
-            self.dma_channel,
+            self._dma_channel,
         )
     }
 }

hal/src/sercom/i2c/config.rs

@@ -232,7 +232,7 @@ impl<P: PadSet> Config<P> {
 
         I2c {
             config: self,
-            dma_channel: NoneT,
+            _dma_channel: NoneT,
         }
     }
 }

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(())
         }
     }

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

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]

hal/src/sercom/spi/impl_ehal.rs → 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)
     }

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<T: ErrorType> ErrorType for PanicOnRead<T> {
@@ -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<P, M, C, T, S> SpiBus<Word<C>> for PanicOnRead<Spi<Config<P, M, C>, Tx, NoneT, T>>