[Linuxptp-devel] [RFC PATCH v2 1/2] phc2sys: update open_clock and deprecate '-i' option

[Jacob Keller]

This patch modifies phc2sys to enable the use of interface names in clock_open
rather than having to do that by hand. This enables cleaner use of the -s and -c
options as they can accept interface names. This also enables the user to set
the slave clock by network interface as well.

-v2-
* fix clock_open as it used device instead of phc_device in the final call to phc_open

Signed-off-by: Jacob Keller <***@intel.com>
---
phc2sys.8 | 37 ++++++++++++++++++-------------------
phc2sys.c | 48 ++++++++++++++++++++++++------------------------
2 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/phc2sys.8 b/phc2sys.8
index f5f72b4..1868919 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -4,18 +4,12 @@ phc2sys \- synchronize two clocks

.SH SYNOPSIS
.B phc2sys
-{
-.BI \-d " pps-device"
[
-.BI \-s " phc-device"
-|
-.BI \-i " interface"
-] |
-.BI \-s " phc-device"
-|
-.BI \-i " interface"
-} [
-.BI \-c " phc-device"
+.BI \-d " pps-device"
+] [
+.BI \-s " device"
+] [
+.BI \-c " device"
] [
.BI \-P " kp"
] [
@@ -71,21 +65,26 @@ is started or the
option should be used too. This option can be used only with the system clock as
the slave clock.
.TP
-.BI \-s " phc-device"
-Specify the master clock by device (e.g. /dev/ptp0) or name (e.g. CLOCK_REALTIME
-for the system clock). When this option is used together with the
+.BI \-s " device"
+Specify the master clock by device (e.g. dev/ptp0) or interface (e.g. eth0) or
+by name (e.g. CLOCK_REALTIME for the system clock). When this option is used
+together with the
.B \-d
option, the master clock is used only to correct the offset by whole number of
seconds, which cannot be fixed with PPS alone.
.TP
.BI \-i " interface"
-Similar to the
+Performs the exact same function as
+.B \-s
+for compatability reasons. Previously enabled specifying master clock by network
+interface. However, this can now be done using
.B \-s
-option, but specified by the interface which provides the master clock.
+and this option is no longer necessary. As such it has been deprecated, and
+should no longer be used.
.TP
-.BI \-c " phc-device"
-Specify the slave clock by device (e.g. /dev/ptp1) or name. The default is
-CLOCK_REALTIME (the system clock).
+.BI \-c " device"
+Specify the slave clock by device (e.g. /dev/ptp1) or interface (e.g. eth1) or
+by name. The default is CLOCK_REALTIME (the system clock).
.TP
.BI \-P " kp"
Specify the proportional constant of the PI controller. The default is 0.7.
diff --git a/phc2sys.c b/phc2sys.c
index dbe2d9a..ba6ff0d 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -61,17 +61,32 @@ static int update_sync_offset(struct clock *clock, int64_t offset, uint64_t ts);

static clockid_t clock_open(char *device)
{
+ struct sk_ts_info ts_info;
+ char phc_device[16];
int clkid;

- if (device[0] != '/') {
- if (!strcasecmp(device, "CLOCK_REALTIME"))
- return CLOCK_REALTIME;
+ /* check if device is CLOCK_REALTIME */
+ if (!strcasecmp(device, "CLOCK_REALTIME"))
+ return CLOCK_REALTIME;

- fprintf(stderr, "unknown clock %s\n", device);
+ /* check if device is valid phc device */
+ clkid = phc_open(device);
+ if (clkid != CLOCK_INVALID)
+ return clkid;
+
+ /* check if device is a valid ethernet device */
+ if (sk_get_ts_info(device, &ts_info) || !ts_info.valid) {
+ fprintf(stderr, "unknown clock %s: %m\n", device);
return CLOCK_INVALID;
}

- clkid = phc_open(device);
+ if (ts_info.phc_index < 0) {
+ fprintf(stderr, "interface %s does not have a PHC\n", device);
+ return CLOCK_INVALID;
+ }
+
+ sprintf(phc_device, "/dev/ptp%d", ts_info.phc_index);
+ clkid = phc_open(phc_device);
if (clkid == CLOCK_INVALID)
fprintf(stderr, "cannot open %s: %m\n", device);
return clkid;
@@ -535,7 +550,6 @@ static void usage(char *progname)
" -c [dev|name] slave clock (CLOCK_REALTIME)\n"
" -d [dev] master PPS device\n"
" -s [dev|name] master clock\n"
- " -i [iface] master clock by network interface\n"
" -P [kp] proportional constant (0.7)\n"
" -I [ki] integration constant (0.3)\n"
" -S [step] step threshold (disabled)\n"
@@ -557,7 +571,7 @@ static void usage(char *progname)

int main(int argc, char *argv[])
{
- char *progname, *ethdev = NULL;
+ char *progname;
clockid_t src = CLOCK_INVALID;
int c, domain_number = 0, phc_readings = 5, pps_fd = -1;
int max_ppb, r, wait_sync = 0, forced_sync_offset = 0;
@@ -589,6 +603,9 @@ int main(int argc, char *argv[])
return -1;
}
break;
+ case 'i':
+ fprintf(stderr,
+ "'-i' has been deprecated. please use '-s' instead.\n");
case 's':
src = clock_open(optarg);
break;
@@ -612,9 +629,6 @@ int main(int argc, char *argv[])
dst_clock.sync_offset_direction = -1;
forced_sync_offset = 1;
break;
- case 'i':
- ethdev = optarg;
- break;
case 'u':
dst_clock.stats_max_count = atoi(optarg);
break;
@@ -648,20 +662,6 @@ int main(int argc, char *argv[])
}
}

- if (src == CLOCK_INVALID && ethdev) {
- struct sk_ts_info ts_info;
- char phc_device[16];
- if (sk_get_ts_info(ethdev, &ts_info) || !ts_info.valid) {
- fprintf(stderr, "can't autodiscover PHC device\n");
- return -1;
- }
- if (ts_info.phc_index < 0) {
- fprintf(stderr, "interface %s doesn't have a PHC\n", ethdev);
- return -1;
- }
- sprintf(phc_device, "/dev/ptp%d", ts_info.phc_index);
- src = clock_open(phc_device);
- }
if (!(pps_fd >= 0 || src != CLOCK_INVALID) ||
dst_clock.clkid == CLOCK_INVALID ||
(pps_fd >= 0 && dst_clock.clkid != CLOCK_REALTIME)) {

[Jacob Keller]

This patch updates phc2sys usage reporting to give a slightly better indication
of why the program was unable to run.

Signed-off-by: Jacob Keller <***@intel.com>
---
phc2sys.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index ba6ff0d..4c1cee3 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -662,9 +662,23 @@ int main(int argc, char *argv[])
}
}

- if (!(pps_fd >= 0 || src != CLOCK_INVALID) ||
- dst_clock.clkid == CLOCK_INVALID ||
- (pps_fd >= 0 && dst_clock.clkid != CLOCK_REALTIME)) {
+ if (pps_fd < 0 && src == CLOCK_INVALID) {
+ fprintf(stderr,
+ "valid source clock must be selected.\n");
+ usage(progname);
+ return -1;
+ }
+
+ if (dst_clock.clkid == CLOCK_INVALID) {
+ fprintf(stderr,
+ "valid destination clock must be selected.\n");
+ usage(progname);
+ return -1;
+ }
+
+ if (pps_fd >= 0 && dst_clock.clkid != CLOCK_REALTIME) {
+ fprintf(stderr,
+ "cannot use a pps device unless destination is CLOCK_REALTIME\n");
usage(progname);
return -1;
}

[Miroslav Lichvar]

Post by Jacob Keller
This patch updates phc2sys usage reporting to give a slightly better indication
of why the program was unable to run.

The patch looks good to me.

Thanks,

[Richard Cochran]

Post by Jacob Keller
This patch updates phc2sys usage reporting to give a slightly better indication
of why the program was unable to run.
---
phc2sys.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

Applied.

Thanks,
Richard

[Richard Cochran]

Miroslav,

Since you have been the most active with phc2sys, can I ask you to
please comment/ack these two patches?

Thanks,
Richard

[Miroslav Lichvar]

Post by Jacob Keller
This patch modifies phc2sys to enable the use of interface names in clock_open
rather than having to do that by hand. This enables cleaner use of the -s and -c
options as they can accept interface names. This also enables the user to set
the slave clock by network interface as well.

The patch looks good and I really like the idea.

Post by Jacob Keller
.BI \-i " interface"
-Similar to the
+Performs the exact same function as
+.B \-s
+for compatability reasons. Previously enabled specifying master clock by network

Typo --------^, but maybe the option doesn't need to be described as
it is aliased to -s and a warning is printed.

Thanks,

[Richard Cochran]

Post by Jacob Keller
This patch modifies phc2sys to enable the use of interface names in clock_open
rather than having to do that by hand. This enables cleaner use of the -s and -c
options as they can accept interface names. This also enables the user to set
the slave clock by network interface as well.
-v2-
* fix clock_open as it used device instead of phc_device in the final call to phc_open
---
phc2sys.8 | 37 ++++++++++++++++++-------------------
phc2sys.c | 48 ++++++++++++++++++++++++------------------------
2 files changed, 42 insertions(+), 43 deletions(-)

Applied, with spelling correction.

Thanks,
Richard