Discussion:
[Linuxptp-devel] [PATCH V3 0/7] Link state tracking
Richard Cochran
2016-10-16 10:55:44 UTC
Permalink
Changes in V3:
~~~~~~~~~~~~~~
- Free rtnl's malloc'ed buffer on the exit path.

Changes in V2:
~~~~~~~~~~~~~~
- Add more explanation to patch #7
- Fix missing parenthesis in the realloc() size in patch #7

This series implements link state tracking over all port via a single
RT netlink socket. Unfortunately this ends up being a fair amount of
code simply because the netlink API does not allow selective per-port
tracking. We work around this limitation by de-multiplexing the
notifications from a single socket in the clock module.

Patches 1 and 2 add generic rtnl code started long ago.
Patches 3 and 4 fix coding style I noticed in passing.
Patches 5-7 implement the link state tracking.

Thanks,
Richard


Richard Cochran (7):
rtnl: Introduce RT netlink sockets.
sk: Add a method to obtain a socket for utility purposes.
clock: Remove stray semicolon.
clock: Fix coding style within a helper function.
clock: Remember each port's interface index.
port: Provide methods to set and get the link status.
clock: Monitor the link status using a RT netlink socket.

clock.c | 104 +++++++++++++++++++++++++++++++++------
makefile | 2 +-
port.c | 13 +++++
port.h | 14 ++++++
rtnl.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
rtnl.h | 54 +++++++++++++++++++++
sk.c | 10 ++++
sk.h | 6 +++
8 files changed, 352 insertions(+), 17 deletions(-)
create mode 100644 rtnl.c
create mode 100644 rtnl.h
--
2.1.4
Richard Cochran
2016-10-16 10:55:45 UTC
Permalink
This patch adds a source module that implements RT netlink sockets
for the purpose of link monitoring. Unfortunately the netlink API
offers no possibility for per-port notification. Instead it
forces us to use a de-multiplexing pattern.

Signed-off-by: Richard Cochran <***@gmail.com>
---
makefile | 2 +-
rtnl.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
rtnl.h | 54 +++++++++++++++++++++
3 files changed, 221 insertions(+), 1 deletion(-)
create mode 100644 rtnl.c
create mode 100644 rtnl.h

diff --git a/makefile b/makefile
index bcdbb92..44be067 100644
--- a/makefile
+++ b/makefile
@@ -25,7 +25,7 @@ LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
PRG = ptp4l pmc phc2sys hwstamp_ctl phc_ctl timemaster
OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \
filter.o fsm.o hash.o linreg.o mave.o mmedian.o msg.o ntpshm.o nullf.o phc.o \
- pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \
+ pi.o port.o print.o ptp4l.o raw.o rtnl.o servo.o sk.o stats.o tlv.o \
transport.o tsproc.o udp.o udp6.o uds.o util.o version.o

OBJECTS = $(OBJ) hwstamp_ctl.o phc2sys.o phc_ctl.o pmc.o pmc_common.o \
diff --git a/rtnl.c b/rtnl.c
new file mode 100644
index 0000000..7f5dc45
--- /dev/null
+++ b/rtnl.c
@@ -0,0 +1,166 @@
+/**
+ * @file rtnl.c
+ * @note Copyright (C) 2012 Richard Cochran <***@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#include <asm/types.h>
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+#include <net/if.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+#include "print.h"
+#include "rtnl.h"
+
+static int rtnl_len;
+static char *rtnl_buf;
+
+int rtnl_close(int fd)
+{
+ if (rtnl_buf) {
+ free(rtnl_buf);
+ rtnl_buf = NULL;
+ rtnl_len = 0;
+ }
+ return close(fd);
+}
+
+int rtnl_link_query(int fd)
+{
+ struct sockaddr_nl sa;
+ struct msghdr msg;
+ struct iovec iov;
+ int cnt;
+
+ struct {
+ struct nlmsghdr hdr;
+ struct rtgenmsg gen;
+ } __attribute__((packed)) request;
+
+ memset(&sa, 0, sizeof(sa));
+ sa.nl_family = AF_NETLINK;
+
+ memset(&request, 0, sizeof(request));
+ request.hdr.nlmsg_len = NLMSG_LENGTH(sizeof(request.gen));
+ request.hdr.nlmsg_type = RTM_GETLINK;
+ request.hdr.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP;
+ request.hdr.nlmsg_seq = 1;
+ request.hdr.nlmsg_pid = 0;
+ request.gen.rtgen_family = AF_UNSPEC;
+
+ iov.iov_base = &request;
+ iov.iov_len = sizeof(request);
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_name = &sa;
+ msg.msg_namelen = sizeof(sa);
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+
+ cnt = sendmsg(fd, &msg, 0);
+ if (cnt < 0) {
+ pr_err("rtnl: sendmsg: %m");
+ return -1;
+ }
+ return 0;
+}
+
+int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
+{
+ int index, len;
+ struct iovec iov;
+ struct sockaddr_nl sa;
+ struct msghdr msg;
+ struct nlmsghdr *nh;
+ struct ifinfomsg *info = NULL;
+
+ if (!rtnl_buf) {
+ rtnl_len = 4096;
+ rtnl_buf = malloc(rtnl_len);
+ if (!rtnl_buf) {
+ pr_err("rtnl: low memory");
+ return -1;
+ }
+ }
+
+ iov.iov_base = rtnl_buf;
+ iov.iov_len = rtnl_len;
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_name = &sa;
+ msg.msg_namelen = sizeof(sa);
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+
+ len = recvmsg(fd, &msg, MSG_PEEK | MSG_TRUNC);
+ if (len < 1) {
+ pr_err("rtnl: recvmsg: %m");
+ return -1;
+ }
+ if (len > rtnl_len) {
+ free(rtnl_buf);
+ rtnl_len = len;
+ rtnl_buf = malloc(len);
+ if (!rtnl_buf) {
+ pr_err("rtnl: failed to resize to %d bytes", len);
+ return -1;
+ }
+ iov.iov_base = rtnl_buf;
+ iov.iov_len = rtnl_len;
+ }
+
+ len = recvmsg(fd, &msg, 0);
+ if (len < 1) {
+ pr_err("rtnl: recvmsg: %m");
+ return -1;
+ }
+ nh = (struct nlmsghdr *) rtnl_buf;
+
+ for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
+ if (nh->nlmsg_type == RTM_NEWLINK) {
+ info = NLMSG_DATA(nh);
+ index = info->ifi_index;
+ pr_debug("interface index %d is %s", index,
+ info->ifi_flags & IFF_RUNNING ? "up" : "down");
+ cb(ctx, index, info->ifi_flags & IFF_RUNNING ? 1 : 0);
+ }
+ }
+ return 0;
+}
+
+int rtnl_open(void)
+{
+ int fd;
+ struct sockaddr_nl sa;
+
+ memset(&sa, 0, sizeof(sa));
+ sa.nl_family = AF_NETLINK;
+ sa.nl_groups = RTMGRP_LINK;
+
+ fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
+ if (fd < 0) {
+ pr_err("failed to open netlink socket: %m");
+ return -1;
+ }
+ if (bind(fd, (struct sockaddr *) &sa, sizeof(sa))) {
+ pr_err("failed to bind netlink socket: %m");
+ return -1;
+ }
+ return fd;
+}
diff --git a/rtnl.h b/rtnl.h
new file mode 100644
index 0000000..f1871f2
--- /dev/null
+++ b/rtnl.h
@@ -0,0 +1,54 @@
+/**
+ * @file rtnl.h
+ * @brief Interface link status via RT netlink
+ * @note Copyright (C) 2012 Richard Cochran <***@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#ifndef HAVE_RTNL_H
+#define HAVE_RTNL_H
+
+typedef void (*rtnl_callback)(void *ctx, int index, int linkup);
+
+/**
+ * Close a RT netlink socket.
+ * @param fd A socket obtained via rtnl_open().
+ * @return Zero on success, non-zero otherwise.
+ */
+int rtnl_close(int fd);
+
+/**
+ * Request the link status from the kernel.
+ * @param fd A socket obtained via rtnl_open().
+ * @return Zero on success, non-zero otherwise.
+ */
+int rtnl_link_query(int fd);
+
+/**
+ * Read kernel messages looking for a link up/down events.
+ * @param fd Readable socket obtained via rtnl_open().
+ * @param cb Callback function to be invoked on each event.
+ * @param ctx Private context passed to the callback.
+ * @return Zero on success, non-zero otherwise.
+ */
+int rtnl_link_status(int fd, rtnl_callback cb, void *ctx);
+
+/**
+ * Open a RT netlink socket for monitoring link state.
+ * @return A valid socket, or -1 on error.
+ */
+int rtnl_open(void);
+
+#endif
--
2.1.4
Richard Cochran
2016-10-16 10:55:46 UTC
Permalink
The clock module will want to know the interface indexes, in order to
implement link monitoring. However, the clock does not open any sockets
directly. This helper function lets us keep the clock module free of
socket level code.

Signed-off-by: Richard Cochran <***@gmail.com>
---
sk.c | 10 ++++++++++
sk.h | 6 ++++++
2 files changed, 16 insertions(+)

diff --git a/sk.c b/sk.c
index ad30f71..63ec206 100644
--- a/sk.c
+++ b/sk.c
@@ -80,6 +80,16 @@ static int hwts_init(int fd, const char *device, int rx_filter, int one_step)

/* public methods */

+int sk_interface_fd(void)
+{
+ int fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
+ if (fd < 0) {
+ pr_err("socket failed: %m");
+ return -1;
+ }
+ return fd;
+}
+
int sk_interface_index(int fd, const char *name)
{
struct ifreq ifreq;
diff --git a/sk.h b/sk.h
index 31cc88e..d91d5d8 100644
--- a/sk.h
+++ b/sk.h
@@ -40,6 +40,12 @@ struct sk_ts_info {
};

/**
+ * Obtains a socket suitable for use with sk_interface_index().
+ * @return An open socket on success, -1 otherwise.
+ */
+int sk_interface_fd(void);
+
+/**
* Obtain the numerical index from a network interface by name.
* @param fd An open socket.
* @param device The name of the network interface of interest.
--
2.1.4
Richard Cochran
2016-10-16 10:55:47 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index 8ca749e..55532b8 100644
--- a/clock.c
+++ b/clock.c
@@ -314,7 +314,7 @@ static void clock_freq_est_reset(struct clock *c)
c->fest.origin1 = tmv_zero();
c->fest.ingress1 = tmv_zero();
c->fest.count = 0;
-};
+}

static void clock_management_send_error(struct port *p,
struct ptp_message *msg, int error_id)
--
2.1.4
Richard Cochran
2016-10-16 10:55:48 UTC
Permalink
No functional changes.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/clock.c b/clock.c
index 55532b8..3b42109 100644
--- a/clock.c
+++ b/clock.c
@@ -768,20 +768,22 @@ static int clock_add_port(struct clock *c, int phc_index,
{
struct port *p, *piter, *lastp = NULL;

- if (clock_resize_pollfd(c, c->nports + 1))
+ if (clock_resize_pollfd(c, c->nports + 1)) {
return -1;
- p = port_open(phc_index, timestamping, ++c->last_port_number,
- iface, c);
+ }
+ p = port_open(phc_index, timestamping, ++c->last_port_number, iface, c);
if (!p) {
/* No need to shrink pollfd */
return -1;
}
- LIST_FOREACH(piter, &c->ports, list)
+ LIST_FOREACH(piter, &c->ports, list) {
lastp = piter;
- if (lastp)
+ }
+ if (lastp) {
LIST_INSERT_AFTER(lastp, p, list);
- else
+ } else {
LIST_INSERT_HEAD(&c->ports, p, list);
+ }
c->nports++;
clock_fda_changed(c);
return 0;
--
2.1.4
Richard Cochran
2016-10-16 10:55:49 UTC
Permalink
We use a hash table to remember the mapping. Since our existing hash
table is a simply string hash, we convert the integer index into a decimal
string. Although hashing integers in this way is sub-optimal, the table
will not be used in a performance critical path.

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

diff --git a/clock.c b/clock.c
index 3b42109..a0124d2 100644
--- a/clock.c
+++ b/clock.c
@@ -31,6 +31,7 @@
#include "clockcheck.h"
#include "foreign.h"
#include "filter.h"
+#include "hash.h"
#include "missing.h"
#include "msg.h"
#include "phc.h"
@@ -38,6 +39,7 @@
#include "servo.h"
#include "stats.h"
#include "print.h"
+#include "sk.h"
#include "tlv.h"
#include "tsproc.h"
#include "uds.h"
@@ -93,6 +95,7 @@ struct clock {
int pollfd_valid;
int nports; /* does not include the UDS port */
int last_port_number;
+ struct hash *index2port;
int free_running;
int freq_est_interval;
int grand_master_capable; /* for 802.1AS only */
@@ -270,6 +273,7 @@ void clock_destroy(struct clock *c)
}
port_close(c->uds_port);
free(c->pollfd);
+ hash_destroy(c->index2port, NULL);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
}
@@ -767,6 +771,8 @@ static int clock_add_port(struct clock *c, int phc_index,
struct interface *iface)
{
struct port *p, *piter, *lastp = NULL;
+ int fd, index;
+ char key[16];

if (clock_resize_pollfd(c, c->nports + 1)) {
return -1;
@@ -786,6 +792,23 @@ static int clock_add_port(struct clock *c, int phc_index,
}
c->nports++;
clock_fda_changed(c);
+
+ /* Remember the index to port mapping, for link status tracking. */
+ fd = sk_interface_fd();
+ if (fd < 0) {
+ return -1;
+ }
+ index = sk_interface_index(fd, iface->name);
+ if (index < 0) {
+ return -1;
+ }
+ snprintf(key, sizeof(key), "%d", index);
+ if (hash_insert(c->index2port, key, p)) {
+ pr_err("failed to add port with index %d twice!", index);
+ return -1;
+ }
+ close(fd);
+
return 0;
}

@@ -1087,6 +1110,11 @@ struct clock *clock_create(enum clock_type type, struct config *config,
}
clock_fda_changed(c);

+ c->index2port = hash_create();
+ if (!c->index2port) {
+ pr_err("failed create index-to-port hash table");
+ return NULL;
+ }
/* Create the ports. */
STAILQ_FOREACH(iface, &config->interfaces, list) {
if (clock_add_port(c, phc_index, timestamping, iface)) {
--
2.1.4
Richard Cochran
2016-10-16 10:55:50 UTC
Permalink
Signed-off-by: Richard Cochran <***@gmail.com>
---
port.c | 13 +++++++++++++
port.h | 14 ++++++++++++++
2 files changed, 27 insertions(+)

diff --git a/port.c b/port.c
index 5868983..a1ad6f6 100644
--- a/port.c
+++ b/port.c
@@ -117,6 +117,7 @@ struct port {
int path_trace_enabled;
int rx_timestamp_offset;
int tx_timestamp_offset;
+ int link_status;
struct fault_interval flt_interval_pertype[FT_CNT];
enum fault_type last_fault_type;
unsigned int versionNumber; /*UInteger4*/
@@ -2357,6 +2358,17 @@ int port_number(struct port *p)
return portnum(p);
}

+int port_link_status_get(struct port *p)
+{
+ return p->link_status;
+}
+
+void port_link_status_set(struct port *p, int up)
+{
+ p->link_status = up ? 1 : 0;
+ pr_notice("port %hu: link %s", portnum(p), up ? "up" : "down");
+}
+
int port_manage(struct port *p, struct port *ingress, struct ptp_message *msg)
{
struct management_tlv *mgt;
@@ -2573,6 +2585,7 @@ struct port *port_open(int phc_index,
p->path_trace_enabled = config_get_int(cfg, p->name, "path_trace_enabled");
p->rx_timestamp_offset = config_get_int(cfg, p->name, "ingressLatency");
p->tx_timestamp_offset = config_get_int(cfg, p->name, "egressLatency");
+ p->link_status = 1;
p->clock = clock;
p->trp = transport_create(cfg, transport);
if (!p->trp)
diff --git a/port.h b/port.h
index 662eaef..19dec4a 100644
--- a/port.h
+++ b/port.h
@@ -127,6 +127,20 @@ struct PortIdentity port_identity(struct port *p);
int port_number(struct port *p);

/**
+ * Obtain the link status of a port.
+ * @param p A port instance.
+ * @return One (1) if the link is up, zero otherwise.
+ */
+int port_link_status_get(struct port *p);
+
+/**
+ * Sets the link status for a port.
+ * @param p A port instance.
+ * @param up Pass one (1) if the link is up and zero if down.
+ */
+void port_link_status_set(struct port *p, int up);
+
+/**
* Manage a port according to a given message.
* @param p A pointer previously obtained via port_open().
* @param ingress The port on which 'msg' was received.
--
2.1.4
Miroslav Lichvar
2016-10-18 06:59:17 UTC
Permalink
Post by Richard Cochran
+void port_link_status_set(struct port *p, int up)
+{
+ p->link_status = up ? 1 : 0;
+ pr_notice("port %hu: link %s", portnum(p), up ? "up" : "down");
+}
In my testing I see duplicated messages "link up" and "link down"
messages.

ptp4l[2732080.004]: master offset -30612248978 s2 freq -62499999 path delay -9419030
ptp4l[2732080.085]: port 1: link down
ptp4l[2732080.085]: port 1: SLAVE to FAULTY on FAULT_DETECTED (FT_UNSPECIFIED)
ptp4l[2732096.301]: port 1: link down
ptp4l[2732099.258]: port 1: link up
ptp4l[2732099.260]: driver changed our HWTSTAMP options
ptp4l[2732099.260]: tx_type 1 not 1
ptp4l[2732099.260]: rx_filter 1 not 12
ptp4l[2732099.260]: port 1: FAULTY to LISTENING on FAULT_CLEARED
ptp4l[2732099.260]: port 1: link up
ptp4l[2732099.261]: port 1: link up

Would it make sense to print the message only when the status has
changed?
--
Miroslav Lichvar
Richard Cochran
2016-10-19 19:16:02 UTC
Permalink
Post by Miroslav Lichvar
Would it make sense to print the message only when the status has
changed?
I'd rather leave them in. These are the events that the kernel is
giving us, and I want to see what rtnl is doing, especially since I
don't completely trust it. It won't mean too much syslog noise, since
the link events are rare. (Unless something is broken, and in that
case the messages are helpful.)

Thanks,
Richard

Richard Cochran
2016-10-16 10:55:51 UTC
Permalink
Poll for link up/down events. When a link goes down, the port becomes
faulty until the link goes up again. We keep the fault timer from the
existing fault detection, but a downed link prevents clear the fault.

The new state machine is depicted in this ascii art diagram:

+--------+ Fault +---------+
| |------------>| |
| UP | | FAULT |
| |<------------| |
+--+--+--+ Timeout +---------+
A | /
| | /
Link-Up | | Link-Down /
| | /
| V /
+--+--+--+ / Link-Down
| | /
| DOWN |<--------/
| |
+--------+

If the fault timer occurs in the DOWN state, we simply ignore it.
After all, without the link the port is useless.

There is one case where the new code changes the existing behavior.
If the link quickly does down and then up again while another fault
(and its timer) are active, then we will enter the UP state without
waiting for the fault timer expiration. However, this behavior is
acceptable because when a link goes up, you are starting with a clean
slate.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/clock.c b/clock.c
index a0124d2..4adb492 100644
--- a/clock.c
+++ b/clock.c
@@ -39,6 +39,7 @@
#include "servo.h"
#include "stats.h"
#include "print.h"
+#include "rtnl.h"
#include "sk.h"
#include "tlv.h"
#include "tsproc.h"
@@ -271,6 +272,7 @@ void clock_destroy(struct clock *c)
LIST_FOREACH_SAFE(p, &c->ports, list, tmp) {
clock_remove_port(c, p);
}
+ rtnl_close(c->pollfd[0].fd);
port_close(c->uds_port);
free(c->pollfd);
hash_destroy(c->index2port, NULL);
@@ -320,6 +322,25 @@ static void clock_freq_est_reset(struct clock *c)
c->fest.count = 0;
}

+static void clock_link_status(void *ctx, int index, int linkup)
+{
+ struct clock *c = ctx;
+ struct port *p;
+ char key[16];
+
+ snprintf(key, sizeof(key), "%d", index);
+ p = hash_lookup(c->index2port, key);
+ if (!p) {
+ return;
+ }
+ port_link_status_set(p, linkup);
+ if (linkup) {
+ port_dispatch(p, EV_FAULT_CLEARED, 0);
+ } else {
+ port_dispatch(p, EV_FAULT_DETECTED, 0);
+ }
+}
+
static void clock_management_send_error(struct port *p,
struct ptp_message *msg, int error_id)
{
@@ -1096,13 +1117,19 @@ struct clock *clock_create(enum clock_type type, struct config *config,
LIST_INIT(&c->ports);
c->last_port_number = 0;

- /*
- * Create the UDS interface.
- */
if (clock_resize_pollfd(c, 0)) {
pr_err("failed to allocate pollfd");
return NULL;
}
+
+ /* Open a RT netlink socket. */
+ c->pollfd[0].fd = rtnl_open();
+ if (c->pollfd[0].fd < 0) {
+ return NULL;
+ }
+ c->pollfd[0].events = POLLIN|POLLPRI;
+
+ /* Create the UDS interface. */
c->uds_port = port_open(phc_index, timestamping, 0, udsif, c);
if (!c->uds_port) {
pr_err("failed to open the UDS port");
@@ -1129,6 +1156,7 @@ struct clock *clock_create(enum clock_type type, struct config *config,
port_dispatch(p, EV_INITIALIZE, 0);
}
port_dispatch(c->uds_port, EV_INITIALIZE, 0);
+ rtnl_link_query(c->pollfd[0].fd);

return c;
}
@@ -1194,9 +1222,12 @@ static int clock_resize_pollfd(struct clock *c, int new_nports)
{
struct pollfd *new_pollfd;

- /* Need to allocate one extra block of fds for uds */
+ /*
+ * Need to allocate one descriptor for RT netlink and one
+ * whole extra block of fds for UDS.
+ */
new_pollfd = realloc(c->pollfd,
- (new_nports + 1) * N_CLOCK_PFD *
+ (1 + (new_nports + 1) * N_CLOCK_PFD) *
sizeof(struct pollfd));
if (!new_pollfd)
return -1;
@@ -1221,7 +1252,7 @@ static void clock_fill_pollfd(struct pollfd *dest, struct port *p)
static void clock_check_pollfd(struct clock *c)
{
struct port *p;
- struct pollfd *dest = c->pollfd;
+ struct pollfd *dest = c->pollfd + 1;

if (c->pollfd_valid)
return;
@@ -1437,7 +1468,7 @@ int clock_poll(struct clock *c)
struct port *p;

clock_check_pollfd(c);
- cnt = poll(c->pollfd, (c->nports + 1) * N_CLOCK_PFD, -1);
+ cnt = poll(c->pollfd, 1 + (c->nports + 1) * N_CLOCK_PFD, -1);
if (cnt < 0) {
if (EINTR == errno) {
return 0;
@@ -1449,7 +1480,13 @@ int clock_poll(struct clock *c)
return 0;
}

+ /* Check the RT netlink. */
cur = c->pollfd;
+ if (cur->revents & (POLLIN|POLLPRI)) {
+ rtnl_link_status(cur->fd, clock_link_status, c);
+ }
+
+ cur++;
LIST_FOREACH(p, &c->ports, list) {
/* Let the ports handle their events. */
for (i = err = 0; i < N_POLLFD && !err; i++) {
@@ -1468,10 +1505,15 @@ int clock_poll(struct clock *c)
}
}

- /* Check the fault timer. */
+ /*
+ * When the fault timer expires we clear the fault,
+ * but only if the link is up.
+ */
if (cur[N_POLLFD].revents & (POLLIN|POLLPRI)) {
clock_fault_timeout(p, 0);
- port_dispatch(p, EV_FAULT_CLEARED, 0);
+ if (port_link_status_get(p)) {
+ port_dispatch(p, EV_FAULT_CLEARED, 0);
+ }
}

cur += N_CLOCK_PFD;
--
2.1.4
Miroslav Lichvar
2016-10-17 15:16:21 UTC
Permalink
Post by Richard Cochran
Poll for link up/down events. When a link goes down, the port becomes
faulty until the link goes up again. We keep the fault timer from the
existing fault detection, but a downed link prevents clear the fault.
I'm testing this, but I'm not sure if it's working correctly. When I
take the interface down I see a "link up" message from ptp4l. When I
bring it up, there is another "link up" message.

ptp4l[2675017.937]: master offset 3 s2 freq -28502 path delay -8
ptp4l[2675018.937]: master offset -3 s2 freq -28507 path delay -5
ptp4l[2675019.937]: master offset -7 s2 freq -28512 path delay -5
ptp4l[2675020.895]: port 1: link up
ptp4l[2675026.440]: port 1: SLAVE to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[2675026.441]: selected best master clock XXX
ptp4l[2675026.441]: assuming the grand master role
ptp4l[2675457.762]: port 1: link up
ptp4l[2675460.966]: selected best master clock YYY
--
Miroslav Lichvar
Richard Cochran
2016-10-18 06:35:56 UTC
Permalink
Post by Miroslav Lichvar
I'm testing this, but I'm not sure if it's working correctly. When I
take the interface down I see a "link up" message from ptp4l. When I
bring it up, there is another "link up" message.
I only tested by pulling the cable! A quick test with ifdown works
for me. Does the link actually go down after "ifdown ethX"? Maybe
there is a seperate rtnl message for ifup/down...

Thanks,
Richard
Miroslav Lichvar
2016-10-18 06:46:46 UTC
Permalink
Post by Richard Cochran
Post by Miroslav Lichvar
I'm testing this, but I'm not sure if it's working correctly. When I
take the interface down I see a "link up" message from ptp4l. When I
bring it up, there is another "link up" message.
I only tested by pulling the cable! A quick test with ifdown works
for me. Does the link actually go down after "ifdown ethX"? Maybe
there is a seperate rtnl message for ifup/down...
It looks like ifdown with NetworkManager doesn't really take down the
interface, it just removes its addresses. When I take it down manually
with "ip", it seems to be working as expected. Sorry for the noise.

Would it be possible to make the netlink socket optional? If it can't
be opened (e.g. in a simulated environment), I think it would be nice
if it worked as before. I was thinking something like this:

diff --git a/clock.c b/clock.c
index dd0c631..09c737f 100644
--- a/clock.c
+++ b/clock.c
@@ -272,7 +272,8 @@ void clock_destroy(struct clock *c)
LIST_FOREACH_SAFE(p, &c->ports, list, tmp) {
clock_remove_port(c, p);
}
- rtnl_close(c->pollfd[0].fd);
+ if (c->pollfd[0].fd >= 0)
+ rtnl_close(c->pollfd[0].fd);
port_close(c->uds_port);
free(c->pollfd);
hash_destroy(c->index2port, NULL);
@@ -1124,9 +1125,6 @@ struct clock *clock_create(enum clock_type type, struct config *config,

/* Open a RT netlink socket. */
c->pollfd[0].fd = rtnl_open();
- if (c->pollfd[0].fd < 0) {
- return NULL;
- }
c->pollfd[0].events = POLLIN|POLLPRI;

/* Create the UDS interface. */
@@ -1156,7 +1154,8 @@ struct clock *clock_create(enum clock_type type, struct config *config,
port_dispatch(p, EV_INITIALIZE, 0);
}
port_dispatch(c->uds_port, EV_INITIALIZE, 0);
- rtnl_link_query(c->pollfd[0].fd);
+ if (c->pollfd[0].fd >= 0)
+ rtnl_link_query(c->pollfd[0].fd);

return c;
}
--
Miroslav Lichvar
Richard Cochran
2016-10-18 08:55:19 UTC
Permalink
Post by Miroslav Lichvar
Would it be possible to make the netlink socket optional?
Ok, I'll make it into a warning if rtnl is missing.

Thanks,
Richard
Loading...