zephyr: device: gpio: Fix races in waiting

Fix up the async wait for high and low to avoid various races.  Instead
of reading the gpio to determine if we are done, there is an atomic that
holds the bits indicating the callback has been called.  This does
require the irq to always fire, but in the case where the gpio is
already asserted, the irq will be called between enabling it, and
checking, so it does avoid the task actually blocking.

Unfortunately, the query API in Zephyr only returns a general IRQ status
(some interrupt is ready) instead of per pin, so there is no way to read
this other than to let the callback happen.

Signed-off-by: David Brown <[email protected]>

Author: d3zd3z

zephyr/src/device/gpio.rs

@@ -22,28 +22,29 @@ mod async_io {
 
     use core::{
         cell::UnsafeCell,
-        ffi::c_int,
         future::Future,
         mem,
         sync::atomic::Ordering,
         task::{Poll, Waker},
     };
 
     use embassy_sync::waitqueue::AtomicWaker;
-    use portable_atomic::AtomicBool;
     use zephyr_sys::{
-        device, gpio_add_callback, gpio_callback, gpio_init_callback, gpio_pin_get,
-        gpio_pin_interrupt_configure, gpio_pin_interrupt_configure_dt, gpio_port_pins_t,
-        GPIO_INT_LEVEL_HIGH, GPIO_INT_LEVEL_LOW, ZR_GPIO_INT_MODE_DISABLE_ONLY,
+        device, gpio_add_callback, gpio_callback, gpio_init_callback, gpio_pin_interrupt_configure,
+        gpio_pin_interrupt_configure_dt, gpio_port_pins_t, GPIO_INT_LEVEL_HIGH, GPIO_INT_LEVEL_LOW,
+        ZR_GPIO_INT_MODE_DISABLE_ONLY,
     };
 
-    use crate::printkln;
+    use crate::sync::atomic::{AtomicBool, AtomicU32};
 
     use super::{GpioPin, GpioToken};
 
     pub(crate) struct GpioStatic {
         /// The wakers for each of the gpios.
         wakers: [AtomicWaker; 32],
+        /// Indicates when an interrupt has fired.  Used to definitively indicate the event has
+        /// happened, so we can wake.
+        fired: AtomicU32,
         /// Have we been initialized?
         installed: AtomicBool,
         /// The data for the callback itself.
@@ -56,6 +57,7 @@ mod async_io {
         pub(crate) const fn new() -> Self {
             Self {
                 wakers: [const { AtomicWaker::new() }; 32],
+                fired: AtomicU32::new(0),
                 installed: AtomicBool::new(false),
                 // SAFETY: `installed` will tell us this need to be installed.
                 callback: unsafe { mem::zeroed() },
@@ -116,8 +118,6 @@ mod async_io {
                     .cast::<Self>()
             };
 
-            // printkln!("CB called: pins: {pins:#x}");
-
             // For each pin we are informed of.
             while pins > 0 {
                 let pin = pins.trailing_zeros();
@@ -134,13 +134,23 @@ mod async_io {
                     gpio_pin_interrupt_configure(port, pin as u8, ZR_GPIO_INT_MODE_DISABLE_ONLY);
 
                     // Remove the callback bit.  Unclear if this is actually useful.
-                    // (*cb).pin_mask &= !(1 << pin);
+                    (*cb).pin_mask &= !(1 << pin);
+
+                    // Indicate that we have fired.
+                    // AcqRel is sufficient for ordering across a single atomic.
+                    (*data).fired.fetch_or(1 << pin, Ordering::AcqRel);
 
                     // After the interrupt is off, wake the handler.
                     (*data).wakers[pin as usize].wake();
                 }
             }
         }
+
+        /// Check if we have fired for a given pin.  Clears the status.
+        pub(crate) fn has_fired(&self, pin: u8) -> bool {
+            let value = self.fired.fetch_and(!(1 << pin), Ordering::AcqRel);
+            value & (1 << pin) != 0
+        }
     }
 
     impl GpioPin {
@@ -193,6 +203,12 @@ mod async_io {
         ) -> core::task::Poll<Self::Output> {
             self.pin.data.fast_install(self.pin.pin.port);
 
+            // Early detection of the event.  Also clears.
+            // This should be non-racy as long as only one task at a time waits on the gpio.
+            if self.pin.data.has_fired(self.pin.pin.pin) {
+                return Poll::Ready(());
+            }
+
             self.pin.data.register(self.pin.pin.pin, cx.waker());
 
             let mode = match self.level {
@@ -204,10 +220,9 @@ mod async_io {
             unsafe {
                 gpio_pin_interrupt_configure_dt(&self.pin.pin, mode);
 
-                if gpio_pin_get(self.pin.pin.port, self.pin.pin.pin) == self.level as c_int {
-                    // TODO: Need to match with level.
-                    // Zephyr doesn't have a way to determine if a given interrupt is pending, so the
-                    // best we can do is just read the pin.  This doesn't work for edges though.
+                // Before sleeping, check if it fired, to avoid having to pend if it already
+                // happened.
+                if self.pin.data.has_fired(self.pin.pin.pin) {
                     return Poll::Ready(());
                 }
             }