stm32xx-i2c: remove I2cControl function table (#2121)

It turns out that, in practice, we only ever provide a single function
for each of `wfi` and `enable` here. This makes the table itself an
unnecessary level of indirection, which has some deleterious effects:

- It opens the possibility for _different_ I2cControl structs containing
_different_ function pointers to be passed to each call, complicating
analysis. (Searching for this phenomenon was what led me to notice that
we only ever use one implementation.)

- It makes it a lot harder to recognize use of syscalls (or misuse of
syscalls) in the I2C layer.

- It routes important parts of the I2C stack through indirect function
calls, defeating stack analysis.

- It adds a bunch of code that isn't really necessary, as it turns out.

This commit removes the dispatch table, inlining the existing functions
into their callsites (and introducing a utility function for the wfi
behavior).

---------

Co-authored-by: Matt Keeter <[email protected]>

Author: cbiffle

drv/stm32xx-i2c-server/src/main.rs

@@ -95,7 +95,6 @@ fn configure_mux(
     port: PortIndex,
     mux: Option<(Mux, Segment)>,
     muxes: &[I2cMux<'_>],
-    ctrl: &I2cControl,
 ) -> Result<(), ResponseCode> {
     let bus = (controller.controller, port);
 
@@ -127,7 +126,7 @@ fn configure_mux(
                 // (errant) address conflict can be pretty brutal.
                 //
                 find_mux(controller, port, muxes, current_id, |mux| {
-                    mux.driver.enable_segment(mux, controller, None, ctrl)
+                    mux.driver.enable_segment(mux, controller, None)
                 })
                 .map_err(|err| {
                     //
@@ -157,7 +156,7 @@ fn configure_mux(
             // mux state as unknown.
             //
             all_muxes(controller, port, muxes, |mux| {
-                match mux.driver.enable_segment(mux, controller, None, ctrl) {
+                match mux.driver.enable_segment(mux, controller, None) {
                     Err(ResponseCode::MuxMissing) => {
                         //
                         // The mux is gone entirely.  We really don't expect
@@ -201,7 +200,7 @@ fn configure_mux(
     if let Some((id, segment)) = mux {
         find_mux(controller, port, muxes, id, |mux| {
             mux.driver
-                .enable_segment(mux, controller, Some(segment), ctrl)
+                .enable_segment(mux, controller, Some(segment))
                 .map_err(|err| {
                     //
                     // We have failed to enable our new mux+segment.
@@ -378,40 +377,7 @@ fn main() -> ! {
     // Field messages.
     let mut buffer = [0; 4];
 
-    let ctrl = I2cControl {
-        enable: |notification| {
-            sys_irq_control(notification, true);
-        },
-        wfi: |notification, timeout| {
-            const TIMER_NOTIFICATION: u32 = 1 << 31;
-
-            // If the driver passes in a timeout that is large enough that it
-            // would overflow the kernel's 64-bit timestamp space... well, we'll
-            // do the best we can without compiling in an unlikely panic.
-            let dead = sys_get_timer().now.saturating_add(timeout.0);
-
-            sys_set_timer(Some(dead), TIMER_NOTIFICATION);
-
-            let notification =
-                sys_recv_notification(notification | TIMER_NOTIFICATION);
-
-            if notification == TIMER_NOTIFICATION {
-                I2cControlResult::TimedOut
-            } else {
-                sys_set_timer(None, TIMER_NOTIFICATION);
-                I2cControlResult::Interrupted
-            }
-        },
-    };
-
-    configure_muxes(
-        &muxes,
-        &controllers,
-        &pins,
-        &mut portmap,
-        &mut muxmap,
-        &ctrl,
-    );
+    configure_muxes(&muxes, &controllers, &pins, &mut portmap, &mut muxmap);
 
     loop {
         hl::recv_without_notification(&mut buffer, |op, msg| match op {
@@ -438,14 +404,8 @@ fn main() -> ! {
 
                 configure_port(&mut portmap, controller, port, &pins);
 
-                match configure_mux(
-                    &mut muxmap,
-                    controller,
-                    port,
-                    mux,
-                    &muxes,
-                    &ctrl,
-                ) {
+                match configure_mux(&mut muxmap, controller, port, mux, &muxes)
+                {
                     Ok(_) => {}
                     Err(code) => {
                         ringbuf_entry!(Trace::MuxError(code.into()));
@@ -511,7 +471,6 @@ fn main() -> ! {
 
                             rbuf.write_at(pos, byte)
                         },
-                        &ctrl,
                     );
                     match controller_result {
                         Err(code) => {
@@ -775,7 +734,6 @@ fn configure_muxes(
     pins: &[I2cPins],
     map: &mut PortMap,
     muxmap: &mut MuxMap,
-    ctrl: &I2cControl,
 ) {
     let sys = SYS.get_task_id();
     let sys = Sys::from(sys);
@@ -788,7 +746,7 @@ fn configure_muxes(
         let mut reset_attempted = false;
 
         loop {
-            match mux.driver.configure(mux, controller, &sys, ctrl) {
+            match mux.driver.configure(mux, controller, &sys) {
                 Ok(_) => {
                     //
                     // We are going to attempt to disable all segments.  If we
@@ -815,7 +773,7 @@ fn configure_muxes(
                     // deal with the reset).
                     //
                     if let Err(code) =
-                        mux.driver.enable_segment(mux, controller, None, ctrl)
+                        mux.driver.enable_segment(mux, controller, None)
                     {
                         ringbuf_entry!(Trace::SegmentFailed(code.into()));
 

drv/stm32xx-i2c/src/lib.rs

@@ -76,17 +76,6 @@ pub enum I2cControlResult {
     TimedOut,
 }
 
-///
-/// A structure that defines interrupt control flow functions that will be
-/// used to pass control flow into the kernel to either enable or wait for
-/// interrupts.  Note that this is deliberately a struct and not a trait,
-/// allowing the [`I2cMuxDriver`] trait to itself be a trait object.
-///
-pub struct I2cControl {
-    pub enable: fn(u32),
-    pub wfi: fn(u32, I2cTimeout) -> I2cControlResult,
-}
-
 pub struct I2cTargetControl {
     pub enable: fn(u32),
     pub wfi: fn(u32),
@@ -109,7 +98,6 @@ pub trait I2cMuxDriver {
         mux: &I2cMux<'_>,
         controller: &I2cController<'_>,
         sys: &sys_api::Sys,
-        ctrl: &I2cControl,
     ) -> Result<(), drv_i2c_api::ResponseCode>;
 
     /// Reset the mux
@@ -126,7 +114,6 @@ pub trait I2cMuxDriver {
         mux: &I2cMux<'_>,
         controller: &I2cController<'_>,
         segment: Option<drv_i2c_api::Segment>,
-        ctrl: &I2cControl,
     ) -> Result<(), drv_i2c_api::ResponseCode>;
 }
 
@@ -503,15 +490,18 @@ impl I2cController<'_> {
     }
 
     ///
-    /// A common routine to wait for interrupts with a timeout.
+    /// A common routine to wait for interrupts, panicking the driver if the
+    /// interrupt doesn't arrive in an arbitrarily chosen period of time.
     ///
-    fn wfi(&self, ctrl: &I2cControl) -> Result<(), drv_i2c_api::ResponseCode> {
+    fn wfi(&self) -> Result<(), drv_i2c_api::ResponseCode> {
         //
-        // A 100 ms timeout is much, much longer than the I2C timeouts.
+        // A 100 ms timeout is 4x longer than the I2C timeouts, but much shorter
+        // than potential context switches when dumps are being recorded by a
+        // high priority task.
         //
         const TIMEOUT: I2cTimeout = I2cTimeout(100);
 
-        match (ctrl.wfi)(self.notification, TIMEOUT) {
+        match wfi_raw(self.notification, TIMEOUT) {
             I2cControlResult::TimedOut => {
                 //
                 // This really shouldn't happen:  it means that not only did
@@ -607,7 +597,6 @@ impl I2cController<'_> {
         getbyte: impl Fn(usize) -> Option<u8>,
         mut rlen: ReadLength,
         mut putbyte: impl FnMut(usize, u8) -> Option<()>,
-        ctrl: &I2cControl,
     ) -> Result<(), drv_i2c_api::ResponseCode> {
         // Assert our preconditions as described above
         assert!(wlen > 0 || rlen != ReadLength::Fixed(0));
@@ -652,8 +641,8 @@ impl I2cController<'_> {
                         break;
                     }
 
-                    self.wfi(ctrl)?;
-                    (ctrl.enable)(notification);
+                    self.wfi()?;
+                    sys_irq_control(notification, true);
                 }
 
                 // Get a single byte.
@@ -682,8 +671,8 @@ impl I2cController<'_> {
                     break;
                 }
 
-                self.wfi(ctrl)?;
-                (ctrl.enable)(notification);
+                self.wfi()?;
+                sys_irq_control(notification, true);
             }
         }
 
@@ -730,8 +719,8 @@ impl I2cController<'_> {
                 }
 
                 loop {
-                    self.wfi(ctrl)?;
-                    (ctrl.enable)(notification);
+                    self.wfi()?;
+                    sys_irq_control(notification, true);
 
                     let isr = i2c.isr.read();
                     ringbuf_entry!(Trace::Read(Register::ISR, isr.bits()));
@@ -785,8 +774,8 @@ impl I2cController<'_> {
 
                 self.check_errors(&isr)?;
 
-                self.wfi(ctrl)?;
-                (ctrl.enable)(notification);
+                self.wfi()?;
+                sys_irq_control(notification, true);
             }
         }
 
@@ -820,7 +809,6 @@ impl I2cController<'_> {
         &self,
         addr: u8,
         ops: &[I2cKonamiCode],
-        ctrl: &I2cControl,
     ) -> Result<(), drv_i2c_api::ResponseCode> {
         let i2c = self.registers;
         let notification = self.notification;
@@ -864,8 +852,8 @@ impl I2cController<'_> {
                     break;
                 }
 
-                self.wfi(ctrl)?;
-                (ctrl.enable)(notification);
+                self.wfi()?;
+                sys_irq_control(notification, true);
             }
         }
 
@@ -1191,3 +1179,35 @@ impl I2cController<'_> {
         }
     }
 }
+
+/// Waits for `event_mask` or `timeout`, whichever comes first. This is
+/// factored out of `wfi` above for clarity.
+///
+/// This function, like `userlib::hl`, assumes that notification bit 31 is safe
+/// to use for the timer. If `event_mask` also has bit 31 set, weird things
+/// will happen, don't do that.
+fn wfi_raw(event_mask: u32, timeout: I2cTimeout) -> I2cControlResult {
+    const TIMER_NOTIFICATION: u32 = 1 << 31;
+
+    // If the driver passes in a timeout that is large enough that it would
+    // overflow the kernel's 64-bit timestamp space... well, we'll do the
+    // best we can without compiling in an unlikely panic.
+    let dead = sys_get_timer().now.saturating_add(timeout.0);
+
+    sys_set_timer(Some(dead), TIMER_NOTIFICATION);
+
+    let received = sys_recv_notification(event_mask | TIMER_NOTIFICATION);
+
+    // If the event arrived _and_ our timer went off, prioritize the event and
+    // ignore the timeout.
+    if received & event_mask != 0 {
+        // Attempt to clear our timer. Note that this does not protect against
+        // the race where the kernel decided to wake us up, but the timer went
+        // off before we got to this point.
+        sys_set_timer(None, TIMER_NOTIFICATION);
+        I2cControlResult::Interrupted
+    } else {
+        // The event_mask bit was not set, so:
+        I2cControlResult::TimedOut
+    }
+}

drv/stm32xx-i2c/src/ltc4306.rs

@@ -88,7 +88,6 @@ fn read_reg_u8(
     mux: &I2cMux<'_>,
     controller: &I2cController<'_>,
     reg: u8,
-    ctrl: &I2cControl,
 ) -> Result<u8, ResponseCode> {
     let mut rval = 0u8;
     let wlen = 1;
@@ -102,7 +101,6 @@ fn read_reg_u8(
             rval = byte;
             Some(())
         },
-        ctrl,
     );
     match controller_result {
         Err(code) => Err(mux.error_code(code)),
@@ -115,15 +113,13 @@ fn write_reg_u8(
     controller: &I2cController<'_>,
     reg: u8,
     val: u8,
-    ctrl: &I2cControl,
 ) -> Result<(), ResponseCode> {
     match controller.write_read(
         mux.address,
         2,
         |pos| Some(if pos == 0 { reg } else { val }),
         ReadLength::Fixed(0),
         |_, _| Some(()),
-        ctrl,
     ) {
         Err(code) => Err(mux.error_code(code)),
         _ => Ok(()),
@@ -136,7 +132,6 @@ impl I2cMuxDriver for Ltc4306 {
         mux: &I2cMux<'_>,
         _controller: &I2cController<'_>,
         gpio: &sys_api::Sys,
-        _ctrl: &I2cControl,
     ) -> Result<(), drv_i2c_api::ResponseCode> {
         mux.configure(gpio)
     }
@@ -146,7 +141,6 @@ impl I2cMuxDriver for Ltc4306 {
         mux: &I2cMux<'_>,
         controller: &I2cController<'_>,
         segment: Option<Segment>,
-        ctrl: &I2cControl,
     ) -> Result<(), ResponseCode> {
         let mut reg3 = Register3(0);
 
@@ -170,8 +164,8 @@ impl I2cMuxDriver for Ltc4306 {
             }
         }
 
-        write_reg_u8(mux, controller, 3, reg3.0, ctrl)?;
-        let reg0 = Register0(read_reg_u8(mux, controller, 0, ctrl)?);
+        write_reg_u8(mux, controller, 3, reg3.0)?;
+        let reg0 = Register0(read_reg_u8(mux, controller, 0)?);
 
         if !reg0.not_failed() {
             Err(ResponseCode::SegmentDisconnected)