[RFC PATCH v4 2/5] Add ifreq pointer field to kernel_hwtstamp_config structure

[RFC PATCH v4 2/5] 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 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 |  7 ++++
 include/linux/netdevice.h  |  6 +++
 net/core/dev_ioctl.c       | 80 +++++++++++++++++++++++++++++++++++---
 3 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
index 7c59824f43f5..5164dce3f9a0 100644
--- a/include/linux/net_tstamp.h
+++ b/include/linux/net_tstamp.h
@@ -20,6 +20,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 = (1 << 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 ea10fb1dd6fc..40f4018b13f2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3939,6 +3939,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.39.2

Re: [RFC PATCH v4 2/5] Add ifreq pointer field to kernel_hwtstamp_config structure

[Simon Horman]

On Sat, Apr 22, 2023 at 09:28:17PM -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 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 |  7 ++++
>  include/linux/netdevice.h  |  6 +++
>  net/core/dev_ioctl.c       | 80 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
> index 7c59824f43f5..5164dce3f9a0 100644
> --- a/include/linux/net_tstamp.h
> +++ b/include/linux/net_tstamp.h
> @@ -20,6 +20,13 @@ struct kernel_hwtstamp_config {
>  	int flags;
>  	int tx_type;
>  	int rx_filter;
> +	struct ifreq *ifr;
> +	int kernel_flags;

nit: ifr and kernel_flags should be added to the kdoc for this struct
     that appears immediately above it.

> +};
> +
> +/* possible values for kernel_hwtstamp_config->kernel_flags */
> +enum kernel_hwtstamp_flags {
> +	KERNEL_HWTSTAMP_FLAG_IFR_RESULT = (1 << 0),

nit: maybe BIT(0)

>  };
>  
>  static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kernel_cfg,

...

Re: [RFC PATCH v4 2/5] Add ifreq pointer field to kernel_hwtstamp_config structure

[Max Georgiev]

On Wed, Apr 26, 2023 at 1:59 PM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Sat, Apr 22, 2023 at 09:28:17PM -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 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 |  7 ++++
> >  include/linux/netdevice.h  |  6 +++
> >  net/core/dev_ioctl.c       | 80 +++++++++++++++++++++++++++++++++++---
> >  3 files changed, 87 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
> > index 7c59824f43f5..5164dce3f9a0 100644
> > --- a/include/linux/net_tstamp.h
> > +++ b/include/linux/net_tstamp.h
> > @@ -20,6 +20,13 @@ struct kernel_hwtstamp_config {
> >       int flags;
> >       int tx_type;
> >       int rx_filter;
> > +     struct ifreq *ifr;
> > +     int kernel_flags;
>
> nit: ifr and kernel_flags should be added to the kdoc for this struct
>      that appears immediately above it.
>
> > +};
> > +
> > +/* possible values for kernel_hwtstamp_config->kernel_flags */
> > +enum kernel_hwtstamp_flags {
> > +     KERNEL_HWTSTAMP_FLAG_IFR_RESULT = (1 << 0),
>
> nit: maybe BIT(0)
>
> >  };
> >
> >  static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kernel_cfg,
>
> ...

Thank you for the feedback!
I'll update and resend the patch