Skip to content

Commit 22de3d0

Browse files
Sh3Rm4nSh3Rm4n
authored
Enforce documenting unsafe code (#348)
1 parent b4bb14c commit 22de3d0

File tree

15 files changed

+149
-87
lines changed

15 files changed

+149
-87
lines changed

‎src/adc.rs

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1025,23 +1025,26 @@ where
10251025
}
10261026

10271027
// Set the channel in the right sequence field
1028-
match sequence {
1029-
config::Sequence::One => self.reg.sqr1.modify(|_, w| w.sq1().bits(channel.into())),
1030-
config::Sequence::Two => self.reg.sqr1.modify(|_, w| w.sq2().bits(channel.into())),
1031-
config::Sequence::Three => self.reg.sqr1.modify(|_, w| w.sq3().bits(channel.into())),
1032-
config::Sequence::Four => self.reg.sqr1.modify(|_, w| w.sq4().bits(channel.into())),
1033-
config::Sequence::Five => self.reg.sqr2.modify(|_, w| w.sq5().bits(channel.into())),
1034-
config::Sequence::Six => self.reg.sqr2.modify(|_, w| w.sq6().bits(channel.into())),
1035-
config::Sequence::Seven => self.reg.sqr2.modify(|_, w| w.sq7().bits(channel.into())),
1036-
config::Sequence::Eight => self.reg.sqr2.modify(|_, w| w.sq8().bits(channel.into())),
1037-
config::Sequence::Nine => self.reg.sqr2.modify(|_, w| w.sq9().bits(channel.into())),
1038-
config::Sequence::Ten => self.reg.sqr3.modify(|_, w| w.sq10().bits(channel.into())),
1039-
config::Sequence::Eleven => self.reg.sqr3.modify(|_, w| w.sq11().bits(channel.into())),
1040-
config::Sequence::Twelve => self.reg.sqr3.modify(|_, w| w.sq12().bits(channel.into())),
1041-
config::Sequence::Thirteen => self.reg.sqr3.modify(|_, w| w.sq13().bits(channel.into())),
1042-
config::Sequence::Fourteen => self.reg.sqr3.modify(|_, w| w.sq14().bits(channel.into())),
1043-
config::Sequence::Fifteen => self.reg.sqr4.modify(|_, w| w.sq15().bits(channel.into())),
1044-
config::Sequence::Sixteen => self.reg.sqr4.modify(|_, w| w.sq16().bits(channel.into())),
1028+
// SAFETY: the channel.into() implementation ensures that those are valid values
1029+
unsafe {
1030+
match sequence {
1031+
config::Sequence::One => self.reg.sqr1.modify(|_, w| w.sq1().bits(channel.into())),
1032+
config::Sequence::Two => self.reg.sqr1.modify(|_, w| w.sq2().bits(channel.into())),
1033+
config::Sequence::Three => self.reg.sqr1.modify(|_, w| w.sq3().bits(channel.into())),
1034+
config::Sequence::Four => self.reg.sqr1.modify(|_, w| w.sq4().bits(channel.into())),
1035+
config::Sequence::Five => self.reg.sqr2.modify(|_, w| w.sq5().bits(channel.into())),
1036+
config::Sequence::Six => self.reg.sqr2.modify(|_, w| w.sq6().bits(channel.into())),
1037+
config::Sequence::Seven => self.reg.sqr2.modify(|_, w| w.sq7().bits(channel.into())),
1038+
config::Sequence::Eight => self.reg.sqr2.modify(|_, w| w.sq8().bits(channel.into())),
1039+
config::Sequence::Nine => self.reg.sqr2.modify(|_, w| w.sq9().bits(channel.into())),
1040+
config::Sequence::Ten => self.reg.sqr3.modify(|_, w| w.sq10().bits(channel.into())),
1041+
config::Sequence::Eleven => self.reg.sqr3.modify(|_, w| w.sq11().bits(channel.into())),
1042+
config::Sequence::Twelve => self.reg.sqr3.modify(|_, w| w.sq12().bits(channel.into())),
1043+
config::Sequence::Thirteen => self.reg.sqr3.modify(|_, w| w.sq13().bits(channel.into())),
1044+
config::Sequence::Fourteen => self.reg.sqr3.modify(|_, w| w.sq14().bits(channel.into())),
1045+
config::Sequence::Fifteen => self.reg.sqr4.modify(|_, w| w.sq15().bits(channel.into())),
1046+
config::Sequence::Sixteen => self.reg.sqr4.modify(|_, w| w.sq16().bits(channel.into())),
1047+
}
10451048
}
10461049
}
10471050

@@ -1323,10 +1326,14 @@ where
13231326

13241327
/// Synchronously record a single sample of `pin`.
13251328
fn read(&mut self, _pin: &mut Pin) -> nb::Result<Word, Self::Error> {
1329+
// NOTE: as ADC is not configurable (`OneShot` does not implement `Configurable`), we can't
1330+
// use the public methods to configure the ADC but have to fallback to the lower level
1331+
// methods.
1332+
13261333
// self.set_pin_sequence_position(config::Sequence::One, pin);
1327-
self.reg
1328-
.sqr1
1329-
.modify(|_, w| unsafe { w.sq1().bits(Pin::channel().into()) });
1334+
self.reg.sqr1.modify(|_, w|
1335+
// SAFETY: channel().into() ensure the right channel value
1336+
unsafe { w.sq1().bits(Pin::channel().into()) });
13301337

13311338
// Wait for the sequence to complete
13321339

‎src/can.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,12 @@ where
6868
}
6969
}
7070

71+
// SAFETY: Can has ownership of the CAN peripheral and the pointer is pointing to the CAN peripheral
7172
unsafe impl<Tx, Rx> bxcan::Instance for Can<Tx, Rx> {
7273
const REGISTERS: *mut RegisterBlock = pac::CAN::ptr() as *mut _;
7374
}
7475

76+
// SAFETY: The peripheral does own it's associated filter banks
7577
unsafe impl<Tx, Rx> bxcan::FilterOwner for Can<Tx, Rx> {
7678
const NUM_FILTER_BANKS: u8 = 28;
7779
}

‎src/dac.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ impl Dac {
2929
pub fn write_data(&mut self, data: u16) {
3030
self.regs.dhr12r1.write(|w| {
3131
#[allow(unused_unsafe)]
32+
// SAFETY: Direct write to register for easier sharing between different stm32f3xx svd
33+
// generated API
3234
unsafe {
3335
w.dacc1dhr().bits(data)
3436
}

‎src/dma.rs

Lines changed: 35 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -64,21 +64,26 @@ impl<B, C: Channel, T: Target> Transfer<B, C, T> {
6464
B: WriteBuffer + 'static,
6565
T: OnChannel<C>,
6666
{
67-
// NOTE(unsafe) We don't know the concrete type of `buffer` here, all
67+
// SAFETY: We don't know the concrete type of `buffer` here, all
6868
// we can use are its `WriteBuffer` methods. Hence the only `&mut self`
6969
// method we can call is `write_buffer`, which is allowed by
7070
// `WriteBuffer`'s safety requirements.
7171
let (ptr, len) = unsafe { buffer.write_buffer() };
7272
let len = crate::expect!(u16::try_from(len).ok(), "buffer is too large");
7373

74-
// NOTE(unsafe) We are using the address of a 'static WriteBuffer here,
74+
// SAFETY: We are using the address of a 'static WriteBuffer here,
7575
// which is guaranteed to be safe for DMA.
7676
unsafe { channel.set_memory_address(ptr as u32, Increment::Enable) };
7777
channel.set_transfer_length(len);
7878
channel.set_word_size::<B::Word>();
7979
channel.set_direction(Direction::FromPeripheral);
8080

81-
unsafe { Self::start(buffer, channel, target) }
81+
// SAFTEY: we take ownership of the buffer, which is 'static as well, so it lives long
82+
// enough (at least longer that the DMA transfer itself)
83+
#[allow(clippy::undocumented_unsafe_blocks)]
84+
unsafe {
85+
Self::start(buffer, channel, target)
86+
}
8287
}
8388

8489
/// Start a DMA read transfer.
@@ -91,21 +96,26 @@ impl<B, C: Channel, T: Target> Transfer<B, C, T> {
9196
B: ReadBuffer + 'static,
9297
T: OnChannel<C>,
9398
{
94-
// NOTE(unsafe) We don't know the concrete type of `buffer` here, all
99+
// SAFETY: We don't know the concrete type of `buffer` here, all
95100
// we can use are its `ReadBuffer` methods. Hence there are no
96101
// `&mut self` methods we can call, so we are safe according to
97102
// `ReadBuffer`'s safety requirements.
98103
let (ptr, len) = unsafe { buffer.read_buffer() };
99104
let len = crate::expect!(u16::try_from(len).ok(), "buffer is too large");
100105

101-
// NOTE(unsafe) We are using the address of a 'static ReadBuffer here,
106+
// SAFETY: We are using the address of a 'static ReadBuffer here,
102107
// which is guaranteed to be safe for DMA.
103108
unsafe { channel.set_memory_address(ptr as u32, Increment::Enable) };
104109
channel.set_transfer_length(len);
105110
channel.set_word_size::<B::Word>();
106111
channel.set_direction(Direction::FromMemory);
107112

108-
unsafe { Self::start(buffer, channel, target) }
113+
// SAFTEY: We take ownership of the buffer, which is 'static as well, so it lives long
114+
// enough (at least longer that the DMA transfer itself)
115+
#[allow(clippy::undocumented_unsafe_blocks)]
116+
unsafe {
117+
Self::start(buffer, channel, target)
118+
}
109119
}
110120

111121
/// # Safety
@@ -315,7 +325,10 @@ pub trait Channel: private::Channel {
315325
unsafe fn set_peripheral_address(&mut self, address: u32, inc: Increment) {
316326
crate::assert!(!self.is_enabled());
317327

318-
self.ch().par.write(|w| w.pa().bits(address));
328+
// SAFETY: If the caller does ensure, that address is valid address, this should be safe
329+
unsafe {
330+
self.ch().par.write(|w| w.pa().bits(address));
331+
}
319332
self.ch().cr.modify(|_, w| w.pinc().variant(inc.into()));
320333
}
321334

@@ -335,7 +348,10 @@ pub trait Channel: private::Channel {
335348
unsafe fn set_memory_address(&mut self, address: u32, inc: Increment) {
336349
crate::assert!(!self.is_enabled());
337350

338-
self.ch().mar.write(|w| w.ma().bits(address));
351+
// SAFETY: If the caller does ensure, that address is valid address, this should be safe
352+
unsafe {
353+
self.ch().mar.write(|w| w.ma().bits(address));
354+
}
339355
self.ch().cr.modify(|_, w| w.minc().variant(inc.into()));
340356
}
341357

@@ -500,35 +516,31 @@ macro_rules! dma {
500516

501517
impl private::Channel for $Ci {
502518
fn ch(&self) -> &pac::dma1::CH {
503-
// NOTE(unsafe) $Ci grants exclusive access to this register
519+
// SAFETY: $Ci grants exclusive access to this register
504520
unsafe { &(*$DMAx::ptr()).$chi }
505521
}
506522
}
507523

508524
impl Channel for $Ci {
509525
fn is_event_triggered(&self, event: Event) -> bool {
510-
use Event::*;
511-
512-
// NOTE(unsafe) atomic read
526+
// SAFETY: atomic read
513527
let flags = unsafe { (*$DMAx::ptr()).isr.read() };
514528
match event {
515-
HalfTransfer => flags.$htifi().bit_is_set(),
516-
TransferComplete => flags.$tcifi().bit_is_set(),
517-
TransferError => flags.$teifi().bit_is_set(),
518-
Any => flags.$gifi().bit_is_set(),
529+
Event::HalfTransfer => flags.$htifi().bit_is_set(),
530+
Event::TransferComplete => flags.$tcifi().bit_is_set(),
531+
Event::TransferError => flags.$teifi().bit_is_set(),
532+
Event::Any => flags.$gifi().bit_is_set(),
519533
}
520534
}
521535

522536
fn clear_event(&mut self, event: Event) {
523-
use Event::*;
524-
525-
// NOTE(unsafe) atomic write to a stateless register
537+
// SAFETY: atomic write to a stateless register
526538
unsafe {
527539
(*$DMAx::ptr()).ifcr.write(|w| match event {
528-
HalfTransfer => w.$chtifi().set_bit(),
529-
TransferComplete => w.$ctcifi().set_bit(),
530-
TransferError => w.$cteifi().set_bit(),
531-
Any => w.$cgifi().set_bit(),
540+
Event::HalfTransfer => w.$chtifi().set_bit(),
541+
Event::TransferComplete => w.$ctcifi().set_bit(),
542+
Event::TransferError => w.$cteifi().set_bit(),
543+
Event::Any => w.$cgifi().set_bit(),
532544
});
533545
}
534546
}

‎src/gpio.rs

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -181,13 +181,13 @@ impl defmt::Format for Gpiox {
181181
}
182182
}
183183

184-
// # SAFETY
184+
// SAFETY:
185185
// As Gpiox uses `dyn GpioRegExt` pointer internally, `Send` is not auto-implemented.
186186
// But since GpioExt does only do atomic operations without side-effects we can assume
187187
// that it safe to `Send` this type.
188188
unsafe impl Send for Gpiox {}
189189

190-
// # SAFETY
190+
// SAFETY:
191191
// As Gpiox uses `dyn GpioRegExt` pointer internally, `Sync` is not auto-implemented.
192192
// But since GpioExt does only do atomic operations without side-effects we can assume
193193
// that it safe to `Send` this type.
@@ -500,13 +500,13 @@ where
500500
type Error = Infallible;
501501

502502
fn set_high(&mut self) -> Result<(), Self::Error> {
503-
// NOTE(unsafe) atomic write to a stateless register
503+
// SAFETY: atomic write to a stateless register
504504
unsafe { (*self.gpio.ptr()).set_high(self.index.index()) };
505505
Ok(())
506506
}
507507

508508
fn set_low(&mut self) -> Result<(), Self::Error> {
509-
// NOTE(unsafe) atomic write to a stateless register
509+
// SAFETY: atomic write to a stateless register
510510
unsafe { (*self.gpio.ptr()).set_low(self.index.index()) };
511511
Ok(())
512512
}
@@ -525,7 +525,7 @@ where
525525
}
526526

527527
fn is_low(&self) -> Result<bool, Self::Error> {
528-
// NOTE(unsafe) atomic read with no side effects
528+
// SAFETY: atomic read with no side effects
529529
Ok(unsafe { (*self.gpio.ptr()).is_low(self.index.index()) })
530530
}
531531
}
@@ -540,7 +540,7 @@ where
540540
}
541541

542542
fn is_set_low(&self) -> Result<bool, Self::Error> {
543-
// NOTE(unsafe) atomic read with no side effects
543+
// SAFETY: atomic read with no side effects
544544
Ok(unsafe { (*self.gpio.ptr()).is_set_low(self.index.index()) })
545545
}
546546
}
@@ -763,13 +763,13 @@ macro_rules! gpio_trait {
763763

764764
#[inline(always)]
765765
fn set_high(&self, i: u8) {
766-
// NOTE(unsafe, write) atomic write to a stateless register
766+
// SAFETY: atomic write to a stateless register
767767
unsafe { self.bsrr.write(|w| w.bits(1 << i)) };
768768
}
769769

770770
#[inline(always)]
771771
fn set_low(&self, i: u8) {
772-
// NOTE(unsafe, write) atomic write to a stateless register
772+
// SAFETY: atomic write to a stateless register
773773
unsafe { self.bsrr.write(|w| w.bits(1 << (16 + i))) };
774774
}
775775
}
@@ -792,6 +792,8 @@ macro_rules! r_trait {
792792
#[inline]
793793
fn $fn(&mut self, i: u8) {
794794
let value = $gpioy::$xr::$enum::$VARIANT as u32;
795+
// SAFETY: The &mut abstracts all accesses to the register itself,
796+
// which makes sure that this register accesss is exclusive
795797
unsafe { crate::modify_at!((*$GPIOX::ptr()).$xr, $bitwidth, i, value) };
796798
}
797799
)+
@@ -931,6 +933,8 @@ macro_rules! gpio {
931933
#[inline]
932934
fn afx(&mut self, i: u8, x: u8) {
933935
const BITWIDTH: u8 = 4;
936+
// SAFETY: the abstraction of AFRL should ensure exclusive access in addition
937+
// to the &mut exclusive reference
934938
unsafe { crate::modify_at!((*$GPIOX::ptr()).afrh, BITWIDTH, i - 8, x as u32) };
935939
}
936940
}
@@ -942,6 +946,8 @@ macro_rules! gpio {
942946
#[inline]
943947
fn afx(&mut self, i: u8, x: u8) {
944948
const BITWIDTH: u8 = 4;
949+
// SAFETY: the abstraction of AFRL should ensure exclusive access in addition
950+
// to the &mut exclusive reference
945951
unsafe { crate::modify_at!((*$GPIOX::ptr()).afrl, BITWIDTH, i, x as u32) };
946952
}
947953
}

‎src/i2c.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -472,7 +472,7 @@ macro_rules! i2c {
472472
$(
473473
impl Instance for $I2CX {
474474
fn clock(clocks: &Clocks) -> Hertz {
475-
// NOTE(unsafe) atomic read with no side effects
475+
// SAFETY: atomic read of valid pointer with no side effects
476476
match unsafe { (*RCC::ptr()).cfgr3.read().$i2cXsw().variant() } {
477477
I2C1SW_A::Hsi => crate::rcc::HSI,
478478
I2C1SW_A::Sysclk => clocks.sysclk(),

‎src/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,9 @@
123123
#![no_std]
124124
#![allow(clippy::upper_case_acronyms)]
125125
#![warn(missing_docs)]
126+
#![warn(clippy::missing_safety_doc)]
127+
#![warn(clippy::undocumented_unsafe_blocks)]
128+
#![warn(unsafe_op_in_unsafe_fn)]
126129
#![deny(macro_use_extern_crate)]
127130
#![cfg_attr(nightly, deny(rustdoc::broken_intra_doc_links))]
128131
#![cfg_attr(docsrs, feature(doc_cfg))]

‎src/pwm.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,10 @@
155155
[examples/pwm.rs]: https://github.com/stm32-rs/stm32f3xx-hal/blob/v0.6.1/examples/pwm.rs
156156
*/
157157

158+
// FIXME: this PWM module needs refactoring to ensure that no UB / race conditiotns is caused by unsafe acces to
159+
// mmaped registers.
160+
#![allow(clippy::undocumented_unsafe_blocks)]
161+
158162
use core::marker::PhantomData;
159163

160164
use crate::{

‎src/rcc.rs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -167,13 +167,15 @@ macro_rules! bus_struct {
167167

168168
#[allow(unused)]
169169
fn enr(&self) -> &rcc::$EN {
170-
// NOTE(unsafe) this proxy grants exclusive access to this register
170+
// FIXME: this should still be shared carefully
171+
// SAFETY: this proxy grants exclusive access to this register
171172
unsafe { &(*RCC::ptr()).$en }
172173
}
173174

174175
#[allow(unused)]
175176
fn rstr(&self) -> &rcc::$RST {
176-
// NOTE(unsafe) this proxy grants exclusive access to this register
177+
// FIXME: this should still be shared carefully
178+
// SAFETY: this proxy grants exclusive access to this register
177179
unsafe { &(*RCC::ptr()).$rst }
178180
}
179181
}
@@ -365,7 +367,7 @@ impl BDCR {
365367
#[allow(clippy::unused_self)]
366368
#[must_use]
367369
pub(crate) fn bdcr(&mut self) -> &rcc::BDCR {
368-
// NOTE(unsafe) this proxy grants exclusive access to this register
370+
// SAFETY: this proxy grants exclusive access to this register
369371
unsafe { &(*RCC::ptr()).bdcr }
370372
}
371373
}
@@ -860,6 +862,8 @@ impl CFGR {
860862

861863
let (usbpre, usbclk_valid) = usb_clocking::is_valid(sysclk, self.hse, pclk1, &pll_config);
862864

865+
// SAFETY: RCC ptr is guaranteed to be valid. This funciton is the first, which uses the
866+
// RCC peripheral: RCC.constrain() -> Rcc -> CFGR
863867
let rcc = unsafe { &*RCC::ptr() };
864868

865869
// enable HSE and wait for it to be ready

‎src/rtc.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,8 @@ impl Rtc {
115115
F: FnMut(&mut RTC),
116116
{
117117
// Disable write protection
118-
self.rtc.wpr.write(|w| unsafe { w.bits(0xCA) });
119-
self.rtc.wpr.write(|w| unsafe { w.bits(0x53) });
118+
self.rtc.wpr.write(|w| w.key().bits(0xCA));
119+
self.rtc.wpr.write(|w| w.key().bits(0x53));
120120
// Enter init mode
121121
let isr = self.rtc.isr.read();
122122
if isr.initf().bit_is_clear() {
@@ -253,6 +253,7 @@ impl Rtcc for Rtc {
253253
if !(1..=7).contains(&weekday) {
254254
return Err(Error::InvalidInputData);
255255
}
256+
// SAFETY: check above ensures, that the weekday number is in the valid range (0x01 - 0x07)
256257
self.modify(|rtc| rtc.dr.modify(|_, w| unsafe { w.wdu().bits(weekday) }));
257258

258259
Ok(())

0 commit comments

Comments
 (0)