Hi Richard,
Very sorry for the late reply. There is something wrong with my offlineimap
and I didn't got the email until yesterday....
On Sat, May 14, 2022 at 02:35:05PM -0700, Richard Cochran wrote:
> > - }
> > + port_change_phc(p);
> > }
>
> ... alreadly improves the readability of the code. Please put that
> bit into its own patch.
OK, I will.
>
> > @@ -2802,7 +2815,12 @@ static enum fsm_event bc_event(struct port *p, int
> > fd_index)
> >
> > case FD_RTNL:
> > pr_debug("%s: received link status notification", p->log_name);
> > - rtnl_link_status(fd, p->name, port_link_status, p);
> > + if (!interface_get_tsinfo(p->iface) &&
> > + (p->phc_index != interface_phc_index(p->iface)))
> > + port_change_phc(p);
> > + else
> > + rtnl_link_status(fd, p->name, port_link_status, p);
>
> However this part will cause issues. When the RTNL event occurs,
> there can be multiple messages. The callback is invoked on each one:
>
> rtnl_link_status()
> {
> ...
> for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
> ...
> if (cb)
> cb(ctx, link_up, slave_index);
> }
> }
>
> You new logic will drop link up/down notifications (or anything else
> we might add later).
Ah, yes, you are right. Only failover or link down works correct.
If these 2 issue happens at the same time. We will miss the link down
event.
>
> In some cases, your change will only call port_change_phc() but will
> avoid the important other part of port_link_status():
>
> /*
> * A port going down can affect the BMCA result.
> * Force a state decision event.
> */
> if (p->link_status & LINK_DOWN)
> clock_set_sde(p->clock, 1);
>
> Why not simply remove the extra test and check
> (p->phc_index != interface_phc_index(p->iface))
> in the port_change_phc() helper function?
OK, I will.
Thanks
Hangbin
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
