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

Willem de Bruijn
2023-03-08T15:28:57Z

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;