From caa892796a1c6201476f646a93a9f923f8ab60d7 Mon Sep 17 00:00:00 2001 From: Jukka Laitinen Date: Fri, 27 Sep 2024 08:22:22 +0300 Subject: [PATCH 1/3] drivers/timers/arch_alarm.c: Clean-up oneshot_callback for non-tickless build Signed-off-by: Jukka Laitinen --- drivers/timers/arch_alarm.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/drivers/timers/arch_alarm.c b/drivers/timers/arch_alarm.c index 33c61711fa847..d353a6d87a58e 100644 --- a/drivers/timers/arch_alarm.c +++ b/drivers/timers/arch_alarm.c @@ -116,26 +116,34 @@ static void udelay_coarse(useconds_t microseconds) static void oneshot_callback(FAR struct oneshot_lowerhalf_s *lower, FAR void *arg) { - clock_t now = 0; + clock_t now; + ONESHOT_TICK_CURRENT(g_oneshot_lower, &now); #ifdef CONFIG_SCHED_TICKLESS - ONESHOT_TICK_CURRENT(g_oneshot_lower, &now); nxsched_alarm_tick_expiration(now); #else - clock_t delta; + /* Start the next tick first, in order to minimize latency. Ideally + * the ONESHOT_TICK_START would also return the current tick so that + * the retriving the current tick and starting the new one could be done + * atomically w. respect to a HW timer + */ - do - { - clock_t next; + ONESHOT_TICK_START(g_oneshot_lower, oneshot_callback, NULL, 1); + /* It is always an error if this progresses more than 1 tick at a time. + * That would break any timer based on wdog; such timers might timeout + * early. Add a DEBUGASSERT here to catch those errors. It is not added + * here by default, since it would break debugging. These errors + * would occur due to HW timers possibly running while CPU is being halted. + */ + + /* DEBUGASSERT(now - g_current_tick <= 1); */ + + while (now - g_current_tick > 0) + { + g_current_tick++; nxsched_process_timer(); - next = ++g_current_tick; - ONESHOT_TICK_CURRENT(g_oneshot_lower, &now); - delta = next - now; } - while ((sclock_t)delta <= 0); - - ONESHOT_TICK_START(g_oneshot_lower, oneshot_callback, NULL, delta); #endif } From e00061ea0b3ef8508023af5fdbbd57eec01ef0cb Mon Sep 17 00:00:00 2001 From: Jukka Laitinen Date: Fri, 27 Sep 2024 09:24:01 +0300 Subject: [PATCH 2/3] arch/arm64/src/common/arm64_arch_timer.c: Remove clock drift from tick timer This fixes two issues with the tick timer 1) Each tick was longer than the requested period. This is because setting the compare register was done by first reading the current time, and only after that setting the compare register. In addition, when handling the timer interrupts in arch_alarm.c / oneshot_callback, the current_tick is first read, all the tick handling is done and only after that the next tick is started. The whole tick processing time was added to the total tick time. 2) When the compare time is not aligned with tick period, and is drifting, eventually any call to ONESHOT_TICK_CURRENT would either return the current tick, or the next one, depending on the rounding of division by the cycle_per_tick. This again leads to oneshot_callback randomly handling two ticks at a time, which breaks all wdog based timers, causing them to randomly timeout too early. The issues are fixed as follows: Align the compare time register to be evenly divisible by cycle_per_tick. This will lead arm64_tick_current always to return the currently ongoing tick, fixing 2). Also calculating the next tick's start from the aligned current count will fix 1), as there is no time drift in the start cycle. Signed-off-by: Jukka Laitinen --- arch/arm64/src/common/arm64_arch_timer.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/arch/arm64/src/common/arm64_arch_timer.c b/arch/arm64/src/common/arm64_arch_timer.c index 21fc07c3adc2b..ce7e4dda0269e 100644 --- a/arch/arm64/src/common/arm64_arch_timer.c +++ b/arch/arm64/src/common/arm64_arch_timer.c @@ -244,6 +244,7 @@ static int arm64_tick_start(struct oneshot_lowerhalf_s *lower, { struct arm64_oneshot_lowerhalf_s *priv = (struct arm64_oneshot_lowerhalf_s *)lower; + uint64_t next_cycle; DEBUGASSERT(priv != NULL && callback != NULL); @@ -252,10 +253,11 @@ static int arm64_tick_start(struct oneshot_lowerhalf_s *lower, priv->callback = callback; priv->arg = arg; - /* Set the timeout */ + next_cycle = + arm64_arch_timer_count() / priv->cycle_per_tick * priv->cycle_per_tick + + ticks * priv->cycle_per_tick; - arm64_arch_timer_set_compare(arm64_arch_timer_count() + - priv->cycle_per_tick * ticks); + arm64_arch_timer_set_compare(next_cycle); arm64_arch_timer_set_irq_mask(false); return OK; @@ -418,4 +420,4 @@ void arm64_arch_timer_secondary_init() arm64_arch_timer_enable(true); #endif } -#endif \ No newline at end of file +#endif From a2c0d6f8905319044ba579dd39dc48d2e7c7c860 Mon Sep 17 00:00:00 2001 From: Jukka Laitinen Date: Fri, 27 Sep 2024 09:43:47 +0300 Subject: [PATCH 3/3] sched/semaphore/sem_tickwait.c: Fix nxsem_tickwait_uninterruptible end condition nxsem_tickwait correctly sleeps more than 1 tick. But nxsem_tickwait_uninterruptible may wake up to a signal (with -EINTR), in which case the tick + 1 must also be taken into account. Otherwise the nxsem_tickwait_uninterruptible may wake up 1 tick too early. Also fix the nxsem_tickwait to return with -ETIMEDOUT if called with delay 0. This is similar to e.g. posix sem_timedwait. Signed-off-by: Jukka Laitinen --- sched/semaphore/sem_tickwait.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sched/semaphore/sem_tickwait.c b/sched/semaphore/sem_tickwait.c index c6d421739519e..9f219bc5712fa 100644 --- a/sched/semaphore/sem_tickwait.c +++ b/sched/semaphore/sem_tickwait.c @@ -100,8 +100,9 @@ int nxsem_tickwait(FAR sem_t *sem, uint32_t delay) if (delay == 0) { - /* Return the errno from nxsem_trywait() */ + /* Timed out already before waiting */ + ret = -ETIMEDOUT; goto out; } @@ -149,7 +150,7 @@ int nxsem_tickwait(FAR sem_t *sem, uint32_t delay) int nxsem_tickwait_uninterruptible(FAR sem_t *sem, uint32_t delay) { - clock_t end = clock_systime_ticks() + delay; + clock_t end = clock_systime_ticks() + delay + 1; int ret; for (; ; )