Re: [PATCH 06/14] vdso/gettimeofday: Return bool from clock_gettime() helpers

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 08.07.2025 17:17, Marek Szyprowski wrote:
> On 01.07.2025 10:58, Thomas Weißschuh wrote:
>> The internal helpers are effectively using boolean results,
>> while pretending to use error numbers.
>>
>> Switch the return type to bool for more clarity.
>>
>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>> ---
>>   lib/vdso/gettimeofday.c | 58 
>> +++++++++++++++++++++++++------------------------
>>   1 file changed, 30 insertions(+), 28 deletions(-)
>
> This patch landed in today's linux-next as commit fcc8e46f768f 
> ("vdso/gettimeofday: Return bool from clock_gettime() helpers"). In my 
> tests I found that it causes serious problem with hwclock operation on 
> some of my ARM 32bit test boards. I observe that calling "hwclock -w 
> -f /dev/rtc0" never ends on those boards. Disabling vdso support (by 
> removing ARM architected timer) fixes this issue.

I spent some time analyzing the code refactored in this patch and it 
looks that the following change is missing:

diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c
index c5266532a097..7e79b02839b0 100644
--- a/lib/vdso/gettimeofday.c
+++ b/lib/vdso/gettimeofday.c
@@ -344,7 +344,7 @@ __cvdso_gettimeofday_data(const struct 
vdso_time_data *vd,
         if (likely(tv != NULL)) {
                 struct __kernel_timespec ts;

-               if (do_hres(vd, &vc[CS_HRES_COARSE], CLOCK_REALTIME, &ts))
+               if (!do_hres(vd, &vc[CS_HRES_COARSE], CLOCK_REALTIME, &ts))
                         return gettimeofday_fallback(tv, tz);

                 tv->tv_sec = ts.tv_sec;


In my tests this fixed the hwclock issue on the mentioned boards.

>
>> diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c
>> index 
>> 9b77f23566f6a35887d4c9aaefc61a971131b499..c5266532a097c06f33d12e345c695357d75abf42 
>> 100644
>> --- a/lib/vdso/gettimeofday.c
>> +++ b/lib/vdso/gettimeofday.c
>> @@ -82,8 +82,8 @@ const struct vdso_time_data 
>> *__arch_get_vdso_u_timens_data(const struct vdso_tim
>>   #endif /* CONFIG_GENERIC_VDSO_DATA_STORE */
>>     static __always_inline
>> -int do_hres_timens(const struct vdso_time_data *vdns, const struct 
>> vdso_clock *vcns,
>> -           clockid_t clk, struct __kernel_timespec *ts)
>> +bool do_hres_timens(const struct vdso_time_data *vdns, const struct 
>> vdso_clock *vcns,
>> +            clockid_t clk, struct __kernel_timespec *ts)
>>   {
>>       const struct vdso_time_data *vd = 
>> __arch_get_vdso_u_timens_data(vdns);
>>       const struct timens_offset *offs = &vcns->offset[clk];
>> @@ -103,11 +103,11 @@ int do_hres_timens(const struct vdso_time_data 
>> *vdns, const struct vdso_clock *v
>>           seq = vdso_read_begin(vc);
>>             if (unlikely(!vdso_clocksource_ok(vc)))
>> -            return -1;
>> +            return false;
>>             cycles = __arch_get_hw_counter(vc->clock_mode, vd);
>>           if (unlikely(!vdso_cycles_ok(cycles)))
>> -            return -1;
>> +            return false;
>>           ns = vdso_calc_ns(vc, cycles, vdso_ts->nsec);
>>           sec = vdso_ts->sec;
>>       } while (unlikely(vdso_read_retry(vc, seq)));
>> @@ -123,7 +123,7 @@ int do_hres_timens(const struct vdso_time_data 
>> *vdns, const struct vdso_clock *v
>>       ts->tv_sec = sec + __iter_div_u64_rem(ns, NSEC_PER_SEC, &ns);
>>       ts->tv_nsec = ns;
>>   -    return 0;
>> +    return true;
>>   }
>>   #else
>>   static __always_inline
>> @@ -133,16 +133,16 @@ const struct vdso_time_data 
>> *__arch_get_vdso_u_timens_data(const struct vdso_tim
>>   }
>>     static __always_inline
>> -int do_hres_timens(const struct vdso_time_data *vdns, const struct 
>> vdso_clock *vcns,
>> -           clockid_t clk, struct __kernel_timespec *ts)
>> +bool do_hres_timens(const struct vdso_time_data *vdns, const struct 
>> vdso_clock *vcns,
>> +            clockid_t clk, struct __kernel_timespec *ts)
>>   {
>> -    return -EINVAL;
>> +    return false;
>>   }
>>   #endif
>>     static __always_inline
>> -int do_hres(const struct vdso_time_data *vd, const struct vdso_clock 
>> *vc,
>> -        clockid_t clk, struct __kernel_timespec *ts)
>> +bool do_hres(const struct vdso_time_data *vd, const struct 
>> vdso_clock *vc,
>> +         clockid_t clk, struct __kernel_timespec *ts)
>>   {
>>       const struct vdso_timestamp *vdso_ts = &vc->basetime[clk];
>>       u64 cycles, sec, ns;
>> @@ -150,7 +150,7 @@ int do_hres(const struct vdso_time_data *vd, 
>> const struct vdso_clock *vc,
>>         /* Allows to compile the high resolution parts out */
>>       if (!__arch_vdso_hres_capable())
>> -        return -1;
>> +        return false;
>>         do {
>>           /*
>> @@ -173,11 +173,11 @@ int do_hres(const struct vdso_time_data *vd, 
>> const struct vdso_clock *vc,
>>           smp_rmb();
>>             if (unlikely(!vdso_clocksource_ok(vc)))
>> -            return -1;
>> +            return false;
>>             cycles = __arch_get_hw_counter(vc->clock_mode, vd);
>>           if (unlikely(!vdso_cycles_ok(cycles)))
>> -            return -1;
>> +            return false;
>>           ns = vdso_calc_ns(vc, cycles, vdso_ts->nsec);
>>           sec = vdso_ts->sec;
>>       } while (unlikely(vdso_read_retry(vc, seq)));
>> @@ -189,13 +189,13 @@ int do_hres(const struct vdso_time_data *vd, 
>> const struct vdso_clock *vc,
>>       ts->tv_sec = sec + __iter_div_u64_rem(ns, NSEC_PER_SEC, &ns);
>>       ts->tv_nsec = ns;
>>   -    return 0;
>> +    return true;
>>   }
>>     #ifdef CONFIG_TIME_NS
>>   static __always_inline
>> -int do_coarse_timens(const struct vdso_time_data *vdns, const struct 
>> vdso_clock *vcns,
>> -             clockid_t clk, struct __kernel_timespec *ts)
>> +bool do_coarse_timens(const struct vdso_time_data *vdns, const 
>> struct vdso_clock *vcns,
>> +              clockid_t clk, struct __kernel_timespec *ts)
>>   {
>>       const struct vdso_time_data *vd = 
>> __arch_get_vdso_u_timens_data(vdns);
>>       const struct timens_offset *offs = &vcns->offset[clk];
>> @@ -223,20 +223,20 @@ int do_coarse_timens(const struct 
>> vdso_time_data *vdns, const struct vdso_clock
>>        */
>>       ts->tv_sec = sec + __iter_div_u64_rem(nsec, NSEC_PER_SEC, &nsec);
>>       ts->tv_nsec = nsec;
>> -    return 0;
>> +    return true;
>>   }
>>   #else
>>   static __always_inline
>> -int do_coarse_timens(const struct vdso_time_data *vdns, const struct 
>> vdso_clock *vcns,
>> -             clockid_t clk, struct __kernel_timespec *ts)
>> +bool do_coarse_timens(const struct vdso_time_data *vdns, const 
>> struct vdso_clock *vcns,
>> +              clockid_t clk, struct __kernel_timespec *ts)
>>   {
>> -    return -1;
>> +    return false;
>>   }
>>   #endif
>>     static __always_inline
>> -int do_coarse(const struct vdso_time_data *vd, const struct 
>> vdso_clock *vc,
>> -          clockid_t clk, struct __kernel_timespec *ts)
>> +bool do_coarse(const struct vdso_time_data *vd, const struct 
>> vdso_clock *vc,
>> +           clockid_t clk, struct __kernel_timespec *ts)
>>   {
>>       const struct vdso_timestamp *vdso_ts = &vc->basetime[clk];
>>       u32 seq;
>> @@ -258,10 +258,10 @@ int do_coarse(const struct vdso_time_data *vd, 
>> const struct vdso_clock *vc,
>>           ts->tv_nsec = vdso_ts->nsec;
>>       } while (unlikely(vdso_read_retry(vc, seq)));
>>   -    return 0;
>> +    return true;
>>   }
>>   -static __always_inline int
>> +static __always_inline bool
>>   __cvdso_clock_gettime_common(const struct vdso_time_data *vd, 
>> clockid_t clock,
>>                    struct __kernel_timespec *ts)
>>   {
>> @@ -270,7 +270,7 @@ __cvdso_clock_gettime_common(const struct 
>> vdso_time_data *vd, clockid_t clock,
>>         /* Check for negative values or invalid clocks */
>>       if (unlikely((u32) clock >= MAX_CLOCKS))
>> -        return -1;
>> +        return false;
>>         /*
>>        * Convert the clockid to a bitmask and use it to check which
>> @@ -284,7 +284,7 @@ __cvdso_clock_gettime_common(const struct 
>> vdso_time_data *vd, clockid_t clock,
>>       else if (msk & VDSO_RAW)
>>           vc = &vc[CS_RAW];
>>       else
>> -        return -1;
>> +        return false;
>>         return do_hres(vd, vc, clock, ts);
>>   }
>> @@ -293,9 +293,11 @@ static __maybe_unused int
>>   __cvdso_clock_gettime_data(const struct vdso_time_data *vd, 
>> clockid_t clock,
>>                  struct __kernel_timespec *ts)
>>   {
>> -    int ret = __cvdso_clock_gettime_common(vd, clock, ts);
>> +    bool ok;
>>   -    if (unlikely(ret))
>> +    ok = __cvdso_clock_gettime_common(vd, clock, ts);
>> +
>> +    if (unlikely(!ok))
>>           return clock_gettime_fallback(clock, ts);
>>       return 0;
>>   }
>>
> Best regards

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland