linuxptp-devel

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

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

On Sat, Aug 05, 2017 at 10:13:07AM +0200, Richard Cochran wrote:
> >  enum fsm_event port_event(struct port *p, int fd_index)
> > @@ -2301,7 +2313,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
> >  	case FD_RTNL:
> >  		pr_debug("port %hu: received link status notification", portnum(p));
> >  		rtnl_link_status(fd, port_link_status, p);
> > -		return port_link_status_get(p) ? EV_FAULT_CLEARED : EV_FAULT_DETECTED;
> > +		return EV_NONE;
> 
> Maybe we can let rtnl_link_status() return the EV_ value from
> port_link_status(), in order to keep a functional pattern and avoid
> the hidden port_dispatch().

for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
        if (nh->nlmsg_type == RTM_NEWLINK) {

                cb(ctx, index, info->ifi_flags & IFF_RUNNING ? 1 : 0, device);
        }
}

One rtnl msg may have multi messages with nh->nlmsg_type == RTM_NEWLINK, which
will call back port_link_status() multi times. So we could not let
rtnl_link_status() return EV_ value.

Thanks
Hangbin

Re: [Linuxptp-devel] [PATCHv2 8/9] transport: pass struct interface to transport_open

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

On Sat, Jul 15, 2017 at 09:33:10PM +0800, Hangbin Liu wrote:
> Pass struct interface so we can use ts_iface in HW filter.

So the problem is that the raw layer should use

	name = iface->ts_iface

but the others need

	name = iface->name

Let's avoid touching so many files by solving this differently...

> -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)
>  {
	const char *name;

	switch (t->type) {
	case TRANS_IEEE_802_3:
		name = iface->ts_iface;
		break;
	default:
		name = iface->name;
		break;
	}

> 	return t->open(t, name, fda, tt);
>  }

Re: [Linuxptp-devel] [PATCHv2 4/9] rtnl: add function rtnl_link_info

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

On Sat, Jul 15, 2017 at 09:33:06PM +0800, Hangbin Liu wrote:
> @@ -92,6 +151,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;

The meaning of 'ctx' changes according to the value of 'cb'.  This
overloading of the function arguments is gross.

Please find another way to structure this.  You only have two call
paths for rtnl_link_status:

1. port_event() -> rtnl_link_status()
2. clock_create() -> rtnl_link_info() -> rtnl_link_status()

Thanks,
Richard

Re: [Linuxptp-devel] [PATCHv2 4/9] rtnl: add function rtnl_link_info

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

On Sat, Aug 05, 2017 at 10:31:48AM +0200, Richard Cochran wrote:
> On Sat, Jul 15, 2017 at 09:33:06PM +0800, Hangbin Liu wrote:
> > @@ -92,6 +151,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;
> 
> The meaning of 'ctx' changes according to the value of 'cb'.  This
> overloading of the function arguments is gross.

Yes, this is kind of ugly.

> Please find another way to structure this.  You only have two call
> paths for rtnl_link_status:
> 
> 1. port_event() -> rtnl_link_status()

The goal of this call path is to call port_link_status() whenever link status
changed. And the call path should like

1. port_event() -> rtnl_link_status() -> port_link_status()

> 2. clock_create() -> rtnl_link_info() -> rtnl_link_status()

This one only want to get the bond slave info when add iface at the begining.

Ironically, they share the most code...


I tried to add two wrapper function like

int rtnl_link_info(int fd, char *index, char *up, char *device)
{
	/* set index, link up status and device here */
}

void rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
{
	rtnl_link_info(fd, &index, &up, device);
	cb(ctx, index, up, device);
}

static int rtnl_get_interface_slave(int fd, char *device)
{
	return rtnl_link_info(fd, &index, &up, device);
}

But this is incorrect becuase we need call port_link_status() every time
nh->nlmsg_type == RTM_NEWLINK.

for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
	if (nh->nlmsg_type == RTM_NEWLINK) {
		cb(ctx, index, info->ifi_flags & IFF_RUNNING ? 1 : 0, device);
	}
}


So now I only have one workaround: add one more parameter...

int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx)

Then if we want to get device name via rtnl_link_info(), we could call like
rtnl_link_status(fd, iface->ts_label, NULL, NULL);

If we want to callback functions, we call it like
rtnl_link_status(fd, NULL, port_link_status, p);


What do you think? any other good ideas?

Thanks
Hangbin

Re: [Linuxptp-devel] [PATCHv2 8/9] transport: pass struct interface to transport_open

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

On Sat, Aug 05, 2017 at 10:21:41AM +0200, Richard Cochran wrote:
> On Sat, Jul 15, 2017 at 09:33:10PM +0800, Hangbin Liu wrote:
> > Pass struct interface so we can use ts_iface in HW filter.
> 
> So the problem is that the raw layer should use
> 
> 	name = iface->ts_iface
> 
> but the others need
> 
> 	name = iface->name
> 
> Let's avoid touching so many files by solving this differently...
> 
> > -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)
> >  {
> 	const char *name;
> 
> 	switch (t->type) {
> 	case TRANS_IEEE_802_3:
> 		name = iface->ts_iface;
> 		break;
> 	default:
> 		name = iface->name;
> 		break;
> 	}
> 
> > 	return t->open(t, name, fda, tt);

We need ts_iface in sk_timestamping_init. e.g.

--- 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;

So I passed struct interface to t->open().

Thanks
Hangbin

Re: [Linuxptp-devel] [PATCHv2 8/9] transport: pass struct interface to transport_open

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

On Wed, Aug 09, 2017 at 11:45:30AM +0800, Hangbin Liu wrote:
> We need ts_iface in sk_timestamping_init. e.g.

Ok, I overlooked that.  I take my comment back.

Thanks,
Richard

Re: [Linuxptp-devel] [PATCHv2 4/9] rtnl: add function rtnl_link_info

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

On Tue, Aug 08, 2017 at 06:16:37PM +0800, Hangbin Liu wrote:
> The goal of this call path is to call port_link_status() whenever link status
> changed. And the call path should like
> 
> 1. port_event() -> rtnl_link_status() -> port_link_status()
> 
> > 2. clock_create() -> rtnl_link_info() -> rtnl_link_status()
> 
> This one only want to get the bond slave info when add iface at the begining.

Right.

> What do you think? any other good ideas?

Yes, I have an idea.  Pass an interface index (instead of a string) to
port_link_status(), and then you can remove the ugly code.

	if (cb)
		device = calloc(1, sizeof(MAX_IFNAME_SIZE + 1));
	else
		device = (char *)ctx;

Then you can also simplify this block

			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;
				}
			}

by removing the if_indextoname() call.

Thanks,
Richard

Re: [Linuxptp-devel] [PATCHv2 4/9] rtnl: add function rtnl_link_info

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

On Wed, Aug 09, 2017 at 08:49:34AM +0200, Richard Cochran wrote:
> On Tue, Aug 08, 2017 at 06:16:37PM +0800, Hangbin Liu wrote:
> > The goal of this call path is to call port_link_status() whenever link status
> > changed. And the call path should like
> > 
> > 1. port_event() -> rtnl_link_status() -> port_link_status()
> > 
> > > 2. clock_create() -> rtnl_link_info() -> rtnl_link_status()
> > 
> > This one only want to get the bond slave info when add iface at the begining.
> 
> Right.
> 
> > What do you think? any other good ideas?
> 
> Yes, I have an idea.  Pass an interface index (instead of a string) to
> port_link_status(), and then you can remove the ugly code.
> 
> 	if (cb)
> 		device = calloc(1, sizeof(MAX_IFNAME_SIZE + 1));
> 	else
> 		device = (char *)ctx;

Yes, that would be more clear, especially the call path 1. Then how about

2. clock_create() -> rtnl_link_info() -> rtnl_link_status()


Should we get the ts_iface inside rtnl_link_status() like:

err = rtnl_link_status(fd, iface->ts_label, NULL, NULL);

int rtnl_link_status(int fd, char *slave, rtnl_callback cb, void *ctx)
{
	...
	if (tb[IFLA_LINKINFO])
		slave_index = rtnl_linkinfo_parse(tb[IFLA_LINKINFO]);

	if (cb)
		cb(ctx, index, link_up, slave_index);

	if (slave)
		if_indextoname(slave_index, slave);
	...
}


Or just let rtnl_link_status() return ts_iface index directly, like:

ts_iface_index = rtnl_link_status(fd, NULL, NULL);

int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
{
	...
	if (tb[IFLA_LINKINFO])
		slave_index = rtnl_linkinfo_parse(tb[IFLA_LINKINFO]);

	if (cb)
		cb(ctx, index, link_up, slave_index);
	...

	return slave_index;
}

Thanks
Hangbin

Re: [Linuxptp-devel] [PATCHv2 4/9] rtnl: add function rtnl_link_info

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

On Thu, Aug 10, 2017 at 04:10:49PM +0800, Hangbin Liu wrote:
> Yes, that would be more clear, especially the call path 1. Then how about
> 
> 2. clock_create() -> rtnl_link_info() -> rtnl_link_status()
> 
> 
> Should we get the ts_iface inside rtnl_link_status() like:

No.

> Or just let rtnl_link_status() return ts_iface index directly, like:
> 
> ts_iface_index = rtnl_link_status(fd, NULL, NULL);

Well, we need the return value to indicate errors.

So for clock_create(), rtnl_link_info() can provide a special
callback.

First, pass in the desired index into rtnl_link_status() and
initialize the slave index:

int rtnl_link_status(int fd, int index, rtnl_callback cb, void *ctx)
{
	int slave_index = -1;	/* Don't forget this! */
	...
	for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
		if (nh->nlmsg_type != RTM_NEWLINK) {
			continue;
		}
		info = NLMSG_DATA(nh);
		if (index != info->ifi_index) {
			continue;
		}
		...
		if (tb[IFLA_LINKINFO])
			slave_index = rtnl_linkinfo_parse(tb[IFLA_LINKINFO]);
		if (cb)
			cb(ctx, link_up, slave_index);
		/*
		 * cb doesn't need index!
		 */
		...
	}
}

Then define a customized callback:

static void rtnl_link_info_callback(void *ctx, int index, int lnk, int ts_index)
{
	int *dst = ctx;
	*dst = ts_index;
}

int rtnl_link_info(struct interface *iface)
{
	int fd, index, ts_index = -1;
	...

	if (rtnl_link_status(fd, rtnl_link_info_callback, &ts_index))
		goto no_info;
	/*
	 * If we do not have a slave, then use our own interface name
	 * as ts_iface
	 */
	if (ts_index < 0) {
		...
	} else {
		...
	}
	...
}

Thanks,
Richard

Re: [Linuxptp-devel] [PATCHv2 4/9] rtnl: add function rtnl_link_info

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

On Thu, Aug 10, 2017 at 11:22:56AM +0200, Richard Cochran wrote:
> > Or just let rtnl_link_status() return ts_iface index directly, like:
> > 
> > ts_iface_index = rtnl_link_status(fd, NULL, NULL);
> 
> Well, we need the return value to indicate errors.
> 
> So for clock_create(), rtnl_link_info() can provide a special
> callback.

Ah, yes, this would make the code more clear. Thanks a lot for the help.
> 
> First, pass in the desired index into rtnl_link_status() and
> initialize the slave index:

Would it be better to pass a device name? If we pass the index number. Then we
need to convert both iface->name and port->name to index in rtnl_link_info()
and port_event().

And if pass a device name. We can convert the name to index in
rtnl_link_status() unified.
> 
> int rtnl_link_status(int fd, int index, rtnl_callback cb, void *ctx)
> {
> 	int slave_index = -1;	/* Don't forget this! */
> 	...
> 	for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
> 		if (nh->nlmsg_type != RTM_NEWLINK) {
> 			continue;
> 		}
> 		info = NLMSG_DATA(nh);
> 		if (index != info->ifi_index) {
> 			continue;
> 		}
> 		...
> 		if (tb[IFLA_LINKINFO])
> 			slave_index = rtnl_linkinfo_parse(tb[IFLA_LINKINFO]);
> 		if (cb)
> 			cb(ctx, link_up, slave_index);

OK, check the index in rtnl_link_status(), not in port_link_status().
No problem.
> 		/*
> 		 * cb doesn't need index!
> 		 */
> 		...
> 	}
> }
> 
> Then define a customized callback:
> 
> static void rtnl_link_info_callback(void *ctx, int index, int lnk, int ts_index)

I guess we do not need this 'int index' now. as above.
> {
> 	int *dst = ctx;
> 	*dst = ts_index;
> }
> 
> int rtnl_link_info(struct interface *iface)
> {
> 	int fd, index, ts_index = -1;
> 	...
> 
> 	if (rtnl_link_status(fd, rtnl_link_info_callback, &ts_index))

iface_index = if_nametoindex(iface->name);
if (rtnl_link_status(fd, iface_index, rtnl_link_info_callback, &ts_index))

or pass dev name to rtnl_link_status()

if (rtnl_link_status(fd, iface->name, rtnl_link_info_callback, &ts_index))

> 		goto no_info;
> 	/*
> 	 * If we do not have a slave, then use our own interface name
> 	 * as ts_iface
> 	 */

we have moved this check to clock.c. So no need to ensure it here, right?

Just

	if (ts_index > 0 && if_indextoname(ts_index, iface->ts_label))
		err = 0;

should be OK.

Thanks
Hangbin
> 	if (ts_index < 0) {
> 		...
> 	} else {
> 		...
> 	}
> 	...
> }
> 
> Thanks,
> Richard
> 

Re: [Linuxptp-devel] [PATCHv2 4/9] rtnl: add function rtnl_link_info

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

On Thu, Aug 10, 2017 at 06:31:56PM +0800, Hangbin Liu wrote:
> Would it be better to pass a device name? If we pass the index number. Then we
> need to convert both iface->name and port->name to index in rtnl_link_info()
> and port_event().
> 
> And if pass a device name. We can convert the name to index in
> rtnl_link_status() unified.

Yes, use the name string if that simplifies the code.

Thanks,
Richard

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

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

Hi Richard,
On Sat, Aug 05, 2017 at 10:13:07AM +0200, Richard Cochran wrote:
> 
> This testing of two Booleans (status_changed, link_status) smells bad.
> Please refactor this into something that makes sense.

At present, the link could be up or down. On an other hand, when the link stay
up or down, the ts_iface could be changed. So the states could be

LINK_UP -> LINK_DOWN(LINK_STATE_CHANGED):	EV_FAULT_DETECTED
LINK_DOWN -> LINK_UP(LINK_STATE_CHANGED):	EV_FAULT_CLEARED
LINK_UP -> LINK_UP, TS_IFACE_CHANGED:		EV_FAULT_DETECTED
LINK_DOWN -> LINK_DOWN, TS_IFACE_CHANGED:	EV_FAULT_DETECTED

For example, when the first time we receive a linkup + ts_iface change
message, we need to return EV_FAULT_DETECTED. But as we know, we may
recive multi linkup message. For these kind of message, we need to return
EV_NONE.

So I want to make the port link_status to enum like
enum link_state {
        LINK_DOWN  = (1<<0),
        LINK_UP  = (1<<1),
        LINK_STATE_CHANGED = (1<<3),
        TS_LABEL_CHANGED  = (1<<4),
};

Then after rtnl_link_status(fd, p->name, port_link_status, p);

if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
        return EV_FAULT_CLEARED;
else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
         (p->link_status & TS_LABEL_CHANGED))
        return EV_FAULT_DETECTED;
else
        return EV_NONE;


What do you think?

Thanks
Hangbin


> 
> > +			port_dispatch(p, EV_FAULT_CLEARED, 0);
> > +		} else {
> > +			port_dispatch(p, EV_FAULT_DETECTED, 0);
> > +			/*
> > +			 * A port going down can affect the BMCA result.
> > +			 * Force a state decision event.
> > +			 */
> > +			clock_set_sde(p->clock, 1);
> > +		}
> > +	}
> >  }
> >  
> >  enum fsm_event port_event(struct port *p, int fd_index)
> > @@ -2301,7 +2313,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
> >  	case FD_RTNL:
> >  		pr_debug("port %hu: received link status notification", portnum(p));
> >  		rtnl_link_status(fd, port_link_status, p);
> > -		return port_link_status_get(p) ? EV_FAULT_CLEARED : EV_FAULT_DETECTED;
> > +		return EV_NONE;
> 
> Maybe we can let rtnl_link_status() return the EV_ value from
> port_link_status(), in order to keep a functional pattern and avoid
> the hidden port_dispatch().
> 
> >  	}
> >  
> >  	msg = msg_allocate();
> > -- 
> > 2.5.5
> 
> Thanks,
> Richard

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

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

On Sat, Aug 12, 2017 at 10:16:20PM +0800, Hangbin Liu wrote:
> For example, when the first time we receive a linkup + ts_iface change
> message, we need to return EV_FAULT_DETECTED. But as we know, we may
> recive multi linkup message. For these kind of message, we need to return
> EV_NONE.

Right...
 
> Then after rtnl_link_status(fd, p->name, port_link_status, p);
> 
> if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
>         return EV_FAULT_CLEARED;
> else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
>          (p->link_status & TS_LABEL_CHANGED))
>         return EV_FAULT_DETECTED;
> else
>         return EV_NONE;
> 
> 
> What do you think?

Looks better to me, but why not place this code at the end of the
port_link_status() function?

Thanks,
Richard

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

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

On Sat, Aug 12, 2017 at 04:41:23PM +0200, Richard Cochran wrote:
> > Then after rtnl_link_status(fd, p->name, port_link_status, p);
> > 
> > if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
> >         return EV_FAULT_CLEARED;
> > else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
> >          (p->link_status & TS_LABEL_CHANGED))
> >         return EV_FAULT_DETECTED;
> > else
> >         return EV_NONE;
> > 
> > 
> > What do you think?
> 
> Looks better to me, but why not place this code at the end of the
> port_link_status() function?

Hmm... I saw your comments about let rtnl_link_status() return the EV_ value
from port_link_status().

This seems no easy to handle.
1. We need to return both error code and enum fsm_event in rtnl_link_status().
2. If We receive one rtnl message with multi RTM_NEWLINK info, the cb will be
   called more than once, then the return value will be replaced by new ones.

	for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
		if (cb)
			err = cb(ctx, link_up, slave_index);
	}

	return err;

Thanks
Hangbin

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

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

On Sun, Aug 13, 2017 at 11:21:21PM +0800, Hangbin Liu wrote:
> On Sat, Aug 12, 2017 at 04:41:23PM +0200, Richard Cochran wrote:
> > Looks better to me, but why not place this code at the end of the
> > port_link_status() function?

...

> 2. If We receive one rtnl message with multi RTM_NEWLINK info, the cb will be
>    called more than once, then the return value will be replaced by new ones.

Ok, that is clear.  Place the code after rtnl_link_status() like you said.

Thanks,
Richard

[Linuxptp-devel] [PATCHv3 00/10] 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 -> v3:
1. Change the ts_iface to ts_label
2. Separate the original fourth patch "rtnl: add function rtnl_link_info"
   into two parts. The first part is update function rtnl_link_status to
   get bond slave info. And the second part add the new function
   rtnl_get_ts_label() to get interface ts_label info.
3. update port link_status to enum
4. Some small fixes.

v1 -> 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 bond interface and trigger the failover, the result looks good.

Hangbin Liu (10):
  config: add new element ts_label in struct interface
  port: track interface info in port
  rtnl: update rtgenmsg to ifinfomsg when request link info
  rtnl: update function rtnl_link_status to get bond slave info
  rtnl: add function rtnl_get_ts_label to get interface ts_label info
  port: update port link_status to enum
  clock: add clock_required_modes to obtain the required time stamping mode
  ptp4l: use ts label to get ts info
  transport: pass struct interface to transport_open
  phc2sys: update clock clkid and phc_index if device changed

 clock.c             |  63 +++++++++++++++++--------
 clock.h             |   8 ++++
 config.c            |   1 -
 config.h            |   1 +
 phc2sys.c           |  50 ++++++++++++++++++--
 pmc_common.c        |   5 +-
 port.c              |  79 +++++++++++++++++++++++++------
 raw.c               |   5 +-
 rtnl.c              | 131 ++++++++++++++++++++++++++++++++++++++++++++++------
 rtnl.h              |  28 +++++++----
 transport.c         |   4 +-
 transport.h         |   3 +-
 transport_private.h |   4 +-
 udp.c               |   7 +--
 udp6.c              |   7 +--
 uds.c               |   3 +-
 16 files changed, 324 insertions(+), 75 deletions(-)

-- 
2.5.5

[Linuxptp-devel] [PATCHv3 01/10] config: add new element ts_label in struct interface

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

Add new element ts_label 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..c79855e 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_label[MAX_IFNAME_SIZE + 1];
 	struct sk_ts_info ts_info;
 };
 
-- 
2.5.5

[Linuxptp-devel] [PATCHv3 02/10] port: track interface info in port

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

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 port.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/port.c b/port.c
index 34837cc..849a7c1 100644
--- a/port.c
+++ b/port.c
@@ -68,6 +68,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;
@@ -2619,6 +2620,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;
-- 
2.5.5

[Linuxptp-devel] [PATCHv3 03/10] rtnl: update rtgenmsg to ifinfomsg when request link info

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

The previous function use general message and will dump all interfaces'
information. Now update with ifinfomsg so we could get specific interface's
information.

We still could get all interfaces' info if set device to NULL.

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

diff --git a/port.c b/port.c
index 849a7c1..5b85d87 100644
--- a/port.c
+++ b/port.c
@@ -1512,7 +1512,7 @@ static int port_initialize(struct port *p)
 		if (p->fda.fd[FD_RTNL] == -1)
 			p->fda.fd[FD_RTNL] = rtnl_open();
 		if (p->fda.fd[FD_RTNL] >= 0)
-			rtnl_link_query(p->fda.fd[FD_RTNL]);
+			rtnl_link_query(p->fda.fd[FD_RTNL], p->iface->name);
 	}
 
 	port_nrate_initialize(p);
diff --git a/rtnl.c b/rtnl.c
index d7a430d..8ecf6fe 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, char *device)
 {
 	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_nametoindex(device ? device : "");
+	request.ifm.ifi_change = 0xffffffff;
 
 	iov.iov_base = &request;
 	iov.iov_len = sizeof(request);
diff --git a/rtnl.h b/rtnl.h
index f1871f2..5c93eec 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 device Interface name. Request all iface's status if set NULL.
+ * @return       Zero on success, non-zero otherwise.
  */
-int rtnl_link_query(int fd);
+int rtnl_link_query(int fd, char *device);
 
 /**
  * Read kernel messages looking for a link up/down events.
-- 
2.5.5

[Linuxptp-devel] [PATCHv3 04/10] rtnl: update function rtnl_link_status to get bond slave info

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

Update function rtnl_link_status to get bond slave info. Pass the slave index
to call back functions. i.e. port_link_status.

Also check the interface index of rtnl message in function rtnl_link_status.
Then we don't need to check it in port_link_status.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 port.c |  6 ++---
 rtnl.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 rtnl.h | 13 +++++-----
 3 files changed, 89 insertions(+), 18 deletions(-)

diff --git a/port.c b/port.c
index 5b85d87..05fc321 100644
--- a/port.c
+++ b/port.c
@@ -2221,11 +2221,11 @@ void port_dispatch(struct port *p, enum fsm_event event, int mdiff)
 	}
 }
 
-static void port_link_status(void *ctx, int index, int linkup)
+static void port_link_status(void *ctx, int linkup, int ts_index)
 {
 	struct port *p = ctx;
 
-	if (index != if_nametoindex(p->name) || p->link_status == linkup)
+	if (p->link_status == linkup)
 		return;
 
 	p->link_status = linkup;
@@ -2280,7 +2280,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
 
 	case FD_RTNL:
 		pr_debug("port %hu: received link status notification", portnum(p));
-		rtnl_link_status(fd, port_link_status, p);
+		rtnl_link_status(fd, p->name, port_link_status, p);
 		return port_link_status_get(p) ? EV_FAULT_CLEARED : EV_FAULT_DETECTED;
 	}
 
diff --git a/rtnl.c b/rtnl.c
index 8ecf6fe..b695edd 100644
--- a/rtnl.c
+++ b/rtnl.c
@@ -31,6 +31,9 @@
 
 static int rtnl_len;
 static char *rtnl_buf;
+static 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)))
 
 int rtnl_close(int fd)
 {
@@ -84,15 +87,69 @@ int rtnl_link_query(int fd, char *device)
 	return 0;
 }
 
-int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
+static inline __u32 rta_getattr_u32(const struct rtattr *rta)
 {
-	int index, len;
+	return *(__u32 *)RTA_DATA(rta);
+}
+
+static inline const char *rta_getattr_str(const struct rtattr *rta)
+{
+	return (const char *)RTA_DATA(rta);
+}
+
+static int rtnl_rtattr_parse(struct rtattr *tb[], int max, struct rtattr *rta, int len)
+{
+	unsigned short type;
+
+	memset(tb, 0, sizeof(struct rtattr *) * max);
+	while (RTA_OK(rta, len)) {
+		type = rta->rta_type;
+		if ((type <= max) && (!tb[type]))
+			tb[type] = rta;
+		rta = RTA_NEXT(rta, len);
+	}
+	if (len)
+		pr_err("Length mismatch: len %d, rta_len=%d\n", len, rta->rta_len);
+	return 0;
+}
+
+static int rtnl_linkinfo_parse(struct rtattr *rta)
+{
+	int index = -1;
+	const char *kind;
+	struct rtattr *linkinfo[IFLA_INFO_MAX];
+	struct rtattr *bond[IFLA_BOND_MAX];
+
+	rtnl_nested_rtattr_parse(linkinfo, IFLA_INFO_MAX, rta);
+
+	if (linkinfo[IFLA_INFO_KIND]) {
+		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]);
+			}
+		}
+	}
+	return index;
+}
+
+int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx)
+{
+	int index, len, link_up;
+	int slave_index = -1;
 	struct iovec iov;
 	struct sockaddr_nl sa;
 	struct msghdr msg;
 	struct nlmsghdr *nh;
 	struct ifinfomsg *info = NULL;
+	struct rtattr *tb[IFLA_MAX+1];
 
+	index = if_nametoindex(device);
 	if (!rtnl_buf) {
 		rtnl_len = 4096;
 		rtnl_buf = malloc(rtnl_len);
@@ -135,14 +192,27 @@ int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
 	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);
-		}
+		if (nh->nlmsg_type != RTM_NEWLINK)
+			continue;
+
+		info = NLMSG_DATA(nh);
+		if (index != info->ifi_index)
+			continue;
+
+		link_up = info->ifi_flags & IFF_RUNNING ? 1 : 0;
+		pr_debug("interface index %d is %s", index,
+			 link_up ? "up" : "down");
+
+		rtnl_rtattr_parse(tb, IFLA_MAX, IFLA_RTA(info),
+				  IFLA_PAYLOAD(nh));
+
+		if (tb[IFLA_LINKINFO])
+			slave_index = rtnl_linkinfo_parse(tb[IFLA_LINKINFO]);
+
+		if (cb)
+			cb(ctx, link_up, slave_index);
 	}
+
 	return 0;
 }
 
diff --git a/rtnl.h b/rtnl.h
index 5c93eec..20f1491 100644
--- a/rtnl.h
+++ b/rtnl.h
@@ -20,7 +20,7 @@
 #ifndef HAVE_RTNL_H
 #define HAVE_RTNL_H
 
-typedef void (*rtnl_callback)(void *ctx, int index, int linkup);
+typedef void (*rtnl_callback)(void *ctx, int linkup, int ts_index);
 
 /**
  * Close a RT netlink socket.
@@ -39,12 +39,13 @@ int rtnl_link_query(int fd, 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.
- * @param ctx  Private context passed to the callback.
- * @return     Zero on success, non-zero otherwise.
+ * @param fd     Readable socket obtained via rtnl_open().
+ * @param device The device which we need to get link info.
+ * @param cb     Callback function to be invoked on each event.
+ * @param ctx    Private context passed to the callback.
+ * @return       A slave index, or -1 on error.
  */
-int rtnl_link_status(int fd, rtnl_callback cb, void *ctx);
+int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx);
 
 /**
  * Open a RT netlink socket for monitoring link state.
-- 
2.5.5

[Linuxptp-devel] [PATCHv3 05/10] rtnl: add function rtnl_get_ts_label to get interface ts_label info

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

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 rtnl.c | 31 +++++++++++++++++++++++++++++++
 rtnl.h |  8 ++++++++
 2 files changed, 39 insertions(+)

diff --git a/rtnl.c b/rtnl.c
index b695edd..c292a77 100644
--- a/rtnl.c
+++ b/rtnl.c
@@ -237,3 +237,34 @@ int rtnl_open(void)
 	}
 	return fd;
 }
+
+static void rtnl_get_ts_label_callback(void *ctx, int linkup, int ts_index)
+{
+	int *dst = ctx;
+	*dst = ts_index;
+}
+
+int rtnl_get_ts_label(struct interface *iface)
+{
+	int err, fd;
+	int ts_index = -1;
+
+	fd = rtnl_open();
+	if (fd < 0)
+		return fd;
+
+	err = rtnl_link_query(fd, iface->name);
+	if (err) {
+		goto no_info;
+	}
+
+	rtnl_link_status(fd, iface->name, rtnl_get_ts_label_callback, &ts_index);
+	if (ts_index > 0 && if_indextoname(ts_index, iface->ts_label))
+		err = 0;
+	else
+		err = -1;
+
+no_info:
+	rtnl_close(fd);
+	return err;
+}
diff --git a/rtnl.h b/rtnl.h
index 20f1491..d335c40 100644
--- a/rtnl.h
+++ b/rtnl.h
@@ -20,6 +20,8 @@
 #ifndef HAVE_RTNL_H
 #define HAVE_RTNL_H
 
+#include "config.h"
+
 typedef void (*rtnl_callback)(void *ctx, int linkup, int ts_index);
 
 /**
@@ -53,4 +55,10 @@ int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx);
  */
 int rtnl_open(void);
 
+/**
+ * Get interface ts_label information
+ * @param iface  struct interface.
+ * @return       Zero on success, or -1 on error.
+ */
+int rtnl_get_ts_label(struct interface *iface);
 #endif
-- 
2.5.5

[Linuxptp-devel] [PATCHv3 06/10] port: update port link_status to enum

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

Besides link up and down, we may also receive other rtnl message. For example
the bond slave changed info, which the link state keep the same.

So we should return EV_FAULT_CLEARED only when both LINK_UP and
LINK_STATE_CHANGED.

When the link state keep the same, we should return EV_NONE.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 port.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/port.c b/port.c
index 05fc321..81d52ff 100644
--- a/port.c
+++ b/port.c
@@ -56,6 +56,12 @@ enum syfu_event {
 	FUP_MATCH,
 };
 
+enum link_state {
+	LINK_DOWN  = (1<<0),
+	LINK_UP  = (1<<1),
+	LINK_STATE_CHANGED = (1<<3),
+};
+
 struct nrate_estimator {
 	double ratio;
 	tmv_t origin1;
@@ -122,7 +128,7 @@ struct port {
 	int                 path_trace_enabled;
 	int                 rx_timestamp_offset;
 	int                 tx_timestamp_offset;
-	int                 link_status;
+	enum link_state     link_status;
 	struct fault_interval flt_interval_pertype[FT_CNT];
 	enum fault_type     last_fault_type;
 	unsigned int        versionNumber; /*UInteger4*/
@@ -2224,18 +2230,21 @@ void port_dispatch(struct port *p, enum fsm_event event, int mdiff)
 static void port_link_status(void *ctx, int linkup, int ts_index)
 {
 	struct port *p = ctx;
+	int link_state;
 
-	if (p->link_status == linkup)
-		return;
-
-	p->link_status = linkup;
-	pr_notice("port %hu: link %s", portnum(p), linkup ? "up" : "down");
+	link_state = linkup ? LINK_UP : LINK_DOWN;
+	if (p->link_status & link_state) {
+		p->link_status = link_state;
+	} else {
+		p->link_status = link_state | LINK_STATE_CHANGED;
+		pr_notice("port %hu: link %s", portnum(p), linkup ? "up" : "down");
+	}
 
 	/*
 	 * A port going down can affect the BMCA result.
 	 * Force a state decision event.
 	 */
-	if (!p->link_status)
+	if (p->link_status & LINK_DOWN)
 		clock_set_sde(p->clock, 1);
 }
 
@@ -2281,7 +2290,12 @@ enum fsm_event port_event(struct port *p, int fd_index)
 	case FD_RTNL:
 		pr_debug("port %hu: received link status notification", portnum(p));
 		rtnl_link_status(fd, p->name, port_link_status, p);
-		return port_link_status_get(p) ? EV_FAULT_CLEARED : EV_FAULT_DETECTED;
+		if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
+			return EV_FAULT_CLEARED;
+		else if (p->link_status == (LINK_DOWN | LINK_STATE_CHANGED))
+			return EV_FAULT_DETECTED;
+		else
+			return EV_NONE;
 	}
 
 	msg = msg_allocate();
@@ -2409,7 +2423,7 @@ int port_number(struct port *p)
 
 int port_link_status_get(struct port *p)
 {
-	return p->link_status;
+	return !!(p->link_status & LINK_UP);
 }
 
 int port_manage(struct port *p, struct port *ingress, struct ptp_message *msg)
@@ -2630,7 +2644,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->link_status = LINK_UP;
 	p->clock = clock;
 	p->trp = transport_create(cfg, transport);
 	if (!p->trp)
-- 
2.5.5

[Linuxptp-devel] [PATCHv3 07/10] clock: add clock_required_modes to obtain the required time stamping mode

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

Separate required_modes setting from clock_create so we can obtain the
required time stamping flags from other place.

Add enum timestamping in struct clock to store the time stamping mode.

Signed-off-by: Hangbin Liu <liu...@gm...>
---
 clock.c | 49 +++++++++++++++++++++++++++++++------------------
 clock.h |  8 ++++++++
 2 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/clock.c b/clock.c
index da15882..bd2b91b 100644
--- a/clock.c
+++ b/clock.c
@@ -106,6 +106,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;
@@ -805,6 +806,34 @@ static void clock_remove_port(struct clock *c, struct port *p)
 	port_close(p);
 }
 
+int clock_required_modes(struct clock *c)
+{
+	int required_modes = 0;
+
+	switch (c->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;
+}
+
 struct clock *clock_create(enum clock_type type, struct config *config,
 			   const char *phc_device)
 {
@@ -913,24 +942,8 @@ 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;
-	}
+	c->timestamping = timestamping;
+	required_modes = clock_required_modes(c);
 	STAILQ_FOREACH(iface, &config->interfaces, list) {
 		if (iface->ts_info.valid &&
 		    ((iface->ts_info.so_timestamping & required_modes) != required_modes)) {
diff --git a/clock.h b/clock.h
index 49ecb76..986d363 100644
--- a/clock.h
+++ b/clock.h
@@ -73,6 +73,14 @@ UInteger8 clock_class(struct clock *c);
 struct config *clock_config(struct clock *c);
 
 /**
+ * Obtains the required time stamping mode.
+ * @param c  The clock instance.
+ * @return   The value of required time stamping mode, which is a bit mask
+ *           of SOF_TIMESTAMPING_ flags.
+ */
+int clock_required_modes(struct clock *c);
+
+/**
  * Create a clock instance. There can only be one clock in any system,
  * so subsequent calls will destroy the previous clock instance.
  *
-- 
2.5.5

[Linuxptp-devel] [PATCHv3 08/10] ptp4l: use ts label to get ts info

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

Now the ts label will be either the bond 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_label changed, we need to check the
clock_required_modes. We will set the link to LINK_DOWN by force if
the new ts_label's timestamp do not support required mode.

If all good, then we will switch phc index to new one.

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

diff --git a/clock.c b/clock.c
index bd2b91b..a9da8c6 100644
--- a/clock.c
+++ b/clock.c
@@ -38,6 +38,7 @@
 #include "servo.h"
 #include "stats.h"
 #include "print.h"
+#include "rtnl.h"
 #include "tlv.h"
 #include "tsproc.h"
 #include "uds.h"
@@ -834,6 +835,16 @@ int clock_required_modes(struct clock *c)
 	return required_modes;
 }
 
+/*
+ * If we do not have a slave or the rtnl query failed, then use our
+ * own interface name as the time stamping interface name.
+ */
+static void ensure_ts_label(struct interface *iface)
+{
+	if (iface->ts_label[0] == '\0')
+		strncpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
+}
+
 struct clock *clock_create(enum clock_type type, struct config *config,
 			   const char *phc_device)
 {
@@ -945,6 +956,9 @@ struct clock *clock_create(enum clock_type type, struct config *config,
 	c->timestamping = timestamping;
 	required_modes = clock_required_modes(c);
 	STAILQ_FOREACH(iface, &config->interfaces, list) {
+		rtnl_get_ts_label(iface);
+		ensure_ts_label(iface);
+		sk_get_ts_info(iface->ts_label, &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++;
 
diff --git a/port.c b/port.c
index 81d52ff..5f638c0 100644
--- a/port.c
+++ b/port.c
@@ -60,6 +60,7 @@ enum link_state {
 	LINK_DOWN  = (1<<0),
 	LINK_UP  = (1<<1),
 	LINK_STATE_CHANGED = (1<<3),
+	TS_LABEL_CHANGED  = (1<<4),
 };
 
 struct nrate_estimator {
@@ -2231,6 +2232,8 @@ static void port_link_status(void *ctx, int linkup, int ts_index)
 {
 	struct port *p = ctx;
 	int link_state;
+	char ts_label[MAX_IFNAME_SIZE + 1];
+	int required_modes;
 
 	link_state = linkup ? LINK_UP : LINK_DOWN;
 	if (p->link_status & link_state) {
@@ -2240,6 +2243,35 @@ static void port_link_status(void *ctx, int linkup, int ts_index)
 		pr_notice("port %hu: link %s", portnum(p), linkup ? "up" : "down");
 	}
 
+	/* ts_label changed */
+	if (if_indextoname(ts_index, ts_label) && strcmp(p->iface->ts_label, ts_label)) {
+		strncpy(p->iface->ts_label, ts_label, MAX_IFNAME_SIZE);
+		sk_get_ts_info(p->iface->ts_label, &p->iface->ts_info);
+
+		p->link_status |= TS_LABEL_CHANGED;
+		pr_notice("port %hu: ts label changed to %s", portnum(p), ts_label);
+	}
+
+	/* We set the link status to down by force if its timestamp not
+	 * support required mode. But the link's status is actually up.
+	 *
+	 * So the next time we receive this link's rtnl message, we need
+	 * to check the required_modes again. If still not support
+	 * required_modes, then keep the link status down.
+	 */
+	if (p->iface->ts_info.valid) {
+		required_modes = clock_required_modes(p->clock);
+		if ((p->iface->ts_info.so_timestamping & required_modes) != required_modes) {
+			pr_err("interface '%s' does not support requested "
+			       "timestamping mode, set link status down by force.",
+			       p->iface->ts_label);
+			p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
+		} else if (p->link_status & TS_LABEL_CHANGED) {
+			p->phc_index = p->iface->ts_info.phc_index;
+			clock_switch_phc(p->clock, p->phc_index);
+		}
+	}
+
 	/*
 	 * A port going down can affect the BMCA result.
 	 * Force a state decision event.
@@ -2292,7 +2324,8 @@ enum fsm_event port_event(struct port *p, int fd_index)
 		rtnl_link_status(fd, p->name, port_link_status, p);
 		if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
 			return EV_FAULT_CLEARED;
-		else if (p->link_status == (LINK_DOWN | LINK_STATE_CHANGED))
+		else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
+			 (p->link_status & TS_LABEL_CHANGED))
 			return EV_FAULT_DETECTED;
 		else
 			return EV_NONE;
-- 
2.5.5

Re: [Linuxptp-devel] [PATCHv3 08/10] ptp4l: use ts label to get ts info

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

On Wed, Aug 16, 2017 at 09:32:09PM +0800, Hangbin Liu wrote:
> +	/* ts_label changed */
> +	if (if_indextoname(ts_index, ts_label) && strcmp(p->iface->ts_label, ts_label)) {
> +		strncpy(p->iface->ts_label, ts_label, MAX_IFNAME_SIZE);
> +		sk_get_ts_info(p->iface->ts_label, &p->iface->ts_info);
> +
> +		p->link_status |= TS_LABEL_CHANGED;
> +		pr_notice("port %hu: ts label changed to %s", portnum(p), ts_label);
> +	}
> +
> +	/* We set the link status to down by force if its timestamp not
> +	 * support required mode. But the link's status is actually up.
> +	 *
> +	 * So the next time we receive this link's rtnl message, we need
> +	 * to check the required_modes again. If still not support
> +	 * required_modes, then keep the link status down.
> +	 */
> +	if (p->iface->ts_info.valid) {
> +		required_modes = clock_required_modes(p->clock);
> +		if ((p->iface->ts_info.so_timestamping & required_modes) != required_modes) {
> +			pr_err("interface '%s' does not support requested "
> +			       "timestamping mode, set link status down by force.",
> +			       p->iface->ts_label);
> +			p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
> +		} else if (p->link_status & TS_LABEL_CHANGED) {
> +			p->phc_index = p->iface->ts_info.phc_index;
> +			clock_switch_phc(p->clock, p->phc_index);
> +		}
> +	}

One more issue found. When we set link down and up, the phc index may changed.

# ethtool -T p7p1
Time stamping parameters for p7p1:
Capabilities:
        hardware-transmit     (SOF_TIMESTAMPING_TX_HARDWARE)
        software-transmit     (SOF_TIMESTAMPING_TX_SOFTWARE)
        hardware-receive      (SOF_TIMESTAMPING_RX_HARDWARE)
        software-receive      (SOF_TIMESTAMPING_RX_SOFTWARE)
        software-system-clock (SOF_TIMESTAMPING_SOFTWARE)
        hardware-raw-clock    (SOF_TIMESTAMPING_RAW_HARDWARE)
PTP Hardware Clock: 5
Hardware Transmit Timestamp Modes:
        off                   (HWTSTAMP_TX_OFF)
        on                    (HWTSTAMP_TX_ON)
Hardware Receive Filter Modes:
        none                  (HWTSTAMP_FILTER_NONE)
        ptpv1-l4-sync         (HWTSTAMP_FILTER_PTP_V1_L4_SYNC)
        ptpv1-l4-delay-req    (HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ)
        ptpv2-event           (HWTSTAMP_FILTER_PTP_V2_EVENT)

# ip link set p7p1 down && sleep 10 && ip link set p7p1 up

# ethtool -T p7p1
Time stamping parameters for p7p1:
Capabilities:
        hardware-transmit     (SOF_TIMESTAMPING_TX_HARDWARE)
        software-transmit     (SOF_TIMESTAMPING_TX_SOFTWARE)
        hardware-receive      (SOF_TIMESTAMPING_RX_HARDWARE)
        software-receive      (SOF_TIMESTAMPING_RX_SOFTWARE)
        software-system-clock (SOF_TIMESTAMPING_SOFTWARE)
        hardware-raw-clock    (SOF_TIMESTAMPING_RAW_HARDWARE)
PTP Hardware Clock: 6
Hardware Transmit Timestamp Modes:
        off                   (HWTSTAMP_TX_OFF)
        on                    (HWTSTAMP_TX_ON)
Hardware Receive Filter Modes:
        none                  (HWTSTAMP_FILTER_NONE)
        ptpv1-l4-sync         (HWTSTAMP_FILTER_PTP_V1_L4_SYNC)
        ptpv1-l4-delay-req    (HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ)
        ptpv2-event           (HWTSTAMP_FILTER_PTP_V2_EVENT)

This will cause errors like

ptp4l[90454.937]: failed to adjust the clock: No such device
ptp4l[90454.937]: failed to step clock: No such device

after link down/up. So we need to get_ts_info every time link up.

I will fix it in the new version of this patch.

Thanks
Hangbin