Hi Richard,
Is there anything still unclear? What should I do to move this patchset
forward?
Thanks
Hangbin
On Tue, Mar 22, 2022 at 06:11:29PM +0800, Hangbin Liu wrote:
> On Mon, Mar 21, 2022 at 08:06:33PM -0700, Richard Cochran wrote:
> > On Fri, Jan 21, 2022 at 12:42:49PM +0800, Hangbin Liu wrote:
> >
> > > In clock_create(), try get ts info first, if failed, try the old way.
> >
> > How does this sentence relate to this...
> >
> > > @@ -1001,10 +1001,16 @@ 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);
> > > + /* try find lowerlay device if we can't get iface tsinfo
> > > directly */
> > > + if (interface_get_tsinfo(iface) ||
> > > + (interface_tsinfo_valid(iface) &&
> > > + !interface_tsmodes_supported(iface, required_modes))) {
> > > + 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);
> > > + }
> >
> > I really have no idea what this is supposed to do.
> >
> > Why test for
> >
> > interface_tsinfo_valid(iface) && !interface_tsmodes_supported(iface,
> > required_modes)
> >
> > twice in a row !?!?!?
>
> If we run the code in old kernel, that bond do not support get_ts_info. The
> kernel will still return 0 and set phc_index to -1, as kernel function
> __ethtool_get_ts_info() did.
>
> So even interface_get_tsinfo() return 0, we still need to check to make sure
> the tsinfo is valid and tsmode is supported. If all the check failed, we
> fall back to set ts_label and re-do interface_get_tsinfo().
>
> After this step, we need to re-check the tsinfo_valid and tsmode. Did I
> explain it clear?
>
> >
> > > +
> > > if (interface_tsinfo_valid(iface) &&
> > > !interface_tsmodes_supported(iface, required_modes)) {
> >
> > See that ? ^^^^^^^^^^^^^^^^^^^
>
> We can't only check this once. e.g.
>
> if (interface_get_tsinfo(iface)) {
> 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);
> }
>
> if (interface_tsinfo_valid(iface) &&
> !interface_tsmodes_supported(iface, required_modes)) {
> pr_err();
> return NULL;
> }
>
> Thanks
> Hangbin
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
