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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Thu, Apr 06, 2023 at 07:33:07PM +0200, Köry Maincent wrote:
> +static void of_set_timestamp(struct net_device *netdev, struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	const struct ethtool_ops *ops = netdev->ethtool_ops;
> +	const char *s;
> +	enum timestamping_layer ts_layer = 0;

netdev likes variable declaration lines sorted longest to shortest

> +	int i;
> +
> +	/* Backward compatibility to old API */
> +	for (i = 0; phy_timestamping_whitelist[i] != NULL; i++)
> +	{

The kernel coding style likes the opening { on the same line as the for

> +		if (!strcmp(phy_timestamping_whitelist[i],
> +			    phydev->drv->name)) {
> +			if (phy_has_hwtstamp(phydev))
> +				ts_layer = SOF_PHY_TIMESTAMPING;
> +			else if (ops->get_ts_info)
> +				ts_layer = SOF_MAC_TIMESTAMPING;
> +			goto out;
> +		}
> +	}
> +
> +	if (ops->get_ts_info)
> +		ts_layer = SOF_MAC_TIMESTAMPING;
> +	else if (phy_has_hwtstamp(phydev))
> +		ts_layer = SOF_PHY_TIMESTAMPING;
> +
> +	if (of_property_read_string(node, "preferred-timestamp", &s))
> +		goto out;
> +
> +	if (!s)
> +		goto out;
> +
> +	if (phy_has_hwtstamp(phydev) && !strcmp(s, "phy"))
> +		ts_layer = SOF_PHY_TIMESTAMPING;
> +
> +	if (ops->get_ts_info && !strcmp(s, "mac"))
> +		ts_layer = SOF_MAC_TIMESTAMPING;
> +
> +out:
> +	netdev->selected_timestamping_layer = ts_layer;
> +}
> +
>  /**
>   * phy_attach_direct - attach a network device to a given PHY device pointer
>   * @dev: network device to attach
> @@ -1481,6 +1560,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  
>  	phydev->phy_link_change = phy_link_change;
>  	if (dev) {
> +		of_set_timestamp(dev, phydev);
> +
>  		phydev->attached_dev = dev;
>  		dev->phydev = phydev;
>  
> @@ -1811,6 +1892,10 @@ void phy_detach(struct phy_device *phydev)
>  
>  	phy_suspend(phydev);
>  	if (dev) {
> +		if (dev->ethtool_ops->get_ts_info)
> +			dev->selected_timestamping_layer = SOF_MAC_TIMESTAMPING;
> +		else
> +			dev->selected_timestamping_layer = 0;
>  		phydev->attached_dev->phydev = NULL;
>  		phydev->attached_dev = NULL;
>  	}
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a740be3bb911..3dd6be012daf 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -48,6 +48,7 @@
>  #include <uapi/linux/if_bonding.h>
>  #include <uapi/linux/pkt_cls.h>
>  #include <uapi/linux/netdev.h>
> +#include <uapi/linux/net_tstamp.h>
>  #include <linux/hashtable.h>
>  #include <linux/rbtree.h>
>  #include <net/net_trackers.h>
> @@ -2019,6 +2020,9 @@ enum netdev_ml_priv_type {
>   *
>   *	@threaded:	napi threaded mode is enabled
>   *
> + *	@selected_timestamping_layer:	Tracks whether the MAC or the PHY
> + *					performs packet time stamping.
> + *
>   *	@net_notifier_list:	List of per-net netdev notifier block
>   *				that follow this device when it is moved
>   *				to another network namespace.
> @@ -2388,6 +2392,8 @@ struct net_device {
>  	unsigned		wol_enabled:1;
>  	unsigned		threaded:1;
>  
> +	enum timestamping_layer selected_timestamping_layer;
> +
>  	struct list_head	net_notifier_list;
>  
>  #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.

>  };
>  const char *netdev_cmd_to_name(enum netdev_cmd cmd);
>  
> @@ -2934,6 +2941,11 @@ struct netdev_notifier_hwtstamp_info {
>  	struct kernel_hwtstamp_config *config;
>  };
>  
> +struct netdev_notifier_hwtstamp_layer {
> +	struct netdev_notifier_info info; /* must be first */
> +	enum timestamping_layer ts_layer;
> +};
> +
>  enum netdev_offload_xstats_type {
>  	NETDEV_OFFLOAD_XSTATS_TYPE_L3 = 1,
>  };
> 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:

	/* add new constants above here */

>  	ETHTOOL_MSG_CABLE_TEST_ACT,
>  	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
>  	ETHTOOL_MSG_TUNNEL_INFO_GET,
> @@ -96,6 +97,7 @@ enum {
>  	ETHTOOL_MSG_TSINFO_GET_REPLY,
>  	ETHTOOL_MSG_TSLIST_GET_REPLY,
>  	ETHTOOL_MSG_TS_GET_REPLY,
> +	ETHTOOL_MSG_TS_NTF,

Similar issue.

>  	ETHTOOL_MSG_CABLE_TEST_NTF,
>  	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
>  	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 695c7c4a816b..daea7221dd25 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -61,9 +55,65 @@ static int ts_fill_reply(struct sk_buff *skb,
>  			     const struct ethnl_reply_data *reply_base)
>  {
>  	struct ts_reply_data *data = TS_REPDATA(reply_base);
> +

gratuitous change

>  	return nla_put_u32(skb, ETHTOOL_A_TS_LAYER, data->ts);
>  }
>  
> +/* 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".