[Linuxptp-devel] [PATCH RFC V2 00/10] Poor man's boundary clock

[Richard Cochran]

Here is my current series. The first patch is the improved Tx time
stamp polling, and patches 2-5 are trivial clean ups.

Patches 6-10 represent my second attempt at the "jbod" BC. With any
luck, this should work. Compile tested only!

(We still would need to change the max_adj for the servo, and maybe
there are a few other things I have forgotten.)

Comments? Complaints?

Thanks,
Richard

Richard Cochran (10):
Use SO_SELECT_ERR_QUEUE when available.
Coding style: add missing break statement from a switch/case
construct.
trivial: do not assign a FP constant to an integer.
trivial: update gitignore with the timemaster build product.
config: remove useless parameter.
config: add a option to enable a poor man's boundary clock.
phc2sys: make automatic mode actually work.
phc2sys: automatic mode: synchronize all non-slave ports.
Push the node's clock ID into the port data structure.
port: allow running a boundary clock with multiple clock devices.

.gitignore | 1 +
clock.c | 52 +++++++++++++++++++++++++---------------------------
clock.h | 2 ++
config.c | 16 ++++++++++++++--
config.h | 1 +
default.cfg | 1 +
ds.h | 1 +
gPTP.cfg | 1 +
missing.h | 4 ++++
phc2sys.c | 41 +++++++++++++++++++++++++++++++++--------
port.c | 34 +++++++++++++++++++++++++++++-----
port.h | 4 +++-
ptp4l.8 | 12 +++++++++++-
ptp4l.c | 1 +
sk.c | 16 ++++++++++++++--
15 files changed, 141 insertions(+), 46 deletions(-)

[Richard Cochran]

The current implementation fetches a transmit time stamp by polling on the
socket with pollfd.events set to zero, and then checking if POLLERR has
been returned by the kernel in pollfd.revents. This has the unfortunate
side effect of sleeping in poll() for the entire time out duration,
regardless of when the error queue becomes readable.

Linux kernel version 3.10 introduced a new socket option that allows
polling for transmit time stamps explicitly, waking the process as soon as
a time stamp becomes available. This patch enables the socket option,
falling back to the old behavior if necessary.

Suggested-by: Joe Schaack <***@xes-inc.com>
Signed-off-by: Richard Cochran <***@gmail.com>
---
missing.h | 4 ++++
sk.c | 16 ++++++++++++++--
2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/missing.h b/missing.h
index 43ac6cf..f7efd92 100644
--- a/missing.h
+++ b/missing.h
@@ -58,6 +58,10 @@ enum _missing_hwtstamp_tx_types {
#define SIOCGHWTSTAMP 0x89b1
#endif

+#ifndef SO_SELECT_ERR_QUEUE
+#define SO_SELECT_ERR_QUEUE 45
+#endif
+
#ifndef HAVE_CLOCK_ADJTIME
static inline int clock_adjtime(clockid_t id, struct timex *tx)
{
diff --git a/sk.c b/sk.c
index c48cf45..847855a 100644
--- a/sk.c
+++ b/sk.c
@@ -32,6 +32,7 @@

#include "address.h"
#include "ether.h"
+#include "missing.h"
#include "print.h"
#include "sk.h"

@@ -208,6 +209,9 @@ int sk_interface_addr(const char *name, int family, struct address *addr)
return result;
}

+static short sk_events = POLLPRI;
+static short sk_revents = POLLPRI;
+
int sk_receive(int fd, void *buf, int buflen,
struct address *addr, struct hw_timestamp *hwts, int flags)
{
@@ -230,7 +234,7 @@ int sk_receive(int fd, void *buf, int buflen,
msg.msg_controllen = sizeof(control);

if (flags == MSG_ERRQUEUE) {
- struct pollfd pfd = { fd, 0, 0 };
+ struct pollfd pfd = { fd, sk_events, 0 };
res = poll(&pfd, 1, sk_tx_timeout);
if (res < 1) {
pr_err(res ? "poll for tx timestamp failed: %m" :
@@ -238,7 +242,7 @@ int sk_receive(int fd, void *buf, int buflen,
pr_err("increasing tx_timestamp_timeout may correct "
"this issue, but it is likely caused by a driver bug");
return res;
- } else if (!(pfd.revents & POLLERR)) {
+ } else if (!(pfd.revents & sk_revents)) {
pr_err("poll for tx timestamp woke up on non ERR event");
return -1;
}
@@ -352,6 +356,14 @@ int sk_timestamping_init(int fd, const char *device, enum timestamp_type type,
return -1;
}

+ flags = 1;
+ if (setsockopt(fd, SOL_SOCKET, SO_SELECT_ERR_QUEUE,
+ &flags, sizeof(flags)) < 0) {
+ pr_warning("%s: SO_SELECT_ERR_QUEUE: %m", device);
+ sk_events = 0;
+ sk_revents = POLLERR;
+ }
+
/* Enable the sk_check_fupsync option, perhaps. */
if (sk_general_init(fd)) {
return -1;

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
port.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/port.c b/port.c
index caea891..f39ad1e 100644
--- a/port.c
+++ b/port.c
@@ -2183,6 +2183,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
break;
case -EPROTO:
pr_debug("port %hu: ignoring message", portnum(p));
+ break;
}
msg_put(msg);
return EV_NONE;

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index 726999e..df82d2b 100644
--- a/clock.c
+++ b/clock.c
@@ -789,7 +789,7 @@ struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,
enum timestamp_type timestamping, struct default_ds *dds,
enum servo_type servo)
{
- int fadj = 0, max_adj = 0.0, sw_ts = timestamping == TS_SOFTWARE ? 1 : 0;
+ int fadj = 0, max_adj = 0, sw_ts = timestamping == TS_SOFTWARE ? 1 : 0;
struct clock *c = &the_clock;
struct port *p;
char phc[32];

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
.gitignore | 1 +
1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 0ca22af..68a4c3e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -6,3 +6,4 @@
/pmc
/ptp4l
/phc_ctl
+/timemaster

[Richard Cochran]

The 'cfg' passed to parse_port_setting() is never used, so just remove it.

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 1c03fee..d5fa378 100644
--- a/config.c
+++ b/config.c
@@ -162,7 +162,6 @@ static enum parser_result parse_pod_setting(const char *option,

static enum parser_result parse_port_setting(const char *option,
const char *value,
- struct config *cfg,
struct interface *iface)
{
enum parser_result r;
@@ -677,7 +676,7 @@ int config_read(char *name, struct config *cfg)
if (current_section == GLOBAL_SECTION)
parser_res = parse_global_setting(option, value, cfg);
else
- parser_res = parse_port_setting(option, value, cfg, current_port);
+ parser_res = parse_port_setting(option, value, current_port);

switch (parser_res) {
case PARSED_OK:

[Richard Cochran]

This patch adds a configuration option that allows running a boundary clock
using "just a bunch of devices". Normally each port is probed to make sure
they all share the same PTP hardware clock, but this option will allow a
heterogeneous collection of devices, should the user really want it.

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 13 +++++++++++++
config.h | 1 +
default.cfg | 1 +
ds.h | 1 +
gPTP.cfg | 1 +
ptp4l.8 | 12 +++++++++++-
ptp4l.c | 1 +
7 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index d5fa378..154c191 100644
--- a/config.c
+++ b/config.c
@@ -205,6 +205,12 @@ static enum parser_result parse_port_setting(const char *option,
return r;
iface->delay_filter_length = val;

+ } else if (!strcmp(option, "boundary_clock_jbod")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ iface->boundary_clock_jbod = val;
+
} else
return NOT_PARSED;

@@ -562,6 +568,12 @@ static enum parser_result parse_global_setting(const char *option,
return r;
cfg->dds.delay_filter_length = val;

+ } else if (!strcmp(option, "boundary_clock_jbod")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ cfg->dds.boundary_clock_jbod = val;
+
} else
return NOT_PARSED;

@@ -750,6 +762,7 @@ void config_init_interface(struct interface *iface, struct config *cfg)

iface->delay_filter = cfg->dds.delay_filter;
iface->delay_filter_length = cfg->dds.delay_filter_length;
+ iface->boundary_clock_jbod = cfg->dds.boundary_clock_jbod;
}

void config_destroy(struct config *cfg)
diff --git a/config.h b/config.h
index d580496..c870e42 100644
--- a/config.h
+++ b/config.h
@@ -41,6 +41,7 @@ struct interface {
struct sk_ts_info ts_info;
enum filter_type delay_filter;
int delay_filter_length;
+ int boundary_clock_jbod;
};

#define CFG_IGNORE_DM (1 << 0)
diff --git a/default.cfg b/default.cfg
index 9e794ba..4e3ea65 100644
--- a/default.cfg
+++ b/default.cfg
@@ -70,6 +70,7 @@ delay_mechanism E2E
time_stamping hardware
delay_filter moving_median
delay_filter_length 10
+boundary_clock_jbod 0
#
# Clock description
#
diff --git a/ds.h b/ds.h
index ea25fbb..496b120 100644
--- a/ds.h
+++ b/ds.h
@@ -61,6 +61,7 @@ struct default_ds {
struct clock_description clock_desc;
enum filter_type delay_filter;
int delay_filter_length;
+ int boundary_clock_jbod;
};

struct dataset {
diff --git a/gPTP.cfg b/gPTP.cfg
index e15a05a..824b854 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -69,3 +69,4 @@ delay_mechanism P2P
time_stamping hardware
delay_filter moving_median
delay_filter_length 10
+boundary_clock_jbod 0
diff --git a/ptp4l.8 b/ptp4l.8
index 687beb6..6a9e331 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -1,4 +1,4 @@
-.TH PTP4l 8 "October 2013" "linuxptp"
+.TH PTP4l 8 "November 2014" "linuxptp"
.SH NAME
ptp4l \- PTP Boundary/Ordinary Clock

@@ -205,6 +205,16 @@ The default is moving_median.
.B delay_filter_length
The length of the delay filter in samples.
The default is 10.
+.TP
+.B boundary_clock_jbod
+When running as a boundary clock (that is, when more than one network
+interface is configured), ptp4l performs a sanity check to make sure
+that all of the ports share the same hardware clock device. This
+option allows ptp4l to work as a boundary clock using "just a bunch of
+devices" that are not synchronized to each other. For this mode, the
+collection of clocks must be synchronized by an external program, for
+example phc2sys(8) in "automatic" mode.
+The default is 0 (disabled).

.SH PROGRAM AND CLOCK OPTIONS

diff --git a/ptp4l.c b/ptp4l.c
index 83824f7..dadc4da 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -75,6 +75,7 @@ static struct config cfg_settings = {
},
.delay_filter = FILTER_MOVING_MEDIAN,
.delay_filter_length = 10,
+ .boundary_clock_jbod = 0,
},

.pod = {

[Jiri Benc]

Post by Richard Cochran
This patch adds a configuration option that allows running a boundary clock
using "just a bunch of devices". Normally each port is probed to make sure
they all share the same PTP hardware clock, but this option will allow a
heterogeneous collection of devices, should the user really want it.

Acked-by: Jiri Benc <***@redhat.com>

[Richard Cochran]

When running a "jbod" Boundary Clock, as long as we have one slaved port,
we always want the clocks on the other ports to be synchronized, regardless
of their port state.

Signed-off-by: Richard Cochran <***@gmail.com>
---
phc2sys.c | 40 +++++++++++++++++++++++++++++++++-------
1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 47ee3b8..cf47990 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -310,14 +310,23 @@ static void reconfigure(struct node *node)

c->state = c->new_state;

- if (c->state == PS_SLAVE) {
- src = c;
- src_cnt++;
- } else if (c->state == PS_UNCALIBRATED) {
- src_cnt++;
- } else if (c->state == PS_MASTER) {
+ switch (c->state) {
+ case PS_FAULTY:
+ case PS_DISABLED:
+ case PS_LISTENING:
+ case PS_PRE_MASTER:
+ case PS_MASTER:
+ case PS_PASSIVE:
pr_info("selecting %s for synchronization", c->device);
dst_cnt++;
+ break;
+ case PS_UNCALIBRATED:
+ src_cnt++;
+ break;
+ case PS_SLAVE:
+ src = c;
+ src_cnt++;
+ break;
}
}
if (src_cnt > 1) {
@@ -559,6 +568,23 @@ static int do_pps_loop(struct node *node, struct clock *clock, int fd)
return 0;
}

+static int update_needed(struct clock *c)
+{
+ switch (c->state) {
+ case PS_FAULTY:
+ case PS_DISABLED:
+ case PS_LISTENING:
+ case PS_PRE_MASTER:
+ case PS_MASTER:
+ case PS_PASSIVE:
+ return 1;
+ case PS_UNCALIBRATED:
+ case PS_SLAVE:
+ break;
+ }
+ return 0;
+}
+
static int do_loop(struct node *node, int subscriptions)
{
struct timespec interval;
@@ -590,7 +616,7 @@ static int do_loop(struct node *node, int subscriptions)
continue;

LIST_FOREACH(clock, &node->clocks, list) {
- if (clock->state != PS_MASTER)
+ if (!update_needed(clock))
continue;

if (clock->clkid == CLOCK_REALTIME &&

[Jiri Benc]

Post by Richard Cochran
When running a "jbod" Boundary Clock, as long as we have one slaved port,
we always want the clocks on the other ports to be synchronized, regardless
of their port state.

Acked-by: Jiri Benc <***@redhat.com>

[Richard Cochran]

The reconfigure function unnecessarily clears the 'new_state' variable.
On the second and subsequent calls, this zero value incorrectly clobbers
the state of any port whose state has *not* changed.

If, for example, a port in the MASTER state goes FAULTY, when it returns
to MASTER state, phc2sys will not synchronize it, because the SLAVE port's
state variable will have been cleared to zero, effectively erasing the
local time source.

This patch fixes the issue by removing the assignment.

Signed-off-by: Richard Cochran <***@gmail.com>
---
phc2sys.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/phc2sys.c b/phc2sys.c
index 22eb9c9..47ee3b8 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -309,7 +309,6 @@ static void reconfigure(struct node *node)
clock_reinit(c);

c->state = c->new_state;
- c->new_state = 0;

if (c->state == PS_SLAVE) {
src = c;

[Jiri Benc]

Post by Richard Cochran
The reconfigure function unnecessarily clears the 'new_state' variable.
On the second and subsequent calls, this zero value incorrectly clobbers
the state of any port whose state has *not* changed.
If, for example, a port in the MASTER state goes FAULTY, when it returns
to MASTER state, phc2sys will not synchronize it, because the SLAVE port's
state variable will have been cleared to zero, effectively erasing the
local time source.
This patch fixes the issue by removing the assignment.
---
phc2sys.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/phc2sys.c b/phc2sys.c
index 22eb9c9..47ee3b8 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -309,7 +309,6 @@ static void reconfigure(struct node *node)
clock_reinit(c);
c->state = c->new_state;
- c->new_state = 0;
if (c->state == PS_SLAVE) {
src = c;

With this, we're calling clock_reinit for the MASTER state even when it
did not change. The bug here is missing check for new_state == 0. I'll
send a patch.

Thanks,

Jiri

[Richard Cochran]

This patch lets the ports (rather than the clock) remember the clock ID,
passing it back to the clock code during synchronization. This does not
represent a functional change, but rather paves the way for ports to
override the clock ID when running a "jbod" BC.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 50 ++++++++++++++++++++++++--------------------------
clock.h | 2 ++
port.c | 8 ++++++--
port.h | 4 +++-
4 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/clock.c b/clock.c
index df82d2b..ca902ef 100644
--- a/clock.c
+++ b/clock.c
@@ -72,7 +72,6 @@ struct clock_subscriber {
};

struct clock {
- clockid_t clkid;
struct servo *servo;
struct defaultDS dds;
struct dataset default_dataset;
@@ -268,9 +267,6 @@ void clock_destroy(struct clock *c)
}
port_close(c->uds_port);
free(c->pollfd);
- if (c->clkid != CLOCK_REALTIME) {
- phc_close(c->clkid);
- }
servo_destroy(c->servo);
filter_destroy(c->delay_filter);
stats_destroy(c->stats.offset);
@@ -654,7 +650,7 @@ static void clock_update_slave(struct clock *c)
}
}

-static int clock_utc_correct(struct clock *c, tmv_t ingress)
+static int clock_utc_correct(struct clock *c, clockid_t clkid, tmv_t ingress)
{
struct timespec offset;
int utc_offset, leap, clock_leap;
@@ -680,7 +676,7 @@ static int clock_utc_correct(struct clock *c, tmv_t ingress)
}

/* Handle leap seconds. */
- if ((leap || c->leap_set) && c->clkid == CLOCK_REALTIME) {
+ if ((leap || c->leap_set) && clkid == CLOCK_REALTIME) {
/* If the clock will be stepped, the time stamp has to be the
target time. Ignore possible 1 second error in utc_offset. */
if (c->servo_state == SERVO_UNLOCKED) {
@@ -711,7 +707,7 @@ static int clock_utc_correct(struct clock *c, tmv_t ingress)

/* Update TAI-UTC offset of the system clock if valid and traceable. */
if (c->tds.flags & UTC_OFF_VALID && c->tds.flags & TIME_TRACEABLE &&
- c->utc_offset_set != utc_offset && c->clkid == CLOCK_REALTIME) {
+ c->utc_offset_set != utc_offset && clkid == CLOCK_REALTIME) {
sysclk_set_tai_offset(utc_offset);
c->utc_offset_set = utc_offset;
}
@@ -752,7 +748,7 @@ UInteger8 clock_class(struct clock *c)
return c->dds.clockQuality.clockClass;
}

-static int clock_add_port(struct clock *c, int phc_index,
+static int clock_add_port(struct clock *c, clockid_t clkid, int phc_index,
enum timestamp_type timestamping,
struct interface *iface)
{
@@ -760,7 +756,7 @@ static int clock_add_port(struct clock *c, int phc_index,

if (clock_resize_pollfd(c, c->nports + 1))
return -1;
- p = port_open(phc_index, timestamping, ++c->last_port_number,
+ p = port_open(clkid, phc_index, timestamping, ++c->last_port_number,
iface, c);
if (!p) {
/* No need to shrink pollfd */
@@ -796,6 +792,7 @@ struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,
struct interface *udsif = &c->uds_interface;
struct interface *iface;
struct timespec ts;
+ clockid_t clkid;

clock_gettime(CLOCK_REALTIME, &ts);
srandom(ts.tv_sec ^ ts.tv_nsec);
@@ -816,27 +813,27 @@ struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,
c->desc = dds->clock_desc;

if (c->free_running) {
- c->clkid = CLOCK_INVALID;
+ clkid = CLOCK_INVALID;
if (timestamping == TS_SOFTWARE || timestamping == TS_LEGACY_HW) {
c->utc_timescale = 1;
}
} else if (phc_index >= 0) {
snprintf(phc, 31, "/dev/ptp%d", phc_index);
- c->clkid = phc_open(phc);
- if (c->clkid == CLOCK_INVALID) {
+ clkid = phc_open(phc);
+ if (clkid == CLOCK_INVALID) {
pr_err("Failed to open %s: %m", phc);
return NULL;
}
- max_adj = phc_max_adj(c->clkid);
+ max_adj = phc_max_adj(clkid);
if (!max_adj) {
pr_err("clock is not adjustable");
return NULL;
}
- clockadj_init(c->clkid);
+ clockadj_init(clkid);
} else {
- c->clkid = CLOCK_REALTIME;
+ clkid = CLOCK_REALTIME;
c->utc_timescale = 1;
- clockadj_init(c->clkid);
+ clockadj_init(clkid);
max_adj = sysclk_max_freq();
sysclk_set_leap(0);
}
@@ -844,12 +841,12 @@ struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,
c->leap_set = 0;
c->time_flags = c->utc_timescale ? 0 : PTP_TIMESCALE;

- if (c->clkid != CLOCK_INVALID) {
- fadj = (int) clockadj_get_freq(c->clkid);
+ if (clkid != CLOCK_INVALID) {
+ fadj = (int) clockadj_get_freq(clkid);
/* Due to a bug in older kernels, the reading may silently fail
and return 0. Set the frequency back to make sure fadj is
the actual frequency of the clock. */
- clockadj_set_freq(c->clkid, fadj);
+ clockadj_set_freq(clkid, fadj);
}
c->servo = servo_create(servo, -fadj, max_adj, sw_ts);
if (!c->servo) {
@@ -902,7 +899,7 @@ struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,
pr_err("failed to allocate pollfd");
return NULL;
}
- c->uds_port = port_open(phc_index, timestamping, 0, udsif, c);
+ c->uds_port = port_open(clkid, phc_index, timestamping, 0, udsif, c);
if (!c->uds_port) {
pr_err("failed to open the UDS port");
return NULL;
@@ -911,7 +908,7 @@ struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,

/* Create the ports. */
STAILQ_FOREACH(iface, ifaces, list) {
- if (clock_add_port(c, phc_index, timestamping, iface)) {
+ if (clock_add_port(c, clkid, phc_index, timestamping, iface)) {
pr_err("failed to open port %s", iface->name);
return NULL;
}
@@ -1354,6 +1351,7 @@ UInteger16 clock_steps_removed(struct clock *c)
}

enum servo_state clock_synchronize(struct clock *c,
+ clockid_t clkid,
struct timespec ingress_ts,
struct timestamp origin_ts,
Integer64 correction1,
@@ -1381,7 +1379,7 @@ enum servo_state clock_synchronize(struct clock *c,
if (!c->path_delay)
return state;

- if (clock_utc_correct(c, ingress))
+ if (clock_utc_correct(c, clkid, ingress))
return c->servo_state;

c->cur.offsetFromMaster = tmv_to_TimeInterval(c->master_offset);
@@ -1407,8 +1405,8 @@ enum servo_state clock_synchronize(struct clock *c,
case SERVO_UNLOCKED:
break;
case SERVO_JUMP:
- clockadj_set_freq(c->clkid, -adj);
- clockadj_step(c->clkid, -tmv_to_nanoseconds(c->master_offset));
+ clockadj_set_freq(clkid, -adj);
+ clockadj_step(clkid, -tmv_to_nanoseconds(c->master_offset));
c->t1 = tmv_zero();
c->t2 = tmv_zero();
if (c->sanity_check) {
@@ -1418,8 +1416,8 @@ enum servo_state clock_synchronize(struct clock *c,
}
break;
case SERVO_LOCKED:
- clockadj_set_freq(c->clkid, -adj);
- if (c->clkid == CLOCK_REALTIME)
+ clockadj_set_freq(clkid, -adj);
+ if (clkid == CLOCK_REALTIME)
sysclk_set_sync();
if (c->sanity_check)
clockcheck_set_freq(c->sanity_check, -adj);
diff --git a/clock.h b/clock.h
index a2b46be..b5ddd38 100644
--- a/clock.h
+++ b/clock.h
@@ -207,6 +207,7 @@ UInteger16 clock_steps_removed(struct clock *c);
/**
* Provide a data point to synchronize the clock.
* @param c The clock instance to synchronize.
+ * @param clkid The posix clock ID to synchronize.
* @param ingress_ts The ingress time stamp on the sync message.
* @param origin_ts The reported transmission time of the sync message.
* @param correction1 The correction field of the sync message.
@@ -215,6 +216,7 @@ UInteger16 clock_steps_removed(struct clock *c);
* @return The state of the clock's servo.
*/
enum servo_state clock_synchronize(struct clock *c,
+ clockid_t clkid,
struct timespec ingress_ts,
struct timestamp origin_ts,
Integer64 correction1,
diff --git a/port.c b/port.c
index f39ad1e..2176962 100644
--- a/port.c
+++ b/port.c
@@ -64,6 +64,7 @@ struct port {
LIST_ENTRY(port) list;
char *name;
struct clock *clock;
+ clockid_t clkid;
struct transport *trp;
enum timestamp_type timestamping;
struct fdarray fda;
@@ -990,7 +991,7 @@ static void port_synchronize(struct port *p,

port_set_sync_rx_tmo(p);

- state = clock_synchronize(p->clock, ingress_ts, origin_ts,
+ state = clock_synchronize(p->clock, p->clkid, ingress_ts, origin_ts,
correction1, correction2);
switch (state) {
case SERVO_UNLOCKED:
@@ -2441,7 +2442,8 @@ err:
msg_put(msg);
}

-struct port *port_open(int phc_index,
+struct port *port_open(clockid_t clkid,
+ int phc_index,
enum timestamp_type timestamping,
int number,
struct interface *interface,
@@ -2454,6 +2456,8 @@ struct port *port_open(int phc_index,

memset(p, 0, sizeof(*p));

+ p->clkid = clkid;
+
if (interface->transport == TRANS_UDS)
; /* UDS cannot have a PHC. */
else if (!interface->ts_info.valid)
diff --git a/port.h b/port.h
index 662eaef..b5b9841 100644
--- a/port.h
+++ b/port.h
@@ -187,6 +187,7 @@ void port_notify_event(struct port *p, enum notification event);

/**
* Open a network port.
+ * @param clkid The posix clock ID for this node.
* @param phc_index The PHC device index for the network device.
* @param timestamping The timestamping mode for this port.
* @param number An arbitrary number assigned to this port.
@@ -194,7 +195,8 @@ void port_notify_event(struct port *p, enum notification event);
* @param clock A pointer to the system PTP clock.
* @return A pointer to an open port on success, or NULL otherwise.
*/
-struct port *port_open(int phc_index,
+struct port *port_open(clockid_t clkid,
+ int phc_index,
enum timestamp_type timestamping,
int number,
struct interface *interface,

[Jiri Benc]

Post by Richard Cochran
This patch lets the ports (rather than the clock) remember the clock ID,
passing it back to the clock code during synchronization. This does not
represent a functional change, but rather paves the way for ports to
override the clock ID when running a "jbod" BC.
---
clock.c | 50 ++++++++++++++++++++++++--------------------------
clock.h | 2 ++
port.c | 8 ++++++--
port.h | 4 +++-
4 files changed, 35 insertions(+), 29 deletions(-)
diff --git a/clock.c b/clock.c
index df82d2b..ca902ef 100644
--- a/clock.c
+++ b/clock.c
@@ -72,7 +72,6 @@ struct clock_subscriber {
};
struct clock {
- clockid_t clkid;
struct servo *servo;
struct defaultDS dds;
struct dataset default_dataset;
@@ -268,9 +267,6 @@ void clock_destroy(struct clock *c)
}
port_close(c->uds_port);
free(c->pollfd);
- if (c->clkid != CLOCK_REALTIME) {
- phc_close(c->clkid);
- }

I don't like that phc_close is not called anymore after this patch.
I understand that it's not easy to do but IMHO this points out that
this approach is not the best one.

What about creating a reference counted structure holding clkid and
reference it from struct port? This seems even better to me than my
original patch splitting struct clock.

Jiri

[Richard Cochran]

Post by Jiri Benc
I don't like that phc_close is not called anymore after this patch.
I understand that it's not easy to do but IMHO this points out that
this approach is not the best one.

You are right that it is nicer and cleaner to close what we open.
But that is easy to do, too:

The clock instance can keep the ID and close it as before. Then, each
port must only remember if they opened their own ID, and if so, close
it again at the end.

I'll respin this series on top of you new_state fix and incorporate
the other comments.

Post by Jiri Benc
What about creating a reference counted structure holding clkid and
reference it from struct port? This seems even better to me than my
original patch splitting struct clock.

I think ref counts are overkill here.

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran
The clock instance can keep the ID and close it as before.

I'll add this into V3.

Post by Richard Cochran
Then, each
port must only remember if they opened their own ID, and if so, close
it again at the end.

I already had this in port_close().

if (p->max_adj)
phc_close(p->clkid);


Thanks,
Richard

[Richard Cochran]

If the user has configured the appropriate option, then simply warn about
the clock device mismatch, open the port's clock device, and then go on.

Signed-off-by: Richard Cochran <***@gmail.com>
---
port.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/port.c b/port.c
index 2176962..95b6794 100644
--- a/port.c
+++ b/port.c
@@ -29,6 +29,7 @@
#include "filter.h"
#include "missing.h"
#include "msg.h"
+#include "phc.h"
#include "port.h"
#include "print.h"
#include "sk.h"
@@ -65,6 +66,7 @@ struct port {
char *name;
struct clock *clock;
clockid_t clkid;
+ int max_adj;
struct transport *trp;
enum timestamp_type timestamping;
struct fdarray fda;
@@ -1952,6 +1954,8 @@ void port_close(struct port *p)
filter_destroy(p->delay_filter);
if (p->fault_fd >= 0)
close(p->fault_fd);
+ if (p->max_adj)
+ phc_close(p->clkid);
free(p);
}

@@ -2463,10 +2467,25 @@ struct port *port_open(clockid_t clkid,
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) {
+ char phc[32];
+ snprintf(phc, 31, "/dev/ptp%d", interface->ts_info.phc_index);
pr_err("port %d: PHC device mismatch", number);
- pr_err("port %d: /dev/ptp%d requested, but /dev/ptp%d attached",
- number, phc_index, interface->ts_info.phc_index);
- goto err_port;
+ pr_err("port %d: /dev/ptp%d requested, but %s attached",
+ number, phc_index, phc);
+ if (!interface->boundary_clock_jbod) {
+ goto err_port;
+ }
+ pr_warning("port %d: just a bunch of devices", number);
+ p->clkid = phc_open(phc);
+ if (p->clkid == CLOCK_INVALID) {
+ pr_err("port %d: Failed to open %s: %m", number, phc);
+ goto err_port;
+ }
+ p->max_adj = phc_max_adj(p->clkid);
+ if (!p->max_adj) {
+ pr_err("port %d: clock is not adjustable", number);
+ goto err_port;
+ }
}

p->pod = interface->pod;

[Jiri Benc]

Post by Richard Cochran
If the user has configured the appropriate option, then simply warn about
the clock device mismatch, open the port's clock device, and then go on.
---
port.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/port.c b/port.c
index 2176962..95b6794 100644
--- a/port.c
+++ b/port.c
@@ -29,6 +29,7 @@
#include "filter.h"
#include "missing.h"
#include "msg.h"
+#include "phc.h"
#include "port.h"
#include "print.h"
#include "sk.h"
@@ -65,6 +66,7 @@ struct port {
char *name;
struct clock *clock;
clockid_t clkid;
+ int max_adj;
struct transport *trp;
enum timestamp_type timestamping;
struct fdarray fda;
@@ -1952,6 +1954,8 @@ void port_close(struct port *p)
filter_destroy(p->delay_filter);
if (p->fault_fd >= 0)
close(p->fault_fd);
+ if (p->max_adj)
+ phc_close(p->clkid);
free(p);
}
@@ -2463,10 +2467,25 @@ struct port *port_open(clockid_t clkid,
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) {
+ char phc[32];
+ snprintf(phc, 31, "/dev/ptp%d", interface->ts_info.phc_index);
pr_err("port %d: PHC device mismatch", number);
- pr_err("port %d: /dev/ptp%d requested, but /dev/ptp%d attached",
- number, phc_index, interface->ts_info.phc_index);
- goto err_port;
+ pr_err("port %d: /dev/ptp%d requested, but %s attached",
+ number, phc_index, phc);
+ if (!interface->boundary_clock_jbod) {
+ goto err_port;
+ }
+ pr_warning("port %d: just a bunch of devices", number);

I think this warning is enough, the previous ones ("PHC device
mismatch" and "/dev/ptp%d requested, but %s attached") can be printed
only when boundary_clock_jbod is not set. After all, the user is
supposed to know what he's doing when enabling this.

Post by Richard Cochran
+ p->clkid = phc_open(phc);
+ if (p->clkid == CLOCK_INVALID) {
+ pr_err("port %d: Failed to open %s: %m", number, phc);
+ goto err_port;
+ }
+ p->max_adj = phc_max_adj(p->clkid);
+ if (!p->max_adj) {
+ pr_err("port %d: clock is not adjustable", number);
+ goto err_port;
+ }
}
p->pod = interface->pod;

I think the servo state should be separate, too. The reference counted
structure would be a good place to hold that.

Jiri

[Richard Cochran]

Post by Jiri Benc
I think the servo state should be separate, too. The reference counted
structure would be a good place to hold that.

Why should it be separate?

After all, there is only one virtual clock to be adjusted. The
separate devices should be automagically synchronized by phc2sys.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran
Here is my current series. The first patch is the improved Tx time
stamp polling, and patches 2-5 are trivial clean ups.

Those look good to me.

Post by Richard Cochran
Patches 6-10 represent my second attempt at the "jbod" BC. With any
luck, this should work. Compile tested only!

In a test with two ports where the slave/master states are swapped,
it seems to be working as expected now.

I noticed another problem, however. When the two ports are both in a
non-slave state, phc2sys doesn't keep them close together. Should
phc2sys pick the synchronization source randomly?

Post by Richard Cochran
(We still would need to change the max_adj for the servo, and maybe
there are a few other things I have forgotten.)

Hm, yes. Maybe also the clock check should be disabled. The problem is
that the clock struct represents a single physical clock and now it
would be a collection of clocks pretending it's one clock.

[Richard Cochran]

Post by Miroslav Lichvar
I noticed another problem, however. When the two ports are both in a
non-slave state, phc2sys doesn't keep them close together. Should
phc2sys pick the synchronization source randomly?

Yes, or maybe pick port #1, giving the user some control via the order
configured in ptp4l.

Thanks,
Richard


------------------------------------------------------------------------------