diff --git a/components/zkernel/include/boreas/zephyr/kernel.h b/components/zkernel/include/boreas/zephyr/kernel.h index d489852..60b23a6 100644 --- a/components/zkernel/include/boreas/zephyr/kernel.h +++ b/components/zkernel/include/boreas/zephyr/kernel.h @@ -182,18 +182,24 @@ struct k_sem { portMUX_TYPE lock; }; +/* Compile-time k_sem initializer body. @p self must be the sem being + * initialized (the waiters list is self-referential). Single source + * for K_SEM_DEFINE and embedded sems (K_TIMER_DEFINE). */ +#define Z_SEM_INITIALIZER(self, _initial, _limit) \ + { \ + .count = (_initial), \ + .limit = (_limit), \ + .waiters = SYS_DLIST_STATIC_INIT(&(self).waiters), \ + .lock = portMUX_INITIALIZER_UNLOCKED, \ + } + /** * Statically define a fully-initialized semaphore. True compile-time * initializer (matches upstream Zephyr): usable from constructors and * SYS_INIT callbacks without any runtime init step. */ #define K_SEM_DEFINE(name, _initial, _limit) \ - struct k_sem name = { \ - .count = (_initial), \ - .limit = (_limit), \ - .waiters = SYS_DLIST_STATIC_INIT(&name.waiters), \ - .lock = portMUX_INITIALIZER_UNLOCKED, \ - }; \ + struct k_sem name = Z_SEM_INITIALIZER(name, _initial, _limit); \ BUILD_ASSERT(((_limit) != 0) && ((_initial) <= (_limit)), \ "K_SEM_DEFINE: limit must be nonzero and >= initial") /* upstream parity */ @@ -415,6 +421,10 @@ struct k_timer { k_timer_stop_t stop_fn; void *user_data; uint32_t status; /* expiry count since last status read */ + /* status_sync wake latch (binary sem, upstream's single-waiter + * model): given after each expiry and by k_timer_stop. The COUNT + * lives in `status`; the sem only carries the wake. */ + struct k_sem sync_sem; bool running; bool is_periodic; /* last k_timer_start was periodic; one-shot if false */ bool first_interval_pending; /* start-once then switch to periodic */ @@ -429,8 +439,15 @@ struct k_timer { struct k_timer name = { \ .expiry_fn = (_expiry_fn), \ .stop_fn = (_stop_fn), \ + .sync_sem = Z_SEM_INITIALIZER(name.sync_sem, 0, 1), \ } +/** + * @note Re-initializing a timer while a thread is blocked in + * k_timer_status_sync on it is a caller error (it clobbers the + * embedded semaphore's waiter list) -- the same reuse rule as + * re-initializing a k_sem with a blocked taker. + */ void k_timer_init(struct k_timer *timer, k_timer_expiry_t expiry_fn, k_timer_stop_t stop_fn); /** * @brief Start or restart a timer. @@ -445,6 +462,10 @@ void k_timer_init(struct k_timer *timer, k_timer_expiry_t expiry_fn, k_timer_sto * synchronize with an in-flight ISR callback on SMP targets. * On linux, k_timer_stop likewise does not synchronize with a * callback the dispatcher task has already dequeued. + * + * @note Restarting does NOT wake a thread blocked in + * k_timer_status_sync -- it keeps waiting for the restarted + * timer's first expiry (or a stop), matching upstream. */ void k_timer_start(struct k_timer *timer, k_timeout_t duration, k_timeout_t period); void k_timer_stop(struct k_timer *timer); @@ -460,15 +481,16 @@ uint32_t k_timer_status_get(struct k_timer *timer); * Returns immediately if the count is already non-zero, or if the * timer is already stopped. * - * @note Must not be called from an interrupt handler (blocks). + * @note Must not be called from an interrupt handler (blocks) -- + * matches upstream. * - * @note Boreas implementation polls every k_msleep(1). Upstream - * Zephyr blocks the calling thread on a wait queue. Same - * caller-visible semantics; the wake granularity here is - * bounded by the FreeRTOS tick period (portTICK_PERIOD_MS, - * set by CONFIG_FREERTOS_HZ -- 1ms at the Boreas default of - * 1000 Hz, 10ms at the ESP-IDF Kconfig default of 100 Hz). - * A wake can therefore be up to one tick period late. + * @note Blocks on the timer's embedded semaphore, woken by expiry or + * k_timer_stop (upstream's single-waiter wait-queue model: one + * thread waits per timer). + * @note Divergence: a thread blocked here must NOT be aborted. + * Upstream unpends an aborted thread from the timer wait queue; + * Boreas cannot, so the dead thread would leave a dangling + * waiter node (same limitation as k_sem_take -- see its @note). */ uint32_t k_timer_status_sync(struct k_timer *timer); diff --git a/components/zkernel/src/k_timer.c b/components/zkernel/src/k_timer.c index ba85dc3..1f858d8 100644 --- a/components/zkernel/src/k_timer.c +++ b/components/zkernel/src/k_timer.c @@ -40,6 +40,11 @@ static void K_ISR_SAFE k_timer_esp_callback(void *arg) if (!__atomic_load_n(&timer->is_periodic, __ATOMIC_ACQUIRE)) { __atomic_store_n(&timer->running, false, __ATOMIC_RELEASE); } + + /* Wake a status_sync waiter LAST (upstream wakes at the end of + * its expiration handler, after the expiry callback). ISR-safe + * under both dispatch modes. */ + k_sem_give(&timer->sync_sem); } void k_timer_init(struct k_timer *timer, k_timer_expiry_t expiry_fn, k_timer_stop_t stop_fn) @@ -53,6 +58,9 @@ void k_timer_init(struct k_timer *timer, k_timer_expiry_t expiry_fn, k_timer_sto timer->first_interval_pending = false; timer->period_us = 0; timer->handle = NULL; + /* Binary: the sem is the status_sync wake latch, not a counter + * (the expiry count lives in timer->status). */ + (void)k_sem_init(&timer->sync_sem, 0, 1); const esp_timer_create_args_t args = { .callback = k_timer_esp_callback, @@ -133,6 +141,9 @@ void k_timer_stop(struct k_timer *timer) if (timer->stop_fn) { timer->stop_fn(timer); } + /* Wake a status_sync waiter (upstream unpends one waiter on + * stop, after invoking the stop function). */ + k_sem_give(&timer->sync_sem); } } @@ -143,18 +154,32 @@ uint32_t k_timer_status_get(struct k_timer *timer) uint32_t k_timer_status_sync(struct k_timer *timer) { - /* Block until the timer expires or is stopped. Polls every - * k_msleep(1), which rounds up to one FreeRTOS tick - * (portTICK_PERIOD_MS, set by CONFIG_FREERTOS_HZ -- 1ms at the - * Boreas default of 1000 Hz, 10ms at the ESP-IDF Kconfig default - * of 100 Hz). Caller-visible semantics match upstream Zephyr's - * wait-queue version (block until expiry or stop, return count - * atomically); the divergence is the polling implementation. */ - while (__atomic_load_n(&timer->running, __ATOMIC_ACQUIRE) && - __atomic_load_n(&timer->status, __ATOMIC_ACQUIRE) == 0) { - k_msleep(1); + /* Upstream shape: return the accumulated count immediately if + * non-zero, or 0 if the timer is stopped; otherwise block until + * expiry or stop. Blocking rides the embedded sync sem, which + * the expiry path and k_timer_stop give (k_sem_give is ISR-safe, + * so this works under both dispatch modes). The sem is a wake + * LATCH, not a counter: a give latched while no waiter was + * blocked makes a later take return early, so re-check and + * re-block in a loop. */ + for (;;) { + uint32_t result = __atomic_exchange_n(&timer->status, 0, __ATOMIC_ACQ_REL); + + if (result > 0) { + return result; + } + if (!__atomic_load_n(&timer->running, __ATOMIC_ACQUIRE)) { + /* Re-read, don't return 0: an expiry (or a stop after + * an expiry) may have completed between the exchange + * above and the running load. The callback bumps + * status (RELEASE) before clearing running (RELEASE), + * so a running==false ACQUIRE load is guaranteed to + * observe that increment -- upstream returns the + * count here, never drops it. */ + return __atomic_exchange_n(&timer->status, 0, __ATOMIC_ACQ_REL); + } + (void)k_sem_take(&timer->sync_sem, K_FOREVER); } - return __atomic_exchange_n(&timer->status, 0, __ATOMIC_ACQ_REL); } uint32_t k_timer_remaining_get(struct k_timer *timer) diff --git a/components/zkernel/src/k_timer_linux.c b/components/zkernel/src/k_timer_linux.c index 426ee67..e7153f4 100644 --- a/components/zkernel/src/k_timer_linux.c +++ b/components/zkernel/src/k_timer_linux.c @@ -110,6 +110,8 @@ static void z_timer_dispatcher(void *arg) if (!__atomic_load_n(&due->is_periodic, __ATOMIC_ACQUIRE)) { __atomic_store_n(&due->running, false, __ATOMIC_RELEASE); } + /* Wake a status_sync waiter LAST (see k_timer.c). */ + k_sem_give(&due->sync_sem); continue; /* more timers may already be due */ } @@ -164,6 +166,9 @@ void k_timer_init(struct k_timer *timer, k_timer_expiry_t expiry_fn, k_timer_sto timer->first_interval_pending = false; /* unused on linux; deadline math is direct */ timer->period_us = 0; timer->deadline_us = 0; + /* Binary: the sem is the status_sync wake latch, not a counter + * (the expiry count lives in timer->status). */ + (void)k_sem_init(&timer->sync_sem, 0, 1); timer->node.next = NULL; timer->node.prev = NULL; /* No esp_timer exists on linux; `handle` serves only as the shared @@ -214,6 +219,9 @@ void k_timer_stop(struct k_timer *timer) if (timer->stop_fn) { timer->stop_fn(timer); } + /* Wake a status_sync waiter (upstream unpends one waiter on + * stop, after invoking the stop function). */ + k_sem_give(&timer->sync_sem); } uint32_t k_timer_status_get(struct k_timer *timer) @@ -223,13 +231,26 @@ uint32_t k_timer_status_get(struct k_timer *timer) uint32_t k_timer_status_sync(struct k_timer *timer) { - /* Same poll-based implementation as the hardware backend in - * k_timer.c -- see the divergence note there. */ - while (__atomic_load_n(&timer->running, __ATOMIC_ACQUIRE) && - __atomic_load_n(&timer->status, __ATOMIC_ACQUIRE) == 0) { - k_msleep(1); + /* Same sem-backed implementation as the hardware backend -- see + * the rationale in k_timer.c. */ + for (;;) { + uint32_t result = __atomic_exchange_n(&timer->status, 0, __ATOMIC_ACQ_REL); + + if (result > 0) { + return result; + } + if (!__atomic_load_n(&timer->running, __ATOMIC_ACQUIRE)) { + /* Re-read, don't return 0: an expiry (or a stop after + * an expiry) may have completed between the exchange + * above and the running load. The callback bumps + * status (RELEASE) before clearing running (RELEASE), + * so a running==false ACQUIRE load is guaranteed to + * observe that increment -- upstream returns the + * count here, never drops it. */ + return __atomic_exchange_n(&timer->status, 0, __ATOMIC_ACQ_REL); + } + (void)k_sem_take(&timer->sync_sem, K_FOREVER); } - return __atomic_exchange_n(&timer->status, 0, __ATOMIC_ACQ_REL); } /* Locked snapshot of deadline_us: the dispatcher re-arms periodic timers diff --git a/test/main/test_k_timer.c b/test/main/test_k_timer.c index 9d2485c..21af2f5 100644 --- a/test/main/test_k_timer.c +++ b/test/main/test_k_timer.c @@ -196,6 +196,52 @@ static void test_timer_status_sync_wakes_on_stop(void) k_thread_join(&waker_thread, K_FOREVER); } +/* #28 regression: the status_sync wake rides a binary sem LATCH, and a + * latched give from an expiry nobody was waiting on must not satisfy a + * later status_sync early -- after status_get drains the count, sync + * must block until the NEXT expiry (the re-check loop in + * k_timer_status_sync is what absorbs the stale latch). + * + * Deterministic shape (review): one-shot expiries only, and t0 is + * captured BEFORE the restart -- the new expiry cannot fire earlier + * than start+60, so elapsed >= ~60 holds no matter how the host + * stalls the test thread (a periodic re-arm racing the entry was + * flaky under CI load). A buggy implementation that honors the stale + * latch returns 0 with elapsed ~0. */ +static void test_timer_status_sync_after_status_get_blocks(void) +{ + struct k_timer timer; + k_timer_init(&timer, test_timer_cb, NULL); + + /* One-shot expires with NO waiter: the latch is left set. */ + k_timer_start(&timer, K_MSEC(20), K_NO_WAIT); + k_msleep(40); + TEST_ASSERT_EQUAL(1, k_timer_status_get(&timer)); + + /* Restart (start does not drain the latch), then sync. */ + uint32_t t0 = (uint32_t)k_uptime_get(); + k_timer_start(&timer, K_MSEC(60), K_NO_WAIT); + uint32_t status = k_timer_status_sync(&timer); + uint32_t elapsed = (uint32_t)k_uptime_get() - t0; + + TEST_ASSERT_EQUAL(1, status); + TEST_ASSERT_GREATER_OR_EQUAL(50, elapsed); /* blocked until the NEW expiry */ + + k_timer_stop(&timer); +} + +static void test_timer_status_sync_stopped_returns_immediately(void) +{ + struct k_timer timer; + k_timer_init(&timer, test_timer_cb, NULL); + + /* Upstream: a stopped/never-started timer with status 0 returns 0 + * without blocking. */ + uint32_t t0 = (uint32_t)k_uptime_get(); + TEST_ASSERT_EQUAL(0, k_timer_status_sync(&timer)); + TEST_ASSERT_LESS_THAN(10, (uint32_t)k_uptime_get() - t0); +} + static void test_timer_remaining_ticks(void) { struct k_timer timer; @@ -455,6 +501,8 @@ void test_k_timer_group(void) RUN_TEST(test_timer_status_sync_blocking); RUN_TEST(test_timer_status_sync_already_fired); RUN_TEST(test_timer_status_sync_wakes_on_stop); + RUN_TEST(test_timer_status_sync_after_status_get_blocks); + RUN_TEST(test_timer_status_sync_stopped_returns_immediately); RUN_TEST(test_timer_remaining_ticks); RUN_TEST(test_timer_expires_ticks); RUN_TEST(test_timer_one_shot_clears_running);