[Linuxptp-devel] [PATCH v3 1/2] phc2sys: move read_phc into clock_adj.c

2021-11-23 Thread Jacob Keller
The read_phc function implemented in phc2sys.c is used to perform clock
comparison between two arbitrary clocks using clock_gettime.

This support is used to allow phc2sys to work on any pair of clocks and
is implemented in a very similar manner as the kernel PTP_SYS_OFFSET
ioctls.

Make this function easier to re-use by moving it out of phc2sys.c and
into a more accessible location. clockadj.c seems like a reasonable
location as this file has many functions which deal with clockid_t
values, and this functionality is tangentially related to adjusting
clocks.

Moving this function will allow using it in the phc_ctl program in a
future change.

Notice that read_phc returned 0 on failure and 1 on success. This is
fairly non-standard, so lets update clockadj_compare to return 0 on
success and -1 on failure. Fix the call sites to check correctly and
report an error.

Signed-off-by: Jacob Keller 
---
 clockadj.c | 31 +++
 clockadj.h | 18 ++
 phc2sys.c  | 46 +-
 3 files changed, 58 insertions(+), 37 deletions(-)

diff --git a/clockadj.c b/clockadj.c
index b5c78cd112ff..e8c5789864db 100644
--- a/clockadj.c
+++ b/clockadj.c
@@ -139,6 +139,37 @@ int clockadj_max_freq(clockid_t clkid)
return f;
 }
 
+int clockadj_compare(clockid_t clkid, clockid_t sysclk, int readings,
+int64_t *offset, uint64_t *ts, int64_t *delay)
+{
+   struct timespec tdst1, tdst2, tsrc;
+   int i;
+   int64_t interval, best_interval = INT64_MAX;
+
+   /* Pick the quickest clkid reading. */
+   for (i = 0; i < readings; i++) {
+   if (clock_gettime(sysclk, ) ||
+   clock_gettime(clkid, ) ||
+   clock_gettime(sysclk, )) {
+   pr_err("failed to read clock: %m");
+   return -1;
+   }
+
+   interval = (tdst2.tv_sec - tdst1.tv_sec) * NS_PER_SEC +
+   tdst2.tv_nsec - tdst1.tv_nsec;
+
+   if (best_interval > interval) {
+   best_interval = interval;
+   *offset = (tdst1.tv_sec - tsrc.tv_sec) * NS_PER_SEC +
+   tdst1.tv_nsec - tsrc.tv_nsec + interval / 2;
+   *ts = tdst2.tv_sec * NS_PER_SEC + tdst2.tv_nsec;
+   }
+   }
+   *delay = best_interval;
+
+   return 0;
+}
+
 void sysclk_set_leap(int leap)
 {
clockid_t clkid = CLOCK_REALTIME;
diff --git a/clockadj.h b/clockadj.h
index 43325c8d5d15..b63ae3864d34 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -63,6 +63,24 @@ void clockadj_step(clockid_t clkid, int64_t step);
  */
 int clockadj_max_freq(clockid_t clkid);
 
+/**
+ * Compare offset between two clocks
+ * @param clkid A clock ID obtained using phc_open() or CLOCK_REALTIME
+ * @param sysclkA clock ID obtained using phc_open() or CLOCK_REALTIME
+ * @param readings  Number of readings to try
+ * @param offsetOn return, the nanoseconds offset between the clocks
+ * @param tsOn return, the time of sysclk in nanoseconds that was used
+ * @param delay On return, the interval between two reads of sysclk
+ * @return Zero on success and non-zero on failure.
+ *
+ * Compare the offset between two clocks in a similar manner as the
+ * PTP_SYS_OFFSET ioctls. Performs multiple reads of sysclk with a read of
+ * clkid between in order to calculate the time difference of sysclk minus
+ * clkid.
+ */
+int clockadj_compare(clockid_t clkid, clockid_t sysclk, int readings,
+int64_t *offset, uint64_t *ts, int64_t *delay);
+
 /**
  * Set the system clock to insert/delete leap second at midnight.
  * @param leap  +1 to insert leap second, -1 to delete leap second,
diff --git a/phc2sys.c b/phc2sys.c
index 6815c3dee8a0..489e15bc0488 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -470,37 +470,6 @@ static void reconfigure(struct phc2sys_private *priv)
pr_info("selecting %s as the master clock", src->device);
 }
 
-static int read_phc(clockid_t clkid, clockid_t sysclk, int readings,
-   int64_t *offset, uint64_t *ts, int64_t *delay)
-{
-   struct timespec tdst1, tdst2, tsrc;
-   int i;
-   int64_t interval, best_interval = INT64_MAX;
-
-   /* Pick the quickest clkid reading. */
-   for (i = 0; i < readings; i++) {
-   if (clock_gettime(sysclk, ) ||
-   clock_gettime(clkid, ) ||
-   clock_gettime(sysclk, )) {
-   pr_err("failed to read clock: %m");
-   return 0;
-   }
-
-   interval = (tdst2.tv_sec - tdst1.tv_sec) * NS_PER_SEC +
-   tdst2.tv_nsec - tdst1.tv_nsec;
-
-   if (best_interval > interval) {
-   best_interval = interval;
-   *offse

[Linuxptp-devel] [PATCH v3 0/2] fix fallback clock_gettime for test_phc

2021-11-23 Thread Jacob Keller
The current implementation of test_phc cmp has a fallback flow for comparing
the PHC clock to the CLOCK_REAMTIME. This fallback flow calculates the
inverse offset compared to the offsets calculated by the PTP_SYS_OFFSET
ioctls.

Fix this by replacing its implementation with the one from phc2sys. Move
read_phc function into clockadj.c to re-use it.

Changes since v2
* Fix doxygen parameters to use @param
* Add @return for clockadj_compare
* Reword description so its clear that it is sysclk minus clkid.

Changes since v1
* Make clockadj_compare return -1 on failure and 0 on success
* Fix callsites in phc2sys to handle this change

Jacob Keller (2):
  phc2sys: move read_phc into clock_adj.c
  phc_ctl: replace calculate_offset with clockadj_compare

 clockadj.c | 31 +++
 clockadj.h | 18 ++
 phc2sys.c  | 46 +-
 phc_ctl.c  | 54 --
 4 files changed, 74 insertions(+), 75 deletions(-)

-- 
2.34.0



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH v3 2/2] phc_ctl: replace calculate_offset with clockadj_compare

2021-11-23 Thread Jacob Keller
The phc_ctl program includes an implementation for comparing the PHC
with CLOCK_REALTIME when PTP_SYS_OFFSET ioctls are not supported.

This implementation produces inverted results when compared with the
implementation of the ioctl.

The PTP_SYS_OFFSET ioctls calculate the difference as "CLOCK_REALTIME -
PHC_TIME" while this function calculates "PHC_TIME - CLOCK_REALTIME".
This could produce confusing results if phc_ctl is used on a kernel
which lacks the PTP_SYS_OFFSET ioctls.

Fix this by replacing the bespoke implementation in test_phc with the
clockadj_compare function. That function mimics the interface of
sysoff_measure and correctly computes the time offset between the two
clocks in the same manner as the ioctls do. In addition, it supports
performing multiple samples to reduce inaccuracy.

With this change, the fallback implementation of measurement will match
the behavior of the ioctl based PTP_SYS_OFFSET flows.

Signed-off-by: Jacob Keller 
---
 phc_ctl.c | 54 --
 1 file changed, 16 insertions(+), 38 deletions(-)

diff --git a/phc_ctl.c b/phc_ctl.c
index 673cb37e3583..e975dd803578 100644
--- a/phc_ctl.c
+++ b/phc_ctl.c
@@ -90,26 +90,6 @@ static int install_handler(int signum, void(*handler)(int))
return 0;
 }
 
-static int64_t calculate_offset(struct timespec *ts1,
- struct timespec *rt,
- struct timespec *ts2)
-{
-   int64_t interval;
-   int64_t offset;
-
-#define NSEC_PER_SEC 10ULL
-   /* calculate interval between clock realtime */
-   interval = (ts2->tv_sec - ts1->tv_sec) * NSEC_PER_SEC;
-   interval += ts2->tv_nsec - ts1->tv_nsec;
-
-   /* assume PHC read occured half way between CLOCK_REALTIME reads */
-
-   offset = (rt->tv_sec - ts1->tv_sec) * NSEC_PER_SEC;
-   offset += (rt->tv_nsec - ts1->tv_nsec) - (interval / 2);
-
-   return offset;
-}
-
 static void usage(const char *progname)
 {
fprintf(stderr,
@@ -335,34 +315,32 @@ static int do_caps(clockid_t clkid, int cmdc, char 
*cmdv[])
 
 static int do_cmp(clockid_t clkid, int cmdc, char *cmdv[])
 {
-   struct timespec ts, rta, rtb;
-   int64_t sys_offset, delay = 0, offset;
+   int64_t sys_offset, delay;
uint64_t sys_ts;
-   int method;
+   int method, fd;
 
-   method = sysoff_probe(CLOCKID_TO_FD(clkid), 9);
+#define N_SAMPLES 9
 
-   if (method >= 0 && sysoff_measure(CLOCKID_TO_FD(clkid), method, 9,
+   fd = CLOCKID_TO_FD(clkid);
+
+   method = sysoff_probe(fd, N_SAMPLES);
+
+   if (method >= 0 && sysoff_measure(fd, method, N_SAMPLES,
  _offset, _ts, ) >= 0) {
-   pr_notice( "offset from CLOCK_REALTIME is %"PRId64"ns\n",
-   sys_offset);
+   pr_notice("offset from CLOCK_REALTIME is %"PRId64"ns\n",
+ sys_offset);
return 0;
}
 
-   memset(, 0, sizeof(ts));
-   memset(, 0, sizeof(rta));
-   memset(, 0, sizeof(rtb));
-   if (clock_gettime(CLOCK_REALTIME, ) ||
-   clock_gettime(clkid, ) ||
-   clock_gettime(CLOCK_REALTIME, )) {
-   pr_err("cmp: failed clock reads: %s\n",
-   strerror(errno));
+   if (clockadj_compare(clkid, CLOCK_REALTIME, N_SAMPLES,
+_offset, _ts, )) {
+   pr_err("cmp: failed to compare clocks: %s\n",
+  strerror(errno));
return -1;
}
 
-   offset = calculate_offset(, , );
-   pr_notice( "offset from CLOCK_REALTIME is approximately %"PRId64"ns\n",
-   offset);
+   pr_notice("offset from CLOCK_REALTIME is approximately %"PRId64"ns\n",
+ sys_offset);
 
return 0;
 }
-- 
2.34.0



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH v2 0/2] fix fallback clock_gettime for test_phc

2021-10-22 Thread Jacob Keller
The current implementation of test_phc cmp has a fallback flow for comparing
the PHC clock to the CLOCK_REAMTIME. This fallback flow calculates the
inverse offset compared to the offsets calculated by the PTP_SYS_OFFSET
ioctls.

Fix this by replacing its implementation with the one from phc2sys. Move
read_phc function into clockadj.c to re-use it.

Changes since v1
* Make clockadj_compare return -1 on failure and 0 on success
* Fix callsites in phc2sys to handle this change

Jacob Keller (2):
  phc2sys: move read_phc into clock_adj.c
  phc_ctl: replace calculate_offset with clockadj_compare

 clockadj.c | 31 +++
 clockadj.h | 16 
 phc2sys.c  | 46 +-
 phc_ctl.c  | 54 --
 4 files changed, 72 insertions(+), 75 deletions(-)


base-commit: 63592effe3f3ff616a19bfff3757fd3dbd95baf5
-- 
2.31.1.331.gb0c09ab8796f



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH v2 1/2] phc2sys: move read_phc into clock_adj.c

2021-10-22 Thread Jacob Keller
The read_phc function implemented in phc2sys.c is used to perform clock
comparison between two arbitrary clocks using clock_gettime.

This support is used to allow phc2sys to work on any pair of clocks and
is implemented in a very similar manner as the kernel PTP_SYS_OFFSET
ioctls.

Make this function easier to re-use by moving it out of phc2sys.c and
into a more accessible location. clockadj.c seems like a reasonable
location as this file has many functions which deal with clockid_t
values, and this functionality is tangentially related to adjusting
clocks.

Moving this function will allow using it in the phc_ctl program in a
future change.

Notice that read_phc returned 0 on failure and 1 on success. This is
fairly non-standard, so lets update clockadj_compare to return 0 on
success and -1 on failure. Fix the call sites to check correctly and
report an error.

Signed-off-by: Jacob Keller 
---
Changes since v1
* Make clockadj_compare return -1 on failure and 0 on success
* Fix callsites in phc2sys to handle this change

 clockadj.c | 31 +++
 clockadj.h | 16 
 phc2sys.c  | 46 +-
 3 files changed, 56 insertions(+), 37 deletions(-)

diff --git a/clockadj.c b/clockadj.c
index b5c78cd112ff..e8c5789864db 100644
--- a/clockadj.c
+++ b/clockadj.c
@@ -139,6 +139,37 @@ int clockadj_max_freq(clockid_t clkid)
return f;
 }
 
+int clockadj_compare(clockid_t clkid, clockid_t sysclk, int readings,
+int64_t *offset, uint64_t *ts, int64_t *delay)
+{
+   struct timespec tdst1, tdst2, tsrc;
+   int i;
+   int64_t interval, best_interval = INT64_MAX;
+
+   /* Pick the quickest clkid reading. */
+   for (i = 0; i < readings; i++) {
+   if (clock_gettime(sysclk, ) ||
+   clock_gettime(clkid, ) ||
+   clock_gettime(sysclk, )) {
+   pr_err("failed to read clock: %m");
+   return -1;
+   }
+
+   interval = (tdst2.tv_sec - tdst1.tv_sec) * NS_PER_SEC +
+   tdst2.tv_nsec - tdst1.tv_nsec;
+
+   if (best_interval > interval) {
+   best_interval = interval;
+   *offset = (tdst1.tv_sec - tsrc.tv_sec) * NS_PER_SEC +
+   tdst1.tv_nsec - tsrc.tv_nsec + interval / 2;
+   *ts = tdst2.tv_sec * NS_PER_SEC + tdst2.tv_nsec;
+   }
+   }
+   *delay = best_interval;
+
+   return 0;
+}
+
 void sysclk_set_leap(int leap)
 {
clockid_t clkid = CLOCK_REALTIME;
diff --git a/clockadj.h b/clockadj.h
index 43325c8d5d15..995b2af11894 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -63,6 +63,22 @@ void clockadj_step(clockid_t clkid, int64_t step);
  */
 int clockadj_max_freq(clockid_t clkid);
 
+/**
+ * Compare offset between two clocks
+ * @param clkid  A clock ID obtained using phc_open() or CLOCK_REALTIME
+ * @param sysclk A clock ID obtained using phc_open() or CLOCK_REALTIME
+ * @readings Number of readings to try
+ * @offset   On return, the nanoseconds offset between the clocks
+ * @ts   On return, the time of sysclk in nanoseconds that was used
+ * @delayOn return, the interval between two reads of sysclk
+ *
+ * Compare the offset between two clocks in a similar manner as the
+ * PTP_SYS_OFFSET ioctls. Returns the difference between sysclk and clkid by
+ * performing multiple reads of sysclk with a read of clkid between.
+ */
+int clockadj_compare(clockid_t clkid, clockid_t sysclk, int readings,
+int64_t *offset, uint64_t *ts, int64_t *delay);
+
 /**
  * Set the system clock to insert/delete leap second at midnight.
  * @param leap  +1 to insert leap second, -1 to delete leap second,
diff --git a/phc2sys.c b/phc2sys.c
index 6815c3dee8a0..489e15bc0488 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -470,37 +470,6 @@ static void reconfigure(struct phc2sys_private *priv)
pr_info("selecting %s as the master clock", src->device);
 }
 
-static int read_phc(clockid_t clkid, clockid_t sysclk, int readings,
-   int64_t *offset, uint64_t *ts, int64_t *delay)
-{
-   struct timespec tdst1, tdst2, tsrc;
-   int i;
-   int64_t interval, best_interval = INT64_MAX;
-
-   /* Pick the quickest clkid reading. */
-   for (i = 0; i < readings; i++) {
-   if (clock_gettime(sysclk, ) ||
-   clock_gettime(clkid, ) ||
-   clock_gettime(sysclk, )) {
-   pr_err("failed to read clock: %m");
-   return 0;
-   }
-
-   interval = (tdst2.tv_sec - tdst1.tv_sec) * NS_PER_SEC +
-   tdst2.tv_nsec - tdst1.tv_nsec;
-
-   if (best_interval > interval) {
-   best_interva

[Linuxptp-devel] [PATCH v2 2/2] phc_ctl: replace calculate_offset with clockadj_compare

2021-10-22 Thread Jacob Keller
The phc_ctl program includes an implementation for comparing the PHC
with CLOCK_REALTIME when PTP_SYS_OFFSET ioctls are not supported.

This implementation produces inverted results when compared with the
implementation of the ioctl.

The PTP_SYS_OFFSET ioctls calculate the difference as "CLOCK_REALTIME -
PHC_TIME" while this function calculates "PHC_TIME - CLOCK_REALTIME".
This could produce confusing results if phc_ctl is used on a kernel
which lacks the PTP_SYS_OFFSET ioctls.

Fix this by replacing the bespoke implementation in test_phc with the
clockadj_compare function. That function mimics the interface of
sysoff_measure and correctly computes the time offset between the two
clocks in the same manner as the ioctls do. In addition, it supports
performing multiple samples to reduce inaccuracy.

With this change, the fallback implementation of measurement will match
the behavior of the ioctl based PTP_SYS_OFFSET flows.

Signed-off-by: Jacob Keller 
---
 phc_ctl.c | 54 --
 1 file changed, 16 insertions(+), 38 deletions(-)

diff --git a/phc_ctl.c b/phc_ctl.c
index 673cb37e3583..e975dd803578 100644
--- a/phc_ctl.c
+++ b/phc_ctl.c
@@ -90,26 +90,6 @@ static int install_handler(int signum, void(*handler)(int))
return 0;
 }
 
-static int64_t calculate_offset(struct timespec *ts1,
- struct timespec *rt,
- struct timespec *ts2)
-{
-   int64_t interval;
-   int64_t offset;
-
-#define NSEC_PER_SEC 10ULL
-   /* calculate interval between clock realtime */
-   interval = (ts2->tv_sec - ts1->tv_sec) * NSEC_PER_SEC;
-   interval += ts2->tv_nsec - ts1->tv_nsec;
-
-   /* assume PHC read occured half way between CLOCK_REALTIME reads */
-
-   offset = (rt->tv_sec - ts1->tv_sec) * NSEC_PER_SEC;
-   offset += (rt->tv_nsec - ts1->tv_nsec) - (interval / 2);
-
-   return offset;
-}
-
 static void usage(const char *progname)
 {
fprintf(stderr,
@@ -335,34 +315,32 @@ static int do_caps(clockid_t clkid, int cmdc, char 
*cmdv[])
 
 static int do_cmp(clockid_t clkid, int cmdc, char *cmdv[])
 {
-   struct timespec ts, rta, rtb;
-   int64_t sys_offset, delay = 0, offset;
+   int64_t sys_offset, delay;
uint64_t sys_ts;
-   int method;
+   int method, fd;
 
-   method = sysoff_probe(CLOCKID_TO_FD(clkid), 9);
+#define N_SAMPLES 9
 
-   if (method >= 0 && sysoff_measure(CLOCKID_TO_FD(clkid), method, 9,
+   fd = CLOCKID_TO_FD(clkid);
+
+   method = sysoff_probe(fd, N_SAMPLES);
+
+   if (method >= 0 && sysoff_measure(fd, method, N_SAMPLES,
  _offset, _ts, ) >= 0) {
-   pr_notice( "offset from CLOCK_REALTIME is %"PRId64"ns\n",
-   sys_offset);
+   pr_notice("offset from CLOCK_REALTIME is %"PRId64"ns\n",
+ sys_offset);
return 0;
}
 
-   memset(, 0, sizeof(ts));
-   memset(, 0, sizeof(rta));
-   memset(, 0, sizeof(rtb));
-   if (clock_gettime(CLOCK_REALTIME, ) ||
-   clock_gettime(clkid, ) ||
-   clock_gettime(CLOCK_REALTIME, )) {
-   pr_err("cmp: failed clock reads: %s\n",
-   strerror(errno));
+   if (clockadj_compare(clkid, CLOCK_REALTIME, N_SAMPLES,
+_offset, _ts, )) {
+   pr_err("cmp: failed to compare clocks: %s\n",
+  strerror(errno));
return -1;
}
 
-   offset = calculate_offset(, , );
-   pr_notice( "offset from CLOCK_REALTIME is approximately %"PRId64"ns\n",
-   offset);
+   pr_notice("offset from CLOCK_REALTIME is approximately %"PRId64"ns\n",
+ sys_offset);
 
return 0;
 }
-- 
2.31.1.331.gb0c09ab8796f



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH 1/2] phc2sys: move read_phc into clock_adj.c

2021-10-22 Thread Jacob Keller
The read_phc function implemented in phc2sys.c is used to perform clock
comparison between two arbitrary clocks using clock_gettime.

This support is used to allow phc2sys to work on any pair of clocks and
is implemented in a very similar manner as the kernel PTP_SYS_OFFSET
ioctls.

Make this function easier to re-use by moving it out of phc2sys.c and
into a more accessible location. clockadj.c seems like a reasonable
location as this file has many functions which deal with clockid_t
values, and this functionality is tangentially related to adjusting
clocks.

Moving this function will allow using it in the phc_ctl program in a
future change.

Signed-off-by: Jacob Keller 
---
 clockadj.c | 31 +++
 clockadj.h | 16 
 phc2sys.c  | 44 
 3 files changed, 55 insertions(+), 36 deletions(-)

diff --git a/clockadj.c b/clockadj.c
index b5c78cd112ff..c78017049a0e 100644
--- a/clockadj.c
+++ b/clockadj.c
@@ -139,6 +139,37 @@ int clockadj_max_freq(clockid_t clkid)
return f;
 }
 
+int clockadj_compare(clockid_t clkid, clockid_t sysclk, int readings,
+int64_t *offset, uint64_t *ts, int64_t *delay)
+{
+   struct timespec tdst1, tdst2, tsrc;
+   int i;
+   int64_t interval, best_interval = INT64_MAX;
+
+   /* Pick the quickest clkid reading. */
+   for (i = 0; i < readings; i++) {
+   if (clock_gettime(sysclk, ) ||
+   clock_gettime(clkid, ) ||
+   clock_gettime(sysclk, )) {
+   pr_err("failed to read clock: %m");
+   return 0;
+   }
+
+   interval = (tdst2.tv_sec - tdst1.tv_sec) * NS_PER_SEC +
+   tdst2.tv_nsec - tdst1.tv_nsec;
+
+   if (best_interval > interval) {
+   best_interval = interval;
+   *offset = (tdst1.tv_sec - tsrc.tv_sec) * NS_PER_SEC +
+   tdst1.tv_nsec - tsrc.tv_nsec + interval / 2;
+   *ts = tdst2.tv_sec * NS_PER_SEC + tdst2.tv_nsec;
+   }
+   }
+   *delay = best_interval;
+
+   return 1;
+}
+
 void sysclk_set_leap(int leap)
 {
clockid_t clkid = CLOCK_REALTIME;
diff --git a/clockadj.h b/clockadj.h
index 43325c8d5d15..995b2af11894 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -63,6 +63,22 @@ void clockadj_step(clockid_t clkid, int64_t step);
  */
 int clockadj_max_freq(clockid_t clkid);
 
+/**
+ * Compare offset between two clocks
+ * @param clkid  A clock ID obtained using phc_open() or CLOCK_REALTIME
+ * @param sysclk A clock ID obtained using phc_open() or CLOCK_REALTIME
+ * @readings Number of readings to try
+ * @offset   On return, the nanoseconds offset between the clocks
+ * @ts   On return, the time of sysclk in nanoseconds that was used
+ * @delayOn return, the interval between two reads of sysclk
+ *
+ * Compare the offset between two clocks in a similar manner as the
+ * PTP_SYS_OFFSET ioctls. Returns the difference between sysclk and clkid by
+ * performing multiple reads of sysclk with a read of clkid between.
+ */
+int clockadj_compare(clockid_t clkid, clockid_t sysclk, int readings,
+int64_t *offset, uint64_t *ts, int64_t *delay);
+
 /**
  * Set the system clock to insert/delete leap second at midnight.
  * @param leap  +1 to insert leap second, -1 to delete leap second,
diff --git a/phc2sys.c b/phc2sys.c
index 6815c3dee8a0..5a14b0da8786 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -470,37 +470,6 @@ static void reconfigure(struct phc2sys_private *priv)
pr_info("selecting %s as the master clock", src->device);
 }
 
-static int read_phc(clockid_t clkid, clockid_t sysclk, int readings,
-   int64_t *offset, uint64_t *ts, int64_t *delay)
-{
-   struct timespec tdst1, tdst2, tsrc;
-   int i;
-   int64_t interval, best_interval = INT64_MAX;
-
-   /* Pick the quickest clkid reading. */
-   for (i = 0; i < readings; i++) {
-   if (clock_gettime(sysclk, ) ||
-   clock_gettime(clkid, ) ||
-   clock_gettime(sysclk, )) {
-   pr_err("failed to read clock: %m");
-   return 0;
-   }
-
-   interval = (tdst2.tv_sec - tdst1.tv_sec) * NS_PER_SEC +
-   tdst2.tv_nsec - tdst1.tv_nsec;
-
-   if (best_interval > interval) {
-   best_interval = interval;
-   *offset = (tdst1.tv_sec - tsrc.tv_sec) * NS_PER_SEC +
-   tdst1.tv_nsec - tsrc.tv_nsec + interval / 2;
-   *ts = tdst2.tv_sec * NS_PER_SEC + tdst2.tv_nsec;
-   }
-   }
-   *delay = best_interval;
-
-   return 1;
-}
-
 static int64_t get_sync_offset(st

[Linuxptp-devel] [PATCH 2/2] phc_ctl: replace calculate_offset with clockadj_compare

2021-10-22 Thread Jacob Keller
The phc_ctl program includes an implementation for comparing the PHC
with CLOCK_REALTIME when PTP_SYS_OFFSET ioctls are not supported.

This implementation produces inverted results when compared with the
implementation of the ioctl.

The PTP_SYS_OFFSET ioctls calculate the difference as "CLOCK_REALTIME -
PHC_TIME" while this function calculates "PHC_TIME - CLOCK_REALTIME".
This could produce confusing results if phc_ctl is used on a kernel
which lacks the PTP_SYS_OFFSET ioctls.

Fix this by replacing the bespoke implementation in test_phc with the
clockadj_compare function. That function mimics the interface of
sysoff_measure and correctly computes the time offset between the two
clocks in the same manner as the ioctls do. In addition, it supports
performing multiple samples to reduce inaccuracy.

With this change, the fallback implementation of measurement will match
the behavior of the ioctl based PTP_SYS_OFFSET flows.

Signed-off-by: Jacob Keller 
---
 phc_ctl.c | 54 --
 1 file changed, 16 insertions(+), 38 deletions(-)

diff --git a/phc_ctl.c b/phc_ctl.c
index 673cb37e3583..e975dd803578 100644
--- a/phc_ctl.c
+++ b/phc_ctl.c
@@ -90,26 +90,6 @@ static int install_handler(int signum, void(*handler)(int))
return 0;
 }
 
-static int64_t calculate_offset(struct timespec *ts1,
- struct timespec *rt,
- struct timespec *ts2)
-{
-   int64_t interval;
-   int64_t offset;
-
-#define NSEC_PER_SEC 10ULL
-   /* calculate interval between clock realtime */
-   interval = (ts2->tv_sec - ts1->tv_sec) * NSEC_PER_SEC;
-   interval += ts2->tv_nsec - ts1->tv_nsec;
-
-   /* assume PHC read occured half way between CLOCK_REALTIME reads */
-
-   offset = (rt->tv_sec - ts1->tv_sec) * NSEC_PER_SEC;
-   offset += (rt->tv_nsec - ts1->tv_nsec) - (interval / 2);
-
-   return offset;
-}
-
 static void usage(const char *progname)
 {
fprintf(stderr,
@@ -335,34 +315,32 @@ static int do_caps(clockid_t clkid, int cmdc, char 
*cmdv[])
 
 static int do_cmp(clockid_t clkid, int cmdc, char *cmdv[])
 {
-   struct timespec ts, rta, rtb;
-   int64_t sys_offset, delay = 0, offset;
+   int64_t sys_offset, delay;
uint64_t sys_ts;
-   int method;
+   int method, fd;
 
-   method = sysoff_probe(CLOCKID_TO_FD(clkid), 9);
+#define N_SAMPLES 9
 
-   if (method >= 0 && sysoff_measure(CLOCKID_TO_FD(clkid), method, 9,
+   fd = CLOCKID_TO_FD(clkid);
+
+   method = sysoff_probe(fd, N_SAMPLES);
+
+   if (method >= 0 && sysoff_measure(fd, method, N_SAMPLES,
  _offset, _ts, ) >= 0) {
-   pr_notice( "offset from CLOCK_REALTIME is %"PRId64"ns\n",
-   sys_offset);
+   pr_notice("offset from CLOCK_REALTIME is %"PRId64"ns\n",
+ sys_offset);
return 0;
}
 
-   memset(, 0, sizeof(ts));
-   memset(, 0, sizeof(rta));
-   memset(, 0, sizeof(rtb));
-   if (clock_gettime(CLOCK_REALTIME, ) ||
-   clock_gettime(clkid, ) ||
-   clock_gettime(CLOCK_REALTIME, )) {
-   pr_err("cmp: failed clock reads: %s\n",
-   strerror(errno));
+   if (clockadj_compare(clkid, CLOCK_REALTIME, N_SAMPLES,
+_offset, _ts, )) {
+   pr_err("cmp: failed to compare clocks: %s\n",
+  strerror(errno));
return -1;
}
 
-   offset = calculate_offset(, , );
-   pr_notice( "offset from CLOCK_REALTIME is approximately %"PRId64"ns\n",
-   offset);
+   pr_notice("offset from CLOCK_REALTIME is approximately %"PRId64"ns\n",
+ sys_offset);
 
return 0;
 }
-- 
2.31.1.331.gb0c09ab8796f



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH 0/2] fix fallback clock_gettime for test_phc

2021-10-22 Thread Jacob Keller
The current implementation of test_phc cmp has a fallback flow for comparing
the PHC clock to the CLOCK_REAMTIME. This fallback flow calculates the
inverse offset compared to the offsets calculated by the PTP_SYS_OFFSET
ioctls.

Fix this by replacing its implementation with the one from phc2sys. Move
read_phc function into clockadj.c to re-use it.

Jacob Keller (2):
  phc2sys: move read_phc into clock_adj.c
  phc_ctl: replace calculate_offset with clockadj_compare

 clockadj.c | 31 +++
 clockadj.h | 16 
 phc2sys.c  | 44 
 phc_ctl.c  | 54 --
 4 files changed, 71 insertions(+), 74 deletions(-)


base-commit: 63592effe3f3ff616a19bfff3757fd3dbd95baf5
-- 
2.31.1.331.gb0c09ab8796f



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH v2] Increase the default tx_timestamp_timeout to 10

2021-07-08 Thread Jacob Keller
The tx_timestamp_timeout configuration defines the number of
milliseconds to wait for a Tx timestamp from the kernel stack. This
delay is necessary as Tx timestamps are captured after a packet is sent
and reported back via the socket error queue.

The current default is to poll for up to 1 millisecond. In practice, it
turns out that this is not always enough time for hardware and software
to capture the timestamp and report it back. Some hardware designs
require reading timestamps over registers or other slow mechanisms.

This extra delay results in the timestamp not being sent back to
userspace within the default 1 millisecond polling time. If that occurs
the following can be seen from ptp4l:

  ptp4l[4756.840]: timed out while polling for tx timestamp
  ptp4l[4756.840]: increasing tx_timestamp_timeout may correct this issue,
   but it is likely caused by a driver bug
  ptp4l[4756.840]: port 1 (p2p1): send sync failed
  ptp4l[4756.840]: port 1 (p2p1): MASTER to FAULTY on FAULT_DETECTED
   (FT_UNSPECIFIED)

This can confuse users because it implies this is a bug, when the
correct solution in many cases is to just increase the timeout to
a slightly higher value.

Since we know this is a problem for many drivers and hardware designs,
lets increase the default timeout.

Note that a longer timeout should not affect setups which return the
timestamp quickly. On modern kernels, the poll() call will return once
the timestamp is reported back to the socket error queue. (On old
kernels around the 3.x era the poll will sleep for the full duration
before reporting the timestamp, but this is now quite an old kernel
release).

Signed-off-by: Jacob Keller 
---
 config.c| 2 +-
 configs/default.cfg | 2 +-
 ptp4l.8 | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 4472d3d9d6f9..d0f011c0e165 100644
--- a/config.c
+++ b/config.c
@@ -323,7 +323,7 @@ struct config_item config_tab[] = {
GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
GLOB_ITEM_INT("twoStepFlag", 1, 0, 1),
-   GLOB_ITEM_INT("tx_timestamp_timeout", 1, 1, INT_MAX),
+   GLOB_ITEM_INT("tx_timestamp_timeout", 10, 1, INT_MAX),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
PORT_ITEM_INT("udp6_scope", 0x0E, 0x00, 0x0F),
GLOB_ITEM_STR("uds_address", "/var/run/ptp4l"),
diff --git a/configs/default.cfg b/configs/default.cfg
index 64ef3bd7c81d..d61561072334 100644
--- a/configs/default.cfg
+++ b/configs/default.cfg
@@ -51,7 +51,7 @@ hybrid_e2e0
 inhibit_multicast_service  0
 net_sync_monitor   0
 tc_spanning_tree   0
-tx_timestamp_timeout   1
+tx_timestamp_timeout   10
 unicast_listen 0
 unicast_master_table   0
 unicast_req_duration   3600
diff --git a/ptp4l.8 b/ptp4l.8
index fe9e1502231c..7ca3474304e6 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -496,7 +496,7 @@ switches all implement this option together with the BMCA.
 .B tx_timestamp_timeout
 The number of milliseconds to poll waiting for the tx time stamp from the 
kernel
 when a message has recently been sent.
-The default is 1.
+The default is 10.
 .TP
 .B check_fup_sync
 Because of packet reordering that can occur in the network, in the
-- 
2.31.1.331.gb0c09ab8796f



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH] Increase the default tx_timestamp_timeout to 5

2021-07-07 Thread Jacob Keller
The tx_timestamp_timeout configuration defines the number of
milliseconds to wait for a Tx timestamp from the kernel stack. This
delay is necessary as Tx timestamps are captured after a packet is sent
and reported back via the socket error queue.

The current default is to poll for up to 1 millisecond. In practice, it
turns out that this is not always enough time for hardware and software
to capture the timestamp and report it back. Some hardware designs
require reading timestamps over registers or other slow mechanisms.

This extra delay results in the timestamp not being sent back to
userspace within the default 1 millisecond polling time. If that occurs
the following can be seen from ptp4l:

  ptp4l[4756.840]: timed out while polling for tx timestamp
  ptp4l[4756.840]: increasing tx_timestamp_timeout may correct this issue,
   but it is likely caused by a driver bug
  ptp4l[4756.840]: port 1 (p2p1): send sync failed
  ptp4l[4756.840]: port 1 (p2p1): MASTER to FAULTY on FAULT_DETECTED
   (FT_UNSPECIFIED)

This can confuse users because it implies this is a bug, when the
correct solution in many cases is to just increase the timeout to
a slightly higher value.

Since we know this is a problem for many drivers and hardware designs,
lets increase the default timeout. I chose 5 since it is a large enough
increase to avoid the issues on test systems I have. We do want to keep
this timeout small because it prevents ptp4l from doing any other
processing while we wait for the timestamp.

An alternative solution would be to refactor ptp4l so that it does not
stop and wait for a Tx timestamp, but instead handles the timestamps
asynchronously. While this could be done, it adds significant complexity
to the application with minimal or no gain. In most cases, hardware is
only capable of a single outstanding timestamp at a time, so we cannot
send another packet anyways until the first one has completed.

Signed-off-by: Jacob Keller 
---
 config.c| 2 +-
 configs/default.cfg | 2 +-
 ptp4l.8 | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 4472d3d9d6f9..f33f177c696a 100644
--- a/config.c
+++ b/config.c
@@ -323,7 +323,7 @@ struct config_item config_tab[] = {
GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
GLOB_ITEM_INT("twoStepFlag", 1, 0, 1),
-   GLOB_ITEM_INT("tx_timestamp_timeout", 1, 1, INT_MAX),
+   GLOB_ITEM_INT("tx_timestamp_timeout", 5, 1, INT_MAX),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
PORT_ITEM_INT("udp6_scope", 0x0E, 0x00, 0x0F),
GLOB_ITEM_STR("uds_address", "/var/run/ptp4l"),
diff --git a/configs/default.cfg b/configs/default.cfg
index 64ef3bd7c81d..5e9444da57ee 100644
--- a/configs/default.cfg
+++ b/configs/default.cfg
@@ -51,7 +51,7 @@ hybrid_e2e0
 inhibit_multicast_service  0
 net_sync_monitor   0
 tc_spanning_tree   0
-tx_timestamp_timeout   1
+tx_timestamp_timeout   5
 unicast_listen 0
 unicast_master_table   0
 unicast_req_duration   3600
diff --git a/ptp4l.8 b/ptp4l.8
index fe9e1502231c..024fad3d19b2 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -496,7 +496,7 @@ switches all implement this option together with the BMCA.
 .B tx_timestamp_timeout
 The number of milliseconds to poll waiting for the tx time stamp from the 
kernel
 when a message has recently been sent.
-The default is 1.
+The default is 5.
 .TP
 .B check_fup_sync
 Because of packet reordering that can occur in the network, in the
-- 
2.31.1.331.gb0c09ab8796f



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 2/2] Add setting of PTP_TIMESCALE flag

2021-06-14 Thread Jacob Keller



On 6/14/2021 5:03 AM, Miklas, Marcin via Linuxptp-devel wrote:
> From: Marcin Miklas 
> 
> In gPTP PTP_TIMESCALE flag should be set to 1. It looks like the flags
> where not properly set for any of messages used in gPTP.
> 
> Some of the automotive gPTP bridges where rejecting the PDelayReq messages
> because of missing flag.
> 
> I would say that both linuxptp and the bridge doesn't conform to gPTP
> specification where it is said that the PTP_TIMESCALE should be set to 1
> and ignored on reception.
> 

If I recall, this has been discussed on the list multiple times before.

https://sourceforge.net/p/linuxptp/mailman/message/34977023/

According to the normal 1588 standard, the bit is not supposed to be set
for these messages.

Perhaps an optional configuration to set these would be acceptable
though. (Then the gPTP config file could use that flag to make sure it
is set)

> Signed-off-by: Marcin Miklas 
> ---
>  port.c   | 5 +
>  port_signaling.c | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/port.c b/port.c
> index 8175f28..f906a6c 100644
> --- a/port.c
> +++ b/port.c
> @@ -1362,6 +1362,7 @@ static int port_pdelay_request(struct port *p)
>   msg->header.ver= PTP_VERSION;
>   msg->header.messageLength  = sizeof(struct pdelay_req_msg);
>   msg->header.domainNumber   = clock_domain_number(p->clock);
> + msg->header.flagField[1]   = clock_time_properties(p->clock).flags;
>   msg->header.correction = -p->asymmetry;
>   msg->header.sourcePortIdentity = p->portIdentity;
>   msg->header.sequenceId = p->seqnum.delayreq++;
> @@ -1556,6 +1557,7 @@ int port_tx_sync(struct port *p, struct address *dst)
>   msg->header.ver= PTP_VERSION;
>   msg->header.messageLength  = sizeof(struct sync_msg);
>   msg->header.domainNumber   = clock_domain_number(p->clock);
> + msg->header.flagField[1]   = clock_time_properties(p->clock).flags;
>   msg->header.sourcePortIdentity = p->portIdentity;
>   msg->header.sequenceId = p->seqnum.sync++;
>   msg->header.control= CTL_SYNC;
> @@ -1592,6 +1594,7 @@ int port_tx_sync(struct port *p, struct address *dst)
>   fup->header.ver= PTP_VERSION;
>   fup->header.messageLength  = sizeof(struct follow_up_msg);
>   fup->header.domainNumber   = clock_domain_number(p->clock);
> + fup->header.flagField[1]   = clock_time_properties(p->clock).flags;
>   fup->header.sourcePortIdentity = p->portIdentity;
>   fup->header.sequenceId = p->seqnum.sync - 1;
>   fup->header.control= CTL_FOLLOW_UP;
> @@ -2123,6 +2126,7 @@ int process_pdelay_req(struct port *p, struct 
> ptp_message *m)
>   rsp->header.ver= PTP_VERSION;
>   rsp->header.messageLength  = sizeof(struct pdelay_resp_msg);
>   rsp->header.domainNumber   = m->header.domainNumber;
> + rsp->header.flagField[1]   = clock_time_properties(p->clock).flags;
>   rsp->header.sourcePortIdentity = p->portIdentity;
>   rsp->header.sequenceId = m->header.sequenceId;
>   rsp->header.control= CTL_OTHER;
> @@ -2170,6 +2174,7 @@ int process_pdelay_req(struct port *p, struct 
> ptp_message *m)
>   fup->header.ver= PTP_VERSION;
>   fup->header.messageLength  = sizeof(struct pdelay_resp_fup_msg);
>   fup->header.domainNumber   = m->header.domainNumber;
> + fup->header.flagField[1]   = clock_time_properties(p->clock).flags;
>   fup->header.correction = m->header.correction;
>   fup->header.sourcePortIdentity = p->portIdentity;
>   fup->header.sequenceId = m->header.sequenceId;
> diff --git a/port_signaling.c b/port_signaling.c
> index ed217c0..c76bfdf 100644
> --- a/port_signaling.c
> +++ b/port_signaling.c
> @@ -44,6 +44,7 @@ struct ptp_message *port_signaling_construct(struct port *p,
>   msg->header.ver= PTP_VERSION;
>   msg->header.messageLength  = sizeof(struct signaling_msg);
>   msg->header.domainNumber   = clock_domain_number(p->clock);
> + msg->header.flagField[1]   = clock_time_properties(p->clock).flags;
>   msg->header.sourcePortIdentity = p->portIdentity;
>   msg->header.sequenceId = p->seqnum.signaling++;
>   msg->header.control= CTL_OTHER;
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] Recover a faulty master port

2021-06-09 Thread Jacob Keller



On 6/7/2021 1:19 PM, YENDstudio wrote:
> Hello,
> 
> I have configure one of my machines as a unicast BC which is
> synchronized to the grandmaster clock via the first of it's two ports.
> The second port is used to provide sync to another local machine. This
> setup works for a few hours after which one of the ports (master port)
> is marked as faulty, and it never recovers (the second machine stops
> receiving sync) until I restart the ptp4l application. Yet, the first
> port continues sync'ing with the grandmaster clock.
> > The fault is triggered by a timeout during polling of tx timestamp
> (sk_receive function call). As I am not able to fix this issue, I would
> like to at least make the ptp application recover the port
> automatically. I had tried to close-then-open the port when it goes to a
> FAULTY state but it didn't help (the slave machine is not able to sync).
> 

Hi,

ptp4l already attempts recovery from a fault after the fault reset
timeout. This is something like 15 seconds by default.

You should see it recover, something like:


> ptp4l[1022068.490]: selected /dev/ptp2 as PTP clock
> ptp4l[1022068.510]: port 1 (enp244s0f0): INITIALIZING to LISTENING on 
> INIT_COMPLETE
> ptp4l[1022068.510]: port 0 (/var/run/ptp4l): INITIALIZING to LISTENING on 
> INIT_COMPLETE
> ptp4l[1022068.510]: port 0 (/var/run/ptp4lro): INITIALIZING to LISTENING on 
> INIT_COMPLETE
> ptp4l[1022070.454]: port 1 (enp244s0f0): new foreign master 
> 527b94.fffe.96b1f3-1
> ptp4l[1022074.454]: selected best master clock 527b94.fffe.96b1f3
> ptp4l[1022074.454]: port 1 (enp244s0f0): LISTENING to UNCALIBRATED on RS_SLAVE
> ptp4l[1022076.454]: master offset 3148999551 s0 freq  +0 path delay  
> 1466
> ptp4l[1022077.482]: master offset 3149000658 s1 freq   +1107 path delay  
> 1615
> ptp4l[1022078.029]: timed out while polling for tx timestamp
> ptp4l[1022078.029]: increasing tx_timestamp_timeout may correct this issue, 
> but it is likely caused by a driver bug
> ptp4l[1022078.029]: port 1 (enp244s0f0): send delay request failed
> ptp4l[1022078.029]: port 1 (enp244s0f0): UNCALIBRATED to FAULTY on 
> FAULT_DETECTED (FT_UNSPECIFIED)
> ptp4l[1022082.057]: port 1 (enp244s0f0): FAULTY to LISTENING on INIT_COMPLETE

^^^
Specifically here.

> ptp4l[1022082.455]: port 1 (enp244s0f0): new foreign master 
> 527b94.fffe.96b1f3-1
> ptp4l[1022086.455]: selected best master clock 527b94.fffe.96b1f3
> ptp4l[1022086.455]: port 1 (enp244s0f0): LISTENING to UNCALIBRATED on RS_SLAVE
> ptp4l[1022087.460]: master offset   -7124120 s2 freq -7123013 path delay  
> 1615
> ptp4l[1022087.460]: port 1 (enp244s0f0): UNCALIBRATED to SLAVE on 
> MASTER_CLOCK_SELECTED
> ptp4l[1022088.460]: master offset -39903 s2 freq -2176032 path delay  
> 1615
> ptp4l[1022089.460]: master offset2165416 s2 freq  +17316 path delay  
> 1466
> ptp4l[1022090.460]: master offset2161742 s2 freq +663267 path delay  
> 1615
> ptp4l[1022091.460]: master offset1503260 s2 freq +653307 path delay  
> 1615
> ptp4l[1022092.460]: master offset 850970 s2 freq +451995 path delay  
> 1764
> ptp4l[1022093.460]: master offset 398679 s2 freq +254995 path delay  
> 2160
> ptp4l[1022094.460]: master offset 143441 s2 freq +119361 path delay  
> 2556
> ptp4l[1022095.460]: master offset   2567 s2 freq  +21519 path delay 
> 24523


If you're seeing that but it fails to actually recover, (i.e.e
timestamps never begin working again), this is likely a fault of the
driver or hardware for the device.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [Linuxptp-users] phc2sys jump into huge value

2021-04-29 Thread Jacob Keller


On 4/29/2021 8:04 AM, ramesh t wrote:
> hello Jake,
> 
> Did time profiling using time ticks. 
> Under the problem condition, observing clock_gettime of interface
> connected to BC is taking more time ticks. This results in phc offset
> jumping to 4 digit value momentarily. Also i'm not sure if reading
> across numa is triggering this issue.
> 

Not sure I follow here. You mean BC as in the boundary clock you're
connected to?

> Please suggest a way to fix in the phc2sys offset code?
> 1) Should we prevent update on phc2sys value if there is momentarily jump?
> 

You could possibly filter or average the delay to remove outliers, but I
am not sure how this impacts things. Other experts on this list are
probably better to answer that.

To me, it sounds like the issue is that in some cases the DELAY_REQ
packet is actually taking longer in one case.

You might also try peer delay instead if your network setup supports it?

> Please suggest.
> 
> Thanks for your support on this issue.
> 
> Regards,
> Ramesh 
> On Thursday, April 29, 2021, 07:33:49 AM GMT+5:30, ramesh t via
> Linuxptp-users  wrote:


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 0/1] Add master only management TLV

2021-04-22 Thread Jacob Keller


On 4/22/2021 8:46 AM, Luigi 'Comio' Mantellini wrote:
> Generally speaking and in my opinion should be interesting to have the
> following features:
>  - asynchronous clock adjust: I2c is a slow bus with unpredictable
> access time, especially when you have a lot of devices. this is a true
> story.
>  - a thread for each port that handles the ptp protocol and the
> timeouts: this will make more robust the stack on issues depending on TX
> timestamping.
>  - a capability to use transport/interfaces different from linux
> interfaces, loading a custom  .so file. i.e. timestamper send back the
> TS using in band communication.
>  - ability to add/remove ports on the fly without traffic hit.
>  - ability to change all port configurable parameters on the fly.
> 
> My wishlist is from customer requirements, of course.
> 
Most of these do sound like great features to have, but many would
require significant architecture changes. As LinuxPTP is an open source
project, you (or your customers) are free to work on such improvements
and contribute them back.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] SyncE support

2021-03-30 Thread Jacob Keller



On 3/22/2021 8:40 AM, Miroslav Lichvar wrote:
> FWIW, some onboard NICs supported by the e1000e driver can
> "cross-timestamp" using the Always Running Timer (ART), which should
> avoid the asymmetry of PCIe. I have not seen any detailed description
> of how it actually works.
>

I'm not sure in the e1000e case but in general this is done via PCIe
Precision Time Measurement extension which allows an end-point device
that supports the function to setup a transaction that captures the
device clock and ART value near-simultaneously.

I think the e1000e support might be a variation of this which pre-dated
the PTM standard and works when the device is a LAN-on-motherboard.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 2/2] Clock Class Threshold Feature addition for PTP4L

2021-02-16 Thread Jacob Keller



On 2/14/2021 9:59 PM, Karthikkumar V wrote:
> Implemented code review comments.
> This code changes brings in the ability to program the acceptable
> clockClass threshold beyond which device will move to holdover/free-run.
> Default clockClass threshold is 248.
> Example Use-Case
> This is needed in the cases where T-SC/T-BC Slave might want to listen
> only on PRC clockCLass and anything beyond that might not be acceptible
> and would want to go to holdover (with SyncE backup or internal
> oscillator).
> 
> Signed-off-by: Karthikkumar V 
> Signed-off-by: Ramana Reddy 
> ---
>  clock.c | 4 ++--
>  config.c| 2 +-
>  configs/default.cfg | 2 +-
>  port.c  | 7 +--
>  ptp4l.8 | 5 +
>  5 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 542f409..c40476f 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -969,7 +969,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   c->default_dataset.localPriority =
>   config_get_int(config, NULL, "G.8275.defaultDS.localPriority");
>   c->max_steps_removed = config_get_int(config, NULL,"maxStepsRemoved");
> - c->clock_class_threshold = config_get_int(config, NULL, 
> "clockClassThreshold");
> + c->clock_class_threshold = config_get_int(config, NULL, 
> "clock_class_threshold");
>  


Why this change? It seems weird to remove this variable and introduce
another? clockClassThreshold is supposed to be the standard isn't it?

>   /* Harmonize the twoStepFlag with the time_stamping option. */
>   if (config_harmonize_onestep(config)) {
> @@ -1660,7 +1660,7 @@ UInteger8 clock_max_steps_removed(struct clock *c)
>  
>  UInteger8 clock_get_clock_class_threshold(struct clock *c)
>  {
> - if(c != NULL){
> + if (c != NULL) {
>   return c->clock_class_threshold;
>   }
>   return CLOCK_CLASS_THRESHOLD_DEFAULT; /* Return Default Value  */
> diff --git a/config.c b/config.c
> index 41735d3..c840d98 100644
> --- a/config.c
> +++ b/config.c
> @@ -231,6 +231,7 @@ struct config_item config_tab[] = {
>   GLOB_ITEM_INT("clockAccuracy", 0xfe, 0, UINT8_MAX),
>   GLOB_ITEM_INT("clockClass", 248, 0, UINT8_MAX),
>   GLOB_ITEM_STR("clockIdentity", "00..00"),
> + GLOB_ITEM_INT("clock_class_threshold", CLOCK_CLASS_THRESHOLD_DEFAULT, 
> 6, CLOCK_CLASS_THRESHOLD_DEFAULT),
>   GLOB_ITEM_ENU("clock_servo", CLOCK_SERVO_PI, clock_servo_enu),
>   GLOB_ITEM_ENU("clock_type", CLOCK_TYPE_ORDINARY, clock_type_enu),
>   GLOB_ITEM_ENU("dataset_comparison", DS_CMP_IEEE1588, dataset_comp_enu),
> @@ -332,7 +333,6 @@ struct config_item config_tab[] = {
>   GLOB_ITEM_INT("utc_offset", CURRENT_UTC_OFFSET, 0, INT_MAX),
>   GLOB_ITEM_INT("verbose", 0, 0, 1),
>   GLOB_ITEM_INT("write_phase_mode", 0, 0, 1),
> - GLOB_ITEM_INT("clockClassThreshold", CLOCK_CLASS_THRESHOLD_DEFAULT, 6, 
> CLOCK_CLASS_THRESHOLD_DEFAULT),
>  };
>  
>  static struct unicast_master_table *current_uc_mtab;
> diff --git a/configs/default.cfg b/configs/default.cfg
> index 473bbb9..e863239 100644
> --- a/configs/default.cfg
> +++ b/configs/default.cfg
> @@ -29,7 +29,7 @@ logMinDelayReqInterval  0
>  logMinPdelayReqInterval  0
>  operLogPdelayReqInterval 0
>  announceReceiptTimeout   3
> -clockClassThreshold  248
> +clock_class_threshold248
>  syncReceiptTimeout   0
>  delayAsymmetry   0
>  fault_reset_interval 4
> diff --git a/port.c b/port.c
> index ae2a00e..024370f 100644
> --- a/port.c
> +++ b/port.c
> @@ -1861,8 +1861,11 @@ int process_announce(struct port *p, struct 
> ptp_message *m)
>   return result;
>   }
>  
> - /* If the clock class is greater than clock_class_threshold , ignore 
> this master */
> - if(m->announce.grandmasterClockQuality.clockClass > 
> clock_get_clock_class_threshold(p->clock)){
> + if (m->announce.grandmasterClockQuality.clockClass >
> + clock_get_clock_class_threshold(p->clock)) {
> + pl_err(60, "%s: Master clock quality received is "
> +  "greater than configured, ignoring master!",
> +  p->log_name);
>   return result;
>   }
>  
> diff --git a/ptp4l.8 b/ptp4l.8
> index 260aae3..b13471b 100644
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -455,6 +455,11 @@ message is greater than or equal to the value of 
> maxStepsRemoved the
>  Announce message is not considered in the operation of the BMCA.
>  The default value is 255.
>  .TP
> +.B clock_class_threshold
> +The maximum clock class value from master, acceptible to sub-ordinate
> +clock beyond which it moves out of lock state.
> +The default value is 248.
> +.TP
>  
>  .B domainNumber
>  The domain attribute of the local clock.
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 1/1] clock: Introduce step_window to free run x seconds after a clock step.

2021-02-01 Thread Jacob Keller



On 2/1/2021 2:55 PM, vincent.cheng...@renesas.com wrote:
> From: Vincent Cheng 
> 
> When clock stepping is unable to happen instantaneously the subsequent
> timestamps after a clock step does not reflect the step result and
> undesired clock freq and step adjustments will occur.
> 
> When using ts2phc to synchronize timestamping clock using external
> 1 PPS, it could take up to 1 second for the timestamps to reflect the
> clock step.
> 
> step_window, when set, indicates the time in seconds after a clock
> step in which the clock servo will not do any frequency or step
> adjustments.


This seems like it could be useful in a number of circumstances!

> 
> Signed-off-by: Vincent Cheng 
> ---
>  clock.c  | 40 
>  config.c |  1 +
>  ptp4l.8  |  8 
>  3 files changed, 49 insertions(+)
> 
> diff --git a/clock.c b/clock.c
> index a34737a..c03d548 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -132,6 +133,8 @@ struct clock {
>   struct interface *udsif;
>   LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
>   struct monitor *slave_event_monitor;
> + int step_window;
> + timer_t step_timer_id;
>  };
>  
>  struct clock the_clock;
> @@ -141,6 +144,13 @@ static int clock_resize_pollfd(struct clock *c, int 
> new_nports);
>  static void clock_remove_port(struct clock *c, struct port *p);
>  static void clock_stats_display(struct clock_stats *s);
>  
> +static void clock_clear_free_running(union sigval callback_data)
> +{
> + struct clock *c = (struct clock *)(callback_data.sival_ptr);
> +
> + c->free_running  = 0;
> +}
> +

I'm not sure if we really want to overload free_running, but I can see
how this makes some sense.

>  static void remove_subscriber(struct clock_subscriber *s)
>  {
>   LIST_REMOVE(s, list);
> @@ -286,6 +296,7 @@ void clock_destroy(struct clock *c)
>   if (c->sanity_check) {
>   clockcheck_destroy(c->sanity_check);
>   }
> + timer_delete(c->step_timer_id);
>   memset(c, 0, sizeof(*c));
>   msg_cleanup();
>   tc_cleanup();
> @@ -897,6 +908,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   unsigned char oui[OUI_LEN];
>   struct interface *iface;
>   struct timespec ts;
> + struct sigevent se;
>   int sfl;
>  
>   clock_gettime(CLOCK_REALTIME, );
> @@ -1194,6 +1206,18 @@ struct clock *clock_create(enum clock_type type, 
> struct config *config,
>   return NULL;
>   }
>  
> + /* Set up timer expire notification mechanism */
> + se.sigev_notify = SIGEV_THREAD;
> + se.sigev_value.sival_ptr = (void *)c;
> + se.sigev_notify_function = clock_clear_free_running;
> + se.sigev_notify_attributes = NULL;
> +
> + if (timer_create(CLOCK_MONOTONIC, , >step_timer_id)) {
> + pr_err("failed to create step timer_id");
> + return NULL;
> + }
> + c->step_window = config_get_int(config, NULL, "step_window");
> +
>   /* Create the ports. */
>   STAILQ_FOREACH(iface, >interfaces, list) {
>   if (clock_add_port(c, phc_device, phc_index, timestamping, 
> iface)) {
> @@ -1696,6 +1720,21 @@ int clock_switch_phc(struct clock *c, int phc_index)
>   return 0;
>  }
>  
> +static void clock_step_window(struct clock *c)
> +{
> + struct itimerspec tmo = {
> + {0, 0}, {0, 0}
> + };
> +
> + if (!c->step_window)
> + return;
> +
> + c->free_running = 1;
> +
> + tmo.it_value.tv_sec = c->step_window;
> + timer_settime(c->step_timer_id, 0, , 0);
> +}
> +
>  static void clock_synchronize_locked(struct clock *c, double adj)
>  {
>   clockadj_set_freq(c->clkid, -adj);
> @@ -1748,6 +1787,7 @@ enum servo_state clock_synchronize(struct clock *c, 
> tmv_t ingress, tmv_t origin)
>   case SERVO_JUMP:
>   clockadj_set_freq(c->clkid, -adj);
>   clockadj_step(c->clkid, -tmv_to_nanoseconds(c->master_offset));
> + clock_step_window(c);
>   c->ingress_ts = tmv_zero();
>   if (c->sanity_check) {
>   clockcheck_set_freq(c->sanity_check, -adj);
> diff --git a/config.c b/config.c
> index 4095b33..2b74bc8 100644
> --- a/config.c
> +++ b/config.c
> @@ -303,6 +303,7 @@ struct config_item config_tab[] = {
>   GLOB_ITEM_INT("slaveOnly", 0, 0, 1), /*deprecated*/
>   GLOB_ITEM_INT("socket_priority", 0, 0, 15),
>   GLOB_ITEM_DBL("step_threshold", 0.0, 0.0, DBL_MAX),
> + GLOB_ITEM_INT("step_window", 0, 0, UINT8_MAX),
>   GLOB_ITEM_INT("summary_interval", 0, INT_MIN, INT_MAX),
>   PORT_ITEM_INT("syncReceiptTimeout", 0, 0, UINT8_MAX),
>   GLOB_ITEM_INT("tc_spanning_tree", 0, 0, 1),
> diff --git a/ptp4l.8 b/ptp4l.8
> index 260aae3..0923bf3 100644
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -699,6 +699,14 @@ one-second offset 

Re: [Linuxptp-devel] [PATCH 5/6] clock: Add read-only UDS port for monitoring.

2021-01-26 Thread Jacob Keller



On 1/26/2021 2:00 AM, Miroslav Lichvar wrote:
> Add a second UDS port to allow untrusted applications to monitor ptp4l.
> On this "read-only" UDS port disable non-GET actions and forwarding.
> The path can be configured with the uds_address2 option (default is
> /var/run/ptp4lro).
> 

should uds_address2 be "uds_ro_address"? or some other name that implies
the read only aspect?

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 1/6] port: Don't assume transport from port number.

2021-01-26 Thread Jacob Keller



On 1/26/2021 2:00 AM, Miroslav Lichvar wrote:
> In port_open(), don't assume that UDS ports always have to have a zero
> number. Check the transport directly to make make the code cleaner.
> 
> Signed-off-by: Miroslav Lichvar 

This seems more straight forward than assuming the clock number is zero.

Reviewed-by: Jacob Keller 

> ---
>  port.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/port.c b/port.c
> index 0a93c06..6136153 100644
> --- a/port.c
> +++ b/port.c
> @@ -3108,23 +3108,25 @@ struct port *port_open(const char *phc_device,
>   p->versionNumber = PTP_VERSION;
>   p->slave_event_monitor = clock_slave_monitor(clock);
>  
> - if (number && unicast_client_initialize(p)) {
> + if (transport != TRANS_UDS && unicast_client_initialize(p)) {
>   goto err_transport;
>   }
>   if (unicast_client_enabled(p) &&
>   config_set_section_int(cfg, p->name, "hybrid_e2e", 1)) {
>   goto err_uc_client;
>   }
> - if (number && unicast_service_initialize(p)) {
> + if (transport != TRANS_UDS && unicast_service_initialize(p)) {
>   goto err_uc_client;
>   }
>   p->hybrid_e2e = config_get_int(cfg, p->name, "hybrid_e2e");
>  
> - if (number && type == CLOCK_TYPE_P2P && p->delayMechanism != DM_P2P) {
> + if (transport != TRANS_UDS &&
> + type == CLOCK_TYPE_P2P && p->delayMechanism != DM_P2P) {
>   pr_err("port %d: P2P TC needs P2P ports", number);
>   goto err_uc_service;
>   }
> - if (number && type == CLOCK_TYPE_E2E && p->delayMechanism != DM_E2E) {
> + if (transport != TRANS_UDS &&
> + type == CLOCK_TYPE_E2E && p->delayMechanism != DM_E2E) {
>   pr_err("port %d: E2E TC needs E2E ports", number);
>   goto err_uc_service;
>   }
> @@ -3158,7 +3160,7 @@ struct port *port_open(const char *phc_device,
>  
>   port_clear_fda(p, N_POLLFD);
>   p->fault_fd = -1;
> - if (number) {
> + if (transport != TRANS_UDS) {
>   p->fault_fd = timerfd_create(CLOCK_MONOTONIC, 0);
>   if (p->fault_fd < 0) {
>   pr_err("timerfd_create failed: %m");
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for monitoring.

2021-01-21 Thread Jacob Keller



On 1/21/2021 12:31 AM, Miroslav Lichvar wrote:
> On Wed, Jan 20, 2021 at 10:13:25PM +, Keller, Jacob E wrote:
>> It makes sense to remove forwarding, but I am not sure I understand the 
>> justification for removing access to subscriptions.. if the subscription is 
>> for read only data, why doesn't it make sense to allow that over the read 
>> only socket?
> 
> The subscription itself requires some state. If we say the new socket
> is safe to be accessed by untrusted applications, we need to be really
> sure they cannot do anything bad, e.g. create a large number of
> subscriptions to crash ptp4l or break subscriptions on the "rw"
> socket. Such issues would become security issues.
> 
> There might be a way to provide subscriptions on the "ro" socket, but
> they need to be separate from the "rw" subscriptions and have some
> limiting implemented.
> 
> I'd like to start with a minimal feature set that we can be
> comfortable with and maybe add other features later if there is a
> demand for them.
> 

Right. This makes sense. We can obviously extend the RO sockets in the
future, but I think it makes sense to limit it. To me, it seems good to
have some condensed version of this explanation in the commit message or
somewhere, since it may not be obvious why it is limited to those on the
outside.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 2/4] Deprecate the slaveOnly option in favor of clientOnly.

2021-01-14 Thread Jacob Keller



On 1/14/2021 6:15 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
> ---
>  clock.c  | 4 ++--
>  config.c | 5 -
>  port.c   | 2 +-
>  ptp4l.c  | 2 +-
>  timemaster.c | 2 +-
>  5 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 0156bc3..08c61eb 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -955,12 +955,12 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>  
>   c->dds.domainNumber = config_get_int(config, NULL, "domainNumber");
>  
> - if (config_get_int(config, NULL, "slaveOnly")) {
> + if (config_get_int(config, NULL, "clientOnly")) {
>   c->dds.flags |= DDS_SLAVE_ONLY;
>   }
>   if (!config_get_int(config, NULL, "gmCapable") &&
>   c->dds.flags & DDS_SLAVE_ONLY) {
> - pr_err("Cannot mix 1588 slaveOnly with 802.1AS !gmCapable");
> + pr_err("Cannot mix 1588 clientOnly with 802.1AS !gmCapable");
>   return NULL;
>   }
>   if (!config_get_int(config, NULL, "gmCapable") ||
> diff --git a/config.c b/config.c
> index a09739d..4095b33 100644
> --- a/config.c
> +++ b/config.c
> @@ -227,6 +227,7 @@ struct config_item config_tab[] = {
>   PORT_ITEM_INT("boundary_clock_jbod", 0, 0, 1),
>   PORT_ITEM_ENU("BMCA", BMCA_PTP, bmca_enu),
>   GLOB_ITEM_INT("check_fup_sync", 0, 0, 1),
> + GLOB_ITEM_INT("clientOnly", 0, 0, 1),
>   GLOB_ITEM_INT("clockAccuracy", 0xfe, 0, UINT8_MAX),
>   GLOB_ITEM_INT("clockClass", 248, 0, UINT8_MAX),
>   GLOB_ITEM_STR("clockIdentity", "00..00"),
> @@ -299,7 +300,7 @@ struct config_item config_tab[] = {
>   GLOB_ITEM_INT("servo_num_offset_values", 10, 0, INT_MAX),
>   GLOB_ITEM_INT("servo_offset_threshold", 0, 0, INT_MAX),
>   GLOB_ITEM_STR("slave_event_monitor", ""),
> - GLOB_ITEM_INT("slaveOnly", 0, 0, 1),
> + GLOB_ITEM_INT("slaveOnly", 0, 0, 1), /*deprecated*/>
> GLOB_ITEM_INT("socket_priority", 0, 0, 15),
>   GLOB_ITEM_DBL("step_threshold", 0.0, 0.0, DBL_MAX),
>   GLOB_ITEM_INT("summary_interval", 0, INT_MIN, INT_MAX),
> @@ -707,6 +708,8 @@ static void check_deprecated_options(const char **option)
>   new_option = "first_step_threshold";
>   } else if (!strcmp(*option, "pi_max_frequency")) {
>   new_option = "max_frequency";
> + } else if (!strcmp(*option, "slaveOnly")) {
> + new_option = "clientOnly";

Ahh. here is where the options get converted to support switching. Ok!

>   }
>  
>   if (new_option) {
> diff --git a/port.c b/port.c
> index 6c52004..0a93c06 100644
> --- a/port.c
> +++ b/port.c
> @@ -3048,7 +3048,7 @@ struct port *port_open(const char *phc_device,
>   } else if (clock_slave_only(clock)) {
>   p->state_machine = designated_slave_fsm;
>   } else {
> - pr_err("Please enable at least one of masterOnly or 
> slaveOnly when BMCA == noop.\n");
> + pr_err("Please enable at least one of masterOnly or 
> clientOnly when BMCA == noop.\n");
>   goto err_port;
>   }
>   } else {
> diff --git a/ptp4l.c b/ptp4l.c
> index ccbaa02..c61175b 100644
> --- a/ptp4l.c
> +++ b/ptp4l.c
> @@ -146,7 +146,7 @@ int main(int argc, char *argv[])
>   req_phc = optarg;
>   break;
>   case 's':
> - if (config_set_int(cfg, "slaveOnly", 1)) {
> + if (config_set_int(cfg, "clientOnly", 1)) {
>   goto out;
>   }
>   break;
> diff --git a/timemaster.c b/timemaster.c
> index 00db59f..fb27d72 100644
> --- a/timemaster.c
> +++ b/timemaster.c
> @@ -829,7 +829,7 @@ static int add_ptp_source(struct ptp_domain *source,
>   extend_config_string(_file->content,
>source->ptp4l_settings);
>   string_appendf(_file->content,
> -"slaveOnly 1\n"
> +"clientOnly 1\n"
>  "domainNumber %d\n"
>  "uds_address %s\n"
>  "message_tag %s\n",
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 1/4] Check for deprecated "long" options on the command line.

2021-01-14 Thread Jacob Keller



On 1/14/2021 6:15 AM, Richard Cochran wrote:
> The slaveOnly and masterOnly options will be deprecated in favor of
> clientOnly and serverOnly, respectively.  However, existing user scripts
> may very well make use of these options.  Apply the deprecated option
> check to the long command line options in order to avoid breaking existing
> deployments in the near future.
> 
> Signed-off-by: Richard Cochran 
> ---
>  config.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/config.c b/config.c
> index d237de9..a09739d 100644
> --- a/config.c
> +++ b/config.c
> @@ -1036,6 +1036,8 @@ int config_parse_option(struct config *cfg, const char 
> *opt, const char *val)
>  {
>   enum parser_result result;
>  
> + check_deprecated_options();
> +

Was this function introduced in a previous change? I don't recall it...

>   result = parse_item(cfg, 1, NULL, opt, val);
>  
>   switch (result) {
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 0/8] Convert to inclusive terminology, Part I

2021-01-11 Thread Jacob Keller



On 1/11/2021 9:05 AM, Richard Cochran wrote:
> On Mon, Jan 11, 2021 at 10:07:41AM +0100, Miroslav Lichvar wrote:
>> What will happen with the "grandmaster" term? Does it stay, or should
> 
> No decision yet on that one.
> 
>> it be replaced with something like "Primary time server"?
> 
> That sound reasonable to me.
>  
>> My only concern is the risk that the next standard will use a
>> different terminology and linuxptp users will have have to go through
>> the transition twice.
> 

I think I agree with Richard that much of the things we're renaming
don't strictly have to match. Plus, if we are able to influence the
standards towards this terminology we can avoid the potential.

> We can choose the terminology we like without causing confusion.  In
> fact, thanks to your insight of differentiating server/client and
> source/sink, this project will have clear and helpful documentation!
> 
> The *only* things we'll have to adapt to the new standard (if and when
> it ever appears) are the log messages that include port states.  Those
> will optionally follow the new terminology.  For now, I'm keeping
> almost all log messages unchanged, because some users are surely have
> scripts that scrape through the logs.
> 

Right. We can worry about changing those once standards have caught up.
I also think it's important to use the correct terms for various states
and other standard-specific names when possible (much like how you keep
some of the standard names the same even if they do not follow coding
guidelines such as camel casing or not)

> Thanks,
> Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC Patch 2/2] port: Fix link down/up to continue using phc_index set from command line -p option.

2021-01-07 Thread Jacob Keller



On 1/6/2021 11:39 AM, vincent.cheng...@renesas.com wrote:
> From: Vincent Cheng 
> 
> In the scenario where a port link goes down and up, current code checks
> the port's phc_index against the interface's phc_index and if they are
> different will set the port phc_index to the interface phc_index.
> 
> If the phc_index was initially set by the command line -p option, then we
> end up using the wrong phc_index.
> 
> Fix is to skip updating the port phc_index with the interface phc_index
> when port link is back up if it was initialy set from the command line.
> 
> Signed-off-by: Vincent Cheng 
> ---
>  port.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/port.c b/port.c
> index db3e9ac..bdc73e4 100644
> --- a/port.c
> +++ b/port.c
> @@ -2568,7 +2568,8 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>  "timestamping mode, set link status down 
> by force.",
>  interface_label(p->iface));
>   p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
> - } else if (p->phc_index != 
> interface_phc_index(p->iface)) {
> + } else if ((p->phc_index != 
> interface_phc_index(p->iface)) &&
> +(!p->phc_from_cmdline)) {
>   p->phc_index = interface_phc_index(p->iface);
>  

Makes sense. But maybe we would want to do a warning about when the PHC
index on the networking device no longer matches the command line? (like
we do during the initial setup?)

>   if (clock_switch_phc(p->clock, p->phc_index)) {
> @@ -3064,6 +3065,7 @@ struct port *port_open(const char *phc_device,
>  "not the attached ptp%d", number, phc_device,
>  interface_phc_index(interface));
>   p->phc_index = phc_index;
> + p->phc_from_cmdline = 1;
>   } else {
>   pr_err("port %d: PHC device mismatch", number);
>   pr_err("port %d: /dev/ptp%d requested, ptp%d attached",
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC Patch 1/2] port: Add phc_from_cmdline to indicate the phc index was from the command line.

2021-01-07 Thread Jacob Keller



On 1/6/2021 11:39 AM, vincent.cheng...@renesas.com wrote:
> From: Vincent Cheng 
> 
> Signed-off-by: Vincent Cheng 
> ---
>  port_private.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/port_private.h b/port_private.h
> index fcabaa6..6e40e15 100644
> --- a/port_private.h
> +++ b/port_private.h
> @@ -69,6 +69,7 @@ struct port {
>   struct fdarray fda;
>   int fault_fd;
>   int phc_index;
> + int phc_from_cmdline;
>  

This doesn't need to be a separate patch. You can just squash these
together, since there is no user added in this patch.

>   void (*dispatch)(struct port *p, enum fsm_event event, int mdiff);
>   enum fsm_event (*event)(struct port *p, int fd_index);
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 8/8] ts2phc: Convert usage message to time source/sink terminology.

2021-01-07 Thread Jacob Keller



On 1/5/2021 6:42 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  ts2phc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ts2phc.c b/ts2phc.c
> index 2342858..bc41041 100644
> --- a/ts2phc.c
> +++ b/ts2phc.c
> @@ -35,7 +35,7 @@ static void usage(char *progname)
>   fprintf(stderr,
>   "\n"
>   "usage: %s [options]\n\n"
> - " -c [dev|name]  phc slave clock (like /dev/ptp0 or eth0)\n"
> + " -c [dev|name]  PHC time sink (like /dev/ptp0 or eth0)\n"


You might have used "PHC device" here too, but since this is supposed to
be the side that is synchronized, I think this makes sense too.

>   "(may be specified multiple times)\n"
>   " -f [file]  read configuration from 'file'\n"
>   " -h prints this message and exits\n"
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 7/8] ptp4l: Convert usage messages to client/server terminology.

2021-01-07 Thread Jacob Keller



On 1/5/2021 6:42 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  ptp4l.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ptp4l.c b/ptp4l.c
> index 84661c5..ccbaa02 100644
> --- a/ptp4l.c
> +++ b/ptp4l.c
> @@ -58,7 +58,7 @@ static void usage(char *progname)
>   "   (may be specified multiple times)\n"
>   " -p [dev]  Clock device to use, default auto\n"
>   "   (ignored for SOFTWARE/LEGACY HW time stamping)\n"
> - " -sslave only mode (overrides configuration file)\n"
> + " -sclient only synchronization mode (overrides 
> configuration file)\n"
>   " -l [num]  set the logging level to 'num'\n"
>   " -mprint messages to stdout\n"
>   " -qdo not print messages to the syslog\n"
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 6/8] phc2sys: Convert usage messages to time source/sink terminology.

2021-01-07 Thread Jacob Keller



On 1/5/2021 6:42 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  phc2sys.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index aafff6c..70155f9 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -1024,10 +1024,10 @@ static void usage(char *progname)
>   " -r synchronize system (realtime) clock\n"
>   "repeat -r to consider it also as a time 
> source\n"
>   " manual configuration:\n"
> - " -c [dev|name]  slave clock (CLOCK_REALTIME)\n"
> - " -d [dev]   master PPS device\n"
> - " -s [dev|name]  master clock\n"
> - " -O [offset]slave-master time offset (0)\n"
> + " -c [dev|name]  time sink device (CLOCK_REALTIME)\n"
> + " -d [dev]   time source PPS device\n"
> + " -s [dev|name]  time source device\n"
> + " -O [offset]sink-source time offset in seconds (0)\n"
>   " -w wait for ptp4l\n"
>   " common options:\n"
>   " -f [file]  configuration file\n"
> @@ -1036,8 +1036,8 @@ static void usage(char *progname)
>   " -I [ki]integration constant (0.3)\n"
>   " -S [step]  step threshold (disabled)\n"
>   " -F [step]  step threshold only on start (0.2)\n"
> - " -R [rate]  slave clock update rate in HZ (1.0)\n"
> - " -N [num]   number of master clock readings per update 
> (5)\n"
> + " -R [rate]  update rate for the time sink devices in HZ 
> (1.0)\n"
> + " -N [num]   number of source clock readings per update 
> (5)\n"
>   " -L [limit] sanity frequency limit in ppb (2)\n"
>   " -M [num]   NTP SHM segment number (0)\n"
>   " -u [num]   number of clock updates in summary stats (0)\n"
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 5/8] ts2phc: Convert man page to source/sink terminology.

2021-01-07 Thread Jacob Keller



On 1/5/2021 6:42 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  ts2phc.8 | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/ts2phc.8 b/ts2phc.8
> index 77f8940..0bd523d 100644
> --- a/ts2phc.8
> +++ b/ts2phc.8
> @@ -1,4 +1,4 @@
> -.TH TS2PHC 8 "December 2019" "linuxptp"
> +.TH TS2PHC 8 "January 2021" "linuxptp"
>  .SH NAME
>  ts2phc - Synchronizes one or more PTP Hardware Clocks using external time 
> stamps.
>  
> @@ -27,7 +27,7 @@ A single source may be used to distribute time to one or 
> more PHC devices.
>  .SH OPTIONS
>  .TP
>  .BI \-c " device|name"
> -Specifies a PHC slave clock to be synchronized.
> +Specifies a PHC to be synchronized.

Makes sense, no reason to keep "clock" since PHC as an acronym already
includes clock.

>  The clock may be identified by its character device (like /dev/ptp0)
>  or its associated network interface (like eth0).
>  This option may be given multiple times.
> @@ -52,7 +52,7 @@ Prevents sending log messages to the system logger.
>  .BI \-s " device|name"
>  Specifies the source of the PPS signal.
>  Use the key word "generic" for an external 1-PPS without ToD information.
> -When using a master PHC, the clock may be identified by its character
> +When using a PHC as the time source, the clock may be identified by its 
> character
>  device (like /dev/ptp0) or its associated network interface (like
>  eth0).
>  Use the key word "nmea" for an external 1-PPS from a GPS providing ToD
> @@ -88,14 +88,14 @@ There are two different section types.
>  .B 1.
>  The global section (indicated as
>  .BR [global] )
> -sets the program options and default slave clock options. Other
> +sets the program options and the default time sink options. Other
>  sections are clock specific sections, and they override the default
>  options.
>  .TP
>  .B 2.
> -Slave clock sections give the name of the configured slave (e.g.
> +Time sink sections give the name of the configured PHC (e.g.
>  .BR [eth0] ).
> -Slave clocks specified in the configuration file need not be specified
> +Time sinks specified in the configuration file need not be specified
>  with the
>  .B \-c
>  command line option.
> @@ -111,10 +111,10 @@ step the clock on start.
>  The default is 0.2 (20 microseconds).
>  .TP
>  .B free_running
> -When set to 1, no the slave clock will be adjusted.
> +When set to 1, no PHC will be adjusted.
>  This option can be useful in test scenarios, for example to determine
>  how well synchronized a group of local clocks are to each other.
> -The default is 0 (adjust the slave clocks).
> +The default is 0 (adjust the clocks).
>  .TP
>  .B leapfile
>  The path to the current leap seconds definition file.
> @@ -173,7 +173,7 @@ Print messages to the system log if enabled.  The default 
> is 1 (enabled).
>  .B verbose
>  Print messages to the standard output if enabled.  The default is 0 
> (disabled).
>  
> -.SH SLAVE CLOCK OPTIONS
> +.SH TIME SINK OPTIONS
>  
>  .TP
>  .B ts2phc.channel
> @@ -199,7 +199,7 @@ The default is "rising".
>  .B ts2phc.master
>  Setting this option to 1 configures the given PHC device as the source
>  of the PPS signal.
> -The default is 0 for the slave role.
> +The default is 0 for the time sink role.
>  .TP
>  .B ts2phc.pin_index
>  The pin index to be used.
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 4/8] ptp4l: Convert man page to client/server terminology.

2021-01-07 Thread Jacob Keller



On 1/5/2021 6:42 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  ptp4l.8 | 80 +
>  1 file changed, 41 insertions(+), 39 deletions(-)
> 
> diff --git a/ptp4l.8 b/ptp4l.8
> index 473215b..42c9bde 100644
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -1,4 +1,4 @@
> -.TH PTP4l 8 "April 2018" "linuxptp"
> +.TH PTP4l 8 "January 2021" "linuxptp"
>  .SH NAME
>  ptp4l - PTP Boundary/Ordinary/Transparent Clock
>  
> @@ -145,13 +145,13 @@ See UNICAST DISCOVERY OPTIONS, below.
>  .TP
>  .B delayAsymmetry
>  The time difference in nanoseconds of the transmit and receive
> -paths. This value should be positive when the master-to-slave
> -propagation time is longer and negative when the slave-to-master time
> +paths. This value should be positive when the server-to-client
> +propagation time is longer and negative when the client-to-server time
>  is longer. The default is 0 nanoseconds.
>  .TP
>  .B logAnnounceInterval
>  The mean time interval between Announce messages. A shorter interval makes
> -ptp4l react faster to the changes in the master-slave hierarchy. The interval
> +ptp4l react faster to the changes in the client/server hierarchy. The 
> interval
>  should be the same in the whole domain. It's specified as a power of two in
>  seconds.
>  The default is 1 (2 seconds).
> @@ -164,7 +164,7 @@ The default is 0 (1 second).
>  .B operLogSyncInterval
>  The Sync message interval to be requested once the clock enters the
>  SERVO_LOCKED_STABLE state.  If the 'msg_interval_request' option is
> -set, then the local slave port will request the remote master to
> +set, then the local client port will request the remote server to
>  switch to the given message rate via a signaling message containing a
>  Message interval request TLV.  This option is specified as a power of
>  two in seconds, and default value is 0 (1 second).
> @@ -183,14 +183,14 @@ The default is 0 (1 second).
>  .B operLogPdelayReqInterval
>  The Pdelay Request messages interval to be used once the clock enters
>  the SERVO_LOCKED_STABLE state.  If the 'msg_interval_request' option
> -is set, then the local slave port will adopt this rate when the local
> +is set, then the local client port will adopt this rate when the local
>  clock enters the "locked stable" state.  This option is specified as a
>  power of two in seconds, and the default value is 0 (1 second).
>  .TP
>  .B inhibit_delay_req
>  Don't send any delay requests. This will need the asCapable config option to 
> be
> -set to 'true'. This is useful when running as a designated master who does 
> not
> -need to calculate offset from slave. The default is 0 (disabled).
> +set to 'true'. This is useful when running as a designated server who does 
> not
> +need to calculate offset from client. The default is 0 (disabled).
>  .TP
>  .B announceReceiptTimeout
>  The number of missed Announce messages before the last Announce messages
> @@ -244,9 +244,9 @@ The default is E2E.
>  .TP
>  .B hybrid_e2e
>  Enables the "hybrid" delay mechanism from the draft Enterprise
> -Profile. When enabled, ports in the slave state send their delay
> -request messages to the unicast address taken from the master's
> -announce message. Ports in the master state will reply to unicast
> +Profile. When enabled, ports in the client state send their delay
> +request messages to the unicast address taken from the server's
> +announce message. Ports in the server state will reply to unicast
>  delay requests using unicast delay responses. This option has no
>  effect if the delay_mechanism is set to P2P.
>  The default is 0 (disabled).
> @@ -302,7 +302,7 @@ greater than this value the port is marked as not 802.1AS 
> capable.
>  .TP
>  .B masterOnly
>  Setting this option to one (1) prevents the port from entering the
> -SLAVE state. In addition, the local clock will ignore Announce
> +client state. In addition, the local clock will ignore Announce
>  messages received on this port. This option's intended use is to
>  support the Telecom Profiles according to ITU-T G.8265.1, G.8275.1,
>  and G.8275.2. The default value is zero or false.
> @@ -381,7 +381,7 @@ hardware time stamping.
>  The default is 1 (enabled).
>  .TP
>  .B slaveOnly
> -The local clock is a slave-only clock if enabled. The default is 0 
> (disabled).
> +The local clock is a client-only clock if enabled. The default is 0 
> (disabled).
>  .TP
>  .B socket_priority
>  Configure the SO_PRIORITY of sockets. This is to support cases where a user
> @@ -396,13 +396,13 @@ This is only for use with 802.1AS clocks

Re: [Linuxptp-devel] [PATCH v2 3/8] phc2sys: Convert man page to client/server terminology.

2021-01-07 Thread Jacob Keller



On 1/5/2021 6:42 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  phc2sys.8 | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/phc2sys.8 b/phc2sys.8
> index 7773fd0..99fc937 100644
> --- a/phc2sys.8
> +++ b/phc2sys.8
> @@ -414,22 +414,22 @@ or
>  .B \-w
>  is in effect or from command line when
>  .B \-O
> -is supplied.  Failure to maintain the correct offset can result in local 
> system
> -clock being off some seconds to domain master system clock when in slave 
> mode,
> -or incorect PTP time announced to the network in case the host is the domain
> -master.
> +is supplied.  Failure to maintain the correct offset can result in the
> +local system clock being offset some whole number of seconds from the
> +domain server's clock when in client mode, or incorect PTP time
> +announced to the network in case the host is the domain server.
>  
>  .SH EXAMPLES
>  
>  Synchronize time automatically according to the current
>  .B ptp4l
> -state, synchronize the system clock to the remote master.
> +state, synchronizing the system clock to the remote server.
>  
>  .RS
>  \f(CWphc2sys \-a \-r\fP
>  .RE
>  
> -Same as above, but when the host becomes the domain master, synchronize time
> +Same as above, but when the host becomes the domain server, synchronize time
>  in the domain to its system clock.
>  
>  .RS
> @@ -442,13 +442,13 @@ Same as above, in an IEEE 802.1AS domain.
>  \f(CWphc2sys \-a \-rr --transportSpecific=1\fP
>  .RE
>  
> -The host is a domain master, PTP clock is synchronized to system clock and 
> the
> +The host is a domain server, PTP clock is synchronized to system clock and 
> the
>  time offset is obtained from
>  .BR ptp4l .
>  .B Phc2sys
>  waits for
>  .B ptp4l
> -to get at least one port in master or slave mode before starting the
> +to get at least one port in server or client mode before starting the
>  synchronization.
>  
>  .RS
> @@ -464,7 +464,7 @@ does not wait for
>  \f(CWphc2sys \-c /dev/ptp0 \-s CLOCK_REALTIME \-O 35\fP
>  .RE
>  
> -The host is in slave mode, system clock is synchronized from PTP clock,
> +The host is in client mode, system clock is synchronized from PTP clock,
>  .B phc2sys
>  waits for
>  .B ptp4l
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 2/8] phc2sys: Convert man page to source/sink terminology.

2021-01-07 Thread Jacob Keller



On 1/5/2021 6:42 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
> ---

Makes sense. I like this direction for the terminology change.

Reviewed-by: Jacob Keller 

>  phc2sys.8 | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/phc2sys.8 b/phc2sys.8
> index 66007eb..7773fd0 100644
> --- a/phc2sys.8
> +++ b/phc2sys.8
> @@ -52,7 +52,7 @@ Manual configuration is also possible. When using manual 
> configuration, two
>  synchronization modes are supported, one uses a pulse per second (PPS)
>  signal provided by the source clock and the other mode reads time from the
>  source clock directly. Some clocks can be used in both modes, the mode which
> -will synchronize the slave clock with better accuracy depends on hardware
> +will synchronize the time sink with better accuracy depends on hardware
>  and driver implementation.
>  
>  .SH OPTIONS
> @@ -82,22 +82,22 @@ Read configuration from the specified file. No 
> configuration file is read by
>  default.
>  .TP
>  .BI \-d " pps-device"
> -Specify the PPS device of the master clock (e.g. /dev/pps0). With this option
> +Specify the PPS device of the source clock (e.g. /dev/pps0). With this option
>  the PPS synchronization mode is used instead of the direct mode.  The
>  matching PHC must be specified using the
>  .B \-s
>  command line option.
> -This option can be used only with the system clock as the slave clock. Not
> +This option can be used only with the system clock as the time sink. Not
>  compatible with the
>  .B \-a
>  option.
>  .TP
>  .BI \-s " device"
> -Specify the master clock by device (e.g. /dev/ptp0) or interface (e.g. eth0) 
> or
> +Specify the source clock by device (e.g. /dev/ptp0) or interface (e.g. eth0) 
> or
>  by name (e.g. CLOCK_REALTIME for the system clock). When this option is used
>  together with the
>  .B \-d
> -option, the master clock is used only to correct the offset by whole number 
> of
> +option, the source clock is used only to correct the offset by whole number 
> of
>  seconds, which cannot be fixed with PPS alone. Not compatible with the
>  .B \-a
>  option. This option does not support bonded interface (e.g. bond0, team0). If
> @@ -109,14 +109,14 @@ option can be used to track the active interface.
>  .BI \-i " interface"
>  Performs the exact same function as
>  .B \-s
> -for compatibility reasons. Previously enabled specifying master clock by 
> network
> +for compatibility reasons. Previously enabled specifying source clock by 
> network
>  interface. However, this can now be done using
>  .B \-s
>  and this option is no longer necessary. As such it has been deprecated, and
>  should no longer be used.
>  .TP
>  .BI \-c " device"
> -Specify the slave clock by device (e.g. /dev/ptp1) or interface (e.g. eth1) 
> or
> +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
> @@ -151,17 +151,17 @@ seconds. The value of 0.0 disables stepping on start. 
> The default is 0.2
>  (20 microseconds).
>  .TP
>  .BI \-R " update-rate"
> -Specify the slave clock update rate when running in the direct 
> synchronization
> +Specify the time sink update rate when running in the direct synchronization
>  mode. The default is 1 per second.
>  .TP
>  .BI \-N " phc-num"
> -Specify the number of master clock readings per one slave clock update. Only
> -the fastest reading is used to update the slave clock, this is useful to
> +Specify the number of source clock readings used for each time sink update.
> +Only the fastest reading is used to update the clock.  This is useful to
>  minimize the error caused by random delays in scheduling and bus utilization.
>  The default is 5.
>  .TP
>  .BI \-O " offset"
> -Specify the offset between the slave and master times in seconds. Not
> +Specify the offset between the sink and source times in seconds. Not
>  compatible with the
>  .B \-a
>  option.  See
> @@ -192,7 +192,7 @@ The default is 0 (disabled).
>  .B \-w
>  Wait until ptp4l is in a synchronized state. If the
>  .B \-O
> -option is not used, also keep the offset between the slave and master
> +option is not used, also keep the offset between the sink and source
>  times updated according to the currentUtcOffset value obtained from ptp4l and
>  the direction of the clock synchronization. Not compatible with the
>  .B \-a
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 1/8] phc2sys: Update man page to reflect the new restriction on the PPS mode.

2021-01-07 Thread Jacob Keller



On 1/5/2021 6:42 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
> ---
>  phc2sys.8 | 16 
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/phc2sys.8 b/phc2sys.8
> index b3a3de3..66007eb 100644
> --- a/phc2sys.8
> +++ b/phc2sys.8
> @@ -1,4 +1,4 @@
> -.TH PHC2SYS 8 "April 2018" "linuxptp"
> +.TH PHC2SYS 8 "January 2021" "linuxptp"
>  .SH NAME
>  phc2sys \- synchronize two or more clocks
>  
> @@ -83,18 +83,10 @@ default.
>  .TP
>  .BI \-d " pps-device"
>  Specify the PPS device of the master clock (e.g. /dev/pps0). With this option
> -the PPS synchronization mode is used instead of the direct mode. As the PPS
> -signal does not specify time and only marks start of a second, the slave 
> clock
> -should be already close to the correct time before
> -.B phc2sys
> -is started or the
> -.B \-s
> -option should be used too. With the
> +the PPS synchronization mode is used instead of the direct mode.  The
> +matching PHC must be specified using the
>  .B \-s
Makes sense. We don't need a further explanation about the clocks being
close together because -s is always required now. ok.

> -option the PPS signal of the master clock is enabled automatically, otherwise
> -it has to be enabled before
> -.B phc2sys
> -is started (e.g. by running \f(CWecho 1 > /sys/class/ptp/ptp0/pps_enable\fP).
> +command line option.
>  This option can be used only with the system clock as the slave clock. Not
>  compatible with the
>  .B \-a
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 6/6] phc2sys: Replace magical test with a proper test.

2020-11-30 Thread Jacob Keller



On 11/29/2020 7:50 PM, Richard Cochran wrote:
> When using a PPS time source with the -w flag, the program closes the PMC
> agent's connection once the ptp4l service has appeared and the UTC offset
> has been optionally queried.
> 
> In order to tell whether PPS is the time source, the code tests for
> 
> src->clkid == CLOCK_INVALID
> 
> which is completely opaque and non-obvious.  Presently only the case where
> src_name is NULL (and pps_fd is valid) will trigger this test, but that is
> only incidental.
> 
> Clarify the intention of the code by using a proper test.
> 
> Signed-off-by: Richard Cochran 

Significant improvement here, it is a bit more clear what's going on
here now.

Reviewed-by: Jacob Keller 

Thanks,
Jake

> ---
>  phc2sys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index e65bdf5..15ccb80 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -1328,7 +1328,7 @@ int main(int argc, char *argv[])
>  
>   if (priv.forced_sync_offset ||
>   (src->clkid != CLOCK_REALTIME && dst->clkid != 
> CLOCK_REALTIME) ||
> - src->clkid == CLOCK_INVALID) {
> + hardpps_configured(pps_fd)) {
>   pmc_agent_disable(priv.agent);
>   }
>   }
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 5/6] phc2sys: Expand the validation of the PPS mode.

2020-11-30 Thread Jacob Keller



On 11/29/2020 7:50 PM, Richard Cochran wrote:
> When using a PPS time source, the PHC character device or network interface
> specified using the -s command line flag will be ignored.  Detect this
> mis-configuration and throw a usage error.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  phc2sys.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index 03347f0..e65bdf5 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -1253,6 +1253,10 @@ int main(int argc, char *argv[])
>   "cannot use a pps device unless destination is 
> CLOCK_REALTIME\n");
>   goto bad_usage;
>   }
> + if (hardpps_configured(pps_fd) && src_name) {
> + fprintf(stderr, "please specify -s or -d, but not both\n");
> + goto bad_usage;
> + }

Should this have an associated manual page update, or is the manual page
already correct?

>  
>   print_set_progname(progname);
>   print_set_tag(config_get_string(cfg, NULL, "message_tag"));
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 4/6] phc2sys: Validate the PPS mode right away.

2020-11-30 Thread Jacob Keller



On 11/29/2020 7:50 PM, Richard Cochran wrote:
> When using a PPS time source, the code only checks the destination
> clock after performing much static setup.  There is no need to delay
> the validation, so move this to the other sanity checks.
> 
> Signed-off-by: Richard Cochran 

Makes sense.

Reviewed-by: Jacob Keller 

> ---
>  phc2sys.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index f28e9be..03347f0 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -1245,6 +1245,15 @@ int main(int argc, char *argv[])
>   goto bad_usage;
>   }
>  
> + if (!dst_name) {
> + dst_name = "CLOCK_REALTIME";
> + }
> + if (hardpps_configured(pps_fd) && strcmp(dst_name, "CLOCK_REALTIME")) {
> + fprintf(stderr,
> + "cannot use a pps device unless destination is 
> CLOCK_REALTIME\n");
> + goto bad_usage;

Now we do a strcmp instead of checking the raw clkid, but that is only
because we haven't yet opened the clock. Makes sense.

> + }
> +
>   print_set_progname(progname);
>   print_set_tag(config_get_string(cfg, NULL, "message_tag"));
>   print_set_verbose(config_get_int(cfg, NULL, "verbose"));
> @@ -1280,7 +1289,7 @@ int main(int argc, char *argv[])
>   src->state = PS_SLAVE;
>   priv.master = src;
>  
> - dst = clock_add(, dst_name ? dst_name : "CLOCK_REALTIME");
> + dst = clock_add(, dst_name);
>   if (!dst) {
>   fprintf(stderr, "valid destination clock must be selected.\n");
>   goto bad_usage;
> @@ -1288,12 +1297,6 @@ int main(int argc, char *argv[])
>   dst->state = PS_MASTER;
>   LIST_INSERT_HEAD(_clocks, dst, dst_list);
>  
> - if (hardpps_configured(pps_fd) && dst->clkid != CLOCK_REALTIME) {
> - fprintf(stderr,
> - "cannot use a pps device unless destination is 
> CLOCK_REALTIME\n");
> - goto bad_usage;
> - }
> -
>   r = -1;
>  
>   if (wait_sync) {
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 3/6] phc2sys: Replace hard coded tests with a readable helper function.

2020-11-30 Thread Jacob Keller



On 11/29/2020 7:50 PM, Richard Cochran wrote:
> The main program determines whether a PPS device is the time source
> with an open-coded test for a valid file descriptor.  Replace it with
> a helper routine whose name properly signifies the meaning of the
> test.
> 
> Signed-off-by: Richard Cochran 

Makes sense.

Reviewed-by: Jacob Keller 

> ---
>  phc2sys.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index e17909f..f28e9be 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -979,6 +979,11 @@ static int clock_handle_leap(struct phc2sys_private 
> *priv, struct clock *clock,
>   return 0;
>  }
>  
> +static bool hardpps_configured(int fd)
> +{
> + return fd >= 0;
> +}
> +
>  static void usage(char *progname)
>  {
>   fprintf(stderr,
> @@ -1222,12 +1227,13 @@ int main(int argc, char *argv[])
>   return c;
>   }
>  
> - if (autocfg && (src_name || dst_name || pps_fd >= 0 || wait_sync || 
> priv.forced_sync_offset)) {
> + if (autocfg && (src_name || dst_name || hardpps_configured(pps_fd) ||
> + wait_sync || priv.forced_sync_offset)) {
>   fprintf(stderr,
>   "autoconfiguration cannot be mixed with manual config 
> options.\n");
>   goto bad_usage;
>   }
> - if (!autocfg && pps_fd < 0 && !src_name) {
> + if (!autocfg && !hardpps_configured(pps_fd) && !src_name) {
>   fprintf(stderr,
>   "autoconfiguration or valid source clock must be 
> selected.\n");
>   goto bad_usage;
> @@ -1282,7 +1288,7 @@ int main(int argc, char *argv[])
>   dst->state = PS_MASTER;
>   LIST_INSERT_HEAD(_clocks, dst, dst_list);
>  
> - if (pps_fd >= 0 && dst->clkid != CLOCK_REALTIME) {
> + if (hardpps_configured(pps_fd) && dst->clkid != CLOCK_REALTIME) {
>   fprintf(stderr,
>   "cannot use a pps device unless destination is 
> CLOCK_REALTIME\n");
>   goto bad_usage;
> @@ -1320,7 +1326,7 @@ int main(int argc, char *argv[])
>   }
>   }
>  
> - if (pps_fd >= 0) {
> + if (hardpps_configured(pps_fd)) {
>   /* only one destination clock allowed with PPS until we
>* implement a mean to specify PTP port to PPS mapping */
>   dst->servo = servo_add(, dst);
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 2/6] phc2sys: Rename PMC agent pointer from node to agent.

2020-11-30 Thread Jacob Keller



On 11/29/2020 7:50 PM, Richard Cochran wrote:
> Node is not a very descriptive name.  Rename it.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  phc2sys.c | 48 
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index 13cf235..e17909f 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -103,7 +103,7 @@ struct phc2sys_private {
>   int forced_sync_offset;
>   int kernel_leap;
>   int state_changed;
> - struct pmc_agent *node;
> + struct pmc_agent *agent;
>   LIST_HEAD(port_head, port) ports;
>   LIST_HEAD(clock_head, clock) clocks;
>   LIST_HEAD(dst_clock_head, clock) dst_clocks;
> @@ -302,7 +302,7 @@ static void clock_reinit(struct phc2sys_private *priv, 
> struct clock *clock,
>   if (p->clock != clock) {
>   continue;
>   }
> - err = pmc_agent_query_port_properties(priv->node, 1000,
> + err = pmc_agent_query_port_properties(priv->agent, 1000,
> p->number, ,
> , iface);
>   if (!err) {
> @@ -644,7 +644,7 @@ static int do_pps_loop(struct phc2sys_private *priv, 
> struct clock *clock,
>  
>   if (src == CLOCK_INVALID) {
>   /* The sync offset can't be applied with PPS alone. */
> - pmc_agent_set_sync_offset(priv->node, 0);
> + pmc_agent_set_sync_offset(priv->agent, 0);
>   } else {
>   enable_pps_output(priv->master->clkid);
>   }
> @@ -675,7 +675,7 @@ static int do_pps_loop(struct phc2sys_private *priv, 
> struct clock *clock,
>   pps_offset = pps_ts - phc_ts;
>   }
>  
> - if (pmc_agent_update(priv->node) < 0)
> + if (pmc_agent_update(priv->agent) < 0)
>   continue;
>   update_clock(priv, clock, pps_offset, pps_ts, -1);
>   }
> @@ -713,16 +713,16 @@ static int do_loop(struct phc2sys_private *priv, int 
> subscriptions)
>   while (is_running()) {
>   clock_nanosleep(CLOCK_MONOTONIC, 0, , NULL);
>  
> - if (pmc_agent_update(priv->node) < 0) {
> + if (pmc_agent_update(priv->agent) < 0) {
>   continue;
>   }
>  
>   if (subscriptions) {
> - run_pmc_events(priv->node);
> + run_pmc_events(priv->agent);
>   if (priv->state_changed) {
>   /* force getting offset, as it may have
>* changed after the port state change */
> - if (pmc_agent_query_utc_offset(priv->node, 
> 1000)) {
> + if (pmc_agent_query_utc_offset(priv->agent, 
> 1000)) {
>   pr_err("failed to get UTC offset");
>   continue;
>   }
> @@ -853,7 +853,7 @@ static int auto_init_ports(struct phc2sys_private *priv, 
> int add_rt)
>   if (!is_running()) {
>   return -1;
>   }
> - err = pmc_agent_query_dds(priv->node, 1000);
> + err = pmc_agent_query_dds(priv->agent, 1000);
>   if (!err) {
>   break;
>   }
> @@ -864,20 +864,20 @@ static int auto_init_ports(struct phc2sys_private 
> *priv, int add_rt)
>   }
>   }
>  
> - number_ports = pmc_agent_get_number_ports(priv->node);
> + number_ports = pmc_agent_get_number_ports(priv->agent);
>   if (number_ports <= 0) {
>   pr_err("failed to get number of ports");
>   return -1;
>   }
>  
> - err = pmc_agent_subscribe(priv->node, 1000);
> + err = pmc_agent_subscribe(priv->agent, 1000);
>   if (err) {
>   pr_err("failed to subscribe");
>   return -1;
>   }
>  
>   for (i = 1; i <= number_ports; i++) {
> - err = pmc_agent_query_port_properties(priv->node, 1000, i,
> + err = pmc_agent_query_port_properties(priv->agent, 1000, i,
> , ,
> iface);
>   if (err == -ENODEV) {
> @@ -915,7 +915,7 @@ static int auto_init_ports(struct phc2sys_private *priv, 
> int add_rt)
>   }
>  
>   /* get

Re: [Linuxptp-devel] [PATCH 1/6] phc2sys: Don't duplicate the command line arguments.

2020-11-30 Thread Jacob Keller



On 11/29/2020 7:50 PM, Richard Cochran wrote:
> The names of the source and destination clocks are generated by
> duplicating command line arguments, and then the newly allocated
> memory is immediately freed.  Remove the unnecessary malloc/free dance
> and use the arguments directly.
> 
> Signed-off-by: Richard Cochran 

Yep, makes sense.

Reviewed-by: Jacob Keller 

> ---
>  phc2sys.c | 12 
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index c300984..13cf235 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -1071,7 +1071,7 @@ int main(int argc, char *argv[])
>   rt++;
>   break;
>   case 'c':
> - dst_name = strdup(optarg);
> + dst_name = optarg;
>   break;
>   case 'd':
>   pps_fd = open(optarg, O_RDONLY);
> @@ -1089,7 +1089,7 @@ int main(int argc, char *argv[])
>   "'-i' has been deprecated. please use '-s' 
> instead.\n");
>  /* fallthrough */
>   case 's':
> - src_name = strdup(optarg);
> + src_name = optarg;
>   break;
>   case 'E':
>   if (!strcasecmp(optarg, "pi")) {
> @@ -1267,20 +1267,16 @@ int main(int argc, char *argv[])
>   }
>  
>   src = clock_add(, src_name);
> - free(src_name);
>   if (!src) {
> - fprintf(stderr,
> - "valid source clock must be selected.\n");
> + fprintf(stderr, "valid source clock must be selected.\n");
>   goto bad_usage;
>   }
>   src->state = PS_SLAVE;
>   priv.master = src;
>  
>   dst = clock_add(, dst_name ? dst_name : "CLOCK_REALTIME");
> - free(dst_name);
>   if (!dst) {
> - fprintf(stderr,
> - "valid destination clock must be selected.\n");
> + fprintf(stderr, "valid destination clock must be selected.\n");
>   goto bad_usage;
>   }
>   dst->state = PS_MASTER;
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 4/4] pmc_agent: Simplify the method that gets of the number of local ports.

2020-11-30 Thread Jacob Keller



On 11/28/2020 9:08 AM, Richard Cochran wrote:
> The number of ports is already available in the cached default data
> set.  Use it directly.
> 
> Signed-off-by: Richard Cochran 
> ---
>  phc2sys.c   |  2 +-
>  pmc_agent.c | 24 
>  pmc_agent.h | 11 ++-
>  3 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index 5386cf4..16daa66 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -864,7 +864,7 @@ static int auto_init_ports(struct phc2sys_private *priv, 
> int add_rt)
>   }
>   }
>  
> - number_ports = run_pmc_get_number_ports(priv->node, 1000);
> + number_ports = pmc_agent_get_number_ports(priv->node);
>   if (number_ports <= 0) {
>   pr_err("failed to get number of ports");
>   return -1;
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 4c5b18a..d3db975 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -229,22 +229,6 @@ int run_pmc_wait_sync(struct pmc_agent *node, int 
> timeout)
>   }
>  }
>  
> -int run_pmc_get_number_ports(struct pmc_agent *node, int timeout)
> -{
> - struct ptp_message *msg;
> - int res;
> - struct defaultDS *dds;
> -
> - res = run_pmc(node, timeout, TLV_DEFAULT_DATA_SET, );
> - if (res <= 0)
> - return res;
> -
> - dds = (struct defaultDS *) management_tlv_data(msg);
> - res = dds->numberPorts;
> - msg_put(msg);
> - return res;
> -}
> -
>  void run_pmc_events(struct pmc_agent *node)
>  {
>   struct ptp_message *msg;
> @@ -292,6 +276,14 @@ int pmc_agent_get_sync_offset(struct pmc_agent *agent)
>   return agent->sync_offset;
>  }
>  
> +int pmc_agent_get_number_ports(struct pmc_agent *node)
> +{
> + if (!node->dds_valid) {
> + return -1;
> + }
> + return node->dds.numberPorts;
> +}
> +

Based on your previous patches suggestions, shouldn't this be
"pmc_agent_query_number_ports", considering that this is returning a
cached value?

>  int pmc_agent_query_dds(struct pmc_agent *node, int timeout)
>  {
>   struct ptp_message *msg;
> diff --git a/pmc_agent.h b/pmc_agent.h
> index 2bb3101..abe62f5 100644
> --- a/pmc_agent.h
> +++ b/pmc_agent.h
> @@ -34,7 +34,6 @@ typedef int pmc_node_recv_subscribed_t(void *context, 
> struct ptp_message *msg,
>  int init_pmc_node(struct config *cfg, struct pmc_agent *agent, const char 
> *uds,
> pmc_node_recv_subscribed_t *recv_subscribed, void *context);
>  int run_pmc_wait_sync(struct pmc_agent *agent, int timeout);
> -int run_pmc_get_number_ports(struct pmc_agent *agent, int timeout);
>  void run_pmc_events(struct pmc_agent *agent);
>  
>  /**
> @@ -56,6 +55,16 @@ void pmc_agent_destroy(struct pmc_agent *agent);
>   */
>  int pmc_agent_get_leap(struct pmc_agent *agent);
>  
> +/**
> + * Gets the number of local ports from the default data set.  Users
> + * should first call pmc_agent_query_dds() before invoking this
> + * function.
> + *
> + * @param agent  Pointer to a PMC instance obtained via @ref 
> pmc_agent_create().
> + * @return   The non-negative number of ports, or -1 if unknown.
> + */
> +int pmc_agent_get_number_ports(struct pmc_agent *agent);
> +
>  /**
>   * Gets the TAI-UTC offset.
>   * @param agent  Pointer to a PMC instance obtained via @ref 
> pmc_agent_create().
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 2/4] pmc_agent: Convert the method that queries the port properties.

2020-11-30 Thread Jacob Keller



On 11/28/2020 9:08 AM, Richard Cochran wrote:
> Prefix the function with the module name and correct the return code
> semantics.
> 
> The active word in the function's name is "query" rather that "get" in
> order to distinguish methods that send and receive over the network
> from those that merely return a cached value.
> 
> Signed-off-by: Richard Cochran 
> ---
>  phc2sys.c   | 41 +++--
>  pmc_agent.c | 74 ++---
>  pmc_agent.h | 22 +---
>  3 files changed, 78 insertions(+), 59 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index 725a25c..610b8bb 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -292,24 +292,26 @@ static struct port *port_add(struct phc2sys_private 
> *priv, unsigned int number,
>  static void clock_reinit(struct phc2sys_private *priv, struct clock *clock,
>int new_state)
>  {
> - int phc_index = -1, phc_switched = 0;
> - int state, timestamping, ret = -1;
> + int err = -1, phc_index = -1, phc_switched = 0, state, timestamping;
>   struct port *p;
>   struct sk_ts_info ts_info;
>   char iface[IFNAMSIZ];
>   clockid_t clkid = CLOCK_INVALID;
>  
>   LIST_FOREACH(p, >ports, list) {
> - if (p->clock == clock) {
> - ret = run_pmc_port_properties(priv->node, 1000, 
> p->number,
> -   , ,
> -   iface);
> - if (ret > 0)
> - p->state = normalize_state(state);
> + if (p->clock != clock) {
> + continue;
> + }

We do a continue now to skip over the clock until we found it. Ok.. Bit odd.

Seems like it might be worth introducing some helper function that loops
over the port list and locates the clock.

> + err = pmc_agent_query_port_properties(priv->node, 1000,
> +   p->number, ,
> +   , iface);
> + if (!err) {
> + p->state = normalize_state(state);
>   }
> + break;
>   }
>  

To me, this entire block might have read better as something like:

LIST_FOREACH(p, >ports, list) {
if (p->clock == clock) {
break
}
}

/* exit if we failed to find a clock */

/* do the run_pmc_port_properties */


But that's a bit awkward because you do need to check that the loop
didn't hit the end without finding a clock. In the previous code that
was snuck in by having "ret" be -1 by initialization. This could be even
further reduced if we had a helper function that returned the first port
with the matching clock.

None of this is really the fault of this patch, and could easily be left
for a future cleanup/refactor. I believe the patch as written has the
same semantics as the original before the return code cleanup.

> - if (ret > 0 && timestamping != TS_SOFTWARE) {
> + if (!err && timestamping != TS_SOFTWARE) {
>   /* Check if device changed */
>   if (strcmp(clock->device, iface)) {
>   free(clock->device);
> @@ -841,12 +843,12 @@ static int phc2sys_recv_subscribed(void *context, 
> struct ptp_message *msg,
>  
>  static int auto_init_ports(struct phc2sys_private *priv, int add_rt)
>  {
> - struct port *port;
> - struct clock *clock;
> - int number_ports, res;
> - unsigned int i;
> + int err, number_ports, res;
>   int state, timestamping;
>   char iface[IFNAMSIZ];
> + struct clock *clock;
> + struct port *port;
> + unsigned int i;
>  
>   while (1) {
>   if (!is_running())
> @@ -866,20 +868,21 @@ static int auto_init_ports(struct phc2sys_private 
> *priv, int add_rt)
>   return -1;
>   }
>  
> - res = pmc_agent_subscribe(priv->node, 1000);
> - if (res) {
> + err = pmc_agent_subscribe(priv->node, 1000);
> + if (err) {
>   pr_err("failed to subscribe");
>   return -1;
>   }
>  
>   for (i = 1; i <= number_ports; i++) {
> - res = run_pmc_port_properties(priv->node, 1000, i, ,
> -   , iface);
> - if (res == -1) {
> + err = pmc_agent_query_port_properties(priv->node, 1000, i,
> +   , ,
> +   iface);
> + if (err == -ENODEV) {
>   /* port does not exist, ignore the port */
>   continue;
>   }
> - if (res <= 0) {
> + if (err) {
>   pr_err("failed to get port properties");
>   return -1;
>   }
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 5e066ac..f5ce59d 100644
> --- a/pmc_agent.c
> +++ 

Re: [Linuxptp-devel] [PATCH 1/4] pmc_agent: Convert the method that queries TAI-UTC offset into the canonical form.

2020-11-30 Thread Jacob Keller



On 11/28/2020 9:08 AM, Richard Cochran wrote:
> This patch renames the function to have the module prefix and corrects the
> return code semantics.
> 
> The active word in the function's name is "query" rather that "get" in
> order to distinguish methods that send and receive over the network
> from those that merely return a cached value.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

I appreciate the function rename too, it seems a bit more clear. (Plus,
if this were a project with many patches in flight, renaming the
function helps prevent potential non-text merge conflicts where a new
user was added but didn't update with the new return semantics).

> ---
>  phc2sys.c   |  8 +++
>  pmc_agent.c | 63 +++--
>  pmc_agent.h | 16 --
>  3 files changed, 50 insertions(+), 37 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index 999d20e..725a25c 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -720,7 +720,7 @@ static int do_loop(struct phc2sys_private *priv, int 
> subscriptions)
>   if (priv->state_changed) {
>   /* force getting offset, as it may have
>* changed after the port state change */
> - if (run_pmc_get_utc_offset(priv->node, 1000) <= 
> 0) {
> + if (pmc_agent_query_utc_offset(priv->node, 
> 1000)) {
>   pr_err("failed to get UTC offset");
>   continue;
>   }
> @@ -910,7 +910,7 @@ static int auto_init_ports(struct phc2sys_private *priv, 
> int add_rt)
>   }
>  
>   /* get initial offset */
> - if (run_pmc_get_utc_offset(priv->node, 1000) <= 0) {
> + if (pmc_agent_query_utc_offset(priv->node, 1000)) {
>   pr_err("failed to get UTC offset");
>   return -1;
>   }
> @@ -1305,8 +1305,8 @@ int main(int argc, char *argv[])
>   }
>  
>   if (!priv.forced_sync_offset) {
> - r = run_pmc_get_utc_offset(priv.node, 1000);
> - if (r <= 0) {
> + r = pmc_agent_query_utc_offset(priv.node, 1000);
> + if (r) {
>   pr_err("failed to get UTC offset");
>   goto end;
>   }
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 22af306..5e066ac 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -228,36 +228,6 @@ int run_pmc_wait_sync(struct pmc_agent *node, int 
> timeout)
>   }
>  }
>  
> -int run_pmc_get_utc_offset(struct pmc_agent *node, int timeout)
> -{
> - struct ptp_message *msg;
> - int res;
> - struct timePropertiesDS *tds;
> -
> - res = run_pmc(node, timeout, TLV_TIME_PROPERTIES_DATA_SET, );
> - if (res <= 0)
> - return res;
> -
> - tds = (struct timePropertiesDS *) management_tlv_data(msg);
> - if (tds->flags & PTP_TIMESCALE) {
> - node->sync_offset = tds->currentUtcOffset;
> - if (tds->flags & LEAP_61)
> - node->leap = 1;
> - else if (tds->flags & LEAP_59)
> - node->leap = -1;
> - else
> - node->leap = 0;
> - node->utc_offset_traceable = tds->flags & UTC_OFF_VALID &&
> -  tds->flags & TIME_TRACEABLE;
> - } else {
> - node->sync_offset = 0;
> - node->leap = 0;
> - node->utc_offset_traceable = 0;
> - }
> - msg_put(msg);
> - return 1;
> -}
> -
>  int run_pmc_get_number_ports(struct pmc_agent *node, int timeout)
>  {
>   struct ptp_message *msg;
> @@ -376,6 +346,37 @@ int pmc_agent_get_sync_offset(struct pmc_agent *agent)
>   return agent->sync_offset;
>  }
>  
> +int pmc_agent_query_utc_offset(struct pmc_agent *node, int timeout)
> +{
> + struct timePropertiesDS *tds;
> + struct ptp_message *msg;
> + int res;
> +
> + res = run_pmc(node, timeout, TLV_TIME_PROPERTIES_DATA_SET, );
> + if (is_run_pmc_error(res)) {
> + return run_pmc_err2errno(res);
> + }
> +
> + tds = (struct timePropertiesDS *) management_tlv_data(msg);
> + if (tds->flags & PTP_TIMESCALE) {
> + node->sync_offset = tds->currentUtcOffset;
> + if (tds->flags & LEAP_61)
> +  

Re: [Linuxptp-devel] [PATCH 1/7] Introduce error codes for the run_pmc method.

2020-11-11 Thread Jacob Keller



On 11/11/2020 10:50 AM, Richard Cochran wrote:
> On Wed, Nov 11, 2020 at 10:35:20AM -0800, Jacob Keller wrote:
>> Thoughts on making this an enum instead so that it's even more clear
>> from the function signatures that this is not an integer return code?
>> The run_pmc function is static so it would only affect callers in this file.
> 
> I considered an enum, but that meant churn on all the callers due to
> the change in return type of run_pmc().
> 
> At the end, I want to have run_pmc return errno codes.
> 
> Thanks,
> Richard
> 

Makes sense. Having run_pmc just return error codes or zero makes more
sense in the long run, so the enumeration isn't really necessary. This
is especially true since the return values really are one success and 3
possible failures, where we pretty much just forward the error value out.

It might be different if many of the callers had to switch on the return
and do things differently based on which return value.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH] Avoid setting clock frequency when free running.

2020-11-11 Thread Jacob Keller



On 11/9/2020 8:14 AM, Richard Cochran wrote:
> On Mon, Nov 09, 2020 at 10:12:58AM +0100, Miroslav Lichvar wrote:
>>
>> Makes sense to me, but maybe it's time to drop the workaround? Looking
>> at the git log, it was added in 2013. Are people still using linuxptp
>> on kernels older than that?
> 
> Yes, I think so.
> 
> When I was collaborating with Balint on his ts2phc not too long ago,
> someone from Intel was asking to please make sure it still works on
> Linux v3.0.
> 
> Thanks,
> Richard
> 

I know we get requests and reports from a few folks who end up sticking
to the same release for a long time.

If we were going to change where LinuxPTP is supported, I think we would
want to do so in some staged manner where we produce deprecation
warnings in one release indicating that support will be dropped for
older kernels, and only drop support in a later release after we've
given some time for folks to (hopefully) be made aware of this fact.

Of course, we should weigh the cost of maintaining the code vs the cost
of headaches when someone uses the program and it doesn't work quite
right. Special care should be taken when failure is non-obvious, and the
application doesn't fail immediately when it isn't working.

In this case, the issue is that our attempt to read the frequency may
not give us valid results. If it fails, then we'll mistakenly assume an
incorrect frequency. This will cause the programs calculations of
frequency drift to be incorrect, resulting in non-optimal adjustments
especially early on. Because this type of failure does not result in an
immediate and obvious failure, I think we ought to be more careful in
handling this work around.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 4/7] pmc_agent: Simplify logic in update method.

2020-11-11 Thread Jacob Keller



On 11/10/2020 2:21 PM, Richard Cochran wrote:
> If the pmc pointer is not set, then there is no need to read the time only
> to later discard the result.  This patch simplifies the flow by returning
> early if there is no work to be done.
> 
> Signed-off-by: Richard Cochran 


Makes sense.

Reviewed-by: Jacob Keller 

> ---
>  pmc_agent.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 24587b4..47562bc 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -342,14 +342,16 @@ int update_pmc_node(struct pmc_agent *node)
>   struct timespec tp;
>   uint64_t ts;
>  
> + if (!node->pmc) {
> + return 0;
> + }
>   if (clock_gettime(CLOCK_MONOTONIC, )) {
>   pr_err("failed to read clock: %m");
>   return -1;
>   }
>   ts = tp.tv_sec * NS_PER_SEC + tp.tv_nsec;
>  
> - if (node->pmc &&
> - !(ts > node->pmc_last_update &&
> + if (!(ts > node->pmc_last_update &&
> ts - node->pmc_last_update < PMC_UPDATE_INTERVAL)) {
>   if (node->subscription_active) {
>   renew_subscription(node, 0);
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 3/7] pmc_agent: Simplify the update method.

2020-11-11 Thread Jacob Keller



On 11/10/2020 2:21 PM, Richard Cochran wrote:
> The main method that causes the PMC agent to update its status takes a flag
> that results in different behavior when push notifications are active.
> This patch simplifies the interface by letting the agent remember whether
> or not the caller subscribed to the notifications in the first place.
> 
> Signed-off-by: Richard Cochran 

Makes sense. I definitely like dropping the on/off parameter. It makes
the API between the callers easier to follow.

> ---
>  phc2sys.c   |  6 --
>  pmc_agent.c | 32 
>  pmc_agent.h |  2 +-
>  3 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index d758aca..436ccea 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -673,7 +673,7 @@ static int do_pps_loop(struct phc2sys_private *priv, 
> struct clock *clock,
>   pps_offset = pps_ts - phc_ts;
>   }
>  
> - if (update_pmc_node(priv->node, 0) < 0)
> + if (update_pmc_node(priv->node) < 0)
>   continue;
>   update_clock(priv, clock, pps_offset, pps_ts, -1);
>   }
> @@ -710,8 +710,10 @@ static int do_loop(struct phc2sys_private *priv, int 
> subscriptions)
>  
>   while (is_running()) {
>   clock_nanosleep(CLOCK_MONOTONIC, 0, , NULL);
> - if (update_pmc_node(priv->node, subscriptions) < 0)
> +
> + if (update_pmc_node(priv->node) < 0) {
>   continue;
> + }

Ok, so now the node/agent tracks the subscriptions automatically. So
someone else needs to call renew_subscription first.

Patch context alone isn't enough to follow where that happens. We pass
subscriptions to do_loop. Is this still used? No longer necessary? hmm.

>  
>   if (subscriptions) {
>   run_pmc_events(priv->node);
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 714c5c5..24587b4 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -42,6 +42,7 @@ struct pmc_agent {
>   int clock_identity_set;
>   int leap;
>   int pmc_ds_requested;
> + int subscription_active;
>   int sync_offset;
>   int utc_offset_traceable;
>  
> @@ -188,6 +189,19 @@ static int run_pmc(struct pmc_agent *node, int timeout, 
> int ds_id,
>   }
>  }
>  
> +static int renew_subscription(struct pmc_agent *node, int timeout)
> +{
> + struct ptp_message *msg;
> + int res;
> +
> + res = run_pmc(node, timeout, TLV_SUBSCRIBE_EVENTS_NP, );
> + if (is_run_pmc_error(res)) {
> + return run_pmc_err2errno(res);
> + }
> + msg_put(msg);
> + return 0;
> +}
> +
>  int run_pmc_wait_sync(struct pmc_agent *node, int timeout)
>  {
>   struct ptp_message *msg;
> @@ -323,7 +337,7 @@ int run_pmc_clock_identity(struct pmc_agent *node, int 
> timeout)
>  }
>  
>  /* Returns: -1 in case of error, 0 otherwise */
> -int update_pmc_node(struct pmc_agent *node, int subscribe)
> +int update_pmc_node(struct pmc_agent *node)
>  {
>   struct timespec tp;
>   uint64_t ts;
> @@ -337,8 +351,9 @@ int update_pmc_node(struct pmc_agent *node, int subscribe)
>   if (node->pmc &&
>   !(ts > node->pmc_last_update &&
> ts - node->pmc_last_update < PMC_UPDATE_INTERVAL)) {
> - if (subscribe)
> - pmc_agent_subscribe(node, 0);
> + if (node->subscription_active) {
> + renew_subscription(node, 0);
> + }
>   if (run_pmc_get_utc_offset(node, 0) > 0)
>   node->pmc_last_update = ts;
>   }
> @@ -393,15 +408,8 @@ void pmc_agent_set_sync_offset(struct pmc_agent *agent, 
> int offset)
>  
>  int pmc_agent_subscribe(struct pmc_agent *node, int timeout)
>  {
> - struct ptp_message *msg;
> - int res;
> -
> - res = run_pmc(node, timeout, TLV_SUBSCRIBE_EVENTS_NP, );
> - if (is_run_pmc_error(res)) {
> - return run_pmc_err2errno(res);
> - }
> - msg_put(msg);
> - return 0;
> + node->subscription_active = 1;
> + return renew_subscription(node, timeout);
>  }
>  
>  bool pmc_agent_utc_offset_traceable(struct pmc_agent *agent)
> diff --git a/pmc_agent.h b/pmc_agent.h
> index 551c4a5..e4d3c3c 100644
> --- a/pmc_agent.h
> +++ b/pmc_agent.h
> @@ -33,7 +33,7 @@ typedef int pmc_node_recv_subscribed_t(void *context, 
> struct ptp_message *msg,
>  
>  int init_pmc_node(struct config *cfg, struct pmc_agent *agent, const char 
> *uds,
> pmc_node_recv_subscribed_t *recv_subscribed, void *context);
> -int update_pmc_node(struct pmc_agent *agent, int subscribe);
> +int update_pmc_node(struct pmc_agent *agent);
>  int run_pmc_clock_identity(struct pmc_agent *agent, int timeout);
>  int run_pmc_wait_sync(struct pmc_agent *agent, int timeout);
>  int run_pmc_get_number_ports(struct pmc_agent *agent, int timeout);
> 


___
Linuxptp-devel mailing list

Re: [Linuxptp-devel] [PATCH 2/7] pmc_agent: Convert the subscribe method into the canonical form.

2020-11-11 Thread Jacob Keller



On 11/10/2020 2:21 PM, Richard Cochran wrote:
> This patch renames the function to have the module prefix and corrects the
> return code semantics.
> 
> Signed-off-by: Richard Cochran 

This looks good to me.

Reviewed-by: Jacob Keller 

> ---
>  phc2sys.c   |  4 ++--
>  pmc_agent.c | 48 +++-
>  pmc_agent.h |  9 -
>  3 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index 7b488ba..d758aca 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -864,8 +864,8 @@ static int auto_init_ports(struct phc2sys_private *priv, 
> int add_rt)
>   return -1;
>   }
>  
> - res = run_pmc_subscribe(priv->node, 1000);
> - if (res <= 0) {
> + res = pmc_agent_subscribe(priv->node, 1000);
> + if (res) {
>   pr_err("failed to subscribe");
>   return -1;
>   }
> diff --git a/pmc_agent.c b/pmc_agent.c
> index d28f3b6..714c5c5 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -17,6 +17,7 @@
>   * with this program; if not, write to the Free Software Foundation, Inc.,
>   * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>   */
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -98,6 +99,26 @@ static int get_mgt_err_id(struct ptp_message *msg)
>  #define RUN_PMC_BADMSG   -1
>  #define RUN_PMC_INTR -2
>  
> +static bool is_run_pmc_error(int code)
> +{
> + return code != RUN_PMC_OKAY;
> +}
> +
> +static int run_pmc_err2errno(int code)
> +{
> + switch (code) {
> + case RUN_PMC_TMO:
> + return -ETIMEDOUT;
> + case RUN_PMC_BADMSG:
> + return -EBADMSG;
> + case RUN_PMC_INTR:
> + return -EINTR;
> + case RUN_PMC_OKAY:
> + default:
> + return 0;
> + }
> +}
> +
>  static int run_pmc(struct pmc_agent *node, int timeout, int ds_id,
>  struct ptp_message **msg)
>  {
> @@ -239,18 +260,6 @@ int run_pmc_get_number_ports(struct pmc_agent *node, int 
> timeout)
>   return res;
>  }
>  
> -int run_pmc_subscribe(struct pmc_agent *node, int timeout)
> -{
> - struct ptp_message *msg;
> - int res;
> -
> - res = run_pmc(node, timeout, TLV_SUBSCRIBE_EVENTS_NP, );
> - if (res <= 0)
> - return res;
> - msg_put(msg);
> - return 1;
> -}
> -
>  void run_pmc_events(struct pmc_agent *node)
>  {
>   struct ptp_message *msg;
> @@ -329,7 +338,7 @@ int update_pmc_node(struct pmc_agent *node, int subscribe)
>   !(ts > node->pmc_last_update &&
> ts - node->pmc_last_update < PMC_UPDATE_INTERVAL)) {
>   if (subscribe)
> - run_pmc_subscribe(node, 0);
> + pmc_agent_subscribe(node, 0);
>   if (run_pmc_get_utc_offset(node, 0) > 0)
>   node->pmc_last_update = ts;
>   }
> @@ -382,6 +391,19 @@ void pmc_agent_set_sync_offset(struct pmc_agent *agent, 
> int offset)
>   agent->sync_offset = offset;
>  }
>  
> +int pmc_agent_subscribe(struct pmc_agent *node, int timeout)
> +{
> + struct ptp_message *msg;
> + int res;
> +
> + res = run_pmc(node, timeout, TLV_SUBSCRIBE_EVENTS_NP, );
> + if (is_run_pmc_error(res)) {
> + return run_pmc_err2errno(res);
> + }
> + msg_put(msg);
> + return 0;
> +}
> +
>  bool pmc_agent_utc_offset_traceable(struct pmc_agent *agent)
>  {
>   return agent->utc_offset_traceable;
> diff --git a/pmc_agent.h b/pmc_agent.h
> index f3a26fe..551c4a5 100644
> --- a/pmc_agent.h
> +++ b/pmc_agent.h
> @@ -34,7 +34,6 @@ typedef int pmc_node_recv_subscribed_t(void *context, 
> struct ptp_message *msg,
>  int init_pmc_node(struct config *cfg, struct pmc_agent *agent, const char 
> *uds,
> pmc_node_recv_subscribed_t *recv_subscribed, void *context);
>  int update_pmc_node(struct pmc_agent *agent, int subscribe);
> -int run_pmc_subscribe(struct pmc_agent *agent, int timeout);
>  int run_pmc_clock_identity(struct pmc_agent *agent, int timeout);
>  int run_pmc_wait_sync(struct pmc_agent *agent, int timeout);
>  int run_pmc_get_number_ports(struct pmc_agent *agent, int timeout);
> @@ -78,6 +77,14 @@ int pmc_agent_get_sync_offset(struct pmc_agent *agent);
>   */
>  void pmc_agent_set_sync_offset(struct pmc_agent *agent, int offset);
>  
> +/**
> + * Subscribes to push notifications of changes in port state.
> + * @param agent  Pointer to a PMC instance obtained via @ref 
> pmc_agent_create().
> + * @param timeout  Transmit timeout in millis

Re: [Linuxptp-devel] [PATCH 1/7] Introduce error codes for the run_pmc method.

2020-11-11 Thread Jacob Keller



On 11/10/2020 2:21 PM, Richard Cochran wrote:
> The run_pmc function is used by several of the PMC agent methods, but it
> breaks the pattern of returning zero on success.  However, the user facing
> PMC agent methods will need to conform to the return code convention used
> throughout the stack.
> 
> In order to migrate to proper return codes, this patch replaces the hard
> coded result values with macros so that the interface methods can translate
> them to the required semantics of zero on success.
> 
> Signed-off-by: Richard Cochran 
> ---
>  pmc_agent.c | 19 +--
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 6e9c023..d28f3b6 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -93,12 +93,11 @@ static int get_mgt_err_id(struct ptp_message *msg)
>   return mgt->id;
>  }
>  
> -/* Return values:
> - * 1: success
> - * 0: timeout
> - * -1: error reported by the other side
> - * -2: local error, fatal
> - */
> +#define RUN_PMC_OKAY  1
> +#define RUN_PMC_TMO   0
> +#define RUN_PMC_BADMSG   -1
> +#define RUN_PMC_INTR -2
> +

Thoughts on making this an enum instead so that it's even more clear
from the function signatures that this is not an integer return code?
The run_pmc function is static so it would only affect callers in this file.

Either way, this is a marked improvement over leaving the values as raw
numbers.

>  static int run_pmc(struct pmc_agent *node, int timeout, int ds_id,
>  struct ptp_message **msg)
>  {
> @@ -115,12 +114,12 @@ static int run_pmc(struct pmc_agent *node, int timeout, 
> int ds_id,
>   cnt = poll(pollfd, N_FD, timeout);
>   if (cnt < 0) {
>   pr_err("poll failed");
> - return -2;
> + return RUN_PMC_INTR;
>   }
>   if (!cnt) {
>   /* Request the data set again in the next run. */
>   node->pmc_ds_requested = 0;
> - return 0;
> + return RUN_PMC_TMO;
>   }
>  
>   /* Send a new request if there are no pending messages. */
> @@ -154,7 +153,7 @@ static int run_pmc(struct pmc_agent *node, int timeout, 
> int ds_id,
>   res = is_msg_mgt(*msg);
>   if (res < 0 && get_mgt_err_id(*msg) == ds_id) {
>   node->pmc_ds_requested = 0;
> - return -1;
> + return RUN_PMC_BADMSG;
>   }
>   if (res <= 0 ||
>   node->recv_subscribed(node->recv_context, *msg, ds_id) ||
> @@ -164,7 +163,7 @@ static int run_pmc(struct pmc_agent *node, int timeout, 
> int ds_id,
>   continue;
>   }
>   node->pmc_ds_requested = 0;
> - return 1;
> + return RUN_PMC_OKAY;
>   }
>  }
>  
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 0/7] PMC Agent - Part II

2020-11-11 Thread Jacob Keller



On 11/10/2020 2:21 PM, Richard Cochran wrote:
> When you start pulling on a thread ...
> 

Indeed!

> Looking into the PMC agent code, there are issues that will need
> resolution.  This series converts the subscribe and update methods
> into the canonical form.
> 
> The first patch prepares the way for implementing consistent return
> value semantics.
> 
> The second patch renames the subscribe method, which is a
> straightforward conversion.
> 
> The remaining five patches deal with the update method.  Even with the
> simplifications, still the end result seems suspicious, because when
> writing the documentation, it is hard to put purpose of this method
> into words.  This method calls into run_pmc, as do most of the
> remaining methods to be converted, and that function is fairly
> complex.  It remains to be seen whether it can be disentangled into a
> more coherent form.
> 
> So this series is only Part II of an undetermined number of
> installments...
> 

Hah. Well I look forward to an undetermined number of sequels.

> 
> Richard Cochran (7):
>   Introduce error codes for the run_pmc method.
>   pmc_agent: Convert the subscribe method into the canonical form.
>   pmc_agent: Simplify the update method.
>   pmc_agent: Simplify logic in update method.
>   pmc_agent: Simplify logic in update method even more.
>   pmc_agent: Simplify update method yet again.
>   pmc_agent: Rename the update method and attempt to document it.
> 
>  phc2sys.c   |  10 +++--
>  pmc_agent.c | 122 
>  pmc_agent.h |  26 ++-
>  3 files changed, 106 insertions(+), 52 deletions(-)
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 2/5] pmc_agent: Rename pmc_node to something more descriptive.

2020-11-11 Thread Jacob Keller



On 11/9/2020 3:06 PM, Richard Cochran wrote:
> On Tue, Nov 10, 2020 at 12:54:16AM +0200, Vladimir Oltean wrote:
>> It will remain named "node" though?
> 
> No, it will change, too.  I just want to avoid having too much churn
> in a single patch...  Part II will finish the job...
> 
> Thanks,
> Richard

Thanks. Slowing this process a little helps aid reviewing!


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH RFC 0/1] Introduce inclusive terminology

2020-10-27 Thread Jacob Keller



On 10/27/2020 4:26 PM, Richard Cochran wrote:
> On Mon, Aug 24, 2020 at 09:41:06AM +0200, Miroslav Lichvar wrote:
>> It seems you are set on this terminology. I think it would work for
>> me, although in my head I mostly see the packets on the network
>> instead of a time signal. Have you considered adopting a server/client
>> terminology like NTP is using? I know some people that use it with PTP
>> and maybe it would be easier for a wider adoption.
> 
> I considered that, but ...
> 
> How would you feel about client/server terminology in the context of
> phc2sys?
> 
> For example, you would have to call eth0 the "server" and
> CLOCK_REALTIME the "client".
> 
> Thoughts?
> 
> Richard

For myself, I would be reasonably ok with this, but I do prefer the
source/sink terminology overall because it feels more accurate to this
relationship.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC index from device name if possible

2020-10-02 Thread Jacob Keller



On 8/12/2020 2:12 PM, Keller, Jacob E wrote:
>> -Original Message-
>> From: Richard Cochran 
>> Sent: Wednesday, August 12, 2020 8:40 AM
>> To: Keller, Jacob E 
>> Cc: linuxptp-devel@lists.sourceforge.net
>> Subject: Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC
>> index from device name if possible
>>
>> On Wed, Aug 05, 2020 at 04:12:02PM -0700, Jacob Keller wrote:
>>> Here, we are making the implicit assumption that all ptp clock devices
>>> will always have /dev/ptpX format. I don't think anyone is crazy enough
>>> to rename these devices...
>>
>> (Not yet, anyways  ;^)
>>
>>> An alternative (requiring kernel implementation maybe?) would be to read
>>> the phc index from the kernel somehow. It doesn't look like this is
>>> exported anywhere else besides the name.
>>
>> How about /sys/class/ptp/ptpX/clock_index ?
>>
>> Is that something you would like to code up for the lkml?
>>
>> Thanks,
>> Richard
> 
> Yea, I can do that.
> 
> -Jake
>

I hadn't forgotten this. However, it turns out that this isn't a huge
value. Recent versions of udev don't seem to support re-naming devices
other than networking devices. For this reason, the primary way you
might "change" the name is a symlink device.

However, that won't impact the sysfs tree, and thus you can't actually
determine the clock index from the symlink device on its own.

I suppose if someone were to change how the device name was generated in
the kernel they could break this, but I tend to think that isn't really
a case we should support.

Thus.. I think this turns out not to be super valuable... I'm sending
the patch as RFC to the netdev list either way for some further
discussion, as it is quite trivial.

https://lore.kernel.org/netdev/20201002233743.1688517-1-jacob.e.kel...@intel.com/T/#u


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 4/8] ts2phc: nmea: Add a configuration option for the current leap seconds file.

2020-09-21 Thread Jacob Keller



On 9/21/2020 11:58 AM, Richard Cochran wrote:
> The PTP time scale is TAI, and a PTP GM needs to provide it.  However, most
> GPS devices only provide UTC time of day information, and they do not, in
> general, offer any kind of reliable, standardized leap seconds data.  After
> all, this information is only broadcast every 12.5 minutes through the GPS
> network, and that is far too long for a PTP GM to wait after startup.
> 
> This patch allows the built in leap seconds table to be overridden by an
> up to date file provided in the environment.
> 
> Signed-off-by: Richard Cochran 
> ---
>  config.c | 1 +
>  ts2phc.8 | 8 
>  ts2phc_nmea_master.c | 2 +-
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/config.c b/config.c
> index d3446b4..d237de9 100644
> --- a/config.c
> +++ b/config.c
> @@ -260,6 +260,7 @@ struct config_item config_tab[] = {
>   PORT_ITEM_INT("inhibit_multicast_service", 0, 0, 1),
>   GLOB_ITEM_INT("initial_delay", 0, 0, INT_MAX),
>   GLOB_ITEM_INT("kernel_leap", 1, 0, 1),
> + GLOB_ITEM_STR("leapfile", NULL),
>   PORT_ITEM_INT("logAnnounceInterval", 1, INT8_MIN, INT8_MAX),
>   PORT_ITEM_INT("logMinDelayReqInterval", 0, INT8_MIN, INT8_MAX),
>   PORT_ITEM_INT("logMinPdelayReqInterval", 0, INT8_MIN, INT8_MAX),
> diff --git a/ts2phc.8 b/ts2phc.8
> index 07a4026..77f8940 100644
> --- a/ts2phc.8
> +++ b/ts2phc.8
> @@ -116,6 +116,14 @@ This option can be useful in test scenarios, for example 
> to determine
>  how well synchronized a group of local clocks are to each other.
>  The default is 0 (adjust the slave clocks).
>  .TP
> +.B leapfile
> +The path to the current leap seconds definition file.
> +In a Debian system this file is provided by the tzdata package and can
> +be found at /usr/share/zoneinfo/leap-seconds.list.
> +The default is an empty string, which causes the program to use a hard
> +coded table that reflects the known leap seconds on the date of the
> +software's release.
> +.TP
>  .B logging_level
>  The maximum logging level of messages which should be printed.
>  The default is 6 (LOG_INFO).
> diff --git a/ts2phc_nmea_master.c b/ts2phc_nmea_master.c
> index b8f7014..76fc7ae 100644
> --- a/ts2phc_nmea_master.c
> +++ b/ts2phc_nmea_master.c
> @@ -210,8 +210,8 @@ static int ts2phc_nmea_master_getppstime(struct 
> ts2phc_master *master,
>  
>  struct ts2phc_master *ts2phc_nmea_master_create(struct config *cfg, const 
> char *dev)
>  {
> + const char *leapfile = config_get_string(cfg, NULL, "leapfile");
>   struct ts2phc_nmea_master *master;
> - const char *leapfile = NULL;// TODO - read from config.
>   int err;
>  

Hah, all of the code to read leapfiles is here, but just wasn't exposed yet.

Nice to see this!

Thanks,
Jake

>   master = calloc(1, sizeof(*master));
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH RFC 0/1] Introduce inclusive terminology

2020-08-20 Thread Jacob Keller



On 8/20/2020 2:16 PM, Vladimir Oltean wrote:
> On Thu, Aug 20, 2020 at 11:07:09AM -0700, Richard Cochran wrote:
>> On Thu, Aug 20, 2020 at 06:45:37PM +0300, Vladimir Oltean wrote:
>>
>>> What if the terms on which IEEE 1588 settles are not the terms in your
>>> mind ("source"/"sink")?
>>
>> Because linuxptp is widely deployed, what we do could very well
>> influence the committee work.
>>
> 
> Aren't there simpler ways to influence the 1588 working groups, like
> sending them a direct email? I don't think a significant number of them
> are subscribed to linuxptp-devel@lists.sourceforge.net.
> Which begs the question, if you already had your mind set on a
> particular list of euphemisms, and you weren't actually looking for a
> second opinion from this group, why send this RFC patch at all? You can
> commit changes to linuxptp without peer review at any time.
> 
> Also (and this might be not for me to understand), if you are:
> 
>> doubtful whether changing the wording of the 1588 standard will
>> actually have a positive effect on the world or not
> 
> then why pressure them in the first place to make this change?
> 

The expectation is that the standards committee will change the terms
one way or another in the future.

If we assume that the terms master and slave will be removed, and do not
like many of the alternative proposals, it makes sense to try and
influence them to choose proposals we do find more suitable.

Doing nothing may increase the probability that the standards committee
chooses terms that Richard clearly dislikes: (leader/follower, etc).

> Thanks,
> -Vladimir
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH RFC 0/1] Introduce inclusive terminology

2020-08-18 Thread Jacob Keller



On 8/18/2020 1:43 PM, Richard Cochran wrote:
> There is an industry wide effort underway to replace historically and
> culturally loaded terms like master/slave with neutral alternatives.
> The IEEE 1588 committee will most likely amend the standard, but so
> far no consensus on the new terminology has been reached.
> 
> Most of the proposed alternative terms are, IMHO, either awful
> sounding or just plain silly.  There is a window of opportunity for
> this project to take the lead in recommending terminology that is, at
> the same time, both culturally neutral and technically more accurate.
> 
> The original designation of the PTP port roles made little sense in
> the first place.  Under the institution of slavery, the role of a
> slave is to perform work for the master.  In a PTP network it is the
> "master" port that serves the slaves, the opposite of what the terms
> suggest.
> 
> The information flowing through a PTP network may best be described as
> a time signal.  As any EE will tell you, a signal flows from its
> source to one or more sinks.  Thus we can trace the time signal in a
> PTP network as it flows from a given time source to a time sink.
> 
> The approach I'm considering is to start today with the human readable
> program help, after that the man pages, and later on the identifiers
> in the program.  With very few exceptions, none of the re-naming would
> impact any existing user configuration scripts.  We will take care not
> to cause issues for the myriad deployments of this software world wide.
> 

Yep, makes sense. Long term, after changing the stuff which doesn't
impact config, we can work towards finding a way to deprecate and rename
config options in a way that won't break existing deployment. I'm
personally Ok with simply leaving the original name as a deprecated
alternative name with an explanation.

> Thanks,
> Richard
> 
> 
> Richard Cochran (1):
>   Convert usage messages to time source/sink terminology.
> 
>  phc2sys.c | 10 +-
>  ptp4l.c   |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 

This gets a +1 from me!

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC index from device name if possible

2020-08-17 Thread Jacob Keller



On 8/12/2020 2:40 PM, Keller, Jacob E wrote:
> 
> 
>> -Original Message-
>> From: Richard Cochran 
>> Sent: Wednesday, August 12, 2020 8:40 AM
>> To: Keller, Jacob E 
>> Cc: linuxptp-devel@lists.sourceforge.net
>> Subject: Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC
>> index from device name if possible
>>
>> On Wed, Aug 05, 2020 at 04:12:02PM -0700, Jacob Keller wrote:
>>> Here, we are making the implicit assumption that all ptp clock devices
>>> will always have /dev/ptpX format. I don't think anyone is crazy enough
>>> to rename these devices...
>>
>> (Not yet, anyways  ;^)
>>
>>> An alternative (requiring kernel implementation maybe?) would be to read
>>> the phc index from the kernel somehow. It doesn't look like this is
>>> exported anywhere else besides the name.
>>
>> How about /sys/class/ptp/ptpX/clock_index ?
>>
>> Is that something you would like to code up for the lkml?
>>
>> Thanks,
>> Richard
> 
> To make this fully work we'll probably want to adjust the LinuxPTP code base 
> with a phc_index_to_dev function that scans the sys/class/ptp folder for the 
> matching clock based on the index to obtain the name. I'll see about adding 
> that as well (with a fallback to attempting /dev/ptpX if clock_index doesn't 
> exist)
> 
> Thanks,
> Jake
> 

So I looked at this, and implementing the kernel side is trivial (less
than a dozen lines of code).

However, I'm not sure how valuable it would be as a sysfs thing.

So, there are two ways to rename devices (besides modifying the kernel
to use a different naming scheme) in udev. We can either totally rename
them, or provide a symlink alias.

It looks like recent-ish versions of udev don't support renaming
non-network devices. However, symlinks still work.

If we know the actual kernel device name, then looking up the
clock_index via the sysfs works: (take the kernel name and append it to
/sys/class/ptp/ to get the folder).

But if we just have a symlink, I'm not sure how the application can get
back to the original kernel name?

I am also not sure if udev device rename actually changes the folder
names in sysfs or not. I suspect it does not, but can't confirm because
I've only got systems with newer udev that do not allow renaming other
types of devices.

I do think having clock_index exposed is valuable still, since it
provides a more guaranteed method of matching the index to the device.
It solves the case where the device has been renamed on the kernel side
at least.

If we also exposed the clock index via something like get_caps then it
would work from the device node as an ioctl.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH 15/15] ts2phc_phc_master: make use of new kernel API for perout waveform

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> This API was introduced for 2 reasons:
> 
> 1. Some hardware can emit PPS signals but not starting from arbitrary
>absolute times, but rather phase-aligned to the beginning of a
>second. We _could_ patch ts2phc to always specify a start time of
>0.0 to PTP_PEROUT_REQUEST, but in practice, we would never
>know whether that would actually work with all in-tree PHC drivers.
>So there was a need for a new flag that only specifies the phase of
>the periodic signal, and not the absolute start time.
> 
> 2. Some hardware can, rather unfortunately, not distinguish between a
>rising and a falling extts edge. And, since whatever rises also has
>to fall before rising again, the strategy in ts2phc is to set a
>'large' pulse width (half the period) and ignore the extts event
>corresponding to the mid-way between one second and another. This is
>all fine, but currently, ts2phc.pulsewidth is a read-only property in
>the config file. The kernel is not instructed in any way to use this
>value, it is simply that must be configured based on prior knowledge
>of the PHC's implementation. This API changes that.
> 
> The introduction of a phase adjustment for the master PHC means we have
> to adjust our approximation of the precise perout timestamp. We put that
> code into a common function and convert all call sites to call that. We
> also need to do the same thing for the edge ignoring logic.
> 
> Signed-off-by: Vladimir Oltean 
> ---
>  config.c|  1 +
>  missing.h   | 53 
>  ts2phc.8|  5 +++
>  ts2phc.c| 86 ++---
>  ts2phc.h|  1 +
>  ts2phc_phc_master.c | 20 +--
>  ts2phc_slave.c  | 16 +++--
>  7 files changed, 150 insertions(+), 32 deletions(-)
> 
> diff --git a/config.c b/config.c
> index d3446b4c8f27..a10f30998d10 100644
> --- a/config.c
> +++ b/config.c
> @@ -314,6 +314,7 @@ struct config_item config_tab[] = {
>   GLOB_ITEM_STR("ts2phc.nmea_remote_host", ""),
>   GLOB_ITEM_STR("ts2phc.nmea_remote_port", ""),
>   GLOB_ITEM_STR("ts2phc.nmea_serialport", "/dev/ttyS0"),
> + PORT_ITEM_INT("ts2phc.perout_phase", -1, 0, 9),
>   PORT_ITEM_INT("ts2phc.pin_index", 0, 0, INT_MAX),
>   GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
>   PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
> diff --git a/missing.h b/missing.h
> index 35eaf155fc51..e52915e8b621 100644
> --- a/missing.h
> +++ b/missing.h
> @@ -97,6 +97,59 @@ struct compat_ptp_clock_caps {
>  
>  #endif /*LINUX_VERSION_CODE < 5.8*/
>  
> +/*
> + * Bits of the ptp_perout_request.flags field:
> + */
> +
> +#ifndef PTP_PEROUT_ONE_SHOT
> +#define PTP_PEROUT_ONE_SHOT  (1<<0)
> +#endif
> +
> +#ifndef PTP_PEROUT_DUTY_CYCLE
> +#define PTP_PEROUT_DUTY_CYCLE(1<<1)
> +#endif
> +
> +#ifndef PTP_PEROUT_PHASE
> +#define PTP_PEROUT_PHASE (1<<2)
> +#endif
> +
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(5,9,0)
> +
> +/* from upcoming Linux kernel version 5.9 */
> +struct compat_ptp_perout_request {
> + union {
> + /*
> +  * Absolute start time.
> +  * Valid only if (flags & PTP_PEROUT_PHASE) is unset.
> +  */
> + struct ptp_clock_time start;
> + /*
> +  * Phase offset. The signal should start toggling at an
> +  * unspecified integer multiple of the period, plus this value.
> +  * The start time should be "as soon as possible".
> +  * Valid only if (flags & PTP_PEROUT_PHASE) is set.
> +  */
> + struct ptp_clock_time phase;
> + };
> + struct ptp_clock_time period; /* Desired period, zero means disable. */
> + unsigned int index;   /* Which channel to configure. */
> + unsigned int flags;
> + union {
> + /*
> +  * The "on" time of the signal.
> +  * Must be lower than the period.
> +  * Valid only if (flags & PTP_PEROUT_DUTY_CYCLE) is set.
> +  */
> + struct ptp_clock_time on;
> + /* Reserved for future use. */
> + unsigned int rsv[4];
> + };
> +};
> +
> +#define ptp_perout_request compat_ptp_perout_request
> +
> +#endif /*LINUX_VERSION_CODE < 5.8*/
> +
>  #ifndef PTP_MAX_SAMPLES
>  #define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement samples. */
>  #endif /* PTP_MAX_SAMPLES */
> diff --git a/ts2phc.8 b/ts2phc.8
> index 07a402601c9e..f690e243864d 100644
> --- a/ts2phc.8
> +++ b/ts2phc.8
> @@ -152,6 +152,11 @@ specified, then the given remote connection will be used 
> in preference
>  to the configured serial port.
>  The default is "/dev/ttyS0".
>  .TP
> +.B ts2phc.perout_phase
> +Configures the offset between the beginning of the second and the PPS
> +master's 

Re: [Linuxptp-devel] [RFC PATCH 09/15] ts2phc_slave: print master offset

2020-08-05 Thread Jacob Keller
On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> Make this information more visible by default, since it is the key
> output of this program.
> 
> Signed-off-by: Vladimir Oltean 

Yep.

Reviewed-by: Jacob Keller 

> ---
>  ts2phc_slave.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ts2phc_slave.c b/ts2phc_slave.c
> index 7a0522fb0f86..566cb14436fb 100644
> --- a/ts2phc_slave.c
> +++ b/ts2phc_slave.c
> @@ -273,8 +273,8 @@ static int ts2phc_slave_event(struct ts2phc_slave *slave,
>   adj = servo_sample(slave->clock->servo, offset, extts_ts,
>  SAMPLE_WEIGHT, >clock->servo_state);
>  
> - pr_debug("%s master offset %10" PRId64 " s%d freq %+7.0f",
> -  slave->name, offset, slave->clock->servo_state, adj);
> + pr_info("%s master offset %10" PRId64 " s%d freq %+7.0f",
> + slave->name, offset, slave->clock->servo_state, adj);
>  
>   switch (slave->clock->servo_state) {
>   case SERVO_UNLOCKED:
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH 08/15] ts2phc: instantiate a pmc node

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> This introduces the '-a' option in ts2phc, an option inspired from
> phc2sys that puts the clocks in "automatic" mode. In this mode, ts2phc
> listens, as a PMC, to port state change events from ptp4l, and detects
> which port state machine, if any, has transitioned to PS_SLAVE. That
> port's clock will become the synchronization master for the hierarchy
> described by ts2phc.
> 
> The use case is a multi-switch DSA setup with boundary_clock_jbod, where
> there is only one grandmaster, connected to one switch's port. The other
> switches, connected together through a PPS signal, must adapt themselves
> to this new source of time, while the switch connected to the GM must
> not be synchronized by ts2phc because it is already synchronized by
> ptp4l.
> 
> Signed-off-by: Vladimir Oltean 
> ---
>  makefile |   3 +-
>  ts2phc.c | 208 ++-
>  ts2phc.h |  11 +++
>  3 files changed, 220 insertions(+), 2 deletions(-)
> 
> diff --git a/makefile b/makefile
> index 27c4d7809553..f9fe12dde170 100644
> --- a/makefile
> +++ b/makefile
> @@ -27,7 +27,8 @@ FILTERS = filter.o mave.o mmedian.o
>  SERVOS   = linreg.o ntpshm.o nullf.o pi.o servo.o
>  TRANSP   = raw.o transport.o udp.o udp6.o uds.o
>  TS2PHC   = ts2phc.o lstab.o nmea.o serial.o sock.o 
> ts2phc_generic_master.o \
> - ts2phc_master.o ts2phc_phc_master.o ts2phc_nmea_master.o ts2phc_slave.o
> + ts2phc_master.o ts2phc_phc_master.o ts2phc_nmea_master.o ts2phc_slave.o \
> + pmc_common.o transport.o msg.o tlv.o uds.o udp.o udp6.o raw.o
>  OBJ  = bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
>   e2e_tc.o fault.o $(FILTERS) fsm.o hash.o interface.o monitor.o msg.o phc.o \
>   port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) \
> diff --git a/ts2phc.c b/ts2phc.c
> index 97e9718c999e..2c8a42c04d0c 100644
> --- a/ts2phc.c
> +++ b/ts2phc.c
> @@ -33,6 +33,91 @@ static void ts2phc_cleanup(struct ts2phc_private *priv)
>   if (priv->cfg) {
>   config_destroy(priv->cfg);
>   }
> + close_pmc_node(>node);
> +}
> +
> +/* FIXME: Copied from phc2sys */

I assume these FIXME comments are intended because you plan to share
these in the future?

> +static int normalize_state(int state)
> +{
> + if (state != PS_MASTER && state != PS_SLAVE &&
> + state != PS_PRE_MASTER && state != PS_UNCALIBRATED) {
> + /* treat any other state as "not a master nor a slave" */
> + state = PS_DISABLED;
> + }
> + return state;
> +}
> +
> +/* FIXME: Copied from phc2sys */
> +static struct port *port_get(struct ts2phc_private *priv, unsigned int 
> number)
> +{
> + struct port *p;
> +
> + LIST_FOREACH(p, >ports, list) {
> + if (p->number == number)
> + return p;
> + }
> + return NULL;
> +}
> +
> +/* FIXME: Copied from phc2sys */
> +static int clock_compute_state(struct ts2phc_private *priv,
> +struct clock *clock)
> +{
> + int state = PS_DISABLED;
> + struct port *p;
> +
> + LIST_FOREACH(p, >ports, list) {
> + if (p->clock != clock)
> + continue;
> + /* PS_SLAVE takes the highest precedence, PS_UNCALIBRATED
> +  * after that, PS_MASTER is third, PS_PRE_MASTER fourth and
> +  * all of that overrides PS_DISABLED, which corresponds
> +  * nicely with the numerical values */
> + if (p->state > state)
> + state = p->state;
> + }
> + return state;
> +}
> +
> +#define node_to_ts2phc(node) \
> + container_of(node, struct ts2phc_private, node)
> +
> +static int ts2phc_recv_subscribed(struct pmc_node *node,
> +   struct ptp_message *msg, int excluded)
> +{
> + struct ts2phc_private *priv = node_to_ts2phc(node);
> + int mgt_id, state;
> + struct portDS *pds;
> + struct port *port;
> + struct clock *clock;
> +
> + mgt_id = get_mgt_id(msg);
> + if (mgt_id == excluded)
> + return 0;
> + switch (mgt_id) {
> + case TLV_PORT_DATA_SET:
> + pds = get_mgt_data(msg);
> + port = port_get(priv, pds->portIdentity.portNumber);
> + if (!port) {
> + pr_info("received data for unknown port %s",
> + pid2str(>portIdentity));
> + return 1;
> + }
> + state = normalize_state(pds->portState);
> + if (port->state != state) {
> + pr_info("port %s changed state",
> + pid2str(>portIdentity));
> + port->state = state;
> + clock = port->clock;
> + state = clock_compute_state(priv, clock);
> + if (clock->state != state || clock->new_state) {
> + 

Re: [Linuxptp-devel] [RFC PATCH 07/15] ts2phc: instantiate a full clock structure for every PHC master

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> This is particularly useful when using the "automatic" mode of ts2phc,
> and the PPS distribution tree is fixed in hardware (as opposed to port
> states).
> 
This refers to work not yet implemented by any of its parent commits. It
might make sense to re-word it to be clear that this is about the future
automatic mode, and why it's important for the case of a fixed tree.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH 07/15] ts2phc: instantiate a full clock structure for every PHC master

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> This propagates the use of "struct ts2phc_private" all the way into the
> master API, in preparation of a new use case that will be supported
> soon: some PPS masters (to be precise, the "PHC" kind) instantiate a
> struct clock which could be disciplined by ts2phc.
> 
> When a PHC A emits a pulse and another PHC B timestamps it, the offset
> between their precise timestamps can be used to synchronize either one
> of them. So far in ts2phc, only the slave PHC (the one using extts) has
> been synchronized to the master (the one using perout).
> 
> This is partly because there is no proper kernel API to report the
> precise timestamp of a perout pulse. We only have the periodic API, and
> that doesn't report precise timestamps either; we just use vague
> approximations of what the PPS master PHC's time was, based on reading
> that PHC immediately after a slave extts event was received by the
> application. While this is far from ideal, it does work, and does allow
> PHC A to be synchronized to B.
> 

Seems like this would be a good place for an extension if HW could
support it? I don't actually know if any existing HW supports this kind
of "create a pulse and timestamp it at the same time" though.

> This is particularly useful when using the "automatic" mode of ts2phc,
> and the PPS distribution tree is fixed in hardware (as opposed to port
> states).
> 
> Signed-off-by: Vladimir Oltean 
> ---
>  ts2phc.c|  2 +-
>  ts2phc_generic_master.c |  2 +-
>  ts2phc_generic_master.h |  3 ++-
>  ts2phc_master.c | 10 ++
>  ts2phc_master.h |  6 --
>  ts2phc_nmea_master.c|  5 +++--
>  ts2phc_nmea_master.h|  3 ++-
>  ts2phc_phc_master.c | 33 +
>  ts2phc_phc_master.h |  3 ++-
>  ts2phc_slave.c  |  2 +-
>  10 files changed, 39 insertions(+), 30 deletions(-)
> 
> diff --git a/ts2phc.c b/ts2phc.c
> index 8a4071d083e3..97e9718c999e 100644
> --- a/ts2phc.c
> +++ b/ts2phc.c
> @@ -260,7 +260,7 @@ int main(int argc, char *argv[])
>   } else {
>   pps_type = TS2PHC_MASTER_PHC;
>   }
> - priv.master = ts2phc_master_create(cfg, pps_source, pps_type);
> + priv.master = ts2phc_master_create(, pps_source, pps_type);
>   if (!priv.master) {
>   fprintf(stderr, "failed to create master\n");
>   ts2phc_cleanup();
> diff --git a/ts2phc_generic_master.c b/ts2phc_generic_master.c
> index ad4f7f1cf1d7..05dd8f3742fc 100644
> --- a/ts2phc_generic_master.c
> +++ b/ts2phc_generic_master.c
> @@ -47,7 +47,7 @@ static int ts2phc_generic_master_getppstime(struct 
> ts2phc_master *m,
>   return 0;
>  }
>  
> -struct ts2phc_master *ts2phc_generic_master_create(struct config *cfg,
> +struct ts2phc_master *ts2phc_generic_master_create(struct ts2phc_private 
> *priv,
>  const char *dev)
>  {
>   struct ts2phc_generic_master *master;
> diff --git a/ts2phc_generic_master.h b/ts2phc_generic_master.h
> index ac0ce4f11cb8..c8fc099ad066 100644
> --- a/ts2phc_generic_master.h
> +++ b/ts2phc_generic_master.h
> @@ -6,9 +6,10 @@
>  #ifndef HAVE_TS2PHC_GENERIC_MASTER_H
>  #define HAVE_TS2PHC_GENERIC_MASTER_H
>  
> +#include "ts2phc.h"
>  #include "ts2phc_master.h"
>  
> -struct ts2phc_master *ts2phc_generic_master_create(struct config *cfg,
> +struct ts2phc_master *ts2phc_generic_master_create(struct ts2phc_private 
> *priv,
>  const char *dev);
>  
>  #endif
> diff --git a/ts2phc_master.c b/ts2phc_master.c
> index 9283580df3fc..4617c4aecbe5 100644
> --- a/ts2phc_master.c
> +++ b/ts2phc_master.c
> @@ -3,25 +3,27 @@
>   * @note Copyright (C) 2019 Richard Cochran 
>   * @note SPDX-License-Identifier: GPL-2.0+
>   */
> +#include "ts2phc.h"
>  #include "ts2phc_generic_master.h"
>  #include "ts2phc_master_private.h"
>  #include "ts2phc_nmea_master.h"
>  #include "ts2phc_phc_master.h"
>  
> -struct ts2phc_master *ts2phc_master_create(struct config *cfg, const char 
> *dev,
> +struct ts2phc_master *ts2phc_master_create(struct ts2phc_private *priv,
> +const char *dev,
>  enum ts2phc_master_type type)
>  {
>   struct ts2phc_master *master = NULL;
>  
>   switch (type) {
>   case TS2PHC_MASTER_GENERIC:
> - master = ts2phc_generic_master_create(cfg, dev);
> + master = ts2phc_generic_master_create(priv, dev);
>   break;
>   case TS2PHC_MASTER_NMEA:
> - master = ts2phc_nmea_master_create(cfg, dev);
> + master = ts2phc_nmea_master_create(priv, dev);
>   break;
>   case TS2PHC_MASTER_PHC:
> - master = ts2phc_phc_master_create(cfg, dev);
> + master = ts2phc_phc_master_create(priv, dev);
>   break;
>   }
>   return master;
> diff --git a/ts2phc_master.h 

Re: [Linuxptp-devel] [RFC PATCH 06/15] ts2phc: instantiate a full clock structure for every slave PHC

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> Slaves in ts2phc are PHC devices that implement the extts kernel API.
> They are slaves just in the sense that they timestamp a pulse emitted by
> somebody else.
> 
> Currently in ts2phc, PPS slaves are also the only candidates for the
> clocks that get synchronized. There are 2 aspects that make this too
> restrictive:
> 
> - Not all PPS slaves may be synchronization slaves. Consider a dynamic
>   environment of multiple DSA switches using boundary_clock_jbod, and
>   only one port is in the PS_SLAVE state. In that case, the clock of
>   that port should be the synchronization master (called the "source
>   clock" from now on, as far as ts2phc is concerned), regardless of
>   whether it supports the extts API or not.
> 
> - Not all synchronization slaves may be PPS slaves. Specifically, the
>   "PHC" type of PPS master in ts2phc can also be, fundamentally,
>   disciplined. The code should be prepared to handle this by recognizing
>   that things that can be disciplined by a servo should be represented
>   by a "struct clock", and things that can timestamp external events
>   should be represented by a "struct ts2phc_slave".
> 

This seems straight forward. Do we have any entity which can timestamp
external events but can't be disciplined by a servo? I don't think we do.

> Signed-off-by: Vladimir Oltean 
> ---
>  ts2phc.c   | 69 +++
>  ts2phc.h   | 26 +++
>  ts2phc_slave.c | 88 ++
>  3 files changed, 134 insertions(+), 49 deletions(-)
> 
> diff --git a/ts2phc.c b/ts2phc.c
> index 0be7b18bda71..8a4071d083e3 100644
> --- a/ts2phc.c
> +++ b/ts2phc.c
> @@ -7,12 +7,18 @@
>   * @note SPDX-License-Identifier: GPL-2.0+
>   */
>  #include 
> +#include 
> +#include 
> +#include 
>  
> +#include "clockadj.h"
>  #include "config.h"
>  #include "interface.h"
> +#include "phc.h"
>  #include "print.h"
>  #include "ts2phc.h"
>  #include "version.h"
> +#include "contain.h"
>  
>  struct interface {
>   STAILQ_ENTRY(interface) list;
> @@ -29,6 +35,69 @@ static void ts2phc_cleanup(struct ts2phc_private *priv)
>   }
>  }
>  
> +struct servo *servo_add(struct ts2phc_private *priv, struct clock *clock)
> +{
> + enum servo_type type = config_get_int(priv->cfg, NULL, "clock_servo");
> + struct servo *servo;
> + 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. */
> + clockadj_set_freq(clock->clkid, fadj);
> +
> + max_adj = phc_max_adj(clock->clkid);
> +
> + servo = servo_create(priv->cfg, type, -fadj, max_adj, 0);
> + if (!servo)
> + return NULL;
> +
> + servo_sync_interval(servo, SERVO_SYNC_INTERVAL);
> +
> + return servo;
> +}
> +
> +struct clock *clock_add(struct ts2phc_private *priv, const char *device)
> +{
> + clockid_t clkid = CLOCK_INVALID;
> + int phc_index = -1;
> + struct clock *c;
> + int err;
> +
> + clkid = posix_clock_open(device, _index);
> + if (clkid == CLOCK_INVALID)
> + return NULL;
> +
> + LIST_FOREACH(c, >clocks, list) {
> + if (c->phc_index == phc_index) {
> + /* Already have the clock, don't add it again */
> + posix_clock_close(clkid);
> + return c;
> + }
> + }
> +
> + c = calloc(1, sizeof(*c));
> + if (!c) {
> + pr_err("failed to allocate memory for a clock");
> + return NULL;
> + }
> + c->clkid = clkid;
> + c->phc_index = phc_index;
> + c->servo_state = SERVO_UNLOCKED;
> + c->servo = servo_add(priv, c);
> + c->no_adj = config_get_int(priv->cfg, NULL, "free_running");
> + err = asprintf(>name, "/dev/ptp%d", phc_index);
> + if (err < 0) {
> + free(c);
> + posix_clock_close(clkid);
> + return NULL;
> + }
> +
> + LIST_INSERT_HEAD(>clocks, c, list);
> + return c;
> +}
> +

It feels a bit weird that this code isn't already itself a separate .c
file that can be reused or shared with other programs that create
clocks. But you're mostly just moving this code here from a previous
location so I think it is fine.



>  static void usage(char *progname)
>  {
>   fprintf(stderr,
> diff --git a/ts2phc.h b/ts2phc.h
> index a5d50fc7e3be..135b795f11dc 100644
> --- a/ts2phc.h
> +++ b/ts2phc.h
> @@ -21,16 +21,42 @@
>  #ifndef HAVE_TS2PHC_H
>  #define HAVE_TS2PHC_H
>  
> +#include 
> +#include 
> +#include "servo.h"
> +
>  struct ts2phc_slave_array;
>  
> +#define SERVO_SYNC_INTERVAL1.0
> +
> +struct clock {
> + LIST_ENTRY(clock) list;
> + LIST_ENTRY(clock) dst_list;
> + clockid_t clkid;
> + int phc_index;
> + int state;
> + int new_state;

Re: [Linuxptp-devel] [RFC PATCH 05/15] ts2phc: create a private data structure

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> Eliminate the ad-hoc use of global variables in the ts2phc program by
> introducing one data structure that incorporates them. This might make
> the code more understandable to people coming from a kernel background,
> since it resembles the type of data organization used there. It is also
> now closer to the data organization of phc2sys, a similar program in
> both purpose and implementation.
> 
> Signed-off-by: Vladimir Oltean 

This patch is a bit difficult to read because a number of things get
moved at once. Hoever, I think it's a significant improvement on the
current state of things, and trying to do this transition piece-meal
would be difficult.

Avoiding global variables here is definitely an improvement, as it makes
it easier to think about who has access to what data.

Reviewed-by: Jacob Keller 

> ---
>  ts2phc.c   |  64 ---
>  ts2phc.h   |  37 ++
>  ts2phc_slave.c | 136 -
>  ts2phc_slave.h |  10 ++--
>  4 files changed, 153 insertions(+), 94 deletions(-)
>  create mode 100644 ts2phc.h
> 
> diff --git a/ts2phc.c b/ts2phc.c
> index 23428586ef66..0be7b18bda71 100644
> --- a/ts2phc.c
> +++ b/ts2phc.c
> @@ -11,22 +11,21 @@
>  #include "config.h"
>  #include "interface.h"
>  #include "print.h"
> -#include "ts2phc_master.h"
> -#include "ts2phc_slave.h"
> +#include "ts2phc.h"
>  #include "version.h"
>  
>  struct interface {
>   STAILQ_ENTRY(interface) list;
>  };
>  
> -static void ts2phc_cleanup(struct config *cfg, struct ts2phc_master *master)
> +static void ts2phc_cleanup(struct ts2phc_private *priv)
>  {
> - ts2phc_slave_cleanup();
> - if (master) {
> - ts2phc_master_destroy(master);
> + ts2phc_slave_cleanup(priv);
> + if (priv->master) {
> + ts2phc_master_destroy(priv->master);
>   }
> - if (cfg) {
> - config_destroy(cfg);
> + if (priv->cfg) {
> + config_destroy(priv->cfg);
>   }
>  }
>  
> @@ -56,8 +55,8 @@ static void usage(char *progname)
>  int main(int argc, char *argv[])
>  {
>   int c, err = 0, have_slave = 0, index, print_level;
> - struct ts2phc_master *master = NULL;
>   enum ts2phc_master_type pps_type;
> + struct ts2phc_private priv = {0};
>   char *config = NULL, *progname;
>   const char *pps_source = NULL;
>   struct config *cfg = NULL;
> @@ -68,7 +67,7 @@ int main(int argc, char *argv[])
>  
>   cfg = config_create();
>   if (!cfg) {
> - ts2phc_cleanup(cfg, master);
> + ts2phc_cleanup();
>   return -1;
>   }
>  
> @@ -81,14 +80,14 @@ int main(int argc, char *argv[])
>   switch (c) {
>   case 0:
>   if (config_parse_option(cfg, opts[index].name, optarg)) 
> {
> - ts2phc_cleanup(cfg, master);
> + ts2phc_cleanup();
>   return -1;
>   }
>   break;
>   case 'c':
>   if (!config_create_interface(optarg, cfg)) {
>   fprintf(stderr, "failed to add slave\n");
> - ts2phc_cleanup(cfg, master);
> + ts2phc_cleanup();
>   return -1;
>   }
>   have_slave = 1;
> @@ -99,7 +98,7 @@ int main(int argc, char *argv[])
>   case 'l':
>   if (get_arg_val_i(c, optarg, _level,
> PRINT_LEVEL_MIN, PRINT_LEVEL_MAX)) {
> - ts2phc_cleanup(cfg, master);
> + ts2phc_cleanup();
>   return -1;
>   }
>   config_set_int(cfg, "logging_level", print_level);
> @@ -116,29 +115,29 @@ int main(int argc, char *argv[])
>   case 's':
>   if (pps_source) {
>   fprintf(stderr, "too many PPS sources\n");
> - ts2phc_cleanup(cfg, master);
> + ts2phc_cleanup();
>   return -1;
>   }
>   pps_source = optarg;
>   break;
>   case 'v':
> - ts2phc_cleanup(cfg, master);
> + ts2phc_cleanup();
>  

Re: [Linuxptp-devel] [RFC PATCH 04/15] pmc_common: fix potential memory leak in run_pmc_events()

2020-08-05 Thread Jacob Keller
The subject says "potential" leak, but in fact it looks like we leaked
every single time we succeeded.

On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> The convention in all other parts of the code that call run_pmc() is to
> free the management PTP message if an error code was returned, or pass
> the message to the caller otherwise.
> 
> run_pmc_events() wasn't doing that, and was leaking a reference to the
> message, while also discarding the return code from run_pmc(). Fix that.
> 
> Signed-off-by: Vladimir Oltean 
> ---
>  phc2sys.c| 8 +++-
>  pmc_common.c | 9 +++--
>  pmc_common.h | 2 +-
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index c4d72bd7d17a..6c048b887d0b 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -702,6 +702,7 @@ static int do_loop(struct phc2sys_private *priv, int 
> subscriptions)
>   struct clock *clock;
>   uint64_t ts;
>   int64_t offset, delay;
> + int err;
>  
>   interval.tv_sec = priv->phc_interval;
>   interval.tv_nsec = (priv->phc_interval - interval.tv_sec) * 1e9;
> @@ -712,7 +713,12 @@ static int do_loop(struct phc2sys_private *priv, int 
> subscriptions)
>   continue;
>  
>   if (subscriptions) {
> - run_pmc_events(>node);
> + err = run_pmc_events(>node);
> + if (err) {
> + pr_err("run_pmc_events returned %d", err);
> + return err;
> + }
> +
>   if (priv->state_changed) {
>   /* force getting offset, as it may have
>* changed after the port state change */
> diff --git a/pmc_common.c b/pmc_common.c
> index 89d7f40b84fe..cdc1eb4616ae 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -939,11 +939,16 @@ int run_pmc_subscribe(struct pmc_node *node, int 
> timeout)
>   return 1;
>  }
>  
> -void run_pmc_events(struct pmc_node *node)
> +int run_pmc_events(struct pmc_node *node)
>  {
>   struct ptp_message *msg;
> + int res;
>  
> - run_pmc(node, 0, -1, );
> + res = run_pmc(node, 0, -1, );
> + if (res <= 0)
> + return res;
> + msg_put(msg);

Ok, so this is a bit confusing, and I think it is right but the commit
message is wrong.

run_pmc appears to call msg_put whenever it fails, so we do want to
avoid calling msg_put on a failure.

But either (a) we were always leaking the msg because we weren't
cleaning it up before or (b) on success run_pmc_events shouldn't cleanup
the message since it would be used by the caller.. but that doesn't make
sense since the msg pointer is a local variable.

Based on the other callers, I think this is right, but the commit
message is wrong.

> + return 1;
>  }
>  
>  int run_pmc_port_properties(struct pmc_node *node, int timeout,
> diff --git a/pmc_common.h b/pmc_common.h
> index a28bab767e9c..a3c7e956cba6 100644
> --- a/pmc_common.h
> +++ b/pmc_common.h
> @@ -76,7 +76,7 @@ int run_pmc_subscribe(struct pmc_node *node, int timeout);
>  int run_pmc_clock_identity(struct pmc_node *node, int timeout);
>  int run_pmc_wait_sync(struct pmc_node *node, int timeout);
>  int run_pmc_get_number_ports(struct pmc_node *node, int timeout);
> -void run_pmc_events(struct pmc_node *node);
> +int run_pmc_events(struct pmc_node *node);
>  int run_pmc_port_properties(struct pmc_node *node, int timeout,
>   unsigned int port, int *state,
>   int *tstamping, char *iface);
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH 03/15] phc2sys: break out pmc code into pmc_common.c

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> Move the code for sending various messages to ptp4l via pmc to a common
> translation module, outside of phc2sys. This makes it available to other
> programs that want to subscribe to port state change events too, such as
> ts2phc.
> 
> This creates a smaller structure within phc2sys_private, which embeds
> all properties related to the PMC. This structure is called "pmc_node",
> which is somewhat reminiscent of the old name of phc2sys_private (struct
> node). But the advantage is that struct pmc_node can be reused by other
> modules.

Ah, perfect: pmc_node is not too generic, so this is great.

I looked this over using git diff with the moved lines coloring options,
and the only places where the new code differs is in name change from
priv to pmc_node.

Makes sense.

Reviewed-by: Jacob Keller 

> 
> Signed-off-by: Vladimir Oltean 
> ---
>  phc2sys.c| 404 +--
>  pmc_common.c | 337 ++
>  pmc_common.h |  35 +
>  3 files changed, 407 insertions(+), 369 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index a36cbe071d7d..c4d72bd7d17a 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -56,18 +56,13 @@
>  #include "uds.h"
>  #include "util.h"
>  #include "version.h"
> +#include "contain.h"
>  
>  #define KP 0.7
>  #define KI 0.3
>  #define NS_PER_SEC 10LL
>  
>  #define PHC_PPS_OFFSET_LIMIT 1000
> -#define PMC_UPDATE_INTERVAL (60 * NS_PER_SEC)
> -#define PMC_SUBSCRIBE_DURATION 180   /* 3 minutes */
> -/* Note that PMC_SUBSCRIBE_DURATION has to be longer than
> - * PMC_UPDATE_INTERVAL otherwise subscription will time out before it is
> - * renewed.
> - */
>  
>  struct clock {
>   LIST_ENTRY(clock) list;
> @@ -105,17 +100,10 @@ struct phc2sys_private {
>   enum servo_type servo_type;
>   int phc_readings;
>   double phc_interval;
> - int sync_offset;
>   int forced_sync_offset;
> - int utc_offset_traceable;
> - int leap;
>   int kernel_leap;
> - struct pmc *pmc;
> - int pmc_ds_requested;
> - uint64_t pmc_last_update;
>   int state_changed;
> - int clock_identity_set;
> - struct ClockIdentity clock_identity;
> + struct pmc_node node;
>   LIST_HEAD(port_head, port) ports;
>   LIST_HEAD(clock_head, clock) clocks;
>   LIST_HEAD(dst_clock_head, clock) dst_clocks;
> @@ -124,18 +112,11 @@ struct phc2sys_private {
>  
>  static struct config *phc2sys_config;
>  
> -static int update_pmc(struct phc2sys_private *priv, int subscribe);
>  static int clock_handle_leap(struct phc2sys_private *priv,
>struct clock *clock,
>int64_t offset, uint64_t ts);
> -static int run_pmc_get_utc_offset(struct phc2sys_private *priv,
> -   int timeout);
> -static void run_pmc_events(struct phc2sys_private *priv);
>  
>  static int normalize_state(int state);
> -static int run_pmc_port_properties(struct phc2sys_private *priv,
> -int timeout, unsigned int port,
> -int *state, int *tstamping, char *iface);
>  
>  static struct servo *servo_add(struct phc2sys_private *priv,
>  struct clock *clock)
> @@ -324,7 +305,7 @@ static void clock_reinit(struct phc2sys_private *priv, 
> struct clock *clock,
>  
>   LIST_FOREACH(p, >ports, list) {
>   if (p->clock == clock) {
> - ret = run_pmc_port_properties(priv, 1000, p->number,
> + ret = run_pmc_port_properties(>node, 1000, 
> p->number,
> , ,
> iface);
>   if (ret > 0)
> @@ -659,7 +640,7 @@ static int do_pps_loop(struct phc2sys_private *priv, 
> struct clock *clock,
>  
>   if (src == CLOCK_INVALID) {
>   /* The sync offset can't be applied with PPS alone. */
> - priv->sync_offset = 0;
> + priv->node.sync_offset = 0;
>   } else {
>   enable_pps_output(priv->master->clkid);
>   }
> @@ -690,7 +671,7 @@ static int do_pps_loop(struct phc2sys_private *priv, 
> struct clock *clock,
>   pps_offset = pps_ts - phc_ts;
>   }
>  
> - if (update_pmc(priv, 0) < 0)
> + if (update_pmc_node(>node, 0) < 0)
>   continue;
>   update_clock(priv, clock

Re: [Linuxptp-devel] [RFC PATCH 02/15] phc2sys: rename struct node to struct phc2sys_private

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> We will be reusing some PMC code between phc2sys and ts2phc. In
> preparation of that, we would like to extract the PMC related properties
> of the current private program data structure of phc2sys, "struct node",
> into something smaller that can be shared properly.
> 
> The "struct node" name is nice enough, so use that to denote the smaller
> data structure for PMC from now on. Rename the bigger data structure to
> phc2sys_private.

If this gets extracted out to another file I feel like node might be too
generic. But the change to phc2sys_private seems reasonable.

The patch is noisy, but I checked using word-diff and it only changes
the name.

Reviewed-by: Jacob Keller 

> 
> Signed-off-by: Vladimir Oltean 
> ---
>  phc2sys.c | 433 --
>  1 file changed, 221 insertions(+), 212 deletions(-)
> 
> diff --git a/phc2sys.c b/phc2sys.c
> index 64bdf2664fe4..a36cbe071d7d 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -99,7 +99,7 @@ struct port {
>   struct clock *clock;
>  };
>  
> -struct node {
> +struct phc2sys_private {
>   unsigned int stats_max_count;
>   int sanity_freq_limit;
>   enum servo_type servo_type;
> @@ -124,18 +124,21 @@ struct node {
>  
>  static struct config *phc2sys_config;
>  
> -static int update_pmc(struct node *node, int subscribe);
> -static int clock_handle_leap(struct node *node, struct clock *clock,
> +static int update_pmc(struct phc2sys_private *priv, int subscribe);
> +static int clock_handle_leap(struct phc2sys_private *priv,
> +  struct clock *clock,
>int64_t offset, uint64_t ts);
> -static int run_pmc_get_utc_offset(struct node *node, int timeout);
> -static void run_pmc_events(struct node *node);
> +static int run_pmc_get_utc_offset(struct phc2sys_private *priv,
> +   int timeout);
> +static void run_pmc_events(struct phc2sys_private *priv);
>  
>  static int normalize_state(int state);
> -static int run_pmc_port_properties(struct node *node, int timeout,
> -unsigned int port,
> +static int run_pmc_port_properties(struct phc2sys_private *priv,
> +int timeout, unsigned int port,
>  int *state, int *tstamping, char *iface);
>  
> -static struct servo *servo_add(struct node *node, struct clock *clock)
> +static struct servo *servo_add(struct phc2sys_private *priv,
> +struct clock *clock)
>  {
>   double ppb;
>   int max_ppb;
> @@ -157,19 +160,19 @@ static struct servo *servo_add(struct node *node, 
> struct clock *clock)
>   }
>   }
>  
> - servo = servo_create(phc2sys_config, node->servo_type,
> + servo = servo_create(phc2sys_config, priv->servo_type,
>-ppb, max_ppb, 0);
>   if (!servo) {
>   pr_err("Failed to create servo");
>   return NULL;
>   }
>  
> - servo_sync_interval(servo, node->phc_interval);
> + servo_sync_interval(servo, priv->phc_interval);
>  
>   return servo;
>  }
>  
> -static struct clock *clock_add(struct node *node, char *device)
> +static struct clock *clock_add(struct phc2sys_private *priv, char *device)
>  {
>   struct clock *c;
>   clockid_t clkid = CLOCK_INVALID;
> @@ -198,7 +201,7 @@ static struct clock *clock_add(struct node *node, char 
> *device)
>   c->source_label = "phc";
>   }
>  
> - if (node->stats_max_count > 0) {
> + if (priv->stats_max_count > 0) {
>   c->offset_stats = stats_create();
>   c->freq_stats = stats_create();
>   c->delay_stats = stats_create();
> @@ -209,8 +212,8 @@ static struct clock *clock_add(struct node *node, char 
> *device)
>   return NULL;
>   }
>   }
> - if (node->sanity_freq_limit) {
> - c->sanity_check = clockcheck_create(node->sanity_freq_limit);
> + if (priv->sanity_freq_limit) {
> + c->sanity_check = clockcheck_create(priv->sanity_freq_limit);
>   if (!c->sanity_check) {
>   pr_err("failed to create clock check");
>   return NULL;
> @@ -218,21 +221,21 @@ static struct clock *clock_add(struct node *node, char 
> *device)
>   }
>  
>   if (clkid != CLOCK_INVALID)
> - c->servo = servo_add(node, c);
> + c->servo = servo_add(priv, c);
>  
>

Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC index from device name if possible

2020-08-05 Thread Jacob Keller



On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> Currently the PHC index is retrieved only through an ethtool ioctl if
> the PHC is specified as an Ethernet interface. If it's a char device
> such as /dev/ptp5, the phc_index will remain unpopulated. Try to infer
> it from the char device's path.
> 
> This is useful when trying to determine whether multiple clocks are in
> fact the same (such as /dev/ptp3 and sw1p3), just compare their PHC
> index.
> 

Makes sense. The header already indicates the phc_index will be set if
any, so I think this is good.

> Signed-off-by: Vladimir Oltean 
> ---
>  util.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/util.c b/util.c
> index 296dd59a08c1..027d694ea854 100644
> --- a/util.c
> +++ b/util.c
> @@ -211,6 +211,16 @@ clockid_t posix_clock_open(const char *device, int 
> *phc_index)
>   /* check if device is valid phc device */
>   clkid = phc_open(device);
>   if (clkid != CLOCK_INVALID) {
> + if (!strncmp(device, "/dev/ptp", strlen("/dev/ptp"))) {
> + int r = get_ranged_int(device + strlen("/dev/ptp"),
> +phc_index, 0, 65535);
> + if (r) {
> + fprintf(stderr,
> + "failed to parse PHC index from %s\n",
> + device);
> + return -1;
> + }

Here, we are making the implicit assumption that all ptp clock devices
will always have /dev/ptpX format. I don't think anyone is crazy enough
to rename these devices...

An alternative (requiring kernel implementation maybe?) would be to read
the phc index from the kernel somehow. It doesn't look like this is
exported anywhere else besides the name.

Since we don't actually expect these devices to have names changed by
userspace, I think this is acceptable.

> + }
>   return clkid;
>   }
>   /* check if device is a valid ethernet device */
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH] phc_ctl: display all capability information

2020-08-05 Thread Jacob Keller
The capability command for phc_ctl does not display the number of pins
or the cross timestamping support. Add this as output so that the user
can see the complete device capabilities.

Signed-off-by: Jacob Keller 
---
 phc_ctl.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/phc_ctl.c b/phc_ctl.c
index 91913426ec1b..00d7a1cd68a4 100644
--- a/phc_ctl.c
+++ b/phc_ctl.c
@@ -320,12 +320,16 @@ static int do_caps(clockid_t clkid, int cmdc, char 
*cmdv[])
"  %d programable alarms\n"
"  %d external time stamp channels\n"
"  %d programmable periodic signals\n"
-   "  %s pulse per second support",
+   "  %d configurable input/output pins\n"
+   "  %s pulse per second support\n"
+   "  %s cross timestamping support\n",
caps.max_adj,
caps.n_alarm,
caps.n_ext_ts,
caps.n_per_out,
-   caps.pps ? "has" : "doesn't have");
+   caps.n_pins,
+   caps.pps ? "has" : "doesn't have",
+   caps.cross_timestamping ? "has" : "doesn't have");
return 0;
 }
 

base-commit: 3da961bb112b4d0e823ff125901b5674e7b57c0e
-- 
2.28.0.163.g6104cc2f0b60



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] Machine Readable Output

2020-07-27 Thread Jacob Keller


On 7/25/2020 7:34 PM, Matthew P. Grosvenor wrote:
> 
>> The messages are standardized and use TLV format so writing your own
>> send/receive bits would not be too difficult. Plus, if you don't mind
>> GPL you can just simply fork and port pmc into whatever language you
>> prefer.
> 
> This is exactly what I’m trying to avoid: committing the cardinal sin of
> open source. If the project doesn’t do exactly what you need, just write
> yet-another similar but different implementation written in yet-another
> language, rather than maintain and extend the existing one. 
> 

It's good to ask. However, this project doesn't wish to maintain
something more complex than the tiny PMC tool already provided, as
pointed out in another reply in this thread.

This is also the beauty of open source: you can build on top of this
work and maintain what you need, even if the original authors do not
wish to do so.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] Machine Readable Output

2020-07-22 Thread Jacob Keller



On 7/22/2020 5:07 PM, Richard Cochran wrote:
> On Thu, Jul 23, 2020 at 09:57:35AM +1000, Luke Howard wrote:
>> Possibly an extension to pmc(8) for emitting JSON would better suit your use 
>> case, depends on how your application is structured I guess?
> 
> You can pipe the pmc output through a script (python, etc) that emits json.
> That might cover simple use cases.
> 
> For more complex monitoring, I would, in fact, recommend developing
> your own client logic.
> 
> The pmc program is super simple on purpose, and it is meant as an
> example.  Trying to make a monitoring client/library that satisfies
> everyone's needs will get very complicated very fast!
> 
> Thanks,
> Richard
> 
> Agreed.

I'm not personally against having JSON output as an option, but it also
doesn't seem that much more useful beyond simply writing your own tool
that does more complex logic.

The messages are standardized and use TLV format so writing your own
send/receive bits would not be too difficult. Plus, if you don't mind
GPL you can just simply fork and port pmc into whatever language you prefer.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH] ts2phc_phc_master: specify start time to PPS master as 0.000000000

2020-07-06 Thread Jacob Keller



On 6/26/2020 5:55 AM, Richard Cochran wrote:
> On Fri, Jun 26, 2020 at 01:15:11PM +0300, Vladimir Oltean wrote:
> 
>> So, to your point: I didn't see any driver that _rejects_ time in the
>> past. If it works, I don't know. If it doesn't work, I would do the
>> time fixup at driver level, so in my opinion there's no need for an
>> application-level flag. What do you think?
> 
> I agree with your analysis.  Ideally the drivers would handle this
> perfectly.** But even if they could, still it will take months, maybe
> years to fix all the kernel drivers.  Moreover, the user space stack
> would still need to work with the "legacy" kernels.
> 
> So I think it will have to be a flag.
> 
> Thanks,
> Richard
> 
> 
> ** Without hardware support, it is actually quite tricky to solve the
>race condition in software in a way which is 100% reliable.  You
>are correct that setting the start time even a whole second in the
>future might not be enough on a non-preempt_rt kernel.
> 
>Regarding the start=0 for HW without start time control, this is
>not an ideal interface.  I would like to have a proper "frequency
>output" or "clock output" ABI in the PHC world.  If you have the
>time, maybe this is something you like to work on?
> 

This would be useful. I know some of the Intel hardware can do "start
time" but not all of them can, and it usually takes up more resources
because we have to tie both a periodic output function and a start
trigger function to the setup.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH] ts2phc_phc_master: specify start time to PPS master as 0.000000000

2020-07-06 Thread Jacob Keller



On 6/26/2020 3:15 AM, Vladimir Oltean wrote:
> *Although clock stepping makes that challenging. But I don't know how
> many drivers still treat perout correctly after a clock step, and I'm
> not even sure what the correct treatment would be.
>

The way I've seen this done in the past is to remove and re-add the
perout setup. Ofcourse this requires some judgement of what the new
"start time" is going to be since it would no longer make sense in the
future. Usually I think we just round-up to the next period value.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 2/3] clock: dump unexpected packets received on the error queues of sockets

2020-06-18 Thread Jacob Keller



On 6/18/2020 1:17 PM, Vladimir Oltean wrote:
> On Thu, 18 Jun 2020 at 23:02, Jacob Keller  wrote:
>>
>>
>>
>> On 6/18/2020 12:56 PM, Vladimir Oltean wrote:
>>>> Could this use the pr facility, so that it honors the printing options
>>>> to log to syslog and manages the level?
>>>>
>>>> I'm not sure what I would mark these as: debug because they no longer
>>>> impact the syncing process, or true error because they are unexpected?
>>>>
>>>> Thanks,
>>>> Jake
>>>>
>>>
>>> Thanks a lot for looking at this.
>>> Good point about redirecting to syslog, I did not think about that.
>>> The reason why I did not use pr_err was due to the automatic line
>>> ending, which would have unnecessarily complicated this function by
>>> having me print each line of the packet to a temporary buffer first.
>>> But now that I see there's no way around it, I guess I'll have to do
>>> that.
>> What about implementing this as part of pr.c?
>>
>> Thanks,
>> Jake
> 
> Not really sure what you mean. What would the prototype of the
> function be (i.e. what would it do)?
> 
> -Vladimir
> 

I was thinking like something that would do a hexdump to a given print
level.

Perhaps that's overkill since not many places would need it...

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 2/3] clock: dump unexpected packets received on the error queues of sockets

2020-06-18 Thread Jacob Keller



On 6/18/2020 12:56 PM, Vladimir Oltean wrote:
>> Could this use the pr facility, so that it honors the printing options
>> to log to syslog and manages the level?
>>
>> I'm not sure what I would mark these as: debug because they no longer
>> impact the syncing process, or true error because they are unexpected?
>>
>> Thanks,
>> Jake
>>
> 
> Thanks a lot for looking at this.
> Good point about redirecting to syslog, I did not think about that.
> The reason why I did not use pr_err was due to the automatic line
> ending, which would have unnecessarily complicated this function by
> having me print each line of the packet to a temporary buffer first.
> But now that I see there's no way around it, I guess I'll have to do
> that.
What about implementing this as part of pr.c?

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 2/3] clock: dump unexpected packets received on the error queues of sockets

2020-06-18 Thread Jacob Keller



On 6/15/2020 8:23 AM, Vladimir Oltean wrote:
> In the current design of the SO_SELECT_ERR_QUEUE socket option (which is
> enabled by sk_timestamping_init on the event fd), it is a bug to only
> check revents & POLLIN, but not also POLLERR.
> 
> Normally the error queue events that the application expects (i.e. TX
> timestamps) are received, within a given timeout, in-band with the
> transmission of the timestampable message itself (for example in
> raw_send).
> 
> For messages that the application does not / no longer expects, such as
> TX timestamps delivered late, duplicate TX timestamps, general
> exceptional messages enqueued by the kernel in the socket error queue
> etc, ptp4l will be taken by surprise in clock_poll() by these, and will
> think that there is data, since POLLIN is set (but in fact in the socket
> error queue etc, ptp4l will be taken by surprise in clock_poll() by
> these, and will think that there is data, since POLLIN is set (but in
> fact POLLERR is also set, and this has an entirely different meaning).
> 
> A very, very simple reproducer is to take a DSA switch and run:
> 
> tcpdump -i eth0 -j adapter_unsynced
> 
> on its DSA master net device. The above command will enable timestamping
> on that net device, and if both the DSA switch and the master support
> PTP, this will make the kernel send duplicate TX timestamps for every
> sent event packet, which will completely kill ptp4l until a reboot, with
> no indication whatsoever of what's going on.
> 
> Since the messages on the error queue are unexpected, we have no need
> for them. And they can be in theory anything, so simply hex dumping
> their content and moving along sounds like a good idea.
> 
> Printing them to the user is optional (and helpful), but reading them is
> not. With this patch, even with extraneous data delivered by a buggy
> kernel (which the application now loudly complains about), the
> synchronization keeps chugging along. Otherwise the application starts
> reordering packets in recvmsg() due to misinterpreting which socket
> queue has data available.
> 
> Signed-off-by: Vladimir Oltean 
> ---
> Changes in v2:
> Make msg_raw_dump print to stderr instead of stdout, matching the
> "Unexpected data on socket err queue:" text.
> 
>  clock.c | 11 +++
>  msg.c   | 12 
>  msg.h   |  8 
>  3 files changed, 31 insertions(+)
> 
> diff --git a/clock.c b/clock.c
> index f43cc2afd49e..d61881c5db7a 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1559,6 +1559,17 @@ int clock_poll(struct clock *c)
>   LIST_FOREACH(p, >ports, list) {
>   /* Let the ports handle their events. */
>   for (i = 0; i < N_POLLFD; i++) {
> + if (cur[i].revents & POLLERR) {
> + unsigned char pkt[1600];
> +
> + cnt = recv(cur[i].fd, pkt, sizeof(pkt),
> +MSG_ERRQUEUE);
> + pr_err("Unexpected data on socket err queue:");
> + msg_raw_dump(pkt, cnt, stderr);
> +
> + continue;
> + }
> +
>   if (cur[i].revents & (POLLIN|POLLPRI)) {
>   event = port_event(p, i);
>   if (EV_STATE_DECISION_EVENT == event) {
> diff --git a/msg.c b/msg.c
> index d1619d4973f1..59bb55d6cd89 100644
> --- a/msg.c
> +++ b/msg.c
> @@ -601,3 +601,15 @@ int msg_sots_missing(struct ptp_message *m)
>   }
>   return msg_sots_valid(m) ? 0 : 1;
>  }
> +
> +void msg_raw_dump(unsigned char *pkt, int cnt, FILE *fp)
> +{
> + int k;
> +
> + for (k = 0; k < cnt; k++) {
> + if (k % 16 == 0)
> + fprintf(fp, "\n%04x ", k);
> + fprintf(fp, "%02x ", pkt[k]);
> + }
> + fprintf(fp, "\n");
> +}

Could this use the pr facility, so that it honors the printing options
to log to syslog and manages the level?

I'm not sure what I would mark these as: debug because they no longer
impact the syncing process, or true error because they are unexpected?

Thanks,
Jake

> diff --git a/msg.h b/msg.h
> index e71d3ceec4b9..f2165084395a 100644
> --- a/msg.h
> +++ b/msg.h
> @@ -395,6 +395,14 @@ void msg_put(struct ptp_message *m);
>   */
>  int msg_sots_missing(struct ptp_message *m);
>  
> +/**
> + * Print a wireshark-compatible hex dump of a message buffer.
> + * @param pkt  Message buffer to print
> + * @param cnt  Length of message buffer
> + * @param fp   An open file pointer.
> + */
> +void msg_raw_dump(unsigned char *pkt, int cnt, FILE *fp);
> +
>  /**
>   * Test whether a message has a valid SO_TIMESTAMPING time stamp.
>   * @param m  Message to test.
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH v2 1/3] ptp4l: call recvmsg() with the MSG_DONTWAIT flag

2020-06-18 Thread Jacob Keller



On 6/15/2020 8:23 AM, Vladimir Oltean wrote:
> The application's main event loop (clock_poll) is woken up by poll() and
> dispatches the socket receive queue events to the corresponding ports as
> needed.
> 
> So it is a bug if poll() wakes up the process for data availability on a
> socket's receive queue, and then recvmsg(), called immediately
> afterwards, goes to sleep trying to retrieve it. This patch will
> generate an error that will be propagated to the user if this condition
> happens.
> 
> Can it happen?
> 
> As of this patch, ptp4l uses the SO_SELECT_ERR_QUEUE socket option,
> which means that poll() will wake the process up, with revents ==
> (POLLIN | POLLERR), if data is available in the error queue. But
> clock_poll() does not check POLLERR, just POLLIN, and draws the wrong
> conclusion that there is data available in the receive queue (when it is
> in fact available in the error queue).
> 
> When the above condition happens, recvmsg() will sleep typically for a
> whole sync interval waiting for data on the event socket, and will be
> woken up when the new real frame arrives. It will not dequeue follow-up
> messages during this time (which are sent to the general message socket)
> and when it does, it will already be late for them (their seqid will be
> out of order). So it will drop them and everything that comes after. The
> synchronization process will fail.
> 
> The above condition shouldn't typically happen, but exceptional kernel
> events will trigger it. It helps to be strict in ptp4l in order for
> those events to not blow up in even stranger symptoms unrelated to the
> root cause of the problem.
> 

Makes sense. Using MSG_DONTWAIT seems reasonable since we already know
there should be data available.

> Signed-off-by: Vladimir Oltean 
> ---
> Changes in v2:
> None.
> 
>  raw.c  | 2 +-
>  udp.c  | 2 +-
>  udp6.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/raw.c b/raw.c
> index 15c97561066e..0bd15b08e0a2 100644
> --- a/raw.c
> +++ b/raw.c
> @@ -279,7 +279,7 @@ static int raw_recv(struct transport *t, int fd, void 
> *buf, int buflen,
>   buflen += hlen;
>   hdr = (struct eth_hdr *) ptr;
>  
> - cnt = sk_receive(fd, ptr, buflen, addr, hwts, 0);
> + cnt = sk_receive(fd, ptr, buflen, addr, hwts, MSG_DONTWAIT);
>  
>   if (cnt >= 0)
>   cnt -= hlen;
> diff --git a/udp.c b/udp.c
> index 36802fb67b74..826bd124deef 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -210,7 +210,7 @@ no_event:
>  static int udp_recv(struct transport *t, int fd, void *buf, int buflen,
>   struct address *addr, struct hw_timestamp *hwts)
>  {
> - return sk_receive(fd, buf, buflen, addr, hwts, 0);
> + return sk_receive(fd, buf, buflen, addr, hwts, MSG_DONTWAIT);
>  }
>  
>  static int udp_send(struct transport *t, struct fdarray *fda,
> diff --git a/udp6.c b/udp6.c
> index 744a5bc8adcb..ba5482e3d4c9 100644
> --- a/udp6.c
> +++ b/udp6.c
> @@ -227,7 +227,7 @@ no_event:
>  static int udp6_recv(struct transport *t, int fd, void *buf, int buflen,
>struct address *addr, struct hw_timestamp *hwts)
>  {
> - return sk_receive(fd, buf, buflen, addr, hwts, 0);
> + return sk_receive(fd, buf, buflen, addr, hwts, MSG_DONTWAIT);
>  }
>  
>  static int udp6_send(struct transport *t, struct fdarray *fda,
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 04/11] uds: Convey transmit path errors to the caller.

2020-05-18 Thread Jacob Keller



On 5/18/2020 4:24 PM, Richard Cochran wrote:
> On Mon, May 18, 2020 at 04:17:34PM -0700, Jacob Keller wrote:
>>> @@ -119,8 +119,8 @@ static int uds_send(struct transport *t, struct fdarray 
>>> *fda,
>>> addr = >address;
>>>  
>>> cnt = sendto(fd, buf, buflen, 0, >sa, addr->len);
>>> -   if (cnt <= 0 && errno != ECONNREFUSED) {
>>> -   pr_err("uds: sendto failed: %m");
>>> +   if (cnt < 1) {
>>> +   return -errno;
>>> }
>>> return cnt;
>>
>>
>> This felt like a bigger functional change, but looking carefully: cnt
>> would be either 0 or -1 and already indicate an error. Removing this
>> print makes sense.
> 
> Yeah, and the thing about cnt=0 is this.  We first call poll(), and so
> the return value from recv() simply cannot be zero.  But still, in the
> spirit of comprehensive error checking, the code treats zero as an
> error, even thought still "can never happen" (TM).
> 

Right. Previously we would have returned 0 in that case!

> Thanks,
> Richard
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH 3/3] phc2pwm: Introduce an utility to sync pwm with PTP clock

2020-05-18 Thread Jacob Keller


On 5/16/2020 11:22 AM, Lokesh Vutla via Linuxptp-devel wrote:
> Hi Richard,
> 
> On 16/05/20 10:44 PM, Richard Cochran wrote:
>> On Sat, May 16, 2020 at 10:35:33PM +0530, Lokesh Vutla wrote:
>>> Hi Richard,
>>>
>>> On 16/05/20 10:24 PM, Richard Cochran wrote:
 On Fri, Apr 17, 2020 at 10:00:09AM +0530, Lokesh Vutla wrote:
>  phc2pwm.c | 233 ++
>  2 files changed, 237 insertions(+), 2 deletions(-)
>  create mode 100644 phc2pwm.c

 I wanted to try this today.  There are some issues:

 /home/richard/git/linuxptp/phc2pwm.c: In function ‘main’:
 /home/richard/git/linuxptp/phc2pwm.c:222:3: error: ‘period’ may be used 
 uninitialized in this function [-Werror=maybe-uninitialized]
pwm_chan_set_period(chan, pwm_servo_sample(, ts));
^~~~
 /home/richard/git/linuxptp/phc2pwm.c:193:8: error: ‘ptp_dev’ may be used 
 uninitialized in this function [-Werror=maybe-uninitialized]
   clkid = phc_open(ptp_dev);
   ~~^~~
 /home/richard/git/linuxptp/phc2pwm.c:203:8: error: ‘event_index’ may be 
 used uninitialized in this function [-Werror=maybe-uninitialized]
   err = phc_enable_extts(clkid, event_index);
 ^~~~
 /home/richard/git/linuxptp/phc2pwm.c:197:9: error: ‘pwm_chan’ may be used 
 uninitialized in this function [-Werror=maybe-uninitialized]
   chan = pwm_chan_create(pwm_chip, pwm_chan);
  ^~~
 /home/richard/git/linuxptp/phc2pwm.c:197:9: error: ‘pwm_chip’ may be used 
 uninitialized in this function [-Werror=maybe-uninitialized]
 cc1: all warnings being treated as errors
>>>
>>> I am using 9.2 arm compiler and the build is successful. Which compiler are 
>>> you
>>> using?
>>
>> gcc-linaro-7.4.1-2019.02-x86_64_arm-linux-gnueabihf
> 
> hmm..that's old. May be time to upgrade.
> 
>>
>> Compile with -Werror please.
> 
> I tried enabling -Werror in makefile, but the repo doesn't build at all with 
> the
> following error:
> clock.c:444:48: error: taking address of packed member of ‘struct
> subscribe_events_np’ may result in an unaligned pointer value
> [-Werror=address-of-packed-member]
>   444 |   clock_get_subscription(c, req, sen->bitmask, >duration);
>   |^~
> cc1: all warnings being treated as errors
> : recipe for target 'clock.o' failed
> 

Yep, the current code doing that is incorrect if the platform can't do
unaligned access.

I'm not sure what actually happens here: will the compiler for such a
platform produce the right code for reading, or will it simply give up
on the unaligned pointers?

I know kernel code uses "get_unaligned" and "put_unaligned" to avoid
this kind of issue.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 11/11] Reject path trace TLVs with excessive elements.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> The current code truncates the size of path trace TLVs which exceed the
> expected maximum based on the largest possible message size.  However if
> another TLV follows, then a gap would appear, that is, an area in the
> message buffer not pointed to by any TLV descriptor.  In order to avoid
> forwarding such malformed messages, this patch changes the logic to reject
> them.
> 
> Signed-off-by: Richard Cochran 
> ---
>  tlv.c | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/tlv.c b/tlv.c
> index 2440482..6ab54a5 100644
> --- a/tlv.c
> +++ b/tlv.c
> @@ -18,6 +18,7 @@
>   */
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -79,6 +80,17 @@ static int64_t net2host64_unaligned(int64_t *p)
>   return v;
>  }
>  
> +static bool tlv_array_invalid(struct TLV *tlv, size_t base_size, size_t 
> item_size)
> +{
> + size_t expected_length, n_items;
> +
> + n_items = (tlv->length - base_size) / item_size;
> +
> + expected_length = base_size + n_items * item_size;
> +
> + return (tlv->length == expected_length) ? false : true;
> +}
> +
>  static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
>struct tlv_extra *extra)
>  {
> @@ -678,11 +690,10 @@ void tlv_extra_recycle(struct tlv_extra *extra)
>  
>  int tlv_post_recv(struct tlv_extra *extra)
>  {
> - int result = 0;
> - struct management_tlv *mgt;
>   struct management_error_status *mes;
>   struct TLV *tlv = extra->tlv;
> - struct path_trace_tlv *ptt;
> + struct management_tlv *mgt;
> + int result = 0;
>  
>   switch (tlv->type) {
>   case TLV_MANAGEMENT:
> @@ -712,9 +723,8 @@ int tlv_post_recv(struct tlv_extra *extra)
>   result = unicast_negotiation_post_recv(extra);
>   break;
>   case TLV_PATH_TRACE:
> - ptt = (struct path_trace_tlv *) tlv;
> - if (path_length(ptt) > PATH_TRACE_MAX) {
> - ptt->length = PATH_TRACE_MAX * sizeof(struct 
> ClockIdentity);
> + if (tlv_array_invalid(tlv, 0, sizeof(struct ClockIdentity))) {
> + goto bad_length;
>   }
>   break;
>   case TLV_ALTERNATE_TIME_OFFSET_INDICATOR:
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 09/11] port: Export the value of the wildcard port identity.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> Code in other modules will need this special port ID value.  This patch
> makes it available through the port header file.
> 
> Signed-off-by: Richard Cochran 
> ---
>  port.h   | 3 +++
>  port_signaling.c | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/port.h b/port.h
> index e347e36..80c543e 100644
> --- a/port.h
> +++ b/port.h
> @@ -33,6 +33,9 @@ struct clock;
>  /** Opaque type. */
>  struct port;
>  
> +/** The port identity that matches any port. */
> +extern const struct PortIdentity wildcard_pid;
> +
>  /**
>   * Returns the dataset from a port's best foreign clock record, if any
>   * has yet been discovered. This function does not bring the returned
> diff --git a/port_signaling.c b/port_signaling.c
> index c4d5469..cbd0cf4 100644
> --- a/port_signaling.c
> +++ b/port_signaling.c
> @@ -23,7 +23,7 @@
>  #include "unicast_client.h"
>  #include "unicast_service.h"
>  
> -static struct PortIdentity wildcard = {
> +const struct PortIdentity wildcard_pid = {
>   .clockIdentity = {
>   {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}
>   },
> @@ -126,7 +126,7 @@ int process_signaling(struct port *p, struct ptp_message 
> *m)
>  
>   /* Ignore signaling messages not addressed to this port. */
>   if (!pid_eq(>signaling.targetPortIdentity, >portIdentity) &&
> - !pid_eq(>signaling.targetPortIdentity, )) {
> + !pid_eq(>signaling.targetPortIdentity, _pid)) {
>   return 0;
>   }
>  
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 10/11] port: Publish the method for creating signaling messages.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
> ---
>  port.h   | 3 +++
>  port_signaling.c | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/port.h b/port.h
> index 80c543e..0b07d55 100644
> --- a/port.h
> +++ b/port.h
> @@ -210,6 +210,9 @@ struct port *port_open(const char *phc_device,
>  struct interface *interface,
>  struct clock *clock);
>  
> +struct ptp_message *port_signaling_construct(struct port *p,
> +  const struct PortIdentity *tpid);
> +
>  /**
>   * Returns a port's current state.
>   * @param port  A port instance.
> diff --git a/port_signaling.c b/port_signaling.c
> index cbd0cf4..2f5a682 100644
> --- a/port_signaling.c
> +++ b/port_signaling.c
> @@ -30,8 +30,8 @@ const struct PortIdentity wildcard_pid = {
>   .portNumber = 0x,
>  };
>  
> -static struct ptp_message *port_signaling_construct(struct port *p,
> - struct PortIdentity *tpid)
> +struct ptp_message *port_signaling_construct(struct port *p,
> +  const struct PortIdentity *tpid)
>  {
>   struct ptp_message *msg;
>  
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 07/11] port: Convey targeted forwarding errors to the caller.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> The port_forward_to() method clobbers the specific error code returned
> by the transport layer with -1.  This patch lets the code preserve the
> specific error in question.
> 
> Signed-off-by: Richard Cochran 
> ---
>  port.c | 6 --
>  port.h | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/port.c b/port.c
> index 6104061..48fff1c 100644
> --- a/port.c
> +++ b/port.c
> @@ -2737,8 +2737,10 @@ int port_forward_to(struct port *p, struct ptp_message 
> *msg)
>  {
>   int cnt;
>   cnt = transport_sendto(p->trp, >fda, TRANS_GENERAL, msg);
> - if (cnt <= 0) {
> - return -1;
> + if (cnt < 0) {
> + return cnt;
> + } else if (!cnt) {
> + return -EIO;
>   }
>   port_stats_inc_tx(p, msg);
>   return 0;
> diff --git a/port.h b/port.h
> index a45a7a4..e347e36 100644
> --- a/port.h
> +++ b/port.h
> @@ -94,7 +94,7 @@ int port_forward(struct port *p, struct ptp_message *msg);
>   * Forward a message on a given port to the address stored in the message.
>   * @param portA pointer previously obtained via port_open().
>   * @param msg The message to send. Must be in network byte order.
> - * @returnZero on success, non-zero otherwise.
> + * @returnZero on success, negative errno value otherwise.
>   */
>  int port_forward_to(struct port *p, struct ptp_message *msg);
>  
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 08/11] util: Mark port identity comparisons as const.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> The utility function to compare port IDs takes pointers, but it only needs
> to read the referenced data.  This patch marks the parameters as const,
> allowing passing constants in the future.
> 
> Signed-off-by: Richard Cochran 
> ---
>  util.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util.h b/util.h
> index 6e104ea..41e33d4 100644
> --- a/util.h
> +++ b/util.h
> @@ -131,7 +131,8 @@ clockid_t posix_clock_open(const char *device, int 
> *phc_index);
>   * @param b  Second port identity.
>   * @return   1 if identities are equal, 0 otherwise.
>   */
> -static inline int pid_eq(struct PortIdentity *a, struct PortIdentity *b)
> +static inline int pid_eq(const struct PortIdentity *a,
> +  const struct PortIdentity *b)
>  {
>   return memcmp(a, b, sizeof(*a)) == 0;
>  }
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 06/11] sk: Convey transmit path errors to the caller.

2020-05-18 Thread Jacob Keller
Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> The transport layer's functional interface foresees having error codes
> percolate back up to the caller.  However, up until now, the sk module
> simply returned -1 for any error.  This patch lets the code return the
> specific error instead.
> 
> Signed-off-by: Richard Cochran 
> ---
>  sk.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/sk.c b/sk.c
> index e211175..c9ef4d2 100644
> --- a/sk.c
> +++ b/sk.c
> @@ -354,7 +354,7 @@ int sk_receive(int fd, void *buf, int buflen,
>"timed out while polling for tx 
> timestamp");
>   pr_err("increasing tx_timestamp_timeout may correct "
>  "this issue, but it is likely caused by a driver 
> bug");
> - return res;
> + return -errno;
>   } else if (!(pfd.revents & sk_revents)) {
>   pr_err("poll for tx timestamp woke up on non ERR 
> event");
>   return -1;
> @@ -362,24 +362,24 @@ int sk_receive(int fd, void *buf, int buflen,
>   }
>  
>   cnt = recvmsg(fd, , flags);
> - if (cnt < 0)
> + if (cnt < 0) {
>   pr_err("recvmsg%sfailed: %m",
>  flags == MSG_ERRQUEUE ? " tx timestamp " : " ");
> -
> + }

Nice. I dislike style where {} is not used when it's 'technically' a
one-line statement even though it appears over multiple lines. This
avoids having to catch that when reading the code. This wasn't mentioned
in the commit message though.

>   for (cm = CMSG_FIRSTHDR(); cm != NULL; cm = CMSG_NXTHDR(, cm)) {
>   level = cm->cmsg_level;
>   type  = cm->cmsg_type;
>   if (SOL_SOCKET == level && SO_TIMESTAMPING == type) {
>   if (cm->cmsg_len < sizeof(*ts) * 3) {
>   pr_warning("short SO_TIMESTAMPING message");
> - return -1;
> + return -EMSGSIZE;
>   }
>   ts = (struct timespec *) CMSG_DATA(cm);
>   }
>   if (SOL_SOCKET == level && SO_TIMESTAMPNS == type) {
>   if (cm->cmsg_len < sizeof(*sw)) {
>   pr_warning("short SO_TIMESTAMPNS message");
> - return -1;
> + return -EMSGSIZE;
>   }
>   sw = (struct timespec *) CMSG_DATA(cm);
>   hwts->sw = timespec_to_tmv(*sw);
> @@ -391,7 +391,7 @@ int sk_receive(int fd, void *buf, int buflen,
>  
>   if (!ts) {
>   memset(>ts, 0, sizeof(hwts->ts));
> - return cnt;
> + return cnt < 1 ? -errno : cnt;
>   }
>  
>   switch (hwts->type) {
> @@ -407,7 +407,7 @@ int sk_receive(int fd, void *buf, int buflen,
>   hwts->ts = timespec_to_tmv(ts[1]);
>   break;
>   }
> - return cnt;
> + return cnt < 1 ? -errno : cnt;
>  }
>  
>  int sk_set_priority(int fd, int family, uint8_t dscp)
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 05/11] raw: Convey transmit path errors to the caller.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

This one is even easier to tell it is correct.

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> The transport layer's functional interface foresees having error codes
> percolate back up to the caller.  However, up until now, the raw module
> simply returned -1 for any error.  This patch lets the code return the
> specific error instead.  In addition, it removes the gratuitous printing
> of the error message, leaving that task up to caller, just like the other
> transport modules.
> 
> Signed-off-by: Richard Cochran 
> ---
>  raw.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/raw.c b/raw.c
> index 81ec431..15c9756 100644
> --- a/raw.c
> +++ b/raw.c
> @@ -336,8 +336,7 @@ static int raw_send(struct transport *t, struct fdarray 
> *fda,
>  
>   cnt = send(fd, ptr, len, 0);
>   if (cnt < 1) {
> - pr_err("send failed: %d %m", errno);
> - return cnt;
> + return -errno;
>   }
>   /*
>* Get the time stamp right away.
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 04/11] uds: Convey transmit path errors to the caller.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> The transport layer's functional interface foresees having error codes
> percolate back up to the caller.  However, up until now, the uds module
> simply returned -1 for any error.  This patch lets the code return the
> specific error instead.  In addition, it removes the gratuitous printing
> of the error message, leaving that task up to caller, just like the other
> transport modules.
> 
> Signed-off-by: Richard Cochran 
> ---
>  uds.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/uds.c b/uds.c
> index a4c856b..641a672 100644
> --- a/uds.c
> +++ b/uds.c
> @@ -119,8 +119,8 @@ static int uds_send(struct transport *t, struct fdarray 
> *fda,
>   addr = >address;
>  
>   cnt = sendto(fd, buf, buflen, 0, >sa, addr->len);
> - if (cnt <= 0 && errno != ECONNREFUSED) {
> - pr_err("uds: sendto failed: %m");
> + if (cnt < 1) {
> + return -errno;
>   }
>   return cnt;


This felt like a bigger functional change, but looking carefully: cnt
would be either 0 or -1 and already indicate an error. Removing this
print makes sense.

Ok.


>  }
> @@ -144,4 +144,3 @@ struct transport *uds_transport_create(void)
>   uds->t.release = uds_release;
>   return >t;
>  }
> -
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 02/11] udp: Convey transmit path errors to the caller.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> The transport layer's functional interface foresees having error codes
> percolate back up to the caller.  However, up until now, the udp module
> simply returned -1 for any error.  This patch lets the code return the
> specific error instead.
> 
> Signed-off-by: Richard Cochran 
> ---
>  udp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/udp.c b/udp.c
> index eb7be78..36802fb 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -256,7 +256,7 @@ static int udp_send(struct transport *t, struct fdarray 
> *fda,
>   cnt = sendto(fd, buf, len, 0, >sa, sizeof(addr->sin));
>   if (cnt < 1) {
>   pr_err("sendto failed: %m");
> - return cnt;
> + return -errno;
>   }
>   /*
>* Get the time stamp right away.
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 03/11] udp6: Convey transmit path errors to the caller.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> The transport layer's functional interface foresees having error codes
> percolate back up to the caller.  However, up until now, the udp6 module
> simply returned -1 for any error.  This patch lets the code return the
> specific error instead.
> 
> Signed-off-by: Richard Cochran 
> ---
>  udp6.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/udp6.c b/udp6.c
> index 6cb571b..744a5bc 100644
> --- a/udp6.c
> +++ b/udp6.c
> @@ -271,7 +271,7 @@ static int udp6_send(struct transport *t, struct fdarray 
> *fda,
>   cnt = sendto(fd, buf, len, 0, >sa, sizeof(addr->sin6));
>   if (cnt < 1) {
>   pr_err("sendto failed: %m");
> - return cnt;
> + return -errno;
>   }
>   /*
>* Get the time stamp right away.
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 01/11] transport: Correct grammar in the doxygen comments.

2020-05-18 Thread Jacob Keller


Reviewed-by: Jacob Keller 

On 5/16/2020 8:03 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
> ---
>  transport.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/transport.h b/transport.h
> index 5b8c413..7a7f87b 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -70,7 +70,7 @@ int transport_recv(struct transport *t, int fd, struct 
> ptp_message *msg);
>   * @param fdaThe array of descriptors filled in by transport_open.
>   * @param event  One of the @ref transport_event enumeration values.
>   * @param msgThe message to send.
> - * @return   Number of bytes send, or negative value in case of an error.
> + * @return   Number of bytes sent, or negative value in case of an error.
>   */
>  int transport_send(struct transport *t, struct fdarray *fda,
>  enum transport_event event, struct ptp_message *msg);
> @@ -83,7 +83,7 @@ int transport_send(struct transport *t, struct fdarray *fda,
>   * @param fdaThe array of descriptors filled in by transport_open.
>   * @param event  One of the @ref transport_event enumeration values.
>   * @param msgThe message to send.
> - * @return   Number of bytes send, or negative value in case of an error.
> + * @return   Number of bytes sent, or negative value in case of an error.
>   */
>  int transport_peer(struct transport *t, struct fdarray *fda,
>  enum transport_event event, struct ptp_message *msg);
> @@ -96,7 +96,7 @@ int transport_peer(struct transport *t, struct fdarray *fda,
>   * @param event  One of the @ref transport_event enumeration values.
>   * @param msgThe message to send. The address of the destination has 
> to
>   *   be set in the address field.
> - * @return   Number of bytes send, or negative value in case of an error.
> + * @return   Number of bytes sent, or negative value in case of an error.
>   */
>  int transport_sendto(struct transport *t, struct fdarray *fda,
>enum transport_event event, struct ptp_message *msg);
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH V1 1/2] Decouple servo state from automotive profile.

2020-05-18 Thread Jacob Keller



On 5/16/2020 7:42 AM, Richard Cochran wrote:
> From: Vincent Cheng 
> 
> The logic for the Automotive Profile added a message interval update
> mechanism that triggers whenever the servo enters the "stable locked"
> state.  This SERVO_LOCKED_STABLE state is active when the
> configuration option servo_offset_threshold is non-zero and
> servo_offset_threshold criteria is satisfied.
> 
> However, in general, the state of the servo can and should be
> independent of any profile specific optional behavior.  In particular,
> the "stable locked" state will be used in the future to trigger other
> kinds useful logic.  For example, an upcoming write phase mode feature
> would like to take advantage of the SERVO_LOCKED_STABLE state to
> trigger its activation.
> 

Makes sense.

> This patch introduces a proper configuration option to enable
> transmission of the message interval request that is specific to the
> Automotive Profile.>
> Signed-off-by: Vincent Cheng 
> Signed-off-by: Richard Cochran 
> ---
>  config.c |  1 +
>  configs/automotive-slave.cfg |  1 +
>  configs/default.cfg  |  1 +
>  port.c   | 41 +++-
>  port_private.h   |  1 +
>  ptp4l.8  | 18 ++--
>  6 files changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/config.c b/config.c
> index 53ad788..0cbab4c 100644
> --- a/config.c
> +++ b/config.c
> @@ -271,6 +271,7 @@ struct config_item config_tab[] = {
>   GLOB_ITEM_STR("manufacturerIdentity", "00:00:00"),
>   GLOB_ITEM_INT("max_frequency", 9, 0, INT_MAX),
>   PORT_ITEM_INT("min_neighbor_prop_delay", -2000, INT_MIN, -1),
> + PORT_ITEM_INT("msg_interval_request", 0, 0, 1),
>   PORT_ITEM_INT("neighborPropDelayThresh", 2000, 0, INT_MAX),
>   PORT_ITEM_INT("net_sync_monitor", 0, 0, 1),
>   PORT_ITEM_ENU("network_transport", TRANS_UDP_IPV4, nw_trans_enu),
> diff --git a/configs/automotive-slave.cfg b/configs/automotive-slave.cfg
> index 8cc7221..0898660 100644
> --- a/configs/automotive-slave.cfg
> +++ b/configs/automotive-slave.cfg
> @@ -33,5 +33,6 @@ ignore_source_id1
>  step_threshold  1
>  operLogSyncInterval 0
>  operLogPdelayReqInterval 2
> +msg_interval_request 1
>  servo_offset_threshold   30
>  servo_num_offset_values  10
> diff --git a/configs/default.cfg b/configs/default.cfg
> index 119df7b..91f6aaa 100644
> --- a/configs/default.cfg
> +++ b/configs/default.cfg
> @@ -77,6 +77,7 @@ max_frequency   9
>  clock_servo  pi
>  sanity_freq_limit2
>  ntpshm_segment   0
> +msg_interval_request 0
>  servo_num_offset_values 10
>  servo_offset_threshold  0
>  #
> diff --git a/port.c b/port.c
> index 6b87bc9..368c3b8 100644
> --- a/port.c
> +++ b/port.c
> @@ -1131,6 +1131,30 @@ static void port_slave_priority_warning(struct port *p)
>   pr_warning("port %hu: defaultDS.priority1 probably misconfigured", n);
>  }
>  
> +static void message_interval_request(struct port *p,
> +  enum servo_state last_state,
> +  Integer8 sync_interval)
> +{
> + if (!p->msg_interval_request)
> + return;
> +

The documentation says this is only supported if BMCA is noop, but is
that enforced anywhere in the code?

> + if (last_state == SERVO_LOCKED) {
> + p->logPdelayReqInterval = p->operLogPdelayReqInterval;
> + p->logSyncInterval = p->operLogSyncInterval;
> + port_tx_interval_request(p, SIGNAL_NO_CHANGE,
> +  p->logSyncInterval,
> +  SIGNAL_NO_CHANGE);
> + port_dispatch(p, EV_MASTER_CLOCK_SELECTED, 0);
> + } else if (sync_interval != p->operLogSyncInterval) {
> + /*
> +  * The most likely reason for this to happen is the
> +  * master daemon re-initialized due to some fault.
> +  */
> + servo_reset(clock_servo(p->clock));
> + port_dispatch(p, EV_SYNCHRONIZATION_FAULT, 0);
> + }
> +}
> +
>  static void port_synchronize(struct port *p,
>tmv_t ingress_ts,
>struct timestamp origin_ts,
> @@ -1174,21 +1198,7 @@ static void port_synchronize(struct port *p,
>   port_dispatch(p, EV_MASTER_CLOCK_SELECTED, 0);
>   break;
>   case SERVO_LOCKED_STABLE:
> - if (last_state == SERVO_LOCKED) {
> - p->logPdelayReqInterval = p->operLogPdelayReqInterval;
> - p->logSyncInterval = p->operLogSyncInterval;
> - port_tx_interval_request(p, SIGNAL_NO_CHANGE,
> -  p->logSyncInterval,
> -  SIGNAL_NO_CHANGE);
> - port_dispatch(p, EV_MASTER_CLOCK_SELECTED, 0);
> -  

Re: [Linuxptp-devel] [PATCH V1 RFC 0/5] GPS based Grand Master Support

2020-05-01 Thread Jacob Keller



On 5/1/2020 12:05 PM, Richard Cochran wrote:
> This series adds support for substantial new features.
> 
> 1. Using a 1-PPS signal from a GPS in order to become a Grand Master
>from a high quality, globally traceable time source.
> 
> 2. Using a heterogeneous group of PHC cards wired together via their
>auxiliary pins to form a Transparent Clock whose ports are
>synchronized in hardware.
> 
> 3. Unlocking many testing scenarios with PHC cards synchronized in
>hardware.
> 

Excellent! I'm excited to read through these.

> Credit for the original ts2phc program goes to Balint Ferencz.  This
> series is a further development of his work.
> 
> There a couple of TODOs outstanding, but the code is already quite
> usable.
> 
> Richard Cochran (5):
>   Introduce the ts2phc program.
>   ts2phc: Support using a PHC as the master clock.
>   Introduce a leap second table.
>   ts2phc: Support using a GPS radio as the master clock.
>   Add documentation for the ts2phc program.
> 
>  .gitignore |   1 +
>  config.c   |  17 ++
>  configs/ts2phc-TC.cfg  |  27 +++
>  configs/ts2phc-generic.cfg |  18 ++
>  lstab.c| 206 ++
>  lstab.h|  64 ++
>  makefile   |  11 +-
>  nmea.c | 202 ++
>  nmea.h |  44 
>  serial.c   |  96 +
>  serial.h   |  19 ++
>  sock.c |  58 +
>  sock.h |  17 ++
>  ts2phc.8   | 217 +++
>  ts2phc.c   | 209 ++
>  ts2phc_generic_master.c|  63 ++
>  ts2phc_generic_master.h|  14 ++
>  ts2phc_master.c|  38 
>  ts2phc_master.h|  52 +
>  ts2phc_master_private.h|  20 ++
>  ts2phc_nmea_master.c   | 227 
>  ts2phc_nmea_master.h   |  13 ++
>  ts2phc_phc_master.c| 113 ++
>  ts2phc_phc_master.h|  14 ++
>  ts2phc_slave.c | 425 +
>  ts2phc_slave.h |  20 ++
>  26 files changed, 2202 insertions(+), 3 deletions(-)
>  create mode 100644 configs/ts2phc-TC.cfg
>  create mode 100644 configs/ts2phc-generic.cfg
>  create mode 100644 lstab.c
>  create mode 100644 lstab.h
>  create mode 100644 nmea.c
>  create mode 100644 nmea.h
>  create mode 100644 serial.c
>  create mode 100644 serial.h
>  create mode 100644 sock.c
>  create mode 100644 sock.h
>  create mode 100644 ts2phc.8
>  create mode 100644 ts2phc.c
>  create mode 100644 ts2phc_generic_master.c
>  create mode 100644 ts2phc_generic_master.h
>  create mode 100644 ts2phc_master.c
>  create mode 100644 ts2phc_master.h
>  create mode 100644 ts2phc_master_private.h
>  create mode 100644 ts2phc_nmea_master.c
>  create mode 100644 ts2phc_nmea_master.h
>  create mode 100644 ts2phc_phc_master.c
>  create mode 100644 ts2phc_phc_master.h
>  create mode 100644 ts2phc_slave.c
>  create mode 100644 ts2phc_slave.h
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH V3 0/6] Clean up in preparation for GrandMaster support.

2020-03-23 Thread Jacob Keller



On 3/21/2020 4:57 AM, Richard Cochran wrote:
> I have been preparing Balint Ferencz's ts2phc program for review on
> this list.  The present series presents a mixture of fixes and new
> code in support of the upcoming GrandMaster features.
> 
> Patches 1-3 are random fixes found along the way.
> Patches 4-6 add new helper code, preparing for GM support.
> 
> Changes in V3
> ~
> - use compat_ptp_clock_caps per Jacob's review
> - pick up Jacob's review tags
> 

V3 looks good to me!

Thanks,
Jake

> Changes in V2
> ~
> - introduce an out-of-tree 'struct ptp_clock_caps' in missing.h
> 
> 
> Richard Cochran (6):
>   clock: Safely remove event subscribers from list.
>   Remove the unfinished SNMP code.
>   Balance the posix clock open function with a close method.
>   Add definitions for PTP pin ioctls for backwards kernel compatibility.
>   Add PHC methods for querying and configuring the pin functionality.
>   Provide a method to convert a tmv_t into a timespec.
> 
>  clock.c |   9 +--
>  makefile|  14 
>  missing.h   |  60 ++-
>  phc.c   |  23 +-
>  phc.h   |  21 ++
>  phc2sys.c   |   2 +-
>  phc_ctl.c   |   1 +
>  snmp4lptp.8 | 119 --
>  snmp4lptp.c | 192 
>  snmp4lptp_mib.h |  30 
>  snmpflg.sh  |  42 ---
>  tmv.h   |  10 +++
>  util.c  |   8 ++
>  util.h  |   6 ++
>  14 files changed, 131 insertions(+), 406 deletions(-)
>  delete mode 100644 snmp4lptp.8
>  delete mode 100644 snmp4lptp.c
>  delete mode 100644 snmp4lptp_mib.h
>  delete mode 100755 snmpflg.sh
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] PPS generation on BeagleBoneBlack

2020-03-18 Thread Jacob Keller



On 3/16/2020 1:58 PM, Grygorii Strashko via Linuxptp-devel wrote:
>> This is the userspace program I have worked on to get the synchronization
>> working. Currently the servo part of the tool is in very hacky shape. Before
>> cleanup I wanted to get more inputs on where it can be integrated. But 
>> anyways
>> as you said, Ill clean it up and post it as a separate tool. We can then 
>> decide
>> the correct location.
> 
> And I'd like to rise additional question here.
> The above solution adds PPS generation using  PTP_EXTTS events and PIN output,
> but, as per my understanding, there are no way now to feed phc2sys with such 
> PPS.
> So question is how PPS generated by PTP_EXTTS events can be wired to phc2sys?
>   - option1: update phc2sys to accept PTP_EXTTS as PPS
>   - option2: allow kernel PHC to generate or PTP_CLOCK_EXTTS/or 
> PTP_CLOCK_PPS/or both
> by somehow configuring that PTP_EXTTSx is also PPS.
> Challenge with option 2 is that Kernel PPS configuration is part of
> ptp_clock_register() and so static.
> 

I think a new option that can use PTP_EXTTS as an input, and some option
to specify what the expected period of that signal is, so that any rate
could be used, not just 1 PPS.

So, a variation of option 1, where phc2sys is modified to have a mode
for PTP_EXTTS which is somewhat distinct from PPS mode.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH V2 4/6] Add definitions for PTP pin ioctls for backwards kernel compatibility.

2020-03-11 Thread Jacob Keller
On 3/11/2020 11:24 AM, Richard Cochran wrote:
> Upcoming functionality will need to configure the input and output pins of
> PHC devices.  However, this requires fairly recent kernel support.  This
> patch adds the needed definitions for compiling with older kernel headers.
> 
> In addition, kernel v5.4 introduced a second set of ioctls for the
> ancillary PTP Hardware Clock functionality.  The original ioctls
> failed to enforce the various flags and reversed fields, but the
> second version has fixed the issues.  Going forward, our user space
> PTP stack ought to use the newer ioctls (if available) from day one.
> 
> Signed-off-by: Richard Cochran 
> ---
>  missing.h | 62 ++-
>  phc.c |  8 +++
>  phc_ctl.c |  2 +-
>  3 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/missing.h b/missing.h
> index 8f92079..e12e12f 100644
> --- a/missing.h
> +++ b/missing.h
> @@ -23,9 +23,10 @@
>  #ifndef HAVE_MISSING_H
>  #define HAVE_MISSING_H
>  
> -#include 
> +#include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #ifndef ADJ_TAI
> @@ -60,6 +61,65 @@ enum {
>  };
>  #endif
>  
> +#ifdef PTP_EXTTS_REQUEST2
> +#define PTP_EXTTS_REQUEST_FAILED "PTP_EXTTS_REQUEST2 failed: %m"
> +#else
> +#define PTP_EXTTS_REQUEST_FAILED "PTP_EXTTS_REQUEST failed: %m"
> +#define PTP_EXTTS_REQUEST2 PTP_EXTTS_REQUEST
> +#endif
> +
> +#ifdef PTP_PEROUT_REQUEST2
> +#define PTP_PEROUT_REQUEST_FAILED "PTP_PEROUT_REQUEST2 failed: %m"
> +#else
> +#define PTP_PEROUT_REQUEST_FAILED "PTP_PEROUT_REQUEST failed: %m"
> +#define PTP_PEROUT_REQUEST2 PTP_PEROUT_REQUEST
> +#endif
> +
> +#ifdef PTP_PIN_SETFUNC
> +
> +#define ptp_clock_capabilities ptp_clock_caps
> +

I'm used to seeing this done like

#ifndef PTP_PIN_SETFUNC

struct compat_ptp_clock_caps {
}

#define ptp_clock_caps compat_ptp_clock_caps

#endif

so that the other code doesn't need to change, as the ptp_clock_caps
gets turned into compat_ptp_clock_caps.

Thanks,
Jake

> +#else /*PTP_PIN_SETFUNC*/
> +
> +/* from Linux kernel version 5.4 */
> +struct ptp_clock_capabilities {
> + int max_adj;   /* Maximum frequency adjustment in parts per billon. */
> + int n_alarm;   /* Number of programmable alarms. */
> + int n_ext_ts;  /* Number of external time stamp channels. */
> + int n_per_out; /* Number of programmable periodic signals. */
> + int pps;   /* Whether the clock supports a PPS callback. */
> + int n_pins;/* Number of input/output pins. */
> + /* Whether the clock supports precise system-device cross timestamps */
> + int cross_timestamping;
> + int rsv[13];   /* Reserved for future use. */
> +};
> +
> +enum ptp_pin_function {
> + PTP_PF_NONE,
> + PTP_PF_EXTTS,
> + PTP_PF_PEROUT,
> + PTP_PF_PHYSYNC,
> +};
> +
> +struct ptp_pin_desc {
> + char name[64];
> + unsigned int index;
> + unsigned int func;
> + unsigned int chan;
> + unsigned int rsv[5];
> +};
> +
> +#define PTP_PIN_SETFUNC_IOW(PTP_CLK_MAGIC, 7, struct ptp_pin_desc)
> +
> +#endif /*!PTP_PIN_SETFUNC*/
> +
> +#ifdef PTP_PIN_SETFUNC2
> +#define PTP_PIN_SETFUNC_FAILED "PTP_PIN_SETFUNC2 failed: %m"
> +#else
> +#define PTP_PIN_SETFUNC_FAILED "PTP_PIN_SETFUNC failed: %m"
> +#define PTP_PIN_SETFUNC2 PTP_PIN_SETFUNC
> +#endif
> +
>  #ifndef LIST_FOREACH_SAFE
>  #define  LIST_FOREACH_SAFE(var, head, field, tvar)   
> \
>   for ((var) = LIST_FIRST((head));\
> diff --git a/phc.c b/phc.c
> index a90d13e..5482f8a 100644
> --- a/phc.c
> +++ b/phc.c
> @@ -37,7 +37,7 @@
>  #define BITS_PER_LONG(sizeof(long)*8)
>  #define MAX_PPB_32   32767999/* 2^31 - 1 / 65.536 */
>  
> -static int phc_get_caps(clockid_t clkid, struct ptp_clock_caps *caps);
> +static int phc_get_caps(clockid_t clkid, struct ptp_clock_capabilities 
> *caps);
>  
>  clockid_t phc_open(const char *phc)
>  {
> @@ -74,7 +74,7 @@ void phc_close(clockid_t clkid)
>   close(CLOCKID_TO_FD(clkid));
>  }
>  
> -static int phc_get_caps(clockid_t clkid, struct ptp_clock_caps *caps)
> +static int phc_get_caps(clockid_t clkid, struct ptp_clock_capabilities *caps)
>  {
>   int fd = CLOCKID_TO_FD(clkid), err;
>  
> @@ -86,8 +86,8 @@ static int phc_get_caps(clockid_t clkid, struct 
> ptp_clock_caps *caps)
>  
>  int phc_max_adj(clockid_t clkid)
>  {
> + struct ptp_clock_capabilities caps;
>   int max;
> - struct ptp_clock_caps caps;
>  
>   if (phc_get_caps(clkid, ))
>   return 0;
> @@ -102,7 +102,7 @@ int phc_max_adj(clockid_t clkid)
>  
>  int phc_has_pps(clockid_t clkid)
>  {
> - struct ptp_clock_caps caps;
> + struct ptp_clock_capabilities caps;
>  
>   if (phc_get_caps(clkid, ))
>   return 0;
> diff --git a/phc_ctl.c b/phc_ctl.c
> index 149ee9e..fa4aaa5 100644
> --- a/phc_ctl.c
> +++ b/phc_ctl.c
> @@ -301,7 +301,7 @@ static int do_freq(clockid_t clkid, int cmdc, char 
> *cmdv[])
>  
>  

Re: [Linuxptp-devel] [PATCH V2 2/6] Remove the unfinished SNMP code.

2020-03-11 Thread Jacob Keller



On 3/11/2020 11:24 AM, Richard Cochran wrote:
> Unfortunately the SNMP code still has issues like not passing the
> valgrind test, and no one is able to finish this up right now.  This
> patch removes the SNMP program so that the upcoming release does not
> contain unfinished work, potentially misleading end users about the
> scope and completeness of the features.
> 
> Signed-off-by: Richard Cochran 

Makes sense. It can always be revived.

Reviewed-by: Jacob Keller 

> ---
>  makefile|  14 
>  snmp4lptp.8 | 119 --
>  snmp4lptp.c | 192 
>  snmp4lptp_mib.h |  30 
>  snmpflg.sh  |  42 ---
>  5 files changed, 397 deletions(-)
>  delete mode 100644 snmp4lptp.8
>  delete mode 100644 snmp4lptp.c
>  delete mode 100644 snmp4lptp_mib.h
>  delete mode 100755 snmpflg.sh
> 
> diff --git a/makefile b/makefile
> index e1dd3fa..a23945a 100644
> --- a/makefile
> +++ b/makefile
> @@ -38,16 +38,9 @@ SRC= $(OBJECTS:.o=.c)
>  DEPEND   = $(OBJECTS:.o=.d)
>  srcdir   := $(dir $(lastword $(MAKEFILE_LIST)))
>  incdefs := $(shell $(srcdir)/incdefs.sh)
> -snmpflg  := $(shell $(srcdir)/snmpflg.sh)
>  version := $(shell $(srcdir)/version.sh $(srcdir))
>  VPATH= $(srcdir)
>  
> -ifneq (,$(findstring -DHAVE_NET_SNMP,$(snmpflg)))
> -PRG  += snmp4lptp
> -OBJECTS  += snmp4lptp.o
> -snmplib  := $(shell net-snmp-config --netsnmp-agent-libs)
> -endif
> -
>  prefix   = /usr/local
>  sbindir  = $(prefix)/sbin
>  mandir   = $(prefix)/man
> @@ -71,13 +64,6 @@ hwstamp_ctl: hwstamp_ctl.o version.o
>  
>  phc_ctl: phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o
>  
> -snmp4lptp: config.o hash.o interface.o msg.o phc.o pmc_common.o print.o sk.o 
> \
> - snmp4lptp.o tlv.o $(TRANSP) util.o
> - $(CC) $^ $(LDFLAGS) $(LOADLIBES) $(LDLIBS) $(snmplib) -o $@
> -
> -snmp4lptp.o: snmp4lptp.c
> - $(CC) $(CPPFLAGS) $(CFLAGS) $(snmpflg) -c $<
> -
>  timemaster: phc.o print.o rtnl.o sk.o timemaster.o util.o version.o
>  
>  version.o: .version version.sh $(filter-out version.d,$(DEPEND))
> diff --git a/snmp4lptp.8 b/snmp4lptp.8
> deleted file mode 100644
> index 7dda4d0..000
> --- a/snmp4lptp.8
> +++ /dev/null
> @@ -1,119 +0,0 @@
> -.TH SNMP4LPTP 8 "September 2018" "linuxptp"
> -.SH NAME
> -snmp4lptp - SNMP sub agent
> -
> -.SH SYNOPSIS
> -.B snmp4lptp
> -[
> -.BI \-f " config-file"
> -] [
> -.B \-m
> -] [
> -.B \-q
> -] [
> -.I long-options
> -]
> -
> -.SH DESCRIPTION
> -.B snmp4lptp
> -is an implementation of a sub agent for handling SNMP requests on
> -the device running ptp4l. Via the UDS port, the sub agent retrieves
> -management information from ptp4l and translates the information to
> -or from an SNMP-specific form. 
> -
> -.SH OPTIONS
> -.TP
> -.BI \-f " file"
> -Specify the path to the \fBsnmp4lptp\fR configuration file.
> -.TP
> -.B \-h
> -Display a help message.
> -.TP
> -.B \-m
> -Print messages to the standard output.
> -.TP
> -.B \-q
> -Don't send messages to the system logger.
> -
> -.SH LONG OPTIONS
> -
> -Each and every configuration file option (see below in sections
> -.BR PROGRAM\ OPTIONS
> -and
> -.BR PORT\ OPTIONS )
> -may also appear
> -as a "long" style command line argument. For example, the transportSpecific
> -option may be set using either of these two forms:
> -
> -.RS
> -\f(CW\-\-transportSpecific 1   \-\-transportSpecific=1\fP
> -.RE
> -
> -Option values given on the command line override values in the global
> -section of the configuration file (which, in turn, overrides default
> -values).
> -
> -.SH CONFIGURATION FILE
> -
> -The configuration file is divided into sections. Each section starts with a
> -line containing its name enclosed in brackets and it follows with settings.
> -Each setting is placed on a separate line, it contains the name of the
> -option and the value separated by whitespace characters. Empty lines and 
> lines
> -starting with # are ignored.
> -
> -The global section (indicated as
> -.BR [global] )
> -sets the global program options as well as the default port specific options.
> -Other sections are port specific sections and they override the default port
> -options. The name of the section is the name of the configured port (e.g.
> -.BR [eth0]
> -). Currently no port specific options other than default are considered.
> -
> -.SH PROGRAM OPTIONS
> -.TP
> -.B domainNumber
> -The domain attribute of the local clock.
> -The default is 0.
> -.TP
>

Re: [Linuxptp-devel] [PATCH V2 1/6] clock: Safely remove event subscribers from list.

2020-03-11 Thread Jacob Keller
On 3/11/2020 11:24 AM, Richard Cochran wrote:
> When updating and potentially removing event subscribers, the code uses
> the simple list traversal macro.  As a result, the list will become
> corrupted whenever a subscriber is removed.  This patch fixes the issue
> by using the appropriate macro.
> 
> Fixes: 5104e3e56b59 ("Event subscribing")
> Signed-off-by: Richard Cochran 
> Reported-by: Michael Walle 

Straight forward conversion.

Reviewed-by: Jacob Keller 

> ---
>  clock.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 1668383..6f9cc21 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -145,9 +145,9 @@ static void remove_subscriber(struct clock_subscriber *s)
>  static void clock_update_subscription(struct clock *c, struct ptp_message 
> *req,
> uint8_t *bitmask, uint16_t duration)
>  {
> - struct clock_subscriber *s;
> - int i, remove = 1;
> + struct clock_subscriber *s, *tmp;
>   struct timespec now;
> + int i, remove = 1;
>  
>   for (i = 0; i < EVENT_BITMASK_CNT; i++) {
>   if (bitmask[i]) {
> @@ -156,12 +156,11 @@ static void clock_update_subscription(struct clock *c, 
> struct ptp_message *req,
>   }
>   }
>  
> - LIST_FOREACH(s, >subscribers, list) {
> + LIST_FOREACH_SAFE(s, >subscribers, list, tmp) {
>   if (pid_eq(>targetPortIdentity,
>  >header.sourcePortIdentity)) {
> - /* Found, update the transport address and event
> -  * mask. */
>   if (!remove) {
> + /* Update transport address and event mask. */
>   s->addr = req->address;
>   memcpy(s->events, bitmask, EVENT_BITMASK_CNT);
>   clock_gettime(CLOCK_MONOTONIC, );
> 


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 5/6] Add PHC methods for querying and configuring the pin functionality.

2020-03-09 Thread Jacob Keller
On 3/9/2020 7:27 AM, Jiri Benc wrote:
> On Mon, 9 Mar 2020 07:16:23 -0700, Richard Cochran wrote:
>> But maybe that wouldn't be worst thing in the world.  There is a trade
>> off between maintaining parallel copies of ptp_clock_caps and the
>> convenience of compiling the stack just once with the "future" kernel
>> definitions.
>>
>> Thoughts?
> 
> The decision is ultimately yours but I'd prefer the copy of the struct
> in missing.h.
> 
> Thanks,
> 
>  Jiri
> 

It would be my preference as well to keep a copy in missing.h

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH RFC 20/30] interface: Introduce a method to test the time stamping information validity.

2020-03-05 Thread Jacob Keller
On 3/4/2020 9:13 AM, Richard Cochran wrote:
> On Tue, Feb 18, 2020 at 01:19:09PM -0800, Jacob Keller wrote:
>>> +bool interface_tsinfo_valid(struct interface *iface)
>>> +{
>>> +   return iface->ts_info.valid ? true : false;
>>> +}
>>
>> Do you actually need the ternary here? shouldn't ts_info.valid get
>> converted to true or false because we are returning a boolean?
> 
> Right.
>  
>> I don't think this is harmful and you may consider it improving
>> readability though.
> 
> Yeah, that is the reason.  I like to have it spelled out in this case.
> 
> Thanks,
> Richard
> 

Sure.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH RFC 14/30] interface: Introduce a method to set the name.

2020-03-05 Thread Jacob Keller
On 3/4/2020 9:06 AM, Richard Cochran wrote:
> On Tue, Feb 18, 2020 at 01:07:52PM -0800, Jacob Keller wrote:
>>
>> Good, the name is marked as constant. Side note, for those interface_*
>> functions that don't modify the interface, does it make sense to mark
>> them as taking a const interface pointer?
> 
> I have heard C++ people insisting on this, but frankly IMHO it doesn't
> make any sense.  If the implementation of the "class" is completely
> hidden, as it should be, then the caller should not care what happens
> offstage.  That is none of the caller's business.
> 
> Thanks,
> Richard
> 

Sure.

Thanks,
Jake


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


  1   2   >