kernel: fix timeout bugs

When CONFIG_TIMEOUT_64BIT is y, positive values are relative/delta
timeouts and negative values are absolute timeouts, except
for two special values. -1 is K_WAIT_FOREVER and 0 is K_NO_WAIT.
The reserved value of -1 means INT64_MAX is not a valid argument
to K_TIMEOUT_ABS_TICKS(), but there was no check. If a literal
was passed, a preprocessor/compiler warning would be generated
for overflow, but if a variable was passed as the argument,
then the code would compile but not work correctly since the
absolute timeout would be changed to a relative one. One
example of this is task_wdt_init() if no channels are enabled.

Rather than just fixing task_wdt, and trying to find other cases
in an adhoc way, this CL changes K_TIMEOUT_ABS_TICKS() to
limit the larges value to (INT64_MAX-1). It does so silently,
but given the range of int64_t, there should be no practical
difference.

Also, change the implementation for Z_IS_TIMEOUT_RELATIVE() to
fix the case where INT64_MAX relative timeout was being
improperly reported as being not a relative timeout. This was
again due to the -1 reserved value.

Add some tests for these changes to the timer_api test.

Signed-off-by: Mike J. Chen <mjchen@google.com>

Author: mjchen0

include/zephyr/kernel.h

@@ -1488,13 +1488,13 @@ const char *k_thread_state_str(k_tid_t thread_id, char *buf, size_t buf_size);
  * This macro generates a timeout delay that represents an expiration
  * at the absolute uptime value specified, in system ticks.  That is, the
  * timeout will expire immediately after the system uptime reaches the
- * specified tick count.
+ * specified tick count. Value is clamped to the range 0 to INT64_MAX-1.
  *
  * @param t Tick uptime value
  * @return Timeout delay value
  */
 #define K_TIMEOUT_ABS_TICKS(t) \
-	Z_TIMEOUT_TICKS(Z_TICK_ABS((k_ticks_t)MAX(t, 0)))
+	Z_TIMEOUT_TICKS(Z_TICK_ABS((k_ticks_t)CLAMP(t, 0, (INT64_MAX - 1))))
 
 /**
  * @brief Generates an absolute/uptime timeout value from seconds

include/zephyr/sys_clock.h

@@ -151,7 +151,14 @@ typedef struct {
 
 /* Test for relative timeout */
 #if CONFIG_TIMEOUT_64BIT
-#define Z_IS_TIMEOUT_RELATIVE(timeout)  (Z_TICK_ABS((timeout).ticks) < 0)
+/* Positive values are relative/delta timeouts and negative values are absolute
+ * timeouts, except -1 which is reserved for K_TIMEOUT_FOREVER. 0 is K_NO_WAIT,
+ * which is historically considered a relative timeout.
+ * K_TIMEOUT_FOREVER is not considered a relative timeout and neither is it
+ * considerd an absolute timeouts (so !Z_IS_TIMEOUT_RELATIVE() does not
+ * necessarily mean it is an absolute timeout if ticks == -1);
+ */
+#define Z_IS_TIMEOUT_RELATIVE(timeout)  (((timeout).ticks) >= 0)
 #else
 #define Z_IS_TIMEOUT_RELATIVE(timeout)  true
 #endif

tests/kernel/timer/timer_api/src/main.c

@@ -752,6 +752,33 @@ ZTEST_USER(timer_api, test_timeout_abs)
 	/* Ensure second alignment for K_TIMEOUT_ABS_SEC */
 	zassert_true(exp_ms % MSEC_PER_SEC == 0);
 
+	/* Check K_TIMEOUT_ABS_TICKS() and Z_IS_TIMEOUT_RELATIVE macros */
+	t2 = K_NO_WAIT;
+	zassert_true(Z_IS_TIMEOUT_RELATIVE(t2));
+	t2 = K_TICKS(1);
+	zassert_true(Z_IS_TIMEOUT_RELATIVE(t2));
+	t2 = K_TICKS(INT64_MAX-1);
+	zassert_true(Z_IS_TIMEOUT_RELATIVE(t2));
+	t2 = K_TICKS(INT64_MAX);
+	zassert_true(Z_IS_TIMEOUT_RELATIVE(t2));
+
+	zassert_false(Z_IS_TIMEOUT_RELATIVE(t));
+	t2 = K_TIMEOUT_ABS_TICKS(1);
+	zassert_false(Z_IS_TIMEOUT_RELATIVE(t2));
+	t2 = K_TIMEOUT_ABS_TICKS(INT64_MAX-1);
+	zassert_false(Z_IS_TIMEOUT_RELATIVE(t2));
+
+	/* Check when INT64_MAX passed to K_TIMEOUT_ABS_TICKS(), with
+	 * both a literal and variable argument.
+	 */
+	t2 = K_TIMEOUT_ABS_TICKS(INT64_MAX);
+	zassert_false(Z_IS_TIMEOUT_RELATIVE(t2));
+
+	uint64_t max_int64 = INT64_MAX;
+
+	t2 = K_TIMEOUT_ABS_TICKS(max_int64);
+	zassert_false(Z_IS_TIMEOUT_RELATIVE(t2));
+
 	/* Check the other generator macros to make sure they produce
 	 * the same (whiteboxed) converted values
 	 */
@@ -796,6 +823,33 @@ ZTEST_USER(timer_api, test_timeout_abs)
 		     t0+rem_ticks-exp_ticks);
 
 	k_timer_stop(&remain_timer);
+
+	/* Rerun test with t set to INT64_MAX. K_TIMEOUT_ABS_TICKS()
+	 * should adjust the abs timeout to INT64_MAX-1 because the negative
+	 * range used for absolute timeouts needs to reserve -1 for
+	 * K_TIMEOUT_FOREVER.
+	 */
+	init_timer_data();
+	exp_ticks = INT64_MAX - 1;
+	k_timer_start(&remain_timer, K_TIMEOUT_ABS_TICKS(INT64_MAX), K_FOREVER);
+
+	if (IS_ENABLED(CONFIG_MULTITHREADING)) {
+		k_usleep(1);
+	}
+
+	do {
+		t0 = k_uptime_ticks();
+		rem_ticks = k_timer_remaining_ticks(&remain_timer);
+		t1 = k_uptime_ticks();
+	} while (t0 != t1);
+
+	zassert_true(t0 + rem_ticks == exp_ticks,
+		     "Wrong remaining: now %lld rem %lld expires %lld (%lld)",
+		     (uint64_t)t0, (uint64_t)rem_ticks, (uint64_t)exp_ticks,
+		     t0+rem_ticks-exp_ticks);
+
+	k_timer_stop(&remain_timer);
+
 #endif
 }