[PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
identify the 'mask' parameter as known to be constant at compile time,
which is required to use the FIELD_GET() macro.

The forced inlining does the trick for gcc, but for kernel builds with
clang it results in undefined symbols:

drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.o: In function
  `__nfp_eth_set_aneg':
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x787):
  undefined reference to `__compiletime_assert_492'
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x7b1):
  undefined reference to `__compiletime_assert_496'

These __compiletime_assert_xyx() calls would have been optimized away if
the compiler had seen 'mask' as a constant.

Convert nfp_eth_set_bit_config() into a macro, which allows both gcc and
clang to identify 'mask' as a compile time constant.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
I am aware that a lengthy macro is not a pretty solution, I'm open for
better suggestions.

Note: The patch has been build-tested only since I don't have any NFP
hardware.

 .../ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c   | 67 +++++++++++-----------
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
index f6f7c085f8e0..e9c635867918 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
@@ -469,39 +469,40 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed)
 	return nfp_eth_config_commit_end(nsp);
 }
 
-/* Force inline, FIELD_* macroes require masks to be compilation-time known */
-static __always_inline int
-nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
-		       const u64 mask, unsigned int val, const u64 ctrl_bit)
-{
-	union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
-	unsigned int idx = nfp_nsp_config_idx(nsp);
-	u64 reg;
-
-	/* Note: set features were added in ABI 0.14 but the error
-	 *	 codes were initially not populated correctly.
-	 */
-	if (nfp_nsp_get_abi_ver_minor(nsp) < 17) {
-		nfp_err(nfp_nsp_cpp(nsp),
-			"set operations not supported, please update flash\n");
-		return -EOPNOTSUPP;
-	}
-
-	/* Check if we are already in requested state */
-	reg = le64_to_cpu(entries[idx].raw[raw_idx]);
-	if (val == FIELD_GET(mask, reg))
-		return 0;
-
-	reg &= ~mask;
-	reg |= FIELD_PREP(mask, val);
-	entries[idx].raw[raw_idx] = cpu_to_le64(reg);
-
-	entries[idx].control |= cpu_to_le64(ctrl_bit);
-
-	nfp_nsp_config_set_modified(nsp, true);
-
-	return 0;
-}
+#define nfp_eth_set_bit_config(nsp, raw_idx, mask, val, ctrl_bit)	\
+({									\
+	union eth_table_entry *entries = nfp_nsp_config_entries(nsp);	\
+	unsigned int idx = nfp_nsp_config_idx(nsp);			\
+	u64 reg;							\
+	int rc;								\
+									\
+	/* Note: set features were added in ABI 0.14 but the error */	\
+	/*	 codes were initially not populated correctly.	   */	\
+	if (nfp_nsp_get_abi_ver_minor(nsp) < 17) {			\
+		nfp_err(nfp_nsp_cpp(nsp),				\
+			"set operations not supported, please update flash\n"); \
+		rc = -EOPNOTSUPP;					\
+		goto out;						\
+	}								\
+									\
+	rc = 0;								\
+									\
+	/* Check if we are already in requested state */		\
+	reg = le64_to_cpu(entries[idx].raw[raw_idx]);			\
+	if (val == FIELD_GET(mask, reg))				\
+		goto out;						\
+									\
+	reg &= ~mask;							\
+	reg |= FIELD_PREP(mask, val);					\
+	entries[idx].raw[raw_idx] = cpu_to_le64(reg);			\
+									\
+	entries[idx].control |= cpu_to_le64(ctrl_bit);			\
+									\
+	nfp_nsp_config_set_modified(nsp, true);				\
+									\
+out:									\
+	rc;								\
+})
 
 /**
  * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
-- 
2.14.2.920.gcf0c67979c-goog

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Jakub Kicinski]

On Tue,  3 Oct 2017 13:05:46 -0700, Matthias Kaehlcke wrote:
> nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> identify the 'mask' parameter as known to be constant at compile time,
> which is required to use the FIELD_GET() macro.
> 
> The forced inlining does the trick for gcc, but for kernel builds with
> clang it results in undefined symbols:
> 
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.o: In function
>   `__nfp_eth_set_aneg':
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x787):
>   undefined reference to `__compiletime_assert_492'
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x7b1):
>   undefined reference to `__compiletime_assert_496'
> 
> These __compiletime_assert_xyx() calls would have been optimized away if
> the compiler had seen 'mask' as a constant.
> 
> Convert nfp_eth_set_bit_config() into a macro, which allows both gcc and
> clang to identify 'mask' as a compile time constant.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

:(

Is there no chance of fixing the constant propagation in the compiler?

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

El Tue, Oct 03, 2017 at 02:50:00PM -0700 Jakub Kicinski ha dit:

> On Tue,  3 Oct 2017 13:05:46 -0700, Matthias Kaehlcke wrote:
> > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> > identify the 'mask' parameter as known to be constant at compile time,
> > which is required to use the FIELD_GET() macro.
> > 
> > The forced inlining does the trick for gcc, but for kernel builds with
> > clang it results in undefined symbols:
> > 
> > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.o: In function
> >   `__nfp_eth_set_aneg':
> > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x787):
> >   undefined reference to `__compiletime_assert_492'
> > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x7b1):
> >   undefined reference to `__compiletime_assert_496'
> > 
> > These __compiletime_assert_xyx() calls would have been optimized away if
> > the compiler had seen 'mask' as a constant.
> > 
> > Convert nfp_eth_set_bit_config() into a macro, which allows both gcc and
> > clang to identify 'mask' as a compile time constant.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> :(
> 
> Is there no chance of fixing the constant propagation in the compiler?

LLVM developers are reluctant and would like us kernel folks to
evaluate possible alternatives for the affected code first:
https://bugs.llvm.org/show_bug.cgi?id=4898

Given that this doesn't seem to be a widespread issue in the kernel
personally I would consider the conversion to a macro in this case an
acceptable solution, though it is definitely ugly. However I'm not the
owner of the driver or the subsystem, so my opinion doesn't really
carry much weight here ;-)

Any ideas about other, less ugly alternatives?

Matthias

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[David Miller]

From: Matthias Kaehlcke <mka@chromium.org>
Date: Wed, 4 Oct 2017 10:42:15 -0700

> Given that this doesn't seem to be a widespread issue in the kernel
> personally I would consider the conversion to a macro in this case an
> acceptable solution, though it is definitely ugly. However I'm not the
> owner of the driver or the subsystem, so my opinion doesn't really
> carry much weight here ;-)

Losing type checking is a serious regression as far as I am concerned.

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Jakub Kicinski]

On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote:
> Hi Jakub,
> 
> I had discussed about supporting this code with some clang developers.
> However, the consensus was this code relies on a specific GCC optimizer
> behavior and Clang does not share the same behavior by design.

Hm.  I find surprising that constant propagation to inlined functions
is considered something GCC-specific rather than obvious/basic.

> Note that even GCC can't compile this code at -O0.

Yes, that makes me feel uncomfortable...  Could you try this?

-------8<-------

diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
index f6f7c085f8e0..47251396fcae 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
@@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed)
 	return nfp_eth_config_commit_end(nsp);
 }
 
-/* Force inline, FIELD_* macroes require masks to be compilation-time known */
-static __always_inline int
+static int
 nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
-		       const u64 mask, unsigned int val, const u64 ctrl_bit)
+		       const u64 mask, const unsigned int shift,
+		       unsigned int val, const u64 ctrl_bit)
 {
 	union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
 	unsigned int idx = nfp_nsp_config_idx(nsp);
@@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
 
 	/* Check if we are already in requested state */
 	reg = le64_to_cpu(entries[idx].raw[raw_idx]);
-	if (val == FIELD_GET(mask, reg))
+	if (val == (reg & mask) >> shift)
 		return 0;
 
 	reg &= ~mask;
-	reg |= FIELD_PREP(mask, val);
+	reg |= (val << shift) & mask;
 	entries[idx].raw[raw_idx] = cpu_to_le64(reg);
 
 	entries[idx].control |= cpu_to_le64(ctrl_bit);
@@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
 	return 0;
 }
 
+#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)	\
+	({								\
+		__BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \
+		nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \
+				       val, ctrl_bit);			\
+	})
+
 /**
  * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
  * @nsp:	NFP NSP handle returned from nfp_eth_config_start()
@@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
  */
 int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode)
 {
-	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
+	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
 				      NSP_ETH_STATE_ANEG, mode,
 				      NSP_ETH_CTRL_SET_ANEG);
 }
@@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
 		return -EINVAL;
 	}
 
-	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
+	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
 				      NSP_ETH_STATE_RATE, rate,
 				      NSP_ETH_CTRL_SET_RATE);
 }
@@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
  */
 int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
 {
-	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
+	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
 				      lanes, NSP_ETH_CTRL_SET_LANES);
 }
-- 
2.14.1

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

El Wed, Oct 04, 2017 at 11:17:24AM -0700 Jakub Kicinski ha dit:

> On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote:
> > Hi Jakub,
> > 
> > I had discussed about supporting this code with some clang developers.
> > However, the consensus was this code relies on a specific GCC optimizer
> > behavior and Clang does not share the same behavior by design.
> 
> Hm.  I find surprising that constant propagation to inlined functions
> is considered something GCC-specific rather than obvious/basic.
> 
> > Note that even GCC can't compile this code at -O0.
> 
> Yes, that makes me feel uncomfortable...  Could you try this?

The kernel as a whole does not build with -O0, so I couldn't try
that. I know Manoj (who isn't a kernel dev) isolated the
compiletime_assert macros and encountered the same 'undefined
reference' errors with gcc -O0 that we are seeing with clang.
This is due to:

 "if you use it in an inlined function and pass an argument of the
 function as the argument to the built-in, GCC never returns 1 when
 you call the inline function with a string constant or compound
 literal (see Compound Literals) and does not return 1 when you pass a
 constant numeric value to the inline function unless you specify the
 -O option."

https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Other-Builtins.html

> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> index f6f7c085f8e0..47251396fcae 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> @@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed)
>  	return nfp_eth_config_commit_end(nsp);
>  }
>  
> -/* Force inline, FIELD_* macroes require masks to be compilation-time known */
> -static __always_inline int
> +static int
>  nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
> -		       const u64 mask, unsigned int val, const u64 ctrl_bit)
> +		       const u64 mask, const unsigned int shift,
> +		       unsigned int val, const u64 ctrl_bit)
>  {
>  	union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
>  	unsigned int idx = nfp_nsp_config_idx(nsp);
> @@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>  
>  	/* Check if we are already in requested state */
>  	reg = le64_to_cpu(entries[idx].raw[raw_idx]);
> -	if (val == FIELD_GET(mask, reg))
> +	if (val == (reg & mask) >> shift)
>  		return 0;
>  
>  	reg &= ~mask;
> -	reg |= FIELD_PREP(mask, val);
> +	reg |= (val << shift) & mask;
>  	entries[idx].raw[raw_idx] = cpu_to_le64(reg);
>  
>  	entries[idx].control |= cpu_to_le64(ctrl_bit);
> @@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>  	return 0;
>  }
>  
> +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)	\
> +	({								\
> +		__BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \
> +		nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \
> +				       val, ctrl_bit);			\
> +	})
> +
>  /**
>   * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
>   * @nsp:	NFP NSP handle returned from nfp_eth_config_start()
> @@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>   */
>  int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode)
>  {
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
>  				      NSP_ETH_STATE_ANEG, mode,
>  				      NSP_ETH_CTRL_SET_ANEG);
>  }
> @@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
>  		return -EINVAL;
>  	}
>  
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
>  				      NSP_ETH_STATE_RATE, rate,
>  				      NSP_ETH_CTRL_SET_RATE);
>  }
> @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
>   */
>  int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
>  {
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
>  				      lanes, NSP_ETH_CTRL_SET_LANES);
>  }

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

Hi Jakub,

El Wed, Oct 04, 2017 at 11:17:24AM -0700 Jakub Kicinski ha dit:

> On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote:
> > Hi Jakub,
> > 
> > I had discussed about supporting this code with some clang developers.
> > However, the consensus was this code relies on a specific GCC optimizer
> > behavior and Clang does not share the same behavior by design.
> 
> Hm.  I find surprising that constant propagation to inlined functions
> is considered something GCC-specific rather than obvious/basic.
> 
> > Note that even GCC can't compile this code at -O0.
> 
> Yes, that makes me feel uncomfortable...  Could you try this?
> 
> -------8<-------
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> index f6f7c085f8e0..47251396fcae 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> @@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed)
>  	return nfp_eth_config_commit_end(nsp);
>  }
>  
> -/* Force inline, FIELD_* macroes require masks to be compilation-time known */
> -static __always_inline int
> +static int
>  nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
> -		       const u64 mask, unsigned int val, const u64 ctrl_bit)
> +		       const u64 mask, const unsigned int shift,
> +		       unsigned int val, const u64 ctrl_bit)
>  {
>  	union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
>  	unsigned int idx = nfp_nsp_config_idx(nsp);
> @@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>  
>  	/* Check if we are already in requested state */
>  	reg = le64_to_cpu(entries[idx].raw[raw_idx]);
> -	if (val == FIELD_GET(mask, reg))
> +	if (val == (reg & mask) >> shift)
>  		return 0;
>  
>  	reg &= ~mask;
> -	reg |= FIELD_PREP(mask, val);
> +	reg |= (val << shift) & mask;
>  	entries[idx].raw[raw_idx] = cpu_to_le64(reg);
>  
>  	entries[idx].control |= cpu_to_le64(ctrl_bit);
> @@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>  	return 0;
>  }
>  
> +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)	\
> +	({								\
> +		__BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \
> +		nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \
> +				       val, ctrl_bit);			\
> +	})
> +
>  /**
>   * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
>   * @nsp:	NFP NSP handle returned from nfp_eth_config_start()
> @@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>   */
>  int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode)
>  {
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
>  				      NSP_ETH_STATE_ANEG, mode,
>  				      NSP_ETH_CTRL_SET_ANEG);
>  }
> @@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
>  		return -EINVAL;
>  	}
>  
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
>  				      NSP_ETH_STATE_RATE, rate,
>  				      NSP_ETH_CTRL_SET_RATE);
>  }
> @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
>   */
>  int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
>  {
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
>  				      lanes, NSP_ETH_CTRL_SET_LANES);
>  }

Do you plan to send this as an official patch?

Thanks

Matthias

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Jakub Kicinski]

On Tue, 24 Oct 2017 09:56:03 -0700, Matthias Kaehlcke wrote:
> > @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
> >   */
> >  int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
> >  {
> > -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> > +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> >  				      lanes, NSP_ETH_CTRL_SET_LANES);
> >  }  
> 
> Do you plan to send this as an official patch?

Sorry...  Dirk is prepping a larger series for this area of code and it
got stuck there a bit :S  He says it's coming any day now...

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

El Tue, Oct 24, 2017 at 10:03:56AM -0700 Jakub Kicinski ha dit:

> On Tue, 24 Oct 2017 09:56:03 -0700, Matthias Kaehlcke wrote:
> > > @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
> > >   */
> > >  int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
> > >  {
> > > -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> > > +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> > >  				      lanes, NSP_ETH_CTRL_SET_LANES);
> > >  }  
> > 
> > Do you plan to send this as an official patch?
> 
> Sorry...  Dirk is prepping a larger series for this area of code and it
> got stuck there a bit :S

No prob, just wanted to make sure it didn't slip under the radar.

>  He says it's coming any day now...

Great, thanks!

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Joe Perches]

On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote:
> nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> identify the 'mask' parameter as known to be constant at compile time,
> which is required to use the FIELD_GET() macro.
> 
> The forced inlining does the trick for gcc, but for kernel builds with
> clang it results in undefined symbols:

Can't you use local different FIELD_PREP/FIELD_GET macros
with a different name without the BUILD_BUG tests?

i.e.:

#define NFP_FIELD_PREP(_mask, _val)				\
({								\
	((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
})

#define NFP_FIELD_GET(_mask, _reg)				\
({								\
	(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
})

Then the __always_inline can be removed from
nfp_eth_set_bit_config too.

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

Hi Joe,

El Wed, Oct 04, 2017 at 11:07:19AM -0700 Joe Perches ha dit:

> On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote:
> > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> > identify the 'mask' parameter as known to be constant at compile time,
> > which is required to use the FIELD_GET() macro.
> > 
> > The forced inlining does the trick for gcc, but for kernel builds with
> > clang it results in undefined symbols:
> 
> Can't you use local different FIELD_PREP/FIELD_GET macros
> with a different name without the BUILD_BUG tests?
> 
> i.e.:
> 
> #define NFP_FIELD_PREP(_mask, _val)				\
> ({								\
> 	((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
> })
> 
> #define NFP_FIELD_GET(_mask, _reg)				\
> ({								\
> 	(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
> })
> 
> Then the __always_inline can be removed from
> nfp_eth_set_bit_config too.

Thanks for the suggestion. This seems a viable alternative if David
and the NFP owners can live without the extra checking provided by
__BF_FIELD_CHECK.

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Jakub Kicinski]

On Wed, 4 Oct 2017 11:49:57 -0700, Matthias Kaehlcke wrote:
> Hi Joe,
> 
> El Wed, Oct 04, 2017 at 11:07:19AM -0700 Joe Perches ha dit:
> 
> > On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote:  
> > > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> > > identify the 'mask' parameter as known to be constant at compile time,
> > > which is required to use the FIELD_GET() macro.
> > > 
> > > The forced inlining does the trick for gcc, but for kernel builds with
> > > clang it results in undefined symbols:  
> > 
> > Can't you use local different FIELD_PREP/FIELD_GET macros
> > with a different name without the BUILD_BUG tests?
> > 
> > i.e.:
> > 
> > #define NFP_FIELD_PREP(_mask, _val)				\
> > ({								\
> > 	((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
> > })
> > 
> > #define NFP_FIELD_GET(_mask, _reg)				\
> > ({								\
> > 	(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
> > })
> > 
> > Then the __always_inline can be removed from
> > nfp_eth_set_bit_config too.  
> 
> Thanks for the suggestion. This seems a viable alternative if David
> and the NFP owners can live without the extra checking provided by
> __BF_FIELD_CHECK.

The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
is that it will require runtime ffs on the mask, which is potentially
costly.  I would also feel quite stupid adding those macros to the nfp
driver, given that I specifically created the bitfield.h header to not
have to reimplement these in every driver I write/maintain.

Can you please test the patch I provided in the other reply?

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

Hi Jakub,

El Wed, Oct 04, 2017 at 03:22:03PM -0700 Jakub Kicinski ha dit:

> On Wed, 4 Oct 2017 11:49:57 -0700, Matthias Kaehlcke wrote:
> > Hi Joe,
> > 
> > El Wed, Oct 04, 2017 at 11:07:19AM -0700 Joe Perches ha dit:
> > 
> > > On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote:  
> > > > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> > > > identify the 'mask' parameter as known to be constant at compile time,
> > > > which is required to use the FIELD_GET() macro.
> > > > 
> > > > The forced inlining does the trick for gcc, but for kernel builds with
> > > > clang it results in undefined symbols:  
> > > 
> > > Can't you use local different FIELD_PREP/FIELD_GET macros
> > > with a different name without the BUILD_BUG tests?
> > > 
> > > i.e.:
> > > 
> > > #define NFP_FIELD_PREP(_mask, _val)				\
> > > ({								\
> > > 	((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
> > > })
> > > 
> > > #define NFP_FIELD_GET(_mask, _reg)				\
> > > ({								\
> > > 	(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
> > > })
> > > 
> > > Then the __always_inline can be removed from
> > > nfp_eth_set_bit_config too.  
> > 
> > Thanks for the suggestion. This seems a viable alternative if David
> > and the NFP owners can live without the extra checking provided by
> > __BF_FIELD_CHECK.
> 
> The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
> is that it will require runtime ffs on the mask, which is potentially
> costly.  I would also feel quite stupid adding those macros to the nfp
> driver, given that I specifically created the bitfield.h header to not
> have to reimplement these in every driver I write/maintain.

That make sense, thanks for providing more context.

> Can you please test the patch I provided in the other reply?

With this patch there are no errors when building the kernel with
clang.

Thanks!

Matthias

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Jakub Kicinski]

On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote:
> > > Thanks for the suggestion. This seems a viable alternative if David
> > > and the NFP owners can live without the extra checking provided by
> > > __BF_FIELD_CHECK.  
> > 
> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
> > is that it will require runtime ffs on the mask, which is potentially
> > costly.  I would also feel quite stupid adding those macros to the nfp
> > driver, given that I specifically created the bitfield.h header to not
> > have to reimplement these in every driver I write/maintain.  
> 
> That make sense, thanks for providing more context.
> 
> > Can you please test the patch I provided in the other reply?  
> 
> With this patch there are no errors when building the kernel with
> clang.

Cool, thanks for checking!  I will run it through full tests and queue
for upstreaming :)

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Manoj Gupta]

Hi Jakub,

On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote:
>> > > Thanks for the suggestion. This seems a viable alternative if David
>> > > and the NFP owners can live without the extra checking provided by
>> > > __BF_FIELD_CHECK.
>> >
>> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
>> > is that it will require runtime ffs on the mask, which is potentially
>> > costly.  I would also feel quite stupid adding those macros to the nfp
>> > driver, given that I specifically created the bitfield.h header to not
>> > have to reimplement these in every driver I write/maintain.
>>
>> That make sense, thanks for providing more context.
>>
>> > Can you please test the patch I provided in the other reply?
>>
>> With this patch there are no errors when building the kernel with
>> clang.
>
> Cool, thanks for checking!  I will run it through full tests and queue
> for upstreaming :)

Just to let you know, using __BF_FIELD_CHECK macro will not Link with
-O0 (GCC or Clang)  since references to __compiletime_assert_xxx will
not be cleaned up.

Thanks,
Manoj

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Jakub Kicinski]

On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote:
> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote:
> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote:  
> >> > > Thanks for the suggestion. This seems a viable alternative if David
> >> > > and the NFP owners can live without the extra checking provided by
> >> > > __BF_FIELD_CHECK.  
> >> >
> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
> >> > is that it will require runtime ffs on the mask, which is potentially
> >> > costly.  I would also feel quite stupid adding those macros to the nfp
> >> > driver, given that I specifically created the bitfield.h header to not
> >> > have to reimplement these in every driver I write/maintain.  
> >>
> >> That make sense, thanks for providing more context.
> >>  
> >> > Can you please test the patch I provided in the other reply?  
> >>
> >> With this patch there are no errors when building the kernel with
> >> clang.  
> >
> > Cool, thanks for checking!  I will run it through full tests and queue
> > for upstreaming :)  
> 
> Just to let you know, using __BF_FIELD_CHECK macro will not Link with
> -O0 (GCC or Clang)  since references to __compiletime_assert_xxx will
> not be cleaned up.

Do you mean the current nfp_eth_set_bit_config() will not work with -O0
on either complier, or any use of __BF_FIELD_CHECK() will not compile
with -O0?

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Manoj Gupta]

On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote:
>> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote:
>> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote:
>> >> > > Thanks for the suggestion. This seems a viable alternative if David
>> >> > > and the NFP owners can live without the extra checking provided by
>> >> > > __BF_FIELD_CHECK.
>> >> >
>> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
>> >> > is that it will require runtime ffs on the mask, which is potentially
>> >> > costly.  I would also feel quite stupid adding those macros to the nfp
>> >> > driver, given that I specifically created the bitfield.h header to not
>> >> > have to reimplement these in every driver I write/maintain.
>> >>
>> >> That make sense, thanks for providing more context.
>> >>
>> >> > Can you please test the patch I provided in the other reply?
>> >>
>> >> With this patch there are no errors when building the kernel with
>> >> clang.
>> >
>> > Cool, thanks for checking!  I will run it through full tests and queue
>> > for upstreaming :)
>>
>> Just to let you know, using __BF_FIELD_CHECK macro will not Link with
>> -O0 (GCC or Clang)  since references to __compiletime_assert_xxx will
>> not be cleaned up.
>
> Do you mean the current nfp_eth_set_bit_config() will not work with -O0
> on either complier, or any use of __BF_FIELD_CHECK() will not compile
> with -O0?

Any use of __BF_FIELD_CHECK. The code will compile but not link since
calls to ____compiletime_assert_xxx (added by compiletime_assert
macro) will not be removed in -O0.

Thanks,
Manoj

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Jakub Kicinski]

On Wed, 4 Oct 2017 18:50:04 -0700, Manoj Gupta wrote:
> On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski wrote:
> > On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote:  
> >> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote:  
> >> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote:  
> >> >> > > Thanks for the suggestion. This seems a viable alternative if David
> >> >> > > and the NFP owners can live without the extra checking provided by
> >> >> > > __BF_FIELD_CHECK.  
> >> >> >
> >> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
> >> >> > is that it will require runtime ffs on the mask, which is potentially
> >> >> > costly.  I would also feel quite stupid adding those macros to the nfp
> >> >> > driver, given that I specifically created the bitfield.h header to not
> >> >> > have to reimplement these in every driver I write/maintain.  
> >> >>
> >> >> That make sense, thanks for providing more context.
> >> >>  
> >> >> > Can you please test the patch I provided in the other reply?  
> >> >>
> >> >> With this patch there are no errors when building the kernel with
> >> >> clang.  
> >> >
> >> > Cool, thanks for checking!  I will run it through full tests and queue
> >> > for upstreaming :)  
> >>
> >> Just to let you know, using __BF_FIELD_CHECK macro will not Link with
> >> -O0 (GCC or Clang)  since references to __compiletime_assert_xxx will
> >> not be cleaned up.  
> >
> > Do you mean the current nfp_eth_set_bit_config() will not work with -O0
> > on either complier, or any use of __BF_FIELD_CHECK() will not compile
> > with -O0?  
> 
> Any use of __BF_FIELD_CHECK. The code will compile but not link since
> calls to ____compiletime_assert_xxx (added by compiletime_assert
> macro) will not be removed in -O0.

Why would that be, it's just a macro?  Does it by extension mean any
use of BUILD_BUG_ON_MSG() will not compile with -O0?

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Manoj Gupta]

On Wed, Oct 4, 2017 at 7:06 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Wed, 4 Oct 2017 18:50:04 -0700, Manoj Gupta wrote:
>> On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski wrote:
>> > On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote:
>> >> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote:
>> >> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote:
>> >> >> > > Thanks for the suggestion. This seems a viable alternative if David
>> >> >> > > and the NFP owners can live without the extra checking provided by
>> >> >> > > __BF_FIELD_CHECK.
>> >> >> >
>> >> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
>> >> >> > is that it will require runtime ffs on the mask, which is potentially
>> >> >> > costly.  I would also feel quite stupid adding those macros to the nfp
>> >> >> > driver, given that I specifically created the bitfield.h header to not
>> >> >> > have to reimplement these in every driver I write/maintain.
>> >> >>
>> >> >> That make sense, thanks for providing more context.
>> >> >>
>> >> >> > Can you please test the patch I provided in the other reply?
>> >> >>
>> >> >> With this patch there are no errors when building the kernel with
>> >> >> clang.
>> >> >
>> >> > Cool, thanks for checking!  I will run it through full tests and queue
>> >> > for upstreaming :)
>> >>
>> >> Just to let you know, using __BF_FIELD_CHECK macro will not Link with
>> >> -O0 (GCC or Clang)  since references to __compiletime_assert_xxx will
>> >> not be cleaned up.
>> >
>> > Do you mean the current nfp_eth_set_bit_config() will not work with -O0
>> > on either complier, or any use of __BF_FIELD_CHECK() will not compile
>> > with -O0?
>>
>> Any use of __BF_FIELD_CHECK. The code will compile but not link since
>> calls to ____compiletime_assert_xxx (added by compiletime_assert
>> macro) will not be removed in -O0.
>
> Why would that be, it's just a macro?  Does it by extension mean any
> use of BUILD_BUG_ON_MSG() will not compile with -O0?

You have to look at the the code added once the macro is expanded :).
Please look at implementation of compiletime_assert at
http://elixir.free-electrons.com/linux/v4.12.14/source/include/linux/compiler.h#L507
It creates a call to __compiler_assert_xxx inside a loop which is not
cleaned up in -O0.

Thanks,
Manoj

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

El Wed, Oct 04, 2017 at 07:13:26PM -0700 Manoj Gupta ha dit:

> On Wed, Oct 4, 2017 at 7:06 PM, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> > On Wed, 4 Oct 2017 18:50:04 -0700, Manoj Gupta wrote:
> >> On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski wrote:
> >> > On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote:
> >> >> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote:
> >> >> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote:
> >> >> >> > > Thanks for the suggestion. This seems a viable alternative if David
> >> >> >> > > and the NFP owners can live without the extra checking provided by
> >> >> >> > > __BF_FIELD_CHECK.
> >> >> >> >
> >> >> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
> >> >> >> > is that it will require runtime ffs on the mask, which is potentially
> >> >> >> > costly.  I would also feel quite stupid adding those macros to the nfp
> >> >> >> > driver, given that I specifically created the bitfield.h header to not
> >> >> >> > have to reimplement these in every driver I write/maintain.
> >> >> >>
> >> >> >> That make sense, thanks for providing more context.
> >> >> >>
> >> >> >> > Can you please test the patch I provided in the other reply?
> >> >> >>
> >> >> >> With this patch there are no errors when building the kernel with
> >> >> >> clang.
> >> >> >
> >> >> > Cool, thanks for checking!  I will run it through full tests and queue
> >> >> > for upstreaming :)
> >> >>
> >> >> Just to let you know, using __BF_FIELD_CHECK macro will not Link with
> >> >> -O0 (GCC or Clang)  since references to __compiletime_assert_xxx will
> >> >> not be cleaned up.
> >> >
> >> > Do you mean the current nfp_eth_set_bit_config() will not work with -O0
> >> > on either complier, or any use of __BF_FIELD_CHECK() will not compile
> >> > with -O0?
> >>
> >> Any use of __BF_FIELD_CHECK. The code will compile but not link since
> >> calls to ____compiletime_assert_xxx (added by compiletime_assert
> >> macro) will not be removed in -O0.
> >
> > Why would that be, it's just a macro?  Does it by extension mean any
> > use of BUILD_BUG_ON_MSG() will not compile with -O0?
> 
> You have to look at the the code added once the macro is expanded :).
> Please look at implementation of compiletime_assert at
> http://elixir.free-electrons.com/linux/v4.12.14/source/include/linux/compiler.h#L507
> It creates a call to __compiler_assert_xxx inside a loop which is not
> cleaned up in -O0.

I just saw that v4.14 will have a fix for that:

commit c03567a8e8d5cf2aaca40e605c48f319dc2ead57
Author: Joe Stringer <joe@ovn.org>
Date:   Thu Aug 31 16:15:33 2017 -0700

include/linux/compiler.h: don't perform compiletime_assert with -O0


Obviously this means that the checks aren't performed, however that
shouldn't be an issue since AFAIK the kernel doesn't officially
support -O0 builds in the first place.