Re: [Linuxptp-devel] [PATCH 2/3] Drop support for old kernels returning zero frequency.

[Jacob K. <jac...@in...>]

On 10/5/2022 11:38 PM, Miroslav Lichvar wrote:
> Some ancient kernels had a bug in reading of the clock frequency, which
> was worked around by commit da347d7a36f2 ("ptp4l: Set clock frequency on
> start").
> 
> Drop this workaround and support for the old kernels to make
> clockadj_get_freq() useful.
> 

For the record I suspect this is related to Linux kernel commit
5c35bad5ffe5 ("ptp: provide the clock's adjusted frequency"). This
landed in v3.7, so its pretty old, and I think dropping support for it
is probably fine.

Interestingly, it looks like the kernel tried to return EOPNOTSUP. I
don't know why this would silently fail... Any recollection?

I guess it doesn't "silently" fail, the function logs an error with
fprintf, but it doesn't have any way to report the error back to the
caller, so the caller sees "0" with a log message, which is the "silent"
failure you mentioned.

Alternatively this could be fixed by refactoring clockadj_get_freq to
return its integer error code and put the value into an argument instead?

That would catch errors related to other failures such as access or
permission. This would also work properly at least on the versions of
the kernel I checked.

Thoughts?

> Signed-off-by: Miroslav Lichvar <mli...@re...>
> ---
>  clock.c   | 7 -------
>  phc2sys.c | 4 ----
>  ts2phc.c  | 7 -------
>  3 files changed, 18 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index d37bb87..46ac9c2 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1144,12 +1144,6 @@ struct clock *clock_create(enum clock_type type, struct config *config,
>  
>  	if (c->clkid != CLOCK_INVALID) {
>  		fadj = (int) clockadj_get_freq(c->clkid);
> -		/* Due to a bug in older kernels, the reading may silently fail
> -		   and return 0. Set the frequency back to make sure fadj is
> -		   the actual frequency of the clock. */
> -		if (!c->free_running) {
> -			clockadj_set_freq(c->clkid, fadj);
> -		}
>  		/* Disable write phase mode if not implemented by driver */
>  		if (c->write_phase_mode && !phc_has_writephase(c->clkid)) {
>  			pr_err("clock does not support write phase mode");
> @@ -1755,7 +1749,6 @@ int clock_switch_phc(struct clock *c, int phc_index)
>  		return -1;
>  	}
>  	fadj = (int) clockadj_get_freq(clkid);
> -	clockadj_set_freq(clkid, fadj);
>  	servo = servo_create(c->config, c->servo_type, -fadj, max_adj, 0);
>  	if (!servo) {
>  		pr_err("Switching PHC, failed to create clock servo");
> diff --git a/phc2sys.c b/phc2sys.c
> index 8d2624f..ebc43e5 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -128,10 +128,6 @@ static struct servo *servo_add(struct phc2sys_private *priv,
>  
>  	clockadj_init(clock->clkid);
>  	ppb = clockadj_get_freq(clock->clkid);
> -	/* The reading may silently fail and return 0, reset the frequency to
> -	   make sure ppb is the actual frequency of the clock. */
> -	if (!priv->free_running)
> -		clockadj_set_freq(clock->clkid, ppb);
>  	if (clock->clkid == CLOCK_REALTIME) {
>  		sysclk_set_leap(0);
>  		max_ppb = sysclk_max_freq();
> diff --git a/ts2phc.c b/ts2phc.c
> index f7a57e4..f345370 100644
> --- a/ts2phc.c
> +++ b/ts2phc.c
> @@ -129,13 +129,6 @@ static struct servo *ts2phc_servo_create(struct ts2phc_private *priv,
>  	int fadj, max_adj;
>  
>  	fadj = (int) clockadj_get_freq(clock->clkid);
> -	/* Due to a bug in older kernels, the reading may silently fail
> -	 * and return 0. Set the frequency back to make sure fadj is
> -	 * the actual frequency of the clock.
> -	 */
> -	if (!clock->no_adj) {
> -		clockadj_set_freq(clock->clkid, fadj);
> -	}
>  
>  	max_adj = phc_max_adj(clock->clkid);
>