From 483030cf380388044853d36c17751ded1c29b4e4 Mon Sep 17 00:00:00 2001 From: Miroslav Lichvar Date: Thu, 5 Jan 2023 15:21:23 +0100 Subject: [PATCH] check for unexpected changes in frequency offset (#2150815) Resolves: #2150815 --- linuxptp-freqcheck.patch | 208 +++++++++++++++++++++++++++++++++++++++ linuxptp.spec | 3 + 2 files changed, 211 insertions(+) create mode 100644 linuxptp-freqcheck.patch diff --git a/linuxptp-freqcheck.patch b/linuxptp-freqcheck.patch new file mode 100644 index 0000000..28d1b93 --- /dev/null +++ b/linuxptp-freqcheck.patch @@ -0,0 +1,208 @@ +commit 513c7457865dba262a43e03fbe9178f4b08ba319 +Author: Miroslav Lichvar +Date: Mon Oct 24 15:25:30 2022 +0200 + + Drop support for old kernels returning zero frequency. + + Kernels before 3.10 had a bug in reading of the system 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. + + (Rebased to 3.1.1) + + Signed-off-by: Miroslav Lichvar + +diff --git a/clock.c b/clock.c +index f3df220..9079428 100644 +--- a/clock.c ++++ b/clock.c +@@ -1147,11 +1147,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. */ +- 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"); +@@ -1746,7 +1741,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 5f6fbaa..af948eb 100644 +--- a/phc2sys.c ++++ b/phc2sys.c +@@ -147,9 +147,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. */ +- clockadj_set_freq(clock->clkid, ppb); + if (clock->clkid == CLOCK_REALTIME) { + sysclk_set_leap(0); + max_ppb = sysclk_max_freq(); +diff --git a/ts2phc_slave.c b/ts2phc_slave.c +index 749efe5..5541d91 100644 +--- a/ts2phc_slave.c ++++ b/ts2phc_slave.c +@@ -183,10 +183,6 @@ static struct ts2phc_slave *ts2phc_slave_create(struct config *cfg, const char * + pr_debug("PHC slave %s has ptp index %d", device, junk); + + fadj = (int) clockadj_get_freq(slave->clk); +- /* 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. */ +- clockadj_set_freq(slave->clk, fadj); + + max_adj = phc_max_adj(slave->clk); + + +commit 80d6875b0072c7ea636a5631e590fa72d169c351 +Author: Miroslav Lichvar +Date: Mon Oct 24 15:25:31 2022 +0200 + + Don't accept errors in clockadj_get_freq(). + + Exit if an error is returned from the clock_adjtime() call in + clockadj_get_freq(). No recoverable errors are expected. + + Signed-off-by: Miroslav Lichvar + +diff --git a/clockadj.c b/clockadj.c +index 957dc57..4c920b9 100644 +--- a/clockadj.c ++++ b/clockadj.c +@@ -19,6 +19,7 @@ + + #include + #include ++#include + #include + #include + +@@ -72,6 +73,7 @@ double clockadj_get_freq(clockid_t clkid) + memset(&tx, 0, sizeof(tx)); + if (clock_adjtime(clkid, &tx) < 0) { + pr_err("failed to read out the clock frequency adjustment: %m"); ++ exit(1); + } else { + f = tx.freq / 65.536; + if (clkid == CLOCK_REALTIME && realtime_nominal_tick && tx.tick) + +commit 211cbfe810ef4082d1af8e9b50f65d4f6fd7246a +Author: Miroslav Lichvar +Date: Mon Oct 24 15:25:32 2022 +0200 + + 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 + +diff --git a/clock.c b/clock.c +index 9079428..eea7983 100644 +--- a/clock.c ++++ b/clock.c +@@ -1760,6 +1760,9 @@ int clock_switch_phc(struct clock *c, int phc_index) + + 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!"); ++ 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 af948eb..4daa1bf 100644 +--- a/phc2sys.c ++++ b/phc2sys.c +@@ -566,6 +566,9 @@ 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(); +diff --git a/ptp4l.8 b/ptp4l.8 +index 4917240..f760e2b 100644 +--- a/ptp4l.8 ++++ b/ptp4l.8 +@@ -605,8 +605,10 @@ 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 +-disabled. The default is 200000000 (20%). ++will be printed and the servo will be reset. If the frequency correction set by ++ptp4l changes unexpectedly between updates of the clock (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 + The initial path delay of the clock in nanoseconds used for synchronization of diff --git a/linuxptp.spec b/linuxptp.spec index b729cc9..3343443 100644 --- a/linuxptp.spec +++ b/linuxptp.spec @@ -49,6 +49,8 @@ Patch13: linuxptp-phcerr.patch Patch14: linuxptp-vlanbond.patch # handle EINTR when waiting for transmit timestamp Patch15: linuxptp-eintr.patch +# check for unexpected changes in frequency offset +Patch16: linuxptp-freqcheck.patch BuildRequires: gcc gcc-c++ make systemd @@ -77,6 +79,7 @@ Supporting legacy APIs and other platforms is not a goal. %patch13 -p1 -b .phcerr %patch14 -p1 -b .vlanbond %patch15 -p1 -b .eintr +%patch16 -p1 -b .freqcheck mv linuxptp-testsuite-%{testsuite_ver}* testsuite mv clknetsim-%{clknetsim_ver}* testsuite/clknetsim