From 8a4ab298192c388bed09fc198ee1786561cd50f9 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Tue, 29 Jun 2021 17:26:16 +1000 Subject: [PATCH 1/6] Add an nRF RNG driver Resolves #187 Like the stm32 driver, this has both a non-blocking and blocking API, and implements `rand_core::RngCore` for the blocking API. --- embassy-nrf/Cargo.toml | 1 + embassy-nrf/src/chips/nrf52805.rs | 3 + embassy-nrf/src/chips/nrf52810.rs | 3 + embassy-nrf/src/chips/nrf52811.rs | 3 + embassy-nrf/src/chips/nrf52820.rs | 3 + embassy-nrf/src/chips/nrf52832.rs | 3 + embassy-nrf/src/chips/nrf52833.rs | 3 + embassy-nrf/src/chips/nrf52840.rs | 3 + embassy-nrf/src/lib.rs | 1 + embassy-nrf/src/rng.rs | 154 ++++++++++++++++++++++++++++++ examples/nrf/Cargo.toml | 1 + examples/nrf/src/bin/rng.rs | 30 ++++++ 12 files changed, 208 insertions(+) create mode 100644 embassy-nrf/src/rng.rs create mode 100644 examples/nrf/src/bin/rng.rs diff --git a/embassy-nrf/Cargo.toml b/embassy-nrf/Cargo.toml index eeba1f54e..7b8e36414 100644 --- a/embassy-nrf/Cargo.toml +++ b/embassy-nrf/Cargo.toml @@ -33,6 +33,7 @@ embedded-hal = { version = "0.2.4" } embedded-dma = { version = "0.1.2" } futures = { version = "0.3.5", default-features = false } critical-section = "0.2.1" +rand_core = "0.6.3" nrf52805-pac = { version = "0.1.0", optional = true, features = [ "rt" ]} nrf52810-pac = { version = "0.9.0", optional = true, features = [ "rt" ]} diff --git a/embassy-nrf/src/chips/nrf52805.rs b/embassy-nrf/src/chips/nrf52805.rs index f3ceb98e4..2b02c1afe 100644 --- a/embassy-nrf/src/chips/nrf52805.rs +++ b/embassy-nrf/src/chips/nrf52805.rs @@ -8,6 +8,9 @@ embassy_extras::peripherals! { RTC0, RTC1, + // RNG + RNG, + // UARTE UARTE0, diff --git a/embassy-nrf/src/chips/nrf52810.rs b/embassy-nrf/src/chips/nrf52810.rs index dae260545..4c93d5046 100644 --- a/embassy-nrf/src/chips/nrf52810.rs +++ b/embassy-nrf/src/chips/nrf52810.rs @@ -8,6 +8,9 @@ embassy_extras::peripherals! { RTC0, RTC1, + // RNG + RNG, + // UARTE UARTE0, diff --git a/embassy-nrf/src/chips/nrf52811.rs b/embassy-nrf/src/chips/nrf52811.rs index 6f9edff35..f840214fa 100644 --- a/embassy-nrf/src/chips/nrf52811.rs +++ b/embassy-nrf/src/chips/nrf52811.rs @@ -8,6 +8,9 @@ embassy_extras::peripherals! { RTC0, RTC1, + // RNG + RNG, + // UARTE UARTE0, diff --git a/embassy-nrf/src/chips/nrf52820.rs b/embassy-nrf/src/chips/nrf52820.rs index 8bc50b6d3..180861f71 100644 --- a/embassy-nrf/src/chips/nrf52820.rs +++ b/embassy-nrf/src/chips/nrf52820.rs @@ -8,6 +8,9 @@ embassy_extras::peripherals! { RTC0, RTC1, + // RNG + RNG, + // UARTE UARTE0, diff --git a/embassy-nrf/src/chips/nrf52832.rs b/embassy-nrf/src/chips/nrf52832.rs index 6f3f7fc7b..1c38a7751 100644 --- a/embassy-nrf/src/chips/nrf52832.rs +++ b/embassy-nrf/src/chips/nrf52832.rs @@ -9,6 +9,9 @@ embassy_extras::peripherals! { RTC1, RTC2, + // RNG + RNG, + // UARTE UARTE0, diff --git a/embassy-nrf/src/chips/nrf52833.rs b/embassy-nrf/src/chips/nrf52833.rs index a0240b196..bcb0fffc0 100644 --- a/embassy-nrf/src/chips/nrf52833.rs +++ b/embassy-nrf/src/chips/nrf52833.rs @@ -9,6 +9,9 @@ embassy_extras::peripherals! { RTC1, RTC2, + // RNG + RNG, + // UARTE UARTE0, UARTE1, diff --git a/embassy-nrf/src/chips/nrf52840.rs b/embassy-nrf/src/chips/nrf52840.rs index d4dcfd063..ee8b5a89c 100644 --- a/embassy-nrf/src/chips/nrf52840.rs +++ b/embassy-nrf/src/chips/nrf52840.rs @@ -9,6 +9,9 @@ embassy_extras::peripherals! { RTC1, RTC2, + // RNG + RNG, + // QSPI QSPI, diff --git a/embassy-nrf/src/lib.rs b/embassy-nrf/src/lib.rs index f39521594..c2e461cf1 100644 --- a/embassy-nrf/src/lib.rs +++ b/embassy-nrf/src/lib.rs @@ -33,6 +33,7 @@ pub mod ppi; pub mod pwm; #[cfg(feature = "nrf52840")] pub mod qspi; +pub mod rng; pub mod rtc; #[cfg(not(feature = "nrf52820"))] pub mod saadc; diff --git a/embassy-nrf/src/rng.rs b/embassy-nrf/src/rng.rs new file mode 100644 index 000000000..40778c64c --- /dev/null +++ b/embassy-nrf/src/rng.rs @@ -0,0 +1,154 @@ +use core::convert::Infallible; +use core::marker::PhantomData; + +use embassy::interrupt::InterruptExt; +use embassy::traits; +use embassy::util::OnDrop; +use embassy::util::Signal; +use embassy::util::Unborrow; +use embassy_extras::unborrow; +use futures::Future; +use rand_core::RngCore; + +use crate::interrupt; +use crate::pac; +use crate::peripherals::RNG; + +impl RNG { + fn regs() -> &'static pac::rng::RegisterBlock { + unsafe { &*pac::RNG::ptr() } + } +} + +static NEXT_BYTE: Signal = Signal::new(); + +/// A wrapper around an nRF RNG peripheral. +/// +/// It has a non-blocking API, through `embassy::traits::Rng`, and a blocking api through `rand`. +pub struct Rng<'d> { + irq: interrupt::RNG, + phantom: PhantomData<&'d mut RNG>, +} + +impl<'d> Rng<'d> { + pub fn new( + _rng: impl Unborrow + 'd, + irq: impl Unborrow + 'd, + ) -> Self { + unborrow!(irq); + + let this = Self { + irq, + phantom: PhantomData, + }; + + this.stop(); + this.disable_irq(); + + this.irq.set_handler(Self::on_interrupt); + this.irq.unpend(); + this.irq.enable(); + + this + } + + fn on_interrupt(_: *mut ()) { + NEXT_BYTE.signal(RNG::regs().value.read().value().bits()); + RNG::regs().events_valrdy.reset(); + } + + fn stop(&self) { + RNG::regs().tasks_stop.write(|w| unsafe { w.bits(1) }) + } + + fn start(&self) { + RNG::regs().tasks_start.write(|w| unsafe { w.bits(1) }) + } + + fn enable_irq(&self) { + RNG::regs().intenset.write(|w| w.valrdy().set()); + } + + fn disable_irq(&self) { + RNG::regs().intenclr.write(|w| w.valrdy().clear()); + } + + /// Enable or disable the RNG's bias correction. + /// + /// Bias correction removes any bias towards a '1' or a '0' in the bits generated. + /// However, this makes the generation of numbers slower. + /// + /// Defaults to disabled. + pub fn bias_correction(&self, enable: bool) { + RNG::regs().config.write(|w| w.dercen().bit(enable)) + } +} + +impl<'d> Drop for Rng<'d> { + fn drop(&mut self) { + self.irq.disable() + } +} + +impl<'d> traits::rng::Rng for Rng<'d> { + type Error = Infallible; + + #[rustfmt::skip] // For some reason rustfmt removes the where clause + type RngFuture<'a> where 'd: 'a = impl Future> + 'a; + + fn fill_bytes<'a>(&'a mut self, dest: &'a mut [u8]) -> Self::RngFuture<'a> { + self.enable_irq(); + self.start(); + + async move { + let on_drop = OnDrop::new(|| { + self.stop(); + self.disable_irq(); + }); + + for byte in dest.iter_mut() { + *byte = NEXT_BYTE.wait().await; + } + + // Trigger the teardown + drop(on_drop); + + Ok(()) + } + } +} + +impl<'d> RngCore for Rng<'d> { + fn fill_bytes(&mut self, dest: &mut [u8]) { + self.start(); + + for byte in dest.iter_mut() { + let regs = RNG::regs(); + while regs.events_valrdy.read().bits() == 0 {} + regs.events_valrdy.reset(); + *byte = regs.value.read().value().bits(); + } + + self.stop(); + } + + fn next_u32(&mut self) -> u32 { + let mut bytes = [0; 4]; + self.fill_bytes(&mut bytes); + // We don't care about the endianness, so just use the native one. + u32::from_ne_bytes(bytes) + } + + fn next_u64(&mut self) -> u64 { + let mut bytes = [0; 8]; + self.fill_bytes(&mut bytes); + u64::from_ne_bytes(bytes) + } + + fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), rand_core::Error> { + self.fill_bytes(dest); + Ok(()) + } +} + +// TODO: Should `Rng` implement `CryptoRng`? It's 'suitable for cryptographic purposes' according to the specification. diff --git a/examples/nrf/Cargo.toml b/examples/nrf/Cargo.toml index fa1dab360..8db0ad53f 100644 --- a/examples/nrf/Cargo.toml +++ b/examples/nrf/Cargo.toml @@ -29,3 +29,4 @@ cortex-m-rt = "0.6.13" embedded-hal = { version = "0.2.4" } panic-probe = { version = "0.2.0", features = ["print-defmt"] } futures = { version = "0.3.8", default-features = false, features = ["async-await"] } +rand = { version = "0.8.4", default-features = false } diff --git a/examples/nrf/src/bin/rng.rs b/examples/nrf/src/bin/rng.rs new file mode 100644 index 000000000..a4c204c2e --- /dev/null +++ b/examples/nrf/src/bin/rng.rs @@ -0,0 +1,30 @@ +#![no_std] +#![no_main] +#![feature(min_type_alias_impl_trait)] +#![feature(impl_trait_in_bindings)] +#![feature(type_alias_impl_trait)] +#![allow(incomplete_features)] + +#[path = "../example_common.rs"] +mod example_common; + +use defmt::panic; +use embassy::executor::Spawner; +use embassy_nrf::Peripherals; +use embassy_nrf::rng::Rng; +use embassy_nrf::interrupt; +use embassy::traits::rng::Rng as _; +use rand::Rng as _; + +#[embassy::main] +async fn main(_spawner: Spawner, p: Peripherals) { + let mut rng = Rng::new(p.RNG, interrupt::take!(RNG)); + + // Async API + let mut bytes = [0; 4]; + rng.fill_bytes(&mut bytes).await.unwrap(); // nRF RNG is infallible + defmt::info!("Some random bytes: {:?}", bytes); + + // Sync API with `rand` + defmt::info!("A random number from 1 to 10: {:?}", rng.gen_range(1..=10)); +} From ae0219de6f32222be5ef18be86376971a92f1751 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Wed, 30 Jun 2021 09:45:49 +1000 Subject: [PATCH 2/6] Move initialisation inside of future --- embassy-nrf/src/rng.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/embassy-nrf/src/rng.rs b/embassy-nrf/src/rng.rs index 40778c64c..e5ec02c67 100644 --- a/embassy-nrf/src/rng.rs +++ b/embassy-nrf/src/rng.rs @@ -97,10 +97,10 @@ impl<'d> traits::rng::Rng for Rng<'d> { type RngFuture<'a> where 'd: 'a = impl Future> + 'a; fn fill_bytes<'a>(&'a mut self, dest: &'a mut [u8]) -> Self::RngFuture<'a> { - self.enable_irq(); - self.start(); - async move { + self.enable_irq(); + self.start(); + let on_drop = OnDrop::new(|| { self.stop(); self.disable_irq(); From 89fdad3a6b8d2c1a702be91a15eaea7eeab5f6db Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Wed, 30 Jun 2021 12:34:57 +1000 Subject: [PATCH 3/6] Don't wake the future for every byte in `fill_bytes` --- embassy-nrf/src/rng.rs | 92 +++++++++++++++++++++++++++++++++++------- 1 file changed, 78 insertions(+), 14 deletions(-) diff --git a/embassy-nrf/src/rng.rs b/embassy-nrf/src/rng.rs index e5ec02c67..0ec87effa 100644 --- a/embassy-nrf/src/rng.rs +++ b/embassy-nrf/src/rng.rs @@ -1,13 +1,18 @@ +use core::cell::RefCell; use core::convert::Infallible; +use core::future::Future; use core::marker::PhantomData; +use core::ptr::NonNull; +use core::task::Poll; +use core::task::Waker; use embassy::interrupt::InterruptExt; use embassy::traits; +use embassy::util::CriticalSectionMutex; use embassy::util::OnDrop; -use embassy::util::Signal; use embassy::util::Unborrow; use embassy_extras::unborrow; -use futures::Future; +use futures::future::poll_fn; use rand_core::RngCore; use crate::interrupt; @@ -20,18 +25,41 @@ impl RNG { } } -static NEXT_BYTE: Signal = Signal::new(); +static STATE: CriticalSectionMutex> = + CriticalSectionMutex::new(RefCell::new(State { + buffer: None, + waker: None, + index: 0, + })); + +struct State { + buffer: Option>, + waker: Option, + index: usize, +} + +// SAFETY: `NonNull` is `!Send` because of the possibility of it being aliased. +// However, `buffer` is only used within `on_interrupt`, +// and the original `&mut` passed to `fill_bytes` cannot be used because the safety contract of `Rng::new` +// means that it must still be borrowed by `RngFuture`, and so `rustc` will not let it be accessed. +unsafe impl Send for State {} /// A wrapper around an nRF RNG peripheral. /// /// It has a non-blocking API, through `embassy::traits::Rng`, and a blocking api through `rand`. pub struct Rng<'d> { irq: interrupt::RNG, - phantom: PhantomData<&'d mut RNG>, + phantom: PhantomData<(&'d mut RNG, &'d mut interrupt::RNG)>, } impl<'d> Rng<'d> { - pub fn new( + /// Creates a new RNG driver from the `RNG` peripheral and interrupt. + /// + /// SAFETY: The future returned from `fill_bytes` must not have its lifetime end without running its destructor, + /// e.g. using `mem::forget`. + /// + /// The synchronous API is safe. + pub unsafe fn new( _rng: impl Unborrow + 'd, irq: impl Unborrow + 'd, ) -> Self { @@ -42,7 +70,7 @@ impl<'d> Rng<'d> { phantom: PhantomData, }; - this.stop(); + Self::stop(); this.disable_irq(); this.irq.set_handler(Self::on_interrupt); @@ -53,11 +81,25 @@ impl<'d> Rng<'d> { } fn on_interrupt(_: *mut ()) { - NEXT_BYTE.signal(RNG::regs().value.read().value().bits()); - RNG::regs().events_valrdy.reset(); + critical_section::with(|cs| { + let mut state = STATE.borrow(cs).borrow_mut(); + // SAFETY: the safety requirements on `Rng::new` make sure that the original `&mut`'s lifetime is still valid, + // meaning it can't be aliased and is a valid pointer. + let buffer = unsafe { state.buffer.unwrap().as_mut() }; + buffer[state.index] = RNG::regs().value.read().value().bits(); + state.index += 1; + if state.index == buffer.len() { + // Stop the RNG within the interrupt so that it doesn't get triggered again on the way to waking the future. + Self::stop(); + if let Some(waker) = state.waker.take() { + waker.wake(); + } + } + RNG::regs().events_valrdy.reset(); + }); } - fn stop(&self) { + fn stop() { RNG::regs().tasks_stop.write(|w| unsafe { w.bits(1) }) } @@ -98,17 +140,39 @@ impl<'d> traits::rng::Rng for Rng<'d> { fn fill_bytes<'a>(&'a mut self, dest: &'a mut [u8]) -> Self::RngFuture<'a> { async move { + critical_section::with(|cs| { + let mut state = STATE.borrow(cs).borrow_mut(); + state.buffer = Some(dest.into()); + }); + self.enable_irq(); self.start(); let on_drop = OnDrop::new(|| { - self.stop(); + Self::stop(); self.disable_irq(); }); - for byte in dest.iter_mut() { - *byte = NEXT_BYTE.wait().await; - } + poll_fn(|cx| { + critical_section::with(|cs| { + let mut state = STATE.borrow(cs).borrow_mut(); + state.waker = Some(cx.waker().clone()); + // SAFETY: see safety message in interrupt handler. + // Also, both here and in the interrupt handler, we're in a critical section, + // so they can't interfere with each other. + let buffer = unsafe { state.buffer.unwrap().as_ref() }; + + if state.index == buffer.len() { + // Reset the state for next time + state.buffer = None; + state.index = 0; + Poll::Ready(()) + } else { + Poll::Pending + } + }) + }) + .await; // Trigger the teardown drop(on_drop); @@ -129,7 +193,7 @@ impl<'d> RngCore for Rng<'d> { *byte = regs.value.read().value().bits(); } - self.stop(); + Self::stop(); } fn next_u32(&mut self) -> u32 { From a64dec517c1bbd28aa8f98f5d978c97fe0037a8f Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Wed, 30 Jun 2021 12:55:30 +1000 Subject: [PATCH 4/6] Update RNG example --- examples/nrf/src/bin/rng.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/examples/nrf/src/bin/rng.rs b/examples/nrf/src/bin/rng.rs index a4c204c2e..6aa43d0db 100644 --- a/examples/nrf/src/bin/rng.rs +++ b/examples/nrf/src/bin/rng.rs @@ -10,15 +10,15 @@ mod example_common; use defmt::panic; use embassy::executor::Spawner; -use embassy_nrf::Peripherals; -use embassy_nrf::rng::Rng; -use embassy_nrf::interrupt; use embassy::traits::rng::Rng as _; +use embassy_nrf::interrupt; +use embassy_nrf::rng::Rng; +use embassy_nrf::Peripherals; use rand::Rng as _; #[embassy::main] async fn main(_spawner: Spawner, p: Peripherals) { - let mut rng = Rng::new(p.RNG, interrupt::take!(RNG)); + let mut rng = unsafe { Rng::new(p.RNG, interrupt::take!(RNG)) }; // Async API let mut bytes = [0; 4]; @@ -27,4 +27,17 @@ async fn main(_spawner: Spawner, p: Peripherals) { // Sync API with `rand` defmt::info!("A random number from 1 to 10: {:?}", rng.gen_range(1..=10)); + + let mut bytes = [0; 1024]; + rng.fill_bytes(&mut bytes).await.unwrap(); + let zero_count: u32 = bytes.iter().fold(0, |acc, val| acc + val.count_zeros()); + let one_count: u32 = bytes.iter().fold(0, |acc, val| acc + val.count_ones()); + defmt::info!( + "Chance of zero: {}%", + zero_count * 100 / (bytes.len() as u32 * 8) + ); + defmt::info!( + "Chance of one: {}%", + one_count * 100 / (bytes.len() as u32 * 8) + ); } From 53b95588df2151c6805d0b543f6c7a6b0285dfda Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Wed, 30 Jun 2021 15:55:52 +1000 Subject: [PATCH 5/6] Use atomics to share state instead of a `RefCell` --- embassy-nrf/src/rng.rs | 138 ++++++++++++++++++++++++----------------- 1 file changed, 82 insertions(+), 56 deletions(-) diff --git a/embassy-nrf/src/rng.rs b/embassy-nrf/src/rng.rs index 0ec87effa..80282c4b0 100644 --- a/embassy-nrf/src/rng.rs +++ b/embassy-nrf/src/rng.rs @@ -1,14 +1,14 @@ -use core::cell::RefCell; use core::convert::Infallible; use core::future::Future; use core::marker::PhantomData; -use core::ptr::NonNull; +use core::ptr; +use core::sync::atomic::AtomicPtr; +use core::sync::atomic::Ordering; use core::task::Poll; -use core::task::Waker; use embassy::interrupt::InterruptExt; use embassy::traits; -use embassy::util::CriticalSectionMutex; +use embassy::util::AtomicWaker; use embassy::util::OnDrop; use embassy::util::Unborrow; use embassy_extras::unborrow; @@ -25,25 +25,18 @@ impl RNG { } } -static STATE: CriticalSectionMutex> = - CriticalSectionMutex::new(RefCell::new(State { - buffer: None, - waker: None, - index: 0, - })); +static STATE: State = State { + ptr: AtomicPtr::new(ptr::null_mut()), + end: AtomicPtr::new(ptr::null_mut()), + waker: AtomicWaker::new(), +}; struct State { - buffer: Option>, - waker: Option, - index: usize, + ptr: AtomicPtr, + end: AtomicPtr, + waker: AtomicWaker, } -// SAFETY: `NonNull` is `!Send` because of the possibility of it being aliased. -// However, `buffer` is only used within `on_interrupt`, -// and the original `&mut` passed to `fill_bytes` cannot be used because the safety contract of `Rng::new` -// means that it must still be borrowed by `RngFuture`, and so `rustc` will not let it be accessed. -unsafe impl Send for State {} - /// A wrapper around an nRF RNG peripheral. /// /// It has a non-blocking API, through `embassy::traits::Rng`, and a blocking api through `rand`. @@ -70,7 +63,7 @@ impl<'d> Rng<'d> { phantom: PhantomData, }; - Self::stop(); + this.stop(); this.disable_irq(); this.irq.set_handler(Self::on_interrupt); @@ -81,25 +74,54 @@ impl<'d> Rng<'d> { } fn on_interrupt(_: *mut ()) { - critical_section::with(|cs| { - let mut state = STATE.borrow(cs).borrow_mut(); - // SAFETY: the safety requirements on `Rng::new` make sure that the original `&mut`'s lifetime is still valid, - // meaning it can't be aliased and is a valid pointer. - let buffer = unsafe { state.buffer.unwrap().as_mut() }; - buffer[state.index] = RNG::regs().value.read().value().bits(); - state.index += 1; - if state.index == buffer.len() { - // Stop the RNG within the interrupt so that it doesn't get triggered again on the way to waking the future. - Self::stop(); - if let Some(waker) = state.waker.take() { - waker.wake(); + // Clear the event. + RNG::regs().events_valrdy.reset(); + + // Mutate the slice within a critical section, + // so that the future isn't dropped in between us loading the pointer and actually dereferencing it. + let (ptr, end) = critical_section::with(|_| { + let ptr = STATE.ptr.load(Ordering::Relaxed); + // We need to make sure we haven't already filled the whole slice, + // in case the interrupt fired again before the executor got back to the future. + let end = STATE.end.load(Ordering::Relaxed); + if !ptr.is_null() && ptr != end { + // If the future was dropped, the pointer would have been set to null, + // so we're still good to mutate the slice. + // The safety contract of `Rng::new` means that the future can't have been dropped + // without calling its destructor. + unsafe { + *ptr = RNG::regs().value.read().value().bits(); } } - RNG::regs().events_valrdy.reset(); + (ptr, end) }); + + if ptr.is_null() || ptr == end { + // If the future was dropped, there's nothing to do. + // If `ptr == end`, we were called by mistake, so return. + return; + } + + let new_ptr = unsafe { ptr.add(1) }; + match STATE + .ptr + .compare_exchange(ptr, new_ptr, Ordering::Relaxed, Ordering::Relaxed) + { + Ok(ptr) => { + let end = STATE.end.load(Ordering::Relaxed); + // It doesn't matter if `end` was changed under our feet, because then this will just be false. + if ptr == end { + STATE.waker.wake(); + } + } + Err(_) => { + // If the future was dropped or finished, there's no point trying to wake it. + // It will have already stopped the RNG, so there's no need to do that either. + } + } } - fn stop() { + fn stop(&self) { RNG::regs().tasks_stop.write(|w| unsafe { w.bits(1) }) } @@ -140,37 +162,41 @@ impl<'d> traits::rng::Rng for Rng<'d> { fn fill_bytes<'a>(&'a mut self, dest: &'a mut [u8]) -> Self::RngFuture<'a> { async move { - critical_section::with(|cs| { - let mut state = STATE.borrow(cs).borrow_mut(); - state.buffer = Some(dest.into()); - }); + if dest.len() == 0 { + return Ok(()); // Nothing to fill + } + + let range = dest.as_mut_ptr_range(); + // Even if we've preempted the interrupt, it can't preempt us again, + // so we don't need to worry about the order we write these in. + STATE.ptr.store(range.start, Ordering::Relaxed); + STATE.end.store(range.end, Ordering::Relaxed); self.enable_irq(); self.start(); let on_drop = OnDrop::new(|| { - Self::stop(); + self.stop(); self.disable_irq(); + + // The interrupt is now disabled and can't preempt us anymore, so the order doesn't matter here. + STATE.ptr.store(ptr::null_mut(), Ordering::Relaxed); + STATE.end.store(ptr::null_mut(), Ordering::Relaxed); }); poll_fn(|cx| { - critical_section::with(|cs| { - let mut state = STATE.borrow(cs).borrow_mut(); - state.waker = Some(cx.waker().clone()); - // SAFETY: see safety message in interrupt handler. - // Also, both here and in the interrupt handler, we're in a critical section, - // so they can't interfere with each other. - let buffer = unsafe { state.buffer.unwrap().as_ref() }; + STATE.waker.register(cx.waker()); - if state.index == buffer.len() { - // Reset the state for next time - state.buffer = None; - state.index = 0; - Poll::Ready(()) - } else { - Poll::Pending - } - }) + // The interrupt will never modify `end`, so load it first and then get the most up-to-date `ptr`. + let end = STATE.end.load(Ordering::Relaxed); + let ptr = STATE.ptr.load(Ordering::Relaxed); + + if ptr == end { + // We're done. + Poll::Ready(()) + } else { + Poll::Pending + } }) .await; @@ -193,7 +219,7 @@ impl<'d> RngCore for Rng<'d> { *byte = regs.value.read().value().bits(); } - Self::stop(); + self.stop(); } fn next_u32(&mut self) -> u32 { From 99339e940e568c08329bce9e93a36fb79a21685d Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Wed, 30 Jun 2021 16:04:34 +1000 Subject: [PATCH 6/6] fix: check if `new_ptr == end`, not the old pointer --- embassy-nrf/src/rng.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/embassy-nrf/src/rng.rs b/embassy-nrf/src/rng.rs index 80282c4b0..a444c9b3f 100644 --- a/embassy-nrf/src/rng.rs +++ b/embassy-nrf/src/rng.rs @@ -107,10 +107,10 @@ impl<'d> Rng<'d> { .ptr .compare_exchange(ptr, new_ptr, Ordering::Relaxed, Ordering::Relaxed) { - Ok(ptr) => { + Ok(_) => { let end = STATE.end.load(Ordering::Relaxed); // It doesn't matter if `end` was changed under our feet, because then this will just be false. - if ptr == end { + if new_ptr == end { STATE.waker.wake(); } }