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.weisssc...@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
