RE: [PATCH v2 3/4] net: Let the active time stamping layer be selectable.

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Köry Maincent wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> 
> Make the sysfs knob writable, and add checks in the ioctl and time
> stamping paths to respect the currently selected time stamping layer.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Notes:
>     Changes in v2:
>     - Move selected_timestamping_layer introduction in this patch.
>     - Replace strmcmp by sysfs_streq.
>     - Use the PHY timestamp only if available.
> 
>  .../ABI/testing/sysfs-class-net-timestamping  |  5 +-
>  drivers/net/phy/phy_device.c                  |  6 +++
>  include/linux/netdevice.h                     | 10 ++++
>  net/core/dev_ioctl.c                          | 44 ++++++++++++++--
>  net/core/net-sysfs.c                          | 50 +++++++++++++++++--
>  net/core/timestamping.c                       |  6 +++
>  net/ethtool/common.c                          | 18 +++++--
>  7 files changed, 127 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net-timestamping b/Documentation/ABI/testing/sysfs-class-net-timestamping
> index 529c3a6eb607..6dfd59740cad 100644
> --- a/Documentation/ABI/testing/sysfs-class-net-timestamping
> +++ b/Documentation/ABI/testing/sysfs-class-net-timestamping
> @@ -11,7 +11,10 @@ What:		/sys/class/net/<iface>/current_timestamping_provider
>  Date:		January 2022
>  Contact:	Richard Cochran <richardcochran@gmail.com>
>  Description:
> -		Show the current SO_TIMESTAMPING provider.
> +		Shows or sets the current SO_TIMESTAMPING provider.
> +		When changing the value, some packets in the kernel
> +		networking stack may still be delivered with time
> +		stamps from the previous provider.
>  		The possible values are:
>  		- "mac"  The MAC provides time stamping.
>  		- "phy"  The PHY or MII device provides time stamping.
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8cff61dbc4b5..8dff0c6493b5 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1451,6 +1451,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  
>  	phydev->phy_link_change = phy_link_change;
>  	if (dev) {
> +		if (phy_has_hwtstamp(phydev))
> +			dev->selected_timestamping_layer = PHY_TIMESTAMPING;
> +		else
> +			dev->selected_timestamping_layer = MAC_TIMESTAMPING;
> +
>  		phydev->attached_dev = dev;
>  		dev->phydev = phydev;
>  
> @@ -1762,6 +1767,7 @@ void phy_detach(struct phy_device *phydev)
>  
>  	phy_suspend(phydev);
>  	if (dev) {
> +		dev->selected_timestamping_layer = MAC_TIMESTAMPING;
>  		phydev->attached_dev->phydev = NULL;
>  		phydev->attached_dev = NULL;
>  	}
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ba2bd604359d..d8e9da2526f0 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1742,6 +1742,11 @@ enum netdev_ml_priv_type {
>  	ML_PRIV_CAN,
>  };
>  
> +enum timestamping_layer {
> +	MAC_TIMESTAMPING,
> +	PHY_TIMESTAMPING,
> +};
> +
>  /**
>   *	struct net_device - The DEVICE structure.
>   *
> @@ -1981,6 +1986,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.
> @@ -2339,6 +2347,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)
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 7674bb9f3076..cc7cf2a542fb 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -262,6 +262,43 @@ static int dev_eth_ioctl(struct net_device *dev,
>  	return err;
>  }
>  
> +static int dev_hwtstamp_ioctl(struct net_device *dev,
> +			      struct ifreq *ifr, unsigned int cmd)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	int err;
> +
> +	err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
> +	if (err == 0 || err != -EOPNOTSUPP)
> +		return err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	switch (dev->selected_timestamping_layer) {
> +
> +	case MAC_TIMESTAMPING:
> +		if (ops->ndo_do_ioctl == phy_do_ioctl) {
> +			/* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */
> +			err = -EOPNOTSUPP;
> +		} else {
> +			err = ops->ndo_eth_ioctl(dev, ifr, cmd);
> +		}
> +		break;
> +
> +	case PHY_TIMESTAMPING:
> +		if (phy_has_hwtstamp(dev->phydev)) {
> +			err = phy_mii_ioctl(dev->phydev, ifr, cmd);
> +		} else {
> +			err = -ENODEV;
> +			WARN_ON(1);
> +		}
> +		break;
> +	}
> +
> +	return err;
> +}
> +
>  static int dev_siocbond(struct net_device *dev,
>  			struct ifreq *ifr, unsigned int cmd)
>  {
> @@ -397,6 +434,9 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>  			return err;
>  		fallthrough;
>  
> +	case SIOCGHWTSTAMP:
> +		return dev_hwtstamp_ioctl(dev, ifr, cmd);
> +
>  	/*
>  	 *	Unknown or private ioctl
>  	 */
> @@ -407,9 +447,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>  
>  		if (cmd == SIOCGMIIPHY ||
>  		    cmd == SIOCGMIIREG ||
> -		    cmd == SIOCSMIIREG ||
> -		    cmd == SIOCSHWTSTAMP ||
> -		    cmd == SIOCGHWTSTAMP) {
> +		    cmd == SIOCSMIIREG) {
>  			err = dev_eth_ioctl(dev, ifr, cmd);
>  		} else if (cmd == SIOCBONDENSLAVE ||
>  		    cmd == SIOCBONDRELEASE ||
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 26095634fb31..66079424b100 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -666,17 +666,59 @@ static ssize_t current_timestamping_provider_show(struct device *dev,
>  	if (!rtnl_trylock())
>  		return restart_syscall();
>  
> -	if (phy_has_tsinfo(phydev)) {
> -		ret = sprintf(buf, "%s\n", "phy");
> -	} else {
> +	switch (netdev->selected_timestamping_layer) {
> +	case MAC_TIMESTAMPING:
>  		ret = sprintf(buf, "%s\n", "mac");
> +		break;
> +	case PHY_TIMESTAMPING:
> +		ret = sprintf(buf, "%s\n", "phy");
> +		break;
>  	}
>  
>  	rtnl_unlock();
>  
>  	return ret;
>  }
> -static DEVICE_ATTR_RO(current_timestamping_provider);
> +
> +static ssize_t current_timestamping_provider_store(struct device *dev,
> +						   struct device_attribute *attr,
> +						   const char *buf, size_t len)
> +{
> +	struct net_device *netdev = to_net_dev(dev);
> +	struct net *net = dev_net(netdev);
> +	enum timestamping_layer flavor;
> +
> +	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	if (sysfs_streq(buf, "mac"))
> +		flavor = MAC_TIMESTAMPING;
> +	else if (sysfs_streq(buf, "phy"))
> +		flavor = PHY_TIMESTAMPING;
> +	else
> +		return -EINVAL;

Should setting netdev->selected_timestamping_layer be limited to
choices that the device supports?

At a higher level, this series assumes that any timestamp not through
phydev is a MAC timestamp. I don't think that is necessarily true for
all devices. Some may timestamp at the phy, but not expose a phydev.
This is a somewhat pedantic point. I understand that the purpose of
the series is to select from among two sets of APIs.