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

[Keller, J. E <jac...@in...>]

> -----Original Message-----
> From: Miroslav Lichvar <mli...@re...>
> Sent: Thursday, October 20, 2022 7:13 AM
> To: lin...@li...
> Subject: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes
> in frequency.
> 
> Before setting the new frequency offset on a clock update, compare the
> current frequency returned by the kernel with the value saved from the
> previous update. Print a warning message if the difference is larger
> than 1 ppb, allowing for rounding errors in conversion to and from
> double. The kernel caches the value set by clock_adjtime() in shifted
> ppm, it doesn't request it from the driver, which can have a lower
> resulution.
> 
> 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.
> 
> Signed-off-by: Miroslav Lichvar <mli...@re...>
> ---
>  clock.c      |  3 +++
>  clockcheck.c | 10 ++++++++++
>  clockcheck.h |  8 ++++++++
>  phc2sys.c    |  3 +++
>  ptp4l.8      |  4 +++-
>  5 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/clock.c b/clock.c
> index 46ac9c2..8177e77 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1776,6 +1776,9 @@ 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();
> diff --git a/clockcheck.c b/clockcheck.c
> index f0141be..b5a69cc 100644
> --- a/clockcheck.c
> +++ b/clockcheck.c
> @@ -123,6 +123,16 @@ void clockcheck_set_freq(struct clockcheck *cc, int freq)
>  	cc->freq_known = 1;
>  }
> 
> +int clockcheck_freq(struct clockcheck *cc, int freq)
> +{
> +	/* Allow difference of 1 ppb due to conversion to/from double */
> +	if (cc->freq_known && abs(cc->current_freq - freq) > 1) {
> +		pr_warning("clockcheck: clock frequency changed
> unexpectedly!");


The modified documentation would make me think this should allow up to the sanity_freq_limit as a difference? Perhaps the documentation should be improved somewhat to clarify the difference?

Thanks,
Jake