[Linuxptp-devel] [PATCH] hwtstamp_ctl: add support for SIOCGHWTSTAMP

[Jacob Keller]

This patch adds support to hwtstamp_ctl for obtaining the current
settings of the device, if the feature is supported. This can be useful
for debugging what mode the device/driver thinks its in.

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

diff --git a/hwstamp_ctl.c b/hwstamp_ctl.c
index 456d96b09147..cca18422fb50 100644
--- a/hwstamp_ctl.c
+++ b/hwstamp_ctl.c
@@ -84,11 +84,12 @@ int main(int argc, char *argv[])
struct hwtstamp_config cfg;
char *device = NULL, *progname;
int c, err, fd, rxopt = HWTSTAMP_FILTER_NONE, txopt = HWTSTAMP_TX_OFF;
+   int getcfg = 0;
 
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
-   while (EOF != (c = getopt(argc, argv, "hi:r:t:v"))) {
+   while (EOF != (c = getopt(argc, argv, "hi:r:t:vg"))) {
switch (c) {
case 'i':
device = optarg;
@@ -99,6 +100,9 @@ int main(int argc, char *argv[])
case 't':
txopt = atoi(optarg);
break;
+   case 'g':
+   getcfg = 1;
+   break;
case 'v':
version_show(stdout);
return 0;
@@ -138,7 +142,7 @@ int main(int argc, char *argv[])
return -1;
}
 
-   err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+   err = ioctl(fd, getcfg ? SIOCGHWTSTAMP : SIOCSHWTSTAMP, &ifreq);
if (err < 0) {
err = errno;
perror("SIOCSHWTSTAMP failed");
-- 
1.9.0


--
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH] hwtstamp_ctl: add support for SIOCGHWTSTAMP

[Jacob Keller]

This patch adds support to hwtstamp_ctl for obtaining the current
settings of the device, if the feature is supported. This can be useful
for debugging what mode the device/driver thinks its in.

- v2
* order options alphabetically
* print correct ioctl name in error message

Signed-off-by: Jacob Keller 
---
 hwstamp_ctl.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hwstamp_ctl.c b/hwstamp_ctl.c
index 456d96b09147..1ab5ab6e3323 100644
--- a/hwstamp_ctl.c
+++ b/hwstamp_ctl.c
@@ -84,12 +84,16 @@ int main(int argc, char *argv[])
struct hwtstamp_config cfg;
char *device = NULL, *progname;
int c, err, fd, rxopt = HWTSTAMP_FILTER_NONE, txopt = HWTSTAMP_TX_OFF;
+   int getcfg = 0;
 
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
-   while (EOF != (c = getopt(argc, argv, "hi:r:t:v"))) {
+   while (EOF != (c = getopt(argc, argv, "ghi:r:t:v"))) {
switch (c) {
+   case 'g':
+   getcfg = 1;
+   break;
case 'i':
device = optarg;
break;
@@ -138,11 +142,11 @@ int main(int argc, char *argv[])
return -1;
}
 
-   err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+   err = ioctl(fd, getcfg ? SIOCGHWTSTAMP : SIOCSHWTSTAMP, &ifreq);
if (err < 0) {
err = errno;
-   perror("SIOCSHWTSTAMP failed");
-   if (err == ERANGE)
+   perror(getcfg ? "SIOCGHWTSTAMP failed" : "SIOCSHWTSTAMP 
failed");
+   if (err == ERANGE && !getcfg)
fprintf(stderr, "The requested time stamping mode is 
not supported by the hardware.\n");
}
 
-- 
1.9.0


--
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 2/2] hwtstamp_ctl: use SIOCGHWTSTAMP ioctl before destructively setting policy

[Jacob Keller]

This patch modifies the hwtstamp_ctl program, so that it will (attempt
to) use the SIOCGHWTSTAMP ioctl to non-destructively read the current
hardware timestamping policy, prior to setting it with SIOCSHWTSTAMP.

This change has 3 primary advantages:

1) It allows reading the current settings of the device, which was
   previously not possible since SIOCSHWTSTAMP is destructive.
2) The default behavior without rx-filter or tx-type selected on the
   command line is no longer destructive, since it does not attempt to
   set the values to 0. The user must explicitly request to disable the
   settings, by using the provided options.
3) It allows only modifying tx-type or rx-filter separately, without
   destroying the other setting.

This patch supersedes a previous submission which added a -g flag. This
new method of getting first is more advantageous and doesn't require
adding an additional option flag.

Signed-off-by: Jacob Keller 
---
 hwstamp_ctl.8 | 12 ++--
 hwstamp_ctl.c | 45 ++---
 2 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/hwstamp_ctl.8 b/hwstamp_ctl.8
index cece74f61866..7bfdf4451839 100644
--- a/hwstamp_ctl.8
+++ b/hwstamp_ctl.8
@@ -15,11 +15,10 @@ hwstamp_ctl \- set time stamping policy at the driver level
 
 .SH DESCRIPTION
 .B hwstamp_ctl
-is a program used to set the hardware time stamping policy at the network
+is a program used to set and get the hardware time stamping policy at the 
network
 driver level with the
 .B SIOCSHWTSTAMP
 .BR ioctl (2).
-The
 .I tx-type
 and
 .I rx-filter
@@ -27,6 +26,15 @@ values are hints to the driver what it is expected to do. If 
the requested
 fine-grained filtering for incoming packets is not supported, the driver may
 time stamp more than just the requested types of packets.
 
+If neither
+.I tx-type
+nor
+.I rx-filter
+values are passed to the program, it will use the
+.B SIOCGHWTSTAMP
+.BR ioctl(2)
+to non-destructively read the current hardware time stamping policy.
+
 This program is a debugging tool. The
 .BR ptp4l (8)
 program does not need this program to function, it will set the policy
diff --git a/hwstamp_ctl.c b/hwstamp_ctl.c
index 456d96b09147..df954fceb321 100644
--- a/hwstamp_ctl.c
+++ b/hwstamp_ctl.c
@@ -30,6 +30,7 @@
 #include 
 
 #include "version.h"
+#include "missing.h"
 
 static void usage(char *progname)
 {
@@ -84,6 +85,7 @@ int main(int argc, char *argv[])
struct hwtstamp_config cfg;
char *device = NULL, *progname;
int c, err, fd, rxopt = HWTSTAMP_FILTER_NONE, txopt = HWTSTAMP_TX_OFF;
+   int setrx = 0, settx = 0;
 
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
@@ -94,9 +96,11 @@ int main(int argc, char *argv[])
device = optarg;
break;
case 'r':
+   setrx = 1;
rxopt = atoi(optarg);
break;
case 't':
+   settx = 1;
txopt = atoi(optarg);
break;
case 'v':
@@ -116,6 +120,7 @@ int main(int argc, char *argv[])
usage(progname);
return -1;
}
+
if (rxopt < HWTSTAMP_FILTER_NONE ||
rxopt > HWTSTAMP_FILTER_PTP_V2_DELAY_REQ ||
txopt < HWTSTAMP_TX_OFF || txopt > HWTSTAMP_TX_ON) {
@@ -129,8 +134,6 @@ int main(int argc, char *argv[])
strncpy(ifreq.ifr_name, device, sizeof(ifreq.ifr_name));
 
ifreq.ifr_data = (void *) &cfg;
-   cfg.tx_type= txopt;
-   cfg.rx_filter  = rxopt;
 
fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
if (fd < 0) {
@@ -138,15 +141,43 @@ int main(int argc, char *argv[])
return -1;
}
 
-   err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+   /* First, attempt to get the current settings. */
+   err = ioctl(fd, SIOCGHWTSTAMP, &ifreq);
if (err < 0) {
err = errno;
-   perror("SIOCSHWTSTAMP failed");
-   if (err == ERANGE)
-   fprintf(stderr, "The requested time stamping mode is 
not supported by the hardware.\n");
+   if (err == ENOTTY)
+   fprintf(stderr, "Kernel does not have support for 
non-destructive for SIOCGHWTSTAMP.\n");
+   else if (err == EOPNOTSUPP)
+   fprintf(stderr, "Device driver does not have support 
for non-destructive SIOCGHWTSTAMP.\n");
+   else
+   perror("SIOCGHWTSTAMP failed");
+   } else {
+   printf("current settings:\n" "tx_type %d\n" "rx_filter %d\n",
+  cfg.tx_type, cfg.rx_filter);
}
 
-   printf("tx_type %d\n&q

[Linuxptp-devel] [PATCH 1/2] missing: add SIOCGHWTSTAMP to missing.h

[Jacob Keller]

In order to allow hwtstamp_ctl to use the non-destructive SIOCGHWTSTAMP
ioctl, we need to add it to missing.h, in order to prevent build failure
on older kernels which don't have this support.

Signed-off-by: Jacob Keller 
---
 missing.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/missing.h b/missing.h
index d6b2355cb8fb..5f21fe58d069 100644
--- a/missing.h
+++ b/missing.h
@@ -50,6 +50,10 @@ enum _missing_hwtstamp_tx_types {
 };
 #endif
 
+#ifndef SIOCGHWTSTAMP
+#define SIOCGHWTSTAMP 0x89b1
+#endif
+
 #ifndef HAVE_CLOCK_ADJTIME
 static inline int clock_adjtime(clockid_t id, struct timex *tx)
 {
-- 
1.9.0


--
The best possible search technologies are now affordable for all companies.
Download your FREE open source Enterprise Search Engine today!
Our experts will assist you in its installation for $59/mo, no commitment.
Test it for FREE on our Cloud platform anytime!
http://pubads.g.doubleclick.net/gampad/clk?id=145328191&iu=/4140/ostg.clktrk
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH v2] hwtstamp_ctl: use SIOCGHWTSTAMP ioctl before destructively setting policy

[Jacob Keller]

This patch modifies the hwtstamp_ctl program, so that it will (attempt
to) use the SIOCGHWTSTAMP ioctl to non-destructively read the current
hardware timestamping policy, prior to setting it with SIOCSHWTSTAMP.

This change has 3 primary advantages:

1) It allows reading the current settings of the device, which was
   previously not possible since SIOCSHWTSTAMP is destructive.
2) The default behavior without rx-filter or tx-type selected on the
   command line is no longer destructive, since it does not attempt to
   set the values to 0. The user must explicitly request to disable the
   settings, by using the provided options.
3) It allows only modifying tx-type or rx-filter separately, without
   destroying the other setting.

This patch supersedes a previous submission which added a -g flag. This
new method of getting first is more advantageous and doesn't require
adding an additional option flag.

- v2
* Fix line lengths, and a few other minor changes

Signed-off-by: Jacob Keller 
---
 hwstamp_ctl.8 | 11 ++-
 hwstamp_ctl.c | 56 +---
 2 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/hwstamp_ctl.8 b/hwstamp_ctl.8
index cece74f61866..0ae4c4a6a456 100644
--- a/hwstamp_ctl.8
+++ b/hwstamp_ctl.8
@@ -15,7 +15,7 @@ hwstamp_ctl \- set time stamping policy at the driver level
 
 .SH DESCRIPTION
 .B hwstamp_ctl
-is a program used to set the hardware time stamping policy at the network
+is a program used to set and get the hardware time stamping policy at the 
network
 driver level with the
 .B SIOCSHWTSTAMP
 .BR ioctl (2).
@@ -27,6 +27,15 @@ values are hints to the driver what it is expected to do. If 
the requested
 fine-grained filtering for incoming packets is not supported, the driver may
 time stamp more than just the requested types of packets.
 
+If neither
+.I tx-type
+nor
+.I rx-filter
+values are passed to the program, it will use the
+.B SIOCGHWTSTAMP
+.BR ioctl(2)
+to non-destructively read the current hardware time stamping policy.
+
 This program is a debugging tool. The
 .BR ptp4l (8)
 program does not need this program to function, it will set the policy
diff --git a/hwstamp_ctl.c b/hwstamp_ctl.c
index 456d96b09147..149f019bf74c 100644
--- a/hwstamp_ctl.c
+++ b/hwstamp_ctl.c
@@ -30,6 +30,7 @@
 #include 
 
 #include "version.h"
+#include "missing.h"
 
 static void usage(char *progname)
 {
@@ -84,6 +85,7 @@ int main(int argc, char *argv[])
struct hwtstamp_config cfg;
char *device = NULL, *progname;
int c, err, fd, rxopt = HWTSTAMP_FILTER_NONE, txopt = HWTSTAMP_TX_OFF;
+   int setrx = 0, settx = 0;
 
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
@@ -94,9 +96,11 @@ int main(int argc, char *argv[])
device = optarg;
break;
case 'r':
+   setrx = 1;
rxopt = atoi(optarg);
break;
case 't':
+   settx = 1;
txopt = atoi(optarg);
break;
case 'v':
@@ -116,6 +120,7 @@ int main(int argc, char *argv[])
usage(progname);
return -1;
}
+
if (rxopt < HWTSTAMP_FILTER_NONE ||
rxopt > HWTSTAMP_FILTER_PTP_V2_DELAY_REQ ||
txopt < HWTSTAMP_TX_OFF || txopt > HWTSTAMP_TX_ON) {
@@ -129,8 +134,6 @@ int main(int argc, char *argv[])
strncpy(ifreq.ifr_name, device, sizeof(ifreq.ifr_name));
 
ifreq.ifr_data = (void *) &cfg;
-   cfg.tx_type= txopt;
-   cfg.rx_filter  = rxopt;
 
fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
if (fd < 0) {
@@ -138,15 +141,54 @@ int main(int argc, char *argv[])
return -1;
}
 
-   err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+   /* First, attempt to get the current settings. */
+   err = ioctl(fd, SIOCGHWTSTAMP, &ifreq);
if (err < 0) {
err = errno;
-   perror("SIOCSHWTSTAMP failed");
-   if (err == ERANGE)
-   fprintf(stderr, "The requested time stamping mode is 
not supported by the hardware.\n");
+   if (err == ENOTTY)
+   fprintf(stderr,
+   "Kernel does not have support"
+   "for non-destructive SIOCGHWTSTAMP.\n");
+   else if (err == EOPNOTSUPP)
+   fprintf(stderr,
+   "Device driver does not have support"
+   "for non-destructive SIOCGHWTSTAMP.\n");
+   else
+   perror("SIOCGHWTSTAMP failed");
+   } else {
+   printf("current 

[Linuxptp-devel] [PATCH v3] hwtstamp_ctl: use SIOCGHWTSTAMP ioctl before destructively setting policy

[Jacob Keller]

This patch modifies the hwtstamp_ctl program, so that it will (attempt
to) use the SIOCGHWTSTAMP ioctl to non-destructively read the current
hardware timestamping policy, prior to setting it with SIOCSHWTSTAMP.

This change has 3 primary advantages:

1) It allows reading the current settings of the device, which was
   previously not possible since SIOCSHWTSTAMP is destructive.
2) The default behavior without rx-filter or tx-type selected on the
   command line is no longer destructive, since it does not attempt to
   set the values to 0. The user must explicitly request to disable the
   settings, by using the provided options.
3) It allows only modifying tx-type or rx-filter separately, without
   destroying the other setting.

This patch supersedes a previous submission which added a -g flag. This
new method of getting first is more advantageous and doesn't require
adding an additional option flag.

Signed-off-by: Jacob Keller 
---
 hwstamp_ctl.8 | 11 ++-
 hwstamp_ctl.c | 56 +---
 2 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/hwstamp_ctl.8 b/hwstamp_ctl.8
index cece74f61866..0ae4c4a6a456 100644
--- a/hwstamp_ctl.8
+++ b/hwstamp_ctl.8
@@ -15,7 +15,7 @@ hwstamp_ctl \- set time stamping policy at the driver level
 
 .SH DESCRIPTION
 .B hwstamp_ctl
-is a program used to set the hardware time stamping policy at the network
+is a program used to set and get the hardware time stamping policy at the 
network
 driver level with the
 .B SIOCSHWTSTAMP
 .BR ioctl (2).
@@ -27,6 +27,15 @@ values are hints to the driver what it is expected to do. If 
the requested
 fine-grained filtering for incoming packets is not supported, the driver may
 time stamp more than just the requested types of packets.
 
+If neither
+.I tx-type
+nor
+.I rx-filter
+values are passed to the program, it will use the
+.B SIOCGHWTSTAMP
+.BR ioctl(2)
+to non-destructively read the current hardware time stamping policy.
+
 This program is a debugging tool. The
 .BR ptp4l (8)
 program does not need this program to function, it will set the policy
diff --git a/hwstamp_ctl.c b/hwstamp_ctl.c
index 456d96b09147..93da8f6e8494 100644
--- a/hwstamp_ctl.c
+++ b/hwstamp_ctl.c
@@ -30,6 +30,7 @@
 #include 
 
 #include "version.h"
+#include "missing.h"
 
 static void usage(char *progname)
 {
@@ -84,6 +85,7 @@ int main(int argc, char *argv[])
struct hwtstamp_config cfg;
char *device = NULL, *progname;
int c, err, fd, rxopt = HWTSTAMP_FILTER_NONE, txopt = HWTSTAMP_TX_OFF;
+   int setrx = 0, settx = 0;
 
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
@@ -94,9 +96,11 @@ int main(int argc, char *argv[])
device = optarg;
break;
case 'r':
+   setrx = 1;
rxopt = atoi(optarg);
break;
case 't':
+   settx = 1;
txopt = atoi(optarg);
break;
case 'v':
@@ -116,6 +120,7 @@ int main(int argc, char *argv[])
usage(progname);
return -1;
}
+
if (rxopt < HWTSTAMP_FILTER_NONE ||
rxopt > HWTSTAMP_FILTER_PTP_V2_DELAY_REQ ||
txopt < HWTSTAMP_TX_OFF || txopt > HWTSTAMP_TX_ON) {
@@ -129,8 +134,6 @@ int main(int argc, char *argv[])
strncpy(ifreq.ifr_name, device, sizeof(ifreq.ifr_name));
 
ifreq.ifr_data = (void *) &cfg;
-   cfg.tx_type= txopt;
-   cfg.rx_filter  = rxopt;
 
fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
if (fd < 0) {
@@ -138,15 +141,54 @@ int main(int argc, char *argv[])
return -1;
}
 
-   err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+   /* First, attempt to get the current settings. */
+   err = ioctl(fd, SIOCGHWTSTAMP, &ifreq);
if (err < 0) {
err = errno;
-   perror("SIOCSHWTSTAMP failed");
-   if (err == ERANGE)
-   fprintf(stderr, "The requested time stamping mode is 
not supported by the hardware.\n");
+   if (err == ENOTTY)
+   fprintf(stderr,
+   "Kernel does not have support "
+   "for non-destructive SIOCGHWTSTAMP.\n");
+   else if (err == EOPNOTSUPP)
+   fprintf(stderr,
+   "Device driver does not have support "
+   "for non-destructive SIOCGHWTSTAMP.\n");
+   else
+   perror("SIOCGHWTSTAMP failed");
+   } else {
+   printf("current settings:\n"
+  &quo

[Linuxptp-devel] [PATCH v4] hwtstamp_ctl: use SIOCGHWTSTAMP ioctl before destructively setting policy

[Jacob Keller]

This patch modifies the hwtstamp_ctl program, so that it will (attempt
to) use the SIOCGHWTSTAMP ioctl to non-destructively read the current
hardware timestamping policy, prior to setting it with SIOCSHWTSTAMP.

This change has 3 primary advantages:

1) It allows reading the current settings of the device, which was
   previously not possible since SIOCSHWTSTAMP is destructive.
2) The default behavior without rx-filter or tx-type selected on the
   command line is no longer destructive, since it does not attempt to
   set the values to 0. The user must explicitly request to disable the
   settings, by using the provided options.
3) It allows only modifying tx-type or rx-filter separately, without
   destroying the other setting.

This patch supersedes a previous submission which added a -g flag. This
new method of getting first is more advantageous and doesn't require
adding an additional option flag.

- v4
* only display results if command succeeds, as the contents are otherwise
  expected to be identical to what we passed in.

Signed-off-by: Jacob Keller 
---
 hwstamp_ctl.8 | 11 ++-
 hwstamp_ctl.c | 55 ---
 2 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/hwstamp_ctl.8 b/hwstamp_ctl.8
index cece74f61866..0ae4c4a6a456 100644
--- a/hwstamp_ctl.8
+++ b/hwstamp_ctl.8
@@ -15,7 +15,7 @@ hwstamp_ctl \- set time stamping policy at the driver level
 
 .SH DESCRIPTION
 .B hwstamp_ctl
-is a program used to set the hardware time stamping policy at the network
+is a program used to set and get the hardware time stamping policy at the 
network
 driver level with the
 .B SIOCSHWTSTAMP
 .BR ioctl (2).
@@ -27,6 +27,15 @@ values are hints to the driver what it is expected to do. If 
the requested
 fine-grained filtering for incoming packets is not supported, the driver may
 time stamp more than just the requested types of packets.
 
+If neither
+.I tx-type
+nor
+.I rx-filter
+values are passed to the program, it will use the
+.B SIOCGHWTSTAMP
+.BR ioctl(2)
+to non-destructively read the current hardware time stamping policy.
+
 This program is a debugging tool. The
 .BR ptp4l (8)
 program does not need this program to function, it will set the policy
diff --git a/hwstamp_ctl.c b/hwstamp_ctl.c
index 456d96b09147..ec5dd6df1dca 100644
--- a/hwstamp_ctl.c
+++ b/hwstamp_ctl.c
@@ -30,6 +30,7 @@
 #include 
 
 #include "version.h"
+#include "missing.h"
 
 static void usage(char *progname)
 {
@@ -84,6 +85,7 @@ int main(int argc, char *argv[])
struct hwtstamp_config cfg;
char *device = NULL, *progname;
int c, err, fd, rxopt = HWTSTAMP_FILTER_NONE, txopt = HWTSTAMP_TX_OFF;
+   int setrx = 0, settx = 0;
 
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
@@ -94,9 +96,11 @@ int main(int argc, char *argv[])
device = optarg;
break;
case 'r':
+   setrx = 1;
rxopt = atoi(optarg);
break;
case 't':
+   settx = 1;
txopt = atoi(optarg);
break;
case 'v':
@@ -116,6 +120,7 @@ int main(int argc, char *argv[])
usage(progname);
return -1;
}
+
if (rxopt < HWTSTAMP_FILTER_NONE ||
rxopt > HWTSTAMP_FILTER_PTP_V2_DELAY_REQ ||
txopt < HWTSTAMP_TX_OFF || txopt > HWTSTAMP_TX_ON) {
@@ -129,8 +134,6 @@ int main(int argc, char *argv[])
strncpy(ifreq.ifr_name, device, sizeof(ifreq.ifr_name));
 
ifreq.ifr_data = (void *) &cfg;
-   cfg.tx_type= txopt;
-   cfg.rx_filter  = rxopt;
 
fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
if (fd < 0) {
@@ -138,15 +141,53 @@ int main(int argc, char *argv[])
return -1;
}
 
-   err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+   /* First, attempt to get the current settings. */
+   err = ioctl(fd, SIOCGHWTSTAMP, &ifreq);
if (err < 0) {
err = errno;
-   perror("SIOCSHWTSTAMP failed");
-   if (err == ERANGE)
-   fprintf(stderr, "The requested time stamping mode is 
not supported by the hardware.\n");
+   if (err == ENOTTY)
+   fprintf(stderr,
+   "Kernel does not have support "
+   "for non-destructive SIOCGHWTSTAMP.\n");
+   else if (err == EOPNOTSUPP)
+   fprintf(stderr,
+   "Device driver does not have support "
+   "for non-destructive SIOCGHWTSTAMP.\n");
+   else
+   perror("SI

[Linuxptp-devel] [PATCH] linuxptp: add phc_ctl program to help debug PHC devices

[Jacob Keller]

This is an updated version of a script I wrote a couple years ago for
debugging the PHC when writing a new driver. I figured that it might be
handy for the LinuxPTP project to include, as it can give some insight
into the PHC directly. I have updated it to make use of the shared code
here, in order to reduce duplication. Hopefully this is of some use to
everyone.

Signed-off-by: Jacob Keller 
---
 makefile  |   4 +-
 phc_ctl.c | 536 ++
 2 files changed, 539 insertions(+), 1 deletion(-)
 create mode 100644 phc_ctl.c

diff --git a/makefile b/makefile
index e36835b05d40..e9f2f8fcf416 100644
--- a/makefile
+++ b/makefile
@@ -22,7 +22,7 @@ CC= $(CROSS_COMPILE)gcc
 VER = -DVER=$(version)
 CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS)
 LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
-PRG= ptp4l pmc phc2sys hwstamp_ctl
+PRG= ptp4l pmc phc2sys hwstamp_ctl phc_ctl
 OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \
  filter.o fsm.o linreg.o mave.o mmedian.o msg.o ntpshm.o phc.o \
  pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \
@@ -54,6 +54,8 @@ phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o 
phc.o phc2sys.o pi.o \
 
 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
+
 version.o: .version version.sh $(filter-out version.d,$(DEPEND))
 
 .version: force
diff --git a/phc_ctl.c b/phc_ctl.c
new file mode 100644
index ..ff90ce4dbf1b
--- /dev/null
+++ b/phc_ctl.c
@@ -0,0 +1,536 @@
+/*
+ * @file phc2sys.c
+ * @brief Utility program to synchronize two clocks via a PPS.
+ * @note Copyright (C) 2012 Richard Cochran 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include "clockadj.h"
+#include "missing.h"
+#include "phc.h"
+#include "print.h"
+#include "sysoff.h"
+#include "util.h"
+#include "version.h"
+
+#define NSEC2SEC 10.0
+
+/* trap the alarm signal so that pause() will wake up on receipt */
+static void handle_alarm(int s)
+{
+   return;
+}
+
+static void double_to_timespec(double d, struct timespec *ts)
+{
+   double fraction, whole;
+
+   fraction = modf(d, &whole);
+
+   /* cast the whole value to a time_t to store as seconds */
+   ts->tv_sec = (time_t)whole;
+   /* tv_nsec is a long, so we multiply the nanoseconds per second double
+* value by our fractional component. This results in a correct
+* timespec from the double representing seconds.
+*/
+   ts->tv_nsec = (long)(NSEC2SEC * fraction);
+}
+
+static int install_handler(int signum, void(*handler)(int))
+{
+   struct sigaction action;
+   sigset_t mask;
+
+   /* Unblock the signal */
+   sigemptyset(&mask);
+   sigaddset(&mask, signum);
+   sigprocmask(SIG_UNBLOCK, &mask, NULL);
+
+   /* Install the signal handler */
+   action.sa_handler = handler;
+   action.sa_flags = 0;
+   sigemptyset(&action.sa_mask);
+   sigaction(signum, &action, NULL);
+
+   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 clockid_t clock_open(char *device)
+{
+   struct sk_ts_info ts_info;
+   char phc_device[16];
+   int clkid;
+
+   /* check if device is CLOCK_REALTIME */
+   if (!strcasecmp(device, "CLOC

[Linuxptp-devel] [PATCH] linuxptp: add phc_ctl program to help debug PHC devices

[Jacob Keller]

This is an updated version of a script I wrote a couple years ago for
debugging the PHC when writing a new driver. I figured that it might be
handy for the LinuxPTP project to include, as it can give some insight
into the PHC directly. I have updated it to make use of the shared code
here, in order to reduce duplication. Hopefully this is of some use to
everyone.

Signed-off-by: Jacob Keller 
---
 makefile  |   4 +-
 phc_ctl.8 | 106 
 phc_ctl.c | 561 ++
 3 files changed, 670 insertions(+), 1 deletion(-)
 create mode 100644 phc_ctl.8
 create mode 100644 phc_ctl.c

diff --git a/makefile b/makefile
index e36835b05d40..e9f2f8fcf416 100644
--- a/makefile
+++ b/makefile
@@ -22,7 +22,7 @@ CC= $(CROSS_COMPILE)gcc
 VER = -DVER=$(version)
 CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS)
 LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
-PRG= ptp4l pmc phc2sys hwstamp_ctl
+PRG= ptp4l pmc phc2sys hwstamp_ctl phc_ctl
 OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \
  filter.o fsm.o linreg.o mave.o mmedian.o msg.o ntpshm.o phc.o \
  pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \
@@ -54,6 +54,8 @@ phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o 
phc.o phc2sys.o pi.o \
 
 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
+
 version.o: .version version.sh $(filter-out version.d,$(DEPEND))
 
 .version: force
diff --git a/phc_ctl.8 b/phc_ctl.8
new file mode 100644
index ..dfc1c76a6aa2
--- /dev/null
+++ b/phc_ctl.8
@@ -0,0 +1,106 @@
+.TH PHC_CTL 8 "June 2014" "linuxptp"
+.SH NAME
+phc_ctl \- directly control PHC device clock
+
+.SH SYNOPSIS
+.B phc_ctl
+[ options ]  [ commands ]
+
+.SH DESCRIPTION
+.B phc_ctl
+is a program which can be used to directly control a PHC clock device.
+Typically, it is used for debugging purposes, and has little use for general
+control of the device. For general control of PHC clock devices,
+.B phc2sys (8)
+should be preferred.
+
+ may be either CLOCK_REALTIME, any /dev/ptpX device, or any ethernet
+device which supports ethtool's get_ts_info ioctl.
+
+.SH OPTIONS
+.TP
+.BI \-l " print-level"
+Set the maximum syslog level of messages which should be printed or sent to the
+system logger. The default is 6 (LOG_INFO).
+.TP
+.BI \-q
+Do not send messages to syslog. By default messages will be sent.
+.TP
+.BI \-Q
+Do not print messages to standard output. By default messages will be printed.
+.TP
+.BI \-h
+Display a help message.
+.TP
+.B \-v
+Prints the software version and exits.
+
+.SH COMMANDS
+
+.B phc_ctl
+is controlled by passing commands which take either an optional or required
+parameter. The commands (outlined below) will control aspects of the PHC clock
+device. These commands may be useful for inspecting or debugging the PHC
+driver, but may have adverse side effects on running instances of
+.B ptp4l (8)
+or
+.B phc2sys (8)
+
+.TP
+.BI set " seconds"
+Set the PHC clock time to the value specified in seconds. Defaults to reading
+CLOCK_REALTIME if no value is provided.
+.TP
+.BI get
+Get the current time of the PHC clock device.
+.TP
+.BI adj " seconds"
+Adjust the PHC clock by an amount of seconds provided. This argument is 
required.
+.TP
+.BI freq " ppb"
+Adjust the frequency of the PHC clock by the specified parts per billion. If no
+argument is provided, it will attempt to read the current frequency and report
+it.
+.TP
+.BI cmp
+Compare the PHC clock device to CLOCK_REALTIME, using the best method 
available.
+.TP caps
+Display the device capabiltiies. This is the default command if no commands are
+provided.
+.TP wait " seconds"
+Sleep the process for the specified period of time, waking up and resuming
+afterwards. This command may be useful for sanity checking whether the PHC
+clock is running as expected.
+
+The arguments specified in seconds are read as double precision floating point
+values, and will scale to nanoseconds. This means providing a value of 5.5
+means 5 and one half seconds. This allows specifying fairly precise values for 
time.
+
+.SH EXAMPLES
+
+Read the current clock time from the device
+.RS
+\f(CWphc_ctl /dev/ptp0 get\fP
+.RE
+
+Set the PHC clock time to CLOCK_REALTIME
+.RS
+\f(CWphc_ctl /dev/ptp0 set\fP
+.RE
+
+Set PHC clock time to 0 (seconds since Epoch)
+.RS
+\f(CWphc_ctl /dev/ptp0 set 0.0\fP
+.RE
+
+Quickly sanity check frequency slewing by setting slewing frequency by positive
+10%, resetting clock to 0.0 time, waiting for 10 seconds, and then reading
+time. The time read back should be (roughly) 11 seconds, since the clock was
+slewed 10% faster.
+.RS
+\f(CWphc_ctl /dev/ptp0 freq 1000 set 0.0 wait 10.0 get
+.RE
+
+.SH SEE ALSO
+.BR ptp4l (8)
+.BR phc2sys (8)
diff --git a/phc_ctl.c b/phc_ctl.c
new file mode 100644
index ..6df101451eff
--- /dev/null
+++ b/phc

[Linuxptp-devel] [PATCH v3] linuxptp: add phc_ctl program to help debug PHC devices

[Jacob Keller]

This is an updated version of a script I wrote a couple years ago for
debugging the PHC when writing a new driver. I figured that it might be
handy for the LinuxPTP project to include, as it can give some insight
into the PHC directly. I have updated it to make use of the shared code
here, in order to reduce duplication. Hopefully this is of some use to
everyone.

-v2
* Add manpage
* remove need for ';' command

-v3
* fix copyright header

Signed-off-by: Jacob Keller 
---
 makefile  |   4 +-
 phc_ctl.8 | 106 
 phc_ctl.c | 561 ++
 3 files changed, 670 insertions(+), 1 deletion(-)
 create mode 100644 phc_ctl.8
 create mode 100644 phc_ctl.c

diff --git a/makefile b/makefile
index e36835b05d40..e9f2f8fcf416 100644
--- a/makefile
+++ b/makefile
@@ -22,7 +22,7 @@ CC= $(CROSS_COMPILE)gcc
 VER = -DVER=$(version)
 CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS)
 LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
-PRG= ptp4l pmc phc2sys hwstamp_ctl
+PRG= ptp4l pmc phc2sys hwstamp_ctl phc_ctl
 OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \
  filter.o fsm.o linreg.o mave.o mmedian.o msg.o ntpshm.o phc.o \
  pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \
@@ -54,6 +54,8 @@ phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o 
phc.o phc2sys.o pi.o \
 
 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
+
 version.o: .version version.sh $(filter-out version.d,$(DEPEND))
 
 .version: force
diff --git a/phc_ctl.8 b/phc_ctl.8
new file mode 100644
index ..dfc1c76a6aa2
--- /dev/null
+++ b/phc_ctl.8
@@ -0,0 +1,106 @@
+.TH PHC_CTL 8 "June 2014" "linuxptp"
+.SH NAME
+phc_ctl \- directly control PHC device clock
+
+.SH SYNOPSIS
+.B phc_ctl
+[ options ]  [ commands ]
+
+.SH DESCRIPTION
+.B phc_ctl
+is a program which can be used to directly control a PHC clock device.
+Typically, it is used for debugging purposes, and has little use for general
+control of the device. For general control of PHC clock devices,
+.B phc2sys (8)
+should be preferred.
+
+ may be either CLOCK_REALTIME, any /dev/ptpX device, or any ethernet
+device which supports ethtool's get_ts_info ioctl.
+
+.SH OPTIONS
+.TP
+.BI \-l " print-level"
+Set the maximum syslog level of messages which should be printed or sent to the
+system logger. The default is 6 (LOG_INFO).
+.TP
+.BI \-q
+Do not send messages to syslog. By default messages will be sent.
+.TP
+.BI \-Q
+Do not print messages to standard output. By default messages will be printed.
+.TP
+.BI \-h
+Display a help message.
+.TP
+.B \-v
+Prints the software version and exits.
+
+.SH COMMANDS
+
+.B phc_ctl
+is controlled by passing commands which take either an optional or required
+parameter. The commands (outlined below) will control aspects of the PHC clock
+device. These commands may be useful for inspecting or debugging the PHC
+driver, but may have adverse side effects on running instances of
+.B ptp4l (8)
+or
+.B phc2sys (8)
+
+.TP
+.BI set " seconds"
+Set the PHC clock time to the value specified in seconds. Defaults to reading
+CLOCK_REALTIME if no value is provided.
+.TP
+.BI get
+Get the current time of the PHC clock device.
+.TP
+.BI adj " seconds"
+Adjust the PHC clock by an amount of seconds provided. This argument is 
required.
+.TP
+.BI freq " ppb"
+Adjust the frequency of the PHC clock by the specified parts per billion. If no
+argument is provided, it will attempt to read the current frequency and report
+it.
+.TP
+.BI cmp
+Compare the PHC clock device to CLOCK_REALTIME, using the best method 
available.
+.TP caps
+Display the device capabiltiies. This is the default command if no commands are
+provided.
+.TP wait " seconds"
+Sleep the process for the specified period of time, waking up and resuming
+afterwards. This command may be useful for sanity checking whether the PHC
+clock is running as expected.
+
+The arguments specified in seconds are read as double precision floating point
+values, and will scale to nanoseconds. This means providing a value of 5.5
+means 5 and one half seconds. This allows specifying fairly precise values for 
time.
+
+.SH EXAMPLES
+
+Read the current clock time from the device
+.RS
+\f(CWphc_ctl /dev/ptp0 get\fP
+.RE
+
+Set the PHC clock time to CLOCK_REALTIME
+.RS
+\f(CWphc_ctl /dev/ptp0 set\fP
+.RE
+
+Set PHC clock time to 0 (seconds since Epoch)
+.RS
+\f(CWphc_ctl /dev/ptp0 set 0.0\fP
+.RE
+
+Quickly sanity check frequency slewing by setting slewing frequency by positive
+10%, resetting clock to 0.0 time, waiting for 10 seconds, and then reading
+time. The time read back should be (roughly) 11 seconds, since the clock was
+slewed 10% faster.
+.RS
+\f(CWphc_ctl /dev/ptp0 freq 1000 set 0.0 wait 10.0 get
+.RE
+
+.SH SEE ALSO
+.BR ptp4l (8)
+.BR phc2sys (8)
diff --git a/phc_ctl.c 

[Linuxptp-devel] [PATCH v4] linuxptp: add phc_ctl program to help debug PHC devices

[Jacob Keller]

This is an updated version of a script I wrote a couple years ago for
debugging the PHC when writing a new driver. I figured that it might be
handy for the LinuxPTP project to include, as it can give some insight
into the PHC directly. I have updated it to make use of the shared code
here, in order to reduce duplication. Hopefully this is of some use to
everyone.

Signed-off-by: Jacob Keller 
---
- v4
* Fix manpage
* add phc_ctl to gitignore

 .gitignore |   1 +
 makefile   |   4 +-
 phc_ctl.8  | 108 
 phc_ctl.c  | 561 +
 4 files changed, 673 insertions(+), 1 deletion(-)
 create mode 100644 phc_ctl.8
 create mode 100644 phc_ctl.c

diff --git a/.gitignore b/.gitignore
index 098dcdfe1ea7..0ca22afe2b9a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -5,3 +5,4 @@
 /phc2sys
 /pmc
 /ptp4l
+/phc_ctl
diff --git a/makefile b/makefile
index e36835b05d40..e9f2f8fcf416 100644
--- a/makefile
+++ b/makefile
@@ -22,7 +22,7 @@ CC= $(CROSS_COMPILE)gcc
 VER = -DVER=$(version)
 CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS)
 LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
-PRG= ptp4l pmc phc2sys hwstamp_ctl
+PRG= ptp4l pmc phc2sys hwstamp_ctl phc_ctl
 OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \
  filter.o fsm.o linreg.o mave.o mmedian.o msg.o ntpshm.o phc.o \
  pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \
@@ -54,6 +54,8 @@ phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o 
phc.o phc2sys.o pi.o \
 
 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
+
 version.o: .version version.sh $(filter-out version.d,$(DEPEND))
 
 .version: force
diff --git a/phc_ctl.8 b/phc_ctl.8
new file mode 100644
index ..06982690410e
--- /dev/null
+++ b/phc_ctl.8
@@ -0,0 +1,108 @@
+.TH PHC_CTL 8 "June 2014" "linuxptp"
+.SH NAME
+phc_ctl \- directly control PHC device clock
+
+.SH SYNOPSIS
+.B phc_ctl
+[ options ]  [ commands ]
+
+.SH DESCRIPTION
+.B phc_ctl
+is a program which can be used to directly control a PHC clock device.
+Typically, it is used for debugging purposes, and has little use for general
+control of the device. For general control of PHC clock devices,
+.B phc2sys (8)
+should be preferred.
+
+ may be either CLOCK_REALTIME, any /dev/ptpX device, or any ethernet
+device which supports ethtool's get_ts_info ioctl.
+
+.SH OPTIONS
+.TP
+.BI \-l " print-level"
+Set the maximum syslog level of messages which should be printed or sent to the
+system logger. The default is 6 (LOG_INFO).
+.TP
+.BI \-q
+Do not send messages to syslog. By default messages will be sent.
+.TP
+.BI \-Q
+Do not print messages to standard output. By default messages will be printed.
+.TP
+.BI \-h
+Display a help message.
+.TP
+.B \-v
+Prints the software version and exits.
+
+.SH COMMANDS
+
+.B phc_ctl
+is controlled by passing commands which take either an optional or required
+parameter. The commands (outlined below) will control aspects of the PHC clock
+device. These commands may be useful for inspecting or debugging the PHC
+driver, but may have adverse side effects on running instances of
+.B ptp4l (8)
+or
+.B phc2sys (8)
+
+.TP
+.BI set " seconds"
+Set the PHC clock time to the value specified in seconds. Defaults to reading
+CLOCK_REALTIME if no value is provided.
+.TP
+.BI get
+Get the current time of the PHC clock device.
+.TP
+.BI adj " seconds"
+Adjust the PHC clock by an amount of seconds provided. This argument is 
required.
+.TP
+.BI freq " ppb"
+Adjust the frequency of the PHC clock by the specified parts per billion. If no
+argument is provided, it will attempt to read the current frequency and report
+it.
+.TP
+.BI cmp
+Compare the PHC clock device to CLOCK_REALTIME, using the best method 
available.
+.TP
+.BI caps
+Display the device capabiltiies. This is the default command if no commands are
+provided.
+.TP
+.BI wait " seconds"
+Sleep the process for the specified period of time, waking up and resuming
+afterwards. This command may be useful for sanity checking whether the PHC
+clock is running as expected.
+
+The arguments specified in seconds are read as double precision floating point
+values, and will scale to nanoseconds. This means providing a value of 5.5
+means 5 and one half seconds. This allows specifying fairly precise values for 
time.
+
+.SH EXAMPLES
+
+Read the current clock time from the device
+.RS
+\f(CWphc_ctl /dev/ptp0 get\fP
+.RE
+
+Set the PHC clock time to CLOCK_REALTIME
+.RS
+\f(CWphc_ctl /dev/ptp0 set\fP
+.RE
+
+Set PHC clock time to 0 (seconds since Epoch)
+.RS
+\f(CWphc_ctl /dev/ptp0 set 0.0\fP
+.RE
+
+Quickly sanity check frequency slewing by setting slewing frequency by positive
+10%, resetting clock to 0.0 time, waiting for 10 seconds, and then reading
+time. The time read back should be (roughly) 11 seconds, since the clock was
+slewed 1

[Linuxptp-devel] [PATCH] gitignore: add .version as this is generated during a make

[Jacob Keller]

Signed-off-by: Jacob Keller 
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index e0710ad5b294..098dcdfe1ea7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,6 @@
 /*.d
 /*.o
+/.version
 /hwstamp_ctl
 /phc2sys
 /pmc
-- 
2.0.1.475.g9b8d714


--
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH v5] linuxptp: add phc_ctl program to help debug PHC devices

[Jacob Keller]

This is an updated version of a script I wrote a couple years ago for
debugging the PHC when writing a new driver. I figured that it might be
handy for the LinuxPTP project to include, as it can give some insight
into the PHC directly. I have updated it to make use of the shared code
here, in order to reduce duplication. Hopefully this is of some use to
everyone.

Signed-off-by: Jacob Keller 
---
This version fixes several issues,

* make clean wasn't removing phc_ctl.o
* manpage used 1% instead of 10% clock adjustment
* default command was not properly added

 .gitignore |   1 +
 makefile   |   6 +-
 phc_ctl.8  | 108 
 phc_ctl.c  | 571 +
 4 files changed, 684 insertions(+), 2 deletions(-)
 create mode 100644 phc_ctl.8
 create mode 100644 phc_ctl.c

diff --git a/.gitignore b/.gitignore
index 098dcdfe1ea7..0ca22afe2b9a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -5,3 +5,4 @@
 /phc2sys
 /pmc
 /ptp4l
+/phc_ctl
diff --git a/makefile b/makefile
index e36835b05d40..74a7fe20e88c 100644
--- a/makefile
+++ b/makefile
@@ -22,13 +22,13 @@ CC  = $(CROSS_COMPILE)gcc
 VER = -DVER=$(version)
 CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS)
 LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
-PRG= ptp4l pmc phc2sys hwstamp_ctl
+PRG= ptp4l pmc phc2sys hwstamp_ctl phc_ctl
 OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \
  filter.o fsm.o linreg.o mave.o mmedian.o msg.o ntpshm.o phc.o \
  pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \
  transport.o udp.o udp6.o uds.o util.o version.o
 
-OBJECTS= $(OBJ) hwstamp_ctl.o phc2sys.o pmc.o pmc_common.o sysoff.o
+OBJECTS= $(OBJ) hwstamp_ctl.o phc2sys.o pmc.o pmc_common.o sysoff.o 
phc_ctl.o
 SRC= $(OBJECTS:.o=.c)
 DEPEND = $(OBJECTS:.o=.d)
 srcdir := $(dir $(lastword $(MAKEFILE_LIST)))
@@ -54,6 +54,8 @@ phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o 
phc.o phc2sys.o pi.o \
 
 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
+
 version.o: .version version.sh $(filter-out version.d,$(DEPEND))
 
 .version: force
diff --git a/phc_ctl.8 b/phc_ctl.8
new file mode 100644
index ..609dbab16519
--- /dev/null
+++ b/phc_ctl.8
@@ -0,0 +1,108 @@
+.TH PHC_CTL 8 "June 2014" "linuxptp"
+.SH NAME
+phc_ctl \- directly control PHC device clock
+
+.SH SYNOPSIS
+.B phc_ctl
+[ options ]  [ commands ]
+
+.SH DESCRIPTION
+.B phc_ctl
+is a program which can be used to directly control a PHC clock device.
+Typically, it is used for debugging purposes, and has little use for general
+control of the device. For general control of PHC clock devices,
+.B phc2sys (8)
+should be preferred.
+
+ may be either CLOCK_REALTIME, any /dev/ptpX device, or any ethernet
+device which supports ethtool's get_ts_info ioctl.
+
+.SH OPTIONS
+.TP
+.BI \-l " print-level"
+Set the maximum syslog level of messages which should be printed or sent to the
+system logger. The default is 6 (LOG_INFO).
+.TP
+.BI \-q
+Do not send messages to syslog. By default messages will be sent.
+.TP
+.BI \-Q
+Do not print messages to standard output. By default messages will be printed.
+.TP
+.BI \-h
+Display a help message.
+.TP
+.B \-v
+Prints the software version and exits.
+
+.SH COMMANDS
+
+.B phc_ctl
+is controlled by passing commands which take either an optional or required
+parameter. The commands (outlined below) will control aspects of the PHC clock
+device. These commands may be useful for inspecting or debugging the PHC
+driver, but may have adverse side effects on running instances of
+.B ptp4l (8)
+or
+.B phc2sys (8)
+
+.TP
+.BI set " seconds"
+Set the PHC clock time to the value specified in seconds. Defaults to reading
+CLOCK_REALTIME if no value is provided.
+.TP
+.BI get
+Get the current time of the PHC clock device.
+.TP
+.BI adj " seconds"
+Adjust the PHC clock by an amount of seconds provided. This argument is 
required.
+.TP
+.BI freq " ppb"
+Adjust the frequency of the PHC clock by the specified parts per billion. If no
+argument is provided, it will attempt to read the current frequency and report
+it.
+.TP
+.BI cmp
+Compare the PHC clock device to CLOCK_REALTIME, using the best method 
available.
+.TP
+.BI caps
+Display the device capabiltiies. This is the default command if no commands are
+provided.
+.TP
+.BI wait " seconds"
+Sleep the process for the specified period of time, waking up and resuming
+afterwards. This command may be useful for sanity checking whether the PHC
+clock is running as expected.
+
+The arguments specified in seconds are read as double precision floating point
+values, and will scale to nanoseconds. This means providing a value of 5.5
+means 5 and one half seconds. This allows specifying fairly precise values for 
time.
+
+.SH EXAMPLES
+
+Read the current clock time from the device
+.RS
+\f(CWphc_ctl /dev

[Linuxptp-devel] [PATCH 1/5] util: move some config code into util for sharing

[Jacob Keller]

Move some various utility functions used by the config parser into
util.c

The main reason for this is to help extend phc2sys to read a
configuration file as well. It is easier to only use the generic
functions, as phc2sys does not use the exact same configuration settings
or structure as ptp4l.

Signed-off-by: Jacob Keller 
---
 config.c | 65 
 util.c   | 60 +++
 util.h   | 42 +
 3 files changed, 102 insertions(+), 65 deletions(-)

diff --git a/config.c b/config.c
index ec8c53801af0..7e783f3f7f88 100644
--- a/config.c
+++ b/config.c
@@ -27,30 +27,6 @@
 #include "print.h"
 #include "util.h"
 
-enum config_section {
-   GLOBAL_SECTION,
-   PORT_SECTION,
-   UNKNOWN_SECTION,
-};
-
-static enum parser_result parse_section_line(char *s, enum config_section 
*section)
-{
-   if (!strcasecmp(s, "[global]")) {
-   *section = GLOBAL_SECTION;
-   } else if (s[0] == '[') {
-   char c;
-   *section = PORT_SECTION;
-   /* Replace square brackets with white space. */
-   while (0 != (c = *s)) {
-   if (c == '[' || c == ']')
-   *s = ' ';
-   s++;
-   }
-   } else
-   return NOT_PARSED;
-   return PARSED_OK;
-}
-
 static enum parser_result parse_pod_setting(const char *option,
const char *value,
struct port_defaults *pod)
@@ -592,47 +568,6 @@ static enum parser_result parse_global_setting(const char 
*option,
return PARSED_OK;
 }
 
-static enum parser_result parse_setting_line(char *line,
-const char **option,
-const char **value)
-{
-   *option = line;
-
-   while (!isspace(line[0])) {
-   if (line[0] == '\0')
-   return NOT_PARSED;
-   line++;
-   }
-
-   while (isspace(line[0])) {
-   line[0] = '\0';
-   line++;
-   }
-
-   *value = line;
-
-   return PARSED_OK;
-}
-
-static void check_deprecated_options(const char **option)
-{
-   const char *new_option = NULL;
-
-   if (!strcmp(*option, "pi_offset_const")) {
-   new_option = "step_threshold";
-   } else if (!strcmp(*option, "pi_f_offset_const")) {
-   new_option = "first_step_threshold";
-   } else if (!strcmp(*option, "pi_max_frequency")) {
-   new_option = "max_frequency";
-   }
-
-   if (new_option) {
-   fprintf(stderr, "option %s is deprecated, please use %s 
instead\n",
-   *option, new_option);
-   *option = new_option;
-   }
-}
-
 int config_read(char *name, struct config *cfg)
 {
enum config_section current_section = UNKNOWN_SECTION;
diff --git a/util.c b/util.c
index 06c3296cf189..861d1269bdd9 100644
--- a/util.c
+++ b/util.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "address.h"
 #include "print.h"
@@ -216,6 +217,65 @@ int leap_second_status(uint64_t ts, int leap_set, int 
*leap, int *utc_offset)
return leap_status;
 }
 
+enum parser_result parse_section_line(char *s, enum config_section *section)
+{
+   if (!strcasecmp(s, "[global]")) {
+   *section = GLOBAL_SECTION;
+   } else if (s[0] == '[') {
+   char c;
+   *section = PORT_SECTION;
+   /* Replace square brackets with white space. */
+   while (0 != (c = *s)) {
+   if (c == '[' || c == ']')
+   *s = ' ';
+   s++;
+   }
+   } else
+   return NOT_PARSED;
+   return PARSED_OK;
+}
+
+enum parser_result parse_setting_line(char *line,
+ const char **option,
+ const char **value)
+{
+   *option = line;
+
+   while (!isspace(line[0])) {
+   if (line[0] == '\0')
+   return NOT_PARSED;
+   line++;
+   }
+
+   while (isspace(line[0])) {
+   line[0] = '\0';
+   line++;
+   }
+
+   *value = line;
+
+   return PARSED_OK;
+}
+
+void check_deprecated_options(const char **option)
+{
+   const char *new_option = NULL;
+
+   if (!strcmp(*option, "pi_offset_const")) {
+   new_option = "step_threshold";
+   } else if (!strcmp(*opt

[Linuxptp-devel] [PATCH 0/5] Add configuration file support to phc2sys

[Jacob Keller]

This series adds phc2sys configuration file support. Right now all the
command line options are supported. Like ptp4l, any existing command
line arguments supersede configuration file. Also, while the generic
options which appear to be simlar for both ptp4l and phc2sys are read
from [global], all options can be specified in [phc2sys] overriding
settings from [global] (but not the command line).

I didn't think the effort necessary to re-write the configuration such
that it was more generic was worth it. Thus, there is some level of code
duplication as phc2sys does not need the same configuration values as
ptp4l.

I did update the manuals, and have done some minor (compile and a few
run time checks) testing. However currently this series is more of an
RFC.

The intention is to enable additional non-command line options for
phc2sys. We could even deprecate several current phc2sys options which
are no longer necessary (similar to ptp4l which has many config options
but few command line parameters).

In addition this opens the door to bring several settings from ptp4l
into phc2sys (like the PI servo settings).

Jacob Keller (5):
  util: move some config code into util for sharing
  config: add phc2sys section ignored by ptp4l
  ptpl4.8: update ptp4l man page to indicate the phc2sys section
  config: add global options check function
  phc2sys: support reading the configuration file

 config.c  | 147 +++-
 config.h  |   2 +
 makefile  |   2 +-
 phc2sys.8 | 174 +++-
 phc2sys.c | 454 +-
 ptp4l.8   |  21 ++-
 util.c|  62 +
 util.h|  43 ++
 8 files changed, 834 insertions(+), 71 deletions(-)

-- 
2.1.2.555.gfbecd99


--
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 4/5] config: add global options check function

[Jacob Keller]

This function is intended to be used by phc2sys in order to ignore (ie:
not warn) about options in the configuration file specific to ptp4l.
Since phc2sys reads from the global section as well as its own specific
section, it is important to ensure that phc2sys won't complain about
ptp4l options.

Add an array of known strings which all future global options must be
added to.

Signed-off-by: Jacob Keller 
---
 config.c | 90 
 config.h |  2 ++
 2 files changed, 92 insertions(+)

diff --git a/config.c b/config.c
index 74c9e5fbccda..ee5f251fa6c9 100644
--- a/config.c
+++ b/config.c
@@ -27,6 +27,96 @@
 #include "print.h"
 #include "util.h"
 
+static const char *all_global_options[] = {
+   "delayAsymmetry",
+   "logAnnounceInterval",
+   "logSyncInterval",
+   "logMinDelayReqInterval",
+   "logMinPdelayReqInterval",
+   "announceReceiptTimeout",
+   "syncReceiptTimeout",
+   "transportSpecific",
+   "path_trace_enabled",
+   "follow_up_info",
+   "neighborPropDelayThresh",
+   "min_neighbor_prop_delay",
+   "egressLatency",
+   "ingressLatency",
+   "fault_badpeernet_interval",
+   "fault_reset_interval",
+   "network_transport",
+   "delay_mechanism",
+   "delay_filter",
+   "delay_filter_length",
+   "boundary_clock_jbod",
+   "twoStepFlag",
+   "slaveOnly",
+   "gmCapable",
+   "priority1",
+   "priority2",
+   "domainNumber",
+   "clockClass",
+   "clockAccuracy",
+   "offsetScaledLogVariance",
+   "free_running",
+   "freq_est_interval",
+   "assume_two_step",
+   "tx_timestamp_timeout",
+   "check_fup_sync",
+   "pi_proportional_const",
+   "pi_integral_const",
+   "pi_proportional_scale",
+   "pi_proportional_exponent",
+   "pi_proportional_norm_max",
+   "pi_integral_scale",
+   "pi_integral_exponent",
+   "pi_integral_norm_max",
+   "step_threshold",
+   "first_step_threshold",
+   "max_frequency",
+   "sanity_freq_limit",
+   "ntpshm_segment",
+   "ptp_dst_mac",
+   "p2p_dst_mac",
+   "udp6_scope",
+   "uds_address",
+   "logging_level",
+   "verbose",
+   "use_syslog",
+   "time_stamping",
+   "delay_mechanism",
+   "network_transport",
+   "clock_servo",
+   "productDescription",
+   "revisionData",
+   "userDescription",
+   "manufacturerIdentity",
+   "summary_interval",
+   "kernel_leap",
+   "timeSource",
+   "delay_filter",
+   "delay_filter_length",
+   "boundary_clock_jbod",
+   /* NULL indicates end of options */
+   NULL,
+};
+
+/* used by phc2sys to ignore global options meant strictly for ptp4l */
+enum parser_result parse_known_global_options(const char *option)
+{
+   const char **item = &all_global_options[0];
+
+   while (*item != NULL) {
+   if (!strcmp(option, *item))
+   return PARSED_OK;
+
+   item++;
+   }
+
+   /* we didn't find option in the list */
+   return NOT_PARSED;
+}
+
 static enum parser_result parse_pod_setting(const char *option,
const char *value,
struct port_defaults *pod)
diff --git a/config.h b/config.h
index c870e42ef162..99aa75435218 100644
--- a/config.h
+++ b/config.h
@@ -95,9 +95,11 @@ struct config {
int verbose;
 };
 
+enum parser_result parse_known_global_options(const char *option);
 int config_read(char *name, struct config *cfg);
 struct interface *config_create_interface(char *name, struct config *cfg);
 void config_init_interface(struct interface *iface, struct config *cfg);
 void config_destroy(struct config *cfg);
 
+
 #endif
-- 
2.1.2.555.gfbecd99


--
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 2/5] config: add phc2sys section ignored by ptp4l

[Jacob Keller]

Add a new configuration section phc2sys which is ignored by ptp4l. It
may be worth adding a "port:" prefix to all per-port configuration in
the configuration file (and deprecate the old syntax) in order to allow
full expression of subsections and port names. Currently it will not be
possible for a network device name to match a subsection name. This is
not a large problem at present, however.

Signed-off-by: Jacob Keller 
---
 config.c | 2 ++
 util.c   | 2 ++
 util.h   | 1 +
 3 files changed, 5 insertions(+)

diff --git a/config.c b/config.c
index 7e783f3f7f88..74c9e5fbccda 100644
--- a/config.c
+++ b/config.c
@@ -664,6 +664,8 @@ int config_read(char *name, struct config *cfg)
case UNKNOWN_SECTION:
fprintf(stderr, "line %d is not in a section\n", 
line_num);
goto parse_error;
+   /* ignore phc2sys specific configuration */
+   case PHC2SYS_SECTION:
default:
continue;
}
diff --git a/util.c b/util.c
index 861d1269bdd9..1991892b36c2 100644
--- a/util.c
+++ b/util.c
@@ -221,6 +221,8 @@ enum parser_result parse_section_line(char *s, enum 
config_section *section)
 {
if (!strcasecmp(s, "[global]")) {
*section = GLOBAL_SECTION;
+   } else if (!strcasecmp(s, "[phc2sys]")) {
+   *section = PHC2SYS_SECTION;
} else if (s[0] == '[') {
char c;
*section = PORT_SECTION;
diff --git a/util.h b/util.h
index 3f2863899b3d..ac5eb89c655e 100644
--- a/util.h
+++ b/util.h
@@ -141,6 +141,7 @@ enum parser_result {
 enum config_section {
GLOBAL_SECTION,
PORT_SECTION,
+   PHC2SYS_SECTION,
UNKNOWN_SECTION,
 };
 
-- 
2.1.2.555.gfbecd99


--
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 5/5] phc2sys: support reading the configuration file

[Jacob Keller]

This commit enables phc2sys to read the configuration file. A new
phc2sys section is added for specific options. Those options which carry
over from ptp4l are read from both global and (with higher precedence)
phc2sys section. I did not go to the extreme effort of refactoring
configuration to be more generic, so there is some level of duplicated
code. I couldn't find an easy way to make the whole config process more
generic without much more re-writing.

Signed-off-by: Jacob Keller 
---
 makefile  |   2 +-
 phc2sys.8 | 174 +++-
 phc2sys.c | 454 +-
 3 files changed, 626 insertions(+), 4 deletions(-)

diff --git a/makefile b/makefile
index 5469301af259..2a4ad9b6de2e 100644
--- a/makefile
+++ b/makefile
@@ -51,7 +51,7 @@ pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o 
transport.o udp.o \
 
 phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o phc.o phc2sys.o pi.o \
  pmc_common.o print.o raw.o servo.o sk.o stats.o sysoff.o tlv.o \
- transport.o udp.o udp6.o uds.o util.o version.o
+ transport.o udp.o udp6.o uds.o util.o version.o config.o
 
 hwstamp_ctl: hwstamp_ctl.o version.o
 
diff --git a/phc2sys.8 b/phc2sys.8
index 22d02c24db7a..ae4faa8d45b8 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -3,6 +3,9 @@
 phc2sys \- synchronize two or more clocks
 
 .SH SYNOPSIS
+.B phc2sys \-f
+[ options ]
+.br
 .B phc2sys \-a
 [
 .B \-r
@@ -33,7 +36,7 @@ program.
 
 With the
 .B \-a
-option, the clocks to synchronize are fetched from the running
+option (automatic mode), the clocks to synchronize are fetched from the running
 .B ptp4l
 daemon and the direction of synchronization automatically follows changes of
 the PTP port states.
@@ -47,6 +50,9 @@ and driver implementation.
 
 .SH OPTIONS
 .TP
+.BI \-f
+Read configuration from the specified file. No configuration is read by 
default.
+.TP
 .BI \-a
 Read the clocks to synchronize from running
 .B ptp4l
@@ -218,6 +224,172 @@ Display a help message.
 .B \-v
 Prints the software version and exits.
 
+.SH CONFIGURATION FILE
+
+The configuration file is divided into sections, and may be shared with ptp4l.
+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 generic program options mostly used by ptp4l, as well as some options
+shared by phc2sys. Note that only the options relevant to phc2sys are outlined
+here. See ptp4l(8) for options relevant to the ptp4l program.
+
+.SH GENERAL PROGAM OPTIONS
+
+.TP
+.B domainNumber
+The domain attribute of the local clock.
+The default is 0.
+.TP
+.B clock_servo
+The servo which is used to synchronize the local clock. Valid values are pi for
+a PI controller, linreg for an adaptive controller using linear regression, and
+ntpshm for the NTP SHM reference clock to allow another process to synchronize
+the local clock (the SHM segment number is set to the domain number).
+The default is pi.
+.TP
+.B pi_proportional_const
+The proportional constant of the PI controller. When set to 0.0, the
+proportional constant will be set by the following formula from the current
+sync interval.
+The default is 0.0.
+
+kp = min(kp_scale * sync^kp_exponent, kp_norm_max / sync))
+.TP
+.B pi_integral_const
+The integral constant of the PI controller. When set to 0.0, the
+integral constant will be set by the following formula from the current
+sync interval.
+The default is 0.0.
+
+ki = min(ki_scale * sync^ki_exponent, ki_norm_max / sync)
+.TP
+.B step_threshold
+The maximum offset the servo will correct by changing the clock
+frequency instead of stepping the clock. When set to 0.0, the servo will
+never step the clock except on start. It's specified in seconds.
+The default is 0.0.
+This option used to be called
+.BR pi_offset_const .
+.TP
+.B first_step_threshold
+The maximum offset the servo will correct by changing the clock
+frequency instead of stepping the clock. This is only applied on the first
+update. It's specified in seconds. When set to 0.0, the servo won't step
+the clock on start.
+The default is 0.2 (20 microseconds).
+This option used to be called
+.BR pi_f_offset_const .
+.TP
+.B sanity_freq_limit
+The maximum allowed frequency offset between uncorrected clock and the system
+monotonic clock in parts per billion (ppb). This is used as a sanity check of
+the synchronized clock. When a larger offset is measured, a warning message
+will be printed and the servo will be reset. When set to 0, the sanity check is
+disabled. The default is 2 (20%).
+.TP
+.B ntpshm_segment
+The number of the SHM segment used by ntpshm servo.
+The default is 0.
+.TP
+.B uds_address
+Specifies the address of the UNIX domain socket for connecting to the local
+pt

[Linuxptp-devel] [PATCH 3/5] ptpl4.8: update ptp4l man page to indicate the phc2sys section

[Jacob Keller]

This section currently does nothing, but is present for future phc2sys
configuration extension. Update the ptp4l manual in order to indicate
that the configuration file could be shared.

Note that the user is not required (obviously) to use the same file for
both ptp4l and phc2sys, though the goal is to make it possible to share
the configuration file if desired.

Signed-off-by: Jacob Keller 
---
 ptp4l.8 | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/ptp4l.8 b/ptp4l.8
index 0d01f29f4bda..fd5b1274bece 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -94,22 +94,29 @@ Display a help message.
 
 .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 configuration file is divided into sections and may be shared with phc2sys.
+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 program options, clock options and default port options. Other
+sets the program options, clock options and default port options. Note that 
some options are shared with
+.B phc2sys.
+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] ).
 Ports specified in the configuration file don't need to be
 specified by the
 .B \-i
-option. An empty port section can be used to replace the command line option.
+option. An empty port section can be used to replace the command line option. 
The
+.BR [phc2sys]
+section is used for options specific to the
+.B phc2sys
+program and is outlined in phc2sys(8)
+
 
 .SH PORT OPTIONS
 
-- 
2.1.2.555.gfbecd99


--
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH] differ message printed when local clock is best master

[Jacob Keller]

Make the log output a bit more clear by changing how we inform the user
when the local clock is the best master clock. This allows easier
parsing of the log and identifying when the local clock is being
selected as the best clock.

Signed-off-by: Jacob Keller 
---
I've often been looking at log output from ptp4l and wondered which
clock id was which port. This patch helps ease the mental burden by
explicitly stating when we selected the local clock as the best choice.

 clock.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/clock.c b/clock.c
index 199c0cbf54f3..8767e0a6dbba 100644
--- a/clock.c
+++ b/clock.c
@@ -1709,8 +1709,13 @@ static void handle_state_decision_event(struct clock *c)
best_id = c->dds.clockIdentity;
}
 
-   pr_notice("selected best master clock %s",
- cid2str(&best_id));
+   if (cid_eq(&best_id, &c->dds.clockIdentity)) {
+   pr_notice("selected local clock %s as best master",
+ cid2str(&best_id));
+   } else {
+   pr_notice("selected best master clock %s",
+ cid2str(&best_id));
+   }
 
if (!cid_eq(&best_id, &c->best_id)) {
clock_freq_est_reset(c);
-- 
2.15.1.478.ga1e07cd25f8b


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH] phc2sys: fix manpage documentation for -F

[Jacob Keller]

Clearly indicate the value is the maximum offset for which clock skewing
will be used at the start. An offset of a lower value will be fixed by
frequency changes only, while a larger value will be fixed with an
initial clock jump. This matches similar wording found in the -S option
explanation.

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

diff --git a/phc2sys.8 b/phc2sys.8
index 3a8683e6933a..45cb0e320fc3 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -154,9 +154,9 @@ after the start. The default is 0.0.
 .TP
 .BI \-F " step"
 Specify the step threshold applied only on the first update. It is the maximum
-offset that is corrected by adjusting clock. It's specified in seconds. The
-value of 0.0 disables stepping on start.
-The default is 0.2 (20 microseconds).
+offset that is corrected by changing the clock frequency. It's specified in
+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
-- 
2.17.0.536.g43f547f64b90


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

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

[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 e55af593471a..5a01e9d69b0f 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;
 }
 
-- 
2.23.0.245.gf157bbb9169d



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

[Linuxptp-devel] [PATCH 0/2] additions to phc_ctl

[Jacob Keller]

This series provides a couple of updates to the phc_ctl utility. First, we
display additional capabillity information such as the cross timestamping
support as well as the number of pins.

The second patch adds a new  command, "pin_cfg", which will query the pin
configuration for all of the supported pins. Suggestions on how to format
this this output is appreciated. I would almost think it's better to combine the
printing into a single pr_* buffer.. but the only way I can think to do that
would be using a local buffer to build up the longer string.

Jacob Keller (2):
  phc_ctl: display all capability information
  phc_ctl: add pin_cfg command to display pin functions

 phc_ctl.c | 81 +--
 1 file changed, 79 insertions(+), 2 deletions(-)

-- 
2.23.0.245.gf157bbb9169d



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

[Linuxptp-devel] [PATCH 2/2] phc_ctl: add pin_cfg command to display pin functions

[Jacob Keller]

Add a new function to phc_ctl to display the devices pin configuration
data. First, obtain the device capabilities to determine the number of
pins. Then, for each pin, print the name, function, and channel
information.

Signed-off-by: Jacob Keller 
---
 phc_ctl.c | 73 +++
 1 file changed, 73 insertions(+)

diff --git a/phc_ctl.c b/phc_ctl.c
index 5a01e9d69b0f..a619d9203472 100644
--- a/phc_ctl.c
+++ b/phc_ctl.c
@@ -134,6 +134,7 @@ static void usage(const char *progname)
"  freq [ppb]  adjust PHC frequency (default returns 
current offset)\n"
"  cmp compare PHC offset to CLOCK_REALTIME\n"
"  capsdisplay device capabilities (default if no 
command given)\n"
+   "  pin_cfg display device pin configuration\n"
"  wait   pause between commands\n"
"\n",
progname);
@@ -333,6 +334,77 @@ static int do_caps(clockid_t clkid, int cmdc, char *cmdv[])
return 0;
 }
 
+static const char *pin_func_string(unsigned int func)
+{
+   switch (func) {
+   case PTP_PF_NONE:
+   return "no function";
+   case PTP_PF_EXTTS:
+   return "external timestamping";
+   case PTP_PF_PEROUT:
+   return "periodic output";
+   case PTP_PF_PHYSYNC:
+   return "phy sync";
+   default:
+   return "unknown function";
+   }
+}
+
+static int do_pin_cfg(clockid_t clkid, int cmdc, char *cmdv[])
+{
+   struct ptp_clock_caps caps;
+   struct ptp_pin_desc pin_desc;
+   unsigned int index;
+
+   if (clkid == CLOCK_REALTIME) {
+   pr_warning("CLOCK_REALTIME is not a PHC device.");
+   return 0;
+   }
+
+   /* Get device capabilities first */
+   if (ioctl(CLOCKID_TO_FD(clkid), PTP_CLOCK_GETCAPS, &caps)) {
+   pr_err("get capabilities failed: %s",
+   strerror(errno));
+   return -1;
+   }
+
+   if (caps.n_pins == 0) {
+   pr_notice("device has no configurable pins");
+   return (0);
+   }
+
+   pr_notice("device has %d configurable input/output pins",
+   caps.n_pins);
+
+   /* For each pin, read its configuration */
+   for (index = 0; index < caps.n_pins; index++) {
+   memset(&pin_desc, 0, sizeof(pin_desc));
+   pin_desc.index = index;
+
+   if (ioctl(CLOCKID_TO_FD(clkid), PTP_PIN_GETFUNC, &pin_desc)) {
+   pr_warning("get pin configuration for pin %d failed: 
%s",
+   index,
+   strerror(errno));
+   /* keep going */
+   continue;
+   }
+
+   if (pin_desc.func == PTP_PF_NONE) {
+   pr_notice("pin %d [%s] is not configured",
+   pin_desc.index,
+   pin_desc.name);
+   } else {
+   pr_notice("pin %d [%s] performing %s using channel %d",
+   pin_desc.index,
+   pin_desc.name,
+   pin_func_string(pin_desc.func),
+   pin_desc.chan);
+   }
+   }
+
+   return 0;
+}
+
 static int do_cmp(clockid_t clkid, int cmdc, char *cmdv[])
 {
struct timespec ts, rta, rtb;
@@ -418,6 +490,7 @@ static const struct cmd_t all_commands[] = {
{ "freq", &do_freq },
{ "cmp", &do_cmp },
{ "caps", &do_caps },
+   { "pin_cfg", &do_pin_cfg },
{ "wait", &do_wait },
{ 0, 0 }
 };
-- 
2.23.0.245.gf157bbb9169d



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

Re: [Linuxptp-devel] [PATCH 1/3] clock: reset master/local rr when best clock id changes

[Jacob Keller]

On 2/4/2020 2:34 PM, Erik Hons wrote:
> This fixes an issue with free running clocks. When the master clock id
> changes the measured master/local ratio should be reset. If it isn't,
> and the local clock becomes master, then the last measured ratio to
> the old master continues to be used in peer delay calculations. If a
> bad master/local ration calculation triggers this state change the
> port can get "stuck" in not "asCapable" mode.
> 

Seems fairly straight forward to me.

> Signed-off-by: Erik Hons 
> ---
>  clock.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/clock.c b/clock.c
> index 146576a..a48656c 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1774,6 +1774,7 @@ static void handle_state_decision_event(struct clock *c)
>   tsproc_set_delay(c->tsproc, c->initial_delay);
>   c->ingress_ts = tmv_zero();
>   c->path_delay = c->initial_delay;
> + c->master_local_rr = 1.0;
>   c->nrr = 1.0;
>   fresh_best = 1;
>   }
> 


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

Re: [Linuxptp-devel] [PATCH RFC 00/30] Convert open coded interface into proper module.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Using gcc8 and -O2, the compiler emits an annoying false positive
> warning.  Since I want to have -Werror, I went about fixing it.  Start
> pulling on a thread, and ...
> 

Excellent goal!

> The 'struct interface' is wide open to its users, and each user
> fiddles with the fields in a different way.  The cure is, of course,
> to hide the implementation details in a source module.  This task
> turned out to be harder than it looked at first.
> 

Yea this can be quite challenging...

> - Patch #1 refactors the makefile a bit (this will be useful in a series to 
> follow)
> - Patches 2-3 clean up interfaces accepting character strings as parameters.
> - Patch #5 introduces the "interface" module.
> - Patches 6-23 convert all the users to proper methods, one method at a time.
> - Patches 24-27 add create/destroy methods.
> - Patch #28 hides the implementation of the struct.
> - Patch #29 _finally_ fixes the compiler warning.
> - Patch #30 removes left over crud.
> 

Nice. A lot of patches, but they sound reasonable. Will review them this
afternoon.

Regards,
Jake

> Thanks for your review,
> 
> Richard Cochran (30):
>   Group related objects together within the makefile.
>   config: Constify the public interface.
>   rtnl: Constify the public interface.
>   utils: Constify the posix clock interface.
>   Move the network interface into its own header file.
>   interface: Introduce an access method for the name field.
>   Convert call sites to the proper method for getting interface names.
>   interface: Introduce an access method for the time stamping label.
>   Convert call sites to the proper method for getting interface labels.
>   interface: Introduce a method to get the time stamping information.
>   Convert call sites to the proper method for getting time stamp
> information.
>   interface: Introduce a method to initialize the time stamping label.
>   Convert call sites to the proper method for initializing the time
> stamping label.
>   interface: Introduce a method to set the name.
>   Convert call sites to the proper method for setting the name.
>   interface: Introduce a method to set the time stamping label.
>   Convert call sites to the proper method for setting the time stamping
> label.
>   interface: Introduce a method to get the PHC index.
>   Convert call sites to the proper method for getting the PHC index.
>   interface: Introduce a method to test the time stamping information
> validity.
>   Convert call sites to the proper method for testing time stamp info
> validity.
>   interface: Introduce a method to test supported time stamping modes.
>   Convert call sites to the proper method for testing time stamping
> modes.
>   interface: Introduce methods to create and destroy instances.
>   clock: Use the proper create/destroy API for network interfaces.
>   config: Use the proper create/destroy API for network interfaces.
>   pmc: Use the proper create/destroy API for network interfaces.
>   interface: Hide the implementation details.
>   interface: Silence warning from gcc version 8.
>   interface: Remove obsolete method.
> 
>  clock.c| 61 
>  config.c   | 26 --
>  config.h   | 19 ++
>  interface.c| 78 +
>  interface.h| 94 ++
>  makefile   | 32 +
>  nsm.c  | 18 ++
>  pmc_common.c   | 19 +-
>  port.c | 59 +--
>  port_private.h |  2 +-
>  raw.c  |  5 +--
>  rtnl.c |  6 ++--
>  rtnl.h |  8 +++--
>  udp.c  |  4 +--
>  udp6.c |  4 +--
>  uds.c  |  8 ++---
>  util.c |  2 +-
>  util.h |  2 +-
>  18 files changed, 316 insertions(+), 131 deletions(-)
>  create mode 100644 interface.c
>  create mode 100644 interface.h
> 


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

Re: [Linuxptp-devel] [PATCH RFC 01/30] Group related objects together within the makefile.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Any program that links to the servo interface must also link with the
> implementations of that interface.  Similarly, the filter and network
> transport interfaces each require their implementations.  This patch
> re-factors the makefile to reflect this fact in order to simplify
> adding new programs making use of these interfaces.
> 
> Signed-off-by: Richard Cochran 

Makes sense. I probably would have kept the variables together instead
of spreading them through the list in alphabetical order, but I see no
issue with that.

This makes sense and helps prevent possible bugs in the future.
Additionally, adding more groups can be done over time in case new
modules have similar coupling in the future.

Reviewed-by: Jacob Keller 

> ---
>  makefile | 32 +---
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/makefile b/makefile
> index 3397d3e..d58d13a 100644
> --- a/makefile
> +++ b/makefile
> @@ -23,12 +23,14 @@ VER = -DVER=$(version)
>  CFLAGS   = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS)
>  LDLIBS   = -lm -lrt $(EXTRA_LDFLAGS)
>  PRG  = ptp4l hwstamp_ctl nsm phc2sys phc_ctl pmc timemaster
> -OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
> -e2e_tc.o fault.o filter.o fsm.o hash.o linreg.o mave.o mmedian.o msg.o 
> ntpshm.o \
> -nullf.o phc.o pi.o port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o 
> \
> -raw.o rtnl.o servo.o sk.o stats.o tc.o telecom.o tlv.o transport.o tsproc.o \
> -udp.o udp6.o uds.o unicast_client.o unicast_fsm.o unicast_service.o util.o \
> -version.o
> +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
> +OBJ  = bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
> + e2e_tc.o fault.o $(FILTERS) fsm.o hash.o msg.o phc.o port.o 
> port_signaling.o \
> + pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) sk.o stats.o tc.o \
> + $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o unicast_fsm.o \
> + unicast_service.o util.o version.o
>  
>  OBJECTS  = $(OBJ) hwstamp_ctl.o nsm.o phc2sys.o phc_ctl.o pmc.o 
> pmc_common.o \
>   sysoff.o timemaster.o
> @@ -55,22 +57,22 @@ all: $(PRG)
>  
>  ptp4l: $(OBJ)
>  
> -nsm: config.o filter.o hash.o mave.o mmedian.o msg.o nsm.o phc.o print.o 
> raw.o \
> - rtnl.o sk.o transport.o tlv.o tsproc.o udp.o udp6.o uds.o util.o version.o
> +nsm: config.o $(FILTERS) hash.o msg.o nsm.o phc.o print.o \
> + rtnl.o sk.o $(TRANSP) tlv.o tsproc.o util.o version.o
>  
> -pmc: config.o hash.o msg.o phc.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o 
> \
> - transport.o udp.o udp6.o uds.o util.o version.o
> +pmc: config.o hash.o msg.o phc.o pmc.o pmc_common.o print.o sk.o tlv.o \
> + $(TRANSP) util.o version.o
>  
> -phc2sys: clockadj.o clockcheck.o config.o hash.o linreg.o msg.o ntpshm.o \
> - nullf.o phc.o phc2sys.o pi.o pmc_common.o print.o raw.o servo.o sk.o 
> stats.o \
> - sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o version.o
> +phc2sys: clockadj.o clockcheck.o config.o hash.o msg.o \
> + phc.o phc2sys.o pmc_common.o print.o $(SERVOS) sk.o stats.o \
> + sysoff.o tlv.o $(TRANSP) util.o version.o
>  
>  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 msg.o phc.o pmc_common.o print.o raw.o sk.o \
> - snmp4lptp.o tlv.o transport.o udp.o udp6.o uds.o util.o
> +snmp4lptp: config.o hash.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
> 


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

Re: [Linuxptp-devel] [PATCH RFC 02/30] config: Constify the public interface.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> The two methods, config_create_interface and config_read, never modify the
> strings passed in.  This patch adds the const keyword to ensure these
> functions stay that way.
> 


Makes sense. Using const in more places like this is great! It helps
make behavior explicit.

> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  config.c | 4 ++--
>  config.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/config.c b/config.c
> index 12eb1f9..65afa70 100644
> --- a/config.c
> +++ b/config.c
> @@ -710,7 +710,7 @@ static struct option *config_alloc_longopts(void)
>   return opts;
>  }
>  
> -int config_read(char *name, struct config *cfg)
> +int config_read(const char *name, struct config *cfg)
>  {
>   enum config_section current_section = UNKNOWN_SECTION;
>   enum parser_result parser_res;
> @@ -817,7 +817,7 @@ parse_error:
>   return -2;
>  }
>  
> -struct interface *config_create_interface(char *name, struct config *cfg)
> +struct interface *config_create_interface(const char *name, struct config 
> *cfg)
>  {
>   struct interface *iface;
>  
> diff --git a/config.h b/config.h
> index f237fb2..e27d3e2 100644
> --- a/config.h
> +++ b/config.h
> @@ -60,8 +60,8 @@ struct config {
>   STAILQ_HEAD(ucmtab_head, unicast_master_table) unicast_master_tables;
>  };
>  
> -int config_read(char *name, struct config *cfg);
> -struct interface *config_create_interface(char *name, struct config *cfg);
> +int config_read(const char *name, struct config *cfg);
> +struct interface *config_create_interface(const char *name, struct config 
> *cfg);
>  void config_destroy(struct config *cfg);
>  
>  /* New, hash table based methods: */
> 


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

Re: [Linuxptp-devel] [PATCH RFC 03/30] rtnl: Constify the public interface.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Three of the rtnl methods never modify the strings passed in.  This
> patch adds the const keyword to ensure these functions stay that way.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  rtnl.c | 6 +++---
>  rtnl.h | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/rtnl.c b/rtnl.c
> index 59ed0ec..d9c76d7 100644
> --- a/rtnl.c
> +++ b/rtnl.c
> @@ -87,7 +87,7 @@ static void rtnl_get_ts_device_callback(void *ctx, int 
> linkup, int ts_index)
>   *dst = ts_index;
>  }
>  
> -int rtnl_get_ts_device(char *device, char *ts_device)
> +int rtnl_get_ts_device(const char *device, char *ts_device)
>  {
>   int err, fd;
>   int ts_index = -1;
> @@ -112,7 +112,7 @@ no_info:
>   return err;
>  }
>  
> -int rtnl_link_query(int fd, char *device)
> +int rtnl_link_query(int fd, const char *device)
>  {
>   struct sockaddr_nl sa;
>   struct msghdr msg;
> @@ -227,7 +227,7 @@ static int rtnl_linkinfo_parse(int master_index, struct 
> rtattr *rta)
>   return index;
>  }
>  
> -int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx)
> +int rtnl_link_status(int fd, const char *device, rtnl_callback cb, void *ctx)
>  {
>   struct rtattr *tb[IFLA_MAX+1];
>   struct ifinfomsg *info = NULL;
> diff --git a/rtnl.h b/rtnl.h
> index f877cd2..c5ea979 100644
> --- a/rtnl.h
> +++ b/rtnl.h
> @@ -37,7 +37,7 @@ int rtnl_close(int fd);
>   *  at least IF_NAMESIZE bytes long.
>   * @return  Zero on success, or -1 on error.
>   */
> -int rtnl_get_ts_device(char *device, char *ts_device);
> +int rtnl_get_ts_device(const char *device, char *ts_device);
>  
>  /**
>   * Request the link status from the kernel.
> @@ -45,7 +45,7 @@ int rtnl_get_ts_device(char *device, char *ts_device);
>   * @param device Interface name. Request all iface's status if set NULL.
>   * @return   Zero on success, non-zero otherwise.
>   */
> -int rtnl_link_query(int fd, char *device);
> +int rtnl_link_query(int fd, const char *device);
>  
>  /**
>   * Read kernel messages looking for a link up/down events.
> @@ -55,7 +55,7 @@ int rtnl_link_query(int fd, char *device);
>   * @param ctxPrivate context passed to the callback.
>   * @return   Zero on success, non-zero otherwise.
>   */
> -int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx);
> +int rtnl_link_status(int fd, const char *device, rtnl_callback cb, void 
> *ctx);
>  
>  /**
>   * Open a RT netlink socket for monitoring link state.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 04/30] utils: Constify the posix clock interface.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> The function to open a posix clock never modifies the passed in
> string.  This patch adds the const keyword to ensure this function
> stays that way.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  util.c | 2 +-
>  util.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/util.c b/util.c
> index e64a93d..43d6224 100644
> --- a/util.c
> +++ b/util.c
> @@ -190,7 +190,7 @@ char *portaddr2str(struct PortAddress *addr)
>   return buf;
>  }
>  
> -clockid_t posix_clock_open(char *device, int *phc_index)
> +clockid_t posix_clock_open(const char *device, int *phc_index)
>  {
>   struct sk_ts_info ts_info;
>   char phc_device[19];
> diff --git a/util.h b/util.h
> index 60d28ac..11e0935 100644
> --- a/util.h
> +++ b/util.h
> @@ -116,7 +116,7 @@ char *portaddr2str(struct PortAddress *addr);
>   * @param phc_index  Returns the PHC index, if any.
>   * @return   A valid clock ID on success or CLOCK_INVALID otherwise.
>   */
> -clockid_t posix_clock_open(char *device, int *phc_index);
> +clockid_t posix_clock_open(const char *device, int *phc_index);
>  
>  /**
>   * Compare two port identities for equality.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 05/30] Move the network interface into its own header file.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Up until now, the users of the interface data structure simply access
> its fields without restriction.  This patch takes the first step
> towards abstracting this data structure by giving it a file of its
> very own.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  config.h| 15 +--
>  interface.h | 28 
>  2 files changed, 29 insertions(+), 14 deletions(-)
>  create mode 100644 interface.h
> 
> diff --git a/config.h b/config.h
> index e27d3e2..14d2f64 100644
> --- a/config.h
> +++ b/config.h
> @@ -26,25 +26,12 @@
>  #include "ds.h"
>  #include "dm.h"
>  #include "filter.h"
> +#include "interface.h"
>  #include "mtab.h"
>  #include "transport.h"
>  #include "servo.h"
>  #include "sk.h"
>  
> -#define MAX_IFNAME_SIZE 108 /* = UNIX_PATH_MAX */
> -
> -#if (IF_NAMESIZE > MAX_IFNAME_SIZE)
> -#error if_namesize larger than expected.
> -#endif
> -
> -/** Defines a network interface, with PTP options. */
> -struct interface {
> - STAILQ_ENTRY(interface) list;
> - char name[MAX_IFNAME_SIZE + 1];
> - char ts_label[MAX_IFNAME_SIZE + 1];
> - struct sk_ts_info ts_info;
> -};
> -

Yea this doesn't really belong in config.h at all.

>  struct config {
>   /* configured interfaces */
>   STAILQ_HEAD(interfaces_head, interface) interfaces;
> diff --git a/interface.h b/interface.h
> new file mode 100644
> index 000..61d53a2
> --- /dev/null
> +++ b/interface.h
> @@ -0,0 +1,28 @@
> +/**
> + * @file interface.h
> + * @brief Implements network interface data structures.
> + * @note Copyright (C) 2020 Richard Cochran 
> + * @note SPDX-License-Identifier: GPL-2.0+
> + */
> +#ifndef HAVE_INTERFACE_H
> +#define HAVE_INTERFACE_H
> +
> +#include 
> +#include "sk.h"
> +
> +#define MAX_IFNAME_SIZE 108 /* = UNIX_PATH_MAX */
> +
> +#if (IF_NAMESIZE > MAX_IFNAME_SIZE)
> +#error if_namesize larger than expected.
> +#endif
> +
> +/** Defines a network interface, with PTP options. */
> +struct interface {
> + STAILQ_ENTRY(interface) list;
> + char name[MAX_IFNAME_SIZE + 1];
> + char ts_label[MAX_IFNAME_SIZE + 1];
> + struct sk_ts_info ts_info;
> +};
> +
> +#endif
> +
> 


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

Re: [Linuxptp-devel] [PATCH RFC 06/30] interface: Introduce an access method for the name field.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Many of the users only require a read only reference to the interface name.
> This patch adds an appropriate method.
> 

Right.

> Signed-off-by: Richard Cochran  ---
>  interface.c | 12 
>  interface.h |  7 +++
>  makefile|  8 
>  3 files changed, 23 insertions(+), 4 deletions(-)
>  create mode 100644 interface.c
> 
> diff --git a/interface.c b/interface.c
> new file mode 100644
> index 000..1231db9
> --- /dev/null
> +++ b/interface.c
> @@ -0,0 +1,12 @@
> +/**
> + * @file interface.c
> + * @brief Implements network interface data structures.
> + * @note Copyright (C) 2020 Richard Cochran 
> + * @note SPDX-License-Identifier: GPL-2.0+
> + */
> +#include "interface.h"
> +
> +const char *interface_name(struct interface *iface)
> +{
> + return iface->name;
> +}
> diff --git a/interface.h b/interface.h
> index 61d53a2..94d5b8f 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,5 +24,12 @@ struct interface {
>   struct sk_ts_info ts_info;
>  };
>  
> +/**
> + * Obtains the name of a network interface.
> + * @param iface  The interface of interest.
> + * @return   The device name of the network interface.
> + */
> +const char *interface_name(struct interface *iface);
> +
>  #endif
>  
> diff --git a/makefile b/makefile
> index d58d13a..e1e0e99 100644
> --- a/makefile
> +++ b/makefile
> @@ -27,10 +27,10 @@ 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
>  OBJ  = bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
> - e2e_tc.o fault.o $(FILTERS) fsm.o hash.o msg.o phc.o port.o 
> port_signaling.o \
> - pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) sk.o stats.o tc.o \
> - $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o unicast_fsm.o \
> - unicast_service.o util.o version.o
> + e2e_tc.o fault.o $(FILTERS) fsm.o hash.o interface.o msg.o phc.o port.o \
> + port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) sk.o \
> + stats.o tc.o $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o \
> + unicast_fsm.o unicast_service.o util.o version.o
>  

So the interface.o isn't being added to something like $(CONFIG)?

>  OBJECTS  = $(OBJ) hwstamp_ctl.o nsm.o phc2sys.o phc_ctl.o pmc.o 
> pmc_common.o \
>   sysoff.o timemaster.o
> 


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

Re: [Linuxptp-devel] [PATCH RFC 07/30] Convert call sites to the proper method for getting interface names.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
> ---
>  clock.c| 21 +++--
>  config.c   | 12 +++-
>  makefile   | 10 +-
>  nsm.c  |  9 +
>  pmc_common.c   |  2 +-
>  port.c | 19 +++
>  port_private.h |  2 +-
>  udp.c  |  2 +-
>  udp6.c |  2 +-
>  uds.c  |  8 
>  10 files changed, 47 insertions(+), 40 deletions(-)



> diff --git a/config.c b/config.c
> index 65afa70..c30f6bc 100644
> --- a/config.c
> +++ b/config.c
> @@ -775,15 +775,15 @@ int config_read(const char *name, struct config *cfg)
>   if (parse_setting_line(line, &option, &value)) {
>   fprintf(stderr, "could not parse line %d in %s 
> section\n",
>   line_num, current_section == GLOBAL_SECTION ?
> - "global" : current_port->name);
> + "global" : interface_name(current_port));
>   goto parse_error;
>   }
>  
>   check_deprecated_options(&option);
>  
>   parser_res = parse_item(cfg, 0, current_section == 
> GLOBAL_SECTION ?
> - NULL : current_port->name, option, 
> value);
> -
> + NULL : interface_name(current_port),
> + option, value);
>   switch (parser_res) {
>   case PARSED_OK:
>   break;
> @@ -791,7 +791,7 @@ int config_read(const char *name, struct config *cfg)
>   fprintf(stderr, "unknown option %s at line %d in %s 
> section\n",
>   option, line_num,
>   current_section == GLOBAL_SECTION ? "global" :
> - current_port->name);
> + interface_name(current_port));
>   goto parse_error;
>   case BAD_VALUE:
>   fprintf(stderr, "%s is a bad value for option %s at 
> line %d\n",
> @@ -820,10 +820,12 @@ parse_error:
>  struct interface *config_create_interface(const char *name, struct config 
> *cfg)
>  {
>   struct interface *iface;
> + const char *ifname;
>  
>   /* only create each interface once (by name) */
>   STAILQ_FOREACH(iface, &cfg->interfaces, list) {
> - if (0 == strncmp(name, iface->name, MAX_IFNAME_SIZE))
> + ifname = interface_name(iface);
> + if (0 == strncmp(name, ifname, MAX_IFNAME_SIZE))
>   return iface;
>   }
>  

We use the new interface_name() in config.c meaning that all users of
config.o must link to interface.o now...

> diff --git a/makefile b/makefile
> index e1e0e99..e1dd3fa 100644
> --- a/makefile
> +++ b/makefile
> @@ -57,13 +57,13 @@ all: $(PRG)
>  
>  ptp4l: $(OBJ)
>  
> -nsm: config.o $(FILTERS) hash.o msg.o nsm.o phc.o print.o \
> +nsm: config.o $(FILTERS) hash.o interface.o msg.o nsm.o phc.o print.o \
>   rtnl.o sk.o $(TRANSP) tlv.o tsproc.o util.o version.o
>  
> -pmc: config.o hash.o msg.o phc.o pmc.o pmc_common.o print.o sk.o tlv.o \
> - $(TRANSP) util.o version.o
> +pmc: config.o hash.o interface.o msg.o phc.o pmc.o pmc_common.o print.o sk.o 
> \
> + tlv.o $(TRANSP) util.o version.o
>  
> -phc2sys: clockadj.o clockcheck.o config.o hash.o msg.o \
> +phc2sys: clockadj.o clockcheck.o config.o hash.o interface.o msg.o \
>   phc.o phc2sys.o pmc_common.o print.o $(SERVOS) sk.o stats.o \
>   sysoff.o tlv.o $(TRANSP) util.o version.o
>  
> @@ -71,7 +71,7 @@ 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 msg.o phc.o pmc_common.o print.o sk.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 $@
>  

Considering the interface logic used to be in config.h and all of the
modified programs load from config does it make sense to add $(CONFIG)
that selects both config.o and interface.o? I mean config.o now requires
interface.o

Hmm. On the one hand it sort of doesn't make that much sense because
interface stuff is distinct from configs?

Is there a way to generate the network of how interconnected the various
object files are?

Thanks,
Jake


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

Re: [Linuxptp-devel] [PATCH RFC 08/30] interface: Introduce an access method for the time stamping label.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Many of the users only require a read only reference to the time
> stamping label of the interface.  This patch adds an appropriate
> method.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 


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

Re: [Linuxptp-devel] [PATCH RFC 08/30] interface: Introduce an access method for the time stamping label.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Many of the users only require a read only reference to the time
> stamping label of the interface.  This patch adds an appropriate
> method.
> 
> Signed-off-by: Richard Cochran 

Makes sense.

> ---
>  interface.c | 5 +
>  interface.h | 9 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index 1231db9..7909a5e 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -6,6 +6,11 @@
>   */
>  #include "interface.h"
>  
> +const char *interface_label(struct interface *iface)
> +{
> + return iface->ts_label;
> +}
> +
>  const char *interface_name(struct interface *iface)
>  {
>   return iface->name;
> diff --git a/interface.h b/interface.h
> index 94d5b8f..89f3e94 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,6 +24,15 @@ struct interface {
>   struct sk_ts_info ts_info;
>  };
>  
> +/**
> + * Obtain the time stamping label of a network interface.  This can be
> + * different from the name of the interface when bonding is in effect.
> + *
> + * @param iface  The interface of interest.
> + * @return   The time stamping device name of the network interface.
> + */
> +const char *interface_label(struct interface *iface);
> +

Nice to see this documented, and explaining why it might be different.

Thanks,
Jake

>  /**
>   * Obtains the name of a network interface.
>   * @param iface  The interface of interest.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 09/30] Convert call sites to the proper method for getting interface labels.

[Jacob Keller]

On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Straight forward.

Besides wondering about the object groups in the makefile,

Reviewed-by: Jacob Keller 

> ---
>  clock.c |  2 +-
>  port.c  | 17 ++---
>  raw.c   |  5 +++--
>  udp.c   |  2 +-
>  udp6.c  |  2 +-
>  5 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 7d13b3b..66c6bc1 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -962,7 +962,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   STAILQ_FOREACH(iface, &config->interfaces, list) {
>   rtnl_get_ts_device(interface_name(iface), iface->ts_label);
>   ensure_ts_label(iface);
> - sk_get_ts_info(iface->ts_label, &iface->ts_info);
> + sk_get_ts_info(interface_label(iface), &iface->ts_info);
>   if (iface->ts_info.valid &&
>   ((iface->ts_info.so_timestamping & required_modes) != 
> required_modes)) {
>   pr_err("interface '%s' does not support requested 
> timestamping mode",
> diff --git a/port.c b/port.c
> index 6423568..52aef86 100644
> --- a/port.c
> +++ b/port.c
> @@ -792,6 +792,7 @@ static int port_management_fill_response(struct port 
> *target,
>   struct management_tlv *tlv;
>   struct port_ds_np *pdsnp;
>   struct tlv_extra *extra;
> + const char *ts_label;
>   struct portDS *pds;
>   uint16_t u16;
>   uint8_t *buf;
> @@ -941,7 +942,8 @@ static int port_management_fill_response(struct port 
> *target,
>   else
>   ppn->port_state = target->state;
>   ppn->timestamping = target->timestamping;
> - ptp_text_set(&ppn->interface, target->iface->ts_label);
> + ts_label = interface_label(target->iface);
> + ptp_text_set(&ppn->interface, ts_label);
>   datalen = sizeof(*ppn) + ppn->interface.length;
>   break;
>   case TLV_PORT_STATS_NP:
> @@ -2482,10 +2484,10 @@ static void bc_dispatch(struct port *p, enum 
> fsm_event event, int mdiff)
>  
>  void port_link_status(void *ctx, int linkup, int ts_index)
>  {
> - struct port *p = ctx;
> - int link_state;
>   char ts_label[MAX_IFNAME_SIZE + 1] = {0};
> - int required_modes;
> + int link_state, required_modes;
> + const char *old_ts_label;
> + struct port *p = ctx;
>  
>   link_state = linkup ? LINK_UP : LINK_DOWN;
>   if (p->link_status & link_state) {
> @@ -2496,7 +2498,8 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>   }
>  
>   /* ts_label changed */
> - if (if_indextoname(ts_index, ts_label) && strcmp(p->iface->ts_label, 
> ts_label)) {
> + old_ts_label = interface_label(p->iface);
> + if (if_indextoname(ts_index, ts_label) && strcmp(old_ts_label, 
> ts_label)) {
>   strncpy(p->iface->ts_label, ts_label, MAX_IFNAME_SIZE);
>   p->link_status |= TS_LABEL_CHANGED;
>   pr_notice("port %hu: ts label changed to %s", portnum(p), 
> ts_label);
> @@ -2505,7 +2508,7 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>   /* Both link down/up and change ts_label may change phc index. */
>   if (p->link_status & LINK_UP &&
>   (p->link_status & LINK_STATE_CHANGED || p->link_status & 
> TS_LABEL_CHANGED)) {
> - sk_get_ts_info(p->iface->ts_label, &p->iface->ts_info);
> + sk_get_ts_info(interface_label(p->iface), &p->iface->ts_info);
>  
>   /* Only switch phc with HW time stamping mode */
>   if (p->iface->ts_info.valid && p->iface->ts_info.phc_index >= 
> 0) {
> @@ -2513,7 +2516,7 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>   if ((p->iface->ts_info.so_timestamping & 
> required_modes) != required_modes) {
>   pr_err("interface '%s' does not support 
> requested "
>  "timestamping mode, set link status down 
> by force.",
> -p->iface->ts_label);
> +interface_label(p->iface));
>   p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
>   } else if (p->phc_index != p->iface->ts_info.phc_index) 
> {
>   

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

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> In order to prevent users from open coding this logic, this patch
> provides a method that populates the time stamping information from
> the interface label.
> 
> Signed-off-by: Richard Cochran 

Makes sense.

Reviewed-by: Jacob Keller 

> ---
>  interface.c | 5 +
>  interface.h | 7 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index 7909a5e..460ceb8 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -6,6 +6,11 @@
>   */
>  #include "interface.h"
>  
> +int interface_get_tsinfo(struct interface *iface)
> +{
> + return sk_get_ts_info(iface->ts_label, &iface->ts_info);
> +}
> +

Presumably callers don't need to directly access ts_info, or if they do
we can later provide an accessor function.

>  const char *interface_label(struct interface *iface)
>  {
>   return iface->ts_label;
> diff --git a/interface.h b/interface.h
> index 89f3e94..05cfb10 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,6 +24,13 @@ struct interface {
>   struct sk_ts_info ts_info;
>  };
>  
> +/**
> + * Populate the time stamping information of a given interface.
> + * @param iface  The interface of interest.
> + * @return   zero on success, negative on failure.
> + */
> +int interface_get_tsinfo(struct interface *iface);
> +
>  /**
>   * Obtain the time stamping label of a network interface.  This can be
>   * different from the name of the interface when bonding is in effect.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 11/30] Convert call sites to the proper method for getting time stamp information.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  clock.c | 2 +-
>  port.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 66c6bc1..f987965 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -962,7 +962,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   STAILQ_FOREACH(iface, &config->interfaces, list) {
>   rtnl_get_ts_device(interface_name(iface), iface->ts_label);
>   ensure_ts_label(iface);

Unrelated to this patch, but I imagine this function wants to be moved
to interface.o too?

> - sk_get_ts_info(interface_label(iface), &iface->ts_info);
> + interface_get_tsinfo(iface);
>   if (iface->ts_info.valid &&
>   ((iface->ts_info.so_timestamping & required_modes) != 
> required_modes)) {
>   pr_err("interface '%s' does not support requested 
> timestamping mode",
> diff --git a/port.c b/port.c
> index 52aef86..05eb1d6 100644
> --- a/port.c
> +++ b/port.c
> @@ -2508,7 +2508,7 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>   /* Both link down/up and change ts_label may change phc index. */
>   if (p->link_status & LINK_UP &&
>   (p->link_status & LINK_STATE_CHANGED || p->link_status & 
> TS_LABEL_CHANGED)) {
> - sk_get_ts_info(interface_label(p->iface), &p->iface->ts_info);
> + interface_get_tsinfo(p->iface);
>  
>   /* Only switch phc with HW time stamping mode */
>   if (p->iface->ts_info.valid && p->iface->ts_info.phc_index >= 
> 0) {
> 

Hmm. So we will also need an accessor for these pieces of data, but that
looks like it's handled in a future patch.

Thanks,
Jake


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

Re: [Linuxptp-devel] [PATCH RFC 12/30] interface: Introduce a method to initialize the time stamping label.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> In many cases, the time stamping label will be the same as the name of
> the interface.  In order to prevent users from open coding the logic that
> initializes the label from the interface name, this patch add an
> appropriate method.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  interface.c | 7 +++
>  interface.h | 6 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index 460ceb8..662552d 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -6,6 +6,13 @@
>   */
>  #include "interface.h"
>  
> +void interface_ensure_tslabel(struct interface *iface)
> +{
> + if (!iface->ts_label[0]) {
> + strncpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
> + }
> +}
> +
>  int interface_get_tsinfo(struct interface *iface)
>  {
>   return sk_get_ts_info(iface->ts_label, &iface->ts_info);
> diff --git a/interface.h b/interface.h
> index 05cfb10..371185d 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,6 +24,12 @@ struct interface {
>   struct sk_ts_info ts_info;
>  };
>  
> +/**
> + * Ensures that an interface has a proper time stamping label.
> + * @param iface  The interface of interest.
> + */
> +void interface_ensure_tslabel(struct interface *iface);
> +
>  /**
>   * Populate the time stamping information of a given interface.
>   * @param iface  The interface of interest.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 12/30] interface: Introduce a method to initialize the time stamping label.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> In many cases, the time stamping label will be the same as the name of
> the interface.  In order to prevent users from open coding the logic that
> initializes the label from the interface name, this patch add an
> appropriate method.
> 
> Signed-off-by: Richard Cochran 
> ---
>  interface.c | 7 +++
>  interface.h | 6 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index 460ceb8..662552d 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -6,6 +6,13 @@
>   */
>  #include "interface.h"
>  
> +void interface_ensure_tslabel(struct interface *iface)
> +{
> + if (!iface->ts_label[0]) {

The original code did "if (label[0] == '\0')" but this is equivalent. Ok.

> + strncpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
> + }
> +}
> +
>  int interface_get_tsinfo(struct interface *iface)
>  {
>   return sk_get_ts_info(iface->ts_label, &iface->ts_info);
> diff --git a/interface.h b/interface.h
> index 05cfb10..371185d 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,6 +24,12 @@ struct interface {
>   struct sk_ts_info ts_info;
>  };
>  
> +/**
> + * Ensures that an interface has a proper time stamping label.
> + * @param iface  The interface of interest.
> + */
> +void interface_ensure_tslabel(struct interface *iface);
> +
>  /**
>   * Populate the time stamping information of a given interface.
>   * @param iface  The interface of interest.
> 


___
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.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> The name field of the interface is set in different ways by different
> callers.  In order to prevent users from open coding the logic that sets
> the name, this patch adds an appropriate method.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  interface.c | 5 +
>  interface.h | 8 +++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/interface.c b/interface.c
> index 662552d..3811679 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -27,3 +27,8 @@ const char *interface_name(struct interface *iface)
>  {
>   return iface->name;
>  }
> +
> +void interface_set_name(struct interface *iface, const char *name)
> +{
> + strncpy(iface->name, name, MAX_IFNAME_SIZE);
> +}


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?

> diff --git a/interface.h b/interface.h
> index 371185d..5f449ae 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -53,5 +53,11 @@ const char *interface_label(struct interface *iface);
>   */
>  const char *interface_name(struct interface *iface);
>  
> -#endif
> +/**
> + * Set the name of a given interface.
> + * @param iface  The interface of interest.
> + * @param name   The desired name for the interface.
> + */
> +void interface_set_name(struct interface *iface, const char *name);
>  
> +#endif
> 


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

Re: [Linuxptp-devel] [PATCH RFC 13/30] Convert call sites to the proper method for initializing the time stamping label.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
> ---
>  clock.c  | 12 +---
>  nsm.c|  4 +---
>  pmc_common.c |  4 +---
>  3 files changed, 3 insertions(+), 17 deletions(-)
> 

Reviewed-by: Jacob Keller 

> diff --git a/clock.c b/clock.c
> index f987965..3895d09 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -842,16 +842,6 @@ int clock_required_modes(struct clock *c)
>   return required_modes;
>  }
>  
> -/*
> - * If we do not have a slave or the rtnl query failed, then use our
> - * own interface name as the time stamping interface name.
> - */
> -static void ensure_ts_label(struct interface *iface)
> -{
> - if (iface->ts_label[0] == '\0')
> - strncpy(iface->ts_label, interface_name(iface), 
> MAX_IFNAME_SIZE);
> -}
> -

Removing both open-coded and the implementation in another .c file. Nice!

Thanks,
Jake

>  struct clock *clock_create(enum clock_type type, struct config *config,
>  const char *phc_device)
>  {
> @@ -961,7 +951,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   required_modes = clock_required_modes(c);
>   STAILQ_FOREACH(iface, &config->interfaces, list) {
>   rtnl_get_ts_device(interface_name(iface), iface->ts_label);
> - ensure_ts_label(iface);
> + interface_ensure_tslabel(iface);
>   interface_get_tsinfo(iface);
>   if (iface->ts_info.valid &&
>   ((iface->ts_info.so_timestamping & required_modes) != 
> required_modes)) {
> diff --git a/nsm.c b/nsm.c
> index 269c3c8..e82fc37 100644
> --- a/nsm.c
> +++ b/nsm.c
> @@ -269,9 +269,7 @@ static int nsm_open(struct nsm *nsm, struct config *cfg)
>   STAILQ_FOREACH(iface, &cfg->interfaces, list) {
>   ifname = interface_name(iface);
>   rtnl_get_ts_device(ifname, iface->ts_label);
> - if (iface->ts_label[0] == '\0') {
> - strncpy(iface->ts_label, ifname, MAX_IFNAME_SIZE);
> - }
> + interface_ensure_tslabel(iface);
>   count++;
>   }
>   if (count != 1) {
> diff --git a/pmc_common.c b/pmc_common.c
> index d5c8b61..6bdaa94 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -351,9 +351,7 @@ struct pmc *pmc_create(struct config *cfg, enum 
> transport_type transport_type,
>   }
>  
>   strncpy(iface.name, iface_name, MAX_IFNAME_SIZE);
> - if (iface.ts_label[0] == '\0') {
> - strncpy(iface.ts_label, interface_name(&iface), 
> MAX_IFNAME_SIZE);
> - }
> + interface_ensure_tslabel(&iface);
>  
>   if (transport_open(pmc->transport, &iface,
>  &pmc->fdarray, TS_SOFTWARE)) {
> 


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

Re: [Linuxptp-devel] [PATCH RFC 15/30] Convert call sites to the proper method for setting the name.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  clock.c  | 5 +++--
>  config.c | 2 +-
>  pmc_common.c | 2 +-
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 3895d09..159fcb2 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -850,6 +850,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   int fadj = 0, max_adj = 0, sw_ts;
>   int phc_index, required_modes = 0;
>   struct clock *c = &the_clock;
> + const char *uds_ifname;
>   struct port *p;
>   unsigned char oui[OUI_LEN];
>   char phc[32], *tmp;
> @@ -999,8 +1000,8 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   }
>  
>   /* Configure the UDS. */
> - snprintf(udsif->name, sizeof(udsif->name), "%s",
> -  config_get_string(config, NULL, "uds_address"));
> + uds_ifname = config_get_string(config, NULL, "uds_address");
> + interface_set_name(udsif, uds_ifname);

This used to do an snprintf and now it does a strncpy. This is slightly
different because snprintf will null terminate while strncpy won't
guarantee this.. However we now copy by MAX_IFNAME_SIZE rather than
snprintf's "full" size. This means that the function will guarantee to
be null terminated if the original data starts as zero-allocated.

Perhaps the interface_set_name should guarantee the last byte is zero
just in case the interface structure is not per-initialized with zeros.

Thanks,
Jake

>   if (config_set_section_int(config, interface_name(udsif),
>  "announceReceiptTimeout", 0)) {
>   return NULL;
> diff --git a/config.c b/config.c
> index c30f6bc..717ee65 100644
> --- a/config.c
> +++ b/config.c
> @@ -835,7 +835,7 @@ struct interface *config_create_interface(const char 
> *name, struct config *cfg)
>   return NULL;
>   }
>  
> - strncpy(iface->name, name, MAX_IFNAME_SIZE);
> + interface_set_name(iface, name);
>   STAILQ_INSERT_TAIL(&cfg->interfaces, iface, list);
>   cfg->n_interfaces++;
>  
> diff --git a/pmc_common.c b/pmc_common.c
> index 6bdaa94..41181fb 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -350,7 +350,7 @@ struct pmc *pmc_create(struct config *cfg, enum 
> transport_type transport_type,
>   goto failed;
>   }
>  
> - strncpy(iface.name, iface_name, MAX_IFNAME_SIZE);
> + interface_set_name(&iface, iface_name);
>   interface_ensure_tslabel(&iface);
>  
>   if (transport_open(pmc->transport, &iface,
> 


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

Re: [Linuxptp-devel] [PATCH RFC 16/30] interface: Introduce a method to set the time stamping label.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> The ts_label field of the interface is set in different ways by different
> callers.  In order to prevent users from open coding the logic that sets
> the label, this patch adds an appropriate method.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  interface.c | 5 +
>  interface.h | 7 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index 3811679..d7eeb41 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -28,6 +28,11 @@ const char *interface_name(struct interface *iface)
>   return iface->name;
>  }
>  
> +void interface_set_label(struct interface *iface, const char *label)
> +{
> + strncpy(iface->ts_label, label, MAX_IFNAME_SIZE);
> +}

Same here, it might be a good idea to ensure that last byte
(MAX_IFNAME_SIZE + 1) is set to '\0'.

> +
>  void interface_set_name(struct interface *iface, const char *name)
>  {
>   strncpy(iface->name, name, MAX_IFNAME_SIZE);
> diff --git a/interface.h b/interface.h
> index 5f449ae..f416b24 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -53,6 +53,13 @@ const char *interface_label(struct interface *iface);
>   */
>  const char *interface_name(struct interface *iface);
>  
> +/**
> + * Set the time stamping label of a given interface.
> + * @param iface  The interface of interest.
> + * @param name   The desired label for the interface.
> + */
> +void interface_set_label(struct interface *iface, const char *label);
> +
>  /**
>   * Set the name of a given interface.
>   * @param iface  The interface of interest.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 17/30] Convert call sites to the proper method for setting the time stamping label.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran > ---
>  clock.c | 6 --
>  nsm.c   | 5 -
>  port.c  | 2 +-
>  rtnl.c  | 2 +-
>  rtnl.h  | 4 +++-
>  5 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 159fcb2..5001e66 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -846,6 +846,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>  const char *phc_device)
>  {
>   enum servo_type servo = config_get_int(config, NULL, "clock_servo");
> + char ts_label[IF_NAMESIZE], phc[32], *tmp;
>   enum timestamp_type timestamping;
>   int fadj = 0, max_adj = 0, sw_ts;
>   int phc_index, required_modes = 0;
> @@ -853,7 +854,6 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   const char *uds_ifname;
>   struct port *p;
>   unsigned char oui[OUI_LEN];
> - char phc[32], *tmp;
>   struct interface *iface, *udsif = &c->uds_interface;
>   struct timespec ts;
>   int sfl;
> @@ -951,7 +951,9 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   c->timestamping = timestamping;
>   required_modes = clock_required_modes(c);
>   STAILQ_FOREACH(iface, &config->interfaces, list) {
> - rtnl_get_ts_device(interface_name(iface), iface->ts_label);
> + memset(ts_label, 0, sizeof(ts_label));
> + rtnl_get_ts_device(interface_name(iface), ts_label);
> + interface_set_label(iface, ts_label);

Hmm.. odd that we now add a memset vs the previous  that didn't use one?
What was the reasoning..? In either case unless rtnl_get_ts_device
doesn't null terminate its output, we should be safe in using strncpy
since it would stop at the NULL terminator or the maximum size.

Thanks,
Jake

>   interface_ensure_tslabel(iface);
>   interface_get_tsinfo(iface);
>   if (iface->ts_info.valid &&
> diff --git a/nsm.c b/nsm.c
> index e82fc37..1292c6b 100644
> --- a/nsm.c
> +++ b/nsm.c
> @@ -262,13 +262,16 @@ static void nsm_help(FILE *fp)
>  static int nsm_open(struct nsm *nsm, struct config *cfg)
>  {
>   enum transport_type transport;
> + char ts_label[IF_NAMESIZE];
>   const char *ifname, *name;
>   struct interface *iface;
>   int count = 0;
>  
>   STAILQ_FOREACH(iface, &cfg->interfaces, list) {
>   ifname = interface_name(iface);
> - rtnl_get_ts_device(ifname, iface->ts_label);
> + memset(ts_label, 0, sizeof(ts_label));
> + rtnl_get_ts_device(ifname, ts_label);
> + interface_set_label(iface, ts_label);
>   interface_ensure_tslabel(iface);
>   count++;
>   }
> diff --git a/port.c b/port.c
> index 05eb1d6..c20c3fc 100644
> --- a/port.c
> +++ b/port.c
> @@ -2500,7 +2500,7 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>   /* ts_label changed */
>   old_ts_label = interface_label(p->iface);
>   if (if_indextoname(ts_index, ts_label) && strcmp(old_ts_label, 
> ts_label)) {
> - strncpy(p->iface->ts_label, ts_label, MAX_IFNAME_SIZE);
> + interface_set_label(p->iface, ts_label);
>   p->link_status |= TS_LABEL_CHANGED;
>   pr_notice("port %hu: ts label changed to %s", portnum(p), 
> ts_label);
>   }
> diff --git a/rtnl.c b/rtnl.c
> index d9c76d7..b7a2667 100644
> --- a/rtnl.c
> +++ b/rtnl.c
> @@ -87,7 +87,7 @@ static void rtnl_get_ts_device_callback(void *ctx, int 
> linkup, int ts_index)
>   *dst = ts_index;
>  }
>  
> -int rtnl_get_ts_device(const char *device, char *ts_device)
> +int rtnl_get_ts_device(const char *device, char ts_device[IF_NAMESIZE])
>  {
>   int err, fd;
>   int ts_index = -1;
> diff --git a/rtnl.h b/rtnl.h
> index c5ea979..8fef4a9 100644
> --- a/rtnl.h
> +++ b/rtnl.h
> @@ -20,6 +20,8 @@
>  #ifndef HAVE_RTNL_H
>  #define HAVE_RTNL_H
>  
> +#include 
> +
>  typedef void (*rtnl_callback)(void *ctx, int linkup, int ts_index);
>  
>  /**
> @@ -37,7 +39,7 @@ int rtnl_close(int fd);
>   *  at least IF_NAMESIZE bytes long.
>   * @return  Zero on success, or -1 on error.
>   */
> -int rtnl_get_ts_device(const char *device, char *ts_device);
> +int rtnl_get_ts_device(const char *device, char ts_device[IF_NAMESIZE]);
>  
>  /**
>   * Request the link status from the kernel.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 18/30] interface: Introduce a method to get the PHC index.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
Reviewed-by: Jacob Keller 

> ---
>  interface.c | 5 +
>  interface.h | 7 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index d7eeb41..02f63a0 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -28,6 +28,11 @@ const char *interface_name(struct interface *iface)
>   return iface->name;
>  }
>  
> +int interface_phc_index(struct interface *iface)
> +{
> + return iface->ts_info.phc_index;
> +}
> +
>  void interface_set_label(struct interface *iface, const char *label)
>  {
>   strncpy(iface->ts_label, label, MAX_IFNAME_SIZE);
> diff --git a/interface.h b/interface.h
> index f416b24..4f408d5 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -53,6 +53,13 @@ const char *interface_label(struct interface *iface);
>   */
>  const char *interface_name(struct interface *iface);
>  
> +/**
> + * Obtains the index of a PTP Hardware Clock device from a network interface.
> + * @param iface  The interface of interest.
> + * @return   The PHC index of the interface.
> + */
> +int interface_phc_index(struct interface *iface);
> +
>  /**
>   * Set the time stamping label of a given interface.
>   * @param iface  The interface of interest.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 19/30] Convert call sites to the proper method for getting the PHC index.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  clock.c |  2 +-
>  port.c  | 17 ++---
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 5001e66..71b3899 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -976,7 +976,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   phc_index = -1;
>   }
>   } else if (iface->ts_info.valid) {
> - phc_index = iface->ts_info.phc_index;
> + phc_index = interface_phc_index(iface);
>   } else {
>   pr_err("PTP device not specified and automatic determination"
>  " is not supported. Please specify PTP device.");
> diff --git a/port.c b/port.c
> index c20c3fc..f4834ba 100644
> --- a/port.c
> +++ b/port.c
> @@ -2511,15 +2511,16 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>   interface_get_tsinfo(p->iface);
>  
>   /* Only switch phc with HW time stamping mode */
> - if (p->iface->ts_info.valid && p->iface->ts_info.phc_index >= 
> 0) {
> + if (p->iface->ts_info.valid &&
> + interface_phc_index(p->iface) >= 0) {
>   required_modes = clock_required_modes(p->clock);
>   if ((p->iface->ts_info.so_timestamping & 
> required_modes) != required_modes) {
>   pr_err("interface '%s' does not support 
> requested "
>  "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 != p->iface->ts_info.phc_index) 
> {
> - p->phc_index = p->iface->ts_info.phc_index;
> + } else if (p->phc_index != 
> interface_phc_index(p->iface)) {
> + p->phc_index = interface_phc_index(p->iface);
>  
>   if (clock_switch_phc(p->clock, p->phc_index)) {
>   p->last_fault_type = FT_SWITCH_PHC;
> @@ -3002,19 +3003,21 @@ struct port *port_open(const char *phc_device,
>   ; /* UDS cannot have a PHC. */
>   } else if (!interface->ts_info.valid) {
>   pr_warning("port %d: get_ts_info not supported", number);
> - } else if (phc_index >= 0 && phc_index != interface->ts_info.phc_index) 
> {
> + } else if (phc_index >= 0 &&
> +phc_index != interface_phc_index(interface)) {
>   if (p->jbod) {
>   pr_warning("port %d: just a bunch of devices", number);
> - p->phc_index = interface->ts_info.phc_index;
> + p->phc_index = interface_phc_index(interface);
>   } else if (phc_device) {
>   pr_warning("port %d: taking %s from the command line, "
>  "not the attached ptp%d", number, phc_device,
> -interface->ts_info.phc_index);
> +interface_phc_index(interface));
>   p->phc_index = phc_index;
>   } else {
>   pr_err("port %d: PHC device mismatch", number);
>   pr_err("port %d: /dev/ptp%d requested, ptp%d attached",
> -number, phc_index, interface->ts_info.phc_index);
> +number, phc_index,
> +interface_phc_index(interface));
>   goto err_port;
>   }
>   }
> 


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

Re: [Linuxptp-devel] [PATCH RFC 18/30] interface: Introduce a method to get the PHC index.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 
> ---
>  interface.c | 5 +
>  interface.h | 7 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index d7eeb41..02f63a0 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -28,6 +28,11 @@ const char *interface_name(struct interface *iface)
>   return iface->name;
>  }
>  
> +int interface_phc_index(struct interface *iface)
> +{
> + return iface->ts_info.phc_index;
> +}

In theory these "getters" could use const struct interface *iface. I'm
not really sure that buys us much overall, though.

Thanks,
Jake

> +
>  void interface_set_label(struct interface *iface, const char *label)
>  {
>   strncpy(iface->ts_label, label, MAX_IFNAME_SIZE);
> diff --git a/interface.h b/interface.h
> index f416b24..4f408d5 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -53,6 +53,13 @@ const char *interface_label(struct interface *iface);
>   */
>  const char *interface_name(struct interface *iface);
>  
> +/**
> + * Obtains the index of a PTP Hardware Clock device from a network interface.
> + * @param iface  The interface of interest.
> + * @return   The PHC index of the interface.
> + */
> +int interface_phc_index(struct interface *iface);
> +
>  /**
>   * Set the time stamping label of a given interface.
>   * @param iface  The interface of interest.
> 


___
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.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  interface.c | 5 +
>  interface.h | 8 
>  2 files changed, 13 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index 02f63a0..7a3eddc 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -42,3 +42,8 @@ void interface_set_name(struct interface *iface, const char 
> *name)
>  {
>   strncpy(iface->name, name, MAX_IFNAME_SIZE);
>  }
> +
> +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?

I don't think this is harmful and you may consider it improving
readability though.

Thanks,
Jake

> diff --git a/interface.h b/interface.h
> index 4f408d5..3526a48 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -7,6 +7,7 @@
>  #ifndef HAVE_INTERFACE_H
>  #define HAVE_INTERFACE_H
>  
> +#include 
>  #include 
>  #include "sk.h"
>  
> @@ -74,4 +75,11 @@ void interface_set_label(struct interface *iface, const 
> char *label);
>   */
>  void interface_set_name(struct interface *iface, const char *name);
>  
> +/**
> + * Tests whether an interface's time stamping information is valid or not.
> + * @param iface  The interface of interest.
> + * @return   True if the time stamping information is valid, false 
> otherwise.
> + */
> +bool interface_tsinfo_valid(struct interface *iface);
> +
>  #endif
> 


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

Re: [Linuxptp-devel] [PATCH RFC 21/30] Convert call sites to the proper method for testing time stamp info validity.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Straight forward.

Reviewed-by: Jacob Keller 

> ---
>  clock.c | 4 ++--
>  port.c  | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 71b3899..845e27a 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -956,7 +956,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   interface_set_label(iface, ts_label);
>   interface_ensure_tslabel(iface);
>   interface_get_tsinfo(iface);
> - if (iface->ts_info.valid &&
> + if (interface_tsinfo_valid(iface) &&
>   ((iface->ts_info.so_timestamping & required_modes) != 
> required_modes)) {
>   pr_err("interface '%s' does not support requested 
> timestamping mode",
>  interface_name(iface));
> @@ -975,7 +975,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   if (1 != sscanf(phc_device, "/dev/ptp%d", &phc_index)) {
>   phc_index = -1;
>   }
> - } else if (iface->ts_info.valid) {
> + } else if (interface_tsinfo_valid(iface)) {
>   phc_index = interface_phc_index(iface);
>   } else {
>   pr_err("PTP device not specified and automatic determination"
> diff --git a/port.c b/port.c
> index f4834ba..b590024 100644
> --- a/port.c
> +++ b/port.c
> @@ -2511,7 +2511,7 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>   interface_get_tsinfo(p->iface);
>  
>   /* Only switch phc with HW time stamping mode */
> - if (p->iface->ts_info.valid &&
> + if (interface_tsinfo_valid(p->iface) &&
>   interface_phc_index(p->iface) >= 0) {
>   required_modes = clock_required_modes(p->clock);
>   if ((p->iface->ts_info.so_timestamping & 
> required_modes) != required_modes) {
> @@ -3001,7 +3001,7 @@ struct port *port_open(const char *phc_device,
>  
>   if (transport == TRANS_UDS) {
>   ; /* UDS cannot have a PHC. */
> - } else if (!interface->ts_info.valid) {
> + } else if (!interface_tsinfo_valid(interface)) {
>   pr_warning("port %d: get_ts_info not supported", number);
>   } else if (phc_index >= 0 &&
>  phc_index != interface_phc_index(interface)) {
> 


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

Re: [Linuxptp-devel] [PATCH RFC 22/30] interface: Introduce a method to test supported time stamping modes.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  interface.c | 8 
>  interface.h | 8 
>  2 files changed, 16 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index 7a3eddc..74a2512 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -47,3 +47,11 @@ bool interface_tsinfo_valid(struct interface *iface)
>  {
>   return iface->ts_info.valid ? true : false;
>  }
> +
> +bool interface_tsmodes_supported(struct interface *iface, int modes)
> +{
> + if ((iface->ts_info.so_timestamping & modes) == modes) {
> + return true;
> + }
> + return false;
> +}
> diff --git a/interface.h b/interface.h
> index 3526a48..32eec7b 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -82,4 +82,12 @@ void interface_set_name(struct interface *iface, const 
> char *name);
>   */
>  bool interface_tsinfo_valid(struct interface *iface);
>  
> +/**
> + * Tests whether an interface supports a set of given time stamping modes.
> + * @param iface  The interface of interest.
> + * @param modes  Bit mask of SOF_TIMESTAMPING_ flags.
> + * @return   True if the time stamping modes are supported, false 
> otherwise.
> + */
> +bool interface_tsmodes_supported(struct interface *iface, int modes);
> + 

Good, the documentation comment indicates modes is a bitmask. In
otherwords, interface_tsmodes_supported can return true if modes is a
subset of the supported interface modes.

Ok.

>  #endif
> 


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

Re: [Linuxptp-devel] [PATCH RFC 23/30] Convert call sites to the proper method for testing time stamping modes.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  clock.c | 2 +-
>  port.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 845e27a..71b5795 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -957,7 +957,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   interface_ensure_tslabel(iface);
>   interface_get_tsinfo(iface);
>   if (interface_tsinfo_valid(iface) &&
> - ((iface->ts_info.so_timestamping & required_modes) != 
> required_modes)) {

This actually makes the line shorter too! Nice.

> + !interface_tsmodes_supported(iface, required_modes)) {
>   pr_err("interface '%s' does not support requested 
> timestamping mode",
>  interface_name(iface));
>   return NULL;
> diff --git a/port.c b/port.c
> index b590024..6b87bc9 100644
> --- a/port.c
> +++ b/port.c
> @@ -2514,7 +2514,7 @@ void port_link_status(void *ctx, int linkup, int 
> ts_index)
>   if (interface_tsinfo_valid(p->iface) &&
>   interface_phc_index(p->iface) >= 0) {
>   required_modes = clock_required_modes(p->clock);
> - if ((p->iface->ts_info.so_timestamping & 
> required_modes) != required_modes) {
> + if (!interface_tsmodes_supported(p->iface, 
> required_modes)) {
>   pr_err("interface '%s' does not support 
> requested "
>  "timestamping mode, set link status down 
> by force.",
>  interface_label(p->iface));
> 


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

Re: [Linuxptp-devel] [PATCH RFC 24/30] interface: Introduce methods to create and destroy instances.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> In order to eventually hide the implementation details of the interface,
> users will need to be able to create and destroy instances thereof.  This
> patch adds the needed methods.
> 
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  interface.c | 19 +++
>  interface.h | 13 +
>  2 files changed, 32 insertions(+)
> 
> diff --git a/interface.c b/interface.c
> index 74a2512..63ed7e4 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -4,8 +4,27 @@
>   * @note Copyright (C) 2020 Richard Cochran 
>   * @note SPDX-License-Identifier: GPL-2.0+
>   */
> +#include 
>  #include "interface.h"
>  
> +struct interface *interface_create(const char *name)
> +{
> + struct interface *iface;
> +
> + iface = calloc(1, sizeof(struct interface));

Good, calloc guarantees that the interface structure is always zero'd.
That means we don't actually need to worry about interfaces where the
MAS_IFNAME_SIZE arrays end on non-zero. Ok.

I still think it would be good to have the functions guarantee the NULL
by manually assigning or using one of the string copy implementations
that will guarantee it. That way they don't have to rely on this assumption.

> + if (!iface) {
> + return NULL;
> + }
> + interface_set_name(iface, name);
> +
> + return iface;
> +}
> +
> +void interface_destroy(struct interface *iface)
> +{
> + free(iface);
> +}
> +
>  void interface_ensure_tslabel(struct interface *iface)
>  {
>   if (!iface->ts_label[0]) {
> diff --git a/interface.h b/interface.h
> index 32eec7b..b61f4d6 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -25,6 +25,19 @@ struct interface {
>   struct sk_ts_info ts_info;
>  };
>  
> +/**
> + * Creates an instance of an interface.
> + * @param name  The device which indentifies this interface.
> + * @return  A pointer to an interface instance on success, NULL 
> otherwise.
> + */
> +struct interface *interface_create(const char *name);
> +
> +/**
> + * Destroys an instance of an interface.
> + * @param iface  A pointer obtained via interface_create().
> + */
> +void interface_destroy(struct interface *iface);
> +
>  /**
>   * Ensures that an interface has a proper time stamping label.
>   * @param iface  The interface of interest.
> 


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

Re: [Linuxptp-devel] [PATCH RFC 26/30] config: Use the proper create/destroy API for network interfaces.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  config.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/config.c b/config.c
> index 717ee65..e033842 100644
> --- a/config.c
> +++ b/config.c
> @@ -829,13 +829,11 @@ struct interface *config_create_interface(const char 
> *name, struct config *cfg)
>   return iface;
>   }
>  
> - iface = calloc(1, sizeof(struct interface));
> + iface = interface_create(name);
>   if (!iface) {
>   fprintf(stderr, "cannot allocate memory for a port\n");
>   return NULL;
>   }
> -
> - interface_set_name(iface, name);
>   STAILQ_INSERT_TAIL(&cfg->interfaces, iface, list);
>   cfg->n_interfaces++;
>  
> @@ -906,7 +904,7 @@ void config_destroy(struct config *cfg)
>  
>   while ((iface = STAILQ_FIRST(&cfg->interfaces))) {
>   STAILQ_REMOVE_HEAD(&cfg->interfaces, list);
> - free(iface);
> + interface_destroy(iface);
>   }
>   while ((table = STAILQ_FIRST(&cfg->unicast_master_tables))) {
>   while ((address = STAILQ_FIRST(&table->addrs))) {
> 


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

Re: [Linuxptp-devel] [PATCH RFC 25/30] clock: Use the proper create/destroy API for network interfaces.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  clock.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index 71b5795..e5f104e 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -122,7 +122,7 @@ struct clock {
>   struct clock_stats stats;
>   int stats_interval;
>   struct clockcheck *sanity_check;
> - struct interface uds_interface;
> + struct interface *udsif;>   LIST_HEAD(clock_subscribers_head, 
> clock_subscriber) subscribers;
>  };
>  
> @@ -259,6 +259,7 @@ void clock_destroy(struct clock *c)
>  {
>   struct port *p, *tmp;
>  
> + interface_destroy(c->udsif);
>   clock_flush_subscriptions(c);
>   LIST_FOREACH_SAFE(p, &c->ports, list, tmp) {
>   clock_remove_port(c, p);
> @@ -854,7 +855,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   const char *uds_ifname;
>   struct port *p;
>   unsigned char oui[OUI_LEN];
> - struct interface *iface, *udsif = &c->uds_interface;
> + struct interface *iface;
>   struct timespec ts;
>   int sfl;
>  
> @@ -1003,20 +1004,20 @@ struct clock *clock_create(enum clock_type type, 
> struct config *config,
>  
>   /* Configure the UDS. */
>   uds_ifname = config_get_string(config, NULL, "uds_address");
> - interface_set_name(udsif, uds_ifname);
> - if (config_set_section_int(config, interface_name(udsif),
> + c->udsif = interface_create(uds_ifname);
> + if (config_set_section_int(config, interface_name(c->udsif),
>  "announceReceiptTimeout", 0)) {
>   return NULL;
>   }
> - if (config_set_section_int(config, interface_name(udsif),
> + if (config_set_section_int(config, interface_name(c->udsif),
>   "delay_mechanism", DM_AUTO)) {
>   return NULL;
>   }
> - if (config_set_section_int(config, interface_name(udsif),
> + if (config_set_section_int(config, interface_name(c->udsif),
>   "network_transport", TRANS_UDS)) {
>   return NULL;
>   }
> - if (config_set_section_int(config, interface_name(udsif),
> + if (config_set_section_int(config, interface_name(c->udsif),
>  "delay_filter_length", 1)) {
>   return NULL;
>   }
> @@ -1131,7 +1132,7 @@ struct clock *clock_create(enum clock_type type, struct 
> config *config,
>   }
>  
>   /* Create the UDS interface. */
> - c->uds_port = port_open(phc_device, phc_index, timestamping, 0, udsif, 
> c);
> + c->uds_port = port_open(phc_device, phc_index, timestamping, 0, 
> c->udsif, c);
>   if (!c->uds_port) {
>   pr_err("failed to open the UDS port");
>   return NULL;
> 


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

Re: [Linuxptp-devel] [PATCH RFC 27/30] pmc: Use the proper create/destroy API for network interfaces.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran 

Reviewed-by: Jacob Keller 

> ---
>  pmc_common.c | 19 ---
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/pmc_common.c b/pmc_common.c
> index 41181fb..3aab4b9 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -313,6 +313,7 @@ struct pmc {
>   struct PortIdentity target;
>  
>   struct transport *transport;
> + struct interface *iface;
>   struct fdarray fdarray;
>   int zero_length_gets;
>  };
> @@ -322,11 +323,8 @@ struct pmc *pmc_create(struct config *cfg, enum 
> transport_type transport_type,
>  UInteger8 domain_number, UInteger8 transport_specific,
>  int zero_datalen)
>  {
> - struct interface iface;
>   struct pmc *pmc;
>  
> - memset(&iface, 0, sizeof(iface));
> -
>   pmc = calloc(1, sizeof *pmc);
>   if (!pmc)
>   return NULL;
> @@ -350,18 +348,24 @@ struct pmc *pmc_create(struct config *cfg, enum 
> transport_type transport_type,
>   goto failed;
>   }
>  
> - interface_set_name(&iface, iface_name);
> - interface_ensure_tslabel(&iface);
> + pmc->iface = interface_create(iface_name);
> + if (!pmc->iface) {
> + pr_err("failed to create interface");
> + goto failed;
> + }
> + interface_ensure_tslabel(pmc->iface);
>  
> - if (transport_open(pmc->transport, &iface,
> + if (transport_open(pmc->transport, pmc->iface,
>  &pmc->fdarray, TS_SOFTWARE)) {
>   pr_err("failed to open transport");
> - goto failed;
> + goto no_trans_open;
>   }
>   pmc->zero_length_gets = zero_datalen ? 1 : 0;
>  
>   return pmc;
>  
> +no_trans_open:
> + interface_destroy(pmc->iface);
>  failed:
>   if (pmc->transport)
>   transport_destroy(pmc->transport);
> @@ -372,6 +376,7 @@ failed:
>  void pmc_destroy(struct pmc *pmc)
>  {
>   transport_close(pmc->transport, &pmc->fdarray);
> + interface_destroy(pmc->iface);
>   transport_destroy(pmc->transport);
>   free(pmc);
>  }
> 


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

Re: [Linuxptp-devel] [PATCH RFC 28/30] interface: Hide the implementation details.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Now that a complete functional API is in place, there is no need to expose
> the inner workings of the network interface data type.  This patch converts
> it into an opaque type while leaving the list marker visible to users
> through a simple form of "friendly exposition".
> 

I'd appreciate if there was some way to ensure we catch the interface
structure layout changing such that the definitions in clock.c and
config.c aren't compatible with interface.c anymore.

Perhaps there isn't a good solution for that besides code review?

> Signed-off-by: Richard Cochran 
> ---
>  clock.c | 4 
>  config.c| 4 
>  interface.c | 7 +++
>  interface.h | 9 ++---
>  nsm.c   | 4 
>  5 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/clock.c b/clock.c
> index e5f104e..1668383 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -47,6 +47,10 @@
>  #define N_CLOCK_PFD (N_POLLFD + 1) /* one extra per port, for the fault 
> timer */
>  #define POW2_41 ((double)(1ULL << 41))
>  
> +struct interface {
> + STAILQ_ENTRY(interface) list;
> +};
> +

I would appreciate some sort of comment here and/or in the .c file which
indicates that list entry must come first.

>  struct port {
>   LIST_ENTRY(port) list;
>  };
> diff --git a/config.c b/config.c
> index e033842..f20c5f7 100644
> --- a/config.c
> +++ b/config.c
> @@ -32,6 +32,10 @@
>  #include "print.h"
>  #include "util.h"
>  
> +struct interface {
> + STAILQ_ENTRY(interface) list;
> +};
> +
>  enum config_section {
>   GLOBAL_SECTION,
>   UC_MTAB_SECTION,
> diff --git a/interface.c b/interface.c
> index 63ed7e4..7cd5b41 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -7,6 +7,13 @@
>  #include 
>  #include "interface.h"
>  
> +struct interface {
> + STAILQ_ENTRY(interface) list;
> + char name[MAX_IFNAME_SIZE + 1];
> + char ts_label[MAX_IFNAME_SIZE + 1];
> + struct sk_ts_info ts_info;
> +};
> +
>  struct interface *interface_create(const char *name)
>  {
>   struct interface *iface;
> diff --git a/interface.h b/interface.h
> index b61f4d6..6cc50ac 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -17,13 +17,8 @@
>  #error if_namesize larger than expected.
>  #endif
>  
> -/** Defines a network interface, with PTP options. */
> -struct interface {
> - STAILQ_ENTRY(interface) list;
> - char name[MAX_IFNAME_SIZE + 1];
> - char ts_label[MAX_IFNAME_SIZE + 1];
> - struct sk_ts_info ts_info;
> -};
> +/** Opaque type */
> +struct interface;
>  

Is there a way that we can include the "friendly exposition" definition
within this header? I imagine because interface.c includes interface.h
the compiler complains...

>  /**
>   * Creates an instance of an interface.
> diff --git a/nsm.c b/nsm.c
> index 1292c6b..5aa925b 100644
> --- a/nsm.c
> +++ b/nsm.c
> @@ -35,6 +35,10 @@
>  #define IFMT "\n\t\t"
>  #define NSM_NFD  3
>  
> +struct interface {
> + STAILQ_ENTRY(interface) list;
> +};
> +
>  struct nsm {
>   struct config   *cfg;
>   struct fdarray  fda;
> 


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

Re: [Linuxptp-devel] [PATCH RFC 29/30] interface: Silence warning from gcc version 8.

[Jacob Keller]

On 2/11/2020 6:04 AM, Richard Cochran wrote:
> When compiling with gcc8 and -O2, the clever compiler complains:
> 
>interface.c: In function ‘interface_ensure_tslabel’:
>interface.c:38:3: error: ‘strncpy’ output may be truncated copying 108 
> bytes from a string of length 108 [-Werror=stringop-truncation]
> 
> Even though this is a false positive, this patch silences the warning
> by using memcpy instead of strncpy.
> 

You could also use snprintf("%s", ..., sizeof(iface->name);

memcpy also copies all of the data even if it doesn't strictly need to.

Thanks,
Jake

> Signed-off-by: Richard Cochran 
> ---
>  interface.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/interface.c b/interface.c
> index 7cd5b41..2cf3b1e 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -35,7 +35,7 @@ void interface_destroy(struct interface *iface)
>  void interface_ensure_tslabel(struct interface *iface)
>  {
>   if (!iface->ts_label[0]) {
> - strncpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
> + memcpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
>   }
>  }
>  
> 


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

Re: [Linuxptp-devel] [PATCH RFC 06/30] interface: Introduce an access method for the name field.

[Jacob Keller]

On 3/4/2020 8:56 AM, Richard Cochran wrote:
> On Tue, Feb 18, 2020 at 11:33:32AM -0800, Jacob Keller wrote:
>> So the interface.o isn't being added to something like $(CONFIG)?
> 
> I like the idea, but I'll leave that as a follow-on task.
> 
> Thanks,
> Richard
> 

Yea makes sense. I don't see a reason to hold up this series due to that.


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

Re: [Linuxptp-devel] [PATCH RFC 07/30] Convert call sites to the proper method for getting interface names.

[Jacob Keller]

On 3/4/2020 8:59 AM, Richard Cochran wrote:
> On Tue, Feb 18, 2020 at 11:38:43AM -0800, Jacob Keller wrote:
>>
>> Is there a way to generate the network of how interconnected the various
>> object files are?
> 
> The .d files have the includes.  I once wrote a script to turn the .d
> files into a dotty graph.  But I guess the utility is low.
> 
> Thanks,
> Richard
> 

Right.

Thanks,
Jake


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

Re: [Linuxptp-devel] More fun with i210 EXTTS: measure some PPS against a reference PPS

[Jacob Keller]

On 3/5/2020 1:11 AM, Frantisek Rysanek wrote:
> And another sideways question is: in the i210 hardware, there's a 
> register called SYSTIMR, supposedly holding the fraction of a 
> nanosecond (= sub-nanosecond resolution). And this register is 
> ignored by the igb driver in Linux - first and foremost because the 
> internal timestamping infrastructure only supports nanosecond 
> resolution. I know that a "ns fraction" field is present in the PTP 
> frames, but everybody except the White Rabbit just leave that field 
> empty (all zeroes). I'm wondering if this SYSTIMR register in the 
> i210 hardware has some practical use, or is always zero, or what.
> Well for my practical purposes, the SYSTIMR does not get reflected in 
> the two AUXSTMP registers - so I can probably just ignore SYSTIMR 
> too.

So, the SYSTIMR field is not "used" directly, but it holds and maintains
fractional nanoseconds. When you adjust the increment value slightly,
these get added to the SYSTIMR field of the system time. As that slowly
increments and eventually overflows, it will then increment the SYSTIML
register.

Essentially we always round down by cutting off SYSTIMR.


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

Re: [Linuxptp-devel] More fun with i210 EXTTS: measure some PPS against a reference PPS

[Jacob Keller]

On 3/5/2020 8:19 AM, Frantisek Rysanek wrote:
> during the initial settling of the PHC frequency / servo loop,
> I can see offsets in low units of ns, not aligned in any way.
> But: once the PHC settles to offset==0 && ppb==0,
> any measured edges captured via EXTTS channel 0 or 1
> will be quantised on 8ns boundaries.
> This is kind of spooky :-)
> Maybe the quantisation just doesn't jump at me so bad
> while the frequency (ppb) is still a little off...
> and it's true that if I keep capturing on both channels,
> their mutual difference is always quantised at 8 ns,
> even though the difference against the internal unsettled
> PHC is not aligned that way.
> 
> I'm giving up this train of research.
> Comments welcome :-)
> 8 ns granularity is better than nothing at all.

This makes sense. The clock source has a period of 8 ns of I recall. We
only update once every period. In order to have lower granularity, you'd
have to update at a higher rate.

We can change the exact amount that we update by modifying INCVAL, but
that doesn't change the underlying clock source. Thus, significantly
different INCVALs would not produce multiples of 8, but if you
eventually sync down to a ppb of 0 it would end up being there.

Thanks,
Jake


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

Re: [Linuxptp-devel] [PATCH RFC 28/30] interface: Hide the implementation details.

[Jacob Keller]

On 3/4/2020 9:24 AM, Richard Cochran wrote:
> On Tue, Feb 18, 2020 at 01:32:01PM -0800, Jacob Keller wrote:
>> I'd appreciate if there was some way to ensure we catch the interface
>> structure layout changing such that the definitions in clock.c and
>> config.c aren't compatible with interface.c anymore.
>>
>> Perhaps there isn't a good solution for that besides code review?
> 
> I am not too worried about the safety, because moving the list head
> will cause the whole thing to explode.  But there would be a better
> way.  The struct port has the same design, and so I'll address this
> issue with a follow up series.

Ok sounds good.

> 
> Thanks,
> Richard
> 


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

Re: [Linuxptp-devel] [PATCH RFC 24/30] interface: Introduce methods to create and destroy instances.

[Jacob Keller]

On 3/4/2020 9:19 AM, Richard Cochran wrote:
> On Tue, Feb 18, 2020 at 01:25:00PM -0800, Jacob Keller wrote:
>>
>> I still think it would be good to have the functions guarantee the NULL
>> by manually assigning or using one of the string copy implementations
>> that will guarantee it. That way they don't have to rely on this assumption.
> 
> In the final version, we have,
> 
>   char name[MAX_IFNAME_SIZE + 1]; 
>   
>  
>   char ts_label[MAX_IFNAME_SIZE + 1]; 
>   
>  
>   strncpy(iface->name, name, MAX_IFNAME_SIZE);
>   
>  
>   memcpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);  
>
>   strncpy(iface->ts_label, label, MAX_IFNAME_SIZE);   
>   
>  
> 
> so I think it is clear enough.  The use of MAX_IFNAME_SIZE is key.
> 
> Thanks,
> Richard
> 

Sure. It's not a big deal to me either way.


___
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.

[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

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

[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 5/6] Add PHC methods for querying and configuring the pin functionality.

[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 V2 1/6] clock: Safely remove event subscribers from list.

[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, &c->subscribers, list) {
> + LIST_FOREACH_SAFE(s, &c->subscribers, list, tmp) {
>   if (pid_eq(&s->targetPortIdentity,
>  &req->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, &now);
> 


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

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

[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.
> -.T

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

[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, &caps))
>   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, &caps))
>   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] PPS generation on BeagleBoneBlack

[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 V3 0/6] Clean up in preparation for GrandMaster support.

[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] [PATCH V1 RFC 0/5] GPS based Grand Master Support

[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 V1 1/2] Decouple servo state from automotive profile.

[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 01/11] transport: Correct grammar in the doxygen comments.

[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 03/11] udp6: Convey transmit path errors to the caller.

[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, &addr->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 02/11] udp: Convey transmit path errors to the caller.

[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, &addr->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 04/11] uds: Convey transmit path errors to the caller.

[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 = &uds->address;
>  
>   cnt = sendto(fd, buf, buflen, 0, &addr->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 &uds->t;
>  }
> -
> 


___
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.

[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 07/11] port: Convey targeted forwarding errors to the caller.

[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, &p->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.

[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.

[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, &msg, 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(&msg); cm != NULL; cm = CMSG_NXTHDR(&msg, 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(&hwts->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 10/11] port: Publish the method for creating signaling messages.

[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 09/11] port: Export the value of the wildcard port identity.

[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(&m->signaling.targetPortIdentity, &p->portIdentity) &&
> - !pid_eq(&m->signaling.targetPortIdentity, &wildcard)) {
> + !pid_eq(&m->signaling.targetPortIdentity, &wildcard_pid)) {
>   return 0;
>   }
>  
> 


___
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.

[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] [RFC PATCH 3/3] phc2pwm: Introduce an utility to sync pwm with PTP clock

[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(&ps, 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, &sen->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 04/11] uds: Convey transmit path errors to the caller.

[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 = &uds->address;
>>>  
>>> cnt = sendto(fd, buf, buflen, 0, &addr->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] [PATCH v2 1/3] ptp4l: call recvmsg() with the MSG_DONTWAIT flag

[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 v2 2/3] clock: dump unexpected packets received on the error queues of sockets

[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, &c->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 2/3] clock: dump unexpected packets received on the error queues of sockets

[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

[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] ts2phc_phc_master: specify start time to PPS master as 0.000000000

[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] ts2phc_phc_master: specify start time to PPS master as 0.000000000

[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] Machine Readable Output

[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] Machine Readable Output

[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

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

[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] [RFC PATCH 01/15] posix_clock_open: derive PHC index from device name if possible

[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

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

[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 03/15] phc2sys: break out pmc code into pmc_common.c

[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, &priv->ports, list) {
>   if (p->clock == clock) {
> - ret = run_pmc_port_properties(priv, 1000, p->number,
> + ret = run_pmc_port_properties(&priv->node, 1000, 
> p->number,
> &state, ×tamping,
> 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(&priv->node, 0) < 0)
>   continue;
>   upd

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

[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(&priv->node);
> + err = run_pmc_events(&priv->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, &msg);
> + res = run_pmc(node, 0, -1, &msg);
> + 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