drivers/timer/systick: Improve clock slippage under irq_lock load

The SysTick logic looked logically sound, but it was allowing us to
set a LOAD value as low as 512 cycles.  On other platforms, that
minimum future interrupt delay is there to protect the "read, compute,
write, unmask" cycle that sets the new interrupt from trying to set
one sooner than it can handle.

But with SysTick, that value then becomes the value of the LOAD
register, which is effectively the frequency with which timer
interrupts arrive.  This has two side effects:

1. It opens up the possibility that future code that masks interrupts
   longer than 512 cycles will miss more than one overflow, slipping
   the clock backward as viewed by z_clock_announce().

2. The original code only understood one overflow cycle, so in the
   event we do set one of these very near timeouts and then mask
   interrupts, we'll only add at most one overflow to the "elapsed()"
   time, slipping the CURRENT time backward (actually turning it into
   a non-monotonic sawtooth which would slip every LOAD cycle) and
   thus messing up future scheduled interrupts, slipping those forward
   relative to what the ISR was counting.

This patch simplifies the logic for reading SysTick VAL/CTRL (the loop
wasn't needed), handles the case where we see more than one overflow,
and increases the MIN_DELAY cycles from 512 to 1/16th of a tick.

Fixes #15216

Signed-off-by: Andy Ross <[email protected]>

Author: Andy Ross

drivers/timer/cortex_m_systick.c

@@ -10,9 +10,6 @@
 
 void z_ExcExit(void);
 
-/* Minimum cycles in the future to try to program. */
-#define MIN_DELAY 512
-
 #define COUNTER_MAX 0x00ffffff
 #define TIMER_STOPPED 0xff000000
 
@@ -21,9 +18,26 @@ void z_ExcExit(void);
 #define MAX_TICKS ((COUNTER_MAX / CYC_PER_TICK) - 1)
 #define MAX_CYCLES (MAX_TICKS * CYC_PER_TICK)
 
+/* Minimum cycles in the future to try to program.  Note that this is
+ * NOT simply "enough cycles to get the counter read and reprogrammed
+ * reliably" -- it becomes the minimum value of the LOAD register, and
+ * thus reflects how much time we can reliably see expire between
+ * calls to elapsed() to read the COUNTFLAG bit.  So it needs to be
+ * set to be larger than the maximum time the interrupt might be
+ * masked.  Choosing a fraction of a tick is probably a good enough
+ * default, with an absolute minimum of 1k cyc.
+ */
+#define MIN_DELAY MAX(1024, (CYC_PER_TICK/16))
+
 #define TICKLESS (IS_ENABLED(CONFIG_TICKLESS_KERNEL) &&			\
 		  !IS_ENABLED(CONFIG_QEMU_TICKLESS_WORKAROUND))
 
+/* VAL value above which we assume that a subsequent COUNTFLAG
+ * overflow seen in CTRL is real and not an artifact of wraparound
+ * timing.
+ */
+#define VAL_ABOUT_TO_WRAP 8
+
 static struct k_spinlock lock;
 
 static u32_t last_load;
@@ -32,19 +46,40 @@ static u32_t cycle_count;
 
 static u32_t announced_cycles;
 
-static volatile u32_t ctrl_cache; /* overflow bit clears on read! */
+static volatile u32_t overflow_cyc;
 
 static u32_t elapsed(void)
 {
-	u32_t val, ov;
+	u32_t val, ctrl1, ctrl2;
+
+	/* SysTick is infuriatingly racy.  The counter wraps at zero
+	 * automatically, setting a 1 in the COUNTFLAG bit of the CTRL
+	 * register when it does.  But reading the control register
+	 * automatically resets that bit, so we need to save it for
+	 * future calls.  And ordering is critical and race-prone: if
+	 * we read CTRL first, then it is possible for VAL to wrap
+	 * after that read but before we read VAL and we'll miss the
+	 * overflow.  If we read VAL first, then it can wrap after we
+	 * read it and we'll see an "extra" overflow in CTRL.  And we
+	 * want to handle multiple overflows, so we effectively must
+	 * read CTRL first otherwise there will be no way to detect
+	 * the double-overflow if called at the end of a cycle.  There
+	 * is no safe algorithm here, so we split the difference by
+	 * reading CTRL twice, suppressing the second overflow bit if
+	 * VAL was "about to overflow".
+	 */
+	ctrl1 = SysTick->CTRL;
+	val = SysTick->VAL & COUNTER_MAX;
+	ctrl2 = SysTick->CTRL;
 
-	do {
-		val = SysTick->VAL & COUNTER_MAX;
-		ctrl_cache |= SysTick->CTRL;
-	} while (SysTick->VAL > val);
+	overflow_cyc += (ctrl1 & SysTick_CTRL_COUNTFLAG_Msk) ? last_load : 0;
+	if (val > VAL_ABOUT_TO_WRAP) {
+		int wrap = ctrl2 & SysTick_CTRL_COUNTFLAG_Msk;
+
+		overflow_cyc += (wrap != 0) ? last_load : 0;
+	}
 
-	ov = (ctrl_cache & SysTick_CTRL_COUNTFLAG_Msk) ? last_load : 0;
-	return (last_load - val) + ov;
+	return (last_load - val) + overflow_cyc;
 }
 
 /* Callout out of platform assembly, not hooked via IRQ_CONNECT... */
@@ -57,8 +92,8 @@ void z_clock_isr(void *arg)
 	dticks = (cycle_count - announced_cycles) / CYC_PER_TICK;
 	announced_cycles += dticks * CYC_PER_TICK;
 
-	ctrl_cache = SysTick->CTRL; /* Reset overflow flag */
-	ctrl_cache = 0U;
+	overflow_cyc = SysTick->CTRL; /* Reset overflow flag */
+	overflow_cyc = 0U;
 
 	z_clock_announce(TICKLESS ? dticks : 1);
 	z_ExcExit();
@@ -68,6 +103,7 @@ int z_clock_driver_init(struct device *device)
 {
 	NVIC_SetPriority(SysTick_IRQn, _IRQ_PRIO_OFFSET);
 	last_load = CYC_PER_TICK;
+	overflow_cyc = 0U;
 	SysTick->LOAD = last_load;
 	SysTick->VAL = 0; /* resets timer to last_load */
 	SysTick->CTRL |= (SysTick_CTRL_ENABLE_Msk |
@@ -107,6 +143,7 @@ void z_clock_set_timeout(s32_t ticks, bool idle)
 	delay = ((delay + CYC_PER_TICK - 1) / CYC_PER_TICK) * CYC_PER_TICK;
 	last_load = delay - (cycle_count - announced_cycles);
 
+	overflow_cyc = 0U;
 	SysTick->LOAD = last_load;
 	SysTick->VAL = 0; /* resets timer to last_load */