Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

Köry Maincent
2023-05-02T11:05:35Z

On Sat, 29 Apr 2023 20:58:07 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

Thanks for the review and sorry for the coding style issues, I forgot to run the
checkpatch script.

> >
> > #if IS_ENABLED(CONFIG_MACSEC)
> > @@ -2879,6 +2885,7 @@ enum netdev_cmd {
> > NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
> > NETDEV_XDP_FEAT_CHANGE,
> > NETDEV_PRE_CHANGE_HWTSTAMP,
> > + NETDEV_CHANGE_HWTSTAMP,
>
> Don't create new netdev notifiers with no users. Also,
> NETDEV_PRE_CHANGE_HWTSTAMP has disappered.

Ok, right you move it on to dsa stub. What do you think of our case, should we
continue with netdev notifier?

> > diff --git a/include/uapi/linux/ethtool_netlink.h
> > b/include/uapi/linux/ethtool_netlink.h index 447908393b91..4f03e7cde271
> > 100644 --- a/include/uapi/linux/ethtool_netlink.h
> > +++ b/include/uapi/linux/ethtool_netlink.h
> > @@ -41,6 +41,7 @@ enum {
> > ETHTOOL_MSG_TSINFO_GET,
> > ETHTOOL_MSG_TSLIST_GET,
> > ETHTOOL_MSG_TS_GET,
> > + ETHTOOL_MSG_TS_SET,
>
> The way in which the Linux kernel ensures a stable API towards user
> space is that programs written against kernel headers from 5 years ago
> still work with kernels from today. In your case, you would be breaking
> that, because before this patch, ETHTOOL_MSG_CABLE_TEST_ACT was 26, and
> after your patch it is 27. So old applications emitting a cable test
> netlink message would be interpreted by new kernels as emitting a "set
> timestamping layer" netlink message. Of course not only the cable test
> is affected, every other netlink message until the end is now shifted by
> 1. This is why we put new enum values only at the very end, where it
> actually says they should go:

Sorry for that, this indeed didn't cross my head. I will fix it.

> >
> > +/* TS_SET */
> > +const struct nla_policy ethnl_ts_set_policy[] = {
> > + [ETHTOOL_A_TS_HEADER] =
> > NLA_POLICY_NESTED(ethnl_header_policy),
> > + [ETHTOOL_A_TS_LAYER] = { .type = NLA_U32 },
> > +};
>
> I wanted to explore this topic myself, but I can't seem to find the
> time, so here's something a bit hand-wavey.
>
> We should make a distinction between what the kernel exposes as UAPI
> (our future selves need to work with that in a backwards-compatible way)
> and the internal, unstable kernel implementation.
>
> It would be good if, instead of the ETHTOOL_A_TS_LAYER netlink
> attribute, the kernel could expose a more generic ETHTOOL_A_TS_PHC, from
> which the ethtool core could deduce if the PHC number belongs to the MAC
> or to the PHY. We could still keep something like
> netdev->selected_timestamping_layer in code private to the kernel, but from
> the UAPI perspective, I agree with Andrew that we should design something
> that is cleanly extensible to N PHCs, not just to a vague "layer".

Just some thought:
Maybe we could create a PHC_ID 0xXY where X is the layer and Y the PHCs number
in this layer, but what will append if there is two MACs consecutively?
Also in case of several MACs or PHYs in serial
netdev->selected_timestamping_layer is inappropriate.

Maybe using it like 0xABC where A is the MAC number, B the PHY number and C
the PHC number in the device.
For example if we select the second MAC and its third PHC, PHC_ID will be
0x203. Or if we select the second PHC of the PHY it will be 0x012.