stm32xx-i2c: fix notification-induced crash

The I2C code uses the kernel timer to attempt to detect cases where the
hardware has "gone away" and stopped generating interrupts.

(What this _actually_ means is that the software state machine has
gotten a transition wrong: we've never found evidence of misbehavior in
the hardware block, at least so far. I'm including this caveat for
people who find this commit online later.)

However, this code has contained two bugs, either of which could crash
the driver.

First, the race condition. Under high load (say, when a crash dump is
being recorded), the I2C driver may not reliably clear accumulated
timer-expired notifications. This can happen as follows:

1. The hardware interrupt arrives. The kernel maps it to the I2C
   driver's notification mask. The notification bit is immediately set,
   and the driver is returned to "runnable" state.

2. Load continues and the driver does not actually get to run.

3. The driver's timer goes off. The kernel sets the timer notification
   bit. However, because the driver has already technically been woken
   up with an atomic snapshot of notification conditions, the bit will
   not be _observed_ until the next recv in the driver task.

4. Next time the driver wants to impose a timeout, it sets up the timer
   and listens for timer notifications. It receives one immediately,
   decides it has "lost an interrupt," and crashes itself.

Second, and hopefully less likely, this behavior allows the driver to be
killed by any other task using `sys_post` to the driver's timer bit.
This is because the I2C driver assumes that any timer notification that
happens before a hardware operation complete implies a failure. This is
obviously not true if the timer notification is made up.

The root cause for both of these is that the I2C driver's time
management code was not following best practices for Hubris timers and
interrupts, which is to _verify that the condition has actually
occurred_ before responding to a notification bit. This prevents
notifications "hung over" from a previous run from disturbing the next
run, and blocks other tasks from disrupting execution as a side effect.

This commit fixes that, by inserting a timer check loop around the recv.

Author: cbiffle

drv/stm32xx-i2c/src/lib.rs

@@ -1196,18 +1196,33 @@ fn wfi_raw(event_mask: u32, timeout: I2cTimeout) -> I2cControlResult {
 
     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
+    loop {
+        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);
+            break I2cControlResult::Interrupted;
+        } else {
+            // The timer bit must have been set. Verify that our timer has
+            // actually expired:
+            if sys_get_timer().now >= dead {
+                break I2cControlResult::TimedOut;
+            }
+
+            // Otherwise, one of two things has happened:
+            // 1. Some joker has posted to our timer bit. Ha ha very funny.
+            // 2. The timer bit was _already set_ on entry to this routine, as a
+            //    hangover from a previous run.
+            //
+            // We don't need to re-set our timer here because we sampled
+            // sys_get_timer _after_ receiving notifications, meaning it either
+            // hasn't gone off, or it has gone off but that fact is still stored
+            // in our notification bits.
+        }
     }
 }