Re: [Linuxptp-devel] [PATCH 3/3] Extend clockcheck to check for changes in frequency.

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

On 10/5/2022 11:38 PM, Miroslav Lichvar wrote:
> Before setting the new frequency offset on a clock update, compare the
> current frequency with the value saved from the previous update and
> print a warning message if they are different.
> 
> This should detect misconfigurations where multiple processes are trying
> to control the clock (e.g. another ptp4l/phc2sys instance or an NTP
> client), even when they don't step the clock.
> 

Nice improvement! I recently saw an issue where this checking would have
been helpful!

> Signed-off-by: Miroslav Lichvar <mli...@re...>
> ---
>  clock.c      | 10 ++++++++--
>  clockcheck.c |  9 +++++++++
>  clockcheck.h |  8 ++++++++
>  phc2sys.c    |  6 +++++-
>  ptp4l.8      |  4 +++-
>  5 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 46ac9c2..69e803b 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1776,12 +1776,17 @@ static void clock_step_window(struct clock *c)
>  
>  static void clock_synchronize_locked(struct clock *c, double adj)
>  {
> +	if (c->sanity_check) {
> +		clockcheck_freq(c->sanity_check,
> +				clockadj_get_freq(c->clkid));
> +	}
>  	clockadj_set_freq(c->clkid, -adj);
>  	if (c->clkid == CLOCK_REALTIME) {
>  		sysclk_set_sync();
>  	}
>  	if (c->sanity_check) {
> -		clockcheck_set_freq(c->sanity_check, -adj);
> +		clockcheck_set_freq(c->sanity_check,
> +				    clockadj_get_freq(c->clkid));

Hmm. Ok so now we use the value reported by clockadj_get_freq rather
than using our internal value.

Does every caller of clockcheck_set_freq and clockcheck_freq now always
use clockadj_get_freq? Could we pull that into the function?

It would simplify adjusting clockadj_get_freq refactoring to return its
error code.

>  	}
>  }
>  
> @@ -1838,7 +1843,8 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t ingress, tmv_t origin)
>  		clockadj_step(c->clkid, -tmv_to_nanoseconds(c->master_offset));
>  		c->ingress_ts = tmv_zero();
>  		if (c->sanity_check) {
> -			clockcheck_set_freq(c->sanity_check, -adj);
> +			clockcheck_set_freq(c->sanity_check,
> +					    clockadj_get_freq(c->clkid));
>  			clockcheck_step(c->sanity_check,
>  					-tmv_to_nanoseconds(c->master_offset));
>  		}
> diff --git a/clockcheck.c b/clockcheck.c
> index f0141be..c174e73 100644
> --- a/clockcheck.c
> +++ b/clockcheck.c
> @@ -123,6 +123,15 @@ void clockcheck_set_freq(struct clockcheck *cc, int freq)
>  	cc->freq_known = 1;
>  }
>  
> +int clockcheck_freq(struct clockcheck *cc, int freq)
> +{
> +	if (cc->freq_known && cc->current_freq != freq) {
> +		pr_warning("clockcheck: clock frequency changed unexpectedly!");
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  void clockcheck_step(struct clockcheck *cc, int64_t step)
>  {
>  	if (cc->last_ts)
> diff --git a/clockcheck.h b/clockcheck.h
> index 1ff86eb..4b09b98 100644
> --- a/clockcheck.h
> +++ b/clockcheck.h
> @@ -54,6 +54,14 @@ int clockcheck_sample(struct clockcheck *cc, uint64_t ts);
>   */
>  void clockcheck_set_freq(struct clockcheck *cc, int freq);
>  
> +/**
> + * Check whether the frequency correction did not change unexpectedly.
> + * @param cc   Pointer to a clock check obtained via @ref clockcheck_create().
> + * @param freq Current reading of the frequency correction in ppb.
> + * @return Zero if the frequency did not change, non-zero otherwise.
> + */
> +int clockcheck_freq(struct clockcheck *cc, int freq);
> +
>  /**
>   * Inform clock check that the clock was stepped.
>   * @param cc   Pointer to a clock check obtained via @ref clockcheck_create().
> diff --git a/phc2sys.c b/phc2sys.c
> index ebc43e5..afe33ec 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -561,11 +561,15 @@ static void update_clock(struct phc2sys_private *priv, struct clock *clock,
>  		/* Fall through. */
>  	case SERVO_LOCKED:
>  	case SERVO_LOCKED_STABLE:
> +		if (clock->sanity_check)
> +			clockcheck_freq(clock->sanity_check,
> +					clockadj_get_freq(clock->clkid));
>  		clockadj_set_freq(clock->clkid, -ppb);
>  		if (clock->clkid == CLOCK_REALTIME)
>  			sysclk_set_sync();
>  		if (clock->sanity_check)
> -			clockcheck_set_freq(clock->sanity_check, -ppb);
> +			clockcheck_set_freq(clock->sanity_check,
> +					    clockadj_get_freq(clock->clkid));
>  		break;
>  	}
>  
> diff --git a/ptp4l.8 b/ptp4l.8
> index 1268802..220d594 100644
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -619,7 +619,9 @@ This option used to be called
>  The maximum allowed frequency offset between uncorrected clock and the system
>  monotonic clock in parts per billion (ppb). This is used as a sanity check of
>  the synchronized clock. When a larger offset is measured, a warning message
> -will be printed and the servo will be reset. When set to 0, the sanity check is
> +will be printed and the servo will be reset. If the frequency correction set by
> +ptp4l changes unexpectedly (e.g. due to another process trying to control the
> +clock), a warning message will be printed. When set to 0, the sanity check is
>  disabled. The default is 200000000 (20%).
>  .TP
>  .B initial_delay