Re: [Linuxptp-devel] [PATCH v2] Add support for DELAY_REQ and SYNC packets RX filters
Hi Ilia, On Fri, Nov 24, 2023 at 10:29:12PM +0300, IlorDash wrote: > From: Ilya Orazov > > I’m using an Ethernet controller with PTP support, which requires > determining which PTP packets on RX must be timestamped: SYNC or > DELAY_REQ, based on whether the device is Slave or Master respectively. > So I can’t use the EVENT RX filter and must dynamically switch RX > filters, when port state changes from Master to Slave and vice versa. > > Therefore this feature adds support for DELAY_REQ and SYNC packets > RX filters. If the interface doesn’t support the EVENT RX filter > (getting info about it through ethtool) then RX filters will dynamically > switch between SYNC and DELAY_REQ when port state changes. > > Signed-off-by: Ilya Orazov > --- What hardware are you targeting, specifically? I know the KSZ9477 DSA switch family has this quirk as well. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Add option to bind raw and UDP sockets to interface
On Tue, Mar 14, 2023 at 12:25:18PM -0700, Richard Cochran wrote: > On Tue, Mar 14, 2023 at 12:38:06PM +0200, Kamil Zaripov wrote: > > Can you explain the problems you see with timestamping in the interface on > > top of a bridge? > > When a MAC joins a bridge, the MAC is no longer avaiable as a network > interface. This is how the bridge thing is implemented in Linux. That is approximately true (by default), but with your permission, some nuance might help. The bridge driver has an rx_handler which steals all traffic from the bridge ports, so it can be processed by sockets opened on the bridge device itself. The exception is link-local multicast traffic (this is why L2 PTP works over sockets opened on bridge ports, or at least with gPTP it does, where all traffic is in the reserved 01-80-c2-00-00-xx space), but it's also possible to add netfilter rules to tell the bridge to stop stealing other traffic flows, such that those remain visible to sockets opened on the bridge ports rather than on the bridge itself. I happened to have these commands sitting around in a drawer, tailored particularly to running PTP over bridge ports. Some adjustments might be necessary depending on distribution and kernel config options. # PTP over L2 /sbin/ebtables --table broute --append BROUTING --protocol 0x88F7 --jump DROP # PTP over IPv4 /sbin/ebtables --table broute --append BROUTING --protocol 0x0800 --ip-protocol udp --ip-destination-port 320 --jump DROP /sbin/ebtables --table broute --append BROUTING --protocol 0x0800 --ip-protocol udp --ip-destination-port 319 --jump DROP # PTP over IPv6 /sbin/ebtables --table broute --append BROUTING --protocol 0x86DD --ip6-protocol udp --ip6-destination-port 320 --jump DROP /sbin/ebtables --table broute --append BROUTING --protocol 0x86DD --ip6-protocol udp --ip6-destination-port 319 --jump DROP ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Add option to bind raw and UDP sockets to interface
On Tue, Mar 14, 2023 at 12:38:06PM +0200, Kamil Zaripov wrote: > > On 13 Mar 2023, at 20:35, Richard Cochran wrote: > > > > "It works for me" is not a strong argument. This software stack must > > work for everyone. > > I agree that “It works for me” is not enough to merge this patch. > > > Time stamping on top of a bridge interface won't > > fly in general, if I'm not mistaken. > > Can you explain the problems you see with timestamping in the interface on > top of a bridge? > > From my point of view when you call setsockopt(.., SO_TIMESTAMP**, > SOF_TIMESTAMPING_**_HARDWARE) > it does not matter if any network card on your system support hw timestamping > capabilities. > At this point network card only record all socket option values to the socket > structure: > https://elixir.bootlin.com/linux/latest/source/net/core/sock.c#L895. > Next during package send we check if socket have appropriate timestamping > flags, and if so > we copy this info to the sk_buff structure: > https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L2768. > At the end after package finally ready to be send in ndo_start_xmit > https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L1401 > we either implement required times taping features or not. For example > igb driver checks if tx_flags in skb_buf have timestamping request > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igb/igb_main.c#L6432 > and ask hardware to timestamp frame. > > So I think that it doesn’t matter how complex your network configuration, > only think that matters is which network card will actually handle frames. > > But I’m neither linux kernel networking expert nor PTP expert. If I’m wrong > please correct me. What will happen if the bridge floods the frame to 2 bridge ports, and both supports hardware TX timestamping? How many TX timestamps will be collected by the kernel, and more importantly, which ones? How many of those will be delivered to user space? Is ptp4l prepared to process more than one hardware TX timestamp per sent packet? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Code freeze for 4.0 release
Hi Richard, On Wed, Feb 22, 2023 at 07:03:59AM -0800, Richard Cochran wrote: > Dear Devs, > > I'll be releasing 4.0 in the next day or two. I won't be taking any > new stuff until after the release. If you have any last minute > critical bug fixes, please let me know ASAP. > > Thanks, > Richard It would have been nice to have a bit longer due notice. I had this performance related patch which had stalled due to some VLAN related issues: https://www.mail-archive.com/linuxptp-devel@lists.sourceforge.net/msg04346.html Seeing how long it takes between linuxptp releases, I guess it would make some sense for me to try to make an effort to come back to that and fix it before this release is made. I remember testing with some VLANs inserted into packets via tc-vlan actions, and the first (and problematic) patch, "Revert "raw: accept VLAN tagged packets."", turned out to be quite unnecessary. It appeared to me that ETH_P_1588 packets got delivered to the ETH_P_1588 socket regardless of whether they were VLAN-tagged or not, which was just what was needed. Maybe it's best to not stall this release, but would it make sense to be more upfront even about future release dates, and/or to plan more of these more frequently? Work expands to fill free time, so it's quite easy to get caught up in other stuff. Thanks, Vladimir ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v6 02/10] ts2phc: instantiate a full clock structure for every PPS sink
Currently in ts2phc, PPS sinks are also the only candidates for the
clocks that get synchronized. There are 2 aspects that make this too
restrictive:
- Not all PPS sinks may be target clocks for synchronization. Consider a
dynamic environment of multiple DSA switches using boundary_clock_jbod,
and only one port is in the PS_SLAVE state. In that case, the clock of
that port should be the reference for synchronization, regardless of
whether it supports the extts API or not.
- Not all target clocks for synchronization may be PPS sinks.
Specifically, the "PHC" type of PPS master in ts2phc can also be,
fundamentally, treated as a target clock, and disciplined.
The code base should be prepared to handle the distinction that exists
between these concepts, by recognizing that things that can be
disciplined by a servo should be represented by a "struct ts2phc_clock",
and things that can timestamp external events should be represented by a
"struct ts2phc_pps_sink".
Signed-off-by: Vladimir Oltean
---
v5->v6:
- add back int fd to struct ts2phc_clock, and use it instead of
CLOCKID_TO_FD every time
v4->v5:
- Rebased on top of data structure renames
v3->v4:
- rebased, needed to pass priv->cfg in ts2phc_nmea_master_create
- setting clock->is_destination can be delayed to a future patch
v2->v3:
None.
ts2phc.c | 82 ++
ts2phc.h | 22 +
ts2phc_pps_sink.c | 83 +--
3 files changed, 134 insertions(+), 53 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index f228351bae3a..13d70dc6e7e1 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -6,10 +6,15 @@
* @note Copyright (C) 2012 Richard Cochran
* @note SPDX-License-Identifier: GPL-2.0+
*/
+#include
#include
+#include
+#include
+#include "clockadj.h"
#include "config.h"
#include "interface.h"
+#include "phc.h"
#include "print.h"
#include "ts2phc.h"
#include "version.h"
@@ -27,6 +32,83 @@ static void ts2phc_cleanup(struct ts2phc_private *priv)
config_destroy(priv->cfg);
}
+static struct servo *ts2phc_servo_create(struct ts2phc_private *priv,
+struct ts2phc_clock *clock)
+{
+ enum servo_type type = config_get_int(priv->cfg, NULL, "clock_servo");
+ struct servo *servo;
+ int fadj, max_adj;
+
+ fadj = (int) clockadj_get_freq(clock->clkid);
+ /* Due to a bug in older kernels, the reading may silently fail
+* and return 0. Set the frequency back to make sure fadj is
+* the actual frequency of the clock.
+*/
+ if (!clock->no_adj) {
+ clockadj_set_freq(clock->clkid, fadj);
+ }
+
+ max_adj = phc_max_adj(clock->clkid);
+
+ servo = servo_create(priv->cfg, type, -fadj, max_adj, 0);
+ if (!servo)
+ return NULL;
+
+ servo_sync_interval(servo, SERVO_SYNC_INTERVAL);
+
+ return servo;
+}
+
+struct ts2phc_clock *ts2phc_clock_add(struct ts2phc_private *priv,
+ const char *device)
+{
+ clockid_t clkid = CLOCK_INVALID;
+ struct ts2phc_clock *c;
+ int phc_index = -1;
+ int err;
+
+ clkid = posix_clock_open(device, _index);
+ if (clkid == CLOCK_INVALID)
+ return NULL;
+
+ LIST_FOREACH(c, >clocks, list) {
+ if (c->phc_index == phc_index) {
+ /* Already have the clock, don't add it again */
+ posix_clock_close(clkid);
+ return c;
+ }
+ }
+
+ c = calloc(1, sizeof(*c));
+ if (!c) {
+ pr_err("failed to allocate memory for a clock");
+ return NULL;
+ }
+ c->clkid = clkid;
+ c->fd = CLOCKID_TO_FD(clkid);
+ c->phc_index = phc_index;
+ c->servo_state = SERVO_UNLOCKED;
+ c->servo = ts2phc_servo_create(priv, c);
+ c->no_adj = config_get_int(priv->cfg, NULL, "free_running");
+ err = asprintf(>name, "/dev/ptp%d", phc_index);
+ if (err < 0) {
+ free(c);
+ posix_clock_close(clkid);
+ return NULL;
+ }
+
+ LIST_INSERT_HEAD(>clocks, c, list);
+ return c;
+}
+
+void ts2phc_clock_destroy(struct ts2phc_clock *c)
+{
+ servo_destroy(c->servo);
+ posix_clock_close(c->clkid);
+ free(c->name);
+ free(c);
+}
+
static void usage(char *progname)
{
fprintf(stderr,
diff --git a/ts2phc.h b/ts2phc.h
index c6b977286f20..589f88725184 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -7,17 +7,39 @@
#ifndef HAVE_TS2PHC_H
#define HAVE_TS2PHC_H
+#include
+#include
+
+#include "servo.h"
#include "ts2ph
[Linuxptp-devel] [PATCH v6 04/10] util: import port_state_normalize() logic from phc2sys
PMC agents such as phc2sys tend to suppress uninteresting port state
transitions, to avoid running useless code when nothing really changed
in ptp4l.
With the addition of a PMC agent in ts2phc, it becomes desirable to
place that logic in a common implementation file, to avoid duplication.
Choose util.c for lack of a better place. Also add some comments and
rewrite the implementation so that it is a bit more clear. No change
intended in the produced code.
Signed-off-by: Vladimir Oltean
---
v5->v6: patch is new
phc2sys.c | 18 +++---
util.c| 13 +
util.h| 8
3 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/phc2sys.c b/phc2sys.c
index 63ec6d56ba3b..2c8e905e7c2c 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -117,8 +117,6 @@ static int clock_handle_leap(struct phc2sys_private *priv,
struct clock *clock,
int64_t offset, uint64_t ts);
-static int normalize_state(int state);
-
static struct servo *servo_add(struct phc2sys_private *priv,
struct clock *clock)
{
@@ -316,7 +314,7 @@ static void clock_reinit(struct phc2sys_private *priv,
struct clock *clock,
, _index,
iface);
if (!err) {
- p->state = normalize_state(state);
+ p->state = port_state_normalize(state);
}
break;
}
@@ -770,16 +768,6 @@ static int do_loop(struct phc2sys_private *priv)
return 0;
}
-static int normalize_state(int state)
-{
- if (state != PS_MASTER && state != PS_SLAVE &&
- state != PS_PRE_MASTER && state != PS_UNCALIBRATED) {
- /* treat any other state as "not a master nor a slave" */
- state = PS_DISABLED;
- }
- return state;
-}
-
static int clock_compute_state(struct phc2sys_private *priv,
struct clock *clock)
{
@@ -820,7 +808,7 @@ static int phc2sys_recv_subscribed(void *context, struct
ptp_message *msg,
pid2str(>portIdentity));
return 1;
}
- state = normalize_state(pds->portState);
+ state = port_state_normalize(pds->portState);
if (port->state != state) {
pr_info("port %s changed state",
pid2str(>portIdentity));
@@ -892,7 +880,7 @@ static int auto_init_ports(struct phc2sys_private *priv,
int add_rt)
port = port_add(priv, i, iface, phc_index);
if (!port)
return -1;
- port->state = normalize_state(state);
+ port->state = port_state_normalize(state);
}
if (LIST_EMPTY(>clocks)) {
pr_err("no suitable ports available");
diff --git a/util.c b/util.c
index a59b559ddf1c..e204c9cdb02a 100644
--- a/util.c
+++ b/util.c
@@ -207,6 +207,19 @@ const char *ustate2str(enum unicast_state ustate)
return "???";
}
+enum port_state port_state_normalize(enum port_state state)
+{
+ switch (state) {
+ case PS_MASTER:
+ case PS_SLAVE:
+ case PS_PRE_MASTER:
+ case PS_UNCALIBRATED:
+ return state;
+ default:
+ return PS_DISABLED;
+ }
+}
+
void posix_clock_close(clockid_t clock)
{
if (clock == CLOCK_REALTIME) {
diff --git a/util.h b/util.h
index 558a6757131e..542f3b544ddb 100644
--- a/util.h
+++ b/util.h
@@ -26,6 +26,7 @@
#include "address.h"
#include "ddt.h"
#include "ether.h"
+#include "fsm.h"
#include "transport.h"
#include "unicast_fsm.h"
@@ -113,6 +114,13 @@ char *portaddr2str(struct PortAddress *addr);
const char *ustate2str(enum unicast_state ustate);
+/**
+ * Reduce all port states for which the sync direction isn't known to
+ * PS_DISABLED, and report the given port state otherwise. This minimizes port
+ * state transitions for PMC agents when nothing interesting happened.
+ */
+enum port_state port_state_normalize(enum port_state state);
+
/**
* Closes a dynamic posix clock.
* @param clock A clock ID obtained via posix_clock_close().
--
2.34.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v6 08/10] ts2phc: reconfigure sync direction by subscribing to ptp4l port events
Monitor the port state change events from ptp4l, and use that
information to determine the "reference" clock.
Then synchronize all other clocks in our list to that source, by feeding
into their respective servo loop an offset equal to the delta between
their timestamp and the timestamp of the reference clock. All timestamps
are representative of the same event, which is the most recent perout
pulse of the PPS source.
Signed-off-by: Vladimir Oltean
---
v5->v6:
- None.
v4->v5:
- Rename source clock to reference clock, destination clock to target
clock
- Rebase on top of earlier data structure renaming (master -> PPS
source, slave -> PPS sink)
v3->v4:
- Use bool for boolean types.
v2->v3:
- None.
ts2phc.c| 130
ts2phc.h| 3 +
ts2phc_phc_pps_source.c | 1 +
ts2phc_pps_sink.c | 1 +
4 files changed, 123 insertions(+), 12 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index 5aaa2702c787..4d23a5dcdf5a 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -113,6 +113,7 @@ static int ts2phc_recv_subscribed(void *context, struct
ptp_message *msg,
state = ts2phc_clock_compute_state(priv, clock);
if (clock->state != state || clock->new_state) {
clock->new_state = state;
+ priv->state_changed = true;
}
}
return 1;
@@ -317,33 +318,126 @@ static int ts2phc_auto_init_ports(struct ts2phc_private
*priv)
LIST_FOREACH(clock, >clocks, list) {
clock->new_state = ts2phc_clock_compute_state(priv, clock);
}
+ priv->state_changed = true;
return 0;
}
-static void ts2phc_synchronize_clocks(struct ts2phc_private *priv)
+static void ts2phc_reconfigure(struct ts2phc_private *priv)
+{
+ struct ts2phc_clock *c, *ref_clk = NULL, *last = NULL;
+ int num_ref_clocks = 0, num_target_clocks = 0;
+
+ pr_info("reconfiguring after port state change");
+ priv->state_changed = false;
+
+ LIST_FOREACH(c, >clocks, list) {
+ if (c->new_state) {
+ c->state = c->new_state;
+ c->new_state = 0;
+ }
+
+ switch (c->state) {
+ case PS_FAULTY:
+ case PS_DISABLED:
+ case PS_LISTENING:
+ case PS_PRE_MASTER:
+ case PS_MASTER:
+ case PS_PASSIVE:
+ if (!c->is_target) {
+ pr_info("selecting %s for synchronization",
+ c->name);
+ c->is_target = true;
+ }
+ num_target_clocks++;
+ break;
+ case PS_UNCALIBRATED:
+ num_ref_clocks++;
+ break;
+ case PS_SLAVE:
+ ref_clk = c;
+ num_ref_clocks++;
+ break;
+ default:
+ break;
+ }
+ last = c;
+ }
+ if (num_target_clocks >= 1 && !ref_clk) {
+ priv->ref_clock = last;
+ priv->ref_clock->is_target = false;
+ /* Reset to original state in next reconfiguration. */
+ priv->ref_clock->new_state = priv->ref_clock->state;
+ priv->ref_clock->state = PS_SLAVE;
+ pr_info("no reference clock, selecting %s by default",
+ last->name);
+ return;
+ }
+ if (num_ref_clocks > 1) {
+ pr_info("multiple reference clocks available, postponing
sync...");
+ priv->ref_clock = NULL;
+ return;
+ }
+ if (num_ref_clocks > 0 && !ref_clk) {
+ pr_info("reference clock not ready, waiting...");
+ priv->ref_clock = NULL;
+ return;
+ }
+ if (!num_ref_clocks && !num_target_clocks) {
+ pr_info("no PHC ready, waiting...");
+ priv->ref_clock = NULL;
+ return;
+ }
+ if (!num_ref_clocks) {
+ pr_info("nothing to synchronize");
+ priv->ref_clock = NULL;
+ return;
+ }
+ ref_clk->is_target = false;
+ priv->ref_clock = ref_clk;
+ pr_info("selecting %s as the reference clock", ref_clk->name);
+}
+
+static void ts2phc_synchronize_clocks(struct ts2phc_private *priv, int autocfg)
{
- struct timespec source_ts;
tmv_t source_tmv;
struct ts2phc_clock *c;
int valid, err;
-
[Linuxptp-devel] [PATCH v6 03/10] ts2phc: instantiate a full clock structure for every PPS source of the PHC kind
This propagates the use of "struct ts2phc_private" all the way into the
PPS source API, in preparation of a new use case that will be supported
soon: some PPS sources (to be precise, the "PHC" kind) instantiate a
struct clock which could be disciplined by ts2phc.
When a PHC A emits a pulse and another PHC B timestamps it, the offset
between their precise timestamps can be used to synchronize either one
of them. So far in ts2phc, only the sink PHC (the one using extts) has
been synchronized to the source (the one using perout).
This is partly because there is no proper kernel API to report the
precise timestamp of a perout pulse. We only have the periodic API, and
that doesn't report precise timestamps either; we just use vague
approximations of what the PPS source PHC's time was, based on reading
that PHC immediately after a sink extts event was received by the
application. While this is far from ideal, it does work, and does allow
PHC A to be synchronized to B.
This is particularly useful with the yet-to-be-introduced "automatic"
mode of ts2phc (similar to '-a' of phc2sys), and the PPS distribution
tree is fixed in hardware (as opposed to port states, which in
"automatic" mode are dynamic, as the name suggests).
Signed-off-by: Vladimir Oltean
---
v5->v6:
- continue to use clock->fd as opposed to CLOCKID_TO_FD
- some include file cleanups, stop touching ts2phc_pps_sink.c from a
patch intended for PPS sources.
ts2phc.c| 2 +-
ts2phc.h| 1 +
ts2phc_generic_pps_source.c | 2 +-
ts2phc_generic_pps_source.h | 3 ++-
ts2phc_nmea_pps_source.c| 6 +++---
ts2phc_nmea_pps_source.h| 3 ++-
ts2phc_phc_pps_source.c | 31 +++
ts2phc_phc_pps_source.h | 3 ++-
ts2phc_pps_source.c | 10 ++
ts2phc_pps_source.h | 6 --
10 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index 13d70dc6e7e1..1f30a9616ec4 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -271,7 +271,7 @@ int main(int argc, char *argv[])
} else {
pps_type = TS2PHC_PPS_SOURCE_PHC;
}
- priv.src = ts2phc_pps_source_create(cfg, pps_source, pps_type);
+ priv.src = ts2phc_pps_source_create(, pps_source, pps_type);
if (!priv.src) {
fprintf(stderr, "failed to create PPS source\n");
ts2phc_cleanup();
diff --git a/ts2phc.h b/ts2phc.h
index 589f88725184..3b341419946e 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -7,6 +7,7 @@
#ifndef HAVE_TS2PHC_H
#define HAVE_TS2PHC_H
+#include
#include
#include
diff --git a/ts2phc_generic_pps_source.c b/ts2phc_generic_pps_source.c
index 397031fe2521..6842d8e0e30a 100644
--- a/ts2phc_generic_pps_source.c
+++ b/ts2phc_generic_pps_source.c
@@ -47,7 +47,7 @@ static int ts2phc_generic_pps_source_getppstime(struct
ts2phc_pps_source *src,
return 0;
}
-struct ts2phc_pps_source *ts2phc_generic_pps_source_create(struct config *cfg,
+struct ts2phc_pps_source *ts2phc_generic_pps_source_create(struct
ts2phc_private *priv,
const char *dev)
{
struct ts2phc_generic_pps_source *src;
diff --git a/ts2phc_generic_pps_source.h b/ts2phc_generic_pps_source.h
index e169ab380ccf..bed62740059e 100644
--- a/ts2phc_generic_pps_source.h
+++ b/ts2phc_generic_pps_source.h
@@ -6,9 +6,10 @@
#ifndef HAVE_TS2PHC_GENERIC_PPS_SOURCE_H
#define HAVE_TS2PHC_GENERIC_PPS_SOURCE_H
+#include "ts2phc.h"
#include "ts2phc_pps_source.h"
-struct ts2phc_pps_source *ts2phc_generic_pps_source_create(struct config *cfg,
+struct ts2phc_pps_source *ts2phc_generic_pps_source_create(struct
ts2phc_private *priv,
const char *dev);
#endif
diff --git a/ts2phc_nmea_pps_source.c b/ts2phc_nmea_pps_source.c
index 625b9fd3f686..db8b5c6297c7 100644
--- a/ts2phc_nmea_pps_source.c
+++ b/ts2phc_nmea_pps_source.c
@@ -249,7 +249,7 @@ static int ts2phc_nmea_pps_source_getppstime(struct
ts2phc_pps_source *src,
return lstab_error;
}
-struct ts2phc_pps_source *ts2phc_nmea_pps_source_create(struct config *cfg,
+struct ts2phc_pps_source *ts2phc_nmea_pps_source_create(struct ts2phc_private
*priv,
const char *dev)
{
struct ts2phc_nmea_pps_source *s;
@@ -260,7 +260,7 @@ struct ts2phc_pps_source
*ts2phc_nmea_pps_source_create(struct config *cfg,
if (!s) {
return NULL;
}
- s->leapfile = config_get_string(cfg, NULL, "leapfile");
+ s->leapfile = config_get_string(priv->cfg, NULL, "leapfile");
s->lstab = lstab_create(s->leapfile);
if (!s->lstab) {
free(s);
@@ -277,7 +277,7 @@ struct ts2phc_pps_source
*ts2phc_nmea_pps_source_create(struct confi
[Linuxptp-devel] [PATCH v6 07/10] ts2phc: split PPS sink poll from servo loop
Previous changes/clarifications to ts2phc have established that:
- a PPS sink deals with extts events
- a clock deals with synchronization via a servo loop
Therefore, the code for synchronization should not be part of the
implementation of a PPS sink. Move it to the main ts2phc.c.
This allows it to be used by a PPS source as well (the PHC kind).
Signed-off-by: Vladimir Oltean
---
v5->v6:
- freeing the pollfd array is now part of patch context rather than
patch additions
v4->v5:
- Rebase on top of the variable renaming.
v3->v4:
- Use bool for boolean types.
v2->v3:
- None.
ts2phc.c | 94 +-
ts2phc.h | 3 +
ts2phc_pps_sink.c | 193 +-
3 files changed, 184 insertions(+), 106 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index fc8a00037dcb..5aaa2702c787 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -21,6 +21,9 @@
#include "ts2phc.h"
#include "version.h"
+#define NS_PER_SEC 10LL
+#define SAMPLE_WEIGHT 1.0
+
struct interface {
STAILQ_ENTRY(interface) list;
};
@@ -144,6 +147,30 @@ static struct servo *ts2phc_servo_create(struct
ts2phc_private *priv,
return servo;
}
+void ts2phc_clock_add_tstamp(struct ts2phc_clock *clock, tmv_t t)
+{
+ struct timespec ts = tmv_to_timespec(t);
+
+ pr_debug("adding tstamp %ld.%09ld to clock %s",
+ts.tv_sec, ts.tv_nsec, clock->name);
+ clock->last_ts = t;
+ clock->is_ts_available = true;
+}
+
+static int ts2phc_clock_get_tstamp(struct ts2phc_clock *clock, tmv_t *ts)
+{
+ if (!clock->is_ts_available)
+ return 0;
+ clock->is_ts_available = false;
+ *ts = clock->last_ts;
+ return 1;
+}
+
+static void ts2phc_clock_flush_tstamp(struct ts2phc_clock *clock)
+{
+ clock->is_ts_available = false;
+}
+
struct ts2phc_clock *ts2phc_clock_add(struct ts2phc_private *priv,
const char *device)
{
@@ -294,6 +321,64 @@ static int ts2phc_auto_init_ports(struct ts2phc_private
*priv)
return 0;
}
+static void ts2phc_synchronize_clocks(struct ts2phc_private *priv)
+{
+ struct timespec source_ts;
+ tmv_t source_tmv;
+ struct ts2phc_clock *c;
+ int valid, err;
+
+ err = ts2phc_pps_source_getppstime(priv->src, _ts);
+ if (err < 0) {
+ pr_err("source ts not valid");
+ return;
+ }
+ if (source_ts.tv_nsec > NS_PER_SEC / 2)
+ source_ts.tv_sec++;
+ source_ts.tv_nsec = 0;
+
+ source_tmv = timespec_to_tmv(source_ts);
+
+ LIST_FOREACH(c, >clocks, list) {
+ int64_t offset;
+ double adj;
+ tmv_t ts;
+
+ valid = ts2phc_clock_get_tstamp(c, );
+ if (!valid) {
+ pr_debug("%s timestamp not valid, skipping", c->name);
+ continue;
+ }
+
+ offset = tmv_to_nanoseconds(tmv_sub(ts, source_tmv));
+
+ if (c->no_adj) {
+ pr_info("%s offset %10" PRId64, c->name,
+ offset);
+ continue;
+ }
+
+ adj = servo_sample(c->servo, offset, tmv_to_nanoseconds(ts),
+ SAMPLE_WEIGHT, >servo_state);
+
+ pr_info("%s offset %10" PRId64 " s%d freq %+7.0f",
+ c->name, offset, c->servo_state, adj);
+
+ switch (c->servo_state) {
+ case SERVO_UNLOCKED:
+ break;
+ case SERVO_JUMP:
+ clockadj_set_freq(c->clkid, -adj);
+ clockadj_step(c->clkid, -offset);
+ break;
+ case SERVO_LOCKED:
+ case SERVO_LOCKED_STABLE:
+ clockadj_set_freq(c->clkid, -adj);
+ break;
+ }
+ }
+}
+
static void usage(char *progname)
{
fprintf(stderr,
@@ -492,11 +577,18 @@ int main(int argc, char *argv[])
}
while (is_running()) {
+ struct ts2phc_clock *c;
+
+ LIST_FOREACH(c, , list)
+ ts2phc_clock_flush_tstamp(c);
+
err = ts2phc_pps_sink_poll();
- if (err) {
+ if (err < 0) {
pr_err("poll failed");
break;
}
+ if (err > 0)
+ ts2phc_synchronize_clocks();
}
ts2phc_cleanup();
diff --git a/ts2phc.h b/ts2phc.h
index f01bad2c8b4c..ae95034b7113 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -31,6 +31,8 @@ struct ts2phc_clock {
enum servo_state servo_state;
ch
[Linuxptp-devel] [PATCH v6 09/10] ts2phc: allow PHC PPS sources to be synchronized
Now that we are registering a clock even for the PPS source when it
supports that (i.e. when it is a PHC), introduce a new API to retrieve
its clock in order to add timestamps to it.
The timestamps of periodic output transitions are not really known.
We configure the kernel to emit periodic output and then never hear back
from that PHC again. We just assume that it emits periodic output as
promised, and that each pulse is emitted at the beginning of each
second. We rely on the kernel to wake us up when PPS sinks have an extts
event to report, and we know who generated that - the PPS source, of
course. So then we proceed to read the PPS source's PHC time, and round
that to what is the most plausible integer second in its time base.
We believe that to be the 'timestamp'. This is fed into the servo
algorithm.
The PHC PPS source can be synchronized to the extts events of a PHC
slave, when in automatic mode.
Signed-off-by: Vladimir Oltean
---
v5->v6:
- eliminate use of offensive word "approximation"
v4->v5:
- rebase on top of data structure renames
v3->v4:
Add one more paragraph to commit message.
v2->v3:
Implement ts2phc_master_get_clock() as part of ts2phc_master.c instead
of ts2phc_phc_master.c.
ts2phc.c| 49 -
ts2phc_phc_pps_source.c | 9 +++
ts2phc_pps_source.c | 8 ++
ts2phc_pps_source.h | 2 ++
ts2phc_pps_source_private.h | 1 +
5 files changed, 68 insertions(+), 1 deletion(-)
diff --git a/ts2phc.c b/ts2phc.c
index 4d23a5dcdf5a..5f0acc31dd47 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -473,6 +473,46 @@ static void ts2phc_synchronize_clocks(struct
ts2phc_private *priv, int autocfg)
}
}
+static int ts2phc_collect_pps_source_tstamp(struct ts2phc_private *priv)
+{
+ struct ts2phc_clock *pps_src_clock;
+ struct timespec source_ts;
+ int err;
+
+ pps_src_clock = ts2phc_pps_source_get_clock(priv->src);
+ /*
+* PPS source isn't a PHC (it may be a generic or a GPS PPS source),
+* don't error out, just don't do anything. If it doesn't have a PHC,
+* there is nothing to synchronize, which is the only point of
+* collecting its perout timestamp in the first place.
+*/
+ if (!pps_src_clock)
+ return 0;
+
+ err = ts2phc_pps_source_getppstime(priv->src, _ts);
+ if (err < 0) {
+ pr_err("source ts not valid");
+ return err;
+ }
+
+ /*
+* As long as the kernel doesn't support a proper API for reporting
+* back a precise perout timestamp, we'll have to implicitly assume
+* assumption that the current time on the PPS source is still within
+* +/- half a second of the past perout output edge, and hence, we can
+* deduce the timestamp (actually only seconds part, nanoseconds are by
+* construction zero) of this edge at the emitter based on the
+* emitter's current time.
+*/
+ if (source_ts.tv_nsec > NS_PER_SEC / 2)
+ source_ts.tv_sec++;
+ source_ts.tv_nsec = 0;
+
+ ts2phc_clock_add_tstamp(pps_src_clock, timespec_to_tmv(source_ts));
+
+ return 0;
+}
+
static void usage(char *progname)
{
fprintf(stderr,
@@ -693,8 +733,15 @@ int main(int argc, char *argv[])
pr_err("poll failed");
break;
}
- if (err > 0)
+ if (err > 0) {
+ err = ts2phc_collect_pps_source_tstamp();
+ if (err) {
+ pr_err("failed to collect PPS source tstamp");
+ break;
+ }
+
ts2phc_synchronize_clocks(, autocfg);
+ }
}
ts2phc_cleanup();
diff --git a/ts2phc_phc_pps_source.c b/ts2phc_phc_pps_source.c
index 92273f4fc14d..f2a9c6196a45 100644
--- a/ts2phc_phc_pps_source.c
+++ b/ts2phc_phc_pps_source.c
@@ -82,6 +82,14 @@ static int ts2phc_phc_pps_source_getppstime(struct
ts2phc_pps_source *src,
return clock_gettime(s->clock->clkid, ts);
}
+struct ts2phc_clock *ts2phc_phc_pps_source_get_clock(struct ts2phc_pps_source
*src)
+{
+ struct ts2phc_phc_pps_source *s =
+ container_of(src, struct ts2phc_phc_pps_source, pps_source);
+
+ return s->clock;
+}
+
struct ts2phc_pps_source *ts2phc_phc_pps_source_create(struct ts2phc_private
*priv,
const char *dev)
{
@@ -93,6 +101,7 @@ struct ts2phc_pps_source
*ts2phc_phc_pps_source_create(struct ts2phc_private *pr
}
s->pps_source.destroy = ts2phc_phc_pps_source_destroy;
s->pps_source.getppstime = ts2phc_phc_pps_source_getppstime;
+ s->pps_source.get_clock = ts2phc_phc_pps_source_get_cloc
[Linuxptp-devel] [PATCH v6 00/10] Dynamic sync direction for ts2phc
e hardware actually uses. The changes should be backwards-compatible with the non-automatic mode. I have only tested non-automatic mode with a PHC master (that's all I have) and it still appears to work as before. Changes in v6: - eliminate use of offensive word "approximate" - add back int fd to struct ts2phc_clock, and use it instead of CLOCKID_TO_FD every time - use generic port_state_normalize() rather than ts2phc logic duplication - rename auto_init_ports() to ts2phc_auto_init_ports() - adapt to newly added phc_index argument to pmc_agent_query_port_properties - fixed bad patch splitting; add free(polling_array) to the patch where it is needed Changes in v5: - Renamed "master" to "PPS source" - Renamed "slave" to "PPS sink" - Renamed "source clock" to "reference clock" - Renamed "destination clock" to "target clock" - Renamed "clock" to "ts2phc_clock" - Renamed "port" to "ts2phc_port" Changes in v4: - Rebased and resolved minor conflicts - Small coding style cleanups - Added patch for perout and phase configuration - Retested Changes in v3: - Split patch "phc2sys: break out pmc code into pmc_common.c" into more trivial chunks. There is zero delta compared to previous monolithic patch, otherwise. - Replaced full license header in ts2phc.h with simple SPDX. - Added usage message and manpage for '-a' option. - Removed some useless curly braces. - Moved ts2phc_master_get_clock() from ts2phc_phc_master.c to ts2phc_master.c where it should be. - Added justification in commit message for creating struct ts2phc_private. - Reordered headers alphabetically. Changes in v2: - Added Jacob's review tags, addressed some feedback. - Dropped patch "pmc_common: fix potential memory leak in run_pmc_events()" - Reordered patches (put the tmv helpers first) - Fixed memory leaks Vladimir Oltean (10): ts2phc: create a private data structure ts2phc: instantiate a full clock structure for every PPS sink ts2phc: instantiate a full clock structure for every PPS source of the PHC kind util: import port_state_normalize() logic from phc2sys ts2phc: instantiate a pmc agent ts2phc_slave: print offset to the source clock ts2phc: split PPS sink poll from servo loop ts2phc: reconfigure sync direction by subscribing to ptp4l port events ts2phc: allow PHC PPS sources to be synchronized ts2phc_phc_pps_source: make use of new kernel API for perout waveform config.c| 1 + makefile| 5 +- missing.h | 52 +++ phc2sys.c | 18 +- ts2phc.8| 32 +- ts2phc.c| 640 ++-- ts2phc.h| 65 ts2phc_generic_pps_source.c | 2 +- ts2phc_generic_pps_source.h | 3 +- ts2phc_nmea_pps_source.c| 6 +- ts2phc_nmea_pps_source.h| 3 +- ts2phc_phc_pps_source.c | 80 +++-- ts2phc_phc_pps_source.h | 3 +- ts2phc_pps_sink.c | 381 +++-- ts2phc_pps_sink.h | 12 +- ts2phc_pps_source.c | 18 +- ts2phc_pps_source.h | 8 +- ts2phc_pps_source_private.h | 1 + util.c | 13 + util.h | 8 + 20 files changed, 1067 insertions(+), 284 deletions(-) create mode 100644 ts2phc.h -- 2.34.1 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v6 05/10] ts2phc: instantiate a pmc agent
This introduces the '-a' option in ts2phc, an option inspired from phc2sys that puts the clocks in "automatic" mode. In this mode, ts2phc listens, as a PMC, to port state change events from ptp4l, and detects which port state machine, if any, has transitioned to PS_SLAVE. That port's clock will become the synchronization reference for the clock hierarchy described by ts2phc. The use case is a multi-switch DSA setup with boundary_clock_jbod, where there is only one grandmaster, connected to one switch's port. The other switches, connected together through a PPS signal, must adapt themselves to this time reference, while the switch connected to the GM must not be synchronized by ts2phc because it is already synchronized by ptp4l. Graphically, the setup looks like this: +---+ | LS1028A /dev/ptp0 (unused) | | +--+| | | DSA master for Felix|| | |(internal ENETC port 2: eno2))|| | ++--+-+ | | | Felix embedded L2 switch /dev/ptp1 | | | | PPS source, sync target | | | | +--+ +--+ +--+ | | | | |DSA master for| |DSA master for| |DSA master for| | | | | | SJA1105 1 | | SJA1105 2 | | SJA1105 3 | | | | | |(Felix port 1)| |(Felix port 2)| |(Felix port 3)| | | +--+-+--+---+--+---+--+--+--+ /dev/ptp2/dev/ptp3/dev/ptp4 PPS sink, sync reference PPS sink, sync target PPS sink, sync target +---+ +---+ +---+ | SJA1105 switch 1| | SJA1105 switch 2| | SJA1105 switch 3| +-+-+-+-+ +-+-+-+-+ +-+-+-+-+ |sw1p0|sw1p1|sw1p2|sw1p3| |sw2p0|sw2p1|sw2p2|sw2p3| |sw3p0|sw3p1|sw3p2|sw3p3| +-+-+-+-+ +-+-+-+-+ +-+-+-+-+ ^ | GM connected here In the described state, the Felix embedded L2 switch is statically configured to emit a 1PPS signal on a sort of "simplex bus", and SJA1105 switches 1 to 3 are statically configured to record it in their respective time bases. Because the GM is connected to one of the ports of SJA1105 switch 1, that port is in PS_SLAVE and is the source of ts2phc time. So the embedded L2 switch as well as the other SJA1105 switches synchronize to the SJA1105. When the GM migrates to a port belonging to a different PHC (say sw2p0), the PPS distribution tree remains unchanged but the port reported by ptp4l as being in PS_SLAVE changes. So ts2phc must reconfigure itself to use /dev/ptp3 as time reference, and the other PHC devices turn into target clocks. Signed-off-by: Vladimir Oltean --- v5->v6: - use generic port_state_normalize() rather than ts2phc logic duplication - rename auto_init_ports() to ts2phc_auto_init_ports() - adapt to newly added phc_index argument to pmc_agent_query_port_properties v4->v5: - rebased on top of the variable renaming v3->v4: - use the new pmc agent API - add more info to the commit message - priv->state_changed can be delayed to a future patch - use enum port_state instead of int v2->v3: - Added man page and usage verbiage. - Removed some useless curly braces in ts2phc_cleanup(). makefile | 5 +- ts2phc.8 | 15 ts2phc.c | 215 ++- ts2phc.h | 12 4 files changed, 244 insertions(+), 3 deletions(-) diff --git a/makefile b/makefile index 5295b60b5dac..9aed38318803 100644 --- a/makefile +++ b/makefile @@ -68,8 +68,9 @@ phc_ctl: phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o timemaster: phc.o print.o rtnl.o sk.o timemaster.o util.o version.o -ts2phc: config.o clockadj.o hash.o interface.o phc.o print.o $(SERVOS) sk.o \ - $(TS2PHC) util.o version.o +ts2phc: config.o clockadj.o hash.o interface.o msg.o phc.o pmc_agent.o \ + pmc_common.o print.o $(SERVOS) sk.o $(TS2PHC) tlv.o transport.o raw.o \ + udp.o udp6.o uds.o util.o version.o version.o: .version version.sh $(filter-out version.d,$(DEPEND)) diff --git a/ts2phc.8 b/ts2phc.8 index 690c4627f9f2..36d56cce0270 100644 --- a/ts2phc.8 +++ b/ts2phc.8 @@ -26,6 +26,21 @@ A single source may be used to distribute time to one or more PHC devices. .SH OPTIONS .TP +.BI \-a +Adjust the direction of synchronization automatically. The program determines +which PHC should the reference clock for time distribution and which should +be the destinations by querying the port states from the running instance of +.B ptp4l. +Note that using this option, the PPS signal distribution hi
[Linuxptp-devel] [PATCH v6 00/10] Dynamic sync direction for ts2phc
e hardware actually uses. The changes should be backwards-compatible with the non-automatic mode. I have only tested non-automatic mode with a PHC master (that's all I have) and it still appears to work as before. Changes in v6: - eliminate use of offensive word "approximate" - add back int fd to struct ts2phc_clock, and use it instead of CLOCKID_TO_FD every time - use generic port_state_normalize() rather than ts2phc logic duplication - rename auto_init_ports() to ts2phc_auto_init_ports() - adapt to newly added phc_index argument to pmc_agent_query_port_properties - fixed bad patch splitting; add free(polling_array) to the patch where it is needed Changes in v5: - Renamed "master" to "PPS source" - Renamed "slave" to "PPS sink" - Renamed "source clock" to "reference clock" - Renamed "destination clock" to "target clock" - Renamed "clock" to "ts2phc_clock" - Renamed "port" to "ts2phc_port" Changes in v4: - Rebased and resolved minor conflicts - Small coding style cleanups - Added patch for perout and phase configuration - Retested Changes in v3: - Split patch "phc2sys: break out pmc code into pmc_common.c" into more trivial chunks. There is zero delta compared to previous monolithic patch, otherwise. - Replaced full license header in ts2phc.h with simple SPDX. - Added usage message and manpage for '-a' option. - Removed some useless curly braces. - Moved ts2phc_master_get_clock() from ts2phc_phc_master.c to ts2phc_master.c where it should be. - Added justification in commit message for creating struct ts2phc_private. - Reordered headers alphabetically. Changes in v2: - Added Jacob's review tags, addressed some feedback. - Dropped patch "pmc_common: fix potential memory leak in run_pmc_events()" - Reordered patches (put the tmv helpers first) - Fixed memory leaks Vladimir Oltean (10): ts2phc: create a private data structure ts2phc: instantiate a full clock structure for every PPS sink ts2phc: instantiate a full clock structure for every PPS source of the PHC kind util: import port_state_normalize() logic from phc2sys ts2phc: instantiate a pmc agent ts2phc_slave: print offset to the source clock ts2phc: split PPS sink poll from servo loop ts2phc: reconfigure sync direction by subscribing to ptp4l port events ts2phc: allow PHC PPS sources to be synchronized ts2phc_phc_pps_source: make use of new kernel API for perout waveform config.c| 1 + makefile| 5 +- missing.h | 52 +++ phc2sys.c | 18 +- ts2phc.8| 32 +- ts2phc.c| 640 ++-- ts2phc.h| 65 ts2phc_generic_pps_source.c | 2 +- ts2phc_generic_pps_source.h | 3 +- ts2phc_nmea_pps_source.c| 6 +- ts2phc_nmea_pps_source.h| 3 +- ts2phc_phc_pps_source.c | 80 +++-- ts2phc_phc_pps_source.h | 3 +- ts2phc_pps_sink.c | 381 +++-- ts2phc_pps_sink.h | 12 +- ts2phc_pps_source.c | 18 +- ts2phc_pps_source.h | 8 +- ts2phc_pps_source_private.h | 1 + util.c | 13 + util.h | 8 + 20 files changed, 1067 insertions(+), 284 deletions(-) create mode 100644 ts2phc.h -- 2.34.1 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v6 01/10] ts2phc: create a private data structure
Eliminate the ad-hoc use of global variables in the ts2phc program by
introducing one data structure that incorporates them. This might make
the code more understandable to people coming from a kernel background,
since it resembles the type of data organization used there. It is also
now closer to the data organization of phc2sys, a similar program in
both purpose and implementation.
The reason why this is needed has to do with the ts2phc polymorphism for
a PPS source.
In the next patches, PPS sources will expose a clock structure, which
will be synchronized from the main ts2phc.c.
Not all PPS sources will expose a clock, only the PHC kind will. So the
current object encapsulation model needs to be loosened up a little bit,
because the main ts2phc.c needs to synchronize a list of clocks, list
which is populated by the PPS sinks and by the subset of PPS sources
which are capable of being synchronized.
So instead of having the translation modules of ts2phc communicate
through global variables, let's make struct ts2phc_private the common
working space for the entire program, which is a paradigm that is more
natural.
Signed-off-by: Vladimir Oltean
Reviewed-by: Jacob Keller
---
v5->v6:
- moved includes in ts2phc.h above struct ts2phc_private definition
- fixed bad patch splitting; add free(polling_array) to this patch
where it is firstly needed, rather than to the next one
v4->v5:
- Rebased on top of data structure renames
v3->v4:
- Rebased, no conflicts
v3->v2:
- Amended commit message.
- Replaced full license header in ts2phc.h with simple SPDX.
ts2phc.c | 66 +++
ts2phc.h | 23
ts2phc_pps_sink.c | 134 +++---
ts2phc_pps_sink.h | 12 +++--
4 files changed, 141 insertions(+), 94 deletions(-)
create mode 100644 ts2phc.h
diff --git a/ts2phc.c b/ts2phc.c
index bc687ba93bb7..f228351bae3a 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -11,23 +11,20 @@
#include "config.h"
#include "interface.h"
#include "print.h"
-#include "ts2phc_pps_sink.h"
-#include "ts2phc_pps_source.h"
+#include "ts2phc.h"
#include "version.h"
struct interface {
STAILQ_ENTRY(interface) list;
};
-static void ts2phc_cleanup(struct config *cfg, struct ts2phc_pps_source *src)
+static void ts2phc_cleanup(struct ts2phc_private *priv)
{
- ts2phc_pps_sink_cleanup();
- if (src) {
- ts2phc_pps_source_destroy(src);
- }
- if (cfg) {
- config_destroy(cfg);
- }
+ ts2phc_pps_sink_cleanup(priv);
+ if (priv->src)
+ ts2phc_pps_source_destroy(priv->src);
+ if (priv->cfg)
+ config_destroy(priv->cfg);
}
static void usage(char *progname)
@@ -56,8 +53,8 @@ static void usage(char *progname)
int main(int argc, char *argv[])
{
int c, err = 0, have_sink = 0, index, print_level;
- struct ts2phc_pps_source *src = NULL;
enum ts2phc_pps_source_type pps_type;
+ struct ts2phc_private priv = {0};
char *config = NULL, *progname;
const char *pps_source = NULL;
struct config *cfg = NULL;
@@ -68,7 +65,7 @@ int main(int argc, char *argv[])
cfg = config_create();
if (!cfg) {
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
@@ -81,14 +78,14 @@ int main(int argc, char *argv[])
switch (c) {
case 0:
if (config_parse_option(cfg, opts[index].name, optarg))
{
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
break;
case 'c':
if (!config_create_interface(optarg, cfg)) {
fprintf(stderr, "failed to add PPS sink\n");
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
have_sink = 1;
@@ -99,7 +96,7 @@ int main(int argc, char *argv[])
case 'l':
if (get_arg_val_i(c, optarg, _level,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX)) {
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
config_set_int(cfg, "logging_level", print_level);
@@ -116,29 +113,29 @@ int main(int argc, char *argv[])
case 's':
if (pps_source) {
fprintf(stderr, "too many PPS sources\n");
-
[Linuxptp-devel] [PATCH v6 10/10] ts2phc_phc_pps_source: make use of new kernel API for perout waveform
This API was introduced for 2 reasons:
1. Some hardware can emit PPS signals but not starting from arbitrary
absolute times, but rather just emit at a certain phase offset from
the beginning of each second. We _could_ patch ts2phc to always
specify a start time of 0.0 to PTP_PEROUT_REQUEST, and in
theory that should then become the kernel's responsibility to advance
that time in the past by an integer number of seconds while keeping
the phase untouched, but in practice, we would never know whether
that would actually work with all in-kernel PHC drivers, since it
wasn't enforced as a requirement before. So there was a need for a
new flag that only specifies the phase of the periodic signal, and
not the absolute start time.
2. Some hardware can, rather unfortunately, not distinguish between a
rising and a falling extts edge. And, since whatever rises also has
to fall before rising again, the strategy in ts2phc is to set a
'large' pulse width (half the period) and ignore the extts event
corresponding to the mid-way between one second and another. This is
all fine, but currently, ts2phc.pulsewidth is a read-only property in
the config file. The kernel is not instructed in any way to use this
value, it is simply that must be configured based on prior knowledge
of the PHC's implementation. This API changes that.
The introduction of a phase adjustment for the PHC kind of PPS sources
means we have to adjust our implicit deduction of the precise perout
timestamp. We put that code into a common function and convert all call
sites to call that. We also need to do the same thing for the edge
ignoring logic.
Signed-off-by: Vladimir Oltean
---
v5->v6:
- eliminate use of offensive word "approximate"
- continue to use clock->fd rather than CLOCKID_TO_FD
v4->v5: rebase on top of variable renames
v3->v4: patch is new.
config.c| 1 +
missing.h | 52 +++
ts2phc.8| 17 +++-
ts2phc.c| 94 +++--
ts2phc.h| 1 +
ts2phc_phc_pps_source.c | 41 --
ts2phc_pps_sink.c | 16 ++-
7 files changed, 182 insertions(+), 40 deletions(-)
diff --git a/config.c b/config.c
index b5cf3970849f..e454c91ff0a1 100644
--- a/config.c
+++ b/config.c
@@ -327,6 +327,7 @@ struct config_item config_tab[] = {
GLOB_ITEM_STR("ts2phc.nmea_remote_host", ""),
GLOB_ITEM_STR("ts2phc.nmea_remote_port", ""),
GLOB_ITEM_STR("ts2phc.nmea_serialport", "/dev/ttyS0"),
+ PORT_ITEM_INT("ts2phc.perout_phase", -1, 0, 9),
PORT_ITEM_INT("ts2phc.pin_index", 0, 0, INT_MAX),
GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
diff --git a/missing.h b/missing.h
index 4c7ac57e5728..86669762046a 100644
--- a/missing.h
+++ b/missing.h
@@ -114,6 +114,58 @@ struct compat_ptp_clock_caps {
#endif /*LINUX_VERSION_CODE < 5.8*/
+/*
+ * Bits of the ptp_perout_request.flags field:
+ */
+
+#ifndef PTP_PEROUT_ONE_SHOT
+#define PTP_PEROUT_ONE_SHOT(1<<0)
+#endif
+
+#ifndef PTP_PEROUT_DUTY_CYCLE
+#define PTP_PEROUT_DUTY_CYCLE (1<<1)
+#endif
+
+#ifndef PTP_PEROUT_PHASE
+#define PTP_PEROUT_PHASE (1<<2)
+#endif
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5,9,0)
+
+struct compat_ptp_perout_request {
+ union {
+ /*
+* Absolute start time.
+* Valid only if (flags & PTP_PEROUT_PHASE) is unset.
+*/
+ struct ptp_clock_time start;
+ /*
+* Phase offset. The signal should start toggling at an
+* unspecified integer multiple of the period, plus this value.
+* The start time should be "as soon as possible".
+* Valid only if (flags & PTP_PEROUT_PHASE) is set.
+*/
+ struct ptp_clock_time phase;
+ };
+ struct ptp_clock_time period; /* Desired period, zero means disable. */
+ unsigned int index; /* Which channel to configure. */
+ unsigned int flags;
+ union {
+ /*
+* The "on" time of the signal.
+* Must be lower than the period.
+* Valid only if (flags & PTP_PEROUT_DUTY_CYCLE) is set.
+*/
+ struct ptp_clock_time on;
+ /* Reserved for future use. */
+ unsigned int rsv[4];
+ };
+};
+
+#define ptp_perout_request compat_ptp_perout_request
+
+#endif /* LINUX_VERSION_CODE < 5.9 */
+
#ifndef PTP_MAX_SAMPLES
#define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement samples. */
#endif /*
[Linuxptp-devel] [PATCH v6 06/10] ts2phc_slave: print offset to the source clock
Make this information more visible by default, since it is the key
output of this program.
Signed-off-by: Vladimir Oltean
Reviewed-by: Jacob Keller
---
v5->v6: none
v4->v5: rebase on top of the variable renames
v1->v4: none
ts2phc_pps_sink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ts2phc_pps_sink.c b/ts2phc_pps_sink.c
index ecaef8ec53ef..3df7965f5f3c 100644
--- a/ts2phc_pps_sink.c
+++ b/ts2phc_pps_sink.c
@@ -265,8 +265,8 @@ static int ts2phc_pps_sink_event(struct ts2phc_pps_sink
*sink,
adj = servo_sample(sink->clock->servo, offset, extts_ts,
SAMPLE_WEIGHT, >clock->servo_state);
- pr_debug("%s source offset %10" PRId64 " s%d freq %+7.0f",
-sink->name, offset, sink->clock->servo_state, adj);
+ pr_info("%s source offset %10" PRId64 " s%d freq %+7.0f",
+ sink->name, offset, sink->clock->servo_state, adj);
switch (sink->clock->servo_state) {
case SERVO_UNLOCKED:
--
2.34.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v5 07/13] ts2phc: instantiate a full clock structure for every PPS source of the PHC kind
On Mon, Jun 13, 2022 at 03:27:13PM -0700, Richard Cochran wrote: > On Tue, Nov 23, 2021 at 02:14:15AM +0200, Vladimir Oltean wrote: > > > diff --git a/ts2phc_phc_pps_source.c b/ts2phc_phc_pps_source.c > > index ffb96ef7299a..fca653f99499 100644 > > --- a/ts2phc_phc_pps_source.c > > +++ b/ts2phc_phc_pps_source.c > > @@ -12,15 +12,14 @@ > > #include "phc.h" > > #include "print.h" > > #include "missing.h" > > +#include "ts2phc.h" > > #include "ts2phc_pps_source_private.h" > > -#include "ts2phc_phc_pps_source.h" > > Need to keep this include in order to make sure declaration and > definition of ts2phc_phc_pps_source_create() are consistent. ts2phc.h includes ts2phc_phc_pps_source.h. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v5 08/13] ts2phc: instantiate a pmc agent
On Tue, Jun 14, 2022 at 05:58:13AM -0700, Richard Cochran wrote:
> On Tue, Nov 23, 2021 at 02:14:16AM +0200, Vladimir Oltean wrote:
>
> > +static enum port_state ts2phc_normalize_state(enum port_state state)
> > +{
> > + if (state != PS_MASTER && state != PS_SLAVE &&
> > + state != PS_PRE_MASTER && state != PS_UNCALIBRATED) {
> > + /* treat any other state as "not a master nor a slave" */
> > + state = PS_DISABLED;
> > + }
> > + return state;
> > +}
>
> Please make this explicit by using switch/case over all the enumerated values.
I'm having trouble applying this feedback.
I assume you mean something like this?
/**
* Reduce all port states for which the sync direction isn't known to
* PS_DISABLED, and report the given port state otherwise. This minimizes port
* state transitions for PMC agents when nothing interesting happened.
*/
enum port_state port_state_normalize(enum port_state state)
{
switch (state) {
case PS_MASTER:
case PS_SLAVE:
case PS_PRE_MASTER:
case PS_UNCALIBRATED:
return state;
case PS_INITIALIZING:
case PS_FAULTY:
case PS_DISABLED:
case PS_LISTENING:
case PS_PASSIVE:
case PS_GRAND_MASTER:
default:
return PS_DISABLED;
}
}
Can't I just delete "case PS_INITIALIZING:" ... "case PS_GRAND_MASTER:"
since I have to put "default:" anyway (we may get arbitrary input over
the management messages)?
Anyway, it seems silly to treat PS_GRAND_MASTER as part of this helper,
since we're not expected to receive it over management messages. It's as
if PMC agents and ptp4l should have their own separate enum port_state.
I'm also wondering if PS_PRE_MASTER and PS_UNCALIBRATED (which are
transient states) couldn't be treated as PS_DISABLED too (as an
admittedly unsolicited change).
Also, I'd like to share this helper between phc2sys and ts2phc.
Is fsm.{c,h} a good place to put it?
> > +static int auto_init_ports(struct ts2phc_private *priv)
> > +{
> > + int number_ports, timestamping, err;
> > + struct ts2phc_clock *clock;
> > + struct ts2phc_port *port;
> > + enum port_state state;
> > + char iface[IFNAMSIZ];
> > + unsigned int i;
> > +
> > + while (1) {
> > + if (!is_running())
> > + return -1;
> > + err = pmc_agent_query_dds(priv->agent, 1000);
> > + if (!err)
> > + break;
> > + if (err == -ETIMEDOUT)
> > + pr_notice("Waiting for ptp4l...");
> > + else
> > + return -1;
> > + }
> > +
> > + number_ports = pmc_agent_get_number_ports(priv->agent);
> > + if (number_ports <= 0) {
> > + pr_err("failed to get number of ports");
> > + return -1;
> > + }
> > +
> > + err = pmc_agent_subscribe(priv->agent, 1000);
> > + if (err) {
> > + pr_err("failed to subscribe");
> > + return -1;
> > + }
> > +
> > + for (i = 1; i <= number_ports; i++) {
> > + err = pmc_agent_query_port_properties(priv->agent, 1000, i,
> > + , ,
> > + iface);
> > + if (err == -ENODEV) {
> > + /* port does not exist, ignore the port */
> > + continue;
> > + }
> > + if (err) {
> > + pr_err("failed to get port properties");
> > + return -1;
> > + }
> > + if (timestamping == TS_SOFTWARE) {
> > + /* ignore ports with software time stamping */
> > + continue;
> > + }
> > + port = ts2phc_port_add(priv, i, iface);
> > + if (!port)
> > + return -1;
> > + port->state = ts2phc_normalize_state(state);
> > + }
> > + if (LIST_EMPTY(>clocks)) {
> > + pr_err("no suitable ports available");
> > + return -1;
> > + }
> > + LIST_FOREACH(clock, >clocks, list) {
> > + clock->new_state = ts2phc_clock_compute_state(priv, clock);
> > + }
> > +
> > + return 0;
> > +}
>
> This is very similar to the code in phc2sys. Can't you refactor to
> share that logic?
The problem is that, even though the logic is identical, the data
structures are not ("struct clock" vs "struct ts2phc_clock",
"struct port" vs "struct ts2phc_port").
I could probably add micro helper blocks to pmc_agent.c like
pmc_agent_wait_ptp4l() and pmc_agent_enumerate_port_properties(), the
latter having an int (*cb) for each MID_PORT_PROPERTIES_NP response.
Is that in line with what you're thinking?
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCHv3 2/8] rtnl: Add function to detect virtual clocks.
On Mon, May 16, 2022 at 09:30:22AM -0700, Richard Cochran wrote: > On Mon, May 16, 2022 at 06:19:01PM +0300, Vladimir Oltean wrote: > > > What I usually do when I need to determine whether a feature is > > available is to compile a dummy C program using the same CFLAGS as the > > main program itself. > > Well you probably love autoconf then. > > I don't. > > What I have for linuxptp is based on decades of my own very > frustrating experience in embedded. > > I am not going to change the build system, especially not compiling > test programs to find things out. That method is simply hacky. > > Sorry, > Richard No, I didn't say I love autoconf or that you should use autoconf. I just pointed out that incdefs.sh doesn't work unless you point it to the kernel headers, because it insists on grepping through files rather than just working with the provided C environment, which is what you _actually_ want. I also compile the Linux kernel so I won't set KBUILD_OUTPUT unless I want to garble my toolchain sysroot with junk. The main problem is that incdefs.sh guesses incorrectly where the header files are, if I don't point KBUILD_OUTPUT to them. It can stop doing that, and guess better, if you can just reimplement incdefs.sh using some other approach, not migrate to the full-blown autotools. I'd rather have something hacky that works and plays nice with other people's environments rather than something hacky that doesn't. But ok, maybe I'm doing something wrong. How do you set up a cross compilation environment that works for linuxptp and for the kernel? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v5 05/13] ts2phc: create a private data structure
On Sun, May 15, 2022 at 01:24:13PM -0700, Richard Cochran wrote:
> On Tue, Nov 23, 2021 at 02:14:13AM +0200, Vladimir Oltean wrote:
>
> > diff --git a/ts2phc.h b/ts2phc.h
> > new file mode 100644
> > index ..14cb2b0c21a3
> > --- /dev/null
> > +++ b/ts2phc.h
> > @@ -0,0 +1,23 @@
> > +/**
> > + * @file ts2phc.h
> > + * @brief Structure definitions for ts2phc
> > + * @note Copyright 2020 Vladimir Oltean
> > + * @note SPDX-License-Identifier: GPL-2.0+
> > + */
> > +#ifndef HAVE_TS2PHC_H
> > +#define HAVE_TS2PHC_H
> > +
> > +struct ts2phc_sink_array;
> > +
> > +struct ts2phc_private {
> > + struct ts2phc_pps_source *src;
> > + STAILQ_HEAD(sink_ifaces_head, ts2phc_pps_sink) sinks;
> > + unsigned int n_sinks;
> > + struct ts2phc_sink_array *polling_array;
> > + struct config *cfg;
> > +};
> > +
> > +#include "ts2phc_pps_source.h"
> > +#include "ts2phc_pps_sink.h"
>
> It is a bit strange to put the include directives after the struct
> definitions. After all, ts2phc_private has a pointer to a
> ts2phc_pps_source. Wonder why compiler doesn't complain about missing
> forward declaration.
>
> Anyhow, please put the include directives at the top.
Ok, I will add a forward declaration for struct ts2phc_private in
ts2phc_pps_source and ts2phc_pps_sink.
>
> > +#endif
>
> > @@ -65,49 +64,55 @@ ts2phc_pps_sink_offset(struct ts2phc_pps_sink *sink,
> >struct ts2phc_source_timestamp ts,
> >int64_t *offset, uint64_t *local_ts);
> >
> > -static STAILQ_HEAD(pps_sink_ifaces_head, ts2phc_pps_sink) ts2phc_sinks =
> > - STAILQ_HEAD_INITIALIZER(ts2phc_sinks);
> > -
> > -static unsigned int ts2phc_n_sinks;
> > -
> > -static int ts2phc_pps_sink_array_create(void)
> > +static int ts2phc_pps_sink_array_create(struct ts2phc_private *priv)
> > {
> > + struct ts2phc_sink_array *polling_array;
> > struct ts2phc_pps_sink *sink;
> > unsigned int i;
> >
> > - if (polling_array.sink) {
> > - return 0;
> > + polling_array = malloc(sizeof(*polling_array));
>
> This polling_array is never freed. Please fix it, so that there is no
> memory leak on normal program termination.
>
> (I use valgrind in pre-release testing to make sure no new memory
> issues have appeared.)
>
> Thanks,
> Richard
Bad patch splitting, sorry. I have free(polling_array) in the next patch
"ts2phc: instantiate a full clock structure for every PPS sink".
Anyway I've moved it here, will resend after retesting if there are no
other comments.
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCHv3 2/8] rtnl: Add function to detect virtual clocks.
On Tue, Mar 08, 2022 at 11:46:59AM +0100, Miroslav Lichvar wrote:
> Add a function using ethtool netlink to check whether a PHC is a virtual
> clock of an interface.
>
> Signed-off-by: Miroslav Lichvar
> Acked-by: Hangbin Liu
> ---
> incdefs.sh | 4 +++
> missing.h | 101 +
> rtnl.c | 85
> rtnl.h | 9 +
> 4 files changed, 199 insertions(+)
>
> diff --git a/incdefs.sh b/incdefs.sh
> index 19e620e..21333e5 100755
> --- a/incdefs.sh
> +++ b/incdefs.sh
> @@ -86,6 +86,10 @@ kernel_flags()
> if grep -q HWTSTAMP_TX_ONESTEP_P2P ${prefix}${tstamp}; then
> printf " -DHAVE_ONESTEP_P2P"
> fi
> +
> + if grep -q SOF_TIMESTAMPING_BIND_PHC ${prefix}${tstamp}; then
> + printf " -DHAVE_VCLOCKS"
> + fi
> }
> diff --git a/rtnl.c b/rtnl.c
> index f8bdbe6..fa02388 100644
> --- a/rtnl.c
> +++ b/rtnl.c
> @@ -19,6 +19,9 @@
> #include
> #include /* Must come before linux/netlink.h on some systems.
> */
> #include
> +#ifdef HAVE_VCLOCKS
> +#include
> +#endif
> #include
> #include
> #include
I will take this opportunity to state that incdefs.h is broken with
cross-compilation, because it wants me to set $KBUILD_OUTPUT to the
sysroot path, otherwise it searches the system-wide
/usr/include/linux/net_tstamp.h
for SOF_TIMESTAMPING_BIND_PHC, and finds it, and says "oh, yeah, I have
vclocks". Then when I go ahead and compile linuxptp, it fails to find
linux/ethtool_netlink.h, because the actual _sysroot_ doesn't have
vclock support (only the system kernel headers do).
You're probably going to say "just set KBUILD_OUTPUT", which I am indeed
forced to do. But in fact I have an environment script that I just source
for cross-compilation. And this variable isn't only used by linuxptp, it
also affects where the Linux kernel output gets compiled to.
Simply put, KBUILD_OUTPUT is _not_ the right choice to determine whether
linuxptp will be compiled with kernel headers that have a certain symbol
exported. It's also frustrating to have an env file that works for cross
compiling everything, including the kernel itself, except linuxptp.
What I usually do when I need to determine whether a feature is
available is to compile a dummy C program using the same CFLAGS as the
main program itself. No dumpster diving through paths on the host
system, depending on variables you're not supposed to, etc.
Like this:
cat toolchain_deps.sh
#!/bin/bash
CC="$1"
CFLAGS="$2"
EXTRA_CFLAGS=""
${CC} ${CFLAGS} -x c -c -o $(mktemp) - > /dev/null 2>&1 << EOF
#include
int main(void)
{
return SOF_TIMESTAMPING_BIND_PHC;
}
EOF
if [ $? = 0 ]; then
EXTRA_CFLAGS="${EXTRA_CFLAGS} -DHAVE_VCLOCKS"
fi
echo ${EXTRA_CFLAGS}
Used like this:
cat Makefile
CFLAGS += $(shell ./toolchain_deps.sh "$(CC)" "$(CFLAGS)")
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v5 00/13] Dynamic sync direction for ts2phc
Hello Richard, On Tue, Nov 23, 2021 at 02:14:08AM +0200, Vladimir Oltean wrote: > As discussed in this email thread: > https://sourceforge.net/p/linuxptp/mailman/message/37047555/ > there is a desire to synchronize multiple DSA switches in a > boundary_clock_jbod setup, using a PPS signal, and the ts2phc program > already offers a solid base. Would you mind taking another look at these patches? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v3 1/2] util: fix dangling file descriptors on the error path of posix_clock_open
When we determine that the "device" argument represents a path to a char
device, we call phc_open() and then we attempt to see, if the device
real path starts with "/dev/ptp", what number comes afterwards. If that
fails, we should close the char device we've just opened.
Fixes: 380d023abb1f ("posix_clock_open: derive PHC index from device name if
possible")
Signed-off-by: Vladimir Oltean
---
v1->v2: patch is new
v2->v3: reverse patch order
util.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/util.c b/util.c
index cc4f11008be2..50f963bf915a 100644
--- a/util.c
+++ b/util.c
@@ -218,7 +218,8 @@ clockid_t posix_clock_open(const char *device, int
*phc_index)
fprintf(stderr,
"failed to parse PHC index from %s\n",
device);
- return -1;
+ phc_close(clkid);
+ return CLOCK_INVALID;
}
}
return clkid;
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v3 2/2] util: attempt to resolve symlinks to the PHC device in posix_clock_open
Ed uses the ptp_kvm driver with phc2sys, and Debian 10 has an udev rule
which creates a symlink for this device called /dev/ptp_kvm.
/lib/udev/rules.d/50-udev-default.rules:
SUBSYSTEM=="ptp", ATTR{clock_name}=="KVM virtual PTP", SYMLINK += "ptp_kvm"
Since the blamed patch, posix_clock_open(), called from a variety of
places including the phc2sys clock_add(), returns an error which states
that it cannot deduce the PHC index from this char device symlink.
It appears that phc2sys supports, to some extent, synchronizing PTP
clocks specified by path to the char device, for which the PHC index is
not known. In fact, there are some places in phc2sys where clocks are
compared by PHC number, but almost miraculously, it manages to not trip
over the fact that the PHC number is missing.
For example:
- find_dst_clock() finds clocks by PHC index, but it is only called from
the "automatic" (phc2sys -a) code path. In that mode, phc2sys finds
the PHC through this algorithm:
- send a PORT_PROPERTIES_NP management TLV to ptp4l which contains the
port interface name as string ("eth0" etc)
- retrieve the PHC device through ethtool ioctls and compute the device
path as /dev/ptpN as discussed
So it manages to find the char device path in the reverse direction
(the PHC number is already known).
- do_loop() has this logic:
/* don't try to synchronize the clock to itself */
if (clock->clkid == priv->master->clkid ||
(clock->phc_index >= 0 &&
clock->phc_index == priv->master->phc_index) ||
!strcmp(clock->device, priv->master->device))
continue;
which again, magically manages to not trip over a comparison between
the priv->master_phc_index (CLOCK_REALTIME, has phc_index == -1) and
clock->phc_index (/dev/ptp_kvm, also has phc_index == -1), simply
because the slave clock first has to have a valid phc_index before the
comparison is even been done.
Otherwise said, for phc2sys it is an actual regression to require that
the path to the PTP char device be in the "/dev/ptpN" format. It
doesn't strictly require having a PHC index, but leaving the data
structures like that, now that we are aware of this corner case, is
prone to breakage introduced by future changes.
To fix the problem, we maintain that the PTP clock char device must
contain a number, if its path begins with /dev/ptp. But this time, we
follow any symbolic links that might exist, before attempting to deduce
the number from the path string.
Fixes: 380d023abb1f ("posix_clock_open: derive PHC index from device name if
possible")
Reported-by: Ed Branch
Signed-off-by: Vladimir Oltean
---
v1->v2: expand symlink instead of ignoring the error
v2->v3: reverse patch order
util.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/util.c b/util.c
index 50f963bf915a..34b0bc8e109c 100644
--- a/util.c
+++ b/util.c
@@ -18,6 +18,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -200,6 +201,7 @@ void posix_clock_close(clockid_t clock)
clockid_t posix_clock_open(const char *device, int *phc_index)
{
+ char phc_device_path[PATH_MAX];
struct sk_ts_info ts_info;
char phc_device[19];
int clkid;
@@ -208,22 +210,29 @@ clockid_t posix_clock_open(const char *device, int
*phc_index)
if (!strcasecmp(device, "CLOCK_REALTIME")) {
return CLOCK_REALTIME;
}
- /* 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"),
+
+ /* if the device name resolves so a plausible filesystem path, we
+* assume it is the path to a PHC char device, and treat it as such
+*/
+ if (realpath(device, phc_device_path)) {
+ clkid = phc_open(device);
+ if (clkid == CLOCK_INVALID)
+ return clkid;
+
+ if (!strncmp(phc_device_path, "/dev/ptp", strlen("/dev/ptp"))) {
+ int r = get_ranged_int(phc_device_path +
strlen("/dev/ptp"),
phc_index, 0, 65535);
if (r) {
fprintf(stderr,
"failed to parse PHC index from %s\n",
- device);
+ phc_device_path);
phc_close(clkid);
return CLOCK_INVALID;
}
}
return cl
[Linuxptp-devel] [PATCH 1/2] util: attempt to resolve symlinks to the PHC device in posix_clock_open
Ed uses the ptp_kvm driver with phc2sys, and Debian 10 has an udev rule
which creates a symlink for this device called /dev/ptp_kvm.
/lib/udev/rules.d/50-udev-default.rules:
SUBSYSTEM=="ptp", ATTR{clock_name}=="KVM virtual PTP", SYMLINK += "ptp_kvm"
Since the blamed patch, posix_clock_open(), called from a variety of
places including the phc2sys clock_add(), returns an error which states
that it cannot deduce the PHC index from this char device symlink.
It appears that phc2sys supports, to some extent, synchronizing PTP
clocks specified by path to the char device, for which the PHC index is
not known. In fact, there are some places in phc2sys where clocks are
compared by PHC number, but almost miraculously, it manages to not trip
over the fact that the PHC number is missing.
For example:
- find_dst_clock() finds clocks by PHC index, but it is only called from
the "automatic" (phc2sys -a) code path. In that mode, phc2sys finds
the PHC through this algorithm:
- send a PORT_PROPERTIES_NP management TLV to ptp4l which contains the
port interface name as string ("eth0" etc)
- retrieve the PHC device through ethtool ioctls and compute the device
path as /dev/ptpN as discussed
So it manages to find the char device path in the reverse direction
(the PHC number is already known).
- do_loop() has this logic:
/* don't try to synchronize the clock to itself */
if (clock->clkid == priv->master->clkid ||
(clock->phc_index >= 0 &&
clock->phc_index == priv->master->phc_index) ||
!strcmp(clock->device, priv->master->device))
continue;
which again, magically manages to not trip over a comparison between
the priv->master_phc_index (CLOCK_REALTIME, has phc_index == -1) and
clock->phc_index (/dev/ptp_kvm, also has phc_index == -1), simply
because the slave clock first has to have a valid phc_index before the
comparison is even been done.
Otherwise said, for phc2sys it is an actual regression to require that
the path to the PTP char device be in the "/dev/ptpN" format. It
doesn't strictly require having a PHC index, but leaving the data
structures like that, now that we are aware of this corner case, is
prone to breakage introduced by future changes.
To fix the problem, we maintain that the PTP clock char device must
contain a number, if its path begins with /dev/ptp. But this time, we
follow any symbolic links that might exist, before attempting to deduce
the number from the path string.
Fixes: 380d023abb1f ("posix_clock_open: derive PHC index from device name if
possible")
Reported-by: Ed Branch
Signed-off-by: Vladimir Oltean
---
util.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/util.c b/util.c
index cc4f11008be2..686ed5e2f110 100644
--- a/util.c
+++ b/util.c
@@ -18,6 +18,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -200,6 +201,7 @@ void posix_clock_close(clockid_t clock)
clockid_t posix_clock_open(const char *device, int *phc_index)
{
+ char phc_device_path[PATH_MAX];
struct sk_ts_info ts_info;
char phc_device[19];
int clkid;
@@ -208,21 +210,28 @@ clockid_t posix_clock_open(const char *device, int
*phc_index)
if (!strcasecmp(device, "CLOCK_REALTIME")) {
return CLOCK_REALTIME;
}
- /* 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"),
+
+ /* if the device name resolves so a plausible filesystem path, we
+* assume it is the path to a PHC char device, and treat it as such
+*/
+ if (realpath(device, phc_device_path)) {
+ clkid = phc_open(device);
+ if (clkid == CLOCK_INVALID)
+ return clkid;
+
+ if (!strncmp(phc_device_path, "/dev/ptp", strlen("/dev/ptp"))) {
+ int r = get_ranged_int(phc_device_path +
strlen("/dev/ptp"),
phc_index, 0, 65535);
if (r) {
fprintf(stderr,
"failed to parse PHC index from %s\n",
- device);
+ phc_device_path);
return -1;
}
}
return clkid;
}
+
/* check if device is a valid ethernet device */
if (sk_get_ts_info(device, _info) || !ts_info.valid) {
pr_err("unknown clock %s: %m", device);
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH 2/2] util: fix dangling file descriptors on the error path of posix_clock_open
When we determine that the "device" argument represents a path to a char
device, we call phc_open() and then we attempt to see, if the device
real path starts with "/dev/ptp", what number comes afterwards. If that
fails, we should close the char device we've just opened.
Fixes: 380d023abb1f ("posix_clock_open: derive PHC index from device name if
possible")
Signed-off-by: Vladimir Oltean
---
util.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/util.c b/util.c
index 686ed5e2f110..34b0bc8e109c 100644
--- a/util.c
+++ b/util.c
@@ -226,7 +226,8 @@ clockid_t posix_clock_open(const char *device, int
*phc_index)
fprintf(stderr,
"failed to parse PHC index from %s\n",
phc_device_path);
- return -1;
+ phc_close(clkid);
+ return CLOCK_INVALID;
}
}
return clkid;
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v5 04/13] ts2phc: rename "master" to "source"
The ts2phc program will introduce clock synchronization which is
orthogonal to the direction in which PPS is emitted (from the master
to the slave).
To have a more consistent terminology, we can avoid using the
ultra-generic term "master" and replace it with "PPS source", which
describes the role of the data structure in the new interpretation of
the program in a much clearer way.
Signed-off-by: Vladimir Oltean
---
v4->v5: patch is new
ts2phc.c| 56 +-
ts2phc_generic_pps_source.c | 32 +-
ts2phc_generic_pps_source.h | 8 +--
ts2phc_nmea_pps_source.c| 113 ++--
ts2phc_nmea_pps_source.h| 8 +--
ts2phc_phc_pps_source.c | 66 ++---
ts2phc_phc_pps_source.h | 8 +--
ts2phc_pps_sink.c | 10 ++--
ts2phc_pps_sink.h | 2 +-
ts2phc_pps_source.c | 30 +-
ts2phc_pps_source.h | 36 ++--
ts2phc_pps_source_private.h | 10 ++--
12 files changed, 190 insertions(+), 189 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index 812ac78b4875..6004e04effdd 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -19,11 +19,11 @@ struct interface {
STAILQ_ENTRY(interface) list;
};
-static void ts2phc_cleanup(struct config *cfg, struct ts2phc_master *master)
+static void ts2phc_cleanup(struct config *cfg, struct ts2phc_pps_source *src)
{
ts2phc_pps_sink_cleanup();
- if (master) {
- ts2phc_master_destroy(master);
+ if (src) {
+ ts2phc_pps_source_destroy(src);
}
if (cfg) {
config_destroy(cfg);
@@ -56,8 +56,8 @@ static void usage(char *progname)
int main(int argc, char *argv[])
{
int c, err = 0, have_sink = 0, index, print_level;
- struct ts2phc_master *master = NULL;
- enum ts2phc_master_type pps_type;
+ struct ts2phc_pps_source *src = NULL;
+ enum ts2phc_pps_source_type pps_type;
char *config = NULL, *progname;
const char *pps_source = NULL;
struct config *cfg = NULL;
@@ -68,7 +68,7 @@ int main(int argc, char *argv[])
cfg = config_create();
if (!cfg) {
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup(cfg, src);
return -1;
}
@@ -81,14 +81,14 @@ int main(int argc, char *argv[])
switch (c) {
case 0:
if (config_parse_option(cfg, opts[index].name, optarg))
{
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup(cfg, src);
return -1;
}
break;
case 'c':
if (!config_create_interface(optarg, cfg)) {
fprintf(stderr, "failed to add PPS sink\n");
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup(cfg, src);
return -1;
}
have_sink = 1;
@@ -99,7 +99,7 @@ int main(int argc, char *argv[])
case 'l':
if (get_arg_val_i(c, optarg, _level,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX)) {
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup(cfg, src);
return -1;
}
config_set_int(cfg, "logging_level", print_level);
@@ -116,29 +116,29 @@ int main(int argc, char *argv[])
case 's':
if (pps_source) {
fprintf(stderr, "too many PPS sources\n");
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup(cfg, src);
return -1;
}
pps_source = optarg;
break;
case 'v':
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup(cfg, src);
version_show(stdout);
return 0;
case 'h':
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup(cfg, src);
usage(progname);
return -1;
case '?':
default:
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup(cfg, src);
usage(progname);
return -1;
}
}
if (config && (c = config_read(config, cfg))) {
fprintf(stderr, "failed to read config\n");
- ts2phc_cleanup(cfg, maste
[Linuxptp-devel] [PATCH v5 07/13] ts2phc: instantiate a full clock structure for every PPS source of the PHC kind
This propagates the use of "struct ts2phc_private" all the way into the
PPS source API, in preparation of a new use case that will be supported
soon: some PPS sources (to be precise, the "PHC" kind) instantiate a
struct clock which could be disciplined by ts2phc.
When a PHC A emits a pulse and another PHC B timestamps it, the offset
between their precise timestamps can be used to synchronize either one
of them. So far in ts2phc, only the sink PHC (the one using extts) has
been synchronized to the source (the one using perout).
This is partly because there is no proper kernel API to report the
precise timestamp of a perout pulse. We only have the periodic API, and
that doesn't report precise timestamps either; we just use vague
approximations of what the PPS source PHC's time was, based on reading
that PHC immediately after a sink extts event was received by the
application. While this is far from ideal, it does work, and does allow
PHC A to be synchronized to B.
This is particularly useful with the yet-to-be-introduced "automatic"
mode of ts2phc (similar to '-a' of phc2sys), and the PPS distribution
tree is fixed in hardware (as opposed to port states, which in
"automatic" mode are dynamic, as the name suggests).
Signed-off-by: Vladimir Oltean
---
ts2phc.c| 2 +-
ts2phc.h| 1 +
ts2phc_generic_pps_source.c | 2 +-
ts2phc_generic_pps_source.h | 3 ++-
ts2phc_nmea_pps_source.c| 6 +++---
ts2phc_nmea_pps_source.h| 3 ++-
ts2phc_phc_pps_source.c | 32
ts2phc_phc_pps_source.h | 3 ++-
ts2phc_pps_sink.c | 2 +-
ts2phc_pps_source.c | 10 ++
ts2phc_pps_source.h | 6 --
11 files changed, 39 insertions(+), 31 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index f63e5503637d..64472a4bb15c 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -270,7 +270,7 @@ int main(int argc, char *argv[])
} else {
pps_type = TS2PHC_PPS_SOURCE_PHC;
}
- priv.src = ts2phc_pps_source_create(cfg, pps_source, pps_type);
+ priv.src = ts2phc_pps_source_create(, pps_source, pps_type);
if (!priv.src) {
fprintf(stderr, "failed to create PPS source\n");
ts2phc_cleanup();
diff --git a/ts2phc.h b/ts2phc.h
index 043215a51609..a633492b5306 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -7,6 +7,7 @@
#ifndef HAVE_TS2PHC_H
#define HAVE_TS2PHC_H
+#include
#include
#include
#include "servo.h"
diff --git a/ts2phc_generic_pps_source.c b/ts2phc_generic_pps_source.c
index 397031fe2521..6842d8e0e30a 100644
--- a/ts2phc_generic_pps_source.c
+++ b/ts2phc_generic_pps_source.c
@@ -47,7 +47,7 @@ static int ts2phc_generic_pps_source_getppstime(struct
ts2phc_pps_source *src,
return 0;
}
-struct ts2phc_pps_source *ts2phc_generic_pps_source_create(struct config *cfg,
+struct ts2phc_pps_source *ts2phc_generic_pps_source_create(struct
ts2phc_private *priv,
const char *dev)
{
struct ts2phc_generic_pps_source *src;
diff --git a/ts2phc_generic_pps_source.h b/ts2phc_generic_pps_source.h
index e169ab380ccf..bed62740059e 100644
--- a/ts2phc_generic_pps_source.h
+++ b/ts2phc_generic_pps_source.h
@@ -6,9 +6,10 @@
#ifndef HAVE_TS2PHC_GENERIC_PPS_SOURCE_H
#define HAVE_TS2PHC_GENERIC_PPS_SOURCE_H
+#include "ts2phc.h"
#include "ts2phc_pps_source.h"
-struct ts2phc_pps_source *ts2phc_generic_pps_source_create(struct config *cfg,
+struct ts2phc_pps_source *ts2phc_generic_pps_source_create(struct
ts2phc_private *priv,
const char *dev);
#endif
diff --git a/ts2phc_nmea_pps_source.c b/ts2phc_nmea_pps_source.c
index 52a96955f3ce..23aeb3840f40 100644
--- a/ts2phc_nmea_pps_source.c
+++ b/ts2phc_nmea_pps_source.c
@@ -249,7 +249,7 @@ static int ts2phc_nmea_pps_source_getppstime(struct
ts2phc_pps_source *src,
return lstab_error;
}
-struct ts2phc_pps_source *ts2phc_nmea_pps_source_create(struct config *cfg,
+struct ts2phc_pps_source *ts2phc_nmea_pps_source_create(struct ts2phc_private
*priv,
const char *dev)
{
struct ts2phc_nmea_pps_source *s;
@@ -260,7 +260,7 @@ struct ts2phc_pps_source
*ts2phc_nmea_pps_source_create(struct config *cfg,
if (!s) {
return NULL;
}
- s->leapfile = config_get_string(cfg, NULL, "leapfile");
+ s->leapfile = config_get_string(priv->cfg, NULL, "leapfile");
s->lstab = lstab_create(s->leapfile);
if (!s->lstab) {
free(s);
@@ -277,7 +277,7 @@ struct ts2phc_pps_source
*ts2phc_nmea_pps_source_create(struct config *cfg,
}
s->pps_source.destroy = ts2phc_nmea_pps_source_destroy;
s->pps_source.getppstime = ts2phc
[Linuxptp-devel] [PATCH v5 02/13] ts2phc: rename source code files ("master" to "source", "slave" to "sink")
At the moment, a clock which emits PPS is not synchronizable, and
therefore called "master", and a clock which timestamps PPS is
synchronizable, and therefore called "slave".
The ts2phc program is preparing to decouple the concept of clock
synchronization from the concept of who emits PPS and who timestamps
PPS.
When that will happen, the "master" and "slave" terminology will become
vague. Rename what is today a "master" to a "PPS source" and a "slave"
to a "PPS sink". Temporarily, the clock representing the time reference
still coincides with the "PPS source", and the target clocks for
synchronization still coincide with the "PPS sinks".
The change only renames files and their occurrences in the source code
(include paths and makefile). No functional change intended.
Suggested-by: Richard Cochran
Signed-off-by: Vladimir Oltean
---
v4->v5: patch is new
makefile | 4 ++--
ts2phc.c | 4 ++--
ts2phc_generic_master.c => ts2phc_generic_pps_source.c | 4 ++--
ts2phc_generic_master.h => ts2phc_generic_pps_source.h | 4 ++--
ts2phc_nmea_master.c => ts2phc_nmea_pps_source.c | 6 +++---
ts2phc_nmea_master.h => ts2phc_nmea_pps_source.h | 4 ++--
ts2phc_phc_master.c => ts2phc_phc_pps_source.c | 6 +++---
ts2phc_phc_master.h => ts2phc_phc_pps_source.h | 4 ++--
ts2phc_slave.c => ts2phc_pps_sink.c| 4 ++--
ts2phc_slave.h => ts2phc_pps_sink.h| 4 ++--
ts2phc_master.c => ts2phc_pps_source.c | 8
ts2phc_master.h => ts2phc_pps_source.h | 2 +-
ts2phc_master_private.h => ts2phc_pps_source_private.h | 4 ++--
13 files changed, 29 insertions(+), 29 deletions(-)
rename ts2phc_generic_master.c => ts2phc_generic_pps_source.c (94%)
rename ts2phc_generic_master.h => ts2phc_generic_pps_source.h (81%)
rename ts2phc_nmea_master.c => ts2phc_nmea_pps_source.c (98%)
rename ts2phc_nmea_master.h => ts2phc_nmea_pps_source.h (81%)
rename ts2phc_phc_master.c => ts2phc_phc_pps_source.c (96%)
rename ts2phc_phc_master.h => ts2phc_phc_pps_source.h (82%)
rename ts2phc_slave.c => ts2phc_pps_sink.c (99%)
rename ts2phc_slave.h => ts2phc_pps_sink.h (88%)
rename ts2phc_master.c => ts2phc_pps_source.c (84%)
rename ts2phc_master.h => ts2phc_pps_source.h (97%)
rename ts2phc_master_private.h => ts2phc_pps_source_private.h (86%)
diff --git a/makefile b/makefile
index 33e7ca0f038b..5295b60b5dac 100644
--- a/makefile
+++ b/makefile
@@ -26,8 +26,8 @@ PRG = ptp4l hwstamp_ctl nsm phc2sys phc_ctl pmc timemaster
ts2phc
FILTERS= filter.o mave.o mmedian.o
SERVOS = linreg.o ntpshm.o nullf.o pi.o servo.o
TRANSP = raw.o transport.o udp.o udp6.o uds.o
-TS2PHC = ts2phc.o lstab.o nmea.o serial.o sock.o ts2phc_generic_master.o \
- ts2phc_master.o ts2phc_phc_master.o ts2phc_nmea_master.o ts2phc_slave.o
+TS2PHC = ts2phc.o lstab.o nmea.o serial.o sock.o ts2phc_generic_pps_source.o \
+ ts2phc_nmea_pps_source.o ts2phc_phc_pps_source.o ts2phc_pps_sink.o
ts2phc_pps_source.o
OBJ= bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
e2e_tc.o fault.o $(FILTERS) fsm.o hash.o interface.o monitor.o msg.o phc.o \
port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) \
diff --git a/ts2phc.c b/ts2phc.c
index bc410415dfc6..8f2039085812 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -11,8 +11,8 @@
#include "config.h"
#include "interface.h"
#include "print.h"
-#include "ts2phc_master.h"
-#include "ts2phc_slave.h"
+#include "ts2phc_pps_source.h"
+#include "ts2phc_pps_sink.h"
#include "version.h"
struct interface {
diff --git a/ts2phc_generic_master.c b/ts2phc_generic_pps_source.c
similarity index 94%
rename from ts2phc_generic_master.c
rename to ts2phc_generic_pps_source.c
index ad4f7f1cf1d7..0afea8ec1594 100644
--- a/ts2phc_generic_master.c
+++ b/ts2phc_generic_pps_source.c
@@ -8,8 +8,8 @@
#include "missing.h"
#include "print.h"
-#include "ts2phc_generic_master.h"
-#include "ts2phc_master_private.h"
+#include "ts2phc_generic_pps_source.h"
+#include "ts2phc_pps_source_private.h"
#include "util.h"
struct ts2phc_generic_master {
diff --git a/ts2phc_generic_master.h b/ts2phc_generic_pps_source.h
similarity index 81%
rename from ts2phc_generic_master.h
rename to ts2phc_generic_pps_source.h
index ac0ce4f11cb8..94ab29b30bcb 100644
--- a/ts2phc_generic_master.h
+++ b/ts2phc_generic_pps_source.h
@@ -1,12 +1,12 @@
/**
- * @file ts2phc_generic_master.h
+ * @file ts2phc_generic_pps_source.h
* @note Copyright (C) 2019 Richard Cochran
* @note SPDX-License-Identifier: GPL-2.0+
*/
#ifndef HAVE_TS2PHC_G
[Linuxptp-devel] [PATCH v5 11/13] ts2phc: reconfigure sync direction by subscribing to ptp4l port events
Monitor the port state change events from ptp4l, and use that
information to determine the "reference" clock.
Then synchronize all other clocks in our list to that source, by feeding
into their respective servo loop an offset equal to the delta between
their timestamp and the timestamp of the reference clock. All timestamps
are representative of the same event, which is the most recent perout
pulse of the PPS source.
Signed-off-by: Vladimir Oltean
---
v4->v5:
- Rename source clock to reference clock, destination clock to target
clock
- Rebase on top of earlier data structure renaming (master -> PPS
source, slave -> PPS sink)
v3->v4:
- Use bool for boolean types.
v2->v3:
- None.
ts2phc.c| 130
ts2phc.h| 3 +
ts2phc_phc_pps_source.c | 1 +
ts2phc_pps_sink.c | 1 +
4 files changed, 123 insertions(+), 12 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index f84950947712..0968ef28ca73 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -123,6 +123,7 @@ static int ts2phc_recv_subscribed(void *context, struct
ptp_message *msg,
state = ts2phc_clock_compute_state(priv, clock);
if (clock->state != state || clock->new_state) {
clock->new_state = state;
+ priv->state_changed = true;
}
}
return 1;
@@ -326,33 +327,126 @@ static int auto_init_ports(struct ts2phc_private *priv)
LIST_FOREACH(clock, >clocks, list) {
clock->new_state = ts2phc_clock_compute_state(priv, clock);
}
+ priv->state_changed = true;
return 0;
}
-static void ts2phc_synchronize_clocks(struct ts2phc_private *priv)
+static void ts2phc_reconfigure(struct ts2phc_private *priv)
+{
+ struct ts2phc_clock *c, *ref_clk = NULL, *last = NULL;
+ int num_ref_clocks = 0, num_target_clocks = 0;
+
+ pr_info("reconfiguring after port state change");
+ priv->state_changed = false;
+
+ LIST_FOREACH(c, >clocks, list) {
+ if (c->new_state) {
+ c->state = c->new_state;
+ c->new_state = 0;
+ }
+
+ switch (c->state) {
+ case PS_FAULTY:
+ case PS_DISABLED:
+ case PS_LISTENING:
+ case PS_PRE_MASTER:
+ case PS_MASTER:
+ case PS_PASSIVE:
+ if (!c->is_target) {
+ pr_info("selecting %s for synchronization",
+ c->name);
+ c->is_target = true;
+ }
+ num_target_clocks++;
+ break;
+ case PS_UNCALIBRATED:
+ num_ref_clocks++;
+ break;
+ case PS_SLAVE:
+ ref_clk = c;
+ num_ref_clocks++;
+ break;
+ default:
+ break;
+ }
+ last = c;
+ }
+ if (num_target_clocks >= 1 && !ref_clk) {
+ priv->ref_clock = last;
+ priv->ref_clock->is_target = false;
+ /* Reset to original state in next reconfiguration. */
+ priv->ref_clock->new_state = priv->ref_clock->state;
+ priv->ref_clock->state = PS_SLAVE;
+ pr_info("no reference clock, selecting %s by default",
+ last->name);
+ return;
+ }
+ if (num_ref_clocks > 1) {
+ pr_info("multiple reference clocks available, postponing
sync...");
+ priv->ref_clock = NULL;
+ return;
+ }
+ if (num_ref_clocks > 0 && !ref_clk) {
+ pr_info("reference clock not ready, waiting...");
+ priv->ref_clock = NULL;
+ return;
+ }
+ if (!num_ref_clocks && !num_target_clocks) {
+ pr_info("no PHC ready, waiting...");
+ priv->ref_clock = NULL;
+ return;
+ }
+ if (!num_ref_clocks) {
+ pr_info("nothing to synchronize");
+ priv->ref_clock = NULL;
+ return;
+ }
+ ref_clk->is_target = false;
+ priv->ref_clock = ref_clk;
+ pr_info("selecting %s as the reference clock", ref_clk->name);
+}
+
+static void ts2phc_synchronize_clocks(struct ts2phc_private *priv, int autocfg)
{
- struct timespec source_ts;
tmv_t source_tmv;
struct ts2phc_clock *c;
int valid, err;
-
[Linuxptp-devel] [PATCH v5 03/13] ts2phc: rename "slave clocks" to "PPS sinks"
The ts2phc program will introduce clock synchronization which is
orthogonal to the direction in which PPS is emitted (from the master
to the slave).
To have a more consistent terminology, we can avoid using the
ultra-generic term "slave" and replace it with "PPS sink", which
describes the role of the data structure in the new interpretation of
the program in a much clearer way.
Signed-off-by: Vladimir Oltean
---
v4->v5: patch is new
ts2phc.c | 24 ++---
ts2phc_pps_sink.c | 255 +++---
ts2phc_pps_sink.h | 12 +--
3 files changed, 146 insertions(+), 145 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index 8f2039085812..812ac78b4875 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -21,7 +21,7 @@ struct interface {
static void ts2phc_cleanup(struct config *cfg, struct ts2phc_master *master)
{
- ts2phc_slave_cleanup();
+ ts2phc_pps_sink_cleanup();
if (master) {
ts2phc_master_destroy(master);
}
@@ -55,7 +55,7 @@ static void usage(char *progname)
int main(int argc, char *argv[])
{
- int c, err = 0, have_slave = 0, index, print_level;
+ int c, err = 0, have_sink = 0, index, print_level;
struct ts2phc_master *master = NULL;
enum ts2phc_master_type pps_type;
char *config = NULL, *progname;
@@ -87,11 +87,11 @@ int main(int argc, char *argv[])
break;
case 'c':
if (!config_create_interface(optarg, cfg)) {
- fprintf(stderr, "failed to add slave\n");
+ fprintf(stderr, "failed to add PPS sink\n");
ts2phc_cleanup(cfg, master);
return -1;
}
- have_slave = 1;
+ have_sink = 1;
break;
case 'f':
config = optarg;
@@ -156,16 +156,16 @@ int main(int argc, char *argv[])
}
pps_source = interface_name(iface);
} else {
- if (ts2phc_slave_add(cfg, interface_name(iface))) {
- fprintf(stderr, "failed to add slave\n");
+ if (ts2phc_pps_sink_add(cfg, interface_name(iface))) {
+ fprintf(stderr, "failed to add PPS sink\n");
ts2phc_cleanup(cfg, master);
return -1;
}
- have_slave = 1;
+ have_sink = 1;
}
}
- if (!have_slave) {
- fprintf(stderr, "no slave clocks specified\n");
+ if (!have_sink) {
+ fprintf(stderr, "no PPS sinks specified\n");
ts2phc_cleanup(cfg, master);
usage(progname);
return -1;
@@ -176,8 +176,8 @@ int main(int argc, char *argv[])
usage(progname);
return -1;
}
- if (ts2phc_slave_arm()) {
- fprintf(stderr, "failed to arm slaves\n");
+ if (ts2phc_pps_sink_arm()) {
+ fprintf(stderr, "failed to arm PPS sinks\n");
ts2phc_cleanup(cfg, master);
return -1;
}
@@ -197,7 +197,7 @@ int main(int argc, char *argv[])
}
while (is_running()) {
- err = ts2phc_slave_poll(master);
+ err = ts2phc_pps_sink_poll(master);
if (err) {
pr_err("poll failed");
break;
diff --git a/ts2phc_pps_sink.c b/ts2phc_pps_sink.c
index a2245e62d58c..7da850c718eb 100644
--- a/ts2phc_pps_sink.c
+++ b/ts2phc_pps_sink.c
@@ -1,5 +1,5 @@
/**
- * @file ts2phc_slave.c
+ * @file ts2phc_pps_sink.c
* @brief Utility program to synchronize the PHC clock to external events
* @note Copyright (C) 2019 Balint Ferencz
* @note SPDX-License-Identifier: GPL-2.0+
@@ -29,9 +29,9 @@
#define SAMPLE_WEIGHT 1.0
#define SERVO_SYNC_INTERVAL1.0
-struct ts2phc_slave {
+struct ts2phc_pps_sink {
char *name;
- STAILQ_ENTRY(ts2phc_slave) list;
+ STAILQ_ENTRY(ts2phc_pps_sink) list;
struct ptp_pin_desc pin_desc;
enum servo_state state;
unsigned int polarity;
@@ -44,8 +44,8 @@ struct ts2phc_slave {
int fd;
};
-struct ts2phc_slave_array {
- struct ts2phc_slave **slave;
+struct ts2phc_sink_array {
+ struct ts2phc_pps_sink **sink;
struct pollfd *pfd;
} polling_array;
@@ -60,61 +60,61 @@ enum extts_result {
EXTTS_IGNORE= 1,
};
-static enum extts_result ts2phc_slave_offset(struct ts2phc_slave *slave,
-struct ts2phc_source_timestamp ts,
-
[Linuxptp-devel] [PATCH v5 05/13] ts2phc: create a private data structure
Eliminate the ad-hoc use of global variables in the ts2phc program by
introducing one data structure that incorporates them. This might make
the code more understandable to people coming from a kernel background,
since it resembles the type of data organization used there. It is also
now closer to the data organization of phc2sys, a similar program in
both purpose and implementation.
The reason why this is needed has to do with the ts2phc polymorphism for
a PPS source.
In the next patches, PPS sources will expose a clock structure, which
will be synchronized from the main ts2phc.c.
Not all PPS sources will expose a clock, only the PHC kind will. So the
current object encapsulation model needs to be loosened up a little bit,
because the main ts2phc.c needs to synchronize a list of clocks, list
which is populated by the PPS sinks and by the subset of PPS sources
which are capable of being synchronized.
So instead of having the translation modules of ts2phc communicate
through global variables, let's make struct ts2phc_private the common
working space for the entire program, which is a paradigm that is more
natural.
Signed-off-by: Vladimir Oltean
Reviewed-by: Jacob Keller
---
v4->v5:
- Rebased on top of data structure renames
v3->v4:
- Rebased, no conflicts
v3->v2:
- Amended commit message.
- Replaced full license header in ts2phc.h with simple SPDX.
ts2phc.c | 66 +--
ts2phc.h | 23 ++
ts2phc_pps_sink.c | 113 ++
ts2phc_pps_sink.h | 10 ++--
4 files changed, 125 insertions(+), 87 deletions(-)
create mode 100644 ts2phc.h
diff --git a/ts2phc.c b/ts2phc.c
index 6004e04effdd..f228351bae3a 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -11,23 +11,20 @@
#include "config.h"
#include "interface.h"
#include "print.h"
-#include "ts2phc_pps_source.h"
-#include "ts2phc_pps_sink.h"
+#include "ts2phc.h"
#include "version.h"
struct interface {
STAILQ_ENTRY(interface) list;
};
-static void ts2phc_cleanup(struct config *cfg, struct ts2phc_pps_source *src)
+static void ts2phc_cleanup(struct ts2phc_private *priv)
{
- ts2phc_pps_sink_cleanup();
- if (src) {
- ts2phc_pps_source_destroy(src);
- }
- if (cfg) {
- config_destroy(cfg);
- }
+ ts2phc_pps_sink_cleanup(priv);
+ if (priv->src)
+ ts2phc_pps_source_destroy(priv->src);
+ if (priv->cfg)
+ config_destroy(priv->cfg);
}
static void usage(char *progname)
@@ -56,8 +53,8 @@ static void usage(char *progname)
int main(int argc, char *argv[])
{
int c, err = 0, have_sink = 0, index, print_level;
- struct ts2phc_pps_source *src = NULL;
enum ts2phc_pps_source_type pps_type;
+ struct ts2phc_private priv = {0};
char *config = NULL, *progname;
const char *pps_source = NULL;
struct config *cfg = NULL;
@@ -68,7 +65,7 @@ int main(int argc, char *argv[])
cfg = config_create();
if (!cfg) {
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
@@ -81,14 +78,14 @@ int main(int argc, char *argv[])
switch (c) {
case 0:
if (config_parse_option(cfg, opts[index].name, optarg))
{
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
break;
case 'c':
if (!config_create_interface(optarg, cfg)) {
fprintf(stderr, "failed to add PPS sink\n");
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
have_sink = 1;
@@ -99,7 +96,7 @@ int main(int argc, char *argv[])
case 'l':
if (get_arg_val_i(c, optarg, _level,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX)) {
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
config_set_int(cfg, "logging_level", print_level);
@@ -116,29 +113,29 @@ int main(int argc, char *argv[])
case 's':
if (pps_source) {
fprintf(stderr, "too many PPS sources\n");
- ts2phc_cleanup(cfg, src);
+ ts2phc_cleanup();
return -1;
}
pps_source = optarg;
[Linuxptp-devel] [PATCH v5 06/13] ts2phc: instantiate a full clock structure for every PPS sink
Currently in ts2phc, PPS sinks are also the only candidates for the
clocks that get synchronized. There are 2 aspects that make this too
restrictive:
- Not all PPS sinks may be target clocks for synchronization. Consider a
dynamic environment of multiple DSA switches using boundary_clock_jbod,
and only one port is in the PS_SLAVE state. In that case, the clock of
that port should be the reference for synchronization, regardless of
whether it supports the extts API or not.
- Not all target clocks for synchronization may be PPS sinks.
Specifically, the "PHC" type of PPS master in ts2phc can also be,
fundamentally, treated as a target clock, and disciplined.
The code base should be prepared to handle the distinction that exists
between these concepts, by recognizing that things that can be
disciplined by a servo should be represented by a "struct ts2phc_clock",
and things that can timestamp external events should be represented by a
"struct ts2phc_pps_sink".
Signed-off-by: Vladimir Oltean
---
v4->v5:
- Rebased on top of data structure renames
v3->v4:
- rebased, needed to pass priv->cfg in ts2phc_nmea_master_create
- setting clock->is_destination can be delayed to a future patch
v2->v3:
None.
ts2phc.c | 81 ++
ts2phc.h | 21 +++
ts2phc_pps_sink.c | 90 ++-
3 files changed, 137 insertions(+), 55 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index f228351bae3a..f63e5503637d 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -6,10 +6,15 @@
* @note Copyright (C) 2012 Richard Cochran
* @note SPDX-License-Identifier: GPL-2.0+
*/
+#include
#include
+#include
+#include
+#include "clockadj.h"
#include "config.h"
#include "interface.h"
+#include "phc.h"
#include "print.h"
#include "ts2phc.h"
#include "version.h"
@@ -27,6 +32,82 @@ static void ts2phc_cleanup(struct ts2phc_private *priv)
config_destroy(priv->cfg);
}
+static struct servo *ts2phc_servo_create(struct ts2phc_private *priv,
+struct ts2phc_clock *clock)
+{
+ enum servo_type type = config_get_int(priv->cfg, NULL, "clock_servo");
+ struct servo *servo;
+ int fadj, max_adj;
+
+ fadj = (int) clockadj_get_freq(clock->clkid);
+ /* Due to a bug in older kernels, the reading may silently fail
+* and return 0. Set the frequency back to make sure fadj is
+* the actual frequency of the clock.
+*/
+ if (!clock->no_adj) {
+ clockadj_set_freq(clock->clkid, fadj);
+ }
+
+ max_adj = phc_max_adj(clock->clkid);
+
+ servo = servo_create(priv->cfg, type, -fadj, max_adj, 0);
+ if (!servo)
+ return NULL;
+
+ servo_sync_interval(servo, SERVO_SYNC_INTERVAL);
+
+ return servo;
+}
+
+struct ts2phc_clock *ts2phc_clock_add(struct ts2phc_private *priv,
+ const char *device)
+{
+ clockid_t clkid = CLOCK_INVALID;
+ struct ts2phc_clock *c;
+ int phc_index = -1;
+ int err;
+
+ clkid = posix_clock_open(device, _index);
+ if (clkid == CLOCK_INVALID)
+ return NULL;
+
+ LIST_FOREACH(c, >clocks, list) {
+ if (c->phc_index == phc_index) {
+ /* Already have the clock, don't add it again */
+ posix_clock_close(clkid);
+ return c;
+ }
+ }
+
+ c = calloc(1, sizeof(*c));
+ if (!c) {
+ pr_err("failed to allocate memory for a clock");
+ return NULL;
+ }
+ c->clkid = clkid;
+ c->phc_index = phc_index;
+ c->servo_state = SERVO_UNLOCKED;
+ c->servo = ts2phc_servo_create(priv, c);
+ c->no_adj = config_get_int(priv->cfg, NULL, "free_running");
+ err = asprintf(>name, "/dev/ptp%d", phc_index);
+ if (err < 0) {
+ free(c);
+ posix_clock_close(clkid);
+ return NULL;
+ }
+
+ LIST_INSERT_HEAD(>clocks, c, list);
+ return c;
+}
+
+void ts2phc_clock_destroy(struct ts2phc_clock *c)
+{
+ servo_destroy(c->servo);
+ posix_clock_close(c->clkid);
+ free(c->name);
+ free(c);
+}
+
static void usage(char *progname)
{
fprintf(stderr,
diff --git a/ts2phc.h b/ts2phc.h
index 14cb2b0c21a3..043215a51609 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -7,16 +7,37 @@
#ifndef HAVE_TS2PHC_H
#define HAVE_TS2PHC_H
+#include
+#include
+#include "servo.h"
+
struct ts2phc_sink_array;
+#define SERVO_SYNC_INTERVAL1.0
+
+struct ts2phc_clock {
+ LIST_ENTRY(ts2phc_clock) list;
+ clockid_t clkid;
+ int phc_index;
+
[Linuxptp-devel] [PATCH v5 09/13] ts2phc_slave: print offset to the source clock
Make this information more visible by default, since it is the key
output of this program.
Signed-off-by: Vladimir Oltean
Reviewed-by: Jacob Keller
---
v4->v5: rebase on top of the variable renames
v1->v4: none
ts2phc_pps_sink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ts2phc_pps_sink.c b/ts2phc_pps_sink.c
index 729aac8f570f..69cc97179e36 100644
--- a/ts2phc_pps_sink.c
+++ b/ts2phc_pps_sink.c
@@ -260,8 +260,8 @@ static int ts2phc_pps_sink_event(struct ts2phc_pps_sink
*sink,
adj = servo_sample(sink->clock->servo, offset, extts_ts,
SAMPLE_WEIGHT, >clock->servo_state);
- pr_debug("%s source offset %10" PRId64 " s%d freq %+7.0f",
-sink->name, offset, sink->clock->servo_state, adj);
+ pr_info("%s source offset %10" PRId64 " s%d freq %+7.0f",
+ sink->name, offset, sink->clock->servo_state, adj);
switch (sink->clock->servo_state) {
case SERVO_UNLOCKED:
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v5 00/13] Dynamic sync direction for ts2phc
e
hardware actually uses.
The changes should be backwards-compatible with the non-automatic mode.
I have only tested non-automatic mode with a PHC master (that's all I
have) and it still appears to work as before.
Changes in v5:
- Renamed "master" to "PPS source"
- Renamed "slave" to "PPS sink"
- Renamed "source clock" to "reference clock"
- Renamed "destination clock" to "target clock"
- Renamed "clock" to "ts2phc_clock"
- Renamed "port" to "ts2phc_port"
Changes in v4:
- Rebased and resolved minor conflicts
- Small coding style cleanups
- Added patch for perout and phase configuration
- Retested
Changes in v3:
- Split patch "phc2sys: break out pmc code into pmc_common.c" into more
trivial chunks. There is zero delta compared to previous monolithic
patch, otherwise.
- Replaced full license header in ts2phc.h with simple SPDX.
- Added usage message and manpage for '-a' option.
- Removed some useless curly braces.
- Moved ts2phc_master_get_clock() from ts2phc_phc_master.c to
ts2phc_master.c where it should be.
- Added justification in commit message for creating struct
ts2phc_private.
- Reordered headers alphabetically.
Changes in v2:
- Added Jacob's review tags, addressed some feedback.
- Dropped patch "pmc_common: fix potential memory leak in
run_pmc_events()"
- Reordered patches (put the tmv helpers first)
- Fixed memory leaks
Vladimir Oltean (13):
pmc_agent: make pmc_agent_query_port_properties take an enum
port_state argument
ts2phc: rename source code files ("master" to "source", "slave" to
"sink")
ts2phc: rename "slave clocks" to "PPS sinks"
ts2phc: rename "master" to "source"
ts2phc: create a private data structure
ts2phc: instantiate a full clock structure for every PPS sink
ts2phc: instantiate a full clock structure for every PPS source of the
PHC kind
ts2phc: instantiate a pmc agent
ts2phc_slave: print offset to the source clock
ts2phc: split PPS sink poll from servo loop
ts2phc: reconfigure sync direction by subscribing to ptp4l port events
ts2phc: allow PHC PPS sources to be synchronized
ts2phc_phc_pps_source: make use of new kernel API for perout waveform
config.c | 1 +
makefile | 9 +-
missing.h | 52 ++
phc2sys.c | 6 +-
pmc_agent.c | 2 +-
pmc_agent.h | 3 +-
ts2phc.8 | 32 +-
ts2phc.c | 669 --
ts2phc.h | 64 ++
ts2phc_generic_master.c | 63 --
ts2phc_generic_master.h | 14 -
ts2phc_generic_pps_source.c | 63 ++
ts2phc_generic_pps_source.h | 15 +
ts2phc_master.c | 38 -
ts2phc_master.h | 52 --
ts2phc_master_private.h | 20 -
ts2phc_nmea_master.h | 13 -
..._nmea_master.c => ts2phc_nmea_pps_source.c | 119 ++--
ts2phc_nmea_pps_source.h | 14 +
ts2phc_phc_master.c | 113 ---
ts2phc_phc_master.h | 14 -
ts2phc_phc_pps_source.c | 157
ts2phc_phc_pps_source.h | 15 +
ts2phc_pps_sink.c | 433
ts2phc_pps_sink.h | 20 +
ts2phc_pps_source.c | 48 ++
ts2phc_pps_source.h | 56 ++
ts2phc_pps_source_private.h | 21 +
ts2phc_slave.c| 435
ts2phc_slave.h| 20 -
30 files changed, 1683 insertions(+), 898 deletions(-)
create mode 100644 ts2phc.h
delete mode 100644 ts2phc_generic_master.c
delete mode 100644 ts2phc_generic_master.h
create mode 100644 ts2phc_generic_pps_source.c
create mode 100644 ts2phc_generic_pps_source.h
delete mode 100644 ts2phc_master.c
delete mode 100644 ts2phc_master.h
delete mode 100644 ts2phc_master_private.h
delete mode 100644 ts2phc_nmea_master.h
rename ts2phc_nmea_master.c => ts2phc_nmea_pps_source.c (64%)
create mode 100644 ts2phc_nmea_pps_source.h
delete mode 100644 ts2phc_phc_master.c
delete mode 100644 ts2phc_phc_master.h
create mode 100644 ts2phc_phc_pps_source.c
create mode 100644 ts2phc_phc_pps_source.h
create mode 100644 ts2phc_pps_sink.c
create mode 100644 ts2phc_pps_sink.h
create mode 100644 ts2phc_pps_source.c
create mode 100644 ts2phc_pps_source.h
create mode 100644 ts2phc_pps_source_private.h
delete mode 100644 ts2phc_slave.c
delete mode 100644 ts2phc_slave.h
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v5 10/13] ts2phc: split PPS sink poll from servo loop
The previous patches have established that:
- a PPS sink deals with extts events
- a clock deals with synchronization via a servo loop
Therefore, the code for synchronization should not be part of the
implementation of a PPS sink. Move it to the main ts2phc.c.
This allows it to be used by a PPS source as well (the PHC kind).
Signed-off-by: Vladimir Oltean
---
v4->v5:
- Rebase on top of the variable renaming.
v3->v4:
- Use bool for boolean types.
v2->v3:
- None.
ts2phc.c | 94 +-
ts2phc.h | 3 +
ts2phc_pps_sink.c | 199 ++
3 files changed, 190 insertions(+), 106 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index 1c577af7768e..f84950947712 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -21,6 +21,9 @@
#include "ts2phc.h"
#include "version.h"
+#define NS_PER_SEC 10LL
+#define SAMPLE_WEIGHT 1.0
+
struct interface {
STAILQ_ENTRY(interface) list;
};
@@ -154,6 +157,30 @@ static struct servo *ts2phc_servo_create(struct
ts2phc_private *priv,
return servo;
}
+void ts2phc_clock_add_tstamp(struct ts2phc_clock *clock, tmv_t t)
+{
+ struct timespec ts = tmv_to_timespec(t);
+
+ pr_debug("adding tstamp %ld.%09ld to clock %s",
+ts.tv_sec, ts.tv_nsec, clock->name);
+ clock->last_ts = t;
+ clock->is_ts_available = true;
+}
+
+static int ts2phc_clock_get_tstamp(struct ts2phc_clock *clock, tmv_t *ts)
+{
+ if (!clock->is_ts_available)
+ return 0;
+ clock->is_ts_available = false;
+ *ts = clock->last_ts;
+ return 1;
+}
+
+static void ts2phc_clock_flush_tstamp(struct ts2phc_clock *clock)
+{
+ clock->is_ts_available = false;
+}
+
struct ts2phc_clock *ts2phc_clock_add(struct ts2phc_private *priv,
const char *device)
{
@@ -303,6 +330,64 @@ static int auto_init_ports(struct ts2phc_private *priv)
return 0;
}
+static void ts2phc_synchronize_clocks(struct ts2phc_private *priv)
+{
+ struct timespec source_ts;
+ tmv_t source_tmv;
+ struct ts2phc_clock *c;
+ int valid, err;
+
+ err = ts2phc_pps_source_getppstime(priv->src, _ts);
+ if (err < 0) {
+ pr_err("source ts not valid");
+ return;
+ }
+ if (source_ts.tv_nsec > NS_PER_SEC / 2)
+ source_ts.tv_sec++;
+ source_ts.tv_nsec = 0;
+
+ source_tmv = timespec_to_tmv(source_ts);
+
+ LIST_FOREACH(c, >clocks, list) {
+ int64_t offset;
+ double adj;
+ tmv_t ts;
+
+ valid = ts2phc_clock_get_tstamp(c, );
+ if (!valid) {
+ pr_debug("%s timestamp not valid, skipping", c->name);
+ continue;
+ }
+
+ offset = tmv_to_nanoseconds(tmv_sub(ts, source_tmv));
+
+ if (c->no_adj) {
+ pr_info("%s offset %10" PRId64, c->name,
+ offset);
+ continue;
+ }
+
+ adj = servo_sample(c->servo, offset, tmv_to_nanoseconds(ts),
+ SAMPLE_WEIGHT, >servo_state);
+
+ pr_info("%s offset %10" PRId64 " s%d freq %+7.0f",
+ c->name, offset, c->servo_state, adj);
+
+ switch (c->servo_state) {
+ case SERVO_UNLOCKED:
+ break;
+ case SERVO_JUMP:
+ clockadj_set_freq(c->clkid, -adj);
+ clockadj_step(c->clkid, -offset);
+ break;
+ case SERVO_LOCKED:
+ case SERVO_LOCKED_STABLE:
+ clockadj_set_freq(c->clkid, -adj);
+ break;
+ }
+ }
+}
+
static void usage(char *progname)
{
fprintf(stderr,
@@ -501,11 +586,18 @@ int main(int argc, char *argv[])
}
while (is_running()) {
+ struct ts2phc_clock *c;
+
+ LIST_FOREACH(c, , list)
+ ts2phc_clock_flush_tstamp(c);
+
err = ts2phc_pps_sink_poll();
- if (err) {
+ if (err < 0) {
pr_err("poll failed");
break;
}
+ if (err > 0)
+ ts2phc_synchronize_clocks();
}
ts2phc_cleanup();
diff --git a/ts2phc.h b/ts2phc.h
index 1fab09dbf130..f52a2f3d3cae 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -27,6 +27,8 @@ struct ts2phc_clock {
enum servo_state servo_state;
char *name;
bool no_adj;
+ bool is_ts_available;
+ tmv_t last_ts;
};
struct ts2phc_port {
@@
[Linuxptp-devel] [PATCH v5 12/13] ts2phc: allow PHC PPS sources to be synchronized
Now that we are registering a clock even for the PPS source when it
supports that (i.e. when it is a PHC), introduce a new API to retrieve
its clock in order to add timestamps to it.
The timestamps are a mere approximation. We configure the kernel to emit
periodic output and then never hear back from that PHC again. We just
assume that it emits periodic output as promised, and that each pulse is
emitted at the beginning of each second. We rely on the PPS sinks to
report an extts event first, and we know who generated that - the PPS
source, of course. So then we proceed to read the PPS source's PHC time,
and round that to what is the most plausible integer second in its time
base. We believe that to be the 'timestamp'. This is fed into the servo
algorithm.
The PHC PPS source can be synchronized to the extts events of a PHC
slave, when in automatic mode.
Signed-off-by: Vladimir Oltean
---
v4->v5:
- rebase on top of data structure renames
v3->v4:
Add one more paragraph to commit message.
v2->v3:
Implement ts2phc_master_get_clock() as part of ts2phc_master.c instead
of ts2phc_phc_master.c.
ts2phc.c| 45 -
ts2phc_phc_pps_source.c | 9
ts2phc_pps_source.c | 8 +++
ts2phc_pps_source.h | 2 ++
ts2phc_pps_source_private.h | 1 +
5 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/ts2phc.c b/ts2phc.c
index 0968ef28ca73..25c8bb3a5fa6 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -482,6 +482,42 @@ static void ts2phc_synchronize_clocks(struct
ts2phc_private *priv, int autocfg)
}
}
+static int ts2phc_collect_pps_source_tstamp(struct ts2phc_private *priv)
+{
+ struct ts2phc_clock *pps_src_clock;
+ struct timespec source_ts;
+ int err;
+
+ pps_src_clock = ts2phc_pps_source_get_clock(priv->src);
+ /*
+* PPS source isn't a PHC (it may be a generic or a GPS PPS source),
+* don't error out, just don't do anything. If it doesn't have a PHC,
+* there is nothing to synchronize, which is the only point of
+* collecting its perout timestamp in the first place.
+*/
+ if (!pps_src_clock)
+ return 0;
+
+ err = ts2phc_pps_source_getppstime(priv->src, _ts);
+ if (err < 0) {
+ pr_err("source ts not valid");
+ return err;
+ }
+
+ /*
+* As long as the kernel doesn't support a proper API for reporting
+* a precise perout timestamp, we'll have to use this crude
+* approximation.
+*/
+ if (source_ts.tv_nsec > NS_PER_SEC / 2)
+ source_ts.tv_sec++;
+ source_ts.tv_nsec = 0;
+
+ ts2phc_clock_add_tstamp(pps_src_clock, timespec_to_tmv(source_ts));
+
+ return 0;
+}
+
static void usage(char *progname)
{
fprintf(stderr,
@@ -702,8 +738,15 @@ int main(int argc, char *argv[])
pr_err("poll failed");
break;
}
- if (err > 0)
+ if (err > 0) {
+ err = ts2phc_collect_pps_source_tstamp();
+ if (err) {
+ pr_err("failed to collect PPS source tstamp");
+ break;
+ }
+
ts2phc_synchronize_clocks(, autocfg);
+ }
}
ts2phc_cleanup();
diff --git a/ts2phc_phc_pps_source.c b/ts2phc_phc_pps_source.c
index 4a4bfb2569b9..d9e00f3b525d 100644
--- a/ts2phc_phc_pps_source.c
+++ b/ts2phc_phc_pps_source.c
@@ -83,6 +83,14 @@ static int ts2phc_phc_pps_source_getppstime(struct
ts2phc_pps_source *src,
return clock_gettime(s->clock->clkid, ts);
}
+struct ts2phc_clock *ts2phc_phc_pps_source_get_clock(struct ts2phc_pps_source
*src)
+{
+ struct ts2phc_phc_pps_source *s =
+ container_of(src, struct ts2phc_phc_pps_source, pps_source);
+
+ return s->clock;
+}
+
struct ts2phc_pps_source *ts2phc_phc_pps_source_create(struct ts2phc_private
*priv,
const char *dev)
{
@@ -94,6 +102,7 @@ struct ts2phc_pps_source
*ts2phc_phc_pps_source_create(struct ts2phc_private *pr
}
s->pps_source.destroy = ts2phc_phc_pps_source_destroy;
s->pps_source.getppstime = ts2phc_phc_pps_source_getppstime;
+ s->pps_source.get_clock = ts2phc_phc_pps_source_get_clock;
s->clock = ts2phc_clock_add(priv, dev);
if (!s->clock) {
diff --git a/ts2phc_pps_source.c b/ts2phc_pps_source.c
index 2a3300aaaf19..53aaf207f566 100644
--- a/ts2phc_pps_source.c
+++ b/ts2phc_pps_source.c
@@ -38,3 +38,11 @@ int ts2phc_pps_source_getppstime(struct ts2phc_pps_source
*src, struct timespec
{
return src->getppstime(src, ts);
}
+
+struct ts2phc_clock *ts2phc_pps_source_get_clock(struct ts2phc_pps_s
[Linuxptp-devel] [PATCH v5 08/13] ts2phc: instantiate a pmc agent
This introduces the '-a' option in ts2phc, an option inspired from phc2sys that puts the clocks in "automatic" mode. In this mode, ts2phc listens, as a PMC, to port state change events from ptp4l, and detects which port state machine, if any, has transitioned to PS_SLAVE. That port's clock will become the synchronization reference for the clock hierarchy described by ts2phc. The use case is a multi-switch DSA setup with boundary_clock_jbod, where there is only one grandmaster, connected to one switch's port. The other switches, connected together through a PPS signal, must adapt themselves to this time reference, while the switch connected to the GM must not be synchronized by ts2phc because it is already synchronized by ptp4l. Graphically, the setup looks like this: +---+ | LS1028A /dev/ptp0 (unused) | | +--+| | | DSA master for Felix|| | |(internal ENETC port 2: eno2))|| | ++--+-+ | | | Felix embedded L2 switch /dev/ptp1 | | | | PPS source, sync target | | | | +--+ +--+ +--+ | | | | |DSA master for| |DSA master for| |DSA master for| | | | | | SJA1105 1 | | SJA1105 2 | | SJA1105 3 | | | | | |(Felix port 1)| |(Felix port 2)| |(Felix port 3)| | | +--+-+--+---+--+---+--+--+--+ /dev/ptp2/dev/ptp3/dev/ptp4 PPS sink, sync reference PPS sink, sync target PPS sink, sync target +---+ +---+ +---+ | SJA1105 switch 1| | SJA1105 switch 2| | SJA1105 switch 3| +-+-+-+-+ +-+-+-+-+ +-+-+-+-+ |sw1p0|sw1p1|sw1p2|sw1p3| |sw2p0|sw2p1|sw2p2|sw2p3| |sw3p0|sw3p1|sw3p2|sw3p3| +-+-+-+-+ +-+-+-+-+ +-+-+-+-+ ^ | GM connected here In the described state, the Felix embedded L2 switch is statically configured to emit a 1PPS signal on a sort of "simplex bus", and SJA1105 switches 1 to 3 are statically configured to record it in their respective time bases. Because the GM is connected to one of the ports of SJA1105 switch 1, that port is in PS_SLAVE and is the source of ts2phc time. So the embedded L2 switch as well as the other SJA1105 switches synchronize to the SJA1105. When the GM migrates to a port belonging to a different PHC (say sw2p0), the PPS distribution tree remains unchanged but the port reported by ptp4l as being in PS_SLAVE changes. So ts2phc must reconfigure itself to use /dev/ptp3 as time reference, and the other PHC devices turn into target clocks. Signed-off-by: Vladimir Oltean --- v4->v5: - rebased on top of the variable renaming v3->v4: - use the new pmc agent API - add more info to the commit message - priv->state_changed can be delayed to a future patch - use enum port_state instead of int v2->v3: - Added man page and usage verbiage. - Removed some useless curly braces in ts2phc_cleanup(). makefile | 5 +- ts2phc.8 | 15 ts2phc.c | 225 ++- ts2phc.h | 12 +++ 4 files changed, 254 insertions(+), 3 deletions(-) diff --git a/makefile b/makefile index 5295b60b5dac..9aed38318803 100644 --- a/makefile +++ b/makefile @@ -68,8 +68,9 @@ phc_ctl: phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o timemaster: phc.o print.o rtnl.o sk.o timemaster.o util.o version.o -ts2phc: config.o clockadj.o hash.o interface.o phc.o print.o $(SERVOS) sk.o \ - $(TS2PHC) util.o version.o +ts2phc: config.o clockadj.o hash.o interface.o msg.o phc.o pmc_agent.o \ + pmc_common.o print.o $(SERVOS) sk.o $(TS2PHC) tlv.o transport.o raw.o \ + udp.o udp6.o uds.o util.o version.o version.o: .version version.sh $(filter-out version.d,$(DEPEND)) diff --git a/ts2phc.8 b/ts2phc.8 index 690c4627f9f2..36d56cce0270 100644 --- a/ts2phc.8 +++ b/ts2phc.8 @@ -26,6 +26,21 @@ A single source may be used to distribute time to one or more PHC devices. .SH OPTIONS .TP +.BI \-a +Adjust the direction of synchronization automatically. The program determines +which PHC should the reference clock for time distribution and which should +be the destinations by querying the port states from the running instance of +.B ptp4l. +Note that using this option, the PPS signal distribution hierarchy still +remains fixed as per the configuration file. This implies that using this +option, a PPS source of the PHC kind may become a target clock, and a PPS sink +may become a reference clock. Other, non-PHC types of PPS
[Linuxptp-devel] [PATCH v5 13/13] ts2phc_phc_pps_source: make use of new kernel API for perout waveform
This API was introduced for 2 reasons:
1. Some hardware can emit PPS signals but not starting from arbitrary
absolute times, but rather just emit at a certain phase offset from
the beginning of each second. We _could_ patch ts2phc to always
specify a start time of 0.0 to PTP_PEROUT_REQUEST, and in
theory that should then become the kernel's responsibility to advance
that time in the past by an integer number of seconds while keeping
the phase untouched, but in practice, we would never know whether
that would actually work with all in-kernel PHC drivers, since it
wasn't enforced as a requirement before. So there was a need for a
new flag that only specifies the phase of the periodic signal, and
not the absolute start time.
2. Some hardware can, rather unfortunately, not distinguish between a
rising and a falling extts edge. And, since whatever rises also has
to fall before rising again, the strategy in ts2phc is to set a
'large' pulse width (half the period) and ignore the extts event
corresponding to the mid-way between one second and another. This is
all fine, but currently, ts2phc.pulsewidth is a read-only property in
the config file. The kernel is not instructed in any way to use this
value, it is simply that must be configured based on prior knowledge
of the PHC's implementation. This API changes that.
The introduction of a phase adjustment for the PHC kind of PPS sources
means we have to adjust our approximation of the precise perout
timestamp. We put that code into a common function and convert all call
sites to call that. We also need to do the same thing for the edge
ignoring logic.
Signed-off-by: Vladimir Oltean
---
v4->v5: rebase on top of variable renames
v3->v4: patch is new.
config.c| 1 +
missing.h | 52 +
ts2phc.8| 17 +++-
ts2phc.c| 86 +++--
ts2phc.h| 1 +
ts2phc_phc_pps_source.c | 44 ++---
ts2phc_pps_sink.c | 16 +++-
7 files changed, 180 insertions(+), 37 deletions(-)
diff --git a/config.c b/config.c
index f3c52baff765..760d0e12b0b6 100644
--- a/config.c
+++ b/config.c
@@ -321,6 +321,7 @@ struct config_item config_tab[] = {
GLOB_ITEM_STR("ts2phc.nmea_remote_host", ""),
GLOB_ITEM_STR("ts2phc.nmea_remote_port", ""),
GLOB_ITEM_STR("ts2phc.nmea_serialport", "/dev/ttyS0"),
+ PORT_ITEM_INT("ts2phc.perout_phase", -1, 0, 9),
PORT_ITEM_INT("ts2phc.pin_index", 0, 0, INT_MAX),
GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
diff --git a/missing.h b/missing.h
index 89cb51360ef7..7f06da3220f2 100644
--- a/missing.h
+++ b/missing.h
@@ -97,6 +97,58 @@ struct compat_ptp_clock_caps {
#endif /*LINUX_VERSION_CODE < 5.8*/
+/*
+ * Bits of the ptp_perout_request.flags field:
+ */
+
+#ifndef PTP_PEROUT_ONE_SHOT
+#define PTP_PEROUT_ONE_SHOT(1<<0)
+#endif
+
+#ifndef PTP_PEROUT_DUTY_CYCLE
+#define PTP_PEROUT_DUTY_CYCLE (1<<1)
+#endif
+
+#ifndef PTP_PEROUT_PHASE
+#define PTP_PEROUT_PHASE (1<<2)
+#endif
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5,9,0)
+
+struct compat_ptp_perout_request {
+ union {
+ /*
+* Absolute start time.
+* Valid only if (flags & PTP_PEROUT_PHASE) is unset.
+*/
+ struct ptp_clock_time start;
+ /*
+* Phase offset. The signal should start toggling at an
+* unspecified integer multiple of the period, plus this value.
+* The start time should be "as soon as possible".
+* Valid only if (flags & PTP_PEROUT_PHASE) is set.
+*/
+ struct ptp_clock_time phase;
+ };
+ struct ptp_clock_time period; /* Desired period, zero means disable. */
+ unsigned int index; /* Which channel to configure. */
+ unsigned int flags;
+ union {
+ /*
+* The "on" time of the signal.
+* Must be lower than the period.
+* Valid only if (flags & PTP_PEROUT_DUTY_CYCLE) is set.
+*/
+ struct ptp_clock_time on;
+ /* Reserved for future use. */
+ unsigned int rsv[4];
+ };
+};
+
+#define ptp_perout_request compat_ptp_perout_request
+
+#endif /* LINUX_VERSION_CODE < 5.9 */
+
#ifndef PTP_MAX_SAMPLES
#define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement samples. */
#endif /* PTP_MAX_SAMPLES */
diff --git a/ts2phc.8 b/ts2phc.8
index 36d56cce0270..ded6f9ac8afb 100644
--- a/ts2phc.8
+++ b/ts2phc.8
@@
[Linuxptp-devel] [PATCH v5 01/13] pmc_agent: make pmc_agent_query_port_properties take an enum port_state argument
Enforce type correctness and make all callers of pmc_agent_query_port_properties
pass an enum port_state argument instead of plain int.
Signed-off-by: Vladimir Oltean
---
v4->v5: patch is new
phc2sys.c | 6 --
pmc_agent.c | 2 +-
pmc_agent.h | 3 ++-
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/phc2sys.c b/phc2sys.c
index 6815c3dee8a0..786831cd3112 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -292,7 +292,8 @@ static struct port *port_add(struct phc2sys_private *priv,
unsigned int number,
static void clock_reinit(struct phc2sys_private *priv, struct clock *clock,
int new_state)
{
- int err = -1, phc_index = -1, phc_switched = 0, state, timestamping;
+ int err = -1, phc_index = -1, phc_switched = 0, timestamping;
+ enum port_state state;
struct port *p;
struct sk_ts_info ts_info;
char iface[IFNAMSIZ];
@@ -844,7 +845,8 @@ static int phc2sys_recv_subscribed(void *context, struct
ptp_message *msg,
static int auto_init_ports(struct phc2sys_private *priv, int add_rt)
{
- int err, number_ports, state, timestamping;
+ int err, number_ports, timestamping;
+ enum port_state state;
char iface[IFNAMSIZ];
struct clock *clock;
struct port *port;
diff --git a/pmc_agent.c b/pmc_agent.c
index 86350d8e6fd1..0dc99a4a83b5 100644
--- a/pmc_agent.c
+++ b/pmc_agent.c
@@ -303,7 +303,7 @@ int pmc_agent_query_dds(struct pmc_agent *node, int timeout)
}
int pmc_agent_query_port_properties(struct pmc_agent *node, int timeout,
- unsigned int port, int *state,
+ unsigned int port, enum port_state *state,
int *tstamping, char *iface)
{
struct port_properties_np *ppn;
diff --git a/pmc_agent.h b/pmc_agent.h
index 11b93479fb0b..43738fa71fe5 100644
--- a/pmc_agent.h
+++ b/pmc_agent.h
@@ -24,6 +24,7 @@
#include
+#include "fsm.h"
#include "pmc_common.h"
struct pmc_agent;
@@ -107,7 +108,7 @@ int pmc_agent_query_dds(struct pmc_agent *agent, int
timeout);
* @return Zero on success, negative error code otherwise.
*/
int pmc_agent_query_port_properties(struct pmc_agent *agent, int timeout,
- unsigned int port, int *state,
+ unsigned int port, enum port_state *state,
int *tstamping, char *iface);
/**
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Include phcIndex in PORT_PROPERTIES_NP PMC command
On Fri, Nov 19, 2021 at 05:22:16AM +, Karthikkumar Valoor wrote: > On Tue, 16 Nov 2021 at 15:22, Vladimir Oltean > mailto:olte...@gmail.com>> wrote: > On Tue, Nov 16, 2021 at 12:47:09PM +, Karthikkumar V via Linuxptp-devel > wrote: > > PORT_PROPERTIES_NP PMC Command will now display the phcIndex along with > > other interface details. A typical use case will be Users can fetch the > > phcIndex to use the clock APIs of the kernel (clock_gettime and > > clock_settime) > > to get the PHC time from user applications. > > > > Signed-off-by: Karthikkumar V > > mailto:kval...@altiostar.com>> > > Signed-off-by: Ramana Reddy > > mailto:rre...@altiostar.com>> > > --- > > May I suggest that > (a) you should not modify the binary format of management TLVs in > incompatible ways, as that would require everybody else to adapt and > somehow know which format ptp4l expects, and > Totally agree. > You should add new fields at the end of the port_properties_np structure. > Make sure a new pmc tool prints the message from an old ptp4l properly. > And verify an old pmc tool works with a new ptp4l. > > [Karthikkumar Valoor] Valid comment. Seems we have issue whether we add at > the end or middle of the structure (error thrown as “pmc[153304.857]: bad > message” > from pmc_recv() function due to size mismatch). > There will always be compatibility issues if one of the binaries (ptp4l/pmc) > are > old versions. Not sure how do we address these issues? What is the way > forward? I'm not sure that there is a precedent for this in ptp4l, but generally speaking your options are either to create a separate management TLV which contains just the additional info (phc_index), or to create a v2 of the PORT_PROPERTIES_NP which has a different management ID and has the size you need. None of those is ideal, and still appear unnecessary to me at this stage. > (b) you can deduce the PHC index already from the struct PTPText interface, > see posix_clock_open() -> sk_get_ts_info(). > > Changing management TLV should be done carefully. > Only when we must. > > [Karthikkumar Valoor] sk_get_ts_info() needs access to the host interface, > which > will not be visible in cloud environment for applications (for ex: > POD/container > trying to fetch the info).The change was added so that Client applications > can directly > access the /dev/ptpX device without calling the sk_get_ts_info() call. Since PTP management messages can also be sent over the network, not just over AF_UNIX, the PHC index would be equally irrelevant outside this host. You just gave one example where the PHC in /dev is visible across multiple containers running on the same host, but individual interfaces are in network namespaces. Personally, I think it is a non-goal for a non-portable management TLV created by ptp4l for phc2sys use to be netns-aware. What I'd do is I'd simply put the consumer of ptp4l management messages right into the same container as ptp4l itself. Then you can also do whatever with the interface name. You can create your own protocol over a separate socket through which your application forwards the information it gets from the local ptp4l to whomever else needs it. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Include phcIndex in PORT_PROPERTIES_NP PMC command
On Tue, Nov 16, 2021 at 12:47:09PM +, Karthikkumar V via Linuxptp-devel
wrote:
> PORT_PROPERTIES_NP PMC Command will now display the phcIndex along with
> other interface details. A typical use case will be Users can fetch the
> phcIndex to use the clock APIs of the kernel (clock_gettime and clock_settime)
> to get the PHC time from user applications.
>
> Signed-off-by: Karthikkumar V
> Signed-off-by: Ramana Reddy
> ---
May I suggest that
(a) you should not modify the binary format of management TLVs in
incompatible ways, as that would require everybody else to adapt and
somehow know which format ptp4l expects, and
(b) you can deduce the PHC index already from the struct PTPText interface,
see posix_clock_open() -> sk_get_ts_info().
> pmc.c | 6 --
> port.c | 1 +
> tlv.h | 1 +
> 3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/pmc.c b/pmc.c
> index a1ee787..67e33a3 100644
> --- a/pmc.c
> +++ b/pmc.c
> @@ -443,11 +443,13 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
> IFMT "portIdentity%s"
> IFMT "portState %s"
> IFMT "timestamping%s"
> - IFMT "interface %s",
> + IFMT "interface %s"
> + IFMT "phcIndex%d",
> pid2str(>portIdentity),
> ps_str[ppn->port_state],
> ts_str(ppn->timestamping),
> - text2str(>interface));
> + text2str(>interface),
> + ppn->phc_index);
> break;
> case MID_PORT_STATS_NP:
> pcp = (struct port_stats_np *) mgt->data;
> diff --git a/port.c b/port.c
> index 7e51e77..743b7c7 100644
> --- a/port.c
> +++ b/port.c
> @@ -956,6 +956,7 @@ static int port_management_fill_response(struct port
> *target,
> else
> ppn->port_state = target->state;
> ppn->timestamping = target->timestamping;
> + ppn->phc_index = target->phc_index;
> ts_label = interface_label(target->iface);
> ptp_text_set(>interface, ts_label);
> datalen = sizeof(*ppn) + ppn->interface.length;
> diff --git a/tlv.h b/tlv.h
> index 97615fd..0f6cf28 100644
> --- a/tlv.h
> +++ b/tlv.h
> @@ -341,6 +341,7 @@ struct port_properties_np {
> struct PortIdentity portIdentity;
> uint8_t port_state;
> uint8_t timestamping;
> + Integer32 phc_index;
> struct PTPText interface;
> } PACKED;
>
> --
> 1.8.3.1
>
>
>
> ___
> Linuxptp-devel mailing list
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v4 0/9] Dynamic sync direction for ts2phc
On Mon, Nov 15, 2021 at 06:15:08AM -0800, Richard Cochran wrote: > On Sat, Oct 09, 2021 at 05:11:51PM +0300, Vladimir Oltean wrote: > > > The overall board design for my use case is that there's an SoC with an > > embedded DSA switch, and hanging off of 3 ports of that embedded switch > > are 3 external switches. Every networking device (the DSA master for the > > embedded switch, the embedded switch, as well as each individual > > external switch) has a PTP clock. The topology for PPS signal > > distribution is fixed - that is given by hardware ability - the > > /dev/ptp1 clock can emit a valid PPS, and all external switches can > > timestamp that PPS (it is connected in a sort-of simplex "bus" design), > > and it cannot be any other way around. It looks like this: > > Thanks for the detailed explanation. Definitely want to support this > kind of HW design. Sounds encouraging! > > - I created an extra abstraction in ts2phc as "struct clock" that would > > represent what's synchronizable. The "master" and "slave" concepts > > retain their meaning, which is: "master" == the device that emits PPS, > > and "slave" == the device that timestamps PPS. > > This whole thing would be much easier to understand if we could > replace "master/slave" with PPS source/sink. So we'd have a PPS source, PPS sink, source clock and destination clock, right? In the code, would "master" become simply "source" or "pps_source"? > I'm not asking you to rename everything, but if you have the time and > inclination, it would be really helpful to me to have Patch #1 simply > rename ts2phc files and source code. I think I can do that. I just wanted to not change too much without further guidance. > With those terms in place, you can say things like "the PPS source is > the synchronization target/destination" (which is the high level goal > of this series). Yes, "the destination clock is also the PPS source" is something that is both (a) supported (b) easy to put into words (and hopefully also easy to understand). ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v4 2/9] ts2phc: instantiate a full clock structure for every slave PHC
On Mon, Nov 15, 2021 at 06:06:48AM -0800, Richard Cochran wrote:
> On Sat, Oct 09, 2021 at 05:11:53PM +0300, Vladimir Oltean wrote:
>
> > diff --git a/ts2phc.c b/ts2phc.c
> > index 67df5a532559..ca7684b314a4 100644
> > --- a/ts2phc.c
> > +++ b/ts2phc.c
> > @@ -7,9 +7,14 @@
> > * @note SPDX-License-Identifier: GPL-2.0+
> > */
> > #include
> > +#include
> > +#include
> > +#include
>
> Alphabetical order please.
>
> > +#include "clockadj.h"
> > #include "config.h"
> > #include "interface.h"
> > +#include "phc.h"
> > #include "print.h"
> > #include "ts2phc.h"
> > #include "version.h"
> > @@ -27,6 +32,80 @@ static void ts2phc_cleanup(struct ts2phc_private *priv)
> > config_destroy(priv->cfg);
> > }
> >
> > +static struct servo *servo_add(struct ts2phc_private *priv, struct clock
> > *clock)
> > +{
>
> This really wants a prefix in the name, like ts2phc_add_servo();
Ok.
> > +struct clock {
>
> How about ts2phc_clock ?
I was thinking about doing that, but I wanted to hear it from you first.
> > + LIST_ENTRY(clock) list;
> > + LIST_ENTRY(clock) dst_list;
> > + clockid_t clkid;
> > + int phc_index;
> > + int state;
> > + int new_state;
> > + struct servo *servo;
> > + enum servo_state servo_state;
> > + char *name;
> > + bool no_adj;
> > +};
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH] util: don't error out in posix_clock_open() if we can't determine PHC index
Ed uses the ptp_kvm driver with phc2sys, and Debian 10 has an udev rule
which creates a symlink for this device called /dev/ptp_kvm.
/lib/udev/rules.d/50-udev-default.rules:
SUBSYSTEM=="ptp", ATTR{clock_name}=="KVM virtual PTP", SYMLINK += "ptp_kvm"
Since the blamed patch, posix_clock_open(), called from a variety of
places including the phc2sys clock_add(), returns an error which states
that it cannot deduce the PHC index from this char device symlink.
It appears that phc2sys supports, to some extent, synchronizing PTP
clocks specified by path to the char device, for which the PHC index is
not known. In fact, there are some places in phc2sys where clocks are
compared by PHC number, but almost miraculously, it manages to not trip
over the fact that the PHC number is missing.
For example:
- find_dst_clock() finds clocks by PHC index, but it is only called from
the "automatic" (phc2sys -a) code path. In that mode, phc2sys finds
the PHC through this algorithm:
- send a PORT_PROPERTIES_NP management TLV to ptp4l which contains the
port interface name as string ("eth0" etc)
- retrieve the PHC device through ethtool ioctls and compute the device
path as /dev/ptpN as discussed
So it manages to find the char device path in the reverse direction
(the PHC number is already known).
- do_loop() has this logic:
/* don't try to synchronize the clock to itself */
if (clock->clkid == priv->master->clkid ||
(clock->phc_index >= 0 &&
clock->phc_index == priv->master->phc_index) ||
!strcmp(clock->device, priv->master->device))
continue;
which again, magically manages to not trip over a comparison between
the priv->master_phc_index (CLOCK_REALTIME, has phc_index == -1) and
clock->phc_index (/dev/ptp_kvm, also has phc_index == -1), simply
because the slave clock first has to have a valid phc_index before the
comparison is even been done.
Otherwise said, for phc2sys it is an actual regression to require that
the path to the PTP char device be in the "/dev/ptpN" format.
On the other hand, ts2phc has very different requirements, and it cannot
do anything useful with /dev/ptp_kvm or a PHC whose index it cannot
determine.
So what we do to fix the issue is remove the error code from
posix_clock_open() itself, and just error out in ts2phc, leaving phc2sys
to continue to work with phc_index == -1 for that clock.
The third caller of posix_clock_open(), phc_ctl, puts the returned
phc_index pointer into a "junk" variable, so technically this fixes a
regression which was introduces for that program, too.
Fixes: 380d023abb1f ("posix_clock_open: derive PHC index from device name if
possible")
Reported-by: Ed Branch
Signed-off-by: Vladimir Oltean
---
Sorry for sending the email twice, last time the patch didn't make it to
the list, I got this reply back from
linuxptp-devel-ow...@lists.sourceforge.net: "In order to reduce spam on
the list, you must be subscribed in order to post new messages."
ts2phc.c | 6 ++
util.c | 6 ++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index f0b63baa6f8b..520ab36eb87c 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -194,6 +194,12 @@ struct clock *clock_add(struct ts2phc_private *priv, const
char *device)
if (clkid == CLOCK_INVALID)
return NULL;
+ if (phc_index < 0) {
+ pr_err("failed to retrieve PHC index for clock %s", device);
+ posix_clock_close(clkid);
+ return NULL;
+ }
+
LIST_FOREACH(c, >clocks, list) {
if (c->phc_index == phc_index) {
/* Already have the clock, don't add it again */
diff --git a/util.c b/util.c
index cc4f11008be2..a11266689166 100644
--- a/util.c
+++ b/util.c
@@ -215,10 +215,8 @@ clockid_t posix_clock_open(const char *device, int
*phc_index)
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;
+ /* Don't completely error out on failure */
+ *phc_index = -1;
}
}
return clkid;
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize
Hi Erez, Please use a decent mail client, and do something about your company man-in-the-middling the links. https://github.com/adbrucker/unsanitize-safelinks On Thu, Nov 04, 2021 at 02:33:46PM +, Geva, Erez wrote: > Nice, > > Your code is GPL, > > So using your code does not helps, we can simply use linuxptp directly. > We can not integrate your code in to our application without GPL our code, > or we need to use it through bash script/system and parse the result. What help do you need? You have your own library, licensed the way you need it. > > Why bother? Why bother with what? My intention wasn't to sell anyone anything, it was just to make it clear that I'm not moving GPL code under closed doors. > Enjoy coding > Erez > > P.S. > We have nothing against GPL. I use GPL and LGPL gladly and I like to share > code freely. > But sometimes we need to write systems with code that uses a different > licence. > This is the reason for LGPL and libraries. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize
On Tue, Nov 02, 2021 at 01:34:01AM +0200, Vladimir Oltean wrote: > On Mon, Nov 01, 2021 at 03:56:38AM -0700, Richard Cochran wrote: > > On Mon, Nov 01, 2021 at 11:19:15AM +0200, Vladimir Oltean wrote: > > > > > For what it's worth, I have solved both my problems (checking ptp4l sync > > > and phc2sys sync) through alternative methods, > > > > How did you do it? > > For checking ptp4l sync I wrote my own PTP management message handling > over the local AF_UNIX socket and I am querying the CURRENT_DATA_SET. > For checking phc2sys sync I shamelessly stole the relevant logic from > phc2sys (read_phc/sysoff_measure), including the various PTP_SYS_OFFSET_* > ioctls, and I am just calculating the PHC's offset to CLOCK_REALTIME. > > So both of these are local. Then I created some communication protocol > in my application (sender and receiver roles) so that the sender queries > the receiver about its local sync data, and then has the logic necessary > to figure out whether now is a good time to send or not. Since it's GPL code that I shamelessly stole, it might be nice to also share the source code here (in case somebody else looking to solve the same problem wants to shamelessly steal mine): https://github.com/vladimiroltean/tsn-scripts/tree/master/isochron The relevant files are ptpmon.c (for ptp4l monitoring) and sysmon.c (for phc2sys monitoring). ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize
On Mon, Nov 01, 2021 at 03:56:38AM -0700, Richard Cochran wrote: > On Mon, Nov 01, 2021 at 11:19:15AM +0200, Vladimir Oltean wrote: > > > For what it's worth, I have solved both my problems (checking ptp4l sync > > and phc2sys sync) through alternative methods, > > How did you do it? For checking ptp4l sync I wrote my own PTP management message handling over the local AF_UNIX socket and I am querying the CURRENT_DATA_SET. For checking phc2sys sync I shamelessly stole the relevant logic from phc2sys (read_phc/sysoff_measure), including the various PTP_SYS_OFFSET_* ioctls, and I am just calculating the PHC's offset to CLOCK_REALTIME. So both of these are local. Then I created some communication protocol in my application (sender and receiver roles) so that the sender queries the receiver about its local sync data, and then has the logic necessary to figure out whether now is a good time to send or not. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize
Hi Miroslav, On Mon, Nov 01, 2021 at 10:13:36AM +0100, Miroslav Lichvar wrote: > On Fri, Oct 29, 2021 at 03:35:30PM +0300, Vladimir Oltean wrote: > > If I look at the struct timex definition from "man adjtimex", I see that > > both maxerror and esterror are expressed in microseconds (i.e. STA_NANO > > does not affect them), is that right? > > Yes, it's only microsecond resolution, but I think a new flag could be > implemented for better resolution if necessary. I gave it a deeper thought, and even with nanosecond resolution, it would still not be what I need. I need to measure the CLOCK_REALTIME offset to something very specific, in this case to a PHC. With a generic kernel interface I wouldn't know who set that maxerror/esterror and what is the CLOCK_REALTIME actually synchronized to - it could very well be an NTP daemon. For what it's worth, I have solved both my problems (checking ptp4l sync and phc2sys sync) through alternative methods, so this program / patch set can be considered abandoned. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize
On Thu, Oct 14, 2021 at 09:44:21AM +0200, Miroslav Lichvar wrote: > On Wed, Oct 13, 2021 at 08:37:27PM +0300, Vladimir Oltean wrote: > > The other topic is how to actually wait for phc2sys to synchronize, > > since the program presented here only works with ptp4l (or at least > > that's all I tested). > > If it is the system clock, phc2sys clears the STA_UNSYNC flag > (which is reported by ntp_gettime()/ntp_adjtime()) when the servo is > in the locked state. There is the esterror (and potentially also > maxerror) field, which could be set by ptp4l/phc2sys to some estimate > of the error. Any application could easily check the value to decide > if the clock is good enough for its purposes. No need to implement PTP > or have access to the ptp4l/phc2sys socket. > > I think it would be great if all PTP clocks had this status and > esterror/maxerror fields. For example, with the ptp_kvm driver guests > have access to the system clock of the host as a PHC, but there is no > way to check if/how well the clock is actually synchronized. > > I had this on my todo list for a long time, but wasn't able to look > into it yet. If I look at the struct timex definition from "man adjtimex", I see that both maxerror and esterror are expressed in microseconds (i.e. STA_NANO does not affect them), is that right? If so, I would need nanosecond resolution to determine whether phc2sys is synchronized well enough. If I'm wrong, could you please consider sending a patch to make phc2sys report the esterror and maxerror values to the kernel? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 10/10] checksync: new program to wait for a local or remote clock to synchronize
On Fri, Oct 15, 2021 at 10:48:28PM +, Geva, Erez wrote: > Regarding the "ugly scripting" of pmc. > > I create the http://libpmc.sf.net/ just to work around this issue. > > The libpmc provides a library with pmc tool capabilities, and wrapper for > scripting as Python, Lua and Perl. Is there an option to use C? I don't see it. I don't know anything other than C, nor do I have the time to learn right now or in the foreseeable future. > I think pmc is as a testing tool for the linuxptp project, but not a real > tool for users. I can agree with that. > The libpmc also comes with a compatible pmc tool that runs much faster then > the original. But equally useless from a functionality perspective, as per above. > Although, I am not oppose your patch. I just find it useless, as I perform > the same result in a Perl script. Here you're wrong, you can't do NOTIFY_TIME_SYNC remotely either, since the remote ptp4l denies that. It may be something you don't need, but that doesn't make it all useless. > And with a much higher flexibility and a better performance. I actually don't need flexibility/feature bloat, I want a simple tool for a simple task. And I use this on embedded systems where I don't have Lua, Perl, Python, Ruby, etc etc. I'm actually completely fine with the API exposed by pmc_agent.c. And the resulting program is so small that I don't think I have a huge problem with maintaining it myself if no other self-contained solution comes along. Also, better performance relative to what? > You can use the libpmc, to write a tool like that in the script language you > like. > And submit it as an example in the libpmc project. > > Enjoy >Erez ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize
On Thu, Oct 14, 2021 at 09:44:21AM +0200, Miroslav Lichvar wrote: > On Wed, Oct 13, 2021 at 08:37:27PM +0300, Vladimir Oltean wrote: > > The other topic is how to actually wait for phc2sys to synchronize, > > since the program presented here only works with ptp4l (or at least > > that's all I tested). > > If it is the system clock, phc2sys clears the STA_UNSYNC flag > (which is reported by ntp_gettime()/ntp_adjtime()) when the servo is > in the locked state. There is the esterror (and potentially also > maxerror) field, which could be set by ptp4l/phc2sys to some estimate > of the error. Any application could easily check the value to decide > if the clock is good enough for its purposes. No need to implement PTP > or have access to the ptp4l/phc2sys socket. > > I think it would be great if all PTP clocks had this status and > esterror/maxerror fields. For example, with the ptp_kvm driver guests > have access to the system clock of the host as a PHC, but there is no > way to check if/how well the clock is actually synchronized. > > I had this on my todo list for a long time, but wasn't able to look > into it yet. There are clearly some merits to this idea, but I don't know if it would cater to the use case where you want to send some traffic on a port that is a PTP master, and you want to know whether the slaves attached to you are synchronized? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize
Background: earlier this year I posted this message about how to wait until phc2sys and ptp4l are synchronized within a given margin: https://www.mail-archive.com/linuxptp-users@lists.sourceforge.net/msg02121.html and it seemed that there wasn't a good solution available for my needs, but I didn't have time to work on one either. In the meantime I was forced to write a kernel selftest to prove that a certain TSN feature works: https://patchwork.kernel.org/project/netdevbpf/cover/20210930075948.36981-1-xiaoliang.yan...@nxp.com/#24498755 and I was so annoyed about the way in which I need to check for sync status, that I said to myself I'll write up a C program the first chance I have. A week later I managed to find a few free hours to satisfy my selfish needs, and here I am presenting the idea to the list for initial feedback. I would really like linuxptp to have something like this going forward, it really helps automation scripts to have consistent results with few lines of shell code. I understand there might be items we need to discuss about more (like allowing remote PMC subscriptions, how to do that safely) and that's one of the topics that the discussion should be about. I thought that simply adding a bool config option would be enough precaution for now. The other topic is how to actually wait for phc2sys to synchronize, since the program presented here only works with ptp4l (or at least that's all I tested). We would need to generalize the clock management API into something that is embeddable in the struct clock used by phc2sys (because yes, phc2sys uses a "struct clock" that isn't the same "struct clock" as ptp4l). Richard suggested in the first link that we could let the "struct clock" from ptp4l receive the subscription requests and just have a special case for portIdentity.portNumber = 0x, those messages could be forwarded to the local phc2sys over UDS. With push notifications I do believe that becomes a bit more complicated, because the local ptp4l would also need to forward the push notification events back, and I don't know right now if that is the best overall design or if we can do better. Vladimir Oltean (10): pmc_agent: allow subscribing to TIME_STATUS_NP events pmc_agent: allow instantiating over other transports than UDS pmc_agent: add a function to retrieve clock identity util: make cid_eq comparisons take a const pointer clock: optionally allow remote subscriptions pmc_agent: make pmc_agent_query_port_properties take an enum port_state argument config: make boundary_hops a config item pmc_agent: print management error code values util: make pid2str and cid2str take const pointer arguments checksync: new program to wait for a local or remote clock to synchronize checksync.c | 577 clock.c | 22 +- config.c| 2 + makefile| 8 +- phc2sys.c | 12 +- pmc_agent.c | 66 +- pmc_agent.h | 28 ++- ts2phc.c| 7 +- util.c | 8 +- util.h | 7 +- 10 files changed, 704 insertions(+), 33 deletions(-) create mode 100644 checksync.c -- 2.25.1 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 03/10] pmc_agent: add a function to retrieve clock identity
With ports being able to be non-local if the PMC agent is using a
non-UDS transport, it is useful to have a unique way of identifying
them. Choose the clock ID as being that unique identifier and export a
function that retrieves the identity of the clock we're connected to.
Signed-off-by: Vladimir Oltean
---
pmc_agent.c | 10 ++
pmc_agent.h | 12
2 files changed, 22 insertions(+)
diff --git a/pmc_agent.c b/pmc_agent.c
index b617d12f5287..e1c513fe10da 100644
--- a/pmc_agent.c
+++ b/pmc_agent.c
@@ -293,6 +293,16 @@ int pmc_agent_get_number_ports(struct pmc_agent *node)
return node->dds.numberPorts;
}
+int pmc_agent_get_clock_identity(struct pmc_agent *node,
+struct ClockIdentity *cid)
+{
+ if (!node->dds_valid)
+ return -1;
+
+ *cid = node->dds.clockIdentity;
+ return 0;
+}
+
int pmc_agent_query_dds(struct pmc_agent *node, int timeout)
{
struct ptp_message *msg;
diff --git a/pmc_agent.h b/pmc_agent.h
index feffac6f1b64..9dc85e300f9e 100644
--- a/pmc_agent.h
+++ b/pmc_agent.h
@@ -72,6 +72,18 @@ int pmc_agent_get_leap(struct pmc_agent *agent);
*/
int pmc_agent_get_number_ports(struct pmc_agent *agent);
+/**
+ * Gets the clock identity from the default data set. Users
+ * should first call pmc_agent_query_dds() before invoking this
+ * function.
+ *
+ * @param agent Pointer to a PMC instance obtained via @ref
pmc_agent_create().
+ * @param cidPointer to the return storage for the clock identity.
+ * @return 0 on success, or -1 if unknown.
+ */
+int pmc_agent_get_clock_identity(struct pmc_agent *node,
+struct ClockIdentity *cid);
+
/**
* Gets the TAI-UTC offset.
* @param agent Pointer to a PMC instance obtained via @ref
pmc_agent_create().
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 06/10] pmc_agent: make pmc_agent_query_port_properties take an enum port_state argument
Enforce type correctness and make all callers of pmc_agent_query_port_properties
pass an enum port_state argument instead of plain int.
Signed-off-by: Vladimir Oltean
---
phc2sys.c | 6 --
pmc_agent.c | 2 +-
pmc_agent.h | 2 +-
ts2phc.c| 3 ++-
4 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/phc2sys.c b/phc2sys.c
index c706a72f144a..67954ed4a699 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -292,7 +292,8 @@ static struct port *port_add(struct phc2sys_private *priv,
unsigned int number,
static void clock_reinit(struct phc2sys_private *priv, struct clock *clock,
int new_state)
{
- int err = -1, phc_index = -1, phc_switched = 0, state, timestamping;
+ int err = -1, phc_index = -1, phc_switched = 0, timestamping;
+ enum port_state state;
struct port *p;
struct sk_ts_info ts_info;
char iface[IFNAMSIZ];
@@ -844,7 +845,8 @@ static int phc2sys_recv_subscribed(void *context, struct
ptp_message *msg,
static int auto_init_ports(struct phc2sys_private *priv, int add_rt)
{
- int err, number_ports, state, timestamping;
+ int err, number_ports, timestamping;
+ enum port_state state;
char iface[IFNAMSIZ];
struct clock *clock;
struct port *port;
diff --git a/pmc_agent.c b/pmc_agent.c
index e1c513fe10da..68300f4303f6 100644
--- a/pmc_agent.c
+++ b/pmc_agent.c
@@ -321,7 +321,7 @@ int pmc_agent_query_dds(struct pmc_agent *node, int timeout)
}
int pmc_agent_query_port_properties(struct pmc_agent *node, int timeout,
- unsigned int port, int *state,
+ unsigned int port, enum port_state *state,
int *tstamping, char *iface)
{
struct port_properties_np *ppn;
diff --git a/pmc_agent.h b/pmc_agent.h
index 9dc85e300f9e..6469f0f70e57 100644
--- a/pmc_agent.h
+++ b/pmc_agent.h
@@ -121,7 +121,7 @@ int pmc_agent_query_dds(struct pmc_agent *agent, int
timeout);
* @return Zero on success, negative error code otherwise.
*/
int pmc_agent_query_port_properties(struct pmc_agent *agent, int timeout,
- unsigned int port, int *state,
+ unsigned int port, enum port_state *state,
int *tstamping, char *iface);
/**
diff --git a/ts2phc.c b/ts2phc.c
index 0d2ed4280eee..f0b63baa6f8b 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -268,11 +268,12 @@ static struct port *port_add(struct ts2phc_private *priv,
unsigned int number,
static int auto_init_ports(struct ts2phc_private *priv)
{
- int state, timestamping;
+ enum port_state state;
int number_ports, err;
char iface[IFNAMSIZ];
struct clock *clock;
struct port *port;
+ int timestamping;
unsigned int i;
while (1) {
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 07/10] config: make boundary_hops a config item
The pmc program has a "-b" option, and the new checksync program will
have such an option too, but not via command-line switches (ok,
technically you can do "--boundary-hops=1"), but instead via its config
file.
So add "boundary_hops" as a valid configuration item, with a default of
1 and a valid range from 0 to 25..
Signed-off-by: Vladimir Oltean
---
config.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/config.c b/config.c
index 6ee46b448adc..82cd6b1ce8bd 100644
--- a/config.c
+++ b/config.c
@@ -225,6 +225,7 @@ struct config_item config_tab[] = {
PORT_ITEM_INT("announceReceiptTimeout", 3, 2, UINT8_MAX),
PORT_ITEM_ENU("asCapable", AS_CAPABLE_AUTO, as_capable_enu),
GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
+ PORT_ITEM_INT("boundary_hops", 1, 0, 255),
PORT_ITEM_INT("boundary_clock_jbod", 0, 0, 1),
PORT_ITEM_ENU("BMCA", BMCA_PTP, bmca_enu),
GLOB_ITEM_INT("check_fup_sync", 0, 0, 1),
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 10/10] checksync: new program to wait for a local or remote clock to synchronize
In the spirit of UNIX where one program does one job and does it well (famous last words), there seems to be a need for a standalone binary that performs a very trivial task: wait until the PTP synchronization finishes*. *The definition we choose for "finishing" is rather primitive: the offset to master is below a certain threshold. We make no guarantees that it may not rise above that threshold right after this program decides that the synchronization ended. Fundamentally, this program works in the following way: it automates what you are able to do using the following command: pmc -u -b 0 'GET TIME_STATUS_NP' sending: GET TIME_STATUS_NP a6f4af.fffe.fdfc73-0 seq 0 RESPONSE MANAGEMENT TIME_STATUS_NP master_offset 0 <- this is the interesting part ingress_time 90010984167360 cumulativeScaledRateOffset +0.0 scaledLastGmPhaseChange0 gmTimeBaseIndicator0 lastGmPhaseChange 0x'. gmPresent true gmIdentity 001f7b.fffe.630248 The program exits when the "master_offset" becomes <= the given threshold (passed via the "-x" option). The default value for the threshold is 0 ns. but because scripting pmc is rather ugly (I'm not even sure if it was designed for that), we do it in a small C program. Opening a PMC agent over the local UNIX domain socket will obviously only be able to reach the ports that are local to this system. However, that creates a problem: we may have a back-to-back connection between a master and a slave, and we haven't turned off the BMCA, so we don't know beforehand if we're going to be master or slave. But nonetheless, we want to send some data once the link is synchronized (i.e. if we're the slave, wait until we're synchronized; if we're the master, it would be nice to check that the link partner is synchronized). To support that use case, we actually need yet another PMC agent, this time one that subscribes for push notifications over the network. So we create one PMC agent over UDS, and one PMC agent over the network for each local port, in case that port might become a master. This new program needs the remote ptp4l instance to be run using "allow_remote_subscribers" set to 1. If that is turned off, it will work in a limited mode where it can only detect that the local system has finished sync, because the local port is a slave and no network agent is required for that. There is one more important option to mention, "-i" for the designated interface. This option can be omitted, and when that happens, checksync will wait for all detected slave ports to finish sync. When "-i" is specified, it will wait for a single port to finish. Which port? see below. The idea is that scripts should be able to do: system-a $ ptp4l -i eth0 -2 -P allow_remote_subscriptions 1 & system-a $ checksync -i eth0 -m -2 -x 20 && echo "Clock A is synchronized" system-b $ ptp4l -i eth1 -2 -P allow_remote_subscriptions 1 & system-b $ checksync -i eth1 -m -2 -x 20 && echo "Clock B is synchronized" otherwise said, run exactly the same script on different systems. However with PTP, one system may become master and the other may become slave, and therefore, one checksync instance will need to check the time status of the local system, and the other instance will need to check the time status of the remote system. But we don't want to know the details of the remote system, nor do we want to log into it using ssh and run checksync there. For all we care, we should be able to run checksync on a port that is in the MASTER state, and it should figure out which remote ports to check. So when "-i" is specified and that interface is a master port, we actually check the sync status of the remote ports attached to it. [ note, we actually only check the sync status of the first remote port, that was sufficient for the point-to-point use case I was testing with, we should check all ports though ] Signed-off-by: Vladimir Oltean --- checksync.c | 577 makefile| 8 +- 2 files changed, 583 insertions(+), 2 deletions(-) create mode 100644 checksync.c diff --git a/checksync.c b/checksync.c new file mode 100644 index ..99f85e473702 --- /dev/null +++ b/checksync.c @@ -0,0 +1,577 @@ +/** + * @file checksync.c + * @brief Utility program to wait until an external program synchronizes + * @note Copyright 2021 Vladimir Oltean + * @note SPDX-License-Identifier: GPL-2.0+ + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include "missing.h" +#include "pmc_agent.h" +#include "pmc_common.h"
[Linuxptp-devel] [RFC PATCH 04/10] util: make cid_eq comparisons take a const pointer
There is no reason why cid_eq takes a non-const argument, so make it const.
Signed-off-by: Vladimir Oltean
---
util.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/util.h b/util.h
index 739c8fda0548..5a22c4a79401 100644
--- a/util.h
+++ b/util.h
@@ -84,7 +84,8 @@ char *cid2str(struct ClockIdentity *id);
* @param b Second clock identity.
* @return 1 if identities are equal, 0 otherwise.
*/
-static inline int cid_eq(struct ClockIdentity *a, struct ClockIdentity *b)
+static inline int cid_eq(const struct ClockIdentity *a,
+const struct ClockIdentity *b)
{
return memcmp(a, b, sizeof(*a)) == 0;
}
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 05/10] clock: optionally allow remote subscriptions
In certain scenarios it is useful to know whether a remote port is
synchronized, for example when the local system is the PTP master and we
are trying to send time-synchronized data to a slave.
Introduce a new option in ptp4l, which defaults to false, and which
allows the following:
- a remote PMC agent to subscribe to a given data set
- the clock API to push notifications towards a non-local subscriber
The only SET that is supported is adding a subscription, no clock state
can be modified remotely. Nonetheless, there is no authentication
implemented, so the option should be used with care.
Signed-off-by: Vladimir Oltean
---
clock.c | 22 +++---
config.c | 1 +
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/clock.c b/clock.c
index d1c85ec5a7b1..0689360db410 100644
--- a/clock.c
+++ b/clock.c
@@ -76,6 +76,7 @@ struct clock_subscriber {
struct address addr;
UInteger16 sequenceId;
time_t expiration;
+ struct port *port;
};
struct clock {
@@ -152,8 +153,9 @@ static void remove_subscriber(struct clock_subscriber *s)
free(s);
}
-static void clock_update_subscription(struct clock *c, struct ptp_message *req,
- uint8_t *bitmask, uint16_t duration)
+static void clock_update_subscription(struct clock *c, struct port *p,
+ struct ptp_message *req, uint8_t *bitmask,
+ uint16_t duration)
{
struct clock_subscriber *s, *tmp;
struct timespec now;
@@ -175,6 +177,7 @@ static void clock_update_subscription(struct clock *c,
struct ptp_message *req,
memcpy(s->events, bitmask, EVENT_BITMASK_CNT);
clock_gettime(CLOCK_MONOTONIC, );
s->expiration = now.tv_sec + duration;
+ s->port = p;
} else {
remove_subscriber(s);
}
@@ -195,6 +198,7 @@ static void clock_update_subscription(struct clock *c,
struct ptp_message *req,
clock_gettime(CLOCK_MONOTONIC, );
s->expiration = now.tv_sec + duration;
s->sequenceId = 0;
+ s->port = p;
LIST_INSERT_HEAD(>subscribers, s, list);
}
@@ -248,10 +252,11 @@ static void clock_prune_subscriptions(struct clock *c)
void clock_send_notification(struct clock *c, struct ptp_message *msg,
enum notification event)
{
- struct port *uds = c->uds_rw_port;
struct clock_subscriber *s;
LIST_FOREACH(s, >subscribers, list) {
+ struct port *port = s->port ? s->port : c->uds_rw_port;
+
if (!event_bitmask_get(s->events, event))
continue;
/* send event */
@@ -262,7 +267,7 @@ void clock_send_notification(struct clock *c, struct
ptp_message *msg,
msg->management.targetPortIdentity.portNumber =
htons(s->targetPortIdentity.portNumber);
msg->address = s->addr;
- port_forward_to(uds, msg);
+ port_forward_to(port, msg);
}
}
@@ -532,7 +537,7 @@ static int clock_management_set(struct clock *c, struct
port *p,
break;
case MID_SUBSCRIBE_EVENTS_NP:
sen = (struct subscribe_events_np *)tlv->data;
- clock_update_subscription(c, req, sen->bitmask, sen->duration);
+ clock_update_subscription(c, p, req, sen->bitmask,
sen->duration);
respond = 1;
break;
case MID_SYNCHRONIZATION_UNCERTAIN_NP:
@@ -1436,6 +1441,7 @@ tmv_t clock_ingress_time(struct clock *c)
int clock_manage(struct clock *c, struct port *p, struct ptp_message *msg)
{
+ bool allow_remote_subscriptions = config_get_int(c->config, NULL,
"allow_remote_subscriptions");
int changed = 0, res, answers;
struct port *piter;
struct management_tlv *mgt;
@@ -1472,7 +1478,9 @@ int clock_manage(struct clock *c, struct port *p, struct
ptp_message *msg)
clock_management_send_error(p, msg, MID_WRONG_LENGTH);
return changed;
}
- if (p != c->uds_rw_port) {
+ if (p != c->uds_rw_port &&
+ (!allow_remote_subscriptions ||
+mgt->id != MID_SUBSCRIBE_EVENTS_NP)) {
/* Sorry, only allowed on the UDS-RW port. */
clock_management_send_error(p, msg, MID_NOT_SUPPORTED);
return changed;
@@ -1493,7 +1501,7 @@ int clock_manage(struct clock *c, struct port *p, struct
ptp_message *msg)
switch (mgt->id) {
case MID_PORT_PROPERTIES_NP:
- if (p != c->uds_rw_port) {
+
[Linuxptp-devel] [RFC PATCH 09/10] util: make pid2str and cid2str take const pointer arguments
These functions do not modify their argument, so make it const for
correctness.
Signed-off-by: Vladimir Oltean
---
util.c | 8
util.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/util.c b/util.c
index 113467d08b9f..cc4f11008be2 100644
--- a/util.c
+++ b/util.c
@@ -139,10 +139,10 @@ char *bin2str_impl(Octet *data, int len, char *buf, int
buf_len)
return buf;
}
-char *cid2str(struct ClockIdentity *id)
+char *cid2str(const struct ClockIdentity *id)
{
static char buf[64];
- unsigned char *ptr = id->id;
+ const unsigned char *ptr = id->id;
snprintf(buf, sizeof(buf), "%02x%02x%02x.%02x%02x.%02x%02x%02x",
ptr[0], ptr[1], ptr[2], ptr[3],
ptr[4], ptr[5], ptr[6], ptr[7]);
@@ -160,10 +160,10 @@ int count_char(const char *str, char c)
return num;
}
-char *pid2str(struct PortIdentity *id)
+char *pid2str(const struct PortIdentity *id)
{
static char buf[64];
- unsigned char *ptr = id->clockIdentity.id;
+ const unsigned char *ptr = id->clockIdentity.id;
snprintf(buf, sizeof(buf), "%02x%02x%02x.%02x%02x.%02x%02x%02x-%hu",
ptr[0], ptr[1], ptr[2], ptr[3],
ptr[4], ptr[5], ptr[6], ptr[7],
diff --git a/util.h b/util.h
index 5a22c4a79401..37d5dcf21a9c 100644
--- a/util.h
+++ b/util.h
@@ -75,7 +75,7 @@ char *bin2str_impl(Octet *data, int len, char *buf, int
buf_len);
* @param id Clock idendtity to show.
* @returnPointer to a static global buffer holding the result.
*/
-char *cid2str(struct ClockIdentity *id);
+char *cid2str(const struct ClockIdentity *id);
/**
* Compare two clock identities for equality.
@@ -107,7 +107,7 @@ int count_char(const char *str, char c);
* @param id Port idendtity to show.
* @returnPointer to a static global buffer holding the result.
*/
-char *pid2str(struct PortIdentity *id);
+char *pid2str(const struct PortIdentity *id);
char *portaddr2str(struct PortAddress *addr);
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 02/10] pmc_agent: allow instantiating over other transports than UDS
In certain scenarios it is useful to know whether a remote port is
synchronized, for example when the local system is the PTP master and we
are trying to send time-synchronized data to a slave.
Allow the PTP management client agent to take the transport as argument,
and modify the existing callers of init_pmc_node() to specify UDS.
Signed-off-by: Vladimir Oltean
---
phc2sys.c | 4 ++--
pmc_agent.c | 6 --
pmc_agent.h | 4 +++-
ts2phc.c| 2 +-
4 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/phc2sys.c b/phc2sys.c
index 1c02e2b605b0..c706a72f144a 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -1309,7 +1309,7 @@ int main(int argc, char *argv[])
getpid());
if (autocfg) {
- if (init_pmc_node(cfg, priv.agent, uds_local,
+ if (init_pmc_node(cfg, priv.agent, TRANS_UDS, uds_local, 0,
phc2sys_recv_subscribed, ))
goto end;
if (auto_init_ports(, rt) < 0)
@@ -1326,7 +1326,7 @@ int main(int argc, char *argv[])
r = -1;
if (wait_sync) {
- if (init_pmc_node(cfg, priv.agent, uds_local,
+ if (init_pmc_node(cfg, priv.agent, TRANS_UDS, uds_local, 0,
phc2sys_recv_subscribed, ))
goto end;
diff --git a/pmc_agent.c b/pmc_agent.c
index 77334e381b6e..b617d12f5287 100644
--- a/pmc_agent.c
+++ b/pmc_agent.c
@@ -235,10 +235,12 @@ int run_pmc_wait_sync(struct pmc_agent *node, int timeout)
}
}
-int init_pmc_node(struct config *cfg, struct pmc_agent *node, const char *uds,
+int init_pmc_node(struct config *cfg, struct pmc_agent *node,
+ enum transport_type transport_type, const char *iface_name,
+ UInteger8 boundary_hops,
pmc_node_recv_subscribed_t *recv_subscribed, void *context)
{
- node->pmc = pmc_create(cfg, TRANS_UDS, uds, 0,
+ node->pmc = pmc_create(cfg, transport_type, iface_name, boundary_hops,
config_get_int(cfg, NULL, "domainNumber"),
config_get_int(cfg, NULL, "transportSpecific")
<< 4, 1);
if (!node->pmc) {
diff --git a/pmc_agent.h b/pmc_agent.h
index ea5788d79606..feffac6f1b64 100644
--- a/pmc_agent.h
+++ b/pmc_agent.h
@@ -31,7 +31,9 @@ struct pmc_agent;
typedef int pmc_node_recv_subscribed_t(void *context, struct ptp_message *msg,
int excluded);
-int init_pmc_node(struct config *cfg, struct pmc_agent *agent, const char *uds,
+int init_pmc_node(struct config *cfg, struct pmc_agent *node,
+ enum transport_type transport_type, const char *iface_name,
+ UInteger8 boundary_hops,
pmc_node_recv_subscribed_t *recv_subscribed, void *context);
int run_pmc_wait_sync(struct pmc_agent *agent, int timeout);
diff --git a/ts2phc.c b/ts2phc.c
index dd6b7ec1b7c9..0d2ed4280eee 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -667,7 +667,7 @@ int main(int argc, char *argv[])
getpid());
if (autocfg) {
- err = init_pmc_node(cfg, priv.agent, uds_local,
+ err = init_pmc_node(cfg, priv.agent, TRANS_UDS, uds_local, 0,
ts2phc_recv_subscribed, );
if (err) {
ts2phc_cleanup();
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 08/10] pmc_agent: print management error code values
It may be useful to see why a remote system has returned an error for a
PMC command, so add some code to print the error code and the string
interpretation of it.
Signed-off-by: Vladimir Oltean
---
pmc_agent.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/pmc_agent.c b/pmc_agent.c
index 68300f4303f6..91bdd34eebb6 100644
--- a/pmc_agent.c
+++ b/pmc_agent.c
@@ -102,6 +102,36 @@ static int get_mgt_err_id(struct ptp_message *msg)
return mgt->id;
}
+static int get_mgt_err_code(struct ptp_message *msg)
+{
+ struct management_error_status *mgt;
+
+ mgt = (struct management_error_status *)msg->management.suffix;
+ return mgt->error;
+}
+
+static const char *mgt_err_code_to_string(int mgt_error)
+{
+ switch (mgt_error) {
+ case MID_RESPONSE_TOO_BIG:
+ return "response too big";
+ case MID_NO_SUCH_ID:
+ return "no such ID";
+ case MID_WRONG_LENGTH:
+ return "wrong length";
+ case MID_WRONG_VALUE:
+ return "wrong value";
+ case MID_NOT_SETABLE:
+ return "not settable";
+ case MID_NOT_SUPPORTED:
+ return "not supported";
+ case MID_GENERAL_ERROR:
+ return "general error";
+ default:
+ return "unknown";
+ }
+}
+
#define RUN_PMC_OKAY1
#define RUN_PMC_TMO 0
#define RUN_PMC_NODEV -1
@@ -181,6 +211,10 @@ static int run_pmc(struct pmc_agent *node, int timeout,
int ds_id,
res = is_msg_mgt(*msg);
if (res < 0 && get_mgt_err_id(*msg) == ds_id) {
+ int err_code = get_mgt_err_code(*msg);
+
+ pr_err("PMC got management error code %d (%s)",
+ err_code, mgt_err_code_to_string(err_code));
node->pmc_ds_requested = 0;
return RUN_PMC_NODEV;
}
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [RFC PATCH 01/10] pmc_agent: allow subscribing to TIME_STATUS_NP events
Currently, PMC agents can only subscribe to port state change events.
However, it may be useful to also create push subscriptions to the
non-portable ptp4l TIME_STATUS management messages. These tell a PTP
management client information such as the offset to the master.
Extend the pmc_agent_subscribe() API by adding two boolean arguments,
one for subscribing to port events and the other to time status events.
Convert all users to pass true to the first and false to the latter.
Signed-off-by: Vladimir Oltean
---
phc2sys.c | 2 +-
pmc_agent.c | 14 --
pmc_agent.h | 10 --
ts2phc.c| 2 +-
4 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/phc2sys.c b/phc2sys.c
index 6815c3dee8a0..1c02e2b605b0 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -871,7 +871,7 @@ static int auto_init_ports(struct phc2sys_private *priv,
int add_rt)
return -1;
}
- err = pmc_agent_subscribe(priv->agent, 1000);
+ err = pmc_agent_subscribe(priv->agent, 1000, true, false);
if (err) {
pr_err("failed to subscribe");
return -1;
diff --git a/pmc_agent.c b/pmc_agent.c
index 86350d8e6fd1..77334e381b6e 100644
--- a/pmc_agent.c
+++ b/pmc_agent.c
@@ -38,6 +38,9 @@ struct pmc_agent {
struct pmc *pmc;
uint64_t pmc_last_update;
+ bool notify_port_state;
+ bool notify_time_sync;
+
struct defaultDS dds;
bool dds_valid;
int leap;
@@ -57,7 +60,10 @@ static void send_subscription(struct pmc_agent *node)
memset(, 0, sizeof(sen));
sen.duration = PMC_SUBSCRIBE_DURATION;
- event_bitmask_set(sen.bitmask, NOTIFY_PORT_STATE, TRUE);
+ event_bitmask_set(sen.bitmask, NOTIFY_PORT_STATE,
+ node->notify_port_state);
+ event_bitmask_set(sen.bitmask, NOTIFY_TIME_SYNC,
+ node->notify_time_sync);
pmc_send_set_action(node->pmc, MID_SUBSCRIBE_EVENTS_NP, ,
sizeof(sen));
}
@@ -375,8 +381,12 @@ void pmc_agent_set_sync_offset(struct pmc_agent *agent,
int offset)
agent->sync_offset = offset;
}
-int pmc_agent_subscribe(struct pmc_agent *node, int timeout)
+int pmc_agent_subscribe(struct pmc_agent *node, int timeout,
+ bool notify_port_state,
+ bool notify_time_sync)
{
+ node->notify_port_state = notify_port_state;
+ node->notify_time_sync = notify_time_sync;
node->stay_subscribed = true;
return renew_subscription(node, timeout);
}
diff --git a/pmc_agent.h b/pmc_agent.h
index 11b93479fb0b..ea5788d79606 100644
--- a/pmc_agent.h
+++ b/pmc_agent.h
@@ -132,12 +132,18 @@ int pmc_agent_query_utc_offset(struct pmc_agent *agent,
int timeout);
void pmc_agent_set_sync_offset(struct pmc_agent *agent, int offset);
/**
- * Subscribes to push notifications of changes in port state.
+ * Subscribes to push notifications.
* @param agent Pointer to a PMC instance obtained via @ref
pmc_agent_create().
* @param timeout Transmit and receive timeout in milliseconds.
+ * @param notify_port_state Receive MID_PORT_DATA_SET management messages
+ * for changes in port state.
+ * @param notify_time_sync Receive MID_TIME_STATUS_NP management messages
+ * for clock synchronization updates.
* @return Zero on success, negative error code otherwise.
*/
-int pmc_agent_subscribe(struct pmc_agent *agent, int timeout);
+int pmc_agent_subscribe(struct pmc_agent *agent, int timeout,
+ bool notify_port_state,
+ bool notify_time_sync);
/**
* Polls for push notifications from the local ptp4l service.
diff --git a/ts2phc.c b/ts2phc.c
index 1301b6c6795b..dd6b7ec1b7c9 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -293,7 +293,7 @@ static int auto_init_ports(struct ts2phc_private *priv)
return -1;
}
- err = pmc_agent_subscribe(priv->agent, 1000);
+ err = pmc_agent_subscribe(priv->agent, 1000, true, false);
if (err) {
pr_err("failed to subscribe");
return -1;
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v4 5/9] ts2phc_slave: print master offset
Make this information more visible by default, since it is the key
output of this program.
Signed-off-by: Vladimir Oltean
Reviewed-by: Jacob Keller
---
Changes in v4:
None.
Changes in v3:
None.
ts2phc_slave.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ts2phc_slave.c b/ts2phc_slave.c
index bbda617ce8ce..435e7d977d55 100644
--- a/ts2phc_slave.c
+++ b/ts2phc_slave.c
@@ -264,8 +264,8 @@ static int ts2phc_slave_event(struct ts2phc_slave *slave,
adj = servo_sample(slave->clock->servo, offset, extts_ts,
SAMPLE_WEIGHT, >clock->servo_state);
- pr_debug("%s master offset %10" PRId64 " s%d freq %+7.0f",
-slave->name, offset, slave->clock->servo_state, adj);
+ pr_info("%s master offset %10" PRId64 " s%d freq %+7.0f",
+ slave->name, offset, slave->clock->servo_state, adj);
switch (slave->clock->servo_state) {
case SERVO_UNLOCKED:
--
2.25.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v4 2/9] ts2phc: instantiate a full clock structure for every slave PHC
Slaves in ts2phc are PHC devices that implement the extts kernel API.
They are slaves just in the sense that they timestamp a pulse emitted by
somebody else.
Currently in ts2phc, PPS slaves are also the only candidates for the
clocks that get synchronized. There are 2 aspects that make this too
restrictive:
- Not all PPS slaves may be synchronization slaves. Consider a dynamic
environment of multiple DSA switches using boundary_clock_jbod, and
only one port is in the PS_SLAVE state. In that case, the clock of
that port should be the synchronization master (called the "source
clock" from now on, as far as ts2phc is concerned), regardless of
whether it supports the extts API or not.
- Not all synchronization slaves may be PPS slaves. Specifically, the
"PHC" type of PPS master in ts2phc can also be, fundamentally,
disciplined. The code should be prepared to handle this by recognizing
that things that can be disciplined by a servo should be represented
by a "struct clock", and things that can timestamp external events
should be represented by a "struct ts2phc_slave".
Signed-off-by: Vladimir Oltean
---
Changes in v4:
- rebase conflict with 6fea40606f79 ("Avoid setting clock frequency when
free running."). I preserved the logic to avoid setting back the freq
if we are free-running, but since ts2phc requires much newer kernel
interfaces, I seriously doubt that avoiding the claimed kernel bug is
something anyone relies on. Yet I did not delete that call, I want to
keep this patch set with the minimal amount of changes required.
- servo_add can be static
- the clock_add_tstamp prototype can be added in a later patch
- made clock->no_adj bool instead of int
- clock->is_destination can be added in a later patch
Changes in v3:
None.
ts2phc.c | 79
ts2phc.h | 24 ++
ts2phc_slave.c | 90 --
3 files changed, 138 insertions(+), 55 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index 67df5a532559..ca7684b314a4 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -7,9 +7,14 @@
* @note SPDX-License-Identifier: GPL-2.0+
*/
#include
+#include
+#include
+#include
+#include "clockadj.h"
#include "config.h"
#include "interface.h"
+#include "phc.h"
#include "print.h"
#include "ts2phc.h"
#include "version.h"
@@ -27,6 +32,80 @@ static void ts2phc_cleanup(struct ts2phc_private *priv)
config_destroy(priv->cfg);
}
+static struct servo *servo_add(struct ts2phc_private *priv, struct clock
*clock)
+{
+ enum servo_type type = config_get_int(priv->cfg, NULL, "clock_servo");
+ struct servo *servo;
+ int fadj, max_adj;
+
+ fadj = (int) clockadj_get_freq(clock->clkid);
+ /* Due to a bug in older kernels, the reading may silently fail
+* and return 0. Set the frequency back to make sure fadj is
+* the actual frequency of the clock.
+*/
+ if (!clock->no_adj) {
+ clockadj_set_freq(clock->clkid, fadj);
+ }
+
+ max_adj = phc_max_adj(clock->clkid);
+
+ servo = servo_create(priv->cfg, type, -fadj, max_adj, 0);
+ if (!servo)
+ return NULL;
+
+ servo_sync_interval(servo, SERVO_SYNC_INTERVAL);
+
+ return servo;
+}
+
+struct clock *clock_add(struct ts2phc_private *priv, const char *device)
+{
+ clockid_t clkid = CLOCK_INVALID;
+ int phc_index = -1;
+ struct clock *c;
+ int err;
+
+ clkid = posix_clock_open(device, _index);
+ if (clkid == CLOCK_INVALID)
+ return NULL;
+
+ LIST_FOREACH(c, >clocks, list) {
+ if (c->phc_index == phc_index) {
+ /* Already have the clock, don't add it again */
+ posix_clock_close(clkid);
+ return c;
+ }
+ }
+
+ c = calloc(1, sizeof(*c));
+ if (!c) {
+ pr_err("failed to allocate memory for a clock");
+ return NULL;
+ }
+ c->clkid = clkid;
+ c->phc_index = phc_index;
+ c->servo_state = SERVO_UNLOCKED;
+ c->servo = servo_add(priv, c);
+ c->no_adj = config_get_int(priv->cfg, NULL, "free_running");
+ err = asprintf(>name, "/dev/ptp%d", phc_index);
+ if (err < 0) {
+ free(c);
+ posix_clock_close(clkid);
+ return NULL;
+ }
+
+ LIST_INSERT_HEAD(>clocks, c, list);
+ return c;
+}
+
+void clock_destroy(struct clock *c)
+{
+ servo_destroy(c->servo);
+ posix_clock_close(c->clkid);
+ free(c->name);
+ free(c);
+}
+
static void usage(char *progname)
{
fprintf(stderr,
diff --git a/ts2phc.h
[Linuxptp-devel] [PATCH v4 0/9] Dynamic sync direction for ts2phc
e hardware actually uses. The changes should be backwards-compatible with the non-automatic mode. I have only tested non-automatic mode with a PHC master (that's all I have) and it still appears to work as before. Changes in v4: - Rebased and resolved minor conflicts - Small coding style cleanups - Added patch for perout and phase configuration - Retested Changes in v3: - Split patch "phc2sys: break out pmc code into pmc_common.c" into more trivial chunks. There is zero delta compared to previous monolithic patch, otherwise. - Replaced full license header in ts2phc.h with simple SPDX. - Added usage message and manpage for '-a' option. - Removed some useless curly braces. - Moved ts2phc_master_get_clock() from ts2phc_phc_master.c to ts2phc_master.c where it should be. - Added justification in commit message for creating struct ts2phc_private. - Reordered headers alphabetically. Changes in v2: - Added Jacob's review tags, addressed some feedback. - Dropped patch "pmc_common: fix potential memory leak in run_pmc_events()" - Reordered patches (put the tmv helpers first) - Fixed memory leaks Vladimir Oltean (9): ts2phc: create a private data structure ts2phc: instantiate a full clock structure for every slave PHC ts2phc: instantiate a full clock structure for every PHC master ts2phc: instantiate a pmc agent ts2phc_slave: print master offset ts2phc: split slave poll from servo loop ts2phc: reconfigure sync direction by subscribing to ptp4l port events ts2phc: allow the PHC PPS master to be synchronized ts2phc_phc_master: make use of new kernel API for perout waveform config.c| 1 + makefile| 5 +- missing.h | 52 ts2phc.8| 32 +- ts2phc.c| 645 +--- ts2phc.h| 64 ts2phc_generic_master.c | 2 +- ts2phc_generic_master.h | 3 +- ts2phc_master.c | 18 +- ts2phc_master.h | 8 +- ts2phc_master_private.h | 1 + ts2phc_nmea_master.c| 7 +- ts2phc_nmea_master.h| 3 +- ts2phc_phc_master.c | 82 +++-- ts2phc_phc_master.h | 3 +- ts2phc_slave.c | 386 ts2phc_slave.h | 10 +- 17 files changed, 1054 insertions(+), 268 deletions(-) create mode 100644 ts2phc.h -- 2.25.1 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v4 9/9] ts2phc_phc_master: make use of new kernel API for perout waveform
This API was introduced for 2 reasons:
1. Some hardware can emit PPS signals but not starting from arbitrary
absolute times, but rather just emit at a certain phase offset from
the beginning of each second. We _could_ patch ts2phc to always
specify a start time of 0.0 to PTP_PEROUT_REQUEST, and in
theory that should then become the kernel's responsibility to advance
that time in the past by an integer number of seconds while keeping
the phase untouched, but in practice, we would never know whether
that would actually work with all in-kernel PHC drivers, since it
wasn't enforced as a requirement before. So there was a need for a
new flag that only specifies the phase of the periodic signal, and
not the absolute start time.
2. Some hardware can, rather unfortunately, not distinguish between a
rising and a falling extts edge. And, since whatever rises also has
to fall before rising again, the strategy in ts2phc is to set a
'large' pulse width (half the period) and ignore the extts event
corresponding to the mid-way between one second and another. This is
all fine, but currently, ts2phc.pulsewidth is a read-only property in
the config file. The kernel is not instructed in any way to use this
value, it is simply that must be configured based on prior knowledge
of the PHC's implementation. This API changes that.
The introduction of a phase adjustment for the master PHC means we have
to adjust our approximation of the precise perout timestamp. We put that
code into a common function and convert all call sites to call that. We
also need to do the same thing for the edge ignoring logic.
Signed-off-by: Vladimir Oltean
---
Changes in v4:
Patch is new.
config.c| 1 +
missing.h | 52 +++
ts2phc.8| 17 +++--
ts2phc.c| 86 ++---
ts2phc.h| 1 +
ts2phc_phc_master.c | 44 ---
ts2phc_slave.c | 16 +++--
7 files changed, 180 insertions(+), 37 deletions(-)
diff --git a/config.c b/config.c
index f3c52baff765..760d0e12b0b6 100644
--- a/config.c
+++ b/config.c
@@ -321,6 +321,7 @@ struct config_item config_tab[] = {
GLOB_ITEM_STR("ts2phc.nmea_remote_host", ""),
GLOB_ITEM_STR("ts2phc.nmea_remote_port", ""),
GLOB_ITEM_STR("ts2phc.nmea_serialport", "/dev/ttyS0"),
+ PORT_ITEM_INT("ts2phc.perout_phase", -1, 0, 9),
PORT_ITEM_INT("ts2phc.pin_index", 0, 0, INT_MAX),
GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
diff --git a/missing.h b/missing.h
index 89cb51360ef7..7f06da3220f2 100644
--- a/missing.h
+++ b/missing.h
@@ -97,6 +97,58 @@ struct compat_ptp_clock_caps {
#endif /*LINUX_VERSION_CODE < 5.8*/
+/*
+ * Bits of the ptp_perout_request.flags field:
+ */
+
+#ifndef PTP_PEROUT_ONE_SHOT
+#define PTP_PEROUT_ONE_SHOT(1<<0)
+#endif
+
+#ifndef PTP_PEROUT_DUTY_CYCLE
+#define PTP_PEROUT_DUTY_CYCLE (1<<1)
+#endif
+
+#ifndef PTP_PEROUT_PHASE
+#define PTP_PEROUT_PHASE (1<<2)
+#endif
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5,9,0)
+
+struct compat_ptp_perout_request {
+ union {
+ /*
+* Absolute start time.
+* Valid only if (flags & PTP_PEROUT_PHASE) is unset.
+*/
+ struct ptp_clock_time start;
+ /*
+* Phase offset. The signal should start toggling at an
+* unspecified integer multiple of the period, plus this value.
+* The start time should be "as soon as possible".
+* Valid only if (flags & PTP_PEROUT_PHASE) is set.
+*/
+ struct ptp_clock_time phase;
+ };
+ struct ptp_clock_time period; /* Desired period, zero means disable. */
+ unsigned int index; /* Which channel to configure. */
+ unsigned int flags;
+ union {
+ /*
+* The "on" time of the signal.
+* Must be lower than the period.
+* Valid only if (flags & PTP_PEROUT_DUTY_CYCLE) is set.
+*/
+ struct ptp_clock_time on;
+ /* Reserved for future use. */
+ unsigned int rsv[4];
+ };
+};
+
+#define ptp_perout_request compat_ptp_perout_request
+
+#endif /* LINUX_VERSION_CODE < 5.9 */
+
#ifndef PTP_MAX_SAMPLES
#define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement samples. */
#endif /* PTP_MAX_SAMPLES */
diff --git a/ts2phc.8 b/ts2phc.8
index 1a3d2feac1b7..5a2d75ffe6f9 100644
--- a/ts2phc.8
+++ b/ts2phc.8
@@ -176,10 +176,23 @@ connection will be used in preference to the co
[Linuxptp-devel] [PATCH v4 3/9] ts2phc: instantiate a full clock structure for every PHC master
This propagates the use of "struct ts2phc_private" all the way into the
master API, in preparation of a new use case that will be supported
soon: some PPS masters (to be precise, the "PHC" kind) instantiate a
struct clock which could be disciplined by ts2phc.
When a PHC A emits a pulse and another PHC B timestamps it, the offset
between their precise timestamps can be used to synchronize either one
of them. So far in ts2phc, only the slave PHC (the one using extts) has
been synchronized to the master (the one using perout).
This is partly because there is no proper kernel API to report the
precise timestamp of a perout pulse. We only have the periodic API, and
that doesn't report precise timestamps either; we just use vague
approximations of what the PPS master PHC's time was, based on reading
that PHC immediately after a slave extts event was received by the
application. While this is far from ideal, it does work, and does allow
PHC A to be synchronized to B.
This is particularly useful with the yet-to-be-introduced "automatic"
mode of ts2phc (similar to '-a' of phc2sys), and the PPS distribution
tree is fixed in hardware (as opposed to port states, which in
"automatic" mode are dynamic, as the name suggests).
Signed-off-by: Vladimir Oltean
---
Changes in v4:
- rebased, needed to pass priv->cfg in ts2phc_nmea_master_create
- setting clock->is_destination can be delayed to a future patch
Changes in v3:
None.
ts2phc.c| 2 +-
ts2phc.h| 1 +
ts2phc_generic_master.c | 2 +-
ts2phc_generic_master.h | 3 ++-
ts2phc_master.c | 10 ++
ts2phc_master.h | 6 --
ts2phc_nmea_master.c| 7 ---
ts2phc_nmea_master.h| 3 ++-
ts2phc_phc_master.c | 32
ts2phc_phc_master.h | 3 ++-
ts2phc_slave.c | 2 +-
11 files changed, 40 insertions(+), 31 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index ca7684b314a4..147133721212 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -268,7 +268,7 @@ int main(int argc, char *argv[])
} else {
pps_type = TS2PHC_MASTER_PHC;
}
- priv.master = ts2phc_master_create(cfg, pps_source, pps_type);
+ priv.master = ts2phc_master_create(, pps_source, pps_type);
if (!priv.master) {
fprintf(stderr, "failed to create master\n");
ts2phc_cleanup();
diff --git a/ts2phc.h b/ts2phc.h
index 43725e9edfdc..f80f11ae77b3 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -7,6 +7,7 @@
#ifndef HAVE_TS2PHC_H
#define HAVE_TS2PHC_H
+#include
#include
#include
#include "servo.h"
diff --git a/ts2phc_generic_master.c b/ts2phc_generic_master.c
index ad4f7f1cf1d7..05dd8f3742fc 100644
--- a/ts2phc_generic_master.c
+++ b/ts2phc_generic_master.c
@@ -47,7 +47,7 @@ static int ts2phc_generic_master_getppstime(struct
ts2phc_master *m,
return 0;
}
-struct ts2phc_master *ts2phc_generic_master_create(struct config *cfg,
+struct ts2phc_master *ts2phc_generic_master_create(struct ts2phc_private *priv,
const char *dev)
{
struct ts2phc_generic_master *master;
diff --git a/ts2phc_generic_master.h b/ts2phc_generic_master.h
index ac0ce4f11cb8..c8fc099ad066 100644
--- a/ts2phc_generic_master.h
+++ b/ts2phc_generic_master.h
@@ -6,9 +6,10 @@
#ifndef HAVE_TS2PHC_GENERIC_MASTER_H
#define HAVE_TS2PHC_GENERIC_MASTER_H
+#include "ts2phc.h"
#include "ts2phc_master.h"
-struct ts2phc_master *ts2phc_generic_master_create(struct config *cfg,
+struct ts2phc_master *ts2phc_generic_master_create(struct ts2phc_private *priv,
const char *dev);
#endif
diff --git a/ts2phc_master.c b/ts2phc_master.c
index 9283580df3fc..4617c4aecbe5 100644
--- a/ts2phc_master.c
+++ b/ts2phc_master.c
@@ -3,25 +3,27 @@
* @note Copyright (C) 2019 Richard Cochran
* @note SPDX-License-Identifier: GPL-2.0+
*/
+#include "ts2phc.h"
#include "ts2phc_generic_master.h"
#include "ts2phc_master_private.h"
#include "ts2phc_nmea_master.h"
#include "ts2phc_phc_master.h"
-struct ts2phc_master *ts2phc_master_create(struct config *cfg, const char *dev,
+struct ts2phc_master *ts2phc_master_create(struct ts2phc_private *priv,
+ const char *dev,
enum ts2phc_master_type type)
{
struct ts2phc_master *master = NULL;
switch (type) {
case TS2PHC_MASTER_GENERIC:
- master = ts2phc_generic_master_create(cfg, dev);
+ master = ts2phc_generic_master_create(priv, dev);
break;
case TS2PHC_MASTER_NMEA:
- master = ts2phc_nmea_master_create(cfg, dev);
+ master = ts2phc_nmea_master_create(priv, dev);
break;
case TS2
[Linuxptp-devel] [PATCH v4 8/9] ts2phc: allow the PHC PPS master to be synchronized
Now that we are registering a clock even for the PPS master when it
supports that (i.e. when it is a PHC), introduce a new API to retrieve
its clock in order to add timestamps to it.
The timestamps are a mere approximation. We configure the kernel to emit
periodic output and then never hear back from that PHC again. We just
assume that it emits periodic output as promised, and that each pulse is
emitted at the beginning of each second. We rely on the slaves to report
an extts event first, and we know who generated that - the master, of
course. So then we proceed to read the master's PHC time, and round that
to what is the most plausible integer second in its time base. We
believe that to be the 'timestamp'. This is fed into the servo algorithm.
The PHC master can be synchronized to the extts events of a PHC slave,
when in automatic mode.
Signed-off-by: Vladimir Oltean
---
Changes in v4:
Add one more paragraph to commit message.
Changes in v3:
Implement ts2phc_master_get_clock() as part of ts2phc_master.c instead
of ts2phc_phc_master.c.
ts2phc.c| 45 -
ts2phc_master.c | 8
ts2phc_master.h | 2 ++
ts2phc_master_private.h | 1 +
ts2phc_phc_master.c | 9 +
5 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/ts2phc.c b/ts2phc.c
index dbe7bf954943..ce2ceb97888a 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -482,6 +482,42 @@ static void ts2phc_synchronize_clocks(struct
ts2phc_private *priv, int autocfg)
}
}
+static int ts2phc_collect_master_tstamp(struct ts2phc_private *priv)
+{
+ struct clock *master_clock;
+ struct timespec master_ts;
+ int err;
+
+ master_clock = ts2phc_master_get_clock(priv->master);
+ /*
+* Master isn't a PHC (it may be a generic or a GPS master),
+* don't error out, just don't do anything. If it doesn't have a PHC,
+* there is nothing to synchronize, which is the only point of
+* collecting its perout timestamp in the first place.
+*/
+ if (!master_clock)
+ return 0;
+
+ err = ts2phc_master_getppstime(priv->master, _ts);
+ if (err < 0) {
+ pr_err("source ts not valid");
+ return err;
+ }
+
+ /*
+* As long as the kernel doesn't support a proper API for reporting
+* a precise perout timestamp, we'll have to use this crude
+* approximation.
+*/
+ if (master_ts.tv_nsec > NS_PER_SEC / 2)
+ master_ts.tv_sec++;
+ master_ts.tv_nsec = 0;
+
+ clock_add_tstamp(master_clock, timespec_to_tmv(master_ts));
+
+ return 0;
+}
+
static void usage(char *progname)
{
fprintf(stderr,
@@ -702,8 +738,15 @@ int main(int argc, char *argv[])
pr_err("poll failed");
break;
}
- if (err > 0)
+ if (err > 0) {
+ err = ts2phc_collect_master_tstamp();
+ if (err) {
+ pr_err("failed to collect master tstamp");
+ break;
+ }
+
ts2phc_synchronize_clocks(, autocfg);
+ }
}
ts2phc_cleanup();
diff --git a/ts2phc_master.c b/ts2phc_master.c
index 4617c4aecbe5..0dc02a103859 100644
--- a/ts2phc_master.c
+++ b/ts2phc_master.c
@@ -38,3 +38,11 @@ int ts2phc_master_getppstime(struct ts2phc_master *master,
struct timespec *ts)
{
return master->getppstime(master, ts);
}
+
+struct clock *ts2phc_master_get_clock(struct ts2phc_master *m)
+{
+ if (m->get_clock)
+ return m->get_clock(m);
+
+ return NULL;
+}
diff --git a/ts2phc_master.h b/ts2phc_master.h
index a7e7186f79a1..6edf8c013af9 100644
--- a/ts2phc_master.h
+++ b/ts2phc_master.h
@@ -51,4 +51,6 @@ void ts2phc_master_destroy(struct ts2phc_master *master);
*/
int ts2phc_master_getppstime(struct ts2phc_master *master, struct timespec
*ts);
+struct clock *ts2phc_master_get_clock(struct ts2phc_master *m);
+
#endif
diff --git a/ts2phc_master_private.h b/ts2phc_master_private.h
index 463a1f003a21..deef1b520a3f 100644
--- a/ts2phc_master_private.h
+++ b/ts2phc_master_private.h
@@ -15,6 +15,7 @@
struct ts2phc_master {
void (*destroy)(struct ts2phc_master *ts2phc_master);
int (*getppstime)(struct ts2phc_master *master, struct timespec *ts);
+ struct clock *(*get_clock)(struct ts2phc_master *m);
};
#endif
diff --git a/ts2phc_phc_master.c b/ts2phc_phc_master.c
index 1a944a960f18..233c95f5c181 100644
--- a/ts2phc_phc_master.c
+++ b/ts2phc_phc_master.c
@@ -83,6 +83,14 @@ static int ts2phc_phc_master_getppstime(struct ts2phc_master
*m,
return clock_gettime(master->clock->clkid, ts);
}
+struct clock *ts2phc_phc_master_get_clock(struc
[Linuxptp-devel] [PATCH v4 4/9] ts2phc: instantiate a pmc agent
This introduces the '-a' option in ts2phc, an option inspired from phc2sys that puts the clocks in "automatic" mode. In this mode, ts2phc listens, as a PMC, to port state change events from ptp4l, and detects which port state machine, if any, has transitioned to PS_SLAVE. That port's clock will become the synchronization master for the hierarchy described by ts2phc. The use case is a multi-switch DSA setup with boundary_clock_jbod, where there is only one grandmaster, connected to one switch's port. The other switches, connected together through a PPS signal, must adapt themselves to this new source of time, while the switch connected to the GM must not be synchronized by ts2phc because it is already synchronized by ptp4l. Graphically, the setup looks like this: +---+ | LS1028A /dev/ptp0 (unused) | | +--+| | | DSA master for Felix|| | |(internal ENETC port 2: eno2))|| | ++--+-+ | | | Felix embedded L2 switch /dev/ptp1 | | | | PPS master, sync slave | | | | +--+ +--+ +--+ | | | | |DSA master for| |DSA master for| |DSA master for| | | | | | SJA1105 1 | | SJA1105 2 | | SJA1105 3 | | | | | |(Felix port 1)| |(Felix port 2)| |(Felix port 3)| | | +--+-+--+---+--+---+--+--+--+ /dev/ptp2/dev/ptp3/dev/ptp4 PPS slave, sync masterPPS slave, sync slave PPS slave, sync slave +---+ +---+ +---+ | SJA1105 switch 1| | SJA1105 switch 2| | SJA1105 switch 3| +-+-+-+-+ +-+-+-+-+ +-+-+-+-+ |sw1p0|sw1p1|sw1p2|sw1p3| |sw2p0|sw2p1|sw2p2|sw2p3| |sw3p0|sw3p1|sw3p2|sw3p3| +-+-+-+-+ +-+-+-+-+ +-+-+-+-+ ^ | GM connected here In the described state, the Felix embedded L2 switch is statically configured to emit a 1PPS signal on a sort of "simplex bus", and SJA1105 switches 1 to 3 are statically configured to record it in their respective time bases. Because the GM is connected to one of the ports of SJA1105 switch 1, that port is in PS_SLAVE and is the source of ts2phc time. So the embedded L2 switch as well as the other SJA1105 switches synchronize to the SJA1105. When the GM migrates to a port belonging to a different PHC (say sw2p0), the PPS distribution tree remains unchanged but the port reported by ptp4l as being in PS_SLAVE changes. So ts2phc must reconfigure itself to use the time of /dev/ptp3 as source, and the other PHC devices turn into sinks. Signed-off-by: Vladimir Oltean --- Changes in v4: - use the new pmc agent API - add more info to the commit message - priv->state_changed can be delayed to a future patch - use enum port_state instead of int Changes in v3: - Added man page and usage verbiage. - Removed some useless curly braces in ts2phc_cleanup(). makefile | 5 +- ts2phc.8 | 15 ts2phc.c | 227 ++- ts2phc.h | 14 +++- 4 files changed, 256 insertions(+), 5 deletions(-) diff --git a/makefile b/makefile index 33e7ca0f038b..6d49b911153e 100644 --- a/makefile +++ b/makefile @@ -68,8 +68,9 @@ phc_ctl: phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o timemaster: phc.o print.o rtnl.o sk.o timemaster.o util.o version.o -ts2phc: config.o clockadj.o hash.o interface.o phc.o print.o $(SERVOS) sk.o \ - $(TS2PHC) util.o version.o +ts2phc: config.o clockadj.o hash.o interface.o msg.o phc.o pmc_agent.o \ + pmc_common.o print.o $(SERVOS) sk.o $(TS2PHC) tlv.o transport.o raw.o \ + udp.o udp6.o uds.o util.o version.o version.o: .version version.sh $(filter-out version.d,$(DEPEND)) diff --git a/ts2phc.8 b/ts2phc.8 index 690c4627f9f2..1a3d2feac1b7 100644 --- a/ts2phc.8 +++ b/ts2phc.8 @@ -26,6 +26,21 @@ A single source may be used to distribute time to one or more PHC devices. .SH OPTIONS .TP +.BI \-a +Adjust the direction of synchronization automatically. The program determines +which PHC should be a source of time and which should be a sink by querying the +port states from the running instance of +.B ptp4l. +Note that using this option, the PPS signal distribution hierarchy still +remains fixed as per the configuration file. This implies that using this +option, a PHC PPS master may become a time sink, and a PPS slave may become a +time source. Other, non-PHC types of PPS masters (generic, NMEA) cannot become +time sinks. Clocks which are not part of +.B ptp4l's +list of por
[Linuxptp-devel] [PATCH v4 6/9] ts2phc: split slave poll from servo loop
It has been explained in previous patches that:
- a ts2phc slave deals with extts events
- a clock deals with synchronization via a servo loop
Therefore, the code for synchronization should not be part of the
implementation of a ts2phc slave. Move it to the main ts2phc.c.
This allows it to be used by a ts2phc master as well (the PHC kind).
Signed-off-by: Vladimir Oltean
---
Changes in v4:
Use bool for boolean types.
Changes in v3:
None.
ts2phc.c | 94 ++-
ts2phc.h | 3 +
ts2phc_slave.c | 201 +++--
3 files changed, 191 insertions(+), 107 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index fe19f11db7d0..5b648d9392c8 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -21,6 +21,9 @@
#include "ts2phc.h"
#include "version.h"
+#define NS_PER_SEC 10LL
+#define SAMPLE_WEIGHT 1.0
+
struct interface {
STAILQ_ENTRY(interface) list;
};
@@ -155,6 +158,30 @@ static struct servo *servo_add(struct ts2phc_private
*priv, struct clock *clock)
return servo;
}
+void clock_add_tstamp(struct clock *clock, tmv_t t)
+{
+ struct timespec ts = tmv_to_timespec(t);
+
+ pr_debug("adding tstamp %ld.%09ld to clock %s",
+ts.tv_sec, ts.tv_nsec, clock->name);
+ clock->last_ts = t;
+ clock->is_ts_available = 1;
+}
+
+static int clock_get_tstamp(struct clock *clock, tmv_t *ts)
+{
+ if (!clock->is_ts_available)
+ return 0;
+ clock->is_ts_available = 0;
+ *ts = clock->last_ts;
+ return 1;
+}
+
+static void clock_flush_tstamp(struct clock *clock)
+{
+ clock->is_ts_available = 0;
+}
+
struct clock *clock_add(struct ts2phc_private *priv, const char *device)
{
clockid_t clkid = CLOCK_INVALID;
@@ -303,6 +330,64 @@ static int auto_init_ports(struct ts2phc_private *priv)
return 0;
}
+static void ts2phc_synchronize_clocks(struct ts2phc_private *priv)
+{
+ struct timespec source_ts;
+ tmv_t source_tmv;
+ struct clock *c;
+ int valid, err;
+
+ err = ts2phc_master_getppstime(priv->master, _ts);
+ if (err < 0) {
+ pr_err("source ts not valid");
+ return;
+ }
+ if (source_ts.tv_nsec > NS_PER_SEC / 2)
+ source_ts.tv_sec++;
+ source_ts.tv_nsec = 0;
+
+ source_tmv = timespec_to_tmv(source_ts);
+
+ LIST_FOREACH(c, >clocks, list) {
+ int64_t offset;
+ double adj;
+ tmv_t ts;
+
+ valid = clock_get_tstamp(c, );
+ if (!valid) {
+ pr_debug("%s timestamp not valid, skipping", c->name);
+ continue;
+ }
+
+ offset = tmv_to_nanoseconds(tmv_sub(ts, source_tmv));
+
+ if (c->no_adj) {
+ pr_info("%s offset %10" PRId64, c->name,
+ offset);
+ continue;
+ }
+
+ adj = servo_sample(c->servo, offset, tmv_to_nanoseconds(ts),
+ SAMPLE_WEIGHT, >servo_state);
+
+ pr_info("%s offset %10" PRId64 " s%d freq %+7.0f",
+ c->name, offset, c->servo_state, adj);
+
+ switch (c->servo_state) {
+ case SERVO_UNLOCKED:
+ break;
+ case SERVO_JUMP:
+ clockadj_set_freq(c->clkid, -adj);
+ clockadj_step(c->clkid, -offset);
+ break;
+ case SERVO_LOCKED:
+ case SERVO_LOCKED_STABLE:
+ clockadj_set_freq(c->clkid, -adj);
+ break;
+ }
+ }
+}
+
static void usage(char *progname)
{
fprintf(stderr,
@@ -501,11 +586,18 @@ int main(int argc, char *argv[])
}
while (is_running()) {
+ struct clock *c;
+
+ LIST_FOREACH(c, , list)
+ clock_flush_tstamp(c);
+
err = ts2phc_slave_poll();
- if (err) {
+ if (err < 0) {
pr_err("poll failed");
break;
}
+ if (err > 0)
+ ts2phc_synchronize_clocks();
}
ts2phc_cleanup();
diff --git a/ts2phc.h b/ts2phc.h
index cd9912d78a1f..156e41998100 100644
--- a/ts2phc.h
+++ b/ts2phc.h
@@ -28,6 +28,8 @@ struct clock {
enum servo_state servo_state;
char *name;
bool no_adj;
+ bool is_ts_available;
+ tmv_t last_ts;
};
struct port {
@@ -50,6 +52,7 @@ struct ts2phc_private {
};
struct clock *clock_add(struct ts2phc_private *priv, const char *device);
+void clock_add_tstamp(struc
[Linuxptp-devel] [PATCH v4 7/9] ts2phc: reconfigure sync direction by subscribing to ptp4l port events
Monitor the port state change events from ptp4l, and use that
information to determine the "source" clock.
Then synchronize all other clocks in our list to that source, by feeding
into their respective servo loop an offset equal to the delta between
their timestamp and the timestamp of the source clock. All timestamps
are representative of the same event, which is the most recent perout
pulse of the ts2phc master.
Signed-off-by: Vladimir Oltean
---
Changes in v4:
Use bool for boolean types.
Changes in v3:
None.
ts2phc.c| 130
ts2phc.h| 2 +
ts2phc_phc_master.c | 1 +
ts2phc_slave.c | 1 +
4 files changed, 122 insertions(+), 12 deletions(-)
diff --git a/ts2phc.c b/ts2phc.c
index 5b648d9392c8..dbe7bf954943 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -125,6 +125,7 @@ static int ts2phc_recv_subscribed(void *context, struct
ptp_message *msg,
state = clock_compute_state(priv, clock);
if (clock->state != state || clock->new_state) {
clock->new_state = state;
+ priv->state_changed = true;
}
}
return 1;
@@ -326,33 +327,126 @@ static int auto_init_ports(struct ts2phc_private *priv)
LIST_FOREACH(clock, >clocks, list) {
clock->new_state = clock_compute_state(priv, clock);
}
+ priv->state_changed = true;
return 0;
}
-static void ts2phc_synchronize_clocks(struct ts2phc_private *priv)
+static void ts2phc_reconfigure(struct ts2phc_private *priv)
+{
+ struct clock *c, *src = NULL, *last = NULL;
+ int src_cnt = 0, dst_cnt = 0;
+
+ pr_info("reconfiguring after port state change");
+ priv->state_changed = false;
+
+ LIST_FOREACH(c, >clocks, list) {
+ if (c->new_state) {
+ c->state = c->new_state;
+ c->new_state = 0;
+ }
+
+ switch (c->state) {
+ case PS_FAULTY:
+ case PS_DISABLED:
+ case PS_LISTENING:
+ case PS_PRE_MASTER:
+ case PS_MASTER:
+ case PS_PASSIVE:
+ if (!c->is_destination) {
+ pr_info("selecting %s for synchronization",
+ c->name);
+ c->is_destination = true;
+ }
+ dst_cnt++;
+ break;
+ case PS_UNCALIBRATED:
+ src_cnt++;
+ break;
+ case PS_SLAVE:
+ src = c;
+ src_cnt++;
+ break;
+ default:
+ break;
+ }
+ last = c;
+ }
+ if (dst_cnt >= 1 && !src) {
+ priv->source = last;
+ priv->source->is_destination = false;
+ /* Reset to original state in next reconfiguration. */
+ priv->source->new_state = priv->source->state;
+ priv->source->state = PS_SLAVE;
+ pr_info("no source, selecting %s as the default clock",
+ last->name);
+ return;
+ }
+ if (src_cnt > 1) {
+ pr_info("multiple source clocks available, postponing sync...");
+ priv->source = NULL;
+ return;
+ }
+ if (src_cnt > 0 && !src) {
+ pr_info("source clock not ready, waiting...");
+ priv->source = NULL;
+ return;
+ }
+ if (!src_cnt && !dst_cnt) {
+ pr_info("no PHC ready, waiting...");
+ priv->source = NULL;
+ return;
+ }
+ if (!src_cnt) {
+ pr_info("nothing to synchronize");
+ priv->source = NULL;
+ return;
+ }
+ src->is_destination = false;
+ priv->source = src;
+ pr_info("selecting %s as the source clock", src->name);
+}
+
+static void ts2phc_synchronize_clocks(struct ts2phc_private *priv, int autocfg)
{
- struct timespec source_ts;
tmv_t source_tmv;
struct clock *c;
int valid, err;
- err = ts2phc_master_getppstime(priv->master, _ts);
- if (err < 0) {
- pr_err("source ts not valid");
- return;
- }
- if (source_ts.tv_nsec > NS_PER_SEC / 2)
- source_ts.tv_sec++;
- source_ts.tv_nsec = 0;
+ if (autocfg) {
+ if (!priv->source) {
+ pr_debug
[Linuxptp-devel] [PATCH v4 1/9] ts2phc: create a private data structure
Eliminate the ad-hoc use of global variables in the ts2phc program by
introducing one data structure that incorporates them. This might make
the code more understandable to people coming from a kernel background,
since it resembles the type of data organization used there. It is also
now closer to the data organization of phc2sys, a similar program in
both purpose and implementation.
The reason why this is needed has to do with the ts2phc polymorphism for
a PPS master.
In the next patches, PPS masters will expose a struct clock, which will
be synchronized from the main ts2phc.c.
Not all PPS masters will expose a clock, only the PHC kind will. So the
current object encapsulation model needs to be loosened up little bit,
because the main ts2phc.c needs to synchronize a list of clocks, list
which is populated by the slaves and the masters which are capable of
being synchronized.
So instead of having the translation modules of ts2phc communicate
through global variables, let's make struct ts2phc_private the common
working space for the entire program, which is a paradigm that is more
natural.
Signed-off-by: Vladimir Oltean
Reviewed-by: Jacob Keller
---
Changes in v4:
- Rebased, no conflicts
Changes in v3:
- Amended commit message.
- Replaced full license header in ts2phc.h with simple SPDX.
ts2phc.c | 66
ts2phc.h | 23 +
ts2phc_slave.c | 136 -
ts2phc_slave.h | 10 ++--
4 files changed, 139 insertions(+), 96 deletions(-)
create mode 100644 ts2phc.h
diff --git a/ts2phc.c b/ts2phc.c
index bc410415dfc6..67df5a532559 100644
--- a/ts2phc.c
+++ b/ts2phc.c
@@ -11,23 +11,20 @@
#include "config.h"
#include "interface.h"
#include "print.h"
-#include "ts2phc_master.h"
-#include "ts2phc_slave.h"
+#include "ts2phc.h"
#include "version.h"
struct interface {
STAILQ_ENTRY(interface) list;
};
-static void ts2phc_cleanup(struct config *cfg, struct ts2phc_master *master)
+static void ts2phc_cleanup(struct ts2phc_private *priv)
{
- ts2phc_slave_cleanup();
- if (master) {
- ts2phc_master_destroy(master);
- }
- if (cfg) {
- config_destroy(cfg);
- }
+ ts2phc_slave_cleanup(priv);
+ if (priv->master)
+ ts2phc_master_destroy(priv->master);
+ if (priv->cfg)
+ config_destroy(priv->cfg);
}
static void usage(char *progname)
@@ -56,8 +53,8 @@ static void usage(char *progname)
int main(int argc, char *argv[])
{
int c, err = 0, have_slave = 0, index, print_level;
- struct ts2phc_master *master = NULL;
enum ts2phc_master_type pps_type;
+ struct ts2phc_private priv = {0};
char *config = NULL, *progname;
const char *pps_source = NULL;
struct config *cfg = NULL;
@@ -68,7 +65,7 @@ int main(int argc, char *argv[])
cfg = config_create();
if (!cfg) {
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup();
return -1;
}
@@ -81,14 +78,14 @@ int main(int argc, char *argv[])
switch (c) {
case 0:
if (config_parse_option(cfg, opts[index].name, optarg))
{
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup();
return -1;
}
break;
case 'c':
if (!config_create_interface(optarg, cfg)) {
fprintf(stderr, "failed to add slave\n");
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup();
return -1;
}
have_slave = 1;
@@ -99,7 +96,7 @@ int main(int argc, char *argv[])
case 'l':
if (get_arg_val_i(c, optarg, _level,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX)) {
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup();
return -1;
}
config_set_int(cfg, "logging_level", print_level);
@@ -116,29 +113,29 @@ int main(int argc, char *argv[])
case 's':
if (pps_source) {
fprintf(stderr, "too many PPS sources\n");
- ts2phc_cleanup(cfg, master);
+ ts2phc_cleanup();
return -1;
}
pps_source = optarg;
break;
case 'v':
- ts2phc_cleanup(cfg, master);
+
Re: [Linuxptp-devel] [PATCH 2/2] phc2sys: fix BC sync fault when port in uncalibrated state
On Thu, Dec 24, 2020 at 06:36:19PM +0200, Grygorii Strashko wrote:
> The ptp4l/phc2sys BC can stuck in sync fault while port is in transitional
> UNCALIBRATED state:
> MASTER -> UNCALIBRATED
> UNCALIBRATED -> SLAVE
> in the later case state change sequence is
> UNCALIBRATED -> SLAVE -> SYNCHRONIZATION_FAULT -> UNCALIBRATED.
> Once issue happens the BC or GM has to be restarted to recover.
>
> To trigger an issue the GM clock has to be restarted few times.
>
> The cause of the issue is that both ptp4l and phc2sys could be syncing the
> same PHC clock while Port is in UNCALIBRATED state. This happens because in
> reconfigure() the check [1]
>
> [1] if (dst_cnt > 1 && !src) {
> ...
> if (src_cnt > 1) {
> ...
> [2] if (src_cnt > 0 && !src) {
> ...
> if (!src_cnt && !dst_cnt) {
> ...
>
> is defined first and selects last clock in the clocks list (usually first
> defined in ptp4l cfg file) as the master clock even if there is some other
> clock(Port) in transitional UNCALIBRATED state. As result, phc2sys will
> start syncing the same clock as ptp4l, and ptp4l has no chances to
> stabilize Port in UNCALIBRATED state.
> Moreover, check [1] bypasses most of follow up checks unless CLOCK_REALTIME
> is allowed to be master clock.
>
> The original commit 46a0b281b915 ("phc2sys: autoconfiguration") gave ptp4l
> chance to proceed by introducing check [2]. But follow up commit
> 2ab2fbbdda86 ("phc2sys: default to the first clock in automatic mode.")
> introduced check [1] and so this issue.
>
> To fix an issue the check [2] is moved down which makes phc2sys to wait for
> ptp4l to settle.
>
> Signed-off-by: Grygorii Strashko
> ---
I don't think Richard does merge commits, so your logs from the cover
letter are going to get lost, save for the few who will search for the
commit on the mailing archives. The patch is ok (the intention probably
was to select a default source clock only when there were no candidates
at all), but I think the commit message leaves the reader wanting to
know how come the clock with PS_UNCALIBRATED can ever reach inside the
priv->dst_clocks list. The clue is only given by the logs that you
posted in the cover letter, so I would suggest either copying them here
too, or just verbally explaining better.
> phc2sys.c | 28 +++-
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/phc2sys.c b/phc2sys.c
> index b15127d..fd80afd 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -419,19 +419,6 @@ static void reconfigure(struct phc2sys_private *priv)
> }
> last = c;
> }
> - if (dst_cnt > 1 && !src) {
> - if (!rt || rt->dest_only) {
> - priv->master = last;
> - /* Reset to original state in next reconfiguration. */
> - priv->master->new_state = priv->master->state;
> - priv->master->state = PS_SLAVE;
> - if (rt)
> - rt->state = PS_SLAVE;
> - pr_info("no source, selecting %s as the default clock",
> - last->device);
> - return;
> - }
> - }
> if (src_cnt > 1) {
> pr_info("multiple master clocks available, postponing sync...");
> priv->master = NULL;
> @@ -447,6 +434,21 @@ static void reconfigure(struct phc2sys_private *priv)
> priv->master = NULL;
> return;
> }
> +
> + if (dst_cnt > 1 && !src) {
> + if (!rt || rt->dest_only) {
> + priv->master = last;
> + /* Reset to original state in next reconfiguration. */
> + priv->master->new_state = priv->master->state;
> + priv->master->state = PS_SLAVE;
> + if (rt)
> + rt->state = PS_SLAVE;
> + pr_info("no source, selecting %s as the default clock",
> + last->device);
> + return;
> + }
> + }
> +
> if ((!src_cnt && (!rt || rt->dest_only)) ||
> (!dst_cnt && !rt)) {
> pr_info("nothing to synchronize");
> --
> 2.17.1
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 0/2] phc2sys: fix BC sync fault during port master->uncalibrated transition
Hi Grygorii,
On Thu, Dec 24, 2020 at 06:36:17PM +0200, Grygorii Strashko wrote:
> Hi
>
> I've found that it's pretty easy to put ptp4l/phc2sys BC in sync fault state
> while port is in transitional UNCALIBRATED state:
> MASTER -> UNCALIBRATED
> UNCALIBRATED -> SLAVE
> in the later case state change sequence is
> UNCALIBRATED -> SLAVE -> SYNCHRONIZATION_FAULT -> UNCALIBRATED.
> Once issue happens the BC or GM has to be restarted to recover (not always
> helps).
>
> To trigger an issue the GM clock has to be restarted few times.
>
> The cause of the issue is that both ptp4l and phc2sys could be syncing the
> same
> PHC clock while Port is in UNCALIBRATED state. This happens because
> in reconfigure() the check [1]
>
> [1] if (dst_cnt > 1 && !src) {
> ...
> if (src_cnt > 1) {
> ...
> [2] if (src_cnt > 0 && !src) {
> ...
> if (!src_cnt && !dst_cnt) {
> ...
>
> is defined first and selects last clock in the clocks list (usually first
> defined
> in ptp4l cfg file) as the master clock even if there is some other
> clock(Port) is
> in transitional UNCALIBRATED state. As result, phc2sys will start syncing the
> same clock as ptp4l, and ptp4l has no chances to stabilize Port in
> UNCALIBRATED
> state. Moreover, check [1] bypasses most of follow up checks unless
> CLOCK_REALTIME is allowed to be master clock.
>
> The original commit 46a0b281b915 ("phc2sys: autoconfiguration") gave ptp4l a
> chance to proceed by introducing check [2]. But follow up commit 2ab2fbbdda86
> ("phc2sys: default to the first clock in automatic mode.") introduced check
> [1]
> and so this issue.
>
> To fix an issue the check [2] is moved down which makes phc2sys to wait for
> ptp4l to settle.
>
> Patch 1 - add debug prints when clock state is changed
> Patch 2 - fixes an issue
>
> BC configuration (jbod):
> - eth1, phc index 0
> - pru10, phc index 1
> - pru11, phc index 1
> - pru20, phc index 2 <-- GM
> - eth21, phc index 2
>
> BC started as:
> sleep
> ./linuxptp/ptp4l -mf abb_bc.cfg -l5 &
> sleep 2
> ./linuxptp/phc2sys -a -S 0.2 -L 0 -m -l7 -u$1 &
>
> Example log:
> ptp4l[688.126]: port 4: UNCALIBRATED to SLAVE on MASTER_CLOCK_SELECTED
> phc2sys[688.294]: port 5cf821.fffe.3b0305-4 changed state
> phc2sys[688.295]: reconfiguring after port state change
> phc2sys[688.295]: selecting pru21 for synchronization
> phc2sys[688.295]: pru20: state change UNCALIBRATED -> SLAVE
> phc2sys[688.295]: selecting pru11 for synchronization
> phc2sys[688.295]: skipping pru10: pru11 has the same clock and is already
> selected
> phc2sys[688.296]: eth1: state change SLAVE -> MASTER
> phc2sys[688.296]: selecting eth1 for synchronization
> phc2sys[688.296]: selecting pru20 as the master clock
> ^^ GM detected properly and BC sync switched to pru20 as master
>
> phc2sys[688.296]: eth1 rms 21 max 21 freq +22755 +/- 0 delay 26781 +/- 0
> phc2sys[688.296]: pru11 rms 134 max 134 freq +22664 +/- 0 delay 5200 +/- 0
> phc2sys[689.297]: eth1 rms 554 max 554 freq +23376 +/- 0 delay 26135 +/- 0
> ...
> phc2sys[693.299]: pru11 rms 205 max 205 freq +22725 +/- 0 delay 5135 +/- 0
> ptp4l[694.126]: port 4: SLAVE to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
> ptp4l[694.126]: selected local clock 5cf821.fffe.3b0305 as best master
> ptp4l[694.126]: port 1: assuming the grand master role
> ptp4l[694.126]: port 2: assuming the grand master role
> ptp4l[694.126]: port 3: assuming the grand master role
> ptp4l[694.126]: port 4: assuming the grand master role
> ptp4l[694.126]: port 5: assuming the grand master role
> phc2sys[694.299]: port 5cf821.fffe.3b0305-4 changed state
> phc2sys[694.300]: reconfiguring after port state change
> phc2sys[694.300]: selecting pru21 for synchronization
> phc2sys[694.300]: pru20: state change SLAVE -> MASTER
> phc2sys[694.300]: skipping pru20: pru21 has the same clock and is already
> selected
> phc2sys[694.300]: selecting pru11 for synchronization
> phc2sys[694.300]: skipping pru10: pru11 has the same clock and is already
> selected
> phc2sys[694.300]: selecting eth1 for synchronization
> phc2sys[694.300]: no source, selecting eth1 as the default clock
> ^^ GM stopped and BC sync switched to eth1 as master
>
> phc2sys[694.301]: pru11 rms 582 max 582 freq +23450 +/- 0 delay 26930 +/- 0
> phc2sys[694.303]: pru21 rms 686 max 686 freq +23427 +/- 0 delay 26929 +/- 0
> ...
> phc2sys[700.310]: pru11 rms 214 max 214 freq +22603 +/- 0 delay 26844 +/- 0
> phc2sys[700.311]: pru21 rms 111 max 111 freq +22310 +/- 0 delay 26485 +/- 0
> ptp4l[701.130]: selected best master clock 5051a9.fffe.fc7697
> ptp4l[701.130]: port 4: MASTER to UNCALIBRATED on RS_SLAVE
> phc2sys[701.311]: port 5cf821.fffe.3b0305-4 changed state
> phc2sys[701.313]: reconfiguring after port state change
> phc2sys[701.313]: selecting pru21 for synchronization
> phc2sys[701.313]: pru20: state change MASTER -> UNCALIBRATED
> phc2sys[701.313]: selecting pru11 for synchronization
> phc2sys[701.313]: skipping pru10: pru11 has the same
Re: [Linuxptp-devel] [PATCH] tc: introduce 1-step for E2E & P2P Transparent Clocks
On Thu, Dec 03, 2020 at 05:26:10PM -0800, Richard Cochran wrote:
> On Fri, Dec 04, 2020 at 03:02:58AM +0200, Vladimir Oltean wrote:
> > On Thu, Dec 03, 2020 at 04:34:46PM -0800, Richard Cochran wrote:
> > > > Actually, for dpaa2, the one-step timestamping procedure just does:
> > > >
> > > > correctionField += t_TX - t_passed_inband
> > > >
> > > > where the driver populates:
> > > > t_passed_inband = originTimestamp = approximate PTP time of software
> > > > packet delivery
> > >
> > > ptp4l sets to originTimestamp zero!
> >
> > And the driver happily overwrites it, I don't understand what the issue
> > is. If not the driver, the MAC would, anyway.
>
> huh?
>
> Doesn't it looks strang if the originTimestamp arrives at the client
> with zero.
>
> Or maybe I didn't understand what the dpaa2 driver/HW does?
>
> Does the driver populate the originTimestamp?
Too much confusion, why don't I just show you the code.
if (skb->cb[0] == TX_TSTAMP_ONESTEP_SYNC) {
if (dpaa2_eth_ptp_parse(skb, , , ,
, ) ||
// offset1 points to correctionField, offset2 to originTimestamp
msgtype != PTP_MSGTYPE_SYNC || twostep) {
WARN_ONCE(1, "Bad packet for one-step timestamping\n");
return;
}
/* Mark the frame annotation status as valid */
frc = dpaa2_fd_get_frc(fd);
dpaa2_fd_set_frc(fd, frc | DPAA2_FD_FRC_FASV);
/* Mark the PTP flag for one step timestamping */
fas = dpaa2_get_fas(buf_start, true);
fas->status = cpu_to_le32(DPAA2_FAS_PTP);
dpaa2_ptp->caps.gettime64(_ptp->caps, );
// driver reads current PTP time here
ns = dpaa2_get_ts(buf_start, true);
*ns = cpu_to_le64(timespec64_to_ns() /
// and puts it into the frame annotation area,
DPAA2_PTP_CLK_PERIOD_NS);
// a memory region that is passed in-band with the packet
/* Update current time to PTP message originTimestamp field */
ns_to_ptp_tstamp(_timestamp, le64_to_cpup(ns));
data = skb_mac_header(skb);
*(__be16 *)(data + offset2) = htons(origin_timestamp.sec_msb);
*(__be32 *)(data + offset2 + 2) =
htonl(origin_timestamp.sec_lsb);
*(__be32 *)(data + offset2 + 6) = htonl(origin_timestamp.nsec);
// it also puts the same value in the originTimestamp field,
// therefore making the two equal
cfg.en = 1;
cfg.ch_update = udp;
cfg.offset = offset1;
cfg.peer_delay = 0;
if (dpni_set_single_step_cfg(priv->mc_io, 0, priv->mc_token,
// then the MAC is told to apply one-step timestamping for this frame
))
WARN_ONCE(1, "Failed to set single step register");
}
Or, in short:
> > > > the driver populates:
> > > > t_passed_inband = originTimestamp = approximate PTP time of software
> > > > packet delivery
This is like, pure C syntax.
When you write a = b = 1, what happens is that the "1" expression gets
written in the left-hand side variables a and b.
the t_passed_inband doesn't _have_ to be equal to the originTimestamp.
It's just that this is how the driver sets things up so that, in the
end, the sum between the originTimestamp and the correctionField
contains the precise t_TX, given the way that this MAC timestamps using
one-step.
The client sees just an approximate originTimestamp, the correctionField
is what makes it precise. Here's what the standard says:
9.5.9.3 One-step clocks
The originTimestamp field of the Sync message shall be an estimate no worse
than ±1 s of the
excluding any fractional nanoseconds.
My entire point was that I hope we won't end up with two different ways
to request the one-step TX timestamping of a Sync message. One way for
the termination scenario, one for the forwarding scenario.
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] tc: introduce 1-step for E2E & P2P Transparent Clocks
On Thu, Dec 03, 2020 at 04:34:46PM -0800, Richard Cochran wrote: > > Actually, for dpaa2, the one-step timestamping procedure just does: > > > > correctionField += t_TX - t_passed_inband > > > > where the driver populates: > > t_passed_inband = originTimestamp = approximate PTP time of software packet > > delivery > > ptp4l sets to originTimestamp zero! And the driver happily overwrites it, I don't understand what the issue is. If not the driver, the MAC would, anyway. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] tc: introduce 1-step for E2E & P2P Transparent Clocks
On Thu, Dec 03, 2020 at 05:35:09PM +0100, Volodymyr Bendiuga wrote: > Hi Vladimir, > > > How? The egress port has no idea what t1 was. Where is t1 recorded? > > The hardware we use, Marvell Alaska 88e1548P phy, has a couple of > possible solutions to our given problem, that is to say, updating the > residence time (t2(egress) - t1(ingress)). The one we have chosen, > does the following if configured for TC mode: > 1. SYNC packet arrives to HW on port 1, t1 is recorded, and, > placed into reserved area of the packet > 2. SYNC packet travels up towards linuxptp > 3.a If we run in P2P mode, uplink delay is added to packet’s > correction field, and the packet is forwarded. > 3.b If we run in E2E mode, the packet is simply forwarded. > 4. HW receives SYNC packet on port 2, t1 is read from packets > reserved area, t2 is taken, residence time is calculated > (t2 - t1) and added to correction field. Packet is sent. So it works almost like dpaa2, except that your hardware is clever enough to search for t1 where it knows it put it, whereas dpaa2 has the concept of a t_passed_inband which can be supplied by the driver from any source, including from the reserved fields. So dpaa2 is more generic than your Marvell PHY. > Since it is common sense that the standard describes the behaviour and > not implementation, the HW we use, implements in its own peculiar way, > the solution to our problem. Different hardware may have contrasting > solution, implemented in its own mysterious way, but as long as the > result is up to what the standard has imposed, the same generic > solution in linuxptp would work. So, coming back to my comment about boundary_clock_jbod=1. The generic solution will not work if you have a one-step TC build out of a Marvell 88E1548P on ingress and a TI PHYTER on egress. They both have the same idea, but it's the specifics that kill the implementation. We need to specify very clearly where t1 should be kept. It's almost as if we should declare some of the reserved fields of the PTP header as "implementation specific for linuxptp". This would be hard ABI that is set in stone. I think it's clear that the correctionField is not going to be enough for this. > Generic solution will not work for hardware X, if hardware X is unable > to calculate residence time in 1-step; but let’s remember that 1-step > residence time calculation is a requirement enforced by the standard. > Any deviation in HW from the standard - is a complication in SW. Come on... > Here we see 2 big categories of TCs: > 1. Hardware-oriented > 2. Software-assisted > > From this we may draw a simple conclusion, namely, that only > software-assisted TC’s are qualified for this patch, since they rely > on linuxptp in general. > > We may further split software-assisted TCs into 2 general sub-categories: > 1. TCs that fully comply with 1-step directives of IEEE 1588 V2 standard > 2. TCs that only partially comply with 1-step directives of IEEE 1588 > V2 standard > > If we start with the second sub-category, we see a plethora of > different HW, each of which, or at least majority, deserve support in > linuxptp. Why would linuxptp even attempt to drive hardware that is not fully compliant to the requirements of IEEE 1588? I don't understand. > They do not have this support yet, and to get it they would > need to patch linuxptp, asking for assistance. Some hardware families > would need a few patches, specific to their platform. Other families > would need another set of patches, to solve their problem, and so on. > This scenario is totally understandable, as it mirrors the reality > that we see every day. > > Now, if we take a look into first sub-category, we perhaps do not see > many examples there yet, but the number is growing. It is this > sub-category given patch intends to support. Regardless of what > hardware solutions different vendors might come up with, as long as > their solutions comply with, not the proposed patch, but the 1-step > directives of IEEE 1588 V2 standard, they are good to go with > linuxptp. I am absolutely confused how you declare that a software-assisted TC falls in bucket 1 (fully compliant with the spec) or bucket 2 (partly compliant with the spec). Please give an example. > Given all that, I do not see any reason for new APIs or additional > features, since everything is already in place. Proposed patch simply > adds a few additional check-points that are used to skip certain > 2-step code blocks. I was actually surprised how little code was > necessary to make 1-step E2E and P2P TC work with linuxptp. Me and my > colleague have been running it for a number of days now, and are happy > with the results. Wrong. Nothing is in place. Not even the convention. And no driver uses a convention that doesn't yet exist, of course. ___ Linuxptp-devel mailing list
Re: [Linuxptp-devel] [PATCH] tc: introduce 1-step for E2E & P2P Transparent Clocks
On Thu, Dec 03, 2020 at 05:52:24AM -0800, Richard Cochran wrote: > On Thu, Dec 03, 2020 at 01:22:35AM +0200, Vladimir Oltean wrote: > > > So now I will speculate a bit, and assume that the hardware behavior you > > need is for the port 1 to subtract the t1 RX timestamp from the Sync > > packet's correctionField, then the port 2 to add the t2 TX timestamp to > > the correctionField. And for the hardware to send the Sync message to > > the CPU and only to the CPU. This is the only situation that I can see, > > where the residence time would be contained into the Sync message at the > > end, and it would be forwarded by software. > > > > Let me ask you based on what criterion do you call this generic? > > Good question. > > In principle, I wouldn't mind supporting 1-step TC in ptp4l, but the > implementation would have to work with the currently available 1-step > HW. If there are different HW mechanisms, then these should be > discoverable by ptp4l via ethtool/etc in order to let the ptp4l > perform the needed adjustments. > > For example, the phyter can support 1-step E2E TC in the following way. > > While the specification calls for adding the residence time to the > correctionField, the same results may be obtained by adding the > incoming originTimestamp to the correctionField and subtracting > the ingress timestamp. Upon transmission, the egress timestamp > for the sync message will automatically be inserted into the > originTimestamp field. The processor should set: > > correctionField = correctionField + originTimestamp - > sync_ingress_timestamp > originTimestamp = 0 > > Using one-step operation, the PHY will automatically set: > > originTimestamp = sync_egress_timestamp > > Since the correctionField only supports a 16-bit seconds field, > this only works if the local PTP clock for the TC is synchronized > within 215 seconds of the master clock. Before forwarding the Isn't this game over already? Who guarantees that the TC will be synchronized within 215 seconds of the master, considering that the spec doesn't even require the TC to synchronize to the GM in the first place, just syntonize? What if the TC doesn't even have a local persistent source of time, like my LS1021A-TSN board doesn't, and always boots in Jan 1970... This solution sounds to me like: How to synchronize two devices using a one-step IEEE 1588 transparent clock --- Use NTP. > first sync message, the transparent clock should first set the PTP > clock time and then modify the ingress time-stamp based on the > adjusted time. > > - TI Application Report AN-1838 > > This method will also work for any 1-step HW that clobbers the > Sync.originTimestamp field with the egress time stamp. I'm willing to > bet that the existing HW, > > - drivers/net/ethernet/cadence/macb_main.c > - drivers/net/ethernet/freescale/dpaa2/ > - drivers/net/ethernet/microchip/lan743x_ptp.c > - drivers/net/ethernet/mscc/ocelot.c ack for ocelot. Here's an excerpt from the documentation for one-step: Origin PTP: REW_OP[2:0] = 9. The time of day at the frame's departure time is written into the origin time stamp field in the PTP frame. > - drivers/net/phy/dp83640.c > - drivers/net/phy/mscc/mscc_ptp.c > > all do exactly this! Actually, for dpaa2, the one-step timestamping procedure just does: correctionField += t_TX - t_passed_inband where the driver populates: t_passed_inband = originTimestamp = approximate PTP time of software packet delivery and where t_TX is, of course, the precise hardware timestamp. So for this hardware, the hardware will only update the correctionField, and the driver is wired up to clobber the originTimestamp in software. In fact there is an entire class of hardware that has the concept of a t_passed_inband, the ksz9477 is one more such example. The "natural" way that this was designed to be used, for example for Sync forwarding in a one-step TC setup, is to set t_passed_inband = t1 (from the RX timestamp on the ingress port). Then, since t_TX is t2, the correctionField is updated with (t2 - t1). It's a bit odd that we would be forcing unneeded limitations to hardware that can do better. > > In order to do its job properly, the sja1105 one-step TC must be left > > alone to do its job of forwarding the packet autonomously and updating > > the residence time in the process. But not forcing the switch to send > > the Sync message to the CPU just for the CPU to send it back, because > > that would mean that the switch adds two small residence times, and the > > big one (the software latency) is completely missing from the calculation. > &g
Re: [Linuxptp-devel] [PATCH] tc: introduce 1-step for E2E & P2P Transparent Clocks
Hi Volodymyr,
On Wed, Dec 02, 2020 at 05:31:37PM +0100, Volodymyr Bendiuga wrote:
> Hi Vladimir, and thank you for extra-well formulated question.
>
> Actually, when I was drafting an answer to your question, I realized I
> had missed one very important point in the patch, namely, that SYNC
> packets in P2P mode should not simply be forwarded, but port’s peer
> delay should be added to the correction field prior to forwarding. I
> apologize for the confusion I may have unintentionally created.
>
> This is what I have missed in the patch in tc_fwd_event() function code-wise:
> if ((q->timestamping >= TS_ONESTEP) && (msg_type(msg) == SYNC))
> {
> corr = tmv_to_TimeInterval(q->peer_delay);
> corr += q->asymmetry;
> msg->header.correction += host2net64(corr);
> }
> cnt = transport_send(p->trp, >fda, TRANS_DEFER_EVENT, msg);
>
> I will update the patch and send it anew.
Receiving thanks for the question, or further code, is not really what I
was expecting, but rather a walk through the life of the Sync packet as
it traverses Node A, the linuxptp one-step TC, and then finally Node B.
I am not at the stage where I could look at code. I don't yet understand
what the hardware is supposed to do in your proposal, and how that is
going to do the job.
>
> Taking into account your diagram, P2P-TC calculates uplink delay
> between its port 1 and Node A, by means of PD_REQ/PD_RESP messages.
> This delay is then stored in linuxptp (q->peer_delay). In your case we
> suppose it is 800 ns. Downstream link delay (port 2 -> Node B) is
> disregarded by P2P TC, since Node B will use it on its side (over
> there it will be called uplink delay), upon reception of SYNC packet.
>
> When SYNC packet arrives to TC, we add uplink ports peer delay
> to correction field and forward it. TC’s port 2 will calculate
> residence time and add it to correction field. Here, the travel
> path of SYNC packet, from its ingress at t1, up to linuxptp and
> down back to driver, will be resolved in port 2 upon egress when
> t2 is taken ((t2 - t1) -> add to correction field.
How? The egress port has no idea what t1 was. Where is t1 recorded?
>
> Upon reception of the SYNC packet, Node B will have everything
> it needs to calculate the offset: origin time and correction
> value from SYNC packet, and its own uplink delay (700 ns).
> Nothing really special going on here, just regular workflow.
Yeah, ok, forget about the link delay, that is only 1500 ns in total in
my example, or 2.3% of the total latency that the TC must account for.
It is not that part that I'm interested in. I want to understand how
your proposal makes (t2 - t1) magically land into the correctionField of
the Sync message. What does the hardware need to do to make that happen.
> As for store-and-forward: unless you can switch it off, like
> placing your hw in cut-through mode, you may have to compensate
> for it in link asymmetry, but that depends on you HW design.
This really doesn't matter. The store-and-forward latency is included in
the (t2 - t1) measurement, so as soon as t1 and t2 are taken as timestamps
at MAC layer, the entire residence time should be accounted for. But
how, that is the question. I've read your emails a few times and I do
not understand this.
> I guess my explanation is totally redundant, since I’ve revealed the
> missing code … . Sorry once again.
No it isn't. I find it is still rather lacking. You mentioned the
Marvell 88E1548 but didn't fully explain what that does special in order
to comply with your rules. As for phrases like "no hardware with support
in mainstream Linux fulfils 1-step TC directives imposed by IEEE 1588 V2
standard as of today, and that’s tragic", let's tone it down a little.
IEEE 1588 just tells you behaviorally what needs to happen, not how to
do it. You would need to come with a more convincing argument that there
is at least one piece of hardware out there marketed as one-step TC that
acts against the standard. Let alone that no hardware follows the spec.
I have asked my first question hoping that you could clarify what the
hardware needs to do, something which you either didn't, or it was too
shy and I just didn't understand it. In situations like this, when you
think you have a solution, it should really be spelled out in bold and
all capitals, as something for everybody to take away.
So now I will speculate a bit, and assume that the hardware behavior you
need is for the port 1 to subtract the t1 RX timestamp from the Sync
packet's correctionField, then the port 2 to add the t2 TX timestamp to
the correctionField. And for the hardware to send the Sync message to
the CPU and only to the CPU. This is the only situation that I can see,
where the residence time would be contained into the Sync message at the
end, and it would be forwarded by
Re: [Linuxptp-devel] [PATCH] tc: introduce 1-step for E2E & P2P Transparent Clocks
Hi Volodymyr, On Tue, Dec 01, 2020 at 05:14:38PM +0100, Volodymyr Bendiuga wrote: > From: Volodymyr Bendiuga > > This tiny patch provides generic solution for 1-step > E2E & P2P Transparent Clocks. > > Before revealing any further details, I want to state > in plain words that changes brought about in this patch > will only work for HW that is compliant with IEEE 1588 V2 > standard, with regard to Transparent Clock and 1-step > operation. That is to say, HW that is capable of updating > correction field on-the-fly, for the following packets: > 1. SYNC > 2. DELAY_REQ > 3. PDELAY_RESP > > Any deviation (in HW) from the standard will most likely > produce unexpected results. > > The general idea is that in 1-step mode HW renders time > stamps and updates correction field, without SW's intervantion > in the process. > > To make linuxptp conform to 1-step TC, a set of simple yet > effective changes must be introduced. > > The following paragraph is plausible for 1-step E2E TC: > > SYNC, DELAY_REQ and DELAY_RESP messages need to be forwarded > by linuxptp, without any intrusion into packets content. No > egress time stamp will be available for these packets, since > HW will update correction field (for SYNC & DELAY_REQ) on-the-fly. > With this in mind, linuxptp essentially needs to skip the code > that fetches time stamps from kernel. No other changes are needed. > > As for 1-step P2P TC - allowing SYNC packets to simply flow through > without collecting egress time stamp, makes the whole idea possible. > Thanks to the fact that peer-2-peer mechanism works identically on > all clock types, no other changes are necessary. > > 1-step related prerequisites for running TC in E2E mode: > 1. --time_stampint=onestep > > Two different possibilities exist for 1-step P2P TC: > 1. --time_stamping=onestep will result in TC > updating correction field of SYNC packets in HW. > Peer delay mechanism will work in 2-step mode though. > > 2. --time_stamping=p2p1step will make TC fully compatible > with 1-step principles of IEEE 1588 V2 standard. > > NOTE: --twoStepFlag=0 must be valid for both E2E & P2P. > > Signed-off-by: Volodymyr Bendiuga > --- +-+ | | |linuxptp one-step| | P2P-TC | | | +-+ ||| | port 1 | port 2 | +++ ^| ++|| ++ || Sync || Sync || | Node A |++>| Node B | || || ++ ++ Node A sends a Sync message with an originTimestamp of 1606855147.0, and a correctionField of 0. The originTimestamp was taken at time 1606855147.0, of course. - The link between Node A and TC port 1 has a delay of 800 ns. - The link between Node B and TC port 2 has a delay of 700 ns. - The NICs on port 1 and port 2 have a store-and-forward delay of 1 us between the time they receive a packet on the MAC and when they deliver that packet to the host. - The time taken by the software running on the TC box to receive a packet from the NIC, send it up the stack, to the application's socket, and back to another socket on port 2, takes 60 us. So the Sync message will be seen and timestamped by Node B at time 1606855147.63500. According to your proposal, at which steps will these latencies be accounted for in the forwarded Sync message? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 7/7] pmc_agent: Rename the update method and attempt to document it.
On Fri, Nov 27, 2020 at 04:15:29PM -0800, Richard Cochran wrote: > The UTC offset is not subscribed. It is polled with a request/reply > once a minute. (Only the port state has a push notification.) So we have a pretty large reconvergence time every time there is a leap second. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 7/7] pmc_agent: Rename the update method and attempt to document it.
On Fri, Nov 27, 2020 at 06:30:11PM -0800, Richard Cochran wrote: > This patch renames the function to have the module prefix and tries to > put into words what it does. > > Signed-off-by: Richard Cochran > --- Reviewed-by: Vladimir Oltean ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 5/7] pmc_agent: Remove bogus comparison between last update and now.
On Fri, Nov 27, 2020 at 06:30:09PM -0800, Richard Cochran wrote: > The monotonic clock can never go backwards. If you take T1 and later T2 > from that clock, then (T2 > T1) is always true. > > This patch removes the useless test. > > [ This test evolved over the years. Originally the time stamp in question > came from a PHC. ] > > Signed-off-by: Richard Cochran > --- Reviewed-by: Vladimir Oltean ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 6/7] pmc_agent: Perform time comparison using positive logic.
On Fri, Nov 27, 2020 at 06:30:10PM -0800, Richard Cochran wrote: > In the update_pmc_node() method, reduce the expression > !(x < y) to (x >= y). > > While we're at it, clean the coding style as well. > > Signed-off-by: Richard Cochran > --- Reviewed-by: Vladimir Oltean ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] pmc_agent: Simplify logic in update method.
On Fri, Nov 27, 2020 at 06:30:08PM -0800, Richard Cochran wrote: > If the pmc pointer is not set, then there is no need to read the time only > to later discard the result. This patch simplifies the flow by returning > early if there is no work to be done. > > Signed-off-by: Richard Cochran > Reviewed-by: Jacob Keller > --- Reviewed-by: Vladimir Oltean ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 3/7] pmc_agent: Simplify the update method.
On Fri, Nov 27, 2020 at 06:30:07PM -0800, Richard Cochran wrote: > The main method that causes the PMC agent to update its status takes a flag > that results in different behavior when push notifications are active. > This patch simplifies the interface by letting the agent remember whether > or not the caller subscribed to the notifications in the first place. > > Signed-off-by: Richard Cochran > --- Reviewed-by: Vladimir Oltean ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 7/7] pmc_agent: Rename the update method and attempt to document it.
On Tue, Nov 10, 2020 at 02:21:42PM -0800, Richard Cochran wrote:
> This patch renames the function to have the module prefix and tries to
> put into words what it does.
Try, well said.
>
> Signed-off-by: Richard Cochran
> ---
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 528d4ee..94ef9ca 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -336,33 +336,6 @@ int run_pmc_clock_identity(struct pmc_agent *node, int
> timeout)
> return 1;
> }
>
> -/* Returns: -1 in case of error, 0 otherwise */
> -int update_pmc_node(struct pmc_agent *node)
> -{
> - struct timespec tp;
> - uint64_t ts;
> -
> - if (!node->pmc) {
> - return 0;
> - }
> - if (clock_gettime(CLOCK_MONOTONIC, )) {
> - pr_err("failed to read clock: %m");
> - return -1;
> - }
> - ts = tp.tv_sec * NS_PER_SEC + tp.tv_nsec;
> -
> - if (ts - node->pmc_last_update >= PMC_UPDATE_INTERVAL) {
> - if (node->subscription_active) {
> - renew_subscription(node, 0);
> - }
> - if (run_pmc_get_utc_offset(node, 0) > 0) {
> - node->pmc_last_update = ts;
> - }
> - }
> -
> - return 0;
> -}
> -
> int init_pmc_node(struct config *cfg, struct pmc_agent *node, const char
> *uds,
> pmc_node_recv_subscribed_t *recv_subscribed, void *context)
> {
> @@ -414,6 +387,32 @@ int pmc_agent_subscribe(struct pmc_agent *node, int
> timeout)
> return renew_subscription(node, timeout);
> }
>
> +int pmc_agent_update(struct pmc_agent *node)
> +{
> + struct timespec tp;
> + uint64_t ts;
> +
> + if (!node->pmc) {
> + return 0;
> + }
> + if (clock_gettime(CLOCK_MONOTONIC, )) {
> + pr_err("failed to read clock: %m");
> + return -errno;
This little change here snuck in.
> + }
> + ts = tp.tv_sec * NS_PER_SEC + tp.tv_nsec;
> +
> + if (ts - node->pmc_last_update >= PMC_UPDATE_INTERVAL) {
> + if (node->subscription_active) {
> + renew_subscription(node, 0);
> + }
> + if (run_pmc_get_utc_offset(node, 0) > 0) {
> + node->pmc_last_update = ts;
> + }
> + }
> +
> + return 0;
> +}
> +
> bool pmc_agent_utc_offset_traceable(struct pmc_agent *agent)
> {
> return agent->utc_offset_traceable;
> diff --git a/pmc_agent.h b/pmc_agent.h
> index e4d3c3c..0466076 100644
> --- a/pmc_agent.h
> +++ b/pmc_agent.h
> @@ -33,7 +33,6 @@ typedef int pmc_node_recv_subscribed_t(void *context,
> struct ptp_message *msg,
>
> int init_pmc_node(struct config *cfg, struct pmc_agent *agent, const char
> *uds,
> pmc_node_recv_subscribed_t *recv_subscribed, void *context);
> -int update_pmc_node(struct pmc_agent *agent);
> int run_pmc_clock_identity(struct pmc_agent *agent, int timeout);
> int run_pmc_wait_sync(struct pmc_agent *agent, int timeout);
> int run_pmc_get_number_ports(struct pmc_agent *agent, int timeout);
> @@ -85,6 +84,22 @@ void pmc_agent_set_sync_offset(struct pmc_agent *agent,
> int offset);
> */
> int pmc_agent_subscribe(struct pmc_agent *agent, int timeout);
>
> +/**
> + * Queries the local ptp4l instance to update the TAI-UTC offset and
> + * the current leap adjustment.
Do you know exactly why we retrieve the UTC offset implicitly and not
explicitly, considering that phc2sys already calls run_pmc_get_utc_offset
enough times directly already?
Does the PTP management protocol require that we send some dummy
keepalive requests or something?
> + *
> + * In addition:
> + *
> + * - Any active port state subscription will be renewed.
> + * - The port state notification callback might be invoked.
Which one is that? You mean ->recv_subscribed?
"Might" be called? That's the best we can say right now, right. Asking
"why" it would be called might be too deep of a question.
> + *
> + * Note that the PMC agent rate limits the query to once per minute.
Somehow the description doesn't say the most important thing from a user
perspective. Could you please mention that this needs to be called
periodically to avoid expiry of the subscription? Maybe move this
comment from where it is right now:
#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.
*/
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 6/7] pmc_agent: Simplify update method yet again.
On Tue, Nov 10, 2020 at 02:21:41PM -0800, Richard Cochran wrote:
> Reduce the expression !(x < y) to (x >= y).
>
> While we're at it, clean the coding style as well.
>
> Signed-off-by: Richard Cochran
> ---
> pmc_agent.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/pmc_agent.c b/pmc_agent.c
> index cfe1b4b..528d4ee 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -351,12 +351,13 @@ int update_pmc_node(struct pmc_agent *node)
> }
> ts = tp.tv_sec * NS_PER_SEC + tp.tv_nsec;
>
> - if (!(ts - node->pmc_last_update < PMC_UPDATE_INTERVAL)) {
> + if (ts - node->pmc_last_update >= PMC_UPDATE_INTERVAL) {
How about:
pmc_agent: update_pmc_node: perform time comparison using positive logic
I wonder how many more "simplify update_pmc_node even moar" patches
there are to come.
> if (node->subscription_active) {
> renew_subscription(node, 0);
> }
> - if (run_pmc_get_utc_offset(node, 0) > 0)
> + if (run_pmc_get_utc_offset(node, 0) > 0) {
> node->pmc_last_update = ts;
> + }
> }
>
> return 0;
> --
> 2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 5/7] pmc_agent: Simplify logic in update method even more.
On Tue, Nov 10, 2020 at 02:21:40PM -0800, Richard Cochran wrote:
> The monotonic clock can never go backwards. If you take T1 and later T2
> from that clock, then (T2 > T1) is always true.
>
> This patch removes the useless test.
>
> [ This test evolved over the years. Originally the time stamp in question
> came from a PHC. ]
>
> Signed-off-by: Richard Cochran
> ---
> pmc_agent.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 47562bc..cfe1b4b 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -351,8 +351,7 @@ int update_pmc_node(struct pmc_agent *node)
> }
> ts = tp.tv_sec * NS_PER_SEC + tp.tv_nsec;
>
> - if (!(ts > node->pmc_last_update &&
> - ts - node->pmc_last_update < PMC_UPDATE_INTERVAL)) {
> + if (!(ts - node->pmc_last_update < PMC_UPDATE_INTERVAL)) {
How about this for a commit title:
pmc_agent: update_pmc_node: remove comparison between last update and now
I think it scales a bit better to describe what the patch is doing,
rather than how much more simpler the code becomes.
> if (node->subscription_active) {
> renew_subscription(node, 0);
> }
> --
> 2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 4/7] pmc_agent: Simplify logic in update method.
On Tue, Nov 10, 2020 at 02:21:39PM -0800, Richard Cochran wrote:
> If the pmc pointer is not set, then there is no need to read the time only
> to later discard the result. This patch simplifies the flow by returning
> early if there is no work to be done.
>
> Signed-off-by: Richard Cochran
> ---
> pmc_agent.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 24587b4..47562bc 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -342,14 +342,16 @@ int update_pmc_node(struct pmc_agent *node)
> struct timespec tp;
> uint64_t ts;
>
> + if (!node->pmc) {
> + return 0;
> + }
> if (clock_gettime(CLOCK_MONOTONIC, )) {
> pr_err("failed to read clock: %m");
> return -1;
> }
> ts = tp.tv_sec * NS_PER_SEC + tp.tv_nsec;
>
> - if (node->pmc &&
> - !(ts > node->pmc_last_update &&
> + if (!(ts > node->pmc_last_update &&
> ts - node->pmc_last_update < PMC_UPDATE_INTERVAL)) {
> if (node->subscription_active) {
> renew_subscription(node, 0);
> --
> 2.20.1
What is even the use case for running update_pmc_node with an agent on
which init_pmc_node was not called? Sounds like a bug.
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 3/7] pmc_agent: Simplify the update method.
On Tue, Nov 10, 2020 at 02:21:38PM -0800, Richard Cochran wrote:
> The main method that causes the PMC agent to update its status takes a flag
> that results in different behavior when push notifications are active.
> This patch simplifies the interface by letting the agent remember whether
> or not the caller subscribed to the notifications in the first place.
>
> Signed-off-by: Richard Cochran
> ---
> phc2sys.c | 6 --
> pmc_agent.c | 32
> pmc_agent.h | 2 +-
> 3 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 714c5c5..24587b4 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -42,6 +42,7 @@ struct pmc_agent {
> int clock_identity_set;
> int leap;
> int pmc_ds_requested;
> + int subscription_active;
Bool please? In some places, bool is used already.
> int sync_offset;
> int utc_offset_traceable;
>
> @@ -188,6 +189,19 @@ static int run_pmc(struct pmc_agent *node, int timeout,
> int ds_id,
> }
> }
>
> +static int renew_subscription(struct pmc_agent *node, int timeout)
> +{
> + struct ptp_message *msg;
> + int res;
> +
> + res = run_pmc(node, timeout, TLV_SUBSCRIBE_EVENTS_NP, );
> + if (is_run_pmc_error(res)) {
> + return run_pmc_err2errno(res);
> + }
> + msg_put(msg);
> + return 0;
> +}
> +
> int run_pmc_wait_sync(struct pmc_agent *node, int timeout)
> {
> struct ptp_message *msg;
> @@ -323,7 +337,7 @@ int run_pmc_clock_identity(struct pmc_agent *node, int
> timeout)
> }
>
> /* Returns: -1 in case of error, 0 otherwise */
> -int update_pmc_node(struct pmc_agent *node, int subscribe)
> +int update_pmc_node(struct pmc_agent *node)
> {
> struct timespec tp;
> uint64_t ts;
> @@ -337,8 +351,9 @@ int update_pmc_node(struct pmc_agent *node, int subscribe)
> if (node->pmc &&
> !(ts > node->pmc_last_update &&
> ts - node->pmc_last_update < PMC_UPDATE_INTERVAL)) {
> - if (subscribe)
> - pmc_agent_subscribe(node, 0);
> + if (node->subscription_active) {
> + renew_subscription(node, 0);
> + }
> if (run_pmc_get_utc_offset(node, 0) > 0)
> node->pmc_last_update = ts;
> }
> @@ -393,15 +408,8 @@ void pmc_agent_set_sync_offset(struct pmc_agent *agent,
> int offset)
>
> int pmc_agent_subscribe(struct pmc_agent *node, int timeout)
> {
> - struct ptp_message *msg;
> - int res;
> -
> - res = run_pmc(node, timeout, TLV_SUBSCRIBE_EVENTS_NP, );
> - if (is_run_pmc_error(res)) {
> - return run_pmc_err2errno(res);
> - }
> - msg_put(msg);
> - return 0;
> + node->subscription_active = 1;
> + return renew_subscription(node, timeout);
> }
Hmm. We introduce a new synonym for pmc_agent_subscribe, which we call
renew_subscription, with the only difference being that the former will
set node->subscription_active = 1 while the other will assume it was
already set that way. So the net change of this patch is that we can
keep calling update_pmc_node from the common path (between subscribers
and non-subscribers), but those common code paths no longer need to know
whether they're subscribers or not. Ok. This works, but update_pmc_node
will also need serious documentation updates when its time comes.
But another question: when ptp4l calls clock_prune_subscriptions(), our
node->subscription_active will remain 1, right? That's confusing.
"Active" might not be the right word, how about "using_subscriptions"?
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/7] pmc_agent: Convert the subscribe method into the canonical form.
On Tue, Nov 10, 2020 at 02:21:37PM -0800, Richard Cochran wrote: > This patch renames the function to have the module prefix and corrects the > return code semantics. > > Signed-off-by: Richard Cochran > --- Reviewed-by: Vladimir Oltean ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/7] Introduce error codes for the run_pmc method.
On Tue, Nov 10, 2020 at 02:21:36PM -0800, Richard Cochran wrote: > The run_pmc function is used by several of the PMC agent methods, but it > breaks the pattern of returning zero on success. However, the user facing > PMC agent methods will need to conform to the return code convention used > throughout the stack. > > In order to migrate to proper return codes, this patch replaces the hard > coded result values with macros so that the interface methods can translate > them to the required semantics of zero on success. > > Signed-off-by: Richard Cochran > --- Reviewed-by: Vladimir Oltean ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 5/5] Find a better home for the management TLV data helper function.
On Tue, Nov 10, 2020 at 01:28:19AM +0200, Vladimir Oltean wrote: > Again, great improvement already, but could you please say > 'Obtain the dataField from the Management TLV in a management message' > in the comments here? Some of us are not great with pattern matching, > and like to have it easy. Sorry, you can disregard this comment. It looks like 'dataField' is used in a single place, while 'data field' is much more widespread. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 5/5] Find a better home for the management TLV data helper function.
On Mon, Nov 09, 2020 at 01:44:59PM -0800, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
> ---
> msg.h | 12
> phc2sys.c | 2 +-
> pmc_agent.c | 18 +-
> pmc_agent.h | 1 -
> 4 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/msg.h b/msg.h
> index b600ff0..84380da 100644
> --- a/msg.h
> +++ b/msg.h
> @@ -247,6 +247,18 @@ static inline uint8_t management_action(struct
> ptp_message *m)
> return m->management.flags & 0x0f;
> }
>
> +/**
> + * Obtain the data field from the TLV in a management message.
> + * @param m A management message.
> + * @return A pointer to the TLV data field.
> + */
> +static inline void *management_tlv_data(struct ptp_message *msg)
> +{
> + struct management_tlv *mgt;
> + mgt = (struct management_tlv *) msg->management.suffix;
> + return mgt->data;
> +}
> +
Again, great improvement already, but could you please say
'Obtain the dataField from the Management TLV in a management message'
in the comments here? Some of us are not great with pattern matching,
and like to have it easy.
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 4/5] Find a better home for the management TLV ID helper function.
On Mon, Nov 09, 2020 at 01:44:58PM -0800, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
> ---
> msg.h | 12
> phc2sys.c | 2 +-
> pmc_agent.c | 10 +-
> pmc_agent.h | 1 -
> 4 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/msg.h b/msg.h
> index e71d3ce..b600ff0 100644
> --- a/msg.h
> +++ b/msg.h
> @@ -247,6 +247,18 @@ static inline uint8_t management_action(struct
> ptp_message *m)
> return m->management.flags & 0x0f;
> }
>
> +/**
> + * Obtain the ID field from the TLV in a management message.
> + * @param m A management message.
> + * @return The value of the ID field.
> + */
> +static inline int management_tlv_id(struct ptp_message *m)
> +{
> + struct management_tlv *mgt;
> + mgt = (struct management_tlv *) m->management.suffix;
> + return mgt->id;
> +}
> +
> /**
> * Test a given bit in a message's flag field.
> * @param m Message to test.
> diff --git a/phc2sys.c b/phc2sys.c
> index 9e47b4f..9c6f2ba 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -808,7 +808,7 @@ static int phc2sys_recv_subscribed(void *context, struct
> ptp_message *msg,
> struct port *port;
> struct clock *clock;
>
> - mgt_id = get_mgt_id(msg);
> + mgt_id = management_tlv_id(msg);
> if (mgt_id == excluded)
> return 0;
> switch (mgt_id) {
This is a lot easier to read, with the new name and the description. Do
you think you could specify in this function's comments that it obtains
the managementId field, for easier cross-referencing with the standard?
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 3/5] pmc_agent: Hide the implementation.
On Mon, Nov 09, 2020 at 01:44:57PM -0800, Richard Cochran wrote:
> The PMC agent's implementation should not be exposed to its users. This
> patch hides the details and provides a method to create an instance. In
> addition, the signature of the receive callback is made generic, removing
> the container_of pattern meant for sub-classing modules.
>
> Signed-off-by: Richard Cochran
> ---
> phc2sys.c | 75 +
> pmc_agent.c | 58 -
> pmc_agent.h | 62 +++
> 3 files changed, 137 insertions(+), 58 deletions(-)
>
> diff --git a/phc2sys.c b/phc2sys.c
> index c1eab1b..9e47b4f 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -1020,11 +1017,12 @@ int main(int argc, char *argv[])
> {
> char *config = NULL, *dst_name = NULL, *progname, *src_name = NULL;
> char uds_local[MAX_IFNAME_SIZE + 1];
> + int autocfg = 0, c, domain_number = 0, index, ntpshm_segment, offset;
> + int pps_fd = -1, print_level = LOG_INFO, r = -1, rt = 0;
> + int wait_sync = 0;
> struct clock *src, *dst;
> struct config *cfg;
> struct option *opts;
> - int autocfg = 0, c, domain_number = 0, index, ntpshm_segment;
> - int pps_fd = -1, print_level = LOG_INFO, r = -1, rt = 0, wait_sync = 0;
> double phc_rate, tmp;
> struct phc2sys_private priv = {
> .phc_readings = 5,
This change looks a bit noisy when all it does is introduce the "offset"
variable, but ok.
> diff --git a/pmc_agent.c b/pmc_agent.c
> index e83895c..8ccafe2 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -19,6 +19,7 @@
> */
> #include
> #include
> +#include
>
> #include "notification.h"
> #include "pmc_agent.h"
> @@ -32,6 +33,22 @@
> * renewed.
> */
>
> +struct pmc_agent {
> + struct pmc *pmc;
> + uint64_t pmc_last_update;
> +
> + struct ClockIdentity clock_identity;
> + int clock_identity_set;
> + int leap;
> + int pmc_ds_requested;
> + int sync_offset;
> + int utc_offset_traceable;
> +
> + /* Callback on message reception */
> + pmc_node_recv_subscribed_t *recv_subscribed;
> + void *recv_context;
Now that the pmc_agent is going to have a more 'stable' interface, I
wonder if this callback is even properly placed in struct pmc_agent and
shouldn't be a simple argument passed to run_pmc_events(), considering
that this is the only place where it's called from. It would also
simplify the implementation, with those "excluded" managementId's.
Of course, that can come later.
> +};
> +
> static void send_subscription(struct pmc_agent *node)
> {
> struct subscribe_events_np sen;
> @@ -58,5 +46,45 @@ int run_pmc_get_utc_offset(struct pmc_agent *agent, int
> timeout);
> int get_mgt_id(struct ptp_message *msg);
> void *get_mgt_data(struct ptp_message *msg);
>
> -#endif
>
> +/**
> + * Creates an instance of a PMC agent.
> + * @return Pointer to a PMC instance on sauces, NULL otherwise.
Sauces?
> + */
> +struct pmc_agent *pmc_agent_create(void);
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/5] pmc_agent: Rename pmc_node to something more descriptive.
On Mon, Nov 09, 2020 at 01:44:56PM -0800, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
> ---
> phc2sys.c | 4 ++--
> pmc_agent.c | 26 +-
> pmc_agent.h | 26 +-
> 3 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/phc2sys.c b/phc2sys.c
> index 17d5b4f..c1eab1b 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -103,7 +103,7 @@ struct phc2sys_private {
> int forced_sync_offset;
> int kernel_leap;
> int state_changed;
> - struct pmc_node node;
> + struct pmc_agent node;
> LIST_HEAD(port_head, port) ports;
> LIST_HEAD(clock_head, clock) clocks;
> LIST_HEAD(dst_clock_head, clock) dst_clocks;
It will remain named "node" though?
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 04/12] phc2sys: break out pmc code into pmc_common.c
On Sun, Nov 08, 2020 at 08:59:46PM +0200, Vladimir Oltean wrote: > Well, I'll probably need to resend anyway, I just noticed that patch > "ts2phc: instantiate a full clock structure for every PHC master" > doesn't build. I'm not sure how that passed my QC. So just let me know > when. Ah, it used to build when I sent it. It's now due to the patch applying differently on top of your "ts2phc NMEA trainwreck" series. The leapfile didn't use to exist. Luckily, the fix to that build breakage is trivial though, just s/cfg/priv->cfg/ on ts2phc_nmea_master.c line 263. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
