Re: Regression on colibri-imx7 due to commit 9c1b13b3fd27 ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")

[Rasmus Villemoes <rv@rasmusvillemoes.dk>]

On Fri, Jul 24 2026, "Marek Vasut" <marek.vasut@mailbox.org> wrote:

> On 7/23/26 10:41 PM, Emanuele Ghidoli wrote:
>>
>>
>> On 7/23/26 20:35, Marek Vasut wrote:
>>> On 7/23/26 4:53 PM, Emanuele Ghidoli wrote:
>>>
>>> Hello everyone,
>>>
>>>> I found that Colibri iMX7 no longer boots when commit 9c1b13b3fd27
>>>> ("cyclic: reduce get_timer_us() calls inside hlist_for_each_entry_safe()")
>>>> is applied. Nothing is printed on the serial.
>>>>
>>>> I bisected the issue: booting the parent commit works fine, and
>>>> reverting this patch on top of current main also restores
>>>> boot. It's not yet obvious to me what in this change causes the
>>>> regression.
>>>>
>>>> Would it make sense to revert it until the root cause is found?
>>>
>>> Give Patrice a bit of time to analyze the problem.
>>>
>>
>> Hello,
>>
>> after the regression, get_timer_us() is called on every cyclic_run() call,
>> even when the cyclic list is empty. On imx7 this happens earlier than before,
>> when the timer has not yet been initialized: get_timer_us() calls get_tbclk(),
>> whose value is zero, and uses it as a divisor in tick_to_time_us(), leading to
>> a division by zero.
>>
>> I would fix this by ensuring get_tbclk() returns at least 1 on imx7, but we
>> could hide bugs in this way. However, this bug could also be present on other
>> architectures, so I would just guard against entering the cyclic loop by
>> checking whether the list is empty before doing anything else, so
>> get_timer_us() is called "at the same point in time" as before.
>> Something like:
>> if (hlist_empty(cyclic_get_list())) {
>> 	gd->flags &= ~GD_FLG_CYCLIC_RUNNING;
>> 	return;
>> }
> It looks like there are two topics -- optimize the cyclic run and fix
> the MX7 timer.
>
> I agree the cyclic run should exit if the list is empty, but please wait
> for input from Rasmus on that.
>

Yes, we could/should definitely exit early if the list is empty, but
don't mix it up with the GD_FLG_CYCLIC_RUNNING bit. Just add it as a
separate early exit condition. But a future reader might read that as
"just" an optimization, seeing that the loop would just do nothing, I
think it also requires an explanation. Something like

diff --git a/common/cyclic.c b/common/cyclic.c
index 573e715587d..e46346536ab 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -69,6 +69,19 @@ static void cyclic_run(void)
        struct hlist_node *tmp;
        u64 now, after, cpu_time;
 
+       /*
+        * Nothing to do if the list is empty. Also, schedule() can be
+        * called before timer infrastructure is ready, in which case
+        * calling get_timer_us() before the (empty) loop could cause
+        * a divide-by-0 or otherwise crash the system. No clients
+        * should be registered before the timer infrastructure is up,
+        * so the check for the list being empty should be
+        * ok. Otherwise, we would need a new GD_FLG_TIMERS_READY
+        * flag.
+        */
+       if (hlist_empty(cyclic_get_list()))
+                return;
+
        /* Prevent recursion */
        if (gd->flags & GD_FLG_CYCLIC_RUNNING)
                return;

Completely unrelated, but now that I see a new use of that
cyclic_get_list() helper, I think it's time to get rid of that and just
spell it &gd->cyclic_list. It was only needed back when gd was
volatile-qualified.

Rasmus