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

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

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.


> 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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland