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

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: "Köry Maincent" <kory.maincent@bootlin.com>,
	"Andrew Lunn" <andrew@lunn.ch>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-omap@vger.kernel.org
Cc: Michael Walle <michael@walle.cc>,
	Maxime Chevallier <maxime.chevallier@bootlin.com>,
	Richard Cochran <richardcochran@gmail.com>,
	Kory Maincent <kory.maincent@bootlin.com>,
	thomas.petazzoni@bootlin.com,
	Russell King <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Jay Vosburgh <j.vosburgh@gmail.com>,
	Veaceslav Falico <vfalico@gmail.com>,
	Andy Gospodarek <andy@greyhouse.net>,
	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>,
	Minghao Chi <chi.minghao@zte.com.cn>,
	Guangbin Huang <huangguangbin2@huawei.com>,
	Jie Wang <wangjie125@huawei.com>,
	Oleksij Rempel <linux@rempel-privat.de>,
	Sean Anderson <sean.anderson@seco.com>,
	Alexandru Tachici <alexandru.tachici@analog.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Alexander Lobakin <alexandr.lobakin@intel.com>,
	Marco Bonelli <marco@mebeim.net>
Subject: RE: [PATCH v3 3/5] net: Let the active time stamping layer be selectable.
Date: Wed, 08 Mar 2023 10:28:51 -0500	[thread overview]
Message-ID: <6408a9b3c7ae1_13061c2082a@willemb.c.googlers.com.notmuch> (raw)
In-Reply-To: <20230308135936.761794-4-kory.maincent@bootlin.com>

Köry Maincent wrote:
> From: Richard Cochran <richardcochran@gmail.com>
> 
> Add the ETHTOOL_SET_PTP ethtool ioctl, and add checks in the ioctl and time
> stamping paths to respect the currently selected time stamping layer.
> 
> Add a preferred-timestamp devicetree binding to select the preferred
> hardware timestamp layer between PHY and MAC. The choice of using
> devicetree binding has been made as the PTP precision and quality depends
> of external things, like adjustable clock, or the lack of a temperature
> compensated crystal or specific features. Even if the preferred timestamp
> is a configuration it is hardly related to the design oh the board.

nit: oh -> of

> 
> 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.
>     
>     Changes in v3:
>     - Added a devicetree binding to select the preferred timestamp
>     - Replace the way to select timestamp through ethtool instead of sysfs
>     You can test it with the ethtool source on branch feature_ptp of:
>     https://github.com/kmaincent/ethtool
> 
>  Documentation/networking/ethtool-netlink.rst |  1 +
>  drivers/net/phy/phy_device.c                 | 34 ++++++++++++++++
>  include/linux/netdevice.h                    |  6 +++
>  include/uapi/linux/ethtool.h                 |  1 +
>  net/core/dev_ioctl.c                         | 43 ++++++++++++++++++--
>  net/core/timestamping.c                      |  6 +++
>  net/ethtool/common.c                         | 16 ++++++--
>  net/ethtool/ioctl.c                          | 41 ++++++++++++++-----
>  8 files changed, 131 insertions(+), 17 deletions(-)
> 
> +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;
> +
> +	if (phy_has_hwtstamp(phydev))
> +		ts_layer = PHY_TIMESTAMPING;
> +	else if (ops->get_ts_info)
> +		ts_layer = MAC_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 = PHY_TIMESTAMPING;
> +
> +	if (ops->get_ts_info && !strcmp(s, "mac"))
> +		ts_layer = MAC_TIMESTAMPING;
> +
> +out:
> +	netdev->selected_timestamping_layer = ts_layer;
> +}
> +
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ba2bd604359d..d9a1c12fc43c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -47,6 +47,7 @@
>  #include <uapi/linux/netdevice.h>
>  #include <uapi/linux/if_bonding.h>
>  #include <uapi/linux/pkt_cls.h>
> +#include <uapi/linux/net_tstamp.h>
>  #include <linux/hashtable.h>
>  #include <linux/rbtree.h>
>  #include <net/net_trackers.h>
> @@ -1981,6 +1982,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 +2343,8 @@ struct net_device {
>  	unsigned		wol_enabled:1;
>  	unsigned		threaded:1;
>  
> +	enum timestamping_layer selected_timestamping_layer;
> +

can perhaps be a single bit rather than an enum

> +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);

Please no WARN_ON on error cases that are known to be reachable
and can be handled safely and reported to userspace.

> +		}
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 64a7e05cf2c2..e55e70bdbb3c 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -548,10 +548,18 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
>  	memset(info, 0, sizeof(*info));
>  	info->cmd = ETHTOOL_GET_TS_INFO;
>  
> -	if (phy_has_tsinfo(phydev))
> -		return phy_ts_info(phydev, info);
> -	if (ops->get_ts_info)
> -		return ops->get_ts_info(dev, info);
> +	switch (dev->selected_timestamping_layer) {
> +	case MAC_TIMESTAMPING:
> +		if (ops->get_ts_info)
> +			return ops->get_ts_info(dev, info);
> +		break;
> +
> +	case PHY_TIMESTAMPING:
> +		if (phy_has_tsinfo(phydev))
> +			return phy_ts_info(phydev, info);
> +		WARN_ON(1);
> +		return -ENODEV;

same

> +	}
>  
>  	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
>  				SOF_TIMESTAMPING_SOFTWARE;