Enforce documenting unsafe code

CHANGELOG.md

@@ -27,7 +27,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Remove `Serial::split`, which possibly creates two mutable references two
   one Serial instance, which could've caused UB. The use case of this function
   was hard to find out anyway. ([#351])
-  was hard to find out anyway.
 
 ### Added
 
@@ -45,7 +44,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   implementation, but it is hard to maintain the current implementation
   and not easy to verify if it is really a safe implementation.
   It is also not consistent with the rest of the crates API. ([#352])
-  It is also not consistent with the rest of the crates API.
 
 ## [v0.9.2] - 2023-02-20
 

src/adc.rs

@@ -1025,23 +1025,26 @@ where
         }
 
         // Set the channel in the right sequence field
-        match sequence {
-            config::Sequence::One      => self.reg.sqr1.modify(|_, w| w.sq1().bits(channel.into())),
-            config::Sequence::Two      => self.reg.sqr1.modify(|_, w| w.sq2().bits(channel.into())),
-            config::Sequence::Three    => self.reg.sqr1.modify(|_, w| w.sq3().bits(channel.into())),
-            config::Sequence::Four     => self.reg.sqr1.modify(|_, w| w.sq4().bits(channel.into())),
-            config::Sequence::Five     => self.reg.sqr2.modify(|_, w| w.sq5().bits(channel.into())),
-            config::Sequence::Six      => self.reg.sqr2.modify(|_, w| w.sq6().bits(channel.into())),
-            config::Sequence::Seven    => self.reg.sqr2.modify(|_, w| w.sq7().bits(channel.into())),
-            config::Sequence::Eight    => self.reg.sqr2.modify(|_, w| w.sq8().bits(channel.into())),
-            config::Sequence::Nine     => self.reg.sqr2.modify(|_, w| w.sq9().bits(channel.into())),
-            config::Sequence::Ten      => self.reg.sqr3.modify(|_, w| w.sq10().bits(channel.into())),
-            config::Sequence::Eleven   => self.reg.sqr3.modify(|_, w| w.sq11().bits(channel.into())),
-            config::Sequence::Twelve   => self.reg.sqr3.modify(|_, w| w.sq12().bits(channel.into())),
-            config::Sequence::Thirteen => self.reg.sqr3.modify(|_, w| w.sq13().bits(channel.into())),
-            config::Sequence::Fourteen => self.reg.sqr3.modify(|_, w| w.sq14().bits(channel.into())),
-            config::Sequence::Fifteen  => self.reg.sqr4.modify(|_, w| w.sq15().bits(channel.into())),
-            config::Sequence::Sixteen  => self.reg.sqr4.modify(|_, w| w.sq16().bits(channel.into())),
+        // SAFETY: the channel.into() implementation ensures that those are valid values
+        unsafe {
+            match sequence {
+                config::Sequence::One      => self.reg.sqr1.modify(|_, w| w.sq1().bits(channel.into())),
+                config::Sequence::Two      => self.reg.sqr1.modify(|_, w| w.sq2().bits(channel.into())),
+                config::Sequence::Three    => self.reg.sqr1.modify(|_, w| w.sq3().bits(channel.into())),
+                config::Sequence::Four     => self.reg.sqr1.modify(|_, w| w.sq4().bits(channel.into())),
+                config::Sequence::Five     => self.reg.sqr2.modify(|_, w| w.sq5().bits(channel.into())),
+                config::Sequence::Six      => self.reg.sqr2.modify(|_, w| w.sq6().bits(channel.into())),
+                config::Sequence::Seven    => self.reg.sqr2.modify(|_, w| w.sq7().bits(channel.into())),
+                config::Sequence::Eight    => self.reg.sqr2.modify(|_, w| w.sq8().bits(channel.into())),
+                config::Sequence::Nine     => self.reg.sqr2.modify(|_, w| w.sq9().bits(channel.into())),
+                config::Sequence::Ten      => self.reg.sqr3.modify(|_, w| w.sq10().bits(channel.into())),
+                config::Sequence::Eleven   => self.reg.sqr3.modify(|_, w| w.sq11().bits(channel.into())),
+                config::Sequence::Twelve   => self.reg.sqr3.modify(|_, w| w.sq12().bits(channel.into())),
+                config::Sequence::Thirteen => self.reg.sqr3.modify(|_, w| w.sq13().bits(channel.into())),
+                config::Sequence::Fourteen => self.reg.sqr3.modify(|_, w| w.sq14().bits(channel.into())),
+                config::Sequence::Fifteen  => self.reg.sqr4.modify(|_, w| w.sq15().bits(channel.into())),
+                config::Sequence::Sixteen  => self.reg.sqr4.modify(|_, w| w.sq16().bits(channel.into())),
+            }
         }
     }
 
@@ -1323,10 +1326,14 @@ where
 
     /// Synchronously record a single sample of `pin`.
     fn read(&mut self, _pin: &mut Pin) -> nb::Result<Word, Self::Error> {
+        // NOTE: as ADC is not configurable (`OneShot` does not implement `Configurable`), we can't
+        // use the public methods to configure the ADC but have to fallback to the lower level
+        // methods.
+
         // self.set_pin_sequence_position(config::Sequence::One, pin);
-        self.reg
-            .sqr1
-            .modify(|_, w| unsafe { w.sq1().bits(Pin::channel().into()) });
+        self.reg.sqr1.modify(|_, w|
+                // SAFETY: channel().into() ensure the right channel value
+                unsafe { w.sq1().bits(Pin::channel().into()) });
 
         // Wait for the sequence to complete
 

src/can.rs

@@ -68,10 +68,12 @@ where
     }
 }
 
+// SAFETY: Can has ownership of the CAN peripheral and the pointer is pointing to the CAN peripheral
 unsafe impl<Tx, Rx> bxcan::Instance for Can<Tx, Rx> {
     const REGISTERS: *mut RegisterBlock = pac::CAN::ptr() as *mut _;
 }
 
+// SAFETY: The peripheral does own it's associated filter banks
 unsafe impl<Tx, Rx> bxcan::FilterOwner for Can<Tx, Rx> {
     const NUM_FILTER_BANKS: u8 = 28;
 }

src/dac.rs

@@ -29,6 +29,8 @@ impl Dac {
     pub fn write_data(&mut self, data: u16) {
         self.regs.dhr12r1.write(|w| {
             #[allow(unused_unsafe)]
+            // SAFETY: Direct write to register for easier sharing between different stm32f3xx svd
+            // generated API
             unsafe {
                 w.dacc1dhr().bits(data)
             }

src/dma.rs

@@ -64,21 +64,26 @@ impl<B, C: Channel, T: Target> Transfer<B, C, T> {
         B: WriteBuffer + 'static,
         T: OnChannel<C>,
     {
-        // NOTE(unsafe) We don't know the concrete type of `buffer` here, all
+        // SAFETY: We don't know the concrete type of `buffer` here, all
         // we can use are its `WriteBuffer` methods. Hence the only `&mut self`
         // method we can call is `write_buffer`, which is allowed by
         // `WriteBuffer`'s safety requirements.
         let (ptr, len) = unsafe { buffer.write_buffer() };
         let len = crate::expect!(u16::try_from(len).ok(), "buffer is too large");
 
-        // NOTE(unsafe) We are using the address of a 'static WriteBuffer here,
+        // SAFETY: We are using the address of a 'static WriteBuffer here,
         // which is guaranteed to be safe for DMA.
         unsafe { channel.set_memory_address(ptr as u32, Increment::Enable) };
         channel.set_transfer_length(len);
         channel.set_word_size::<B::Word>();
         channel.set_direction(Direction::FromPeripheral);
 
-        unsafe { Self::start(buffer, channel, target) }
+        // SAFTEY: we take ownership of the buffer, which is 'static as well, so it lives long
+        // enough (at least longer that the DMA transfer itself)
+        #[allow(clippy::undocumented_unsafe_blocks)]
+        unsafe {
+            Self::start(buffer, channel, target)
+        }
     }
 
     /// Start a DMA read transfer.
@@ -91,21 +96,26 @@ impl<B, C: Channel, T: Target> Transfer<B, C, T> {
         B: ReadBuffer + 'static,
         T: OnChannel<C>,
     {
-        // NOTE(unsafe) We don't know the concrete type of `buffer` here, all
+        // SAFETY: We don't know the concrete type of `buffer` here, all
         // we can use are its `ReadBuffer` methods. Hence there are no
         // `&mut self` methods we can call, so we are safe according to
         // `ReadBuffer`'s safety requirements.
         let (ptr, len) = unsafe { buffer.read_buffer() };
         let len = crate::expect!(u16::try_from(len).ok(), "buffer is too large");
 
-        // NOTE(unsafe) We are using the address of a 'static ReadBuffer here,
+        // SAFETY: We are using the address of a 'static ReadBuffer here,
         // which is guaranteed to be safe for DMA.
         unsafe { channel.set_memory_address(ptr as u32, Increment::Enable) };
         channel.set_transfer_length(len);
         channel.set_word_size::<B::Word>();
         channel.set_direction(Direction::FromMemory);
 
-        unsafe { Self::start(buffer, channel, target) }
+        // SAFTEY: We take ownership of the buffer, which is 'static as well, so it lives long
+        // enough (at least longer that the DMA transfer itself)
+        #[allow(clippy::undocumented_unsafe_blocks)]
+        unsafe {
+            Self::start(buffer, channel, target)
+        }
     }
 
     /// # Safety
@@ -315,7 +325,10 @@ pub trait Channel: private::Channel {
     unsafe fn set_peripheral_address(&mut self, address: u32, inc: Increment) {
         crate::assert!(!self.is_enabled());
 
-        self.ch().par.write(|w| w.pa().bits(address));
+        // SAFETY: If the caller does ensure, that address is valid address, this should be safe
+        unsafe {
+            self.ch().par.write(|w| w.pa().bits(address));
+        }
         self.ch().cr.modify(|_, w| w.pinc().variant(inc.into()));
     }
 
@@ -335,7 +348,10 @@ pub trait Channel: private::Channel {
     unsafe fn set_memory_address(&mut self, address: u32, inc: Increment) {
         crate::assert!(!self.is_enabled());
 
-        self.ch().mar.write(|w| w.ma().bits(address));
+        // SAFETY: If the caller does ensure, that address is valid address, this should be safe
+        unsafe {
+            self.ch().mar.write(|w| w.ma().bits(address));
+        }
         self.ch().cr.modify(|_, w| w.minc().variant(inc.into()));
     }
 
@@ -500,35 +516,31 @@ macro_rules! dma {
 
                     impl private::Channel for $Ci {
                         fn ch(&self) -> &pac::dma1::CH {
-                            // NOTE(unsafe) $Ci grants exclusive access to this register
+                            // SAFETY: $Ci grants exclusive access to this register
                             unsafe { &(*$DMAx::ptr()).$chi }
                         }
                     }
 
                     impl Channel for $Ci {
                         fn is_event_triggered(&self, event: Event) -> bool {
-                            use Event::*;
-
-                            // NOTE(unsafe) atomic read
+                            // SAFETY: atomic read
                             let flags = unsafe { (*$DMAx::ptr()).isr.read() };
                             match event {
-                                HalfTransfer => flags.$htifi().bit_is_set(),
-                                TransferComplete => flags.$tcifi().bit_is_set(),
-                                TransferError => flags.$teifi().bit_is_set(),
-                                Any => flags.$gifi().bit_is_set(),
+                                Event::HalfTransfer => flags.$htifi().bit_is_set(),
+                                Event::TransferComplete => flags.$tcifi().bit_is_set(),
+                                Event::TransferError => flags.$teifi().bit_is_set(),
+                                Event::Any => flags.$gifi().bit_is_set(),
                             }
                         }
 
                         fn clear_event(&mut self, event: Event) {
-                            use Event::*;
-
-                            // NOTE(unsafe) atomic write to a stateless register
+                            // SAFETY: atomic write to a stateless register
                             unsafe {
                                 (*$DMAx::ptr()).ifcr.write(|w| match event {
-                                    HalfTransfer => w.$chtifi().set_bit(),
-                                    TransferComplete => w.$ctcifi().set_bit(),
-                                    TransferError => w.$cteifi().set_bit(),
-                                    Any => w.$cgifi().set_bit(),
+                                    Event::HalfTransfer => w.$chtifi().set_bit(),
+                                    Event::TransferComplete => w.$ctcifi().set_bit(),
+                                    Event::TransferError => w.$cteifi().set_bit(),
+                                    Event::Any => w.$cgifi().set_bit(),
                                 });
                             }
                         }

src/gpio.rs

@@ -181,13 +181,13 @@ impl defmt::Format for Gpiox {
     }
 }
 
-// # SAFETY
+// SAFETY:
 // As Gpiox uses `dyn GpioRegExt` pointer internally, `Send` is not auto-implemented.
 // But since GpioExt does only do atomic operations without side-effects we can assume
 // that it safe to `Send` this type.
 unsafe impl Send for Gpiox {}
 
-// # SAFETY
+// SAFETY:
 // As Gpiox uses `dyn GpioRegExt` pointer internally, `Sync` is not auto-implemented.
 // But since GpioExt does only do atomic operations without side-effects we can assume
 // that it safe to `Send` this type.
@@ -500,13 +500,13 @@ where
     type Error = Infallible;
 
     fn set_high(&mut self) -> Result<(), Self::Error> {
-        // NOTE(unsafe) atomic write to a stateless register
+        // SAFETY: atomic write to a stateless register
         unsafe { (*self.gpio.ptr()).set_high(self.index.index()) };
         Ok(())
     }
 
     fn set_low(&mut self) -> Result<(), Self::Error> {
-        // NOTE(unsafe) atomic write to a stateless register
+        // SAFETY: atomic write to a stateless register
         unsafe { (*self.gpio.ptr()).set_low(self.index.index()) };
         Ok(())
     }
@@ -525,7 +525,7 @@ where
     }
 
     fn is_low(&self) -> Result<bool, Self::Error> {
-        // NOTE(unsafe) atomic read with no side effects
+        // SAFETY: atomic read with no side effects
         Ok(unsafe { (*self.gpio.ptr()).is_low(self.index.index()) })
     }
 }
@@ -540,7 +540,7 @@ where
     }
 
     fn is_set_low(&self) -> Result<bool, Self::Error> {
-        // NOTE(unsafe) atomic read with no side effects
+        // SAFETY: atomic read with no side effects
         Ok(unsafe { (*self.gpio.ptr()).is_set_low(self.index.index()) })
     }
 }
@@ -763,13 +763,13 @@ macro_rules! gpio_trait {
 
                 #[inline(always)]
                 fn set_high(&self, i: u8) {
-                    // NOTE(unsafe, write) atomic write to a stateless register
+                    // SAFETY: atomic write to a stateless register
                     unsafe { self.bsrr.write(|w| w.bits(1 << i)) };
                 }
 
                 #[inline(always)]
                 fn set_low(&self, i: u8) {
-                    // NOTE(unsafe, write) atomic write to a stateless register
+                    // SAFETY: atomic write to a stateless register
                     unsafe { self.bsrr.write(|w| w.bits(1 << (16 + i))) };
                 }
             }
@@ -792,6 +792,8 @@ macro_rules! r_trait {
                 #[inline]
                 fn $fn(&mut self, i: u8) {
                     let value = $gpioy::$xr::$enum::$VARIANT as u32;
+                    // SAFETY: The &mut abstracts all accesses to the register itself,
+                    // which makes sure that this register accesss is exclusive
                     unsafe { crate::modify_at!((*$GPIOX::ptr()).$xr, $bitwidth, i, value) };
                 }
             )+
@@ -931,6 +933,8 @@ macro_rules! gpio {
                 #[inline]
                 fn afx(&mut self, i: u8, x: u8) {
                     const BITWIDTH: u8 = 4;
+                    // SAFETY: the abstraction of AFRL should ensure exclusive access in addition
+                    // to the &mut exclusive reference
                     unsafe { crate::modify_at!((*$GPIOX::ptr()).afrh, BITWIDTH, i - 8, x as u32) };
                 }
             }
@@ -942,6 +946,8 @@ macro_rules! gpio {
                 #[inline]
                 fn afx(&mut self, i: u8, x: u8) {
                     const BITWIDTH: u8 = 4;
+                    // SAFETY: the abstraction of AFRL should ensure exclusive access in addition
+                    // to the &mut exclusive reference
                     unsafe { crate::modify_at!((*$GPIOX::ptr()).afrl, BITWIDTH, i, x as u32) };
                 }
             }

src/i2c.rs

@@ -472,7 +472,7 @@ macro_rules! i2c {
         $(
             impl Instance for $I2CX {
                 fn clock(clocks: &Clocks) -> Hertz {
-                    // NOTE(unsafe) atomic read with no side effects
+                    // SAFETY: atomic read of valid pointer with no side effects
                     match unsafe { (*RCC::ptr()).cfgr3.read().$i2cXsw().variant() } {
                         I2C1SW_A::Hsi => crate::rcc::HSI,
                         I2C1SW_A::Sysclk => clocks.sysclk(),

src/lib.rs

@@ -123,6 +123,9 @@
 #![no_std]
 #![allow(clippy::upper_case_acronyms)]
 #![warn(missing_docs)]
+#![warn(clippy::missing_safety_doc)]
+#![warn(clippy::undocumented_unsafe_blocks)]
+#![warn(unsafe_op_in_unsafe_fn)]
 #![deny(macro_use_extern_crate)]
 #![cfg_attr(nightly, deny(rustdoc::broken_intra_doc_links))]
 #![cfg_attr(docsrs, feature(doc_cfg))]

src/pwm.rs

@@ -155,6 +155,10 @@
   [examples/pwm.rs]: https://github.com/stm32-rs/stm32f3xx-hal/blob/v0.6.1/examples/pwm.rs
 */
 
+// FIXME: this PWM module needs refactoring to ensure that no UB / race conditiotns is caused by unsafe acces to
+// mmaped registers.
+#![allow(clippy::undocumented_unsafe_blocks)]
+
 use core::marker::PhantomData;
 
 use crate::{

src/rcc.rs

@@ -167,13 +167,15 @@ macro_rules! bus_struct {
 
                 #[allow(unused)]
                 fn enr(&self) -> &rcc::$EN {
-                    // NOTE(unsafe) this proxy grants exclusive access to this register
+                    // FIXME: this should still be shared carefully
+                    // SAFETY: this proxy grants exclusive access to this register
                     unsafe { &(*RCC::ptr()).$en }
                 }
 
                 #[allow(unused)]
                 fn rstr(&self) -> &rcc::$RST {
-                    // NOTE(unsafe) this proxy grants exclusive access to this register
+                    // FIXME: this should still be shared carefully
+                    // SAFETY: this proxy grants exclusive access to this register
                     unsafe { &(*RCC::ptr()).$rst }
                 }
             }
@@ -365,7 +367,7 @@ impl BDCR {
     #[allow(clippy::unused_self)]
     #[must_use]
     pub(crate) fn bdcr(&mut self) -> &rcc::BDCR {
-        // NOTE(unsafe) this proxy grants exclusive access to this register
+        // SAFETY: this proxy grants exclusive access to this register
         unsafe { &(*RCC::ptr()).bdcr }
     }
 }
@@ -860,6 +862,8 @@ impl CFGR {
 
         let (usbpre, usbclk_valid) = usb_clocking::is_valid(sysclk, self.hse, pclk1, &pll_config);
 
+        // SAFETY: RCC ptr is guaranteed to be valid. This funciton is the first, which uses the
+        // RCC peripheral: RCC.constrain() -> Rcc -> CFGR
         let rcc = unsafe { &*RCC::ptr() };
 
         // enable HSE and wait for it to be ready