Köry Maincent wrote:
> From: Richard Cochran <richardcochran@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Kory Maincent <kory.maincent@xxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxx>
> 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.
