[RFC PATCH net-next v6 0/5] New NDO methods ndo_hwtstamp_get/set

[RFC PATCH net-next v6 0/5] New NDO methods ndo_hwtstamp_get/set

[Maxim Georgiev]

This stack of patches introduces a couple of new NDO methods,
ndo_hwtstamp_get and ndo_hwtstamp_set. These new methods can be
implemented by NIC drivers to allow setting and querying HW
timestamp settings. Drivers implementing these methods will
not need to handle SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs.
The new NDO methods will handle copying request parameters
between user address space and kernel space.

Maxim Georgiev (5):
  net: Add NDOs for hardware timestamp get/set
  net: Add ifreq pointer field to kernel_hwtstamp_config structure
  vlan/macvlan: Add ndo_hwtstamp_get/set support to vlan/macvlan code
    path
  bond: Add ndo_hwtstamp_get/set support to bond driver
  netdevsim: Implement ndo_hwtstamp_get/set methods in netdevsim driver

 drivers/net/bonding/bond_main.c   | 106 +++++++++++++++++-----------
 drivers/net/macvlan.c             |  35 ++++------
 drivers/net/netdevsim/ethtool.c   |  11 +++
 drivers/net/netdevsim/netdev.c    |  24 +++++++
 drivers/net/netdevsim/netdevsim.h |   1 +
 include/linux/net_tstamp.h        |  17 +++++
 include/linux/netdevice.h         |  22 ++++++
 net/8021q/vlan_dev.c              |  28 ++++++--
 net/core/dev_ioctl.c              | 110 +++++++++++++++++++++++++++++-
 9 files changed, 286 insertions(+), 68 deletions(-)

-- 
2.40.1

[RFC PATCH net-next v6 1/5] net: Add NDOs for hardware timestamp get/set

[Maxim Georgiev]

Current NIC driver API demands drivers supporting hardware timestamping
to implement handling logic for 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.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Maxim Georgiev <glipus@gmail.com>
---
Notes:
  Changes in v6:
  - The patch title was updated. No code changes.
  Changes in v4:
  - Renamed hwtstamp_kernel_to_config() function to
    hwtstamp_config_from_kernel().
  - Added struct kernel_hwtstamp_config zero initialization
    in dev_get_hwtstamp() and in dev_get_hwtstamp().

Changes in v3:
- Moved individual driver conversions to separate patches

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
---
 include/linux/net_tstamp.h |  8 ++++++++
 include/linux/netdevice.h  | 16 +++++++++++++++
 net/core/dev_ioctl.c       | 42 +++++++++++++++++++++++++++++++++++---
 3 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
index fd67f3cc0c4b..7c59824f43f5 100644
--- a/include/linux/net_tstamp.h
+++ b/include/linux/net_tstamp.h
@@ -30,4 +30,12 @@ static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kern
 	kernel_cfg->rx_filter = cfg->rx_filter;
 }
 
+static inline void hwtstamp_config_from_kernel(struct hwtstamp_config *cfg,
+					       const struct kernel_hwtstamp_config *kernel_cfg)
+{
+	cfg->flags = kernel_cfg->flags;
+	cfg->tx_type = kernel_cfg->tx_type;
+	cfg->rx_filter = kernel_cfg->rx_filter;
+}
+
 #endif /* _LINUX_NET_TIMESTAMPING_H_ */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 08fbd4622ccf..7160135ca540 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -57,6 +57,7 @@
 struct netpoll_info;
 struct device;
 struct ethtool_ops;
+struct kernel_hwtstamp_config;
 struct phy_device;
 struct dsa_port;
 struct ip_tunnel_parm;
@@ -1415,6 +1416,15 @@ 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 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 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);
@@ -1649,6 +1659,12 @@ 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 kernel_hwtstamp_config *kernel_config,
+						    struct netlink_ext_ack *extack);
+	int			(*ndo_hwtstamp_set)(struct net_device *dev,
+						    struct kernel_hwtstamp_config *kernel_config,
+						    struct netlink_ext_ack *extack);
 };
 
 struct xdp_metadata_ops {
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 3730945ee294..a157b9ab5237 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -254,12 +254,33 @@ static int dev_eth_ioctl(struct net_device *dev,
 
 static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 {
-	return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct kernel_hwtstamp_config kernel_cfg = {};
+	struct hwtstamp_config config;
+	int 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, &kernel_cfg, NULL);
+	if (err)
+		return err;
+
+	hwtstamp_config_from_kernel(&config, &kernel_cfg);
+
+	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
+		return -EFAULT;
+
+	return 0;
 }
 
 static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 {
-	struct kernel_hwtstamp_config kernel_cfg;
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct kernel_hwtstamp_config kernel_cfg = {};
 	struct netlink_ext_ack extack = {};
 	struct hwtstamp_config cfg;
 	int err;
@@ -280,7 +301,22 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 		return err;
 	}
 
-	return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
+	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, &kernel_cfg, NULL);
+	if (err)
+		return err;
+
+	hwtstamp_config_from_kernel(&cfg, &kernel_cfg);
+
+	if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
+		return -EFAULT;
+
+	return 0;
 }
 
 static int dev_siocbond(struct net_device *dev,
-- 
2.40.1

Re: [RFC PATCH net-next v6 1/5] net: Add NDOs for hardware timestamp get/set

[Vladimir Oltean]

On Mon, May 01, 2023 at 10:31:46PM -0600, Maxim Georgiev wrote:
>  struct net_device_ops {
>  	int			(*ndo_init)(struct net_device *dev);
> @@ -1649,6 +1659,12 @@ 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 kernel_hwtstamp_config *kernel_config,
> +						    struct netlink_ext_ack *extack);

I'm not sure it is necessary to pass an extack to "get". That should
only give a more detailed reason if the driver refuses something.

For that matter, now that ndo_hwtstamp_get() should no longer be
concerned with the copy_to_user(), I'm not even sure that it should
return int at all, and not void. The transition is going to be slightly
problematic though, with the generic_hwtstamp_get_lower() necessarily
still calling dev_eth_ioctl() -> copy_to_user(), so we probably can't
make it void just now.

Re: [RFC PATCH net-next v6 1/5] net: Add NDOs for hardware timestamp get/set

[Max Georgiev]

On Thu, May 11, 2023 at 6:32 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Mon, May 01, 2023 at 10:31:46PM -0600, Maxim Georgiev wrote:
> >  struct net_device_ops {
> >       int                     (*ndo_init)(struct net_device *dev);
> > @@ -1649,6 +1659,12 @@ 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 kernel_hwtstamp_config *kernel_config,
> > +                                                 struct netlink_ext_ack *extack);
>
> I'm not sure it is necessary to pass an extack to "get". That should
> only give a more detailed reason if the driver refuses something.

I have to admit I just followed Jakub's guidance adding "extack" to the
list of parametres. I'll let Jakub to comment on that.

>
> For that matter, now that ndo_hwtstamp_get() should no longer be
> concerned with the copy_to_user(), I'm not even sure that it should
> return int at all, and not void. The transition is going to be slightly
> problematic though, with the generic_hwtstamp_get_lower() necessarily
> still calling dev_eth_ioctl() -> copy_to_user(), so we probably can't
> make it void just now.

Would it make sense to keep the return int value here "for future extensions"?

Re: [RFC PATCH net-next v6 1/5] net: Add NDOs for hardware timestamp get/set

[Jakub Kicinski]

On Thu, 11 May 2023 21:22:23 -0600 Max Georgiev wrote:
> > > +     int                     (*ndo_hwtstamp_get)(struct net_device *dev,
> > > +                                                 struct kernel_hwtstamp_config *kernel_config,
> > > +                                                 struct netlink_ext_ack *extack);  
> >
> > I'm not sure it is necessary to pass an extack to "get". That should
> > only give a more detailed reason if the driver refuses something.  
> 
> I have to admit I just followed Jakub's guidance adding "extack" to the
> list of parametres. I'll let Jakub to comment on that.

We can drop it from get, I can't think of any strong use case.

Re: [RFC PATCH net-next v6 1/5] net: Add NDOs for hardware timestamp get/set

[Max Georgiev]

On Fri, May 12, 2023 at 11:41 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 11 May 2023 21:22:23 -0600 Max Georgiev wrote:
> > > > +     int                     (*ndo_hwtstamp_get)(struct net_device *dev,
> > > > +                                                 struct kernel_hwtstamp_config *kernel_config,
> > > > +                                                 struct netlink_ext_ack *extack);
> > >
> > > I'm not sure it is necessary to pass an extack to "get". That should
> > > only give a more detailed reason if the driver refuses something.
> >
> > I have to admit I just followed Jakub's guidance adding "extack" to the
> > list of parametres. I'll let Jakub to comment on that.
>
> We can drop it from get, I can't think of any strong use case.

Got it. Will remove the  "extack" parameter from ndo_hwtstamp_get() in the
next revision.

[RFC PATCH net-next v6 2/5] net: Add ifreq pointer field to kernel_hwtstamp_config structure

[Maxim Georgiev]

Considering the stackable nature of drivers there will be situations
where a driver implementing ndo_hwtstamp_get/set functions will have
to translate requests back to SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs
to pass them to lower level drivers that do not provide
ndo_hwtstamp_get/set callbacks. To simplify request translation in
such scenarios let's include a pointer to the original struct ifreq
to kernel_hwtstamp_config structure.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Maxim Georgiev <glipus@gmail.com>

Notes:
  Changes in v6:
  - Patch title was updated. No code changes.
  Changes in v5:
  - kernel_hwtstamp_config kdoc is updated with the new field
    descriptions.
  Changes in V4:
  - Introducing KERNEL_HWTSTAMP_FLAG_IFR_RESULT flag indicating that
    the operation results are returned in the ifr referred by
    struct kernel_hwtstamp_config instead of kernel_hwtstamp_config
    glags/tx_type/rx_filter fields.
  - Implementing generic_hwtstamp_set/set_lower() functions
    which will be used by vlan, maxvlan, bond and potentially
    other drivers translating ndo_hwtstamp_set/set calls to
    lower level drivers.
---
 include/linux/net_tstamp.h |  9 +++++
 include/linux/netdevice.h  |  6 +++
 net/core/dev_ioctl.c       | 80 +++++++++++++++++++++++++++++++++++---
 3 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
index 7c59824f43f5..91d2738bf0a0 100644
--- a/include/linux/net_tstamp.h
+++ b/include/linux/net_tstamp.h
@@ -11,6 +11,8 @@
  * @flags: see struct hwtstamp_config
  * @tx_type: see struct hwtstamp_config
  * @rx_filter: see struct hwtstamp_config
+ * @ifr: pointer to ifreq structure from the original IOCTL request
+ * @kernel_flags: possible flags defined by kernel_hwtstamp_flags below
  *
  * Prefer using this structure for in-kernel processing of hardware
  * timestamping configuration, over the inextensible struct hwtstamp_config
@@ -20,6 +22,13 @@ struct kernel_hwtstamp_config {
 	int flags;
 	int tx_type;
 	int rx_filter;
+	struct ifreq *ifr;
+	int kernel_flags;
+};
+
+/* possible values for kernel_hwtstamp_config->kernel_flags */
+enum kernel_hwtstamp_flags {
+	KERNEL_HWTSTAMP_FLAG_IFR_RESULT = BIT(0),
 };
 
 static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kernel_cfg,
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7160135ca540..42e96b12fd21 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3942,6 +3942,12 @@ int put_user_ifreq(struct ifreq *ifr, void __user *arg);
 int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
 		void __user *data, bool *need_copyout);
 int dev_ifconf(struct net *net, struct ifconf __user *ifc);
+int generic_hwtstamp_set_lower(struct net_device *dev,
+			       struct kernel_hwtstamp_config *kernel_cfg,
+			       struct netlink_ext_ack *extack);
+int generic_hwtstamp_get_lower(struct net_device *dev,
+			       struct kernel_hwtstamp_config *kernel_cfg,
+			       struct netlink_ext_ack *extack);
 int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *userdata);
 unsigned int dev_get_flags(const struct net_device *);
 int __dev_change_flags(struct net_device *dev, unsigned int flags,
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index a157b9ab5237..da1d2391822f 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -265,14 +265,17 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 	if (!netif_device_present(dev))
 		return -ENODEV;
 
+	kernel_cfg.ifr = ifr;
 	err = ops->ndo_hwtstamp_get(dev, &kernel_cfg, NULL);
 	if (err)
 		return err;
 
-	hwtstamp_config_from_kernel(&config, &kernel_cfg);
+	if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) {
+		hwtstamp_config_from_kernel(&config, &kernel_cfg);
 
-	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
-		return -EFAULT;
+		if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
+			return -EFAULT;
+	}
 
 	return 0;
 }
@@ -289,6 +292,7 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 		return -EFAULT;
 
 	hwtstamp_config_to_kernel(&kernel_cfg, &cfg);
+	kernel_cfg.ifr = ifr;
 
 	err = net_hwtstamp_validate(&kernel_cfg);
 	if (err)
@@ -311,14 +315,78 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
 	if (err)
 		return err;
 
-	hwtstamp_config_from_kernel(&cfg, &kernel_cfg);
+	if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) {
+		hwtstamp_config_from_kernel(&cfg, &kernel_cfg);
 
-	if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
-		return -EFAULT;
+		if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
+			return -EFAULT;
+	}
 
 	return 0;
 }
 
+int generic_hwtstamp_set_lower(struct net_device *dev,
+			       struct kernel_hwtstamp_config *kernel_cfg,
+			       struct netlink_ext_ack *extack)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct ifreq ifrr;
+	int err;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	if (ops->ndo_hwtstamp_set) {
+		kernel_cfg->kernel_flags &= ~KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
+		err = ops->ndo_hwtstamp_set(dev, kernel_cfg, extack);
+		return err;
+	}
+
+	if (!kernel_cfg->ifr)
+		return -EOPNOTSUPP;
+
+	strscpy_pad(ifrr.ifr_name, dev->name, IFNAMSIZ);
+	ifrr.ifr_ifru = kernel_cfg->ifr->ifr_ifru;
+	err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP);
+	if (!err) {
+		kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru;
+		kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
+	}
+	return err;
+}
+EXPORT_SYMBOL(generic_hwtstamp_set_lower);
+
+int generic_hwtstamp_get_lower(struct net_device *dev,
+			       struct kernel_hwtstamp_config *kernel_cfg,
+			       struct netlink_ext_ack *extack)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct ifreq ifrr;
+	int err;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	if (ops->ndo_hwtstamp_get) {
+		kernel_cfg->kernel_flags &= ~KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
+		err = ops->ndo_hwtstamp_get(dev, kernel_cfg, extack);
+		return err;
+	}
+
+	if (!kernel_cfg->ifr)
+		return -EOPNOTSUPP;
+
+	strscpy_pad(ifrr.ifr_name, dev->name, IFNAMSIZ);
+	ifrr.ifr_ifru = kernel_cfg->ifr->ifr_ifru;
+	err = dev_eth_ioctl(dev, &ifrr, SIOCGHWTSTAMP);
+	if (!err) {
+		kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru;
+		kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
+	}
+	return err;
+}
+EXPORT_SYMBOL(generic_hwtstamp_get_lower);
+
 static int dev_siocbond(struct net_device *dev,
 			struct ifreq *ifr, unsigned int cmd)
 {
-- 
2.40.1

Re: [RFC PATCH net-next v6 2/5] net: Add ifreq pointer field to kernel_hwtstamp_config structure

[Jakub Kicinski]

On Mon,  1 May 2023 22:31:47 -0600 Maxim Georgiev wrote:
> +	err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP);
> +	if (!err) {
> +		kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru;
> +		kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
> +	}
> +	return err;

nit: I think we should stick to the normal flow even if it costs 
a few extra lines:

	err = dev_eth_ioctl(..
	if (err)
		return err;

	kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru;
	kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT;

	return 0;


Other than that patches LGTM :)

Re: [RFC PATCH net-next v6 2/5] net: Add ifreq pointer field to kernel_hwtstamp_config structure

[Max Georgiev]

On Wed, May 3, 2023 at 9:05 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon,  1 May 2023 22:31:47 -0600 Maxim Georgiev wrote:
> > +     err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP);
> > +     if (!err) {
> > +             kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru;
> > +             kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
> > +     }
> > +     return err;
>
> nit: I think we should stick to the normal flow even if it costs
> a few extra lines:
>
>         err = dev_eth_ioctl(..
>         if (err)
>                 return err;
>
>         kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru;
>         kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
>
>         return 0;
>
>
> Other than that patches LGTM :)

Got it, wil update both generic_hwtstamp_get_lower() and
generic_hwtstamp_set_lower().

What would be the best practice with updating a single patch in a
stack (or a couple of
patches in a stack)? Should I resend only the updated patch(es), or
should I increment the
patch stack revision and resend all the parches?

Re: [RFC PATCH net-next v6 2/5] net: Add ifreq pointer field to kernel_hwtstamp_config structure

[Jakub Kicinski]

On Thu, 4 May 2023 09:21:42 -0600 Max Georgiev wrote:
> Got it, wil update both generic_hwtstamp_get_lower() and
> generic_hwtstamp_set_lower().
> 
> What would be the best practice with updating a single patch in a
> stack (or a couple of
> patches in a stack)? Should I resend only the updated patch(es), or
> should I increment the
> patch stack revision and resend all the parches?

You'll need to resend all, but this is minor enough that, unless
there's more comments, I'd just wait until Monday and send non-RFC 
at that point (with "a" driver conversion included).

Re: [RFC PATCH net-next v6 2/5] net: Add ifreq pointer field to kernel_hwtstamp_config structure

[Vladimir Oltean]

On Mon, May 01, 2023 at 10:31:47PM -0600, Maxim Georgiev wrote:
> Considering the stackable nature of drivers there will be situations
> where a driver implementing ndo_hwtstamp_get/set functions will have
> to translate requests back to SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs
> to pass them to lower level drivers that do not provide
> ndo_hwtstamp_get/set callbacks. To simplify request translation in
> such scenarios let's include a pointer to the original struct ifreq
> to kernel_hwtstamp_config structure.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Maxim Georgiev <glipus@gmail.com>
> 
> Notes:
>   Changes in v6:
>   - Patch title was updated. No code changes.
>   Changes in v5:
>   - kernel_hwtstamp_config kdoc is updated with the new field
>     descriptions.
>   Changes in V4:
>   - Introducing KERNEL_HWTSTAMP_FLAG_IFR_RESULT flag indicating that
>     the operation results are returned in the ifr referred by
>     struct kernel_hwtstamp_config instead of kernel_hwtstamp_config
>     glags/tx_type/rx_filter fields.
>   - Implementing generic_hwtstamp_set/set_lower() functions
>     which will be used by vlan, maxvlan, bond and potentially
>     other drivers translating ndo_hwtstamp_set/set calls to
>     lower level drivers.

Usually the notes go below the "---" line so that they are stripped once
the patch gets applied, but are otherwise visible to reviewers. It's
good having them nonetheless.

> ---
>  include/linux/net_tstamp.h |  9 +++++
>  include/linux/netdevice.h  |  6 +++
>  net/core/dev_ioctl.c       | 80 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
> index 7c59824f43f5..91d2738bf0a0 100644
> --- a/include/linux/net_tstamp.h
> +++ b/include/linux/net_tstamp.h
> @@ -11,6 +11,8 @@
>   * @flags: see struct hwtstamp_config
>   * @tx_type: see struct hwtstamp_config
>   * @rx_filter: see struct hwtstamp_config
> + * @ifr: pointer to ifreq structure from the original IOCTL request
> + * @kernel_flags: possible flags defined by kernel_hwtstamp_flags below
>   *
>   * Prefer using this structure for in-kernel processing of hardware
>   * timestamping configuration, over the inextensible struct hwtstamp_config
> @@ -20,6 +22,13 @@ struct kernel_hwtstamp_config {
>  	int flags;
>  	int tx_type;
>  	int rx_filter;
> +	struct ifreq *ifr;
> +	int kernel_flags;
> +};
> +
> +/* possible values for kernel_hwtstamp_config->kernel_flags */
> +enum kernel_hwtstamp_flags {
> +	KERNEL_HWTSTAMP_FLAG_IFR_RESULT = BIT(0),
>  };

I think "kernel_flags" and KERNEL_HWTSTAMP_FLAG_IFR_RESULT are slightly
overengineered and poorly named (I couldn't understand yesterday what
they do, tried again today, finally did).

I believe that a single "bool copied_to_user" would do the trick just
fine and also be more self-explanatory?

>  
>  static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kernel_cfg,
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7160135ca540..42e96b12fd21 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3942,6 +3942,12 @@ int put_user_ifreq(struct ifreq *ifr, void __user *arg);
>  int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
>  		void __user *data, bool *need_copyout);
>  int dev_ifconf(struct net *net, struct ifconf __user *ifc);
> +int generic_hwtstamp_set_lower(struct net_device *dev,
> +			       struct kernel_hwtstamp_config *kernel_cfg,
> +			       struct netlink_ext_ack *extack);
> +int generic_hwtstamp_get_lower(struct net_device *dev,
> +			       struct kernel_hwtstamp_config *kernel_cfg,
> +			       struct netlink_ext_ack *extack);
>  int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *userdata);
>  unsigned int dev_get_flags(const struct net_device *);
>  int __dev_change_flags(struct net_device *dev, unsigned int flags,
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index a157b9ab5237..da1d2391822f 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -265,14 +265,17 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  	if (!netif_device_present(dev))
>  		return -ENODEV;
>  
> +	kernel_cfg.ifr = ifr;
>  	err = ops->ndo_hwtstamp_get(dev, &kernel_cfg, NULL);
>  	if (err)
>  		return err;
>  
> -	hwtstamp_config_from_kernel(&config, &kernel_cfg);
> +	if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) {
> +		hwtstamp_config_from_kernel(&config, &kernel_cfg);
>  
> -	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> -		return -EFAULT;
> +		if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> +			return -EFAULT;
> +	}
>  
>  	return 0;
>  }
> @@ -289,6 +292,7 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  		return -EFAULT;
>  
>  	hwtstamp_config_to_kernel(&kernel_cfg, &cfg);
> +	kernel_cfg.ifr = ifr;
>  
>  	err = net_hwtstamp_validate(&kernel_cfg);
>  	if (err)
> @@ -311,14 +315,78 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  	if (err)
>  		return err;
>  
> -	hwtstamp_config_from_kernel(&cfg, &kernel_cfg);
> +	if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) {
> +		hwtstamp_config_from_kernel(&cfg, &kernel_cfg);
>  
> -	if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
> -		return -EFAULT;
> +		if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
> +			return -EFAULT;
> +	}
>  
>  	return 0;
>  }
>  
> +int generic_hwtstamp_set_lower(struct net_device *dev,
> +			       struct kernel_hwtstamp_config *kernel_cfg,
> +			       struct netlink_ext_ack *extack)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct ifreq ifrr;
> +	int err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	if (ops->ndo_hwtstamp_set) {
> +		kernel_cfg->kernel_flags &= ~KERNEL_HWTSTAMP_FLAG_IFR_RESULT;

I don't think you ever need to unset this flag, just set it? The structure
is initialized in dev_get_hwtstamp() and dev_set_hwtstamp() as:

	struct kernel_hwtstamp_config kernel_cfg = {};

which zero-fills it.

> +		err = ops->ndo_hwtstamp_set(dev, kernel_cfg, extack);
> +		return err;

		return ops->ndo_hwtstamp_set(...)

i.e. skip the "err" assignment

> +	}
> +
> +	if (!kernel_cfg->ifr)
> +		return -EOPNOTSUPP;
> +
> +	strscpy_pad(ifrr.ifr_name, dev->name, IFNAMSIZ);
> +	ifrr.ifr_ifru = kernel_cfg->ifr->ifr_ifru;
> +	err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP);
> +	if (!err) {
> +		kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru;
> +		kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
> +	}
> +	return err;
> +}
> +EXPORT_SYMBOL(generic_hwtstamp_set_lower);

EXPORT_SYMBOL_GPL()? We expect this to be used by core network stack
(vlan, macvlan, bonding), not by vendor drivers.

Same comments for "set".

[RFC PATCH net-next v6 3/5] vlan/macvlan: Add ndo_hwtstamp_get/set support to vlan/macvlan code path

[Maxim Georgiev]

This patch makes VLAN and MACVLAN drivers to use the newly
introduced ndo_hwtstamp_get/set API to pass hw timestamp
requests to underlying NIC drivers in case if these drivers
implement ndo_hwtstamp_get/set functions. Otherwise VLAN
subsystems falls back to calling ndo_eth_ioctl.

Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Maxim Georgiev <glipus@gmail.com>
---
Notes:
  Changes in v6:
  - Patch title was updated. No code changes.
  Changes in v5:
  - Re-introduced the net namespace check which
    was dropped in v4.
  Changes in v4:
  - Moved hw timestamp get/set request processing logic
    from vlan_dev_ioctl() to .ndo_hwtstamp_get/set callbacks.
  - Use the shared generic_hwtstamp_get/set_lower() functions
    to handle ndo_hwtstamp_get/set requests.
  - Applay the same changes to macvlan driver.
---
 drivers/net/macvlan.c | 35 +++++++++++++++--------------------
 net/8021q/vlan_dev.c  | 28 +++++++++++++++++++++++-----
 2 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 4a53debf9d7c..58515c9fdf49 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -868,31 +868,25 @@ static int macvlan_change_mtu(struct net_device *dev, int new_mtu)
 	return 0;
 }
 
-static int macvlan_eth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+static int macvlan_hwtstamp_get(struct net_device *dev,
+				struct kernel_hwtstamp_config *cfg,
+				struct netlink_ext_ack *extack)
 {
 	struct net_device *real_dev = macvlan_dev_real_dev(dev);
-	const struct net_device_ops *ops = real_dev->netdev_ops;
-	struct ifreq ifrr;
-	int err = -EOPNOTSUPP;
 
-	strscpy(ifrr.ifr_name, real_dev->name, IFNAMSIZ);
-	ifrr.ifr_ifru = ifr->ifr_ifru;
+	return generic_hwtstamp_get_lower(real_dev, cfg, extack);
+}
 
-	switch (cmd) {
-	case SIOCSHWTSTAMP:
-		if (!net_eq(dev_net(dev), &init_net))
-			break;
-		fallthrough;
-	case SIOCGHWTSTAMP:
-		if (netif_device_present(real_dev) && ops->ndo_eth_ioctl)
-			err = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
-		break;
-	}
+static int macvlan_hwtstamp_set(struct net_device *dev,
+				struct kernel_hwtstamp_config *cfg,
+				struct netlink_ext_ack *extack)
+{
+	struct net_device *real_dev = macvlan_dev_real_dev(dev);
 
-	if (!err)
-		ifr->ifr_ifru = ifrr.ifr_ifru;
+	if (!net_eq(dev_net(dev), &init_net))
+		return -EOPNOTSUPP;
 
-	return err;
+	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
 }
 
 /*
@@ -1193,7 +1187,6 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.ndo_stop		= macvlan_stop,
 	.ndo_start_xmit		= macvlan_start_xmit,
 	.ndo_change_mtu		= macvlan_change_mtu,
-	.ndo_eth_ioctl		= macvlan_eth_ioctl,
 	.ndo_fix_features	= macvlan_fix_features,
 	.ndo_change_rx_flags	= macvlan_change_rx_flags,
 	.ndo_set_mac_address	= macvlan_set_mac_address,
@@ -1212,6 +1205,8 @@ static const struct net_device_ops macvlan_netdev_ops = {
 #endif
 	.ndo_get_iflink		= macvlan_dev_get_iflink,
 	.ndo_features_check	= passthru_features_check,
+	.ndo_hwtstamp_get	= macvlan_hwtstamp_get,
+	.ndo_hwtstamp_set	= macvlan_hwtstamp_set,
 };
 
 static void macvlan_dev_free(struct net_device *dev)
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 870e4935d6e6..66423eaad84d 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -354,6 +354,27 @@ static int vlan_dev_set_mac_address(struct net_device *dev, void *p)
 	return 0;
 }
 
+static int vlan_hwtstamp_get(struct net_device *dev,
+			     struct kernel_hwtstamp_config *cfg,
+			     struct netlink_ext_ack *extack)
+{
+	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
+
+	return generic_hwtstamp_get_lower(real_dev, cfg, extack);
+}
+
+static int vlan_hwtstamp_set(struct net_device *dev,
+			     struct kernel_hwtstamp_config *cfg,
+			     struct netlink_ext_ack *extack)
+{
+	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
+
+	if (!net_eq(dev_net(dev), &init_net))
+		return -EOPNOTSUPP;
+
+	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
+}
+
 static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
@@ -365,14 +386,9 @@ static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	ifrr.ifr_ifru = ifr->ifr_ifru;
 
 	switch (cmd) {
-	case SIOCSHWTSTAMP:
-		if (!net_eq(dev_net(dev), dev_net(real_dev)))
-			break;
-		fallthrough;
 	case SIOCGMIIPHY:
 	case SIOCGMIIREG:
 	case SIOCSMIIREG:
-	case SIOCGHWTSTAMP:
 		if (netif_device_present(real_dev) && ops->ndo_eth_ioctl)
 			err = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
 		break;
@@ -1081,6 +1097,8 @@ static const struct net_device_ops vlan_netdev_ops = {
 	.ndo_fix_features	= vlan_dev_fix_features,
 	.ndo_get_iflink		= vlan_dev_get_iflink,
 	.ndo_fill_forward_path	= vlan_dev_fill_forward_path,
+	.ndo_hwtstamp_get	= vlan_hwtstamp_get,
+	.ndo_hwtstamp_set	= vlan_hwtstamp_set,
 };
 
 static void vlan_dev_free(struct net_device *dev)
-- 
2.40.1

[RFC PATCH net-next v6 4/5] bond: Add ndo_hwtstamp_get/set support to bond driver

[Maxim Georgiev]

This patch changes bonding net driver to use the newly
introduced ndo_hwtstamp_get/set API to pass hw timestamp
requests to underlying NIC drivers in case if these drivers
implement ndo_hwtstamp_get/set functions. Otherwise Bonding
subsystem falls back to calling ndo_eth_ioctl.

Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Maxim Georgiev <glipus@gmail.com>
---
Notes:
  Changes in v6:
  - Patch title was updated. No code changes.
---
 drivers/net/bonding/bond_main.c | 106 ++++++++++++++++++++------------
 1 file changed, 66 insertions(+), 40 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 710548dbd0c1..21969afff2a9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4408,11 +4408,6 @@ static int bond_eth_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cm
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct mii_ioctl_data *mii = NULL;
-	const struct net_device_ops *ops;
-	struct net_device *real_dev;
-	struct hwtstamp_config cfg;
-	struct ifreq ifrr;
-	int res = 0;
 
 	netdev_dbg(bond_dev, "bond_eth_ioctl: cmd=%d\n", cmd);
 
@@ -4439,44 +4434,11 @@ static int bond_eth_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cm
 		}
 
 		break;
-	case SIOCSHWTSTAMP:
-		if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-			return -EFAULT;
-
-		if (!(cfg.flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX))
-			return -EOPNOTSUPP;
-
-		fallthrough;
-	case SIOCGHWTSTAMP:
-		real_dev = bond_option_active_slave_get_rcu(bond);
-		if (!real_dev)
-			return -EOPNOTSUPP;
-
-		strscpy_pad(ifrr.ifr_name, real_dev->name, IFNAMSIZ);
-		ifrr.ifr_ifru = ifr->ifr_ifru;
-
-		ops = real_dev->netdev_ops;
-		if (netif_device_present(real_dev) && ops->ndo_eth_ioctl) {
-			res = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
-			if (res)
-				return res;
-
-			ifr->ifr_ifru = ifrr.ifr_ifru;
-			if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-				return -EFAULT;
-
-			/* Set the BOND_PHC_INDEX flag to notify user space */
-			cfg.flags |= HWTSTAMP_FLAG_BONDED_PHC_INDEX;
-
-			return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ?
-				-EFAULT : 0;
-		}
-		fallthrough;
 	default:
-		res = -EOPNOTSUPP;
+		return -EOPNOTSUPP;
 	}
 
-	return res;
+	return 0;
 }
 
 static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd)
@@ -5650,6 +5612,68 @@ static u32 bond_mode_bcast_speed(struct slave *slave, u32 speed)
 	return speed;
 }
 
+static int bond_set_phc_index_flag(struct kernel_hwtstamp_config *kernel_cfg)
+{
+	struct ifreq *ifr = kernel_cfg->ifr;
+	struct hwtstamp_config cfg;
+
+	if (kernel_cfg->kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT) {
+		if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
+			return -EFAULT;
+
+		cfg.flags |= HWTSTAMP_FLAG_BONDED_PHC_INDEX;
+		if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
+			return -EFAULT;
+	} else {
+		kernel_cfg->flags |= HWTSTAMP_FLAG_BONDED_PHC_INDEX;
+	}
+
+	return 0;
+}
+
+static int bond_hwtstamp_get(struct net_device *dev,
+			     struct kernel_hwtstamp_config *cfg,
+			     struct netlink_ext_ack *extack)
+{
+	struct bonding *bond = netdev_priv(dev);
+	struct net_device *real_dev;
+	int err;
+
+	real_dev = bond_option_active_slave_get_rcu(bond);
+	if (!real_dev)
+		return -EOPNOTSUPP;
+
+	err = generic_hwtstamp_get_lower(real_dev, cfg, extack);
+	if (err)
+		return err;
+
+	/* Set the BOND_PHC_INDEX flag to notify user space */
+	return bond_set_phc_index_flag(cfg);
+}
+
+static int bond_hwtstamp_set(struct net_device *dev,
+			     struct kernel_hwtstamp_config *kernel_cfg,
+			     struct netlink_ext_ack *extack)
+{
+	struct bonding *bond = netdev_priv(dev);
+	struct net_device *real_dev;
+	int err;
+
+	if (!(kernel_cfg->flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX))
+		return -EOPNOTSUPP;
+
+	real_dev = bond_option_active_slave_get_rcu(bond);
+	if (!real_dev)
+		return -EOPNOTSUPP;
+
+	err = generic_hwtstamp_set_lower(real_dev, kernel_cfg, extack);
+	if (err)
+		return err;
+
+	/* Set the BOND_PHC_INDEX flag to notify user space */
+	return bond_set_phc_index_flag(kernel_cfg);
+}
+
 static int bond_ethtool_get_link_ksettings(struct net_device *bond_dev,
 					   struct ethtool_link_ksettings *cmd)
 {
@@ -5798,6 +5822,8 @@ static const struct net_device_ops bond_netdev_ops = {
 	.ndo_bpf		= bond_xdp,
 	.ndo_xdp_xmit           = bond_xdp_xmit,
 	.ndo_xdp_get_xmit_slave = bond_xdp_get_xmit_slave,
+	.ndo_hwtstamp_get	= bond_hwtstamp_get,
+	.ndo_hwtstamp_set	= bond_hwtstamp_set,
 };
 
 static const struct device_type bond_type = {
-- 
2.40.1

[RFC PATCH net-next v6 5/5] netdevsim: Implement ndo_hwtstamp_get/set methods in netdevsim driver

[Maxim Georgiev]

Implementing ndo_hwtstamp_get/set methods in netdevsim driver
to use the newly introduced ndo_hwtstamp_get/setmake it respond to
 SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs.
 Also adding .get_ts_info ethtool method allowing to monitor
 HW timestamp configuration values set using SIOCSHWTSTAMP·IOCTL.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Maxim Georgiev <glipus@gmail.com>
---

Notes:
  Changes in v6:
  - Patch title was updated. No code changes.
  Changes in V4:
  - Implemented .get_ts_info·ethtool·method.
  - Tested the patch using  hwstamp_ctl and ethtool:

[root@centosvm kernel-net-next]# ethtool -T eni0np1
Time stamping parameters for eni0np1:
Capabilities:
        software-transmit
        software-receive
        software-system-clock
PTP Hardware Clock: none
Hardware Transmit Timestamp Modes: none
Hardware Receive Filter Modes: none
[root@centosvm kernel-net-next]# hwstamp_ctl -i eni0np1 -t 1 -r 0
current settings:
tx_type 0
rx_filter 0
new settings:
tx_type 1
rx_filter 0
[root@centosvm kernel-net-next]# ethtool -T eni0np1
Time stamping parameters for eni0np1:
Capabilities:
        software-transmit
        software-receive
        software-system-clock
PTP Hardware Clock: none
Hardware Transmit Timestamp Modes:
        off
Hardware Receive Filter Modes: none
[root@centosvm kernel-net-next]# hwstamp_ctl -i eni0np1 -t 1 -r 14
current settings:
tx_type 1
rx_filter 0
new settings:
tx_type 1
rx_filter 14
[root@centosvm kernel-net-next]# ethtool -T eni0np1
Time stamping parameters for eni0np1:
Capabilities:
        software-transmit
        software-receive
        software-system-clock
PTP Hardware Clock: none
Hardware Transmit Timestamp Modes:
        off
Hardware Receive Filter Modes:
        all
        some
        ptpv1-l4-event
---
 drivers/net/netdevsim/ethtool.c   | 11 +++++++++++
 drivers/net/netdevsim/netdev.c    | 24 ++++++++++++++++++++++++
 drivers/net/netdevsim/netdevsim.h |  1 +
 3 files changed, 36 insertions(+)

diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index ffd9f84b6644..cbb8e261b759 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -140,6 +140,16 @@ nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
 	return 0;
 }
 
+static int nsim_get_ts_info(struct net_device *netdev,
+			    struct ethtool_ts_info *info)
+{
+	struct netdevsim *ns = netdev_priv(netdev);
+
+	info->tx_types = ns->hw_tstamp_config.tx_type;
+	info->rx_filters = ns->hw_tstamp_config.rx_filter;
+	return ethtool_op_get_ts_info(netdev, info);
+}
+
 static const struct ethtool_ops nsim_ethtool_ops = {
 	.supported_coalesce_params	= ETHTOOL_COALESCE_ALL_PARAMS,
 	.get_pause_stats	        = nsim_get_pause_stats,
@@ -153,6 +163,7 @@ static const struct ethtool_ops nsim_ethtool_ops = {
 	.set_channels			= nsim_set_channels,
 	.get_fecparam			= nsim_get_fecparam,
 	.set_fecparam			= nsim_set_fecparam,
+	.get_ts_info			= nsim_get_ts_info,
 };
 
 static void nsim_ethtool_ring_init(struct netdevsim *ns)
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 35fa1ca98671..6c29dfa3bb4e 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -238,6 +238,28 @@ nsim_set_features(struct net_device *dev, netdev_features_t features)
 	return 0;
 }
 
+static int
+nsim_hwtstamp_get(struct net_device *dev,
+		  struct kernel_hwtstamp_config *kernel_config,
+		  struct netlink_ext_ack *extack)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	*kernel_config = ns->hw_tstamp_config;
+	return 0;
+}
+
+static int
+nsim_hwtstamp_set(struct net_device *dev,
+		  struct kernel_hwtstamp_config *kernel_config,
+		  struct netlink_ext_ack *extack)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	ns->hw_tstamp_config = *kernel_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 +278,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_set,
 };
 
 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..e78e88a0baa1 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 kernel_hwtstamp_config hw_tstamp_config;
 };
 
 struct netdevsim *
-- 
2.40.1

Re: [RFC PATCH net-next v6 0/5] New NDO methods ndo_hwtstamp_get/set

[Jakub Kicinski]

On Mon,  1 May 2023 22:31:45 -0600 Maxim Georgiev wrote:
> This stack of patches introduces a couple of new NDO methods,
> ndo_hwtstamp_get and ndo_hwtstamp_set. These new methods can be
> implemented by NIC drivers to allow setting and querying HW
> timestamp settings. Drivers implementing these methods will
> not need to handle SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs.
> The new NDO methods will handle copying request parameters
> between user address space and kernel space.

Maxim, any ETA on the next version? Should we let someone take over?
It's been over a month since v6 posting.

Re: [RFC PATCH net-next v6 0/5] New NDO methods ndo_hwtstamp_get/set

[Vladimir Oltean]

On Tue, Jun 20, 2023 at 07:23:13PM -0700, Jakub Kicinski wrote:
> On Mon,  1 May 2023 22:31:45 -0600 Maxim Georgiev wrote:
> > This stack of patches introduces a couple of new NDO methods,
> > ndo_hwtstamp_get and ndo_hwtstamp_set. These new methods can be
> > implemented by NIC drivers to allow setting and querying HW
> > timestamp settings. Drivers implementing these methods will
> > not need to handle SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs.
> > The new NDO methods will handle copying request parameters
> > between user address space and kernel space.
> 
> Maxim, any ETA on the next version? Should we let someone take over?
> It's been over a month since v6 posting.

Assuming Maxim does not respond, can I try to take over? I may have some
time this weekend to play with some PTP related stuff.

Re: [RFC PATCH net-next v6 0/5] New NDO methods ndo_hwtstamp_get/set

[Max Georgiev]

On Sat, Jul 1, 2023 at 4:03 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Tue, Jun 20, 2023 at 07:23:13PM -0700, Jakub Kicinski wrote:
> > On Mon,  1 May 2023 22:31:45 -0600 Maxim Georgiev wrote:
> > > This stack of patches introduces a couple of new NDO methods,
> > > ndo_hwtstamp_get and ndo_hwtstamp_set. These new methods can be
> > > implemented by NIC drivers to allow setting and querying HW
> > > timestamp settings. Drivers implementing these methods will
> > > not need to handle SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs.
> > > The new NDO methods will handle copying request parameters
> > > between user address space and kernel space.
> >
> > Maxim, any ETA on the next version? Should we let someone take over?
> > It's been over a month since v6 posting.
>
> Assuming Maxim does not respond, can I try to take over? I may have some
> time this weekend to play with some PTP related stuff.

Yes, it looks like I hold back everyone.
Vladimir, you are welcome to take over this patch stack.