Message-Id: <20230402142435.47105-1-glipus@gmail.com>
Date:   Sun,  2 Apr 2023 08:24:35 -0600
From:   Maxim Georgiev <glipus@...il.com>
To:     kory.maincent@...tlin.com
Cc:     kuba@...nel.org, netdev@...r.kernel.org, glipus@...il.com,
        maxime.chevallier@...tlin.com, vladimir.oltean@....com
Subject: [PATCH net-next RFC v2] Add NDOs for hardware timestamp get/set

Current NIC driver API demands drivers supporting hardware timestamping
to support SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs. Handling these IOCTLs
requires dirivers to implement request parameter structure translation
between user and kernel address spaces, handling possible
translation failures, etc. This translation code is pretty much
identical across most of the NIC drivers that support SIOCGHWTSTAMP/
SIOCSHWTSTAMP.
This patch extends NDO functiuon set with ndo_hwtstamp_get/set
functions, implements SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTL translation
to ndo_hwtstamp_get/set function calls including parameter structure
translation and translation error handling.

This patch is sent out as RFC.
It still pending on basic testing. Implementing ndo_hwtstamp_get/set
in netdevsim driver should allow manual testing of the request
translation logic. Also is there a way to automate this testing?

Suggested-by: Jakub Kicinski <kuba@...nel.org>
Signed-off-by: Maxim Georgiev <glipus@...il.com>
---
Changes in v2:
- Introduced kernel_hwtstamp_config structure
- Added netlink_ext_ack* and kernel_hwtstamp_config* as NDO hw timestamp
  function parameters
- Reodered function variable declarations in dev_hwtstamp()
- Refactored error handling logic in dev_hwtstamp()
- Split dev_hwtstamp() into GET and SET versions
- Changed net_hwtstamp_validate() to accept struct hwtstamp_config *
  as a parameter
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 39 ++++++-----
 drivers/net/netdevsim/netdev.c             | 26 +++++++
 drivers/net/netdevsim/netdevsim.h          |  1 +
 include/linux/netdevice.h                  | 21 ++++++
 include/uapi/linux/net_tstamp.h            | 15 ++++
 net/core/dev_ioctl.c                       | 81 ++++++++++++++++++----
 6 files changed, 149 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 6f5c16aebcbf..5b98f7257c77 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6161,7 +6161,9 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
 /**
  * e1000e_hwtstamp_set - control hardware time stamping
  * @netdev: network interface device structure
- * @ifr: interface request
+ * @config: hwtstamp_config structure containing request parameters
+ * @kernel_config: kernel version of config parameter structure.
+ * @extack: netlink request parameters.
  *
  * Outgoing time stamping can be enabled and disabled. Play nice and
  * disable it when requested, although it shouldn't cause any overhead
@@ -6174,20 +6176,19 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
  * specified. Matching the kind of event packet is not supported, with the
  * exception of "all V2 events regardless of level 2 or 4".
  **/
-static int e1000e_hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
+static int e1000e_hwtstamp_set(struct net_device *netdev,
+			       struct hwtstamp_config *config,
+			       struct kernel_hwtstamp_config *kernel_config,
+			       struct netlink_ext_ack *extack)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
-	struct hwtstamp_config config;
 	int ret_val;
 
-	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-		return -EFAULT;
-
-	ret_val = e1000e_config_hwtstamp(adapter, &config);
+	ret_val = e1000e_config_hwtstamp(adapter, config);
 	if (ret_val)
 		return ret_val;
 
-	switch (config.rx_filter) {
+	switch (config->rx_filter) {
 	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
@@ -6199,22 +6200,24 @@ static int e1000e_hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
 		 * by hardware so notify the caller the requested packets plus
 		 * some others are time stamped.
 		 */
-		config.rx_filter = HWTSTAMP_FILTER_SOME;
+		config->rx_filter = HWTSTAMP_FILTER_SOME;
 		break;
 	default:
 		break;
 	}
-
-	return copy_to_user(ifr->ifr_data, &config,
-			    sizeof(config)) ? -EFAULT : 0;
+	return ret_val;
 }
 
-static int e1000e_hwtstamp_get(struct net_device *netdev, struct ifreq *ifr)
+static int e1000e_hwtstamp_get(struct net_device *netdev,
+			       struct hwtstamp_config *config,
+			       struct kernel_hwtstamp_config *kernel_config,
+			       struct netlink_ext_ack *extack)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 
-	return copy_to_user(ifr->ifr_data, &adapter->hwtstamp_config,
-			    sizeof(adapter->hwtstamp_config)) ? -EFAULT : 0;
+	memcpy(config, &adapter->hwtstamp_config,
+	       sizeof(adapter->hwtstamp_config));
+	return 0;
 }
 
 static int e1000_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
@@ -6224,10 +6227,6 @@ static int e1000_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 	case SIOCGMIIREG:
 	case SIOCSMIIREG:
 		return e1000_mii_ioctl(netdev, ifr, cmd);
-	case SIOCSHWTSTAMP:
-		return e1000e_hwtstamp_set(netdev, ifr);
-	case SIOCGHWTSTAMP:
-		return e1000e_hwtstamp_get(netdev, ifr);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -7365,6 +7364,8 @@ static const struct net_device_ops e1000e_netdev_ops = {
 	.ndo_set_features = e1000_set_features,
 	.ndo_fix_features = e1000_fix_features,
 	.ndo_features_check	= passthru_features_check,
+	.ndo_hwtstamp_get	= e1000e_hwtstamp_get,
+	.ndo_hwtstamp_set	= e1000e_hwtstamp_set,
 };
 
 /**
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 35fa1ca98671..502715c6e9e1 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -238,6 +238,30 @@ nsim_set_features(struct net_device *dev, netdev_features_t features)
 	return 0;
 }
 
+static int
+nsim_hwtstamp_get(struct net_device *dev,
+		  struct hwtstamp_config *config,
+		  struct kernel_hwtstamp_config *kernel_config,
+		  struct netlink_ext_ack *extack)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	memcpy(config, &ns->hw_tstamp_config, sizeof(ns->hw_tstamp_config));
+	return 0;
+}
+
+static int
+nsim_hwtstamp_ges(struct net_device *dev,
+		  struct hwtstamp_config *config,
+		  struct kernel_hwtstamp_config *kernel_config,
+		  struct netlink_ext_ack *extack)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	memcpy(&ns->hw_tstamp_config, config, sizeof(ns->hw_tstamp_config));
+	return 0;
+}
+
 static const struct net_device_ops nsim_netdev_ops = {
 	.ndo_start_xmit		= nsim_start_xmit,
 	.ndo_set_rx_mode	= nsim_set_rx_mode,
@@ -256,6 +280,8 @@ static const struct net_device_ops nsim_netdev_ops = {
 	.ndo_setup_tc		= nsim_setup_tc,
 	.ndo_set_features	= nsim_set_features,
 	.ndo_bpf		= nsim_bpf,
+	.ndo_hwtstamp_get	= nsim_hwtstamp_get,
+	.ndo_hwtstamp_set	= nsim_hwtstamp_get,
 };
 
 static const struct net_device_ops nsim_vf_netdev_ops = {
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 7d8ed8d8df5c..c6efd2383552 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -102,6 +102,7 @@ struct netdevsim {
 	} udp_ports;
 
 	struct nsim_ethtool ethtool;
+	struct hwtstamp_config hw_tstamp_config;
 };
 
 struct netdevsim *
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c8c634091a65..078c9284930a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -57,6 +57,8 @@
 struct netpoll_info;
 struct device;
 struct ethtool_ops;
+struct hwtstamp_config;
+struct kernel_hwtstamp_config;
 struct phy_device;
 struct dsa_port;
 struct ip_tunnel_parm;
@@ -1411,6 +1413,17 @@ struct netdev_net_notifier {
  *	Get hardware timestamp based on normal/adjustable time or free running
  *	cycle counter. This function is required if physical clock supports a
  *	free running cycle counter.
+ *	int (*ndo_hwtstamp_get)(struct net_device *dev,
+ *				struct hwtstamp_config *config,
+ *				struct kernel_hwtstamp_config *kernel_config,
+ *				struct netlink_ext_ack *extack);
+ *	Get hardware timestamping parameters currently configured  for NIC
+ *	device.
+ *	int (*ndo_hwtstamp_set)(struct net_device *dev,
+ *				struct hwtstamp_config *config,
+ *				struct kernel_hwtstamp_config *kernel_config,
+ *				struct netlink_ext_ack *extack);
+ *	Set hardware timestamping parameters for NIC device.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1645,6 +1658,14 @@ struct net_device_ops {
 	ktime_t			(*ndo_get_tstamp)(struct net_device *dev,
 						  const struct skb_shared_hwtstamps *hwtstamps,
 						  bool cycles);
+	int			(*ndo_hwtstamp_get)(struct net_device *dev,
+						    struct hwtstamp_config *config,
+						    struct kernel_hwtstamp_config *kernel_config,
+						    struct netlink_ext_ack *extack);
+	int			(*ndo_hwtstamp_set)(struct net_device *dev,
+						    struct hwtstamp_config *config,
+						    struct kernel_hwtstamp_config *kernel_config,
+						    struct netlink_ext_ack *extack);
 };
 
 struct xdp_metadata_ops {
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index a2c66b3d7f0f..547f73beb479 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -79,6 +79,21 @@ struct hwtstamp_config {
 	int rx_filter;
 };
 
+/**
+ * struct kernel_hwtstamp_config - %SIOCGHWTSTAMP and %SIOCSHWTSTAMP parameter
+ *
+ * @dummy:	a placeholder field added to work around empty struct language
+ *		restriction
+ *
+ * This structure passed as a parameter to NDO methods called in
+ * response to SIOCGHWTSTAMP and SIOCSHWTSTAMP IOCTLs.
+ * The structure is effectively empty for now. Before adding new fields
+ * to the structure "dummy" placeholder field should be removed.
+ */
+struct kernel_hwtstamp_config {
+	u8 dummy;
+};
+
 /* possible values for hwtstamp_config->flags */
 enum hwtstamp_flags {
 	/*
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 846669426236..c145afb3f77e 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -183,22 +183,18 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
 	return err;
 }
 
-static int net_hwtstamp_validate(struct ifreq *ifr)
+static int net_hwtstamp_validate(struct hwtstamp_config *cfg)
 {
-	struct hwtstamp_config cfg;
 	enum hwtstamp_tx_types tx_type;
 	enum hwtstamp_rx_filters rx_filter;
 	int tx_type_valid = 0;
 	int rx_filter_valid = 0;
 
-	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-		return -EFAULT;
-
-	if (cfg.flags & ~HWTSTAMP_FLAG_MASK)
+	if (cfg->flags & ~HWTSTAMP_FLAG_MASK)
 		return -EINVAL;
 
-	tx_type = cfg.tx_type;
-	rx_filter = cfg.rx_filter;
+	tx_type = cfg->tx_type;
+	rx_filter = cfg->rx_filter;
 
 	switch (tx_type) {
 	case HWTSTAMP_TX_OFF:
@@ -277,6 +273,63 @@ static int dev_siocbond(struct net_device *dev,
 	return -EOPNOTSUPP;
 }
 
+static int dev_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct hwtstamp_config config;
+	int err;
+
+	err = dsa_ndo_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
+	if (err == 0 || err != -EOPNOTSUPP)
+		return err;
+
+	if (!ops->ndo_hwtstamp_get)
+		return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	err = ops->ndo_hwtstamp_get(dev, &config, NULL, NULL);
+	if (err)
+		return err;
+
+	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
+		return -EFAULT;
+	return 0;
+}
+
+static int dev_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct hwtstamp_config config;
+	int err;
+
+	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	err = net_hwtstamp_validate(&config);
+	if (err)
+		return err;
+
+	err = dsa_ndo_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
+	if (err == 0 || err != -EOPNOTSUPP)
+		return err;
+
+	if (!ops->ndo_hwtstamp_set)
+		return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	err = ops->ndo_hwtstamp_set(dev, &config, NULL, NULL);
+	if (err)
+		return err;
+
+	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
+		return -EFAULT;
+	return 0;
+}
+
 static int dev_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
 			      void __user *data, unsigned int cmd)
 {
@@ -391,11 +444,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
 		rtnl_lock();
 		return err;
 
+	case SIOCGHWTSTAMP:
+		return dev_hwtstamp_get(dev, ifr);
+
 	case SIOCSHWTSTAMP:
-		err = net_hwtstamp_validate(ifr);
-		if (err)
-			return err;
-		fallthrough;
+		return dev_hwtstamp_set(dev, ifr);
 
 	/*
 	 *	Unknown or private ioctl
@@ -407,9 +460,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 ||
-- 
2.39.2

Powered by blists - more mailing lists