linuxptp-devel

[Linuxptp-devel] [PATCH 0/8] Add linuxptp bond failover support

[Hangbin L. <liu...@gm...>]

Hi Richard,

As we know, more and more customers are asking for linuxptp fail over support.

And here is the bond failover patch set. The first two patches add netlink
support to get bond slave info. Then we start track interface info in
port so we can update interface after fail over. And also we start get
ts info from true ts interface.

After we get true ts interface, we use this interface in
sk_timestamping_init() in transport_open().

Also we need update clock device and phc_index in phc2sys.

Since it's a large patch set. I may miss some part. Please let me know if
you have any questions, comments, or concerns.

Thanks
Hangbin

---
Testing: I tested bond active-backup with L2 SW/HW and L4 SW/HW. All looks
good so far.

1. set up bond
# ip link add bond0 type bond
# ip link set bond0 type bond mode active-backup
# ip link set bond0 type bond miimon 100
# ip link set p4p1 master bond0
# ip link set p4p2 master bond0
# ip link set bond0 up
# ip addr add 192.168.1.1/24 dev bond0

2. start ptp4l with L2 SW/HW and L4 SW/HW
# ./ptp4l -S -2 -i bond0 -m
# ./ptp4l -2 -i bond0 -m
# ./ptp4l -S -i bond0 -m
# ./ptp4l -i bond0 -m

3. When test HW tampstamping, also start phc2sys
# ./phc2sys -a -r -m

4. trigger fail over
# ip addr show bond0
10: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether f4:e9:d4:94:62:f0 brd ff:ff:ff:ff:ff:ff
    inet 192.168.1.1/24 scope global bond0
       valid_lft forever preferred_lft forever
    inet6 fe80::f6e9:d4ff:fe94:62f0/64 scope link
       valid_lft forever preferred_lft forever
# ip -d link show bond0
10: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT qlen 1000
    link/ether f4:e9:d4:94:62:f0 brd ff:ff:ff:ff:ff:ff promiscuity 0
    bond mode active-backup active_slave p4p1 miimon 100 updelay 0 downdelay 0 use_carrier 1 arp_interval 0 arp_validate none arp_all_targets any primary_reselect always fail_over_mac none xmit_hash_policy layer2 resend_igmp 1 num_grat_arp 1 all_slaves_active 0 min_links 0 lp_interval 1 packets_per_slave 1 lacp_rate slow ad_select stable tlb_dynamic_lb 0 addrgenmode eui64
# ip link set p4p1 down
# ip -d link show bond0
10: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT qlen 1000
    link/ether f4:e9:d4:94:62:f0 brd ff:ff:ff:ff:ff:ff promiscuity 0
    bond mode active-backup active_slave p4p2 miimon 100 updelay 0 downdelay 0 use_carrier 1 arp_interval 0 arp_validate none arp_all_targets any primary_reselect always fail_over_mac none xmit_hash_policy layer2 resend_igmp 1 num_grat_arp 1 all_slaves_active 0 min_links 0 lp_interval 1 packets_per_slave 1 lacp_rate slow ad_select stable tlb_dynamic_lb 0 addrgenmode eui64
# ip link set p4p1 up
# ip link set p4p2 down
# ip link set p4p2 up

5. See the logs from ptp4l and phc2sys
# ./ptp4l -i bond0 -m
ptp4l[43749.389]: selected /dev/ptp0 as PTP clock
ptp4l[43749.408]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[43749.408]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[43755.760]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[43755.760]: selected best master clock f4e9d4.fffe.9462f0
ptp4l[43755.760]: assuming the grand master role

ptp4l[43767.206]: port 1: link up
ptp4l[43767.225]: port 1: MASTER to LISTENING on INIT_COMPLETE
ptp4l[43767.225]: port 1: link up
ptp4l[43767.225]: port 1: link up
ptp4l[43774.206]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[43774.206]: selected best master clock f4e9d4.fffe.9462f0
ptp4l[43774.206]: assuming the grand master role

ptp4l[43784.208]: timed out while polling for tx timestamp
ptp4l[43784.208]: increasing tx_timestamp_timeout may correct this issue, but it is likely caused by a driver bug
ptp4l[43784.208]: port 1: send sync failed
ptp4l[43784.208]: port 1: MASTER to FAULTY on FAULT_DETECTED (FT_UNSPECIFIED)
ptp4l[43784.567]: port 1: link up
ptp4l[43784.587]: port 1: FAULTY to LISTENING on INIT_COMPLETE
ptp4l[43784.587]: port 1: link up
ptp4l[43784.587]: port 1: link up
ptp4l[43790.675]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[43790.675]: selected best master clock f4e9d4.fffe.9462f0
ptp4l[43790.675]: assuming the grand master role

# ./phc2sys -a -r -m
phc2sys[43761.356]: reconfiguring after port state change
phc2sys[43761.356]: selecting p4p1 for synchronization
phc2sys[43761.356]: nothing to synchronize

phc2sys[43767.357]: port f4e9d4.fffe.9462f0-1 changed state
phc2sys[43767.357]: reconfiguring after port state change
phc2sys[43767.357]: selecting p4p1 for synchronization
phc2sys[43767.357]: nothing to synchronize
phc2sys[43774.358]: port f4e9d4.fffe.9462f0-1 changed state
phc2sys[43774.358]: reconfiguring after port state change
phc2sys[43774.358]: selecting p4p2 for synchronization
phc2sys[43774.358]: nothing to synchronize

phc2sys[43785.360]: port f4e9d4.fffe.9462f0-1 changed state
phc2sys[43785.360]: reconfiguring after port state change
phc2sys[43785.360]: selecting p4p2 for synchronization
phc2sys[43785.360]: nothing to synchronize
phc2sys[43791.361]: port f4e9d4.fffe.9462f0-1 changed state
phc2sys[43791.361]: reconfiguring after port state change
phc2sys[43791.361]: selecting p4p1 for synchronization
phc2sys[43791.361]: nothing to synchronize

Hangbin Liu (8):
  rtnl: use ifinfomsg instead of rtgenmsg to request link info
  rtnl: extend struct interface and add new function rtnl_link_info
  port: track interface info in port
  port: add new function port_set_phc
  clock: use ts interface to get ts info
  clock: need check required_modes before use new ts info
  transport: pass struct interface instread of name to transport_open
  phc2sys: update clock clkid and phc_index if device changed

 clock.c             |  79 +++++++++++++++++++++++----------
 config.c            |   1 -
 config.h            |   1 +
 phc2sys.c           |  50 +++++++++++++++++++--
 pmc_common.c        |   5 ++-
 port.c              |  18 ++++++--
 port.h              |  14 ++++++
 raw.c               |   5 ++-
 rtnl.c              | 123 ++++++++++++++++++++++++++++++++++++++++++++++++----
 rtnl.h              |  40 +++++++++++++++--
 transport.c         |   4 +-
 transport.h         |   3 +-
 transport_private.h |   4 +-
 udp.c               |   7 +--
 udp6.c              |   7 +--
 uds.c               |   3 +-
 16 files changed, 308 insertions(+), 56 deletions(-)

-- 
2.5.5

[Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Hangbin L. <liu...@gm...>]

Also add a new parameter interface index incase we want to query specific
interface information.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 clock.c |  2 +-
 rtnl.c  | 12 +++++++-----
 rtnl.h  |  7 ++++---
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/clock.c b/clock.c
index b6afba9..59b5f0c 100644
--- a/clock.c
+++ b/clock.c
@@ -1165,7 +1165,7 @@ struct clock *clock_create(enum clock_type type, struct config *config,
 	}
 	port_dispatch(c->uds_port, EV_INITIALIZE, 0);
 	if (c->pollfd[0].fd >= 0) {
-		rtnl_link_query(c->pollfd[0].fd);
+		rtnl_link_query(c->pollfd[0].fd, 0);
 	}
 	return c;
 }
diff --git a/rtnl.c b/rtnl.c
index 5cddc4b..39faeb7 100644
--- a/rtnl.c
+++ b/rtnl.c
@@ -42,7 +42,7 @@ int rtnl_close(int fd)
 	return close(fd);
 }
 
-int rtnl_link_query(int fd)
+int rtnl_link_query(int fd, unsigned int if_index)
 {
 	struct sockaddr_nl sa;
 	struct msghdr msg;
@@ -51,19 +51,21 @@ int rtnl_link_query(int fd)
 
 	struct {
 		struct nlmsghdr hdr;
-		struct rtgenmsg gen;
+		struct ifinfomsg ifm;
 	} __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_len = NLMSG_LENGTH(sizeof(request.ifm));
 	request.hdr.nlmsg_type = RTM_GETLINK;
-	request.hdr.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP;
+	request.hdr.nlmsg_flags = NLM_F_REQUEST;
 	request.hdr.nlmsg_seq = 1;
 	request.hdr.nlmsg_pid = 0;
-	request.gen.rtgen_family = AF_UNSPEC;
+	request.ifm.ifi_family = AF_UNSPEC;
+	request.ifm.ifi_index = if_index;
+	request.ifm.ifi_change = 0xffffffff;
 
 	iov.iov_base = &request;
 	iov.iov_len = sizeof(request);
diff --git a/rtnl.h b/rtnl.h
index f1871f2..bd74cba 100644
--- a/rtnl.h
+++ b/rtnl.h
@@ -31,10 +31,11 @@ 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.
+ * @param fd     A socket obtained via rtnl_open().
+ * @param index  A interface index.
+ * @return       Zero on success, non-zero otherwise.
  */
-int rtnl_link_query(int fd);
+int rtnl_link_query(int fd, unsigned int index);
 
 /**
  * Read kernel messages looking for a link up/down events.
-- 
2.5.5

[Linuxptp-devel] [PATCH 2/8] rtnl: extend struct interface and add new function rtnl_link_info

[Hangbin L. <liu...@gm...>]

Add new element ts_iface in struct interface to track real ts interface.

Add function rtnl_link_info() to get active interface. If no slave interface,
then use our own name as ts interface.

Also add new parameter ts_iface for function clock_link_status() to make aware
of interface change.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 clock.c  |   2 +-
 config.h |   1 +
 rtnl.c   | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 rtnl.h   |  33 ++++++++++++++++++-
 4 files changed, 142 insertions(+), 5 deletions(-)

diff --git a/clock.c b/clock.c
index 59b5f0c..5e9f2cd 100644
--- a/clock.c
+++ b/clock.c
@@ -326,7 +326,7 @@ static void clock_freq_est_reset(struct clock *c)
 	c->fest.count = 0;
 }
 
-static void clock_link_status(void *ctx, int index, int linkup)
+static void clock_link_status(void *ctx, int index, int linkup, char *ts_iface)
 {
 	struct clock *c = ctx;
 	struct port *p;
diff --git a/config.h b/config.h
index 1cc7051..f0ae3fe 100644
--- a/config.h
+++ b/config.h
@@ -36,6 +36,7 @@
 struct interface {
 	STAILQ_ENTRY(interface) list;
 	char name[MAX_IFNAME_SIZE + 1];
+	char ts_iface[MAX_IFNAME_SIZE + 1];
 	struct sk_ts_info ts_info;
 };
 
diff --git a/rtnl.c b/rtnl.c
index 39faeb7..971f273 100644
--- a/rtnl.c
+++ b/rtnl.c
@@ -18,8 +18,6 @@
  */
 #include <asm/types.h>
 #include <sys/socket.h> /* Must come before linux/netlink.h on some systems. */
-#include <linux/netlink.h>
-#include <linux/rtnetlink.h>
 #include <net/if.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -84,6 +82,60 @@ int rtnl_link_query(int fd, unsigned int if_index)
 	return 0;
 }
 
+static inline __u32 rta_getattr_u32(const struct rtattr *rta)
+{
+	return *(__u32 *)RTA_DATA(rta);
+}
+static inline const char *rta_getattr_str(const struct rtattr *rta)
+{
+	return (const char *)RTA_DATA(rta);
+}
+int rtnl_rtattr_parse(struct rtattr *tb[], int max, struct rtattr *rta, int len)
+{
+	unsigned short type;
+
+	memset(tb, 0, sizeof(struct rtattr *) * (max + 1));
+	while (RTA_OK(rta, len)) {
+		type = rta->rta_type;
+		if ((type <= max) && (!tb[type]))
+			tb[type] = rta;
+		rta = RTA_NEXT(rta, len);
+	}
+	if (len)
+		fprintf(stderr, "!!!Deficit %d, rta_len=%d\n",
+			len, rta->rta_len);
+	return 0;
+}
+
+int rtnl_linkinfo_parse(struct rtattr *rta, char *device)
+{
+	int index;
+	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
+	struct rtattr *bond[IFLA_BOND_MAX+1];
+
+	rtnl_nested_rtattr_parse(linkinfo, IFLA_INFO_MAX, rta);
+
+	if (linkinfo[IFLA_INFO_KIND]) {
+		const char *kind = rta_getattr_str(linkinfo[IFLA_INFO_KIND]);
+
+		if (kind && !strncmp(kind, "bond", 4) &&
+		    linkinfo[IFLA_INFO_DATA]) {
+			rtnl_nested_rtattr_parse(bond, IFLA_BOND_MAX,
+						 linkinfo[IFLA_INFO_DATA]);
+
+			if (bond[IFLA_BOND_ACTIVE_SLAVE]) {
+				index = rta_getattr_u32(bond[IFLA_BOND_ACTIVE_SLAVE]);
+
+				if (!if_indextoname(index, device)) {
+					pr_err("failed to get device name: %m");
+					return -1;
+				}
+			}
+		}
+	}
+	return 0;
+}
+
 int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
 {
 	int index, len;
@@ -92,6 +144,18 @@ int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
 	struct msghdr msg;
 	struct nlmsghdr *nh;
 	struct ifinfomsg *info = NULL;
+	char *device;
+	struct rtattr *tb[IFLA_MAX+1];
+
+	if (cb)
+		device = calloc(1, sizeof(MAX_IFNAME_SIZE + 1));
+	else
+		device = (char *)ctx;
+
+	if(!device) {
+		fprintf(stderr, "rtnl: no enought memory for device name\n");
+		return -1;
+	}
 
 	if (!rtnl_buf) {
 		rtnl_len = 4096;
@@ -140,7 +204,18 @@ int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
 			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);
+
+			rtnl_rtattr_parse(tb, IFLA_MAX, IFLA_RTA(info),
+					  IFLA_PAYLOAD(nh));
+
+			if (tb[IFLA_LINKINFO])
+				rtnl_linkinfo_parse(tb[IFLA_LINKINFO], device);
+
+			if (cb) {
+				cb(ctx, index, info->ifi_flags & IFF_RUNNING ? 1 : 0, device);
+				free(device);
+			}
+
 		}
 	}
 	return 0;
@@ -167,3 +242,33 @@ int rtnl_open(void)
 	}
 	return fd;
 }
+
+int rtnl_link_info(struct interface *iface)
+{
+	int fd, index;
+
+	index = if_nametoindex(iface->name);
+
+	if (index == 0) {
+		pr_err("failed to get interface %s index: %m", iface->name);
+		goto failed;
+	}
+
+	fd = rtnl_open();
+	if (rtnl_link_query(fd, index))
+		goto failed;
+	if (rtnl_link_status(fd, NULL, iface->ts_iface))
+		goto failed;
+
+	/* If we do not have a slave, then use our own interface name
+	 * as ts_iface
+	 */
+	if (iface->ts_iface[0] != '\0')
+		strncpy(iface->ts_iface, iface->name, MAX_IFNAME_SIZE);
+
+	rtnl_close(fd);
+
+	return 0;
+failed:
+	return -1;
+}
diff --git a/rtnl.h b/rtnl.h
index bd74cba..ed46cb7 100644
--- a/rtnl.h
+++ b/rtnl.h
@@ -20,7 +20,12 @@
 #ifndef HAVE_RTNL_H
 #define HAVE_RTNL_H
 
-typedef void (*rtnl_callback)(void *ctx, int index, int linkup);
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+
+#include "config.h"
+
+typedef void (*rtnl_callback)(void *ctx, int index, int linkup, char *device);
 
 /**
  * Close a RT netlink socket.
@@ -38,6 +43,26 @@ int rtnl_close(int fd);
 int rtnl_link_query(int fd, unsigned int index);
 
 /**
+ * Parase the rtattr info
+ * @param tb     rtattr array.
+ * @param max    max type of ratttr.
+ * @param rta    rta header
+ * @param len    rta playload length
+ * @return       Zero on success, or -1 on error.
+ */
+int rtnl_rtattr_parse(struct rtattr *tb[], int max, struct rtattr *rta, int len);
+#define rtnl_nested_rtattr_parse(tb, max, rta) \
+	(rtnl_rtattr_parse((tb), (max), RTA_DATA(rta), RTA_PAYLOAD(rta)))
+
+/**
+ * Parase the link info
+ * @param rta    rta header
+ * @param device interface name.
+ * @return       Zero on success, or -1 on error.
+ */
+int rtnl_linkinfo_parse(struct rtattr *rta, char *device);
+
+/**
  * 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.
@@ -52,4 +77,10 @@ int rtnl_link_status(int fd, rtnl_callback cb, void *ctx);
  */
 int rtnl_open(void);
 
+/**
+ * Get interface link status and ts_iface information
+ * @param iface  struct interface.
+ * @return       Zero on success, or -1 on error.
+ */
+int rtnl_link_info(struct interface *iface);
 #endif
-- 
2.5.5

Re: [Linuxptp-devel] [PATCH 2/8] rtnl: extend struct interface and add new function rtnl_link_info

[Richard C. <ric...@gm...>]

This patch has a number of coding style issues...

On Sat, Jun 24, 2017 at 04:48:30PM +0800, Hangbin Liu wrote:
> diff --git a/rtnl.c b/rtnl.c
> index 39faeb7..971f273 100644
> --- a/rtnl.c
> +++ b/rtnl.c
> @@ -18,8 +18,6 @@
>   */
>  #include <asm/types.h>
>  #include <sys/socket.h> /* Must come before linux/netlink.h on some systems. */
> -#include <linux/netlink.h>
> -#include <linux/rtnetlink.h>
>  #include <net/if.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -84,6 +82,60 @@ int rtnl_link_query(int fd, unsigned int if_index)
>  	return 0;
>  }
>  
> +static inline __u32 rta_getattr_u32(const struct rtattr *rta)
> +{
> +	return *(__u32 *)RTA_DATA(rta);
> +}

A function must be followed by one blank line.

> +static inline const char *rta_getattr_str(const struct rtattr *rta)
> +{
> +	return (const char *)RTA_DATA(rta);
> +}

Here too.

> +int rtnl_rtattr_parse(struct rtattr *tb[], int max, struct rtattr *rta, int len)
> +{
> +	unsigned short type;
> +
> +	memset(tb, 0, sizeof(struct rtattr *) * (max + 1));
> +	while (RTA_OK(rta, len)) {
> +		type = rta->rta_type;
> +		if ((type <= max) && (!tb[type]))
> +			tb[type] = rta;
> +		rta = RTA_NEXT(rta, len);
> +	}
> +	if (len)
> +		fprintf(stderr, "!!!Deficit %d, rta_len=%d\n",
> +			len, rta->rta_len);
> +	return 0;
> +}
> +
> +int rtnl_linkinfo_parse(struct rtattr *rta, char *device)
> +{
> +	int index;
> +	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
> +	struct rtattr *bond[IFLA_BOND_MAX+1];

array[MAX + 1];
Spaces   ^^^

> +	rtnl_nested_rtattr_parse(linkinfo, IFLA_INFO_MAX, rta);
> +
> +	if (linkinfo[IFLA_INFO_KIND]) {
> +		const char *kind = rta_getattr_str(linkinfo[IFLA_INFO_KIND]);
> +
> +		if (kind && !strncmp(kind, "bond", 4) &&
> +		    linkinfo[IFLA_INFO_DATA]) {
> +			rtnl_nested_rtattr_parse(bond, IFLA_BOND_MAX,
> +						 linkinfo[IFLA_INFO_DATA]);
> +
> +			if (bond[IFLA_BOND_ACTIVE_SLAVE]) {
> +				index = rta_getattr_u32(bond[IFLA_BOND_ACTIVE_SLAVE]);
> +
> +				if (!if_indextoname(index, device)) {
> +					pr_err("failed to get device name: %m");
> +					return -1;
> +				}
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
>  int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
>  {
>  	int index, len;
> @@ -92,6 +144,18 @@ int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
>  	struct msghdr msg;
>  	struct nlmsghdr *nh;
>  	struct ifinfomsg *info = NULL;
> +	char *device;
> +	struct rtattr *tb[IFLA_MAX+1];
> +
> +	if (cb)
> +		device = calloc(1, sizeof(MAX_IFNAME_SIZE + 1));
> +	else
> +		device = (char *)ctx;
> +
> +	if(!device) {

One space after keywords please.

> +		fprintf(stderr, "rtnl: no enought memory for device name\n");
> +		return -1;
> +	}
>  
>  	if (!rtnl_buf) {
>  		rtnl_len = 4096;

> @@ -38,6 +43,26 @@ int rtnl_close(int fd);
>  int rtnl_link_query(int fd, unsigned int index);
>  
>  /**
> + * Parase the rtattr info
> + * @param tb     rtattr array.
> + * @param max    max type of ratttr.
> + * @param rta    rta header
> + * @param len    rta playload length
> + * @return       Zero on success, or -1 on error.
> + */
> +int rtnl_rtattr_parse(struct rtattr *tb[], int max, struct rtattr *rta, int len);

This function should be private in rtnl.c.

> +#define rtnl_nested_rtattr_parse(tb, max, rta) \
> +	(rtnl_rtattr_parse((tb), (max), RTA_DATA(rta), RTA_PAYLOAD(rta)))

This also.

> +/**
> + * Parase the link info
> + * @param rta    rta header
> + * @param device interface name.
> + * @return       Zero on success, or -1 on error.
> + */
> +int rtnl_linkinfo_parse(struct rtattr *rta, char *device);

This also.

Thanks,
Richard

[Linuxptp-devel] [PATCH 3/8] port: track interface info in port

[Hangbin L. <liu...@gm...>]

Add interface element in struct port. And update ts iface info after
device fail over.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 clock.c | 7 +++++++
 port.c  | 7 +++++++
 port.h  | 7 +++++++
 3 files changed, 21 insertions(+)

diff --git a/clock.c b/clock.c
index 5e9f2cd..67bdecb 100644
--- a/clock.c
+++ b/clock.c
@@ -331,6 +331,7 @@ static void clock_link_status(void *ctx, int index, int linkup, char *ts_iface)
 	struct clock *c = ctx;
 	struct port *p;
 	char key[16];
+	struct interface *iface;
 
 	snprintf(key, sizeof(key), "%d", index);
 	p = hash_lookup(c->index2port, key);
@@ -338,6 +339,12 @@ static void clock_link_status(void *ctx, int index, int linkup, char *ts_iface)
 		return;
 	}
 	port_link_status_set(p, linkup);
+
+	iface = port_interface_get(p);
+	if (ts_iface[0] != '\0' && strcmp(iface->ts_iface, ts_iface)) {
+		strncpy(iface->ts_iface, ts_iface, MAX_IFNAME_SIZE);
+	}
+
 	if (linkup) {
 		port_dispatch(p, EV_FAULT_CLEARED, 0);
 	} else {
diff --git a/port.c b/port.c
index ec02825..b28849f 100644
--- a/port.c
+++ b/port.c
@@ -66,6 +66,7 @@ struct nrate_estimator {
 struct port {
 	LIST_ENTRY(port) list;
 	char *name;
+	struct interface *iface;
 	struct clock *clock;
 	struct transport *trp;
 	enum timestamp_type timestamping;
@@ -2378,6 +2379,11 @@ void port_link_status_set(struct port *p, int up)
 	pr_notice("port %hu: link %s", portnum(p), up ? "up" : "down");
 }
 
+struct interface *port_interface_get(struct port *p)
+{
+	return p->iface;
+}
+
 int port_manage(struct port *p, struct port *ingress, struct ptp_message *msg)
 {
 	struct management_tlv *mgt;
@@ -2586,6 +2592,7 @@ struct port *port_open(int phc_index,
 	}
 
 	p->name = interface->name;
+	p->iface = interface;
 	p->asymmetry = config_get_int(cfg, p->name, "delayAsymmetry");
 	p->asymmetry <<= 16;
 	p->announce_span = transport == TRANS_UDS ? 0 : ANNOUNCE_SPAN;
diff --git a/port.h b/port.h
index b00bc64..7f9be42 100644
--- a/port.h
+++ b/port.h
@@ -139,6 +139,13 @@ int port_link_status_get(struct port *p);
 void port_link_status_set(struct port *p, int up);
 
 /**
+ * Obtain the interface pointer of a port.
+ * @param p        A port instance.
+ * @return         Interface pointer of the port.
+ */
+struct interface *port_interface_get(struct port *p);
+
+/**
  * 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.5.5

[Linuxptp-devel] [PATCH 4/8] port: add new function port_set_phc

[Hangbin L. <liu...@gm...>]

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 port.c | 5 +++++
 port.h | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/port.c b/port.c
index b28849f..d919c3e 100644
--- a/port.c
+++ b/port.c
@@ -2384,6 +2384,11 @@ struct interface *port_interface_get(struct port *p)
 	return p->iface;
 }
 
+void port_set_phc(struct port *p, int phc_index)
+{
+	p->phc_index = phc_index;
+}
+
 int port_manage(struct port *p, struct port *ingress, struct ptp_message *msg)
 {
 	struct management_tlv *mgt;
diff --git a/port.h b/port.h
index 7f9be42..b0d5db3 100644
--- a/port.h
+++ b/port.h
@@ -146,6 +146,13 @@ void port_link_status_set(struct port *p, int up);
 struct interface *port_interface_get(struct port *p);
 
 /**
+ * Sets the phc index for a port.
+ * @param p          A port instance.
+ * @param phc_index  the new phc index.
+ */
+void port_set_phc(struct port *p, int phc_index);
+
+/**
  * 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.5.5

[Linuxptp-devel] [PATCH 5/8] clock: use ts interface to get ts info

[Hangbin L. <liu...@gm...>]

Now the ts interface will be either the active slave or the interface name,
which is the exactly interface we need to get ts info.

If there is a fail over and ts_iface changed. We need to switch phc index and
reset the port status.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 clock.c  | 14 ++++++++++----
 config.c |  1 -
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/clock.c b/clock.c
index 67bdecb..582438c 100644
--- a/clock.c
+++ b/clock.c
@@ -341,11 +341,15 @@ static void clock_link_status(void *ctx, int index, int linkup, char *ts_iface)
 	port_link_status_set(p, linkup);
 
 	iface = port_interface_get(p);
-	if (ts_iface[0] != '\0' && strcmp(iface->ts_iface, ts_iface)) {
+	if (linkup && ts_iface[0] != '\0' && strcmp(iface->ts_iface, ts_iface)) {
 		strncpy(iface->ts_iface, ts_iface, MAX_IFNAME_SIZE);
-	}
-
-	if (linkup) {
+		sk_get_ts_info(iface->ts_iface, &iface->ts_info);
+		if (iface->ts_info.valid) {
+			port_set_phc(p, iface->ts_info.phc_index);
+			clock_switch_phc(c, iface->ts_info.phc_index);
+			port_dispatch(p, EV_INITIALIZE, 0);
+		}
+	} else if (linkup) {
 		port_dispatch(p, EV_FAULT_CLEARED, 0);
 	} else {
 		port_dispatch(p, EV_FAULT_DETECTED, 0);
@@ -991,6 +995,8 @@ struct clock *clock_create(enum clock_type type, struct config *config,
 		break;
 	}
 	STAILQ_FOREACH(iface, &config->interfaces, list) {
+		rtnl_link_info(iface);
+		sk_get_ts_info(iface->ts_iface, &iface->ts_info);
 		if (iface->ts_info.valid &&
 		    ((iface->ts_info.so_timestamping & required_modes) != required_modes)) {
 			pr_err("interface '%s' does not support "
diff --git a/config.c b/config.c
index e6fe676..bbaf36e 100644
--- a/config.c
+++ b/config.c
@@ -633,7 +633,6 @@ struct interface *config_create_interface(char *name, struct config *cfg)
 	}
 
 	strncpy(iface->name, name, MAX_IFNAME_SIZE);
-	sk_get_ts_info(iface->name, &iface->ts_info);
 	STAILQ_INSERT_TAIL(&cfg->interfaces, iface, list);
 	cfg->n_interfaces++;
 
-- 
2.5.5

[Linuxptp-devel] [PATCH 6/8] clock: need check required_modes before use new ts info

[Hangbin L. <liu...@gm...>]

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 clock.c | 60 +++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 19 deletions(-)

diff --git a/clock.c b/clock.c
index 582438c..256d52f 100644
--- a/clock.c
+++ b/clock.c
@@ -110,6 +110,7 @@ struct clock {
 	int time_flags;  /* grand master role */
 	int time_source; /* grand master role */
 	enum servo_state servo_state;
+	enum timestamp_type timestamping;
 	tmv_t master_offset;
 	tmv_t path_delay;
 	tmv_t ingress_ts;
@@ -326,12 +327,40 @@ static void clock_freq_est_reset(struct clock *c)
 	c->fest.count = 0;
 }
 
+static int clock_required_modes(enum timestamp_type timestamping)
+{
+	int required_modes = 0;
+
+	switch (timestamping) {
+	case TS_SOFTWARE:
+		required_modes |= SOF_TIMESTAMPING_TX_SOFTWARE |
+			SOF_TIMESTAMPING_RX_SOFTWARE |
+			SOF_TIMESTAMPING_SOFTWARE;
+		break;
+	case TS_LEGACY_HW:
+		required_modes |= SOF_TIMESTAMPING_TX_HARDWARE |
+			SOF_TIMESTAMPING_RX_HARDWARE |
+			SOF_TIMESTAMPING_SYS_HARDWARE;
+		break;
+	case TS_HARDWARE:
+	case TS_ONESTEP:
+		required_modes |= SOF_TIMESTAMPING_TX_HARDWARE |
+			SOF_TIMESTAMPING_RX_HARDWARE |
+			SOF_TIMESTAMPING_RAW_HARDWARE;
+		break;
+	default:
+		break;
+	}
+
+	return required_modes;
+}
 static void clock_link_status(void *ctx, int index, int linkup, char *ts_iface)
 {
 	struct clock *c = ctx;
 	struct port *p;
 	char key[16];
 	struct interface *iface;
+	int required_modes = 0;
 
 	snprintf(key, sizeof(key), "%d", index);
 	p = hash_lookup(c->index2port, key);
@@ -344,6 +373,15 @@ static void clock_link_status(void *ctx, int index, int linkup, char *ts_iface)
 	if (linkup && ts_iface[0] != '\0' && strcmp(iface->ts_iface, ts_iface)) {
 		strncpy(iface->ts_iface, ts_iface, MAX_IFNAME_SIZE);
 		sk_get_ts_info(iface->ts_iface, &iface->ts_info);
+
+		required_modes = clock_required_modes(c->timestamping);
+		if (iface->ts_info.valid &&
+		    (iface->ts_info.so_timestamping & required_modes) != required_modes) {
+			pr_err("interface '%s' does not support "
+			       "requested timestamping mode", iface->ts_iface);
+			return;
+		}
+
 		if (iface->ts_info.valid) {
 			port_set_phc(p, iface->ts_info.phc_index);
 			clock_switch_phc(c, iface->ts_info.phc_index);
@@ -976,31 +1014,14 @@ struct clock *clock_create(enum clock_type type, struct config *config,
 	}
 
 	/* Check the time stamping mode on each interface. */
-	switch (timestamping) {
-	case TS_SOFTWARE:
-		required_modes |= SOF_TIMESTAMPING_TX_SOFTWARE |
-			SOF_TIMESTAMPING_RX_SOFTWARE |
-			SOF_TIMESTAMPING_SOFTWARE;
-		break;
-	case TS_LEGACY_HW:
-		required_modes |= SOF_TIMESTAMPING_TX_HARDWARE |
-			SOF_TIMESTAMPING_RX_HARDWARE |
-			SOF_TIMESTAMPING_SYS_HARDWARE;
-		break;
-	case TS_HARDWARE:
-	case TS_ONESTEP:
-		required_modes |= SOF_TIMESTAMPING_TX_HARDWARE |
-			SOF_TIMESTAMPING_RX_HARDWARE |
-			SOF_TIMESTAMPING_RAW_HARDWARE;
-		break;
-	}
+	required_modes = clock_required_modes(timestamping);
 	STAILQ_FOREACH(iface, &config->interfaces, list) {
 		rtnl_link_info(iface);
 		sk_get_ts_info(iface->ts_iface, &iface->ts_info);
 		if (iface->ts_info.valid &&
 		    ((iface->ts_info.so_timestamping & required_modes) != required_modes)) {
 			pr_err("interface '%s' does not support "
-			       "requested timestamping mode", iface->name);
+			       "requested timestamping mode", iface->ts_iface);
 			return NULL;
 		}
 	}
@@ -1059,6 +1080,7 @@ struct clock *clock_create(enum clock_type type, struct config *config,
 	c->kernel_leap = config_get_int(config, NULL, "kernel_leap");
 	c->utc_offset = c->current_utc_offset = config_get_int(config, NULL, "utc_offset");
 	c->time_source = config_get_int(config, NULL, "timeSource");
+	c->timestamping = timestamping;
 
 	if (c->free_running) {
 		c->clkid = CLOCK_INVALID;
-- 
2.5.5

[Linuxptp-devel] [PATCH 7/8] transport: pass struct interface instread of name to transport_open

[Hangbin L. <liu...@gm...>]

Pass struct interface so we can use ts_iface in HW filter.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 pmc_common.c        | 5 ++++-
 port.c              | 4 ++--
 raw.c               | 5 +++--
 transport.c         | 4 ++--
 transport.h         | 3 ++-
 transport_private.h | 4 ++--
 udp.c               | 7 ++++---
 udp6.c              | 7 ++++---
 uds.c               | 3 ++-
 9 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/pmc_common.c b/pmc_common.c
index d92b0cd..447cf99 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -67,6 +67,7 @@ struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
 		       int zero_datalen)
 {
 	struct pmc *pmc;
+	struct interface iface;
 
 	pmc = calloc(1, sizeof *pmc);
 	if (!pmc)
@@ -90,7 +91,9 @@ struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
 		pr_err("failed to create transport");
 		goto failed;
 	}
-	if (transport_open(pmc->transport, iface_name,
+
+	strncpy(iface.name, iface_name, MAX_IFNAME_SIZE);
+	if (transport_open(pmc->transport, &iface,
 			   &pmc->fdarray, TS_SOFTWARE)) {
 		pr_err("failed to open transport");
 		goto failed;
diff --git a/port.c b/port.c
index d919c3e..78bf190 100644
--- a/port.c
+++ b/port.c
@@ -1493,7 +1493,7 @@ static int port_initialize(struct port *p)
 			goto no_timers;
 		}
 	}
-	if (transport_open(p->trp, p->name, &p->fda, p->timestamping))
+	if (transport_open(p->trp, p->iface, &p->fda, p->timestamping))
 		goto no_tropen;
 
 	for (i = 0; i < N_TIMER_FDS; i++) {
@@ -1528,7 +1528,7 @@ static int port_renew_transport(struct port *p)
 	}
 	transport_close(p->trp, &p->fda);
 	port_clear_fda(p, FD_ANNOUNCE_TIMER);
-	res = transport_open(p->trp, p->name, &p->fda, p->timestamping);
+	res = transport_open(p->trp, p->iface, &p->fda, p->timestamping);
 	/* Need to call clock_fda_changed even if transport_open failed in
 	 * order to update clock to the now closed descriptors. */
 	clock_fda_changed(p->clock);
diff --git a/raw.c b/raw.c
index 73e45b4..abee8f6 100644
--- a/raw.c
+++ b/raw.c
@@ -198,15 +198,16 @@ static void addr_to_mac(void *mac, struct address *addr)
 	memcpy(mac, &addr->sll.sll_addr, MAC_LEN);
 }
 
-static int raw_open(struct transport *t, const char *name,
+static int raw_open(struct transport *t, struct interface *iface,
 		    struct fdarray *fda, enum timestamp_type ts_type)
 {
 	struct raw *raw = container_of(t, struct raw, t);
 	unsigned char ptp_dst_mac[MAC_LEN];
 	unsigned char p2p_dst_mac[MAC_LEN];
 	int efd, gfd;
-	char *str;
+	char *str, *name;
 
+	name = iface->ts_iface;
 	str = config_get_string(t->cfg, name, "ptp_dst_mac");
 	if (str2mac(str, ptp_dst_mac)) {
 		pr_err("invalid ptp_dst_mac %s", str);
diff --git a/transport.c b/transport.c
index d24c05b..3541394 100644
--- a/transport.c
+++ b/transport.c
@@ -31,10 +31,10 @@ int transport_close(struct transport *t, struct fdarray *fda)
 	return t->close(t, fda);
 }
 
-int transport_open(struct transport *t, const char *name,
+int transport_open(struct transport *t, struct interface *iface,
 		   struct fdarray *fda, enum timestamp_type tt)
 {
-	return t->open(t, name, fda, tt);
+	return t->open(t, iface, fda, tt);
 }
 
 int transport_recv(struct transport *t, int fd, struct ptp_message *msg)
diff --git a/transport.h b/transport.h
index 5d6ba98..15616bb 100644
--- a/transport.h
+++ b/transport.h
@@ -27,6 +27,7 @@
 #include "msg.h"
 
 struct config;
+struct interface;
 
 /* Values from networkProtocol enumeration 7.4.1 Table 3 */
 enum transport_type {
@@ -54,7 +55,7 @@ struct transport;
 
 int transport_close(struct transport *t, struct fdarray *fda);
 
-int transport_open(struct transport *t, const char *name,
+int transport_open(struct transport *t, struct interface *iface,
 		   struct fdarray *fda, enum timestamp_type tt);
 
 int transport_recv(struct transport *t, int fd, struct ptp_message *msg);
diff --git a/transport_private.h b/transport_private.h
index b54f32a..7530896 100644
--- a/transport_private.h
+++ b/transport_private.h
@@ -32,8 +32,8 @@ struct transport {
 
 	int (*close)(struct transport *t, struct fdarray *fda);
 
-	int (*open)(struct transport *t, const char *name, struct fdarray *fda,
-		    enum timestamp_type tt);
+	int (*open)(struct transport *t, struct interface *iface,
+		    struct fdarray *fda, enum timestamp_type tt);
 
 	int (*recv)(struct transport *t, int fd, void *buf, int buflen,
 		    struct address *addr, struct hw_timestamp *hwts);
diff --git a/udp.c b/udp.c
index 530a2ee..d68c8b3 100644
--- a/udp.c
+++ b/udp.c
@@ -152,12 +152,13 @@ enum { MC_PRIMARY, MC_PDELAY };
 
 static struct in_addr mcast_addr[2];
 
-static int udp_open(struct transport *t, const char *name, struct fdarray *fda,
-		    enum timestamp_type ts_type)
+static int udp_open(struct transport *t, struct interface *iface,
+		    struct fdarray *fda, enum timestamp_type ts_type)
 {
 	struct udp *udp = container_of(t, struct udp, t);
 	uint8_t event_dscp, general_dscp;
 	int efd, gfd, ttl;
+	char *name = iface->name;
 
 	ttl = config_get_int(t->cfg, name, "udp_ttl");
 	udp->mac.len = 0;
@@ -180,7 +181,7 @@ static int udp_open(struct transport *t, const char *name, struct fdarray *fda,
 	if (gfd < 0)
 		goto no_general;
 
-	if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
+	if (sk_timestamping_init(efd, iface->ts_iface, ts_type, TRANS_UDP_IPV4))
 		goto no_timestamping;
 
 	if (sk_general_init(gfd))
diff --git a/udp6.c b/udp6.c
index 89e27bf..cbc483f 100644
--- a/udp6.c
+++ b/udp6.c
@@ -160,12 +160,13 @@ enum { MC_PRIMARY, MC_PDELAY };
 
 static struct in6_addr mc6_addr[2];
 
-static int udp6_open(struct transport *t, const char *name, struct fdarray *fda,
-		    enum timestamp_type ts_type)
+static int udp6_open(struct transport *t, struct interface *iface,
+		     struct fdarray *fda, enum timestamp_type ts_type)
 {
 	struct udp6 *udp6 = container_of(t, struct udp6, t);
 	uint8_t event_dscp, general_dscp;
 	int efd, gfd, hop_limit;
+	char *name = iface->name;
 
 	hop_limit = config_get_int(t->cfg, name, "udp_ttl");
 	udp6->mac.len = 0;
@@ -190,7 +191,7 @@ static int udp6_open(struct transport *t, const char *name, struct fdarray *fda,
 	if (gfd < 0)
 		goto no_general;
 
-	if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
+	if (sk_timestamping_init(efd, iface->ts_iface, ts_type, TRANS_UDP_IPV6))
 		goto no_timestamping;
 
 	if (sk_general_init(gfd))
diff --git a/uds.c b/uds.c
index d5e8f50..7e11f63 100644
--- a/uds.c
+++ b/uds.c
@@ -52,13 +52,14 @@ static int uds_close(struct transport *t, struct fdarray *fda)
 	return 0;
 }
 
-static int uds_open(struct transport *t, const char *name, struct fdarray *fda,
+static int uds_open(struct transport *t, struct interface *iface, struct fdarray *fda,
 		    enum timestamp_type tt)
 {
 	int fd, err;
 	struct sockaddr_un sa;
 	struct uds *uds = container_of(t, struct uds, t);
 	char *uds_path = config_get_string(t->cfg, NULL, "uds_address");
+	char *name = iface->name;
 
 	fd = socket(AF_LOCAL, SOCK_DGRAM, 0);
 	if (fd < 0) {
-- 
2.5.5

[Linuxptp-devel] [PATCH 8/8] phc2sys: update clock clkid and phc_index if device changed

[Hangbin L. <liu...@gm...>]

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 phc2sys.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
 port.c    |  2 +-
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index b6f6719..35df731 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -128,6 +128,11 @@ static int clock_handle_leap(struct node *node, struct clock *clock,
 static int run_pmc_get_utc_offset(struct node *node, int timeout);
 static void run_pmc_events(struct node *node);
 
+static int normalize_state(int state);
+static int run_pmc_port_properties(struct node *node, int timeout,
+				   unsigned int port,
+				   int *state, int *tstamping, char *iface);
+
 static clockid_t clock_open(char *device, int *phc_index)
 {
 	struct sk_ts_info ts_info;
@@ -294,8 +299,47 @@ static struct port *port_add(struct node *node, unsigned int number,
 	return p;
 }
 
-static void clock_reinit(struct clock *clock)
+static void clock_reinit(struct node *node, struct clock *clock)
 {
+	struct port *p;
+	int state, timestamping, ret;
+	int phc_index = -1;
+	char iface[IFNAMSIZ];
+	clockid_t clkid = CLOCK_INVALID;
+
+	LIST_FOREACH(p, &node->ports, list) {
+		if (p->clock == clock) {
+			ret = run_pmc_port_properties(node, 1000, p->number,
+					              &state, &timestamping,
+						      iface);
+			if (ret == -1) {
+				/* port does not exist, ignore the port */
+				continue;
+			}
+			if (ret <= 0) {
+				pr_err("failed to get port properties");
+				return;
+			}
+			if (timestamping == TS_SOFTWARE) {
+				/* ignore ports with software time stamping */
+				continue;
+			}
+
+			p->state = normalize_state(state);
+		}
+	}
+
+	if (strcmp(clock->device, iface)) {
+		free(clock->device);
+		clock->device = strdup(iface);
+		clkid = clock_open(clock->device, &phc_index);
+		if (clkid == CLOCK_INVALID)
+			return;
+		phc_close(clock->clkid);
+		clock->clkid = clkid;
+		clock->phc_index = phc_index;
+	}
+
 	servo_reset(clock->servo);
 	clock->servo_state = SERVO_UNLOCKED;
 
@@ -322,7 +366,7 @@ static void reconfigure(struct node *node)
 
 		if (c->new_state) {
 			if (c->new_state == PS_MASTER)
-				clock_reinit(c);
+				clock_reinit(node, c);
 
 			c->state = c->new_state;
 			c->new_state = 0;
@@ -388,7 +432,7 @@ static void reconfigure(struct node *node)
 	} else if (rt) {
 		if (rt->state != PS_MASTER) {
 			rt->state = PS_MASTER;
-			clock_reinit(rt);
+			clock_reinit(node, rt);
 		}
 		pr_info("selecting %s for synchronization", rt->device);
 	}
diff --git a/port.c b/port.c
index 78bf190..eb8dac2 100644
--- a/port.c
+++ b/port.c
@@ -852,7 +852,7 @@ static int port_management_fill_response(struct port *target,
 		else
 			ppn->port_state = target->state;
 		ppn->timestamping = target->timestamping;
-		ptp_text_set(&ppn->interface, target->name);
+		ptp_text_set(&ppn->interface, target->iface->ts_iface);
 		datalen = sizeof(*ppn) + ppn->interface.length;
 		respond = 1;
 		break;
-- 
2.5.5

Re: [Linuxptp-devel] [PATCH 0/8] Add linuxptp bond failover support

[Richard C. <ric...@gm...>]

On Sat, Jun 24, 2017 at 04:48:28PM +0800, Hangbin Liu wrote:
> Since it's a large patch set. I may miss some part. Please let me know if
> you have any questions, comments, or concerns.

I have only just started to review, but overall the form and structure
of the series are okay.  I appreciate that the patches are at a
digestible level of granularity.


Thanks,
Richard

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Richard C. <ric...@gm...>]

On Sat, Jun 24, 2017 at 04:48:29PM +0800, Hangbin Liu wrote:
>  	struct {
>  		struct nlmsghdr hdr;
> -		struct rtgenmsg gen;
> +		struct ifinfomsg ifm;
>  	} __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_len = NLMSG_LENGTH(sizeof(request.ifm));
>  	request.hdr.nlmsg_type = RTM_GETLINK;
> -	request.hdr.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP;
> +	request.hdr.nlmsg_flags = NLM_F_REQUEST;
>  	request.hdr.nlmsg_seq = 1;
>  	request.hdr.nlmsg_pid = 0;
> -	request.gen.rtgen_family = AF_UNSPEC;
> +	request.ifm.ifi_family = AF_UNSPEC;
> +	request.ifm.ifi_index = if_index;
> +	request.ifm.ifi_change = 0xffffffff;

What about portability on older kernel versions?  We need to run on
kernels starting with v3.0.  The rtnetlink(7) man page says:

    struct ifinfomsg {
        unsigned char  ifi_family; /* AF_UNSPEC */
        unsigned short ifi_type;   /* Device type */
        int            ifi_index;  /* Interface index */
        unsigned int   ifi_flags;  /* Device flags  */
        unsigned int   ifi_change; /* change mask */
    };

    ifi_flags contains the device flags, see netdevice(7); ifi_index
    is  the  unique interface index (since Linux 3.7, it is possible
    to feed a nonzero value with the RTM_NEWLINK message, thus  cre‐
    ating a link with the given ifindex); ifi_change is reserved for
    future use and should be always set to 0xFFFFFFFF.

If we could always use a per-port rtnl socket, this would be an
improvement.  Back when I introduced rtnl, I wrote this:

    From 0b3c045a4236f128ab4c1592e6cc91adc1cd6eb0 Mon Sep 17 00:00:00 2001
    From: Richard Cochran <ric...@gm...>
    Date: Sun, 31 Jul 2016 12:08:24 +0200
    Subject: rtnl: Introduce RT netlink sockets.

    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.

At that time, I added one extra FD into clock->pollfdp[], and the
clock then farms out the information to the ports.  I really did not
to do it that way.  A better way would have been to add a new per-port
FD into fd.h, something like this:

    enum {
            FD_EVENT,
            FD_GENERAL,
            FD_ANNOUNCE_TIMER,
            FD_SYNC_RX_TIMER,
            FD_DELAY_TIMER,
            FD_QUALIFICATION_TIMER,
            FD_MANNO_TIMER,
            FD_SYNC_TX_TIMER,
/*here*/    FD_RTNL,
            N_POLLFD,
    };

    struct fdarray {
            int fd[N_POLLFD];
    };

This would simply your patch set, wouldn't it?

Thanks,
Richard

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Richard C. <ric...@gm...>]

On Fri, Jun 30, 2017 at 08:18:15AM +0200, Richard Cochran wrote:
> At that time, I added one extra FD into clock->pollfdp[], and the
> clock then farms out the information to the ports.  I really did not
> to do it that way.  A better way would have been to add a new per-port
> FD into fd.h, something like this:

I meant to say, "I really did not want to do that way."

I can't remember whether I even knew about the possibility of using
ifinfomsg.ifi_index to bind to a particular interface.  At the time, I
either overlooked it or I might have rejected this because of lack of
portability to kernels before v3.7.

In any case, kernel v3.7 is getting pretty old, and so I think we can:

1. remove the rtnl socket from clock.c

2. add one rtnl socket per port.

3. use ifinfomsg on kernel 3.7+

4. fall back to rtgenmsg on kernel 3.0 - 3.6 (only if needed).

   Q: What is the behavior of pre 3.7 kernels WRT ifinfomsg.ifi_index?
      If ifi_index is non-zero, does the kernel return EINVAL, or do
      you get information from all interfaces?

The only drawback is that, when running on earlier kernels, ptp4l will
duplicate work, handling all those per-port rtnl messages.  But this
is acceptable if we can improve the ptp4l code for the future.


Thanks,
Richard

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Hangbin L. <liu...@gm...>]

On Fri, Jun 30, 2017 at 08:44:36AM +0200, Richard Cochran wrote:
> On Fri, Jun 30, 2017 at 08:18:15AM +0200, Richard Cochran wrote:

> >       request.hdr.nlmsg_type = RTM_GETLINK;
> > -     request.hdr.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP;

NLM_F_DUMP flag call function rtnl_dump_ifinfo(), which will dump all
interface info.
> > +     request.hdr.nlmsg_flags = NLM_F_REQUEST;

NLM_F_REQUEST call rtnl_getlink() which get exactally the interface's info.

There is no much change for these two functions since v3.0.
> 
> What about portability on older kernel versions?  We need to run on
> kernels starting with v3.0.  The rtnetlink(7) man page says:
> 
>     ifi_flags contains the device flags, see netdevice(7); ifi_index
>     is  the  unique interface index (since Linux 3.7, it is possible
>     to feed a nonzero value with the RTM_NEWLINK message, thus  cre‐
>     ating a link with the given ifindex); ifi_change is reserved for

Before v3.7 we do not support create interface with given ifindex with
RTM_NEWLINK message[1]. Since we only need to get the interface info.
It should be safe to use ifinfomsg on kernel v3.0

But with your remind. I found we start to support bond rtnetlink message
from kernel v3.13. So before v3.13, we could not get slave info and ptp4l
with bond interface will fail with error not support when check ts info.
I think this should be OK with document "bond fail over support start with
kernel v3.13".

What do you think?

>     future use and should be always set to 0xFFFFFFFF.
> > At that time, I added one extra FD into clock->pollfdp[], and the
> > clock then farms out the information to the ports.  I really did not
> > to do it that way.  A better way would have been to add a new per-port
> > FD into fd.h, something like this:
> 
> I meant to say, "I really did not want to do that way."
> 
> I can't remember whether I even knew about the possibility of using
> ifinfomsg.ifi_index to bind to a particular interface.  At the time, I
> either overlooked it or I might have rejected this because of lack of
> portability to kernels before v3.7.
> 
> In any case, kernel v3.7 is getting pretty old, and so I think we can:
> 
> 1. remove the rtnl socket from clock.c
> 
> 2. add one rtnl socket per port.
> 
> 3. use ifinfomsg on kernel 3.7+

OK, I think we need a separate patch set to improve this.
> 
> 4. fall back to rtgenmsg on kernel 3.0 - 3.6 (only if needed).

And we do not need to concern about this issue now, right?
> 
>    Q: What is the behavior of pre 3.7 kernels WRT ifinfomsg.ifi_index?
>       If ifi_index is non-zero, does the kernel return EINVAL, or do
>       you get information from all interfaces?
> 
> The only drawback is that, when running on earlier kernels, ptp4l will
> duplicate work, handling all those per-port rtnl messages.  But this
> is acceptable if we can improve the ptp4l code for the future.
> 

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/core/rtnetlink.c?h=v3.0#n1728

Thanks
Hangbin

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Richard C. <ric...@gm...>]

On Mon, Jul 03, 2017 at 12:27:42PM +0800, Hangbin Liu wrote:
> Before v3.7 we do not support create interface with given ifindex with
> RTM_NEWLINK message[1]. Since we only need to get the interface info.
> It should be safe to use ifinfomsg on kernel v3.0

Okay, then we need to take two steps.

1. Convert single rtnl FD into one per-port FD.  Also remove
   index2port hash table from clock.c.

2. Add bonding support.
 
> But with your remind. I found we start to support bond rtnetlink message
> from kernel v3.13. So before v3.13, we could not get slave info and ptp4l
> with bond interface will fail with error not support when check ts info.
> I think this should be OK with document "bond fail over support start with
> kernel v3.13".
> 
> What do you think?

I think is it okay to let bond interfaces fail on start up when
running on older kernels.  We can even call uname(2) to check the
kernel version when bonding is requested in the configuration.

Thanks,
Richard

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Jiri B. <jb...@re...>]

On Mon, 3 Jul 2017 09:12:46 +0200, Richard Cochran wrote:
> We can even call uname(2) to check the kernel version when bonding is
> requested in the configuration.

Please don't do that. Always check for the feature being present, never
for a kernel version. It's common for various groups to backport newer
features to older kernels; kernel version alone does not indicate
anything.

Thanks,

 Jiri

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Richard C. <ric...@gm...>]

On Mon, Jul 03, 2017 at 10:28:39AM +0200, Jiri Benc wrote:
> On Mon, 3 Jul 2017 09:12:46 +0200, Richard Cochran wrote:
> > We can even call uname(2) to check the kernel version when bonding is
> > requested in the configuration.
> 
> Please don't do that. Always check for the feature being present, never
> for a kernel version. It's common for various groups to backport newer
> features to older kernels; kernel version alone does not indicate
> anything.

Ok, then just let ptp4l try to configure the bonding interface, and
when it fails, put a hint about the kernel version into the error
message.

Thanks,
Richard

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Hangbin L. <liu...@gm...>]

On Fri, Jun 30, 2017 at 08:44:36AM +0200, Richard Cochran wrote:
> On Fri, Jun 30, 2017 at 08:18:15AM +0200, Richard Cochran wrote:
> > At that time, I added one extra FD into clock->pollfdp[], and the
> > clock then farms out the information to the ports.  I really did not
> > to do it that way.  A better way would have been to add a new per-port
> > FD into fd.h, something like this:
> 
> I meant to say, "I really did not want to do that way."
> 
> I can't remember whether I even knew about the possibility of using
> ifinfomsg.ifi_index to bind to a particular interface.  At the time, I
> either overlooked it or I might have rejected this because of lack of
> portability to kernels before v3.7.

I guess I know why you didn't use ifinfomsg.ifi_index now. Because you didn't
need it...

After check, I find ifinfomsg.ifi_index only used to get spedific interface's
info, just as we know. But it could not get specific interface's notify.

The reason we could get interface link up/down notify is we registered sa
group with RTMGRP_LINK/RTNLGRP_LINK. So whenever a interface changed, we
will get the notify.

Before this patch set. We don't need the interface info, and only care about
the link status. So you don't need ifinfomsg when call rtnl_link_query().

Now since we need to know the interface's active slave info. I update
rtnl_link_query() to use ifinfomsg to get the interface slave info.

But for link status monitor, ifinfomsg.ifi_index doesn't do any help. We still
need to register with groupp RTNLGRP_LINK and keep receive all interfaces'
information.

> 
> 1. remove the rtnl socket from clock.c
> 
> 2. add one rtnl socket per port.
> 

Back to this question, do you still want to move rtnl socket from clock to
per port?

I think we can filter the call back if index to make sure we get correct
interface, something like

+static void port_link_status(void *ctx, int index, int linkup)
+{
+       struct port *p = ctx;
+
+       if (index != if_nametoindex(p->name) && p->link_status != linkup)
+               return;
+
+       p->link_status = linkup;
+       pr_notice("port %hu: link %s", portnum(p), linkup ? "up" : "down");
+
+       if (linkup)
+               port_dispatch(p, EV_FAULT_CLEARED, 0);
+       else
+               port_dispatch(p, EV_FAULT_DETECTED, 0);
+}

But this should have no relation with the failover patch set.

Thanks
Hangbin

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Richard C. <ric...@gm...>]

On Tue, Jul 04, 2017 at 04:32:49PM +0800, Hangbin Liu wrote:
> But for link status monitor, ifinfomsg.ifi_index doesn't do any help. We still
> need to register with groupp RTNLGRP_LINK and keep receive all interfaces'
> information.

I see.

> Back to this question, do you still want to move rtnl socket from clock to
> per port?

Yes, but only if we can use a single rtnl socket for both messages.

> I think we can filter the call back if index to make sure we get correct
> interface, something like
> 
> +static void port_link_status(void *ctx, int index, int linkup)
> +{
> +       struct port *p = ctx;
> +
> +       if (index != if_nametoindex(p->name) && p->link_status != linkup)
> +               return;
> +
> +       p->link_status = linkup;
> +       pr_notice("port %hu: link %s", portnum(p), linkup ? "up" : "down");
> +
> +       if (linkup)
> +               port_dispatch(p, EV_FAULT_CLEARED, 0);
> +       else
> +               port_dispatch(p, EV_FAULT_DETECTED, 0);
> +}

Sounds reasonable.
 
> But this should have no relation with the failover patch set.

Right, and so I would like to see that change first.

Thanks,
Richard

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Hangbin L. <liu...@gm...>]

On Tue, Jul 04, 2017 at 11:02:26AM +0200, Richard Cochran wrote:
> > Back to this question, do you still want to move rtnl socket from clock to
> > per port?
> 
> Yes, but only if we can use a single rtnl socket for both messages.

Hmm.. If we want to use a single rtnl socket on all ports. we need create it
before port_open() and give the fd to the new port.

On the other hand, if we use the same socket,
1) events & POLLIN , we have a notify message with port 2 iface index.
2) on port 1, rtnl_link_status(rtnl_fd, port_link_status, p). Then all notify
   message received. But after check, the if index is not match
3) on port 2, call rtnl_link_status(), but the message on this socket already
   recieved. Then we will miss the notify.

So I think we need to make each port have their own rtnl socket fd. Then each
fd get their own multicast rtnl message.

What do you think?

Thanks
Hangbin

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Richard C. <ric...@gm...>]

On Tue, Jul 04, 2017 at 06:32:19PM +0800, Hangbin Liu wrote:
> On Tue, Jul 04, 2017 at 11:02:26AM +0200, Richard Cochran wrote:
> > > Back to this question, do you still want to move rtnl socket from clock to
> > > per port?
> > 
> > Yes, but only if we can use a single rtnl socket for both messages.

I meant to say, a single rtnl socket per-port.

> So I think we need to make each port have their own rtnl socket fd. Then each
> fd get their own multicast rtnl message.
> 
> What do you think?

Yes, that is what I meant.  

My question was, can we have both RTNLGRP_LINK and ifinfomsg on the
same per-port socket?  (I assume that works.)

Thanks,
Richard

Re: [Linuxptp-devel] [PATCH 1/8] rtnl: use ifinfomsg instead of rtgenmsg to request link info

[Hangbin L. <liu...@gm...>]

On Tue, Jul 04, 2017 at 01:24:58PM +0200, Richard Cochran wrote:
> On Tue, Jul 04, 2017 at 06:32:19PM +0800, Hangbin Liu wrote:
> > On Tue, Jul 04, 2017 at 11:02:26AM +0200, Richard Cochran wrote:
> > > > Back to this question, do you still want to move rtnl socket from clock to
> > > > per port?
> > > 
> > > Yes, but only if we can use a single rtnl socket for both messages.
> 
> I meant to say, a single rtnl socket per-port.

Cool. Sorry for the misunderstanding.

> 
> > So I think we need to make each port have their own rtnl socket fd. Then each
> > fd get their own multicast rtnl message.
> > 
> > What do you think?
> 
> Yes, that is what I meant.  
> 
> My question was, can we have both RTNLGRP_LINK and ifinfomsg on the
> same per-port socket?  (I assume that works.)

Yes, it works.

Thanks
Hangbin

[Linuxptp-devel] [PATCHv2 0/9] Add linuxptp bond failover support

[Hangbin L. <liu...@gm...>]

This patch set is to add linuxptp bond fail over support.

The main idea is get bond's active slave interface via rtnl socket and store
it in struct interface.

When active interface changed, we update the port phc index and switch to
new phc on clock.

After we get true ts interface, we need to use this interface in
sk_timestamping_init() in transport_open().

Also we need update clock and phc_index in phc2sys.

v2:
1. After the rtnl per port update, now we update ts_iface info in
   port_link_status().
2. Fix port_dispatch event flood when change ts_iface info. This issue
   only happen with bond interface when fail over. Normal ethernet
   interface do not have this problem.

Thanks
Hangbin

---
Testing:
1. Run linuxptp-testsuite and all passed.

2. Run ptp4l on non-bond interface

3. Run ptp4l on bond interface with following modes. the result looks good.
# ./ptp4l -S -2 -i bond0 -m
# ./ptp4l -i bond0 -m

4. Any other test scenario recommend?


Hangbin Liu (9):
  config: add new element ts_iface in struct interface
  port: track interface info in port
  rtnl: update rtgenmsg to ifinfomsg when request link info
  rtnl: add function rtnl_link_info
  clock: use ts interface to get ts info
  clock: check required_modes before use new ts info
  port.c: fix port_dispatch event flood when change ts_iface info
  transport: pass struct interface to transport_open
  phc2sys: update clock clkid and phc_index if device changed

 clock.c             |  52 +++++++++++++-------
 clock.h             |   7 +++
 config.c            |   1 -
 config.h            |   1 +
 phc2sys.c           |  50 ++++++++++++++++++--
 pmc_common.c        |   5 +-
 port.c              |  64 +++++++++++++++++++------
 raw.c               |   5 +-
 rtnl.c              | 134 +++++++++++++++++++++++++++++++++++++++++++++++++---
 rtnl.h              |  17 +++++--
 transport.c         |   4 +-
 transport.h         |   3 +-
 transport_private.h |   4 +-
 udp.c               |   7 +--
 udp6.c              |   7 +--
 uds.c               |   3 +-
 16 files changed, 302 insertions(+), 62 deletions(-)

-- 
2.5.5

[Linuxptp-devel] [PATCHv2 1/9] config: add new element ts_iface in struct interface

[Hangbin L. <liu...@gm...>]

Add new element ts_iface in struct interface to track real ts interface.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 config.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.h b/config.h
index 1cc7051..f0ae3fe 100644
--- a/config.h
+++ b/config.h
@@ -36,6 +36,7 @@
 struct interface {
 	STAILQ_ENTRY(interface) list;
 	char name[MAX_IFNAME_SIZE + 1];
+	char ts_iface[MAX_IFNAME_SIZE + 1];
 	struct sk_ts_info ts_info;
 };
 
-- 
2.5.5

Re: [Linuxptp-devel] [PATCHv2 1/9] config: add new element ts_iface in struct interface

[Richard C. <ric...@gm...>]

On Sat, Jul 15, 2017 at 09:33:03PM +0800, Hangbin Liu wrote:
> Add new element ts_iface in struct interface to track real ts interface.
> 
> Signed-off-by: Hangbin Liu <liu...@gm...>
> ---
>  config.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/config.h b/config.h
> index 1cc7051..f0ae3fe 100644
> --- a/config.h
> +++ b/config.h
> @@ -36,6 +36,7 @@
>  struct interface {
>  	STAILQ_ENTRY(interface) list;
>  	char name[MAX_IFNAME_SIZE + 1];
> +	char ts_iface[MAX_IFNAME_SIZE + 1];

This is poorly named.  We use the identifier 'iface' for pointers to
variables of type 'struct interface'.

Maybe use "ts_label" in order to avoid both the words "iface" and "name".

>  	struct sk_ts_info ts_info;
>  };
>  
> -- 
> 2.5.5

Thanks,
Richard