[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6408a9b3c7ae1_13061c2082a@willemb.c.googlers.com.notmuch>
Date: Wed, 08 Mar 2023 10:28:51 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Köry Maincent <kory.maincent@...tlin.com>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-omap@...r.kernel.org
Cc: Michael Walle <michael@...le.cc>,
Maxime Chevallier <maxime.chevallier@...tlin.com>,
Richard Cochran <richardcochran@...il.com>,
Kory Maincent <kory.maincent@...tlin.com>,
thomas.petazzoni@...tlin.com, Russell King <linux@...linux.org.uk>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jonathan Corbet <corbet@....net>,
Jay Vosburgh <j.vosburgh@...il.com>,
Veaceslav Falico <vfalico@...il.com>,
Andy Gospodarek <andy@...yhouse.net>,
Joakim Zhang <qiangqing.zhang@....com>,
Vladimir Oltean <vladimir.oltean@....com>,
Claudiu Manoil <claudiu.manoil@....com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
UNGLinuxDriver@...rochip.com,
Grygorii Strashko <grygorii.strashko@...com>,
Minghao Chi <chi.minghao@....com.cn>,
Guangbin Huang <huangguangbin2@...wei.com>,
Jie Wang <wangjie125@...wei.com>,
Oleksij Rempel <linux@...pel-privat.de>,
Sean Anderson <sean.anderson@...o.com>,
Alexandru Tachici <alexandru.tachici@...log.com>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Wolfram Sang <wsa+renesas@...g-engineering.com>,
Alexander Lobakin <alexandr.lobakin@...el.com>,
Marco Bonelli <marco@...eim.net>
Subject: RE: [PATCH v3 3/5] net: Let the active time stamping layer be
selectable.
Köry Maincent wrote:
> From: Richard Cochran <richardcochran@...il.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@...il.com>
> Signed-off-by: Kory Maincent <kory.maincent@...tlin.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;
Powered by blists - more mailing lists