From 9075d5335596080291ebcc448819fed2110dcb9a Mon Sep 17 00:00:00 2001 From: "Mike J. Chen" Date: Thu, 13 Mar 2025 15:40:53 -0700 Subject: [PATCH] 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 --- include/zephyr/kernel.h | 4 +- include/zephyr/sys_clock.h | 9 ++++- tests/kernel/timer/timer_api/src/main.c | 54 +++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/include/zephyr/kernel.h b/include/zephyr/kernel.h index 34e704b7b9e..f54a07e68ae 100644 --- a/include/zephyr/kernel.h +++ b/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 diff --git a/include/zephyr/sys_clock.h b/include/zephyr/sys_clock.h index 5f31d585ff1..31bbd9a3c2b 100644 --- a/include/zephyr/sys_clock.h +++ b/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 diff --git a/tests/kernel/timer/timer_api/src/main.c b/tests/kernel/timer/timer_api/src/main.c index 6e4b999b352..8ca3e722175 100644 --- a/tests/kernel/timer/timer_api/src/main.c +++ b/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 }