quartiq / stabilizer

Firmware and software for the Sinara Stabilizer module with high speed, low latency ADC/DAC data processing and powerful DSP algorithms in between
http://quartiq.de/stabilizer/
Apache License 2.0
111 stars 25 forks source link

Review DMA implementation for Undefined Behavior (UB) #906

Open ryan-summers opened 4 months ago

ryan-summers commented 4 months ago

It was noted by James Munns that we may need to use an UnsafeCell internal wrapper on the DMA reception buffers to prevent invoking undefined behavior by the Rust compiler (i.e. two owners of the memory cell)

jamesmunns commented 4 months ago

@ryan-summers if you can point me to the driver impl(s) I can give a review, or at least my opinions.

ryan-summers commented 4 months ago

The implementations are specifically in:

Note that there's a few single-byte transfers in there that are not concerning - the memory is static and entirely owned by the DMA module and is never modified.

It's worth noting that this is a double-buffered DMA implementation using the STM32H7xx-Hal. This is implemented here for swapping the DMA double-buffer for the next transfer

dnadlinger commented 1 month ago

Possibly related: nightly now emits warnings like this (same for other DMA/Ethernet description ring buffers), and it seems like this is going to turn into deny by default in 2024 still

warning: creating a mutable reference to mutable static is discouraged
   --> src/hardware/adc.rs:378:25
    |
378 |                           ADC_BUF.write(Default::default())
    |                           ^^^^^^^ mutable reference to mutable static
...
446 | / adc_input!(
447 | |     Adc1Input, 1, Stream3, Stream4, Stream5, SPI3, Channel2, Tim2Ch2, Channel2,
448 | |     Tim3Ch2
449 | | );
    | |_- in this macro invocation
    |
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html>
    = note: mutable references to mutable statics are dangerous; it's undefined behavior if any other pointer to the static is used or if any other reference is created for the static while the mutable reference lives
    = note: this warning originates in the macro `adc_input` (in Nightly builds, run with -Z macro-backtrace for more info)

I am not sure I understand the documentation here – is ptr::addr_of_mut!(ADC_BUF) a valid "fix"?