[Linuxptp-devel] [PATCH v3 3/4] 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
---
clockadj.c | 2 ++
1 file changed, 2 insertions(+)
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(, 0, sizeof(tx));
if (clock_adjtime(clkid, ) < 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)
--
2.37.3
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v3 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
---
clock.c | 3 +++
clockcheck.c | 10 ++
clockcheck.h | 8
phc2sys.c| 3 +++
ptp4l.8 | 6 --
5 files changed, 28 insertions(+), 2 deletions(-)
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!");
+ 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..88ed00c 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -561,6 +561,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 1268802..cd6299f 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -619,8 +619,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 2 (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 2 (20%).
.TP
.B initial_delay
The initial path delay of the clock in nanoseconds used for synchronization of
--
2.37.3
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v3 0/4] Bug fix and improved clockcheck
v3: - improved documentation v2: - added patch to exit on errors returned by read-only clock_adjtime() - simplified last patch to reuse the set frequency and allow +/-1 ppb error due to conversions between int, double and scaled ppm - improved commit messages This set improves the clock check to consider the last frequency set by ptp4l/phc2sys in order to detect cases where multiple processes are trying to control the same clock due to a misconfiguration or bug. Miroslav Lichvar (4): phc2sys: Add clocks after processing configuration. Drop support for old kernels returning zero frequency. Don't accept errors in clockadj_get_freq(). Extend clockcheck to check for changes in frequency. clock.c | 10 +++--- clockadj.c | 2 ++ clockcheck.c | 10 ++ clockcheck.h | 8 phc2sys.8| 2 +- phc2sys.c| 48 ++-- ptp4l.8 | 4 +++- ts2phc.c | 7 --- 8 files changed, 53 insertions(+), 38 deletions(-) -- 2.37.3 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v3 1/4] phc2sys: Add clocks after processing configuration.
Clocks specified by the -c option, or the default CLOCK_REALTIME, were
added before the default or specified values (e.g. sanity_freq_limit)
were set in the private structure used by clock_add(), causing the
clocks to work with unexpected values.
Rework the code to save the names of sink clocks from command line and
add them only after the configuration is complete.
This also fixes the automatic mode to not add CLOCK_REALTIME twice.
Fixes: 33ac7d25cd92 ("phc2sys: Allow multiple sink clocks")
Signed-off-by: Miroslav Lichvar
---
phc2sys.8 | 2 +-
phc2sys.c | 41 +++--
2 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/phc2sys.8 b/phc2sys.8
index 6de2d25..9825ec7 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -120,7 +120,7 @@ Specify the time sink by device (e.g. /dev/ptp1) or
interface (e.g. eth1) or
by name. The default is CLOCK_REALTIME (the system clock). Not compatible
with the
.B \-a
-option. This option may be given multiple times.
+option. This option may be given up to 128 times.
.TP
.BI \-E " servo"
Specify which clock servo should be used. Valid values are pi for a PI
diff --git a/phc2sys.c b/phc2sys.c
index 2c8e905..8d2624f 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -64,6 +64,8 @@
#define PHC_PPS_OFFSET_LIMIT 1000
+#define MAX_DST_CLOCKS 128
+
struct clock {
LIST_ENTRY(clock) list;
LIST_ENTRY(clock) dst_list;
@@ -1056,9 +1058,10 @@ static void usage(char *progname)
int main(int argc, char *argv[])
{
- char *config = NULL, *last_dst_name = NULL, *progname, *src_name = NULL;
+ char *config = NULL, *progname, *src_name = NULL;
+ const char *dst_names[MAX_DST_CLOCKS];
char uds_local[MAX_IFNAME_SIZE + 1];
- int autocfg = 0, c, domain_number = 0, index, ntpshm_segment, offset;
+ int i, autocfg = 0, c, domain_number = 0, index, ntpshm_segment, offset;
int pps_fd = -1, print_level = LOG_INFO, r = -1, rt = 0;
int wait_sync = 0, dst_cnt = 0;
struct config *cfg;
@@ -1104,13 +1107,11 @@ int main(int argc, char *argv[])
rt++;
break;
case 'c':
- last_dst_name = optarg;
- r = phc2sys_static_dst_configuration(,
-last_dst_name);
- if (r) {
- goto end;
+ if (dst_cnt == MAX_DST_CLOCKS) {
+ fprintf(stderr, "too many sink clocks\n");
+ goto bad_usage;
}
- dst_cnt++;
+ dst_names[dst_cnt++] = optarg;
break;
case 'd':
pps_fd = open(optarg, O_RDONLY);
@@ -1261,7 +1262,11 @@ int main(int argc, char *argv[])
return c;
}
- if (autocfg && (src_name || last_dst_name || hardpps_configured(pps_fd)
||
+ if (!autocfg && dst_cnt == 0) {
+ dst_names[dst_cnt++] = "CLOCK_REALTIME";
+ }
+
+ if (autocfg && (src_name || dst_cnt > 0 || hardpps_configured(pps_fd) ||
wait_sync || priv.forced_sync_offset)) {
fprintf(stderr,
"autoconfiguration cannot be mixed with manual config
options.\n");
@@ -1279,15 +1284,8 @@ int main(int argc, char *argv[])
goto bad_usage;
}
- if (!last_dst_name) {
- last_dst_name = "CLOCK_REALTIME";
- r = phc2sys_static_dst_configuration(, last_dst_name);
- if (r) {
- goto end;
- }
- }
- if (hardpps_configured(pps_fd) && (dst_cnt > 1 ||
- strcmp(last_dst_name, "CLOCK_REALTIME"))) {
+ if (hardpps_configured(pps_fd) && (dst_cnt != 1 ||
+ strcmp(dst_names[0], "CLOCK_REALTIME"))) {
fprintf(stderr,
"cannot use a pps device unless destination is
CLOCK_REALTIME\n");
goto bad_usage;
@@ -1308,6 +1306,13 @@ int main(int argc, char *argv[])
priv.kernel_leap = config_get_int(cfg, NULL, "kernel_leap");
priv.sanity_freq_limit = config_get_int(cfg, NULL, "sanity_freq_limit");
+ for (i = 0; i < dst_cnt; i++) {
+ r = phc2sys_static_dst_configuration(, dst_names[i]);
+ if (r) {
+ goto end;
+ }
+ }
+
snprintf(uds_local, sizeof(uds_local), "/var/run/phc2sys.%d",
getpid());
--
2.37.3
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v3 2/4] 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.
Signed-off-by: Miroslav Lichvar
---
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);
--
2.37.3
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes in frequency.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Monday, October 24, 2022 5:43 AM
> To: Keller, Jacob E
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for
> changes in frequency.
>
> On Thu, Oct 20, 2022 at 04:50:37PM +, Keller, Jacob E wrote:
> > > 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?
>
> If I expand the description a bit, is it better?
>
> .B sanity_freq_limit
> 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. 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 2 (20%).
>
>
> If not, what would you suggest?
>
I like this better!
Thanks,
Jake
> Thanks,
>
> --
> Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes in frequency.
On Thu, Oct 20, 2022 at 04:50:37PM +, Keller, Jacob E wrote:
> > 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?
If I expand the description a bit, is it better?
.B sanity_freq_limit
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. 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 2 (20%).
If not, what would you suggest?
Thanks,
--
Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
