Date:   Fri, 03 Mar 2023 18:59:17 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To:     =?UTF-8?B?S8O2cnkgTWFpbmNlbnQ=?= <kory.maincent@bootlin.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-omap@vger.kernel.org
Cc:     Michael Walle <michael@walle.cc>,
        Richard Cochran <richardcochran@gmail.com>,
        Kory Maincent <kory.maincent@bootlin.com>,
        thomas.petazzoni@bootlin.com, Jay Vosburgh <j.vosburgh@gmail.com>,
        Veaceslav Falico <vfalico@gmail.com>,
        Andy Gospodarek <andy@greyhouse.net>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Joakim Zhang <qiangqing.zhang@nxp.com>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        UNGLinuxDriver@microchip.com,
        Grygorii Strashko <grygorii.strashko@ti.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Minghao Chi <chi.minghao@zte.com.cn>,
        Jie Wang <wangjie125@huawei.com>,
        Guangbin Huang <huangguangbin2@huawei.com>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        Wang Yufen <wangyufen@huawei.com>,
        Alexandru Tachici <alexandru.tachici@analog.com>,
        Oleksij Rempel <linux@rempel-privat.de>
Message-ID: <640289d5ef54c_cc8e2087a@willemb.c.googlers.com.notmuch>
In-Reply-To: <20230303164248.499286-4-kory.maincent@bootlin.com>
References: <20230303164248.499286-1-kory.maincent@bootlin.com>
 <20230303164248.499286-4-kory.maincent@bootlin.com>
Subject: RE: [PATCH v2 3/4] net: Let the active time stamping layer be
 selectable.
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

K=C3=B6ry 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/D=
ocumentation/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_p=
rovider
>  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, st=
ruct phy_device *phydev,
>  =

>  	phydev->phy_link_change =3D phy_link_change;
>  	if (dev) {
> +		if (phy_has_hwtstamp(phydev))
> +			dev->selected_timestamping_layer =3D PHY_TIMESTAMPING;
> +		else
> +			dev->selected_timestamping_layer =3D MAC_TIMESTAMPING;
> +
>  		phydev->attached_dev =3D dev;
>  		dev->phydev =3D phydev;
>  =

> @@ -1762,6 +1767,7 @@ void phy_detach(struct phy_device *phydev)
>  =

>  	phy_suspend(phydev);
>  	if (dev) {
> +		dev->selected_timestamping_layer =3D MAC_TIMESTAMPING;
>  		phydev->attached_dev->phydev =3D NULL;
>  		phydev->attached_dev =3D 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 =3D dev->netdev_ops;
> +	int err;
> +
> +	err =3D dsa_ndo_eth_ioctl(dev, ifr, cmd);
> +	if (err =3D=3D 0 || err !=3D -EOPNOTSUPP)
> +		return err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	switch (dev->selected_timestamping_layer) {
> +
> +	case MAC_TIMESTAMPING:
> +		if (ops->ndo_do_ioctl =3D=3D phy_do_ioctl) {
> +			/* Some drivers set .ndo_do_ioctl to phy_do_ioctl. */
> +			err =3D -EOPNOTSUPP;
> +		} else {
> +			err =3D ops->ndo_eth_ioctl(dev, ifr, cmd);
> +		}
> +		break;
> +
> +	case PHY_TIMESTAMPING:
> +		if (phy_has_hwtstamp(dev->phydev)) {
> +			err =3D phy_mii_ioctl(dev->phydev, ifr, cmd);
> +		} else {
> +			err =3D -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 =3D=3D SIOCGMIIPHY ||
>  		    cmd =3D=3D SIOCGMIIREG ||
> -		    cmd =3D=3D SIOCSMIIREG ||
> -		    cmd =3D=3D SIOCSHWTSTAMP ||
> -		    cmd =3D=3D SIOCGHWTSTAMP) {
> +		    cmd =3D=3D SIOCSMIIREG) {
>  			err =3D dev_eth_ioctl(dev, ifr, cmd);
>  		} else if (cmd =3D=3D SIOCBONDENSLAVE ||
>  		    cmd =3D=3D 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 =3D sprintf(buf, "%s\n", "phy");
> -	} else {
> +	switch (netdev->selected_timestamping_layer) {
> +	case MAC_TIMESTAMPING:
>  		ret =3D sprintf(buf, "%s\n", "mac");
> +		break;
> +	case PHY_TIMESTAMPING:
> +		ret =3D 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 =3D to_net_dev(dev);
> +	struct net *net =3D dev_net(netdev);
> +	enum timestamping_layer flavor;
> +
> +	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	if (sysfs_streq(buf, "mac"))
> +		flavor =3D MAC_TIMESTAMPING;
> +	else if (sysfs_streq(buf, "phy"))
> +		flavor =3D 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.