[PATCH v2 1/2] net: ethernet: i40e: fix build error

[PATCH v2 1/2] net: ethernet: i40e: fix build error

[Wang Dongsheng]

Remove "inline" from __i40e_add_stat_strings.

In file included from
drivers/net/ethernet/intel/i40e/i40e_ethtool.c:9:0:
drivers/net/ethernet/intel/i40e/i40e_ethtool.c: In function
‘__i40e_add_stat_strings’:
drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h:193:20: error:
function ‘__i40e_add_stat_strings’ can never be inlined because it uses
variable argument lists
 static inline void __i40e_add_stat_strings(u8 **p, const struct
					    i40e_stats stats[],

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
v2:
1. Move function.
2. Include a new patch at [2/2].

---
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 24 ++++++++++++++++++
 .../ethernet/intel/i40e/i40e_ethtool_stats.h  | 25 ++-----------------
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index d7d3974beca2..f4a70d67a80a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -1821,6 +1821,30 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
 		  "ethtool stats count mismatch!");
 }
 
+/**
+ * __i40e_add_stat_strings - copy stat strings into ethtool buffer
+ * @p: ethtool supplied buffer
+ * @stats: stat definitions array
+ * @size: size of the stats array
+ *
+ * Format and copy the strings described by stats into the buffer pointed at
+ * by p.
+ **/
+void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
+			     const unsigned int size, ...)
+{
+	unsigned int i;
+
+	for (i = 0; i < size; i++) {
+		va_list args;
+
+		va_start(args, size);
+		vsnprintf(*p, ETH_GSTRING_LEN, stats[i].stat_string, args);
+		*p += ETH_GSTRING_LEN;
+		va_end(args);
+	}
+}
+
 /**
  * i40e_get_stat_strings - copy stat strings into supplied buffer
  * @netdev: the netdev to collect strings for
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h b/drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h
index bba1cb0b658f..0874c352136a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h
@@ -181,29 +181,8 @@ i40e_add_queue_stats(u64 **data, struct i40e_ring *ring)
 	*data += size;
 }
 
-/**
- * __i40e_add_stat_strings - copy stat strings into ethtool buffer
- * @p: ethtool supplied buffer
- * @stats: stat definitions array
- * @size: size of the stats array
- *
- * Format and copy the strings described by stats into the buffer pointed at
- * by p.
- **/
-static inline void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
-				    const unsigned int size, ...)
-{
-	unsigned int i;
-
-	for (i = 0; i < size; i++) {
-		va_list args;
-
-		va_start(args, size);
-		vsnprintf(*p, ETH_GSTRING_LEN, stats[i].stat_string, args);
-		*p += ETH_GSTRING_LEN;
-		va_end(args);
-	}
-}
+void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
+			     const unsigned int size, ...);
 
 /**
  * 40e_add_stat_strings - copy stat strings into ethtool buffer
-- 
2.18.0

[PATCH v2 2/2] net: ethernet: i40evf: fix potential build error

[Wang Dongsheng]

Can't have non-inline function in a header file.
There is a risk of "Multiple definition" from cross-including.

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 .../intel/i40evf/i40e_ethtool_stats.h         | 25 ++-----------------
 .../ethernet/intel/i40evf/i40evf_ethtool.c    | 24 ++++++++++++++++++
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40e_ethtool_stats.h b/drivers/net/ethernet/intel/i40evf/i40e_ethtool_stats.h
index 60b595dd8c39..d70a071f065f 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_ethtool_stats.h
+++ b/drivers/net/ethernet/intel/i40evf/i40e_ethtool_stats.h
@@ -181,29 +181,8 @@ i40evf_add_queue_stats(u64 **data, struct i40e_ring *ring)
 	*data += size;
 }
 
-/**
- * __i40e_add_stat_strings - copy stat strings into ethtool buffer
- * @p: ethtool supplied buffer
- * @stats: stat definitions array
- * @size: size of the stats array
- *
- * Format and copy the strings described by stats into the buffer pointed at
- * by p.
- **/
-static void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
-				    const unsigned int size, ...)
-{
-	unsigned int i;
-
-	for (i = 0; i < size; i++) {
-		va_list args;
-
-		va_start(args, size);
-		vsnprintf(*p, ETH_GSTRING_LEN, stats[i].stat_string, args);
-		*p += ETH_GSTRING_LEN;
-		va_end(args);
-	}
-}
+void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
+			     const unsigned int size, ...);
 
 /**
  * 40e_add_stat_strings - copy stat strings into ethtool buffer
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
index 9319971c5c92..c9a54f6de61e 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
@@ -171,6 +171,30 @@ static void i40evf_get_priv_flag_strings(struct net_device *netdev, u8 *data)
 	}
 }
 
+/**
+ * __i40e_add_stat_strings - copy stat strings into ethtool buffer
+ * @p: ethtool supplied buffer
+ * @stats: stat definitions array
+ * @size: size of the stats array
+ *
+ * Format and copy the strings described by stats into the buffer pointed at
+ * by p.
+ **/
+void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
+			     const unsigned int size, ...)
+{
+	unsigned int i;
+
+	for (i = 0; i < size; i++) {
+		va_list args;
+
+		va_start(args, size);
+		vsnprintf(*p, ETH_GSTRING_LEN, stats[i].stat_string, args);
+		*p += ETH_GSTRING_LEN;
+		va_end(args);
+	}
+}
+
 /**
  * i40evf_get_stat_strings - Get stat strings
  * @netdev: network interface device structure
-- 
2.18.0

RE: [PATCH v2 2/2] net: ethernet: i40evf: fix potential build error

[Keller, Jacob E]

> -----Original Message-----
> From: Wang Dongsheng [mailto:dongsheng.wang@hxt-semitech.com]
> Sent: Tuesday, September 04, 2018 7:27 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>;
> sergei.shtylyov@cogentembedded.com
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; davem@davemloft.net; intel-
> wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Wang Dongsheng <dongsheng.wang@hxt-
> semitech.com>
> Subject: [PATCH v2 2/2] net: ethernet: i40evf: fix potential build error
> 
> Can't have non-inline function in a header file.
> There is a risk of "Multiple definition" from cross-including.
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>

Acked-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>  .../intel/i40evf/i40e_ethtool_stats.h         | 25 ++-----------------
>  .../ethernet/intel/i40evf/i40evf_ethtool.c    | 24 ++++++++++++++++++
>  2 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40evf/i40e_ethtool_stats.h
> b/drivers/net/ethernet/intel/i40evf/i40e_ethtool_stats.h
> index 60b595dd8c39..d70a071f065f 100644
> --- a/drivers/net/ethernet/intel/i40evf/i40e_ethtool_stats.h
> +++ b/drivers/net/ethernet/intel/i40evf/i40e_ethtool_stats.h
> @@ -181,29 +181,8 @@ i40evf_add_queue_stats(u64 **data, struct i40e_ring
> *ring)
>  	*data += size;
>  }
> 
> -/**
> - * __i40e_add_stat_strings - copy stat strings into ethtool buffer
> - * @p: ethtool supplied buffer
> - * @stats: stat definitions array
> - * @size: size of the stats array
> - *
> - * Format and copy the strings described by stats into the buffer pointed at
> - * by p.
> - **/
> -static void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
> -				    const unsigned int size, ...)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < size; i++) {
> -		va_list args;
> -
> -		va_start(args, size);
> -		vsnprintf(*p, ETH_GSTRING_LEN, stats[i].stat_string, args);
> -		*p += ETH_GSTRING_LEN;
> -		va_end(args);
> -	}
> -}
> +void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
> +			     const unsigned int size, ...);
> 
>  /**
>   * 40e_add_stat_strings - copy stat strings into ethtool buffer
> diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
> b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
> index 9319971c5c92..c9a54f6de61e 100644
> --- a/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
> @@ -171,6 +171,30 @@ static void i40evf_get_priv_flag_strings(struct
> net_device *netdev, u8 *data)
>  	}
>  }
> 
> +/**
> + * __i40e_add_stat_strings - copy stat strings into ethtool buffer
> + * @p: ethtool supplied buffer
> + * @stats: stat definitions array
> + * @size: size of the stats array
> + *
> + * Format and copy the strings described by stats into the buffer pointed at
> + * by p.
> + **/
> +void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
> +			     const unsigned int size, ...)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < size; i++) {
> +		va_list args;
> +
> +		va_start(args, size);
> +		vsnprintf(*p, ETH_GSTRING_LEN, stats[i].stat_string, args);
> +		*p += ETH_GSTRING_LEN;
> +		va_end(args);
> +	}
> +}
> +
>  /**
>   * i40evf_get_stat_strings - Get stat strings
>   * @netdev: network interface device structure
> --
> 2.18.0

Re: [PATCH v2 2/2] net: ethernet: i40evf: fix potential build error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

Hi Wang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jkirsher-next-queue/dev-queue]
[cannot apply to v4.19-rc2 next-20180906]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Wang-Dongsheng/net-ethernet-i40e-fix-build-error/20180907-063122
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/net/ethernet/intel/i40evf/i40evf_ethtool.o: In function `__i40e_add_stat_strings':
>> i40evf_ethtool.c:(.text+0xeb0): multiple definition of `__i40e_add_stat_strings'
   drivers/net/ethernet/intel/i40e/i40e_ethtool.o:i40e_ethtool.c:(.text+0x5e20): first defined here

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64499 bytes --]

RE: [PATCH v2 1/2] net: ethernet: i40e: fix build error

[Keller, Jacob E]

> -----Original Message-----
> From: Wang Dongsheng [mailto:dongsheng.wang@hxt-semitech.com]
> Sent: Tuesday, September 04, 2018 7:27 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>;
> sergei.shtylyov@cogentembedded.com
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; davem@davemloft.net; intel-
> wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Wang Dongsheng <dongsheng.wang@hxt-
> semitech.com>
> Subject: [PATCH v2 1/2] net: ethernet: i40e: fix build error
> 
> Remove "inline" from __i40e_add_stat_strings.
> 
> In file included from
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c:9:0:
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c: In function
> ‘__i40e_add_stat_strings’:
> drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h:193:20: error:
> function ‘__i40e_add_stat_strings’ can never be inlined because it uses
> variable argument lists
>  static inline void __i40e_add_stat_strings(u8 **p, const struct
> 					    i40e_stats stats[],
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>

Thanks for the fix.

A bit off topic, but these two files in the i40e and i40evf share some code. Is there a good mechanism for sharing these between the two drivers that would allow the modules to be independent? That would be ideal.

Acked-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
> v2:
> 1. Move function.
> 2. Include a new patch at [2/2].
> 
> ---
>  .../net/ethernet/intel/i40e/i40e_ethtool.c    | 24 ++++++++++++++++++
>  .../ethernet/intel/i40e/i40e_ethtool_stats.h  | 25 ++-----------------
>  2 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index d7d3974beca2..f4a70d67a80a 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -1821,6 +1821,30 @@ static void i40e_get_ethtool_stats(struct net_device
> *netdev,
>  		  "ethtool stats count mismatch!");
>  }
> 
> +/**
> + * __i40e_add_stat_strings - copy stat strings into ethtool buffer
> + * @p: ethtool supplied buffer
> + * @stats: stat definitions array
> + * @size: size of the stats array
> + *
> + * Format and copy the strings described by stats into the buffer pointed at
> + * by p.
> + **/
> +void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
> +			     const unsigned int size, ...)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < size; i++) {
> +		va_list args;
> +
> +		va_start(args, size);
> +		vsnprintf(*p, ETH_GSTRING_LEN, stats[i].stat_string, args);
> +		*p += ETH_GSTRING_LEN;
> +		va_end(args);
> +	}
> +}
> +
>  /**
>   * i40e_get_stat_strings - copy stat strings into supplied buffer
>   * @netdev: the netdev to collect strings for
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h
> b/drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h
> index bba1cb0b658f..0874c352136a 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool_stats.h
> @@ -181,29 +181,8 @@ i40e_add_queue_stats(u64 **data, struct i40e_ring
> *ring)
>  	*data += size;
>  }
> 
> -/**
> - * __i40e_add_stat_strings - copy stat strings into ethtool buffer
> - * @p: ethtool supplied buffer
> - * @stats: stat definitions array
> - * @size: size of the stats array
> - *
> - * Format and copy the strings described by stats into the buffer pointed at
> - * by p.
> - **/
> -static inline void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
> -				    const unsigned int size, ...)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < size; i++) {
> -		va_list args;
> -
> -		va_start(args, size);
> -		vsnprintf(*p, ETH_GSTRING_LEN, stats[i].stat_string, args);
> -		*p += ETH_GSTRING_LEN;
> -		va_end(args);
> -	}
> -}
> +void __i40e_add_stat_strings(u8 **p, const struct i40e_stats stats[],
> +			     const unsigned int size, ...);
> 
>  /**
>   * 40e_add_stat_strings - copy stat strings into ethtool buffer
> --
> 2.18.0

RE: [PATCH v2 1/2] net: ethernet: i40e: fix build error

[Keller, Jacob E]

> -----Original Message-----
> From: Wang, Dongsheng [mailto:dongsheng.wang@hxt-semitech.com]
> Sent: Wednesday, September 05, 2018 8:36 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Kirsher, Jeffrey T
> <jeffrey.t.kirsher@intel.com>; sergei.shtylyov@cogentembedded.com
> Cc: davem@davemloft.net; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] net: ethernet: i40e: fix build error
> 
> Yes,  but not only i40e. igb/vf, ixgb/vf also share same code. If we change any of
> them, means we need to broken the whole layout of driver/net/ethernet/intel/ .
> Obviously we can't put header files to $src/include/net. ：|
> 
> Cheers,
> Dongsheng
> 
> 

I'm more worried about how it interacts with modules. For example, we could have i40e and i40evf share some code, but then wouldn't one of them become dependent on the other? i.e. you'd have to load i40e in order to successfully load i40evf? Or you'd have to have some sort of common glue module which you load first, and then load i40e and i40evf after? This also creates some interactions with out-of-tree modules which make it difficult. It would be nice if we could share the code in some way that still resulted in allowing each module to be separate...

And yes, I igb/vf and ixgbe/vf and i40e/vf all share some code though i40e is the most shared, comparatively. It's not an easy undertaking, which is why it's not been done before. For fm10k, the same driver handles both the PF and VF, but I know that users weren't happy about having the driver change a lot when fixing/changing the PF, and thus thinking they might need to update their VF too much.

Thanks,
Jake

Re: [PATCH v2 1/2] net: ethernet: i40e: fix build error

[Andrew Lunn]

> I'm more worried about how it interacts with modules. For example,
> we could have i40e and i40evf share some code, but then wouldn't one
> of them become dependent on the other? i.e. you'd have to load i40e
> in order to successfully load i40evf? Or you'd have to have some
> sort of common glue module which you load first, and then load i40e
> and i40evf after? This also creates some interactions with
> out-of-tree modules which make it difficult. It would be nice if we
> could share the code in some way that still resulted in allowing
> each module to be separate...
 
You have a few options here.

1) A library module, containing shared code. Use EXPORT_SYMBOL_GPL()
in the library module, and the kernel runtime linker will link the
calls into the library. Also, modprobe will ensure the library module
is loaded first, before the driver module.

2) Build time sharing of code. Place the shared code into a .o file,
and link it to both modules.

There is nothing particularly difficult here, this all done lots of
times within the kernel. Just look around and see how others do it.

      Andrew

RE: [PATCH v2 1/2] net: ethernet: i40e: fix build error

[Wyborny, Carolyn]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Andrew Lunn
> Sent: Thursday, September 06, 2018 11:03 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Wang, Dongsheng <dongsheng.wang@hxt-semitech.com>; Kirsher,
> Jeffrey T <jeffrey.t.kirsher@intel.com>;
> sergei.shtylyov@cogentembedded.com; davem@davemloft.net; intel-
> wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] net: ethernet: i40e: fix build error
> 
[..]
> You have a few options here.
> 
> 1) A library module, containing shared code. Use EXPORT_SYMBOL_GPL()
> in the library module, and the kernel runtime linker will link the
> calls into the library. Also, modprobe will ensure the library module
> is loaded first, before the driver module.
> 
> 2) Build time sharing of code. Place the shared code into a .o file,
> and link it to both modules.
> 
> There is nothing particularly difficult here, this all done lots of
> times within the kernel. Just look around and see how others do it.

Thanks Andrew,

Yes, I agree and we do have a team working on doing this.

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation