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

["Köry Maincent" <kory.maincent@bootlin.com>]

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.