userlib: add set_timer_relative operation

This provides a convenient shorthand for the common pattern of reading
the timer, adding a small increment, and writing it back. By limiting
the range of the interval type, I was able to avoid overflows in
realistic scenarios (system uptime < 584 million years), and thus use
non-checked arithmetic to eliminate a common panic site.

I've gone through and fixed all the code where this was obviously the
right thing to use, and left some other code intact if I couldn't
convince myself I wouldn't break it.

In some cases I've just switched the code to use saturating adds, which
are not free (at least for 64-bit integers), but are cheaper than
panicking.

drv/gimlet-seq-server/src/main.rs

@@ -187,7 +187,7 @@ struct ServerImpl<S: SpiServer> {
     deadline: u64,
 }
 
-const TIMER_INTERVAL: u64 = 10;
+const TIMER_INTERVAL: u32 = 10;
 
 impl<S: SpiServer + Clone> ServerImpl<S> {
     fn init(
@@ -832,15 +832,22 @@ impl<S: SpiServer> ServerImpl<S> {
                 //
                 // And establish our timer to check SP3_TO_SP_NIC_PWREN_L.
                 //
-                self.deadline = sys_get_timer().now + TIMER_INTERVAL;
-                sys_set_timer(Some(self.deadline), notifications::TIMER_MASK);
+                self.deadline = set_timer_relative(
+                    TIMER_INTERVAL,
+                    notifications::TIMER_MASK,
+                );
 
                 //
                 // Finally, enable transmission to the SP3's UART
                 //
                 uart_sp_to_sp3_enable();
                 ringbuf_entry!(Trace::UartEnabled);
-                ringbuf_entry!(Trace::A0((sys_get_timer().now - start) as u16));
+                // Using wrapping_sub here because the timer is monotonic, so
+                // we, the programmers, know that now > start. rustc, the
+                // compiler, is not aware of this.
+                ringbuf_entry!(Trace::A0(
+                    (sys_get_timer().now.wrapping_sub(start)) as u16
+                ));
 
                 self.update_state_internal(PowerState::A0);
                 Ok(())

drv/lpc55-swd/src/main.rs

@@ -500,8 +500,7 @@ impl idl::InOrderSpCtrlImpl for ServerImpl {
         }
         // This function is idempotent(ish), so we don't care if the timer was
         // already running; set the new deadline based on current time.
-        let deadline = sys_get_timer().now + time_ms as u64;
-        sys_set_timer(Some(deadline), notifications::TIMER_MASK);
+        set_timer_relative(time_ms, notifications::TIMER_MASK);
         Ok(())
     }
 

drv/meanwell/src/main.rs

@@ -103,8 +103,8 @@ impl idl::InOrderMeanwellImpl for ServerImpl {
 task_slot!(USER_LEDS, user_leds);
 task_slot!(SYS, sys);
 
-const TIMER_INTERVAL_LONG: u64 = 900;
-const TIMER_INTERVAL_SHORT: u64 = 100;
+const TIMER_INTERVAL_LONG: u32 = 900;
+const TIMER_INTERVAL_SHORT: u32 = 100;
 
 impl NotificationHandler for ServerImpl {
     fn current_notification_mask(&self) -> u32 {
@@ -115,17 +115,21 @@ impl NotificationHandler for ServerImpl {
         let user_leds =
             drv_user_leds_api::UserLeds::from(USER_LEDS.get_task_id());
 
-        if !self.led_on {
+        let interval = if !self.led_on {
             user_leds.led_on(LED_INDEX).unwrap();
             self.led_on = true;
-            self.deadline += TIMER_INTERVAL_SHORT;
+            TIMER_INTERVAL_SHORT
         } else {
             user_leds.led_off(LED_INDEX).unwrap();
             self.led_on = false;
-            self.deadline += TIMER_INTERVAL_LONG;
-        }
-
-        sys_set_timer(Some(self.deadline), notifications::TIMER_MASK);
+            TIMER_INTERVAL_LONG
+        };
+
+        // This is technically slightly wrong in that, if there's CPU
+        // contention, the LEDs may blink at slightly lower than their intended
+        // frequency. But since the frequency isn't load-bearing, this is
+        // significantly less code:
+        self.deadline = set_timer_relative(interval, notifications::TIMER_MASK);
     }
 }
 

drv/sidecar-seq-server/src/main.rs

@@ -790,7 +790,10 @@ impl NotificationHandler for ServerImpl {
         // be fine. Anyway, armed with this information, find the next deadline
         // some multiple of `TIMER_INTERVAL` in the future.
 
-        let delta = finish - start;
+        // The timer is monotonic, so finish >= start, so we use wrapping_add
+        // here to avoid an overflow check that the compiler conservatively
+        // inserts.
+        let delta = finish.wrapping_sub(start);
         let next_deadline = finish + TIMER_INTERVAL - (delta % TIMER_INTERVAL);
 
         sys_set_timer(Some(next_deadline), notifications::TIMER_MASK);

drv/stm32h7-sprot-server/src/main.rs

@@ -383,15 +383,7 @@ impl<S: SpiServer> Io<S> {
             .unwrap_lite();
 
         // Determine the deadline after which we'll give up, and start the clock.
-        let deadline = sys_get_timer()
-            .now
-            // Values passed to `max_sleep` are all constants that are small
-            // enough that this will almost certainly not overflow unless the SP
-            // has been running without a reset for at least a couple million
-            // years. Using saturating arithmetic here lets us avoid a bounds
-            // check.
-            .saturating_add(max_sleep as u64);
-        sys_set_timer(Some(deadline), TIMER_MASK);
+        let expected_wake = set_timer_relative(max_sleep, TIMER_MASK);
 
         let mut irq_fired = false;
         while self.is_rot_irq_asserted() != desired {

drv/user-leds/src/main.rs

@@ -45,7 +45,7 @@ task_config::optional_task_config! {
     blink_at_start: &'static [Led],
 }
 
-const BLINK_INTERVAL: u64 = 500;
+const BLINK_INTERVAL: u32 = 500;
 
 cfg_if::cfg_if! {
     // Target boards with 4 leds
@@ -148,10 +148,7 @@ impl idl::InOrderUserLedsImpl for ServerImpl {
         self.blinking[led] = true;
 
         if !any_blinking {
-            sys_set_timer(
-                Some(sys_get_timer().now + BLINK_INTERVAL),
-                notifications::TIMER_MASK,
-            );
+            set_timer_relative(BLINK_INTERVAL, notifications::TIMER_MASK);
         }
         Ok(())
     }
@@ -172,10 +169,7 @@ impl idol_runtime::NotificationHandler for ServerImpl {
                 }
             }
             if any_blinking {
-                sys_set_timer(
-                    Some(sys_get_timer().now + BLINK_INTERVAL),
-                    notifications::TIMER_MASK,
-                );
+                set_timer_relative(BLINK_INTERVAL, notifications::TIMER_MASK);
             }
         }
     }
@@ -193,10 +187,7 @@ fn main() -> ! {
             blinking[led] = true;
         }
         if !config.blink_at_start.is_empty() {
-            sys_set_timer(
-                Some(sys_get_timer().now + BLINK_INTERVAL),
-                notifications::TIMER_MASK,
-            );
+            set_timer_relative(BLINK_INTERVAL, notifications::TIMER_MASK);
         }
     }
     let mut server = ServerImpl { blinking };

lib/multitimer/src/lib.rs

@@ -140,8 +140,12 @@ impl<E: EnumArray<Timer> + Copy> Multitimer<E> {
                     // Apply the repeat setting or disable the timer.
                     if let Some(kind) = r {
                         let next = match kind {
-                            Repeat::AfterWake(period) => t + period,
-                            Repeat::AfterDeadline(period) => d + period,
+                            Repeat::AfterWake(period) => {
+                                t.saturating_add(period)
+                            }
+                            Repeat::AfterDeadline(period) => {
+                                d.saturating_add(period)
+                            }
                         };
                         timer.deadline = Some((next, r));
                     } else {

sys/userlib/src/lib.rs

@@ -577,6 +577,23 @@ pub fn sys_set_timer(deadline: Option<u64>, notifications: u32) {
     }
 }
 
+/// Convenience wrapper for `sys_set_timer` that sets a point in time relative
+/// to whatever the current kernel timestamp happens to be.
+///
+/// This is only useful for setting intervals up to 49.7 days. In practice
+/// our intervals tend to be much shorter than this. This restriction exists to
+/// avoid the possibility of overflow: overflow will start happening only within
+/// 49.7 days of the "end of time" in 584 million years.
+///
+/// Returns the actual computed wake time for your reference.
+pub fn set_timer_relative(interval: u32, notifications: u32) -> u64 {
+    // wrapping add because the uptime is likely to be less than 584 million
+    // years.
+    let wake = sys_get_timer().now.wrapping_add(u64::from(interval));
+    sys_set_timer(Some(wake), notifications);
+    wake
+}
+
 /// Core implementation of the SET_TIMER syscall.
 ///
 /// See the note on syscall stubs at the top of this module for rationale.

task/control-plane-agent/src/mgs_gimlet/host_phase2.rs

@@ -118,9 +118,12 @@ impl HostPhase2Requester {
         // in our outgoing net task queue).
         let port = match current.state {
             State::NeedToSendFirstMgs(port) => {
+                // Using saturating_add here because it's cheaper than
+                // panicking, and timestamps won't saturate for 584 million
+                // years.
                 current.state = State::WaitingForFirstMgs {
                     port,
-                    deadline: now + DELAY_TRY_OTHER_MGS,
+                    deadline: now.saturating_add(DELAY_TRY_OTHER_MGS),
                 };
                 port
             }
@@ -136,7 +139,7 @@ impl HostPhase2Requester {
                 };
                 current.state = State::WaitingForSecondMgs {
                     port,
-                    deadline: now + DELAY_RETRY,
+                    deadline: now.saturating_add(DELAY_RETRY),
                 };
                 port
             }
@@ -158,7 +161,7 @@ impl HostPhase2Requester {
                 };
                 current.state = State::WaitingForFirstMgs {
                     port,
-                    deadline: now + DELAY_TRY_OTHER_MGS,
+                    deadline: now.saturating_add(DELAY_TRY_OTHER_MGS),
                 };
                 port
             }

task/host-sp-comms/src/main.rs

@@ -401,9 +401,13 @@ impl ServerImpl {
                     if reboot {
                         // Somehow we're already in A2 when the host wanted to
                         // reboot; set our reboot timer.
+                        //
+                        // Using saturating add here because it's cheaper than
+                        // potentially panicking, and timestamps won't saturate
+                        // for 584 million years.
                         self.timers.set_timer(
                             Timers::WaitingInA2ToReboot,
-                            sys_get_timer().now + A2_REBOOT_DELAY,
+                            sys_get_timer().now.saturating_add(A2_REBOOT_DELAY),
                             None,
                         );
                         self.reboot_state =

task/jefe/src/main.rs

@@ -51,7 +51,7 @@ pub enum Disposition {
 // generally be fast for a human but slow for a computer; we pick a
 // value of ~100 ms.  Our timer mask can't conflict with our fault
 // notification, but can otherwise be arbitrary.
-const TIMER_INTERVAL: u64 = 100;
+const TIMER_INTERVAL: u32 = 100;
 
 #[export_name = "main"]
 fn main() -> ! {
@@ -60,9 +60,8 @@ fn main() -> ! {
         task_states[held_task as usize].disposition = Disposition::Hold;
     }
 
-    let deadline = sys_get_timer().now + TIMER_INTERVAL;
-
-    sys_set_timer(Some(deadline), notifications::TIMER_MASK);
+    let deadline =
+        set_timer_relative(TIMER_INTERVAL, notifications::TIMER_MASK);
 
     external::set_ready();
 
@@ -295,8 +294,10 @@ impl idol_runtime::NotificationHandler for ServerImpl<'_> {
         if bits & notifications::TIMER_MASK != 0 {
             // If our timer went off, we need to reestablish it
             if sys_get_timer().now >= self.deadline {
-                self.deadline += TIMER_INTERVAL;
-                sys_set_timer(Some(self.deadline), notifications::TIMER_MASK);
+                self.deadline = set_timer_relative(
+                    TIMER_INTERVAL,
+                    notifications::TIMER_MASK,
+                );
             }
         }
 

task/monorail-server/src/bsp/sidecar_bcd.rs

@@ -16,8 +16,7 @@ task_slot!(SEQ, seq);
 task_slot!(FRONT_IO, ecp5_front_io);
 
 /// Interval at which `Bsp::wake()` is called by the main loop
-const WAKE_INTERVAL_MS: u64 = 500;
-pub const WAKE_INTERVAL: Option<u64> = Some(WAKE_INTERVAL_MS);
+pub const WAKE_INTERVAL: Option<u32> = Some(500);
 
 #[derive(Copy, Clone, PartialEq, counters::Count)]
 enum Trace {
@@ -497,7 +496,9 @@ impl<'a, R: Vsc7448Rw> Bsp<'a, R> {
             if link_down {
                 let now = userlib::sys_get_timer().now;
                 if let Some(link_down_at) = self.link_down_at {
-                    if now - link_down_at >= 20_000 {
+                    // We can use wrapping arithmetic here because the timer is
+                    // monotonic.
+                    if now.wrapping_sub(link_down_at) >= 20_000 {
                         self.link_down_at = None;
                         // This logs Trace::Reinit in the ringbuf
                         self.reinit()?;

task/monorail-server/src/server.rs

@@ -61,9 +61,8 @@ impl<'a, R: Vsc7448Rw> ServerImpl<'a, R> {
         if let Some(wake_interval) = bsp::WAKE_INTERVAL {
             if now >= self.wake_target_time {
                 let out = self.bsp.wake();
-                self.wake_target_time = now + wake_interval;
-                sys_set_timer(
-                    Some(self.wake_target_time),
+                self.wake_target_time = userlib::set_timer_relative(
+                    wake_interval,
                     notifications::WAKE_TIMER_MASK,
                 );
                 return out;

task/power/src/main.rs

@@ -51,7 +51,7 @@ enum PowerState {
     A2,
 }
 
-const TIMER_INTERVAL: u64 = 1000;
+const TIMER_INTERVAL: u32 = 1000;
 
 task_slot!(I2C, i2c_driver);
 task_slot!(SENSOR, sensor);
@@ -419,10 +419,7 @@ fn main() -> ! {
     };
     let mut buffer = [0; idl::INCOMING_SIZE];
 
-    sys_set_timer(
-        Some(sys_get_timer().now + TIMER_INTERVAL),
-        notifications::TIMER_MASK,
-    );
+    userlib::set_timer_relative(TIMER_INTERVAL, notifications::TIMER_MASK);
     loop {
         idol_runtime::dispatch(&mut buffer, &mut server);
     }
@@ -626,10 +623,7 @@ impl idol_runtime::NotificationHandler for ServerImpl {
 
     fn handle_notification(&mut self, _bits: u32) {
         self.handle_timer_fired();
-        sys_set_timer(
-            Some(sys_get_timer().now + TIMER_INTERVAL),
-            notifications::TIMER_MASK,
-        );
+        userlib::set_timer_relative(TIMER_INTERVAL, notifications::TIMER_MASK);
     }
 }
 

task/thermal/src/main.rs

@@ -329,7 +329,8 @@ impl<'a> NotificationHandler for ServerImpl<'a> {
             }
             self.deadline = now + TIMER_INTERVAL;
         }
-        self.runtime = sys_get_timer().now - now;
+        // We can use wrapping arithmetic here because the timer is monotonic.
+        self.runtime = sys_get_timer().now.wrapping_sub(now);
         sys_set_timer(Some(self.deadline), notifications::TIMER_MASK);
     }
 }