[RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Maxim Georgiev]

This patch makes VLAN subsystem 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·subsystem
falls back to calling ndo_eth_ioctl.

Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Maxim Georgiev <glipus@gmail.com>
---
 net/8021q/vlan_dev.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 5920544e93e8..66d54c610aa5 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -353,6 +353,44 @@ static int vlan_dev_set_mac_address(struct net_device *dev, void *p)
 	return 0;
 }
 
+static int vlan_dev_hwtstamp(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct kernel_hwtstamp_config kernel_config = {};
+	struct hwtstamp_config config;
+	int err;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	if ((cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set) ||
+	    (cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get)) {
+		if (ops->ndo_eth_ioctl) {
+			return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
+		else
+			return -EOPNOTSUPP;
+	}
+
+	kernel_config.ifr = ifr;
+	if (cmd == SIOCSHWTSTAMP) {
+		if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+			return -EFAULT;
+
+		hwtstamp_config_to_kernel(&kernel_config, &config);
+		err = ops->ndo_hwtstamp_set(dev, &kernel_config, NULL);
+	} else if (cmd == SIOCGHWTSTAMP) {
+		err = ops->ndo_hwtstamp_get(dev, &kernel_config, NULL);
+	}
+
+	if (err)
+		return err;
+
+	hwtstamp_kernel_to_config(&config, &kernel_config);
+	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
+		return -EFAULT;
+	return 0;
+}
+
 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;
@@ -368,10 +406,12 @@ static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		if (!net_eq(dev_net(dev), dev_net(real_dev)))
 			break;
 		fallthrough;
+	case SIOCGHWTSTAMP:
+		err = vlan_dev_hwtstamp(real_dev, &ifrr, cmd);
+		break;
 	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;
-- 
2.39.2

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 12:33:23AM -0600, Maxim Georgiev wrote:
> This patch makes VLAN subsystem 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┬Ěsubsystem

Strange symbols (┬Ě).

> falls back to calling ndo_eth_ioctl.
> 
> Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Maxim Georgiev <glipus@gmail.com>
> ---
>  net/8021q/vlan_dev.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 5920544e93e8..66d54c610aa5 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -353,6 +353,44 @@ static int vlan_dev_set_mac_address(struct net_device *dev, void *p)
>  	return 0;
>  }
>  
> +static int vlan_dev_hwtstamp(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct kernel_hwtstamp_config kernel_config = {};
> +	struct hwtstamp_config config;
> +	int err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	if ((cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set) ||
> +	    (cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get)) {
> +		if (ops->ndo_eth_ioctl) {
> +			return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
> +		else
> +			return -EOPNOTSUPP;
> +	}
> +
> +	kernel_config.ifr = ifr;
> +	if (cmd == SIOCSHWTSTAMP) {
> +		if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> +			return -EFAULT;
> +
> +		hwtstamp_config_to_kernel(&kernel_config, &config);
> +		err = ops->ndo_hwtstamp_set(dev, &kernel_config, NULL);
> +	} else if (cmd == SIOCGHWTSTAMP) {
> +		err = ops->ndo_hwtstamp_get(dev, &kernel_config, NULL);
> +	}
> +
> +	if (err)
> +		return err;
> +
> +	hwtstamp_kernel_to_config(&config, &kernel_config);
> +	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> +		return -EFAULT;
> +	return 0;
> +}
> +
>  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;
> @@ -368,10 +406,12 @@ static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>  		if (!net_eq(dev_net(dev), dev_net(real_dev)))
>  			break;
>  		fallthrough;
> +	case SIOCGHWTSTAMP:
> +		err = vlan_dev_hwtstamp(real_dev, &ifrr, cmd);
> +		break;
>  	case SIOCGMIIPHY:
>  	case SIOCGMIIREG:
>  	case SIOCSMIIREG:
> -	case SIOCGHWTSTAMP:

I would recommend also making vlan_dev_hwtstamp() be called from the
VLAN driver's ndo_hwtstamp_set() rather than from ndo_eth_ioctl().

My understanding of Jakub's suggestion to (temporarily) stuff ifr
inside kernel_config was to do that from top-level net/core/dev_ioctl.c,
not from the VLAN driver.

>  		if (netif_device_present(real_dev) && ops->ndo_eth_ioctl)
>  			err = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
>  		break;
> -- 
> 2.39.2
>

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[kernel test robot]

Hi Maxim,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on tnguy-next-queue/dev-queue]
[also build test ERROR on next-20230405]
[cannot apply to horms-ipvs/master linus/master v6.3-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxim-Georgiev/Add-ifreq-pointer-field-to-kernel_hwtstamp_config-structure/20230405-143518
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git dev-queue
patch link:    https://lore.kernel.org/r/20230405063323.36270-1-glipus%40gmail.com
patch subject: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path
config: hexagon-randconfig-r041-20230403 (https://download.01.org/0day-ci/archive/20230405/202304052343.Nwvcrk03-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e48fe7f9909e9c0768e866c43b1cc880c0eb329f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Maxim-Georgiev/Add-ifreq-pointer-field-to-kernel_hwtstamp_config-structure/20230405-143518
        git checkout e48fe7f9909e9c0768e866c43b1cc880c0eb329f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash net/8021q/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304052343.Nwvcrk03-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from net/8021q/vlan_dev.c:22:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from net/8021q/vlan_dev.c:22:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from net/8021q/vlan_dev.c:22:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> net/8021q/vlan_dev.c:369:30: error: use of undeclared identifier 'real_dev'
                           return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
                                                     ^
>> net/8021q/vlan_dev.c:370:3: error: expected expression
                   else
                   ^
>> net/8021q/vlan_dev.c:395:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:427:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:514:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:526:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:542:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:547:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:554:1: error: function definition is not allowed here
   {
   ^
>> net/8021q/vlan_dev.c:563:20: error: use of undeclared identifier 'vlan_parse_protocol'; did you mean 'vlan_get_protocol'?
           .parse_protocol = vlan_parse_protocol,
                             ^~~~~~~~~~~~~~~~~~~
                             vlan_get_protocol
   include/linux/if_vlan.h:626:22: note: 'vlan_get_protocol' declared here
   static inline __be16 vlan_get_protocol(const struct sk_buff *skb)
                        ^
   net/8021q/vlan_dev.c:570:1: error: function definition is not allowed here
   {
   ^
>> net/8021q/vlan_dev.c:581:13: error: use of undeclared identifier 'vlan_passthru_hard_header'; did you mean 'vlan_dev_hard_header'?
           .create  = vlan_passthru_hard_header,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~
                      vlan_dev_hard_header
   net/8021q/vlan_dev.c:44:12: note: 'vlan_dev_hard_header' declared here
   static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
              ^
   net/8021q/vlan_dev.c:583:20: error: use of undeclared identifier 'vlan_parse_protocol'; did you mean 'vlan_get_protocol'?
           .parse_protocol = vlan_parse_protocol,
                             ^~~~~~~~~~~~~~~~~~~
                             vlan_get_protocol
   include/linux/if_vlan.h:626:22: note: 'vlan_get_protocol' declared here
   static inline __be16 vlan_get_protocol(const struct sk_buff *skb)
                        ^
   net/8021q/vlan_dev.c:593:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:665:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:679:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:685:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:708:1: error: function definition is not allowed here
   {
   ^
   net/8021q/vlan_dev.c:716:1: error: function definition is not allowed here
   {
   ^
   fatal error: too many errors emitted, stopping now [-ferror-limit=]
   6 warnings and 20 errors generated.


vim +/real_dev +369 net/8021q/vlan_dev.c

   355	
   356	static int vlan_dev_hwtstamp(struct net_device *dev, struct ifreq *ifr, int cmd)
   357	{
   358		const struct net_device_ops *ops = dev->netdev_ops;
   359		struct kernel_hwtstamp_config kernel_config = {};
   360		struct hwtstamp_config config;
   361		int err;
   362	
   363		if (!netif_device_present(dev))
   364			return -ENODEV;
   365	
   366		if ((cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set) ||
   367		    (cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get)) {
   368			if (ops->ndo_eth_ioctl) {
 > 369				return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
 > 370			else
   371				return -EOPNOTSUPP;
   372		}
   373	
   374		kernel_config.ifr = ifr;
   375		if (cmd == SIOCSHWTSTAMP) {
   376			if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
   377				return -EFAULT;
   378	
   379			hwtstamp_config_to_kernel(&kernel_config, &config);
   380			err = ops->ndo_hwtstamp_set(dev, &kernel_config, NULL);
   381		} else if (cmd == SIOCGHWTSTAMP) {
   382			err = ops->ndo_hwtstamp_get(dev, &kernel_config, NULL);
   383		}
   384	
   385		if (err)
   386			return err;
   387	
   388		hwtstamp_kernel_to_config(&config, &kernel_config);
   389		if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
   390			return -EFAULT;
   391		return 0;
   392	}
   393	
   394	static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 > 395	{
   396		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   397		const struct net_device_ops *ops = real_dev->netdev_ops;
   398		struct ifreq ifrr;
   399		int err = -EOPNOTSUPP;
   400	
   401		strscpy_pad(ifrr.ifr_name, real_dev->name, IFNAMSIZ);
   402		ifrr.ifr_ifru = ifr->ifr_ifru;
   403	
   404		switch (cmd) {
   405		case SIOCSHWTSTAMP:
   406			if (!net_eq(dev_net(dev), dev_net(real_dev)))
   407				break;
   408			fallthrough;
   409		case SIOCGHWTSTAMP:
   410			err = vlan_dev_hwtstamp(real_dev, &ifrr, cmd);
   411			break;
   412		case SIOCGMIIPHY:
   413		case SIOCGMIIREG:
   414		case SIOCSMIIREG:
   415			if (netif_device_present(real_dev) && ops->ndo_eth_ioctl)
   416				err = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
   417			break;
   418		}
   419	
   420		if (!err)
   421			ifr->ifr_ifru = ifrr.ifr_ifru;
   422	
   423		return err;
   424	}
   425	
   426	static int vlan_dev_neigh_setup(struct net_device *dev, struct neigh_parms *pa)
   427	{
   428		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   429		const struct net_device_ops *ops = real_dev->netdev_ops;
   430		int err = 0;
   431	
   432		if (netif_device_present(real_dev) && ops->ndo_neigh_setup)
   433			err = ops->ndo_neigh_setup(real_dev, pa);
   434	
   435		return err;
   436	}
   437	
   438	#if IS_ENABLED(CONFIG_FCOE)
   439	static int vlan_dev_fcoe_ddp_setup(struct net_device *dev, u16 xid,
   440					   struct scatterlist *sgl, unsigned int sgc)
   441	{
   442		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   443		const struct net_device_ops *ops = real_dev->netdev_ops;
   444		int rc = 0;
   445	
   446		if (ops->ndo_fcoe_ddp_setup)
   447			rc = ops->ndo_fcoe_ddp_setup(real_dev, xid, sgl, sgc);
   448	
   449		return rc;
   450	}
   451	
   452	static int vlan_dev_fcoe_ddp_done(struct net_device *dev, u16 xid)
   453	{
   454		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   455		const struct net_device_ops *ops = real_dev->netdev_ops;
   456		int len = 0;
   457	
   458		if (ops->ndo_fcoe_ddp_done)
   459			len = ops->ndo_fcoe_ddp_done(real_dev, xid);
   460	
   461		return len;
   462	}
   463	
   464	static int vlan_dev_fcoe_enable(struct net_device *dev)
   465	{
   466		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   467		const struct net_device_ops *ops = real_dev->netdev_ops;
   468		int rc = -EINVAL;
   469	
   470		if (ops->ndo_fcoe_enable)
   471			rc = ops->ndo_fcoe_enable(real_dev);
   472		return rc;
   473	}
   474	
   475	static int vlan_dev_fcoe_disable(struct net_device *dev)
   476	{
   477		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   478		const struct net_device_ops *ops = real_dev->netdev_ops;
   479		int rc = -EINVAL;
   480	
   481		if (ops->ndo_fcoe_disable)
   482			rc = ops->ndo_fcoe_disable(real_dev);
   483		return rc;
   484	}
   485	
   486	static int vlan_dev_fcoe_ddp_target(struct net_device *dev, u16 xid,
   487					    struct scatterlist *sgl, unsigned int sgc)
   488	{
   489		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   490		const struct net_device_ops *ops = real_dev->netdev_ops;
   491		int rc = 0;
   492	
   493		if (ops->ndo_fcoe_ddp_target)
   494			rc = ops->ndo_fcoe_ddp_target(real_dev, xid, sgl, sgc);
   495	
   496		return rc;
   497	}
   498	#endif
   499	
   500	#ifdef NETDEV_FCOE_WWNN
   501	static int vlan_dev_fcoe_get_wwn(struct net_device *dev, u64 *wwn, int type)
   502	{
   503		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   504		const struct net_device_ops *ops = real_dev->netdev_ops;
   505		int rc = -EINVAL;
   506	
   507		if (ops->ndo_fcoe_get_wwn)
   508			rc = ops->ndo_fcoe_get_wwn(real_dev, wwn, type);
   509		return rc;
   510	}
   511	#endif
   512	
   513	static void vlan_dev_change_rx_flags(struct net_device *dev, int change)
   514	{
   515		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
   516	
   517		if (dev->flags & IFF_UP) {
   518			if (change & IFF_ALLMULTI)
   519				dev_set_allmulti(real_dev, dev->flags & IFF_ALLMULTI ? 1 : -1);
   520			if (change & IFF_PROMISC)
   521				dev_set_promiscuity(real_dev, dev->flags & IFF_PROMISC ? 1 : -1);
   522		}
   523	}
   524	
   525	static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)
   526	{
   527		dev_mc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);
   528		dev_uc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);
   529	}
   530	
   531	/*
   532	 * vlan network devices have devices nesting below it, and are a special
   533	 * "super class" of normal network devices; split their locks off into a
   534	 * separate class since they always nest.
   535	 */
   536	static struct lock_class_key vlan_netdev_xmit_lock_key;
   537	static struct lock_class_key vlan_netdev_addr_lock_key;
   538	
   539	static void vlan_dev_set_lockdep_one(struct net_device *dev,
   540					     struct netdev_queue *txq,
   541					     void *unused)
   542	{
   543		lockdep_set_class(&txq->_xmit_lock, &vlan_netdev_xmit_lock_key);
   544	}
   545	
   546	static void vlan_dev_set_lockdep_class(struct net_device *dev)
   547	{
   548		lockdep_set_class(&dev->addr_list_lock,
   549				  &vlan_netdev_addr_lock_key);
   550		netdev_for_each_tx_queue(dev, vlan_dev_set_lockdep_one, NULL);
   551	}
   552	
   553	static __be16 vlan_parse_protocol(const struct sk_buff *skb)
   554	{
   555		struct vlan_ethhdr *veth = (struct vlan_ethhdr *)(skb->data);
   556	
   557		return __vlan_get_protocol(skb, veth->h_vlan_proto, NULL);
   558	}
   559	
   560	static const struct header_ops vlan_header_ops = {
   561		.create	 = vlan_dev_hard_header,
   562		.parse	 = eth_header_parse,
 > 563		.parse_protocol = vlan_parse_protocol,
   564	};
   565	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Max Georgiev]

On Wed, Apr 5, 2023 at 6:26 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Wed, Apr 05, 2023 at 12:33:23AM -0600, Maxim Georgiev wrote:
> > This patch makes VLAN subsystem 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┬Ěsubsystem
>
> Strange symbols (┬Ě).

Bad copy-paste, sorry. Fixed.

>
> > falls back to calling ndo_eth_ioctl.
> >
> > Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Signed-off-by: Maxim Georgiev <glipus@gmail.com>
> > ---
> >  net/8021q/vlan_dev.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> > index 5920544e93e8..66d54c610aa5 100644
> > --- a/net/8021q/vlan_dev.c
> > +++ b/net/8021q/vlan_dev.c
> > @@ -353,6 +353,44 @@ static int vlan_dev_set_mac_address(struct net_device *dev, void *p)
> >       return 0;
> >  }
> >
> > +static int vlan_dev_hwtstamp(struct net_device *dev, struct ifreq *ifr, int cmd)
> > +{
> > +     const struct net_device_ops *ops = dev->netdev_ops;
> > +     struct kernel_hwtstamp_config kernel_config = {};
> > +     struct hwtstamp_config config;
> > +     int err;
> > +
> > +     if (!netif_device_present(dev))
> > +             return -ENODEV;
> > +
> > +     if ((cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set) ||
> > +         (cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get)) {
> > +             if (ops->ndo_eth_ioctl) {
> > +                     return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
> > +             else
> > +                     return -EOPNOTSUPP;
> > +     }
> > +
> > +     kernel_config.ifr = ifr;
> > +     if (cmd == SIOCSHWTSTAMP) {
> > +             if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> > +                     return -EFAULT;
> > +
> > +             hwtstamp_config_to_kernel(&kernel_config, &config);
> > +             err = ops->ndo_hwtstamp_set(dev, &kernel_config, NULL);
> > +     } else if (cmd == SIOCGHWTSTAMP) {
> > +             err = ops->ndo_hwtstamp_get(dev, &kernel_config, NULL);
> > +     }
> > +
> > +     if (err)
> > +             return err;
> > +
> > +     hwtstamp_kernel_to_config(&config, &kernel_config);
> > +     if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> > +             return -EFAULT;
> > +     return 0;
> > +}
> > +
> >  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;
> > @@ -368,10 +406,12 @@ static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> >               if (!net_eq(dev_net(dev), dev_net(real_dev)))
> >                       break;
> >               fallthrough;
> > +     case SIOCGHWTSTAMP:
> > +             err = vlan_dev_hwtstamp(real_dev, &ifrr, cmd);
> > +             break;
> >       case SIOCGMIIPHY:
> >       case SIOCGMIIREG:
> >       case SIOCSMIIREG:
> > -     case SIOCGHWTSTAMP:
>
> I would recommend also making vlan_dev_hwtstamp() be called from the
> VLAN driver's ndo_hwtstamp_set() rather than from ndo_eth_ioctl().

Vladimir, could you please elaborate here a bit?
Are you saying that I should go all the way with vlan NDO conversion,
implement ndo_hwtstamp_get/set() for vlan, and stop handling
SIOCGHWTSTAMP/SIOCSHWTSTAMP in vlan_dev_ioctl()?

>
> My understanding of Jakub's suggestion to (temporarily) stuff ifr
> inside kernel_config was to do that from top-level net/core/dev_ioctl.c,
> not from the VLAN driver.

[RFC PATCH v3 2/5] in this patch stack changes net/core/dev_ioctl.c
to insert ifr inside kernel_config. I assumed that I should do it here too
so underlying drivers could rely on ifr pointer in kernel_config being
always initialized.
If the plan is to stop supporting SIOCGHWTSTAMP/SIOCSHWTSTAMP
in vlan_dev_ioctl() all together and move the hw timestamp handling
logic to vlan_get/set_hwtstamp() functions, then this ifr initialization
code will be removed from net/8021q/vlan_dev.c anyway.

>
> >               if (netif_device_present(real_dev) && ops->ndo_eth_ioctl)
> >                       err = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
> >               break;
> > --
> > 2.39.2
> >

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 10:19:11AM -0600, Max Georgiev wrote:
> > I would recommend also making vlan_dev_hwtstamp() be called from the
> > VLAN driver's ndo_hwtstamp_set() rather than from ndo_eth_ioctl().
> 
> Vladimir, could you please elaborate here a bit?
> Are you saying that I should go all the way with vlan NDO conversion,
> implement ndo_hwtstamp_get/set() for vlan, and stop handling
> SIOCGHWTSTAMP/SIOCSHWTSTAMP in vlan_dev_ioctl()?

Yes, sorry for being unclear.

> > My understanding of Jakub's suggestion to (temporarily) stuff ifr
> > inside kernel_config was to do that from top-level net/core/dev_ioctl.c,
> > not from the VLAN driver.
> 
> [RFC PATCH v3 2/5] in this patch stack changes net/core/dev_ioctl.c
> to insert ifr inside kernel_config. I assumed that I should do it here too
> so underlying drivers could rely on ifr pointer in kernel_config being
> always initialized.
> If the plan is to stop supporting SIOCGHWTSTAMP/SIOCSHWTSTAMP
> in vlan_dev_ioctl() all together and move the hw timestamp handling
> logic to vlan_get/set_hwtstamp() functions, then this ifr initialization
> code will be removed from net/8021q/vlan_dev.c anyway.

Yes, correct, dev_set_hwtstamp() should provide it.

There's a small thing I don't like about stuffing "ifr" inside struct
kernel_hwtstamp_config, and that's that some drivers keep the last
configuration privately using memcpy(). If we put "ifr" there, they will
practically have access to a stale pointer, because "ifr" loses meaning
once the ioctl syscall is over.

Since we don't know how long it will take until the ndo_hwtstamp_set()
conversion is complete (experience says: possibly indefinitely), it
would be good if this wasn't possible, because who knows what ideas
people might get to do with it.

Options to avoid it are:
- keep doing what you're doing - let drivers memcpy() the struct
  hwtstamp_config and not the struct kernel_hwtstamp_config.
- pass the ifr as yet another argument to ndo_hwtstamp_set(), and don't
  stuff it inside struct kernel_hwtstamp_config.

Neither of these is particularly great, because at the end of the
conversion, some extra cleanup will be required to fix up the API again
(either to stop all drivers from using struct hwtstamp_config, or to
stop passing the argument which is no longer used by anybody).

I think, personally, I'd opt for the first bullet, keep doing what
you're doing. It should require a bit less cleanup, since not all
drivers do the memcpy() thing.

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Jakub Kicinski]

On Wed,  5 Apr 2023 00:33:23 -0600 Maxim Georgiev wrote:
> +static int vlan_dev_hwtstamp(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct kernel_hwtstamp_config kernel_config = {};
> +	struct hwtstamp_config config;
> +	int err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	if ((cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set) ||
> +	    (cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get)) {
> +		if (ops->ndo_eth_ioctl) {
> +			return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
> +		else
> +			return -EOPNOTSUPP;
> +	}
> +
> +	kernel_config.ifr = ifr;
> +	if (cmd == SIOCSHWTSTAMP) {
> +		if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> +			return -EFAULT;
> +
> +		hwtstamp_config_to_kernel(&kernel_config, &config);
> +		err = ops->ndo_hwtstamp_set(dev, &kernel_config, NULL);
> +	} else if (cmd == SIOCGHWTSTAMP) {
> +		err = ops->ndo_hwtstamp_get(dev, &kernel_config, NULL);
> +	}
> +
> +	if (err)
> +		return err;
> +
> +	hwtstamp_kernel_to_config(&config, &kernel_config);
> +	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> +		return -EFAULT;
> +	return 0;
> +}

This needs to live in the core. I think the real_dev is a lower of the
vlan device? All the vlan driver should do is attach the generic helper:

	.ndo_hwtstamp_get = generic_hwtstamp_get_lower,

and the same for set. No?

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 09:42:10AM -0700, Jakub Kicinski wrote:
> This needs to live in the core. I think the real_dev is a lower of the
> vlan device? All the vlan driver should do is attach the generic helper:
> 
> 	.ndo_hwtstamp_get = generic_hwtstamp_get_lower,
> 
> and the same for set. No?

The goal would be for macvlan and bonding to use the same generic_hwtstamp_get_lower()?
How would the generic helper get to bond_option_active_slave_get_rcu(),
vlan_dev_priv(dev)->real_dev, macvlan_dev_real_dev(dev)?

Perhaps a generic_hwtstamp_get_lower() that takes the lower as argument,
and 3 small wrappers in vlan, macvlan, bonding which identify that lower?

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Jakub Kicinski]

On Wed, 5 Apr 2023 20:03:22 +0300 Vladimir Oltean wrote:
> The goal would be for macvlan and bonding to use the same generic_hwtstamp_get_lower()?
> How would the generic helper get to bond_option_active_slave_get_rcu(),
> vlan_dev_priv(dev)->real_dev, macvlan_dev_real_dev(dev)?
> 
> Perhaps a generic_hwtstamp_get_lower() that takes the lower as argument,
> and 3 small wrappers in vlan, macvlan, bonding which identify that lower?

The bonding situation is probably more complex, I haven't looked,
but for *vlans we can just get the lower from netdev linkage, no?
Sure the drivers have their own pointers for convenience and with
their own lifetime rules but under rtnl lock lower/upper should work...

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 10:13:23AM -0700, Jakub Kicinski wrote:
> On Wed, 5 Apr 2023 20:03:22 +0300 Vladimir Oltean wrote:
> > The goal would be for macvlan and bonding to use the same generic_hwtstamp_get_lower()?
> > How would the generic helper get to bond_option_active_slave_get_rcu(),
> > vlan_dev_priv(dev)->real_dev, macvlan_dev_real_dev(dev)?
> > 
> > Perhaps a generic_hwtstamp_get_lower() that takes the lower as argument,
> > and 3 small wrappers in vlan, macvlan, bonding which identify that lower?
> 
> The bonding situation is probably more complex, I haven't looked,
> but for *vlans we can just get the lower from netdev linkage, no?
> Sure the drivers have their own pointers for convenience and with
> their own lifetime rules but under rtnl lock lower/upper should work...

So what do you suggest doing with bonding, then? Not use the generic
helper at all? It's not that more complex, btw. Here are the differences:

- it changes ifrr.ifr_name with real_dev->name for a reason I can't
  really determine or find in commit 94dd016ae538 ("bond: pass
  get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device"). Since vlan
  and macvlan don't do it, and operate with lower drivers from the same
  pool as bonding, I'd imagine it's not needed.

- it requires cfg.flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX to be set in
  SET requests

- it sets cfg.flags | HWTSTAMP_FLAG_BONDED_PHC_INDEX in GET responses

Notably, something I would have expected it does but it doesn't do is it
doesn't apply the same hwtstamping config to the lower interface that
isn't active, when the switchover happens. Presumably user space does that.

So it's not that much different.

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 08:28:40PM +0300, Vladimir Oltean wrote:
> - it changes ifrr.ifr_name with real_dev->name for a reason I can't
>   really determine or find in commit 94dd016ae538 ("bond: pass
>   get_ts_info and SIOC[SG]HWTSTAMP ioctl to active device"). Since vlan
>   and macvlan don't do it, and operate with lower drivers from the same
>   pool as bonding, I'd imagine it's not needed.

Ah, they do, they do, my bad. So it's down to 2 differences, which can
be handled easily with the wrapper function model - the bonding wrapper
checks, or sets, the HWTSTAMP_FLAG_BONDED_PHC_INDEX flag.

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Jakub Kicinski]

On Wed, 5 Apr 2023 20:28:40 +0300 Vladimir Oltean wrote:
> So what do you suggest doing with bonding, then? Not use the generic
> helper at all?

It'd seem most natural to me to split the generic "descend" helper into
two functions, one which retrieves the lower and one which does the
appropriate calling dance (ndo vs ioctl, and DSA, which I guess is now
gone?). The latter could be used for the first descend as well I'd
presume. And it can be exported for the use of more complex drivers
like bonding which want to walk the lowers themselves.

> - it requires cfg.flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX to be set in
>   SET requests
> 
> - it sets cfg.flags | HWTSTAMP_FLAG_BONDED_PHC_INDEX in GET responses

IIRC that was to indicate to user space that the real PHC may change
for this netdev so it needs to pay attention to netlink notifications.
Shouldn't apply to *vlans?

> Notably, something I would have expected it does but it doesn't do is it
> doesn't apply the same hwtstamping config to the lower interface that
> isn't active, when the switchover happens. Presumably user space does that.

Yes, user space must be involved anyway, because the entire clock will
change. IMHO implementing the pass thru for timestamping requests on
bonding is checkbox engineering, kernel can't make it work
transparently. But nobody else spoke up when it was proposed so...

> So it's not that much different.

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Vladimir Oltean]

On Wed, Apr 05, 2023 at 10:42:53AM -0700, Jakub Kicinski wrote:
> On Wed, 5 Apr 2023 20:28:40 +0300 Vladimir Oltean wrote:
> > So what do you suggest doing with bonding, then? Not use the generic
> > helper at all?
> 
> It'd seem most natural to me to split the generic "descend" helper into
> two functions, one which retrieves the lower and one which does the
> appropriate calling dance (ndo vs ioctl, and DSA, which I guess is now
> gone?).

There's nothing DSA-related to be done. DSA masters can't be lowers of
any other virtual interface kinds except bridge or bonding/team, and:
- bridge doesn't support hwtstamping
- bonding is also DSA master when it has a DSA master as lower, so the
  DSA master restriction has already run once - on the bonding device
  itself

> The latter could be used for the first descend as well I'd presume.
> And it can be exported for the use of more complex drivers like
> bonding which want to walk the lowers themselves.
> 
> > - it requires cfg.flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX to be set in
> >   SET requests
> > 
> > - it sets cfg.flags | HWTSTAMP_FLAG_BONDED_PHC_INDEX in GET responses
> 
> IIRC that was to indicate to user space that the real PHC may change
> for this netdev so it needs to pay attention to netlink notifications.
> Shouldn't apply to *vlans?

No, this shouldn't apply to *vlans, but I didn't suggest that it should.
I don't think my proposal was clear enough, so here's some code
(untested, written in email client).

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);

	return generic_hwtstamp_get_lower(real_dev, cfg, extack);
}

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);

	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
}

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;

	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
}

static int bond_hwtstamp_get(struct net_device *bond_dev,
			     struct kernel_hwtstamp_config *cfg,
			     struct netlink_ext_ack *extack)
{
	struct bonding *bond = netdev_priv(bond_dev);
	struct net_device *real_dev = bond_option_active_slave_get_rcu(bond);
	int err;

	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 */
	cfg->flags |= HWTSTAMP_FLAG_BONDED_PHC_INDEX;

	return 0;
}

static int bond_hwtstamp_set(struct net_device *bond_dev,
			     struct kernel_hwtstamp_config *cfg,
			     struct netlink_ext_ack *extack)
{
	struct bonding *bond = netdev_priv(bond_dev);
	struct net_device *real_dev = bond_option_active_slave_get_rcu(bond);
	int err;

	if (!real_dev)
		return -EOPNOTSUPP;

	if (!(cfg->flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX))
		return -EOPNOTSUPP;

	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
}

Doesn't seem in any way necessary to complicate things with the netdev
adjacence lists?

> Yes, user space must be involved anyway, because the entire clock will
> change. IMHO implementing the pass thru for timestamping requests on
> bonding is checkbox engineering, kernel can't make it work
> transparently. But nobody else spoke up when it was proposed so...

ok, but that's a bit beside the point here.

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[kernel test robot]

Hi Maxim,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on tnguy-next-queue/dev-queue]
[also build test ERROR on next-20230405]
[cannot apply to horms-ipvs/master linus/master v6.3-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxim-Georgiev/Add-ifreq-pointer-field-to-kernel_hwtstamp_config-structure/20230405-143518
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git dev-queue
patch link:    https://lore.kernel.org/r/20230405063323.36270-1-glipus%40gmail.com
patch subject: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path
config: arc-randconfig-r034-20230404 (https://download.01.org/0day-ci/archive/20230406/202304060246.Vxhd7U2x-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e48fe7f9909e9c0768e866c43b1cc880c0eb329f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Maxim-Georgiev/Add-ifreq-pointer-field-to-kernel_hwtstamp_config-structure/20230405-143518
        git checkout e48fe7f9909e9c0768e866c43b1cc880c0eb329f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash net/8021q/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304060246.Vxhd7U2x-lkp@intel.com/

All errors (new ones prefixed by >>):

   net/8021q/vlan_dev.c: In function 'vlan_dev_hwtstamp':
>> net/8021q/vlan_dev.c:369:51: error: 'real_dev' undeclared (first use in this function)
     369 |                         return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
         |                                                   ^~~~~~~~
   net/8021q/vlan_dev.c:369:51: note: each undeclared identifier is reported only once for each function it appears in
>> net/8021q/vlan_dev.c:369:61: error: passing argument 2 of 'ops->ndo_eth_ioctl' from incompatible pointer type [-Werror=incompatible-pointer-types]
     369 |                         return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
         |                                                             ^~~~
         |                                                             |
         |                                                             struct ifreq **
   net/8021q/vlan_dev.c:369:61: note: expected 'struct ifreq *' but argument is of type 'struct ifreq **'
>> net/8021q/vlan_dev.c:370:17: error: expected '}' before 'else'
     370 |                 else
         |                 ^~~~
   cc1: some warnings being treated as errors


vim +/real_dev +369 net/8021q/vlan_dev.c

   355	
   356	static int vlan_dev_hwtstamp(struct net_device *dev, struct ifreq *ifr, int cmd)
   357	{
   358		const struct net_device_ops *ops = dev->netdev_ops;
   359		struct kernel_hwtstamp_config kernel_config = {};
   360		struct hwtstamp_config config;
   361		int err;
   362	
   363		if (!netif_device_present(dev))
   364			return -ENODEV;
   365	
   366		if ((cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set) ||
   367		    (cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get)) {
   368			if (ops->ndo_eth_ioctl) {
 > 369				return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
 > 370			else
   371				return -EOPNOTSUPP;
   372		}
   373	
   374		kernel_config.ifr = ifr;
   375		if (cmd == SIOCSHWTSTAMP) {
   376			if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
   377				return -EFAULT;
   378	
   379			hwtstamp_config_to_kernel(&kernel_config, &config);
   380			err = ops->ndo_hwtstamp_set(dev, &kernel_config, NULL);
   381		} else if (cmd == SIOCGHWTSTAMP) {
   382			err = ops->ndo_hwtstamp_get(dev, &kernel_config, NULL);
   383		}
   384	
   385		if (err)
   386			return err;
   387	
   388		hwtstamp_kernel_to_config(&config, &kernel_config);
   389		if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
   390			return -EFAULT;
   391		return 0;
   392	}
   393	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Jakub Kicinski]

On Wed, 5 Apr 2023 21:01:21 +0300 Vladimir Oltean wrote:
> - bonding is also DSA master when it has a DSA master as lower, so the
>   DSA master restriction has already run once - on the bonding device
>   itself

Huh, didn't know that.

> > The latter could be used for the first descend as well I'd presume.
> > And it can be exported for the use of more complex drivers like
> > bonding which want to walk the lowers themselves.
> >   
> > > - it requires cfg.flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX to be set in
> > >   SET requests
> > > 
> > > - it sets cfg.flags | HWTSTAMP_FLAG_BONDED_PHC_INDEX in GET responses  
> > 
> > IIRC that was to indicate to user space that the real PHC may change
> > for this netdev so it needs to pay attention to netlink notifications.
> > Shouldn't apply to *vlans?  
> 
> No, this shouldn't apply to *vlans, but I didn't suggest that it should.

Good, so if we just target *vlans we don't have to worry.

> I don't think my proposal was clear enough, so here's some code
> (untested, written in email client).
> 
> 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);
> 
> 	return generic_hwtstamp_get_lower(real_dev, cfg, extack);
> }
> 
> 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);
> 
> 	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> }
> 
> 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;
> 
> 	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> }

I got that, but why wouldn't this not be better, as it avoids 
the 3 driver stubs? (also written in the MUA)

int net_lower_hwtstamp_set(struct net_device *dev,
			   struct kernel_hwtstamp_config *cfg,
			   struct netlink_ext_ack *extack)
{
	struct list_head *iter = dev->adj_list.lower.next;
	struct net_device *lower;
	
	lower = netdev_lower_get_next(dev, &iter);
	return generic_hwtstamp_set_lower(lower, cfg, extack);
}

> static int bond_hwtstamp_get(struct net_device *bond_dev,
> 			     struct kernel_hwtstamp_config *cfg,
> 			     struct netlink_ext_ack *extack)
> {
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct net_device *real_dev = bond_option_active_slave_get_rcu(bond);
> 	int err;
> 
> 	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 */
> 	cfg->flags |= HWTSTAMP_FLAG_BONDED_PHC_INDEX;
> 
> 	return 0;
> }
> 
> static int bond_hwtstamp_set(struct net_device *bond_dev,
> 			     struct kernel_hwtstamp_config *cfg,
> 			     struct netlink_ext_ack *extack)
> {
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct net_device *real_dev = bond_option_active_slave_get_rcu(bond);
> 	int err;
> 
> 	if (!real_dev)
> 		return -EOPNOTSUPP;
> 
> 	if (!(cfg->flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX))
> 		return -EOPNOTSUPP;
> 
> 	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> }
> 
> Doesn't seem in any way necessary to complicate things with the netdev
> adjacence lists?

What is the complication? We can add a "get first" helper maybe to hide
the oddities of the linking.

> > Yes, user space must be involved anyway, because the entire clock will
> > change. IMHO implementing the pass thru for timestamping requests on
> > bonding is checkbox engineering, kernel can't make it work
> > transparently. But nobody else spoke up when it was proposed so...  
> 
> ok, but that's a bit beside the point here.

You cut off the quote it was responding to so IDK if it is.

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Max Georgiev]

On Wed, Apr 5, 2023 at 6:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Apr 2023 21:01:21 +0300 Vladimir Oltean wrote:
> > - bonding is also DSA master when it has a DSA master as lower, so the
> >   DSA master restriction has already run once - on the bonding device
> >   itself
>
> Huh, didn't know that.
>
> > > The latter could be used for the first descend as well I'd presume.
> > > And it can be exported for the use of more complex drivers like
> > > bonding which want to walk the lowers themselves.
> > >
> > > > - it requires cfg.flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX to be set in
> > > >   SET requests
> > > >
> > > > - it sets cfg.flags | HWTSTAMP_FLAG_BONDED_PHC_INDEX in GET responses
> > >
> > > IIRC that was to indicate to user space that the real PHC may change
> > > for this netdev so it needs to pay attention to netlink notifications.
> > > Shouldn't apply to *vlans?
> >
> > No, this shouldn't apply to *vlans, but I didn't suggest that it should.
>
> Good, so if we just target *vlans we don't have to worry.
>
> > I don't think my proposal was clear enough, so here's some code
> > (untested, written in email client).
> >
> > 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);
> >
> >       return generic_hwtstamp_get_lower(real_dev, cfg, extack);
> > }
> >
> > 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);
> >
> >       return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> > }
> >
> > 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;
> >
> >       return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> > }
>
> I got that, but why wouldn't this not be better, as it avoids
> the 3 driver stubs? (also written in the MUA)
>
> int net_lower_hwtstamp_set(struct net_device *dev,
>                            struct kernel_hwtstamp_config *cfg,
>                            struct netlink_ext_ack *extack)
> {
>         struct list_head *iter = dev->adj_list.lower.next;
>         struct net_device *lower;
>
>         lower = netdev_lower_get_next(dev, &iter);
>         return generic_hwtstamp_set_lower(lower, cfg, extack);
> }
>
> > static int bond_hwtstamp_get(struct net_device *bond_dev,
> >                            struct kernel_hwtstamp_config *cfg,
> >                            struct netlink_ext_ack *extack)
> > {
> >       struct bonding *bond = netdev_priv(bond_dev);
> >       struct net_device *real_dev = bond_option_active_slave_get_rcu(bond);
> >       int err;
> >
> >       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 */
> >       cfg->flags |= HWTSTAMP_FLAG_BONDED_PHC_INDEX;
> >
> >       return 0;
> > }
> >
> > static int bond_hwtstamp_set(struct net_device *bond_dev,
> >                            struct kernel_hwtstamp_config *cfg,
> >                            struct netlink_ext_ack *extack)
> > {
> >       struct bonding *bond = netdev_priv(bond_dev);
> >       struct net_device *real_dev = bond_option_active_slave_get_rcu(bond);
> >       int err;
> >
> >       if (!real_dev)
> >               return -EOPNOTSUPP;
> >
> >       if (!(cfg->flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX))
> >               return -EOPNOTSUPP;
> >
> >       return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> > }
> >
> > Doesn't seem in any way necessary to complicate things with the netdev
> > adjacence lists?
>
> What is the complication? We can add a "get first" helper maybe to hide
> the oddities of the linking.
>
> > > Yes, user space must be involved anyway, because the entire clock will
> > > change. IMHO implementing the pass thru for timestamping requests on
> > > bonding is checkbox engineering, kernel can't make it work
> > > transparently. But nobody else spoke up when it was proposed so...
> >
> > ok, but that's a bit beside the point here.
>
> You cut off the quote it was responding to so IDK if it is.

I tried my best to follow the discussion, and convert it to compilable code.
Here is what I have in mind for generic_hwtstamp_get_lower():

int generic_hwtstamp_get_lower(struct net_dev *dev,
                           struct kernel_hwtstamp_config *kernel_cfg,
                           struct netlink_ext_ack *extack)
{
        const struct net_device_ops *ops = dev->netdev_ops;
        struct hwtstamp_config cfg;
        int err;

        if (!netif_device_present(dev))
                return -ENODEV;

        if (ops->ndo_hwtstamp_get)
                return ops->ndo_hwtstamp_get(dev, cfg, extack);

        if (!cfg->ifr)
                return -EOPNOTSUPP;

        err = dev_eth_ioctl(dev, cfg->ifr, SIOCGHWTSTAMP);
        if (err)
            return err;

        if (copy_from_user(&cfg, cfg->ifr->ifr_data, sizeof(cfg)))
                return -EFAULT;

        hwtstamp_config_to_kernel(kernel_cfg, &cfg);

        return 0;
}

It looks like there is a possibility that the returned hwtstamp_config structure
will be copied twice to ifr and copied once from ifr on the return path
in case if the underlying driver does not implement ndo_hwtstamp_get():
- the underlying driver calls copy_to_user() inside its ndo_eth_ioctl()
  implementation to return the data to generic_hwtstamp_get_lower();
- then generic_hwtstamp_get_lower() calls copy_from_user() to copy it
  back out of the ifr to kernel_hwtstamp_config structure;
- then dev_get_hwtstamp() calls copy_to_user() again to update
  the same ifr with the same data the ifr already contains.

Should we consider this acceptable?

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Vladimir Oltean]

On Thu, Apr 06, 2023 at 12:21:37AM -0600, Max Georgiev wrote:
> I tried my best to follow the discussion, and convert it to compilable code.
> Here is what I have in mind for generic_hwtstamp_get_lower():
> 
> int generic_hwtstamp_get_lower(struct net_dev *dev,
>                            struct kernel_hwtstamp_config *kernel_cfg,
>                            struct netlink_ext_ack *extack)
> {
>         const struct net_device_ops *ops = dev->netdev_ops;
>         struct hwtstamp_config cfg;
>         int err;
> 
>         if (!netif_device_present(dev))
>                 return -ENODEV;
> 
>         if (ops->ndo_hwtstamp_get)
>                 return ops->ndo_hwtstamp_get(dev, cfg, extack);
> 
>         if (!cfg->ifr)
>                 return -EOPNOTSUPP;
> 
>         err = dev_eth_ioctl(dev, cfg->ifr, SIOCGHWTSTAMP);
>         if (err)
>             return err;
> 
>         if (copy_from_user(&cfg, cfg->ifr->ifr_data, sizeof(cfg)))
>                 return -EFAULT;
> 
>         hwtstamp_config_to_kernel(kernel_cfg, &cfg);
> 
>         return 0;
> }

Side note, it doesn't look like this code is particularly compilable
either - "cfg" is used in some places instead of "kernel_cfg".

> 
> It looks like there is a possibility that the returned hwtstamp_config structure
> will be copied twice to ifr and copied once from ifr on the return path
> in case if the underlying driver does not implement ndo_hwtstamp_get():
> - the underlying driver calls copy_to_user() inside its ndo_eth_ioctl()
>   implementation to return the data to generic_hwtstamp_get_lower();
> - then generic_hwtstamp_get_lower() calls copy_from_user() to copy it
>   back out of the ifr to kernel_hwtstamp_config structure;
> - then dev_get_hwtstamp() calls copy_to_user() again to update
>   the same ifr with the same data the ifr already contains.
> 
> Should we consider this acceptable?

Thanks for laying this out. I guess with a table it's going to be
clearer, so to summarize, I believe this is the status:

Assuming we convert *vlan to ndo_hwtstamp_set():

===================

If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev
driver uses ndo_eth_ioctl(), we have:
- one copy_from_user() in dev_set_hwtstamp()
- one copy_from_user() in the real_dev's ndo_eth_ioctl()
- one copy_to_user() in the real_dev's ndo_eth_ioctl()
- one copy_from_user() in generic_hwtstamp_get_lower()
- one copy_to_user() in dev_set_hwtstamp()

If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev
is converted too, we have:
- one copy_from_user() in dev_set_hwtstamp()
- one copy_to_user() in dev_set_hwtstamp()

===================

Assuming we don't convert *vlan to ndo_hwtstamp_set():

===================

If the vlan driver isn't converted to ndo_hwtstamp_set() and the
real_dev driver isn't converted either, we have:
- one copy_from_user() in dev_set_hwtstamp()
- one copy_from_user() in the real_dev's ndo_eth_ioctl()
- one copy_to_user() in the real_dev's ndo_eth_ioctl()

If the vlan driver isn't converted to ndo_hwtstamp_set(), but the
real_dev driver is, we have:
- one copy_from_user() in dev_set_hwtstamp()
- one copy_from_user() in the vlan's ndo_eth_ioctl()
- one copy_to_user() in the vlan's ndo_eth_ioctl()

===================

So between converting and not converting the *vlans to ndo_hwtstamp_set(),
the worst case is going to be worse (with a mix of new API in *vlan and
old API in real_dev) and the best case is going to be better (with new
API in both *vlan and real_dev). OTOH, with old API in *vlan, the number
of copies to/from the user buffer is going to be constant at 3, which is
not the best, not the worst.

I guess the data indicates that we should convert the *vlans to
ndo_hwtstamp_set() at the very end of the process, and for now, just
make them compatible with a real_dev that uses the new API?

Note that I haven't done the math for the "get" operation yet, but I
believe it to be similar.

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Max Georgiev]

On Thu, Apr 6, 2023 at 9:02 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Thu, Apr 06, 2023 at 12:21:37AM -0600, Max Georgiev wrote:
> > I tried my best to follow the discussion, and convert it to compilable code.
> > Here is what I have in mind for generic_hwtstamp_get_lower():
> >
> > int generic_hwtstamp_get_lower(struct net_dev *dev,
> >                            struct kernel_hwtstamp_config *kernel_cfg,
> >                            struct netlink_ext_ack *extack)
> > {
> >         const struct net_device_ops *ops = dev->netdev_ops;
> >         struct hwtstamp_config cfg;
> >         int err;
> >
> >         if (!netif_device_present(dev))
> >                 return -ENODEV;
> >
> >         if (ops->ndo_hwtstamp_get)
> >                 return ops->ndo_hwtstamp_get(dev, cfg, extack);
> >
> >         if (!cfg->ifr)
> >                 return -EOPNOTSUPP;
> >
> >         err = dev_eth_ioctl(dev, cfg->ifr, SIOCGHWTSTAMP);
> >         if (err)
> >             return err;
> >
> >         if (copy_from_user(&cfg, cfg->ifr->ifr_data, sizeof(cfg)))
> >                 return -EFAULT;
> >
> >         hwtstamp_config_to_kernel(kernel_cfg, &cfg);
> >
> >         return 0;
> > }
>
> Side note, it doesn't look like this code is particularly compilable
> either - "cfg" is used in some places instead of "kernel_cfg".

That's true, this code wouldn't compile. I copied it here to
illustrate the potentially concerning logic.
Thank you for pointing this out though!

>
> >
> > It looks like there is a possibility that the returned hwtstamp_config structure
> > will be copied twice to ifr and copied once from ifr on the return path
> > in case if the underlying driver does not implement ndo_hwtstamp_get():
> > - the underlying driver calls copy_to_user() inside its ndo_eth_ioctl()
> >   implementation to return the data to generic_hwtstamp_get_lower();
> > - then generic_hwtstamp_get_lower() calls copy_from_user() to copy it
> >   back out of the ifr to kernel_hwtstamp_config structure;
> > - then dev_get_hwtstamp() calls copy_to_user() again to update
> >   the same ifr with the same data the ifr already contains.
> >
> > Should we consider this acceptable?
>
> Thanks for laying this out. I guess with a table it's going to be
> clearer, so to summarize, I believe this is the status:
>
> Assuming we convert *vlan to ndo_hwtstamp_set():
>
> ===================
>
> If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev
> driver uses ndo_eth_ioctl(), we have:
> - one copy_from_user() in dev_set_hwtstamp()
> - one copy_from_user() in the real_dev's ndo_eth_ioctl()
> - one copy_to_user() in the real_dev's ndo_eth_ioctl()
> - one copy_from_user() in generic_hwtstamp_get_lower()
> - one copy_to_user() in dev_set_hwtstamp()
>
> If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev
> is converted too, we have:
> - one copy_from_user() in dev_set_hwtstamp()
> - one copy_to_user() in dev_set_hwtstamp()
>
> ===================
>
> Assuming we don't convert *vlan to ndo_hwtstamp_set():
>
> ===================
>
> If the vlan driver isn't converted to ndo_hwtstamp_set() and the
> real_dev driver isn't converted either, we have:
> - one copy_from_user() in dev_set_hwtstamp()
> - one copy_from_user() in the real_dev's ndo_eth_ioctl()
> - one copy_to_user() in the real_dev's ndo_eth_ioctl()
>
> If the vlan driver isn't converted to ndo_hwtstamp_set(), but the
> real_dev driver is, we have:
> - one copy_from_user() in dev_set_hwtstamp()
> - one copy_from_user() in the vlan's ndo_eth_ioctl()
> - one copy_to_user() in the vlan's ndo_eth_ioctl()
>
> ===================
>
> So between converting and not converting the *vlans to ndo_hwtstamp_set(),
> the worst case is going to be worse (with a mix of new API in *vlan and
> old API in real_dev) and the best case is going to be better (with new
> API in both *vlan and real_dev). OTOH, with old API in *vlan, the number
> of copies to/from the user buffer is going to be constant at 3, which is
> not the best, not the worst.
>
> I guess the data indicates that we should convert the *vlans to
> ndo_hwtstamp_set() at the very end of the process, and for now, just
> make them compatible with a real_dev that uses the new API?
>
> Note that I haven't done the math for the "get" operation yet, but I
> believe it to be similar.

Thank you for putting this overhead tracking table together!

Here is my guess on how this accounting would look like
for the "get" codepath:

Assuming we convert *vlan to ndo_hwtstamp_set():

===================

If the vlan driver is converted to ndo_hwtstamp_get() and the real_dev
driver uses ndo_eth_ioctl(), we have:
- one copy_to_user() in the real_dev's ndo_eth_ioctl()
- one copy_from_user() in generic_hwtstamp_get_lower()
- one copy_to_user() in dev_get_hwtstamp()

If the vlan driver is converted to ndo_hwtstamp_get() and the real_dev
is converted too, we have:
- one copy_to_user() in dev_get_hwtstamp()

===================

Assuming we don't convert *vlan to ndo_hwtstamp_get():

===================

If the vlan driver isn't converted to ndo_hwtstamp_get() and the
real_dev driver isn't converted either, we have:
- one copy_to_user() in the real_dev's ndo_eth_ioctl()

If the vlan driver isn't converted to ndo_hwtstamp_get(), but the
real_dev driver is, we have:
- one copy_to_user() in the vlan's ndo_eth_ioctl()

===================

Adding real_dev->ndo_hwtstamp_get/set() support to vlan_eth_ioctl()
and holding back with ndo_hwtstamp_get/set() implementation in vlan
code looks like a winner again.

If I may, there are other ways to work around this inefficiency.
Since kernel_hwtstamp_config was meant to be easily extendable,
we can abuse it and add a flag field there. One of the flag values
can indicate that the operation result structure was already copied
to kernel_config->ifr by the function that received this kernel_config
instance as a parameter, and that the content of the
hwtstamp_config-related fields in kernel_config structure must
be ignored when the function returns. It would complicate the
implementation logic, but we'd avoid some unnecessary copy
operations while converting *vlan components to the newer interface.
Would it be a completely unreasonable approach?

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Vladimir Oltean]

On Thu, Apr 06, 2023 at 10:18:36AM -0600, Max Georgiev wrote:
> If I may, there are other ways to work around this inefficiency.
> Since kernel_hwtstamp_config was meant to be easily extendable,
> we can abuse it and add a flag field there. One of the flag values
> can indicate that the operation result structure was already copied
> to kernel_config->ifr by the function that received this kernel_config
> instance as a parameter, and that the content of the
> hwtstamp_config-related fields in kernel_config structure must
> be ignored when the function returns. It would complicate the
> implementation logic, but we'd avoid some unnecessary copy
> operations while converting *vlan components to the newer interface.
> Would it be a completely unreasonable approach?

No, I think that's fair game and a good idea. It would make the best
case better (SET request on a converted real_dev drops from 3 copies to 2),
while keeping the worst case the same (SET request on an unconverted
real_dev remains at 3 copies).

Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Richard Cochran]

On Thu, Apr 06, 2023 at 12:21:37AM -0600, Max Georgiev wrote:

> It looks like there is a possibility that the returned hwtstamp_config structure
> will be copied twice to ifr and copied once from ifr on the return path
> in case if the underlying driver does not implement ndo_hwtstamp_get():
> - the underlying driver calls copy_to_user() inside its ndo_eth_ioctl()
>   implementation to return the data to generic_hwtstamp_get_lower();
> - then generic_hwtstamp_get_lower() calls copy_from_user() to copy it
>   back out of the ifr to kernel_hwtstamp_config structure;
> - then dev_get_hwtstamp() calls copy_to_user() again to update
>   the same ifr with the same data the ifr already contains.
> 
> Should we consider this acceptable?

This is a slow path so copying a small structure is not a concern.

Thanks,
Richard