On Thu, Jan 06, 2022 at 12:35:10PM +0100, Miroslav Lichvar wrote:
> On Thu, Jan 06, 2022 at 06:19:01PM +0800, Hangbin Liu wrote:
> > Latest Linux kernel has supported getting PHC from bond directly. This would
> > help topology like VLAN over bond to get PHC natively. To achieve this,
> > the new hwtstamp flag is needed when passing hwtstamp_config to kernel.
>
> > @@ -1001,10 +1001,18 @@ 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) {
> > - memset(ts_label, 0, sizeof(ts_label));
> > - if (!rtnl_get_ts_device(interface_name(iface), ts_label))
> > - interface_set_label(iface, ts_label);
> > - interface_get_tsinfo(iface);
> > +#ifdef HWTSTAMP_FLAG_BONDED_PHC_INDEX
> > + if (interface_get_tsinfo(iface) ||
> > + (interface_tsinfo_valid(iface) &&
> > + !interface_tsmodes_supported(iface, required_modes))) {
> > +#endif
>
> Why is this and the other parts in the patch wrapped in the ifdef?
> I'd expect the kernel feature to matter only in sk.c. If not using
> bonding, the code should work the same no matter the kernel has a new
> flag, right?
Right, without ifdef, the code still works even the kernel does not support
the new flag. But in this block we will check the tsinfo twice every time.
And in other blocks, e.g.
case FD_RTNL:
pr_debug("%s: received link status notification", p->log_name);
- rtnl_link_status(fd, p->name, port_link_status, p);
+
+#ifdef HWTSTAMP_FLAG_BONDED_PHC_INDEX
+ if (!interface_get_tsinfo(p->iface) &&
+ (p->phc_index != interface_phc_index(p->iface)))
+ port_change_phc(p);
+ else
+#endif
+ rtnl_link_status(fd, p->name, port_link_status, p);
+
we will waste time on the if check as we always need to fall to else condition
and call port_link_status at the end.
So with the ifdef we can save some code path on old kernels.
>
> > + 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)) {
> > + (p->link_status & TS_LABEL_CHANGED) ||
> > + (p->link_status & PHC_INDEX_CHANGED)) {
>
> I think this and the other instances can be simplified to
> (p->link_status &
> (TS_LABEL_CHANGED|PHC_INDEX_CHANGED))
yep
>
> > @@ -41,6 +41,7 @@ enum link_state {
> > LINK_UP = (1<<1),
> > LINK_STATE_CHANGED = (1<<3),
> > TS_LABEL_CHANGED = (1<<4),
> > + PHC_INDEX_CHANGED = (1<<5),
> > };
>
> Just wondering, is the new state necessary? Would it be simpler to
> handle it as a special case of TS_LABEL_CHANGED, where the label
> "changed" to the same value, only refreshing the PHC index?
Hmm... Yes, makes sense to me. Let me do some testing in case any regression.
Thanks
Hangbin
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
