Re: [Linuxptp-devel] [PATCH 3/3] Extend clockcheck to check for changes in frequency.
PTP IEEE 1588 stack for Linux
Brought to you by:
rcochran
From: Jacob K. <jac...@in...> - 2022-10-12 22:29:56
|
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 |