[Linuxptp-devel] [PATCH]Recognize PTP port link status

[Jesuiter, Henry (ALC NetworX GmbH)]

---
This patch implements a primitive way to recognize a PTP ports operational link
status. If a link on a network interface goes down, it will create an
EV_DESIGNATED_DISABLED transition for the appropriate PTP port (setting it into
PS_DISABLED state). If a link on a network interface goes up, it will create a
EV_DESIGNATED_ENABLED transition to return the related PTP port into PS_LISTENING
state.

The most important advantage of this patch is its massive reduction of log noise
if a PTP port loses its link for some reason.

Signed-off-by: Henry Jesuiter <***@alcnetworx.de>
---
diff -rwbBu -p a/clock.c b/clock.c
--- a/clock.c 2016-07-21 16:49:14.000000000 +0200
+++ b/clock.c 2016-07-28 11:01:08.949982556 +0200
@@ -1422,6 +1422,13 @@ int clock_poll(struct clock *c)
cur = c->pollfd;
LIST_FOREACH(p, &c->ports, list) {
/* Let the ports handle their events. */
+ event = port_link_state(p);
+ if ((EV_DESIGNATED_DISABLED == event)
+ || (EV_DESIGNATED_ENABLED == event)) {
+ sde = 1;
+ port_dispatch(p, event, 0);
+ }
+
for (i = err = 0; i < N_POLLFD && !err; i++) {
if (cur[i].revents & (POLLIN|POLLPRI)) {
event = port_event(p, i);
diff -rwbBu -p a/port.c b/port.c
--- a/port.c 2016-07-21 16:49:14.000000000 +0200
+++ b/port.c 2016-07-28 10:59:56.377398582 +0200
@@ -2194,6 +2194,24 @@ int port_dispatch(struct port *p, enum f
return 0;
}

+enum fsm_event port_link_state(struct port* p)
+{
+ int link_status = sk_interface_link_state(p->name);
+
+ if((link_status <= 0) && (port_state(p) != PS_DISABLED)) {
+ /* either link down, or failure on getting link status */
+ pr_debug("port %hu: link down on %s", portnum(p), p->name);
+ return EV_DESIGNATED_DISABLED;
+ }
+
+ if((link_status > 0) && (port_state(p) == PS_DISABLED)) {
+ pr_debug("port %hu: link up on %s", portnum(p), p->name);
+ return EV_DESIGNATED_ENABLED;
+ }
+
+ return EV_NONE;
+}
+
enum fsm_event port_event(struct port *p, int fd_index)
{
enum fsm_event event = EV_NONE;
diff -rwbBu -p a/port.h b/port.h
--- a/port.h 2016-07-21 16:49:14.000000000 +0200
+++ b/port.h 2016-07-28 11:00:22.021614463 +0200
@@ -120,6 +120,14 @@ int port_prepare_and_send(struct port *p
struct PortIdentity port_identity(struct port *p);

/**
+ * Check for port link state changes.
+ * @param port A pointer previously obtained via port_open().
+ * @return EV_NONE if nothing happened, EV_DESIGNATED_DISABLED on
+ * link down and EV_DESIGNATED_ENABLED on link up
+ */
+enum fsm_event port_link_state(struct port *p);
+
+/**
* Obtain a port number.
* @param p A port instance.
* @return The port number of 'p'.
diff -rwbBu -p a/sk.c b/sk.c
--- a/sk.c 2016-07-21 16:49:14.000000000 +0200
+++ b/sk.c 2016-07-28 11:01:31.870150427 +0200
@@ -211,6 +211,27 @@ int sk_interface_addr(const char *name,
return result;
}

+int sk_interface_link_state(const char *name) {
+ int sock;
+ struct ifreq if_req;
+
+ sock = socket(AF_INET, SOCK_DGRAM, IPPROTO_IP);
+ if(sock < 0) {
+ return -1;
+ }
+
+ strncpy(if_req.ifr_name, name, sizeof(if_req.ifr_name));
+ if(ioctl(sock, SIOCGIFFLAGS, &if_req) < 0) {
+ close(sock);
+ return -1;
+ }
+ close(sock);
+
+ /* IFF_RUNNING => operational status, e.g. actual link status */
+ /* IFF_UP => administrative status, interface up/down in system */
+ return if_req.ifr_flags & IFF_RUNNING ? 1 : 0;
+}
+
static short sk_events = POLLPRI;
static short sk_revents = POLLPRI;

diff -rwbBu -p a/sk.h b/sk.h
--- a/sk.h 2016-07-21 16:49:14.000000000 +0200
+++ b/sk.h 2016-07-28 10:57:41.944071527 +0200
@@ -80,6 +80,13 @@ int sk_interface_macaddr(const char *nam
int sk_interface_addr(const char *name, int family, struct address *addr);

/**
+ * Check operational link status of an interface
+ * @param name The name of the interface
+ * @return 0 on link down, 1 on link up, -1 on failure
+ */
+int sk_interface_link_state(const char *name);
+
+/**
* Read a message from a socket.
* @param fd An open socket.
* @param buf Buffer to receive the message.

---

Please note: as the commit message tells, that is a primitive method to recognize the link status,
since it will poll for this information each time clock_poll() will be called. Another (better)
solution could use the linux netlink socket infrastructure. If you prefer such implementation
I could provide it as well.

Best regards
Henry Jesuiter



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

[Richard Cochran]

Post by Jesuiter, Henry (ALC NetworX GmbH)
The most important advantage of this patch is its massive reduction of log noise
if a PTP port loses its link for some reason.

I agree that the log noise can be annoying. However, this can be
mitigated by increasing the fault reset interval, if your links are
not reliable.

Post by Jesuiter, Henry (ALC NetworX GmbH)
+enum fsm_event port_link_state(struct port* p)
+{
+ int link_status = sk_interface_link_state(p->name);
+
+ if((link_status <= 0) && (port_state(p) != PS_DISABLED)) {
+ /* either link down, or failure on getting link status */
+ pr_debug("port %hu: link down on %s", portnum(p), p->name);
+ return EV_DESIGNATED_DISABLED;
+ }
+
+ if((link_status > 0) && (port_state(p) == PS_DISABLED)) {
+ pr_debug("port %hu: link up on %s", portnum(p), p->name);
+ return EV_DESIGNATED_ENABLED;
+ }
+
+ return EV_NONE;
+}

This is not what EV_DESIGNATED_ are meant to be used for. They are
for administrative purposes, using management messages.

When the link goes down unexpectedly, this is a fault, and it should
be treated as such. Currently, we do not detect that the reason for a
fault is a downed link. The way to improve the code would be to
expand the fault logic to handle a downed link explicitly.

Post by Jesuiter, Henry (ALC NetworX GmbH)
Please note: as the commit message tells, that is a primitive method to recognize the link status,
since it will poll for this information each time clock_poll() will be called. Another (better)
solution could use the linux netlink socket infrastructure. If you prefer such implementation
I could provide it as well.

Don't bother. I already wrote a RT netlink module some years ago, but
I never merged it because I thought the utility would be low.

However, if this becomes an important feature that people want, then
we can talk about adding it. So far, no one has really asked for
this, and so I would like to hear convincing arguments why this
feature is important...

Thanks,
Richard




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

[Jesuiter, Henry (ALC NetworX GmbH)]

Hi,

Post by Richard Cochran
I agree that the log noise can be annoying. However, this can be
mitigated by increasing the fault reset interval, if your links are
not reliable.

It's indeed some kind of issue for us. We have several devices
here, utilizing more than one Ethernet-Port (but having only one
PHC per device). So we would like to put these devices behind
a BC switch, and run ptp4l on as much ports as are available on
the device. This will work out well, as long as all ports are connected.

But we must be aware of disconnecting one or more ports (or even
connecting them later on) and need meaningful log messages from
our own applications in this case. Unfortunately we might miss
these messages, as long as ptp4l gets that noisy.

So yes we would like that feature to be added (on one or the other
way).

Best regards
Henry



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

[Richard Cochran]

Post by Jesuiter, Henry (ALC NetworX GmbH)
So yes we would like that feature to be added (on one or the other
way).

Ok, I'll post a RFC series soon, and we can work from there.

Thanks,
Richard

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