From da917791174510637c69660b10b1e201d22cfe9d Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Fri, 26 Feb 2021 02:04:48 +0100 Subject: [PATCH] interrupt: Split set_handler context. Since introducing the ctx pointer, the handler is now two words, so setting it can race with the interrupt firing. On race it's possible for the new handler to be alled with the old ctx pointer or viceversa. Rather than documenting this, it's better to split the function in two to make it obvious to the user that it's not atomic. The user can use a critical section, or disable/enable the interrupt to avoid races if this is a concern. --- embassy-nrf/src/gpiote.rs | 2 +- embassy-nrf/src/qspi.rs | 2 +- embassy-nrf/src/rtc.rs | 12 +++++------- embassy-nrf/src/uarte.rs | 2 +- embassy-nrf/src/util/peripheral.rs | 18 ++++++++---------- embassy-stm32f4/src/rtc.rs | 12 +++++------- embassy-stm32f4/src/serial.rs | 6 +++--- embassy/src/executor/mod.rs | 12 +++++------- embassy/src/interrupt.rs | 8 ++++++-- embassy/src/util/signal.rs | 9 ++++++--- 10 files changed, 41 insertions(+), 42 deletions(-) diff --git a/embassy-nrf/src/gpiote.rs b/embassy-nrf/src/gpiote.rs index 592ca82a..01e61bd5 100644 --- a/embassy-nrf/src/gpiote.rs +++ b/embassy-nrf/src/gpiote.rs @@ -118,7 +118,7 @@ impl Gpiote { // Enable interrupts gpiote.events_port.write(|w| w); gpiote.intenset.write(|w| w.port().set()); - irq.set_handler(Self::on_irq, core::ptr::null_mut()); + irq.set_handler(Self::on_irq); irq.unpend(); irq.enable(); diff --git a/embassy-nrf/src/qspi.rs b/embassy-nrf/src/qspi.rs index 902d6511..300b32ad 100644 --- a/embassy-nrf/src/qspi.rs +++ b/embassy-nrf/src/qspi.rs @@ -146,7 +146,7 @@ impl Qspi { SIGNAL.reset(); qspi.intenset.write(|w| w.ready().set()); - irq.set_handler(irq_handler, core::ptr::null_mut()); + irq.set_handler(irq_handler); irq.unpend(); irq.enable(); diff --git a/embassy-nrf/src/rtc.rs b/embassy-nrf/src/rtc.rs index 867c710c..7a79580c 100644 --- a/embassy-nrf/src/rtc.rs +++ b/embassy-nrf/src/rtc.rs @@ -109,13 +109,11 @@ impl RTC { // Wait for clear while self.rtc.counter.read().bits() != 0 {} - self.irq.set_handler( - |ptr| unsafe { - let this = &*(ptr as *const () as *const Self); - this.on_interrupt(); - }, - self as *const _ as *mut _, - ); + self.irq.set_handler(|ptr| unsafe { + let this = &*(ptr as *const () as *const Self); + this.on_interrupt(); + }); + self.irq.set_handler_context(self as *const _ as *mut _); self.irq.unpend(); self.irq.enable(); } diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs index f029276b..07718a1d 100644 --- a/embassy-nrf/src/uarte.rs +++ b/embassy-nrf/src/uarte.rs @@ -119,7 +119,7 @@ where .write(|w| w.endtx().set().txstopped().set().endrx().set().rxto().set()); // Register ISR - irq.set_handler(Self::on_irq, core::ptr::null_mut()); + irq.set_handler(Self::on_irq); irq.unpend(); irq.enable(); diff --git a/embassy-nrf/src/util/peripheral.rs b/embassy-nrf/src/util/peripheral.rs index 8cefaeef..bf78d776 100644 --- a/embassy-nrf/src/util/peripheral.rs +++ b/embassy-nrf/src/util/peripheral.rs @@ -33,16 +33,14 @@ impl PeripheralMutex { irq.disable(); compiler_fence(Ordering::SeqCst); - irq.set_handler( - |p| { - // Safety: it's OK to get a &mut to the state, since - // - We're in the IRQ, no one else can't preempt us - // - We can't have preempted a with() call because the irq is disabled during it. - let state = unsafe { &mut *(p as *mut S) }; - state.on_interrupt(); - }, - state.get() as *mut (), - ); + irq.set_handler(|p| { + // Safety: it's OK to get a &mut to the state, since + // - We're in the IRQ, no one else can't preempt us + // - We can't have preempted a with() call because the irq is disabled during it. + let state = unsafe { &mut *(p as *mut S) }; + state.on_interrupt(); + }); + irq.set_handler_context(state.get() as *mut ()); // Safety: it's OK to get a &mut to the state, since the irq is disabled. let state = unsafe { &mut *state.get() }; diff --git a/embassy-stm32f4/src/rtc.rs b/embassy-stm32f4/src/rtc.rs index da770287..21589f5d 100644 --- a/embassy-stm32f4/src/rtc.rs +++ b/embassy-stm32f4/src/rtc.rs @@ -92,13 +92,11 @@ impl RTC { self.rtc.set_compare(0, 0x8000); self.rtc.set_compare_interrupt(0, true); - self.irq.set_handler( - |ptr| unsafe { - let this = &*(ptr as *const () as *const Self); - this.on_interrupt(); - }, - self as *const _ as *mut _, - ); + self.irq.set_handler(|ptr| unsafe { + let this = &*(ptr as *const () as *const Self); + this.on_interrupt(); + }); + self.irq.set_handler_context(self as *const _ as *mut _); self.irq.unpend(); self.irq.enable(); diff --git a/embassy-stm32f4/src/serial.rs b/embassy-stm32f4/src/serial.rs index d84d8cb0..669d7856 100644 --- a/embassy-stm32f4/src/serial.rs +++ b/embassy-stm32f4/src/serial.rs @@ -70,9 +70,9 @@ impl Serial, Stream2> { let (usart, _) = serial.release(); // Register ISR - tx_int.set_handler(Self::on_tx_irq, core::ptr::null_mut()); - rx_int.set_handler(Self::on_rx_irq, core::ptr::null_mut()); - usart_int.set_handler(Self::on_rx_irq, core::ptr::null_mut()); + tx_int.set_handler(Self::on_tx_irq); + rx_int.set_handler(Self::on_rx_irq); + usart_int.set_handler(Self::on_rx_irq); // usart_int.unpend(); // usart_int.enable(); diff --git a/embassy/src/executor/mod.rs b/embassy/src/executor/mod.rs index 6c42764b..be0d1b9e 100644 --- a/embassy/src/executor/mod.rs +++ b/embassy/src/executor/mod.rs @@ -244,13 +244,11 @@ impl IrqExecutor { init(unsafe { self.inner.spawner() }); - self.irq.set_handler( - |ctx| unsafe { - let executor = &*(ctx as *const raw::Executor); - executor.run_queued(); - }, - &self.inner as *const _ as _, - ); + self.irq.set_handler(|ctx| unsafe { + let executor = &*(ctx as *const raw::Executor); + executor.run_queued(); + }); + self.irq.set_handler_context(&self.inner as *const _ as _); self.irq.enable(); } } diff --git a/embassy/src/interrupt.rs b/embassy/src/interrupt.rs index 9106def5..53413d41 100644 --- a/embassy/src/interrupt.rs +++ b/embassy/src/interrupt.rs @@ -38,10 +38,9 @@ pub unsafe trait Interrupt { #[doc(hidden)] unsafe fn __handler(&self) -> &'static Handler; - fn set_handler(&self, func: unsafe fn(*mut ()), ctx: *mut ()) { + fn set_handler(&self, func: unsafe fn(*mut ())) { let handler = unsafe { self.__handler() }; handler.func.store(func as *mut (), Ordering::Release); - handler.ctx.store(ctx, Ordering::Release); } fn remove_handler(&self) { @@ -49,6 +48,11 @@ pub unsafe trait Interrupt { handler.func.store(ptr::null_mut(), Ordering::Release); } + fn set_handler_context(&self, ctx: *mut ()) { + let handler = unsafe { self.__handler() }; + handler.ctx.store(ctx, Ordering::Release); + } + #[inline] fn enable(&self) { unsafe { diff --git a/embassy/src/util/signal.rs b/embassy/src/util/signal.rs index 9fcfa7ac..0abcc75f 100644 --- a/embassy/src/util/signal.rs +++ b/embassy/src/util/signal.rs @@ -93,7 +93,8 @@ impl<'a, I: Interrupt> Drop for InterruptFuture<'a, I> { impl<'a, I: Interrupt> InterruptFuture<'a, I> { pub fn new(interrupt: &'a mut I) -> Self { interrupt.disable(); - interrupt.set_handler(Self::interrupt_handler, ptr::null_mut()); + interrupt.set_handler(Self::interrupt_handler); + interrupt.set_handler_context(ptr::null_mut()); interrupt.unpend(); interrupt.enable(); @@ -121,8 +122,10 @@ impl<'a, I: Interrupt> Future for InterruptFuture<'a, I> { fn poll(self: core::pin::Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<()> { let s = unsafe { self.get_unchecked_mut() }; - let ctx = unsafe { executor::raw::task_from_waker(&cx.waker()).cast().as_ptr() }; - s.interrupt.set_handler(Self::interrupt_handler, ctx); + s.interrupt.set_handler(Self::interrupt_handler); + s.interrupt.set_handler_context(unsafe { + executor::raw::task_from_waker(&cx.waker()).cast().as_ptr() + }); if s.interrupt.is_enabled() { Poll::Pending } else {