Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce documenting unsafe code #348

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Enforce documenting unsafe code #348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
520886a
Deprecate pwm implementation
Sh3Rm4n Nov 26, 2023
60991a1
Remove Serial::split
Sh3Rm4n Nov 26, 2023
1cf0183
Enforce documenting unsafe code
Sh3Rm4n Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 27 additions & 20 deletions src/adc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())),
}
}
}

Expand Down Expand Up @@ -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

Expand Down
2 changes: 2 additions & 0 deletions src/can.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
2 changes: 2 additions & 0 deletions src/dac.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
58 changes: 35 additions & 23 deletions src/dma.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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()));
}

Expand All @@ -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()));
}

Expand Down Expand Up @@ -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(),
});
}
}
Expand Down
22 changes: 14 additions & 8 deletions src/gpio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(())
}
Expand All @@ -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()) })
}
}
Expand All @@ -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()) })
}
}
Expand Down Expand Up @@ -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))) };
}
}
Expand All @@ -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) };
}
)+
Expand Down Expand Up @@ -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) };
}
}
Expand All @@ -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) };
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/i2c.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
3 changes: 3 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Expand Down
4 changes: 4 additions & 0 deletions src/pwm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down
10 changes: 7 additions & 3 deletions src/rcc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
}
}
Expand Down Expand Up @@ -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 }
}
}
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions src/rtc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ impl Rtc {
F: FnMut(&mut RTC),
{
// Disable write protection
self.rtc.wpr.write(|w| unsafe { w.bits(0xCA) });
self.rtc.wpr.write(|w| unsafe { w.bits(0x53) });
self.rtc.wpr.write(|w| w.key().bits(0xCA));
self.rtc.wpr.write(|w| w.key().bits(0x53));
// Enter init mode
let isr = self.rtc.isr.read();
if isr.initf().bit_is_clear() {
Expand Down Expand Up @@ -253,6 +253,7 @@ impl Rtcc for Rtc {
if !(1..=7).contains(&weekday) {
return Err(Error::InvalidInputData);
}
// SAFETY: check above ensures, that the weekday number is in the valid range (0x01 - 0x07)
self.modify(|rtc| rtc.dr.modify(|_, w| unsafe { w.wdu().bits(weekday) }));

Ok(())
Expand Down
Loading
Loading