[PATCH v2 0/2 net] bitfield.h cleanups

[PATCH v2 0/2 net] bitfield.h cleanups

[Nick Desaulniers]

Two patches, one that removes a BUILD_BUG_ON for a case that is not a
compile time bug (exposed by compiler optimization).

The second is a general cleanup in the area.

I decided to leave the BUILD_BUG_ON case first, as I hope it will
simplify being able to backport it to stable, and because I don't think
there's any type promotion+conversion bugs there.

Though it would be nice to use consistent types widths and signedness,
equality against literal zero is not an issue.

Jakub Kicinski (1):
  bitfield.h: don't compile-time validate _val in FIELD_FIT

Nick Desaulniers (1):
  bitfield.h: split up __BF_FIELD_CHECK macro

 .../netronome/nfp/nfpcore/nfp_nsp_eth.c       | 11 ++++----
 include/linux/bitfield.h                      | 26 +++++++++++++------
 2 files changed, 24 insertions(+), 13 deletions(-)

-- 
2.27.0.383.g050319c2ae-goog

[PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT

[Nick Desaulniers]

From: Jakub Kicinski <kuba@kernel.org>

When ur_load_imm_any() is inlined into jeq_imm(), it's possible for the
compiler to deduce a case where _val can only have the value of -1 at
compile time. Specifically,

/* struct bpf_insn: _s32 imm */
u64 imm = insn->imm; /* sign extend */
if (imm >> 32) { /* non-zero only if insn->imm is negative */
  /* inlined from ur_load_imm_any */
  u32 __imm = imm >> 32; /* therefore, always 0xffffffff */
  if (__builtin_constant_p(__imm) && __imm > 255)
    compiletime_assert_XXX()

This can result in tripping a BUILD_BUG_ON() in __BF_FIELD_CHECK() that
checks that a given value is representable in one byte (interpreted as
unsigned).

FIELD_FIT() should return true or false at runtime for whether a value
can fit for not. Don't break the build over a value that's too large for
the mask. We'd prefer to keep the inlining and compiler optimizations
though we know this case will always return false.

Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/kernel-hardening/CAK7LNASvb0UDJ0U5wkYYRzTAdnEs64HjXpEUL7d=V0CXiAXcNw@mail.gmail.com/
Reported-by: Masahiro Yamada <masahiroy@kernel.org>
Debugged-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V1->V2:
* None

 include/linux/bitfield.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 48ea093ff04c..4e035aca6f7e 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -77,7 +77,7 @@
  */
 #define FIELD_FIT(_mask, _val)						\
 	({								\
-		__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_FIT: ");	\
+		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: ");	\
 		!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
 	})
 
-- 
2.27.0.383.g050319c2ae-goog

[PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro

[Nick Desaulniers]

This macro has a few expansion sites that pass literal 0s as parameters.
Split these up so that we do the precise checks where we care about
them.

Suggested-by: Alex Elder <elder@linaro.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V1-V2:
* New patch in v2.
* Rebased on 0001.

 .../netronome/nfp/nfpcore/nfp_nsp_eth.c       | 11 ++++----
 include/linux/bitfield.h                      | 26 +++++++++++++------
 2 files changed, 24 insertions(+), 13 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 311a5be25acb..938fc733fccb 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
@@ -492,11 +492,12 @@ 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);			\
+#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)		\
+	({									\
+		__BF_FIELD_CHECK_MASK(mask, "NFP_ETH_SET_BIT_CONFIG: ");	\
+		__BF_FIELD_CHECK_VAL(mask, val, "NFP_ETH_SET_BIT_CONFIG: ");	\
+		nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask),	\
+				       val, ctrl_bit);				\
 	})
 
 /**
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 4e035aca6f7e..79651867beb3 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -41,18 +41,26 @@
 
 #define __bf_shf(x) (__builtin_ffsll(x) - 1)
 
-#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)			\
+#define __BF_FIELD_CHECK_MASK(_mask, _pfx)				\
 	({								\
 		BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),		\
 				 _pfx "mask is not constant");		\
 		BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");	\
+		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
+					      (1ULL << __bf_shf(_mask))); \
+	})
+
+#define __BF_FIELD_CHECK_VAL(_mask, _val, _pfx)				\
+	({								\
 		BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?		\
 				 ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
 				 _pfx "value too large for the field"); \
-		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull,		\
+	})
+
+#define __BF_FIELD_CHECK_REG(_mask, _reg, _pfx)				\
+	({								\
+		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ULL,		\
 				 _pfx "type of reg too small for mask"); \
-		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
-					      (1ULL << __bf_shf(_mask))); \
 	})
 
 /**
@@ -64,7 +72,7 @@
  */
 #define FIELD_MAX(_mask)						\
 	({								\
-		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: ");	\
+		__BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: ");		\
 		(typeof(_mask))((_mask) >> __bf_shf(_mask));		\
 	})
 
@@ -77,7 +85,7 @@
  */
 #define FIELD_FIT(_mask, _val)						\
 	({								\
-		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: ");	\
+		__BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: ");		\
 		!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
 	})
 
@@ -91,7 +99,8 @@
  */
 #define FIELD_PREP(_mask, _val)						\
 	({								\
-		__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");	\
+		__BF_FIELD_CHECK_MASK(_mask, "FIELD_PREP: ");		\
+		__BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_PREP: ");	\
 		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
 	})
 
@@ -105,7 +114,8 @@
  */
 #define FIELD_GET(_mask, _reg)						\
 	({								\
-		__BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: ");	\
+		__BF_FIELD_CHECK_MASK(_mask, "FIELD_GET: ");		\
+		__BF_FIELD_CHECK_REG(_mask, _reg,  "FIELD_GET: ");	\
 		(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
 	})
 
-- 
2.27.0.383.g050319c2ae-goog

Re: [PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT

[Alex Elder]

On 7/8/20 6:04 PM, Nick Desaulniers wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> When ur_load_imm_any() is inlined into jeq_imm(), it's possible for the
> compiler to deduce a case where _val can only have the value of -1 at
> compile time. Specifically,
> 
> /* struct bpf_insn: _s32 imm */
> u64 imm = insn->imm; /* sign extend */
> if (imm >> 32) { /* non-zero only if insn->imm is negative */
>   /* inlined from ur_load_imm_any */
>   u32 __imm = imm >> 32; /* therefore, always 0xffffffff */
>   if (__builtin_constant_p(__imm) && __imm > 255)
>     compiletime_assert_XXX()
> 
> This can result in tripping a BUILD_BUG_ON() in __BF_FIELD_CHECK() that
> checks that a given value is representable in one byte (interpreted as
> unsigned).

Looking at the 12 other callers of FIELD_FIT(), it's hard to
tell but it appears most of them don't supply constant _val
to the macro.  So maybe relaxing this check is no great loss.

I feel like I need to look much more deeply into this to call
it a review, so:

Acked-by: Alex Elder <elder@linaro.org>

> FIELD_FIT() should return true or false at runtime for whether a value
> can fit for not. Don't break the build over a value that's too large for
> the mask. We'd prefer to keep the inlining and compiler optimizations
> though we know this case will always return false.
> 
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/kernel-hardening/CAK7LNASvb0UDJ0U5wkYYRzTAdnEs64HjXpEUL7d=V0CXiAXcNw@mail.gmail.com/
> Reported-by: Masahiro Yamada <masahiroy@kernel.org>
> Debugged-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes V1->V2:
> * None
> 
>  include/linux/bitfield.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 48ea093ff04c..4e035aca6f7e 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -77,7 +77,7 @@
>   */
>  #define FIELD_FIT(_mask, _val)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_FIT: ");	\
> +		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: ");	\
>  		!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
>  	})
>  
> 

Re: [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro

[Alex Elder]

On 7/8/20 6:04 PM, Nick Desaulniers wrote:
> This macro has a few expansion sites that pass literal 0s as parameters.
> Split these up so that we do the precise checks where we care about
> them.
> 
> Suggested-by: Alex Elder <elder@linaro.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

I do like this better.  It makes it more obvious when only
the mask is being verified (in FIELD_FIT() for example).
I also appreciate that you distinguished the register
checks from the value checks.
- field masks must be (2^x - 1) << y, x > 0, and constant
- values must be representable within a field mask
- registers must be able to represent the field mask

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
> Changes V1-V2:
> * New patch in v2.
> * Rebased on 0001.
> 
>  .../netronome/nfp/nfpcore/nfp_nsp_eth.c       | 11 ++++----
>  include/linux/bitfield.h                      | 26 +++++++++++++------
>  2 files changed, 24 insertions(+), 13 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 311a5be25acb..938fc733fccb 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> @@ -492,11 +492,12 @@ 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);			\
> +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)		\
> +	({									\
> +		__BF_FIELD_CHECK_MASK(mask, "NFP_ETH_SET_BIT_CONFIG: ");	\
> +		__BF_FIELD_CHECK_VAL(mask, val, "NFP_ETH_SET_BIT_CONFIG: ");	\
> +		nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask),	\
> +				       val, ctrl_bit);				\
>  	})
>  
>  /**
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 4e035aca6f7e..79651867beb3 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -41,18 +41,26 @@
>  
>  #define __bf_shf(x) (__builtin_ffsll(x) - 1)
>  
> -#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)			\
> +#define __BF_FIELD_CHECK_MASK(_mask, _pfx)				\
>  	({								\
>  		BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),		\
>  				 _pfx "mask is not constant");		\
>  		BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");	\
> +		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
> +					      (1ULL << __bf_shf(_mask))); \
> +	})
> +
> +#define __BF_FIELD_CHECK_VAL(_mask, _val, _pfx)				\
> +	({								\
>  		BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?		\
>  				 ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
>  				 _pfx "value too large for the field"); \
> -		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull,		\
> +	})
> +
> +#define __BF_FIELD_CHECK_REG(_mask, _reg, _pfx)				\
> +	({								\
> +		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ULL,		\
>  				 _pfx "type of reg too small for mask"); \
> -		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
> -					      (1ULL << __bf_shf(_mask))); \
>  	})
>  
>  /**
> @@ -64,7 +72,7 @@
>   */
>  #define FIELD_MAX(_mask)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: ");	\
> +		__BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: ");		\
>  		(typeof(_mask))((_mask) >> __bf_shf(_mask));		\
>  	})
>  
> @@ -77,7 +85,7 @@
>   */
>  #define FIELD_FIT(_mask, _val)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: ");	\
> +		__BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: ");		\
>  		!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
>  	})
>  
> @@ -91,7 +99,8 @@
>   */
>  #define FIELD_PREP(_mask, _val)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");	\
> +		__BF_FIELD_CHECK_MASK(_mask, "FIELD_PREP: ");		\
> +		__BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_PREP: ");	\
>  		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
>  	})
>  
> @@ -105,7 +114,8 @@
>   */
>  #define FIELD_GET(_mask, _reg)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: ");	\
> +		__BF_FIELD_CHECK_MASK(_mask, "FIELD_GET: ");		\
> +		__BF_FIELD_CHECK_REG(_mask, _reg,  "FIELD_GET: ");	\
>  		(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
>  	})
>  
> 

Re: [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro

[Alex Elder]

On 7/8/20 6:04 PM, Nick Desaulniers wrote:
> This macro has a few expansion sites that pass literal 0s as parameters.
> Split these up so that we do the precise checks where we care about
> them.
> 
> Suggested-by: Alex Elder <elder@linaro.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

I do like this better.  It makes it more obvious when only
the mask is being verified (in FIELD_FIT() for example).
I also appreciate that you distinguished the register
checks from the value checks.
- field masks must be (2^x - 1) << y, x > 0, and constant
- values must be representable within a field mask
- registers must be able to represent the field mask

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
> Changes V1-V2:
> * New patch in v2.
> * Rebased on 0001.
> 
>  .../netronome/nfp/nfpcore/nfp_nsp_eth.c       | 11 ++++----
>  include/linux/bitfield.h                      | 26 +++++++++++++------
>  2 files changed, 24 insertions(+), 13 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 311a5be25acb..938fc733fccb 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> @@ -492,11 +492,12 @@ 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);			\
> +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)		\
> +	({									\
> +		__BF_FIELD_CHECK_MASK(mask, "NFP_ETH_SET_BIT_CONFIG: ");	\
> +		__BF_FIELD_CHECK_VAL(mask, val, "NFP_ETH_SET_BIT_CONFIG: ");	\
> +		nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask),	\
> +				       val, ctrl_bit);				\
>  	})
>  
>  /**
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 4e035aca6f7e..79651867beb3 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -41,18 +41,26 @@
>  
>  #define __bf_shf(x) (__builtin_ffsll(x) - 1)
>  
> -#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)			\
> +#define __BF_FIELD_CHECK_MASK(_mask, _pfx)				\
>  	({								\
>  		BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),		\
>  				 _pfx "mask is not constant");		\
>  		BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");	\
> +		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
> +					      (1ULL << __bf_shf(_mask))); \
> +	})
> +
> +#define __BF_FIELD_CHECK_VAL(_mask, _val, _pfx)				\
> +	({								\
>  		BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?		\
>  				 ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
>  				 _pfx "value too large for the field"); \
> -		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull,		\
> +	})
> +
> +#define __BF_FIELD_CHECK_REG(_mask, _reg, _pfx)				\
> +	({								\
> +		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ULL,		\
>  				 _pfx "type of reg too small for mask"); \
> -		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
> -					      (1ULL << __bf_shf(_mask))); \
>  	})
>  
>  /**
> @@ -64,7 +72,7 @@
>   */
>  #define FIELD_MAX(_mask)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: ");	\
> +		__BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: ");		\
>  		(typeof(_mask))((_mask) >> __bf_shf(_mask));		\
>  	})
>  
> @@ -77,7 +85,7 @@
>   */
>  #define FIELD_FIT(_mask, _val)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: ");	\
> +		__BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: ");		\
>  		!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
>  	})
>  
> @@ -91,7 +99,8 @@
>   */
>  #define FIELD_PREP(_mask, _val)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");	\
> +		__BF_FIELD_CHECK_MASK(_mask, "FIELD_PREP: ");		\
> +		__BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_PREP: ");	\
>  		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
>  	})
>  
> @@ -105,7 +114,8 @@
>   */
>  #define FIELD_GET(_mask, _reg)						\
>  	({								\
> -		__BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: ");	\
> +		__BF_FIELD_CHECK_MASK(_mask, "FIELD_GET: ");		\
> +		__BF_FIELD_CHECK_REG(_mask, _reg,  "FIELD_GET: ");	\
>  		(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
>  	})
>  
> 

Re: [PATCH v2 0/2 net] bitfield.h cleanups

[Nick Desaulniers]

On Wed, Jul 8, 2020 at 4:04 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Two patches, one that removes a BUILD_BUG_ON for a case that is not a
> compile time bug (exposed by compiler optimization).
>
> The second is a general cleanup in the area.
>
> I decided to leave the BUILD_BUG_ON case first, as I hope it will
> simplify being able to backport it to stable, and because I don't think
> there's any type promotion+conversion bugs there.
>
> Though it would be nice to use consistent types widths and signedness,
> equality against literal zero is not an issue.
>
> Jakub Kicinski (1):
>   bitfield.h: don't compile-time validate _val in FIELD_FIT
>
> Nick Desaulniers (1):
>   bitfield.h: split up __BF_FIELD_CHECK macro
>
>  .../netronome/nfp/nfpcore/nfp_nsp_eth.c       | 11 ++++----
>  include/linux/bitfield.h                      | 26 +++++++++++++------
>  2 files changed, 24 insertions(+), 13 deletions(-)
>
> --
> 2.27.0.383.g050319c2ae-goog
>

Hey David, when you have a chance, could you please consider picking
up this series?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2 net] bitfield.h cleanups

[Nick Desaulniers]

On Tue, Jul 14, 2020 at 11:23 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Jul 8, 2020 at 4:04 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > Two patches, one that removes a BUILD_BUG_ON for a case that is not a
> > compile time bug (exposed by compiler optimization).
> >
> > The second is a general cleanup in the area.
> >
> > I decided to leave the BUILD_BUG_ON case first, as I hope it will
> > simplify being able to backport it to stable, and because I don't think
> > there's any type promotion+conversion bugs there.
> >
> > Though it would be nice to use consistent types widths and signedness,
> > equality against literal zero is not an issue.
> >
> > Jakub Kicinski (1):
> >   bitfield.h: don't compile-time validate _val in FIELD_FIT
> >
> > Nick Desaulniers (1):
> >   bitfield.h: split up __BF_FIELD_CHECK macro
> >
> >  .../netronome/nfp/nfpcore/nfp_nsp_eth.c       | 11 ++++----
> >  include/linux/bitfield.h                      | 26 +++++++++++++------
> >  2 files changed, 24 insertions(+), 13 deletions(-)
> >
> > --
> > 2.27.0.383.g050319c2ae-goog
> >
>
> Hey David, when you have a chance, could you please consider picking
> up this series?
> --

Hi David, bumping for review.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2 net] bitfield.h cleanups

[David Miller]

From: Nick Desaulniers <ndesaulniers@google.com>
Date: Mon, 20 Jul 2020 12:48:38 -0700

> Hi David, bumping for review.

If it's not active in my networking patchwork you can't just bump your original
submission like this.

You have to submit your series entirely again.

And if it is in patchwork, such "bumping" is %100 unnecessary.  It's
in my queue, it is not lost, and I will process it when I have the
time.

Thank you.

Re: [PATCH v2 0/2 net] bitfield.h cleanups

[Nick Desaulniers]

On Mon, Jul 20, 2020 at 4:35 PM David Miller <davem@davemloft.net> wrote:
>
> From: Nick Desaulniers <ndesaulniers@google.com>
> Date: Mon, 20 Jul 2020 12:48:38 -0700
>
> > Hi David, bumping for review.
>
> If it's not active in my networking patchwork you can't just bump your original
> submission like this.
>
> You have to submit your series entirely again.
>
> And if it is in patchwork, such "bumping" is %100 unnecessary.  It's
> in my queue, it is not lost, and I will process it when I have the
> time.
>
> Thank you.

Hi David,
Sorry, I'm not familiar with your workflow.  By patchwork, are you
referring to patchwork.ozlabs.org?  If so, I guess filtering on
Delegate=davem
(https://patchwork.ozlabs.org/project/netdev/list/?series=&submitter=&state=&q=&archive=&delegate=34)
is your queue?  I don't see my patches in the above query, but I do
see my series here:
https://patchwork.ozlabs.org/project/netdev/list/?series=188486&state=*
but they're marked "Not Applicable".  What does that mean?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2 net] bitfield.h cleanups

[Nick Desaulniers]

On Thu, Jul 30, 2020 at 3:37 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Jul 20, 2020 at 4:35 PM David Miller <davem@davemloft.net> wrote:
> >
> > From: Nick Desaulniers <ndesaulniers@google.com>
> > Date: Mon, 20 Jul 2020 12:48:38 -0700
> >
> > > Hi David, bumping for review.
> >
> > If it's not active in my networking patchwork you can't just bump your original
> > submission like this.
> >
> > You have to submit your series entirely again.
> >
> > And if it is in patchwork, such "bumping" is %100 unnecessary.  It's
> > in my queue, it is not lost, and I will process it when I have the
> > time.
> >
> > Thank you.
>
> Hi David,
> Sorry, I'm not familiar with your workflow.  By patchwork, are you
> referring to patchwork.ozlabs.org?  If so, I guess filtering on
> Delegate=davem
> (https://patchwork.ozlabs.org/project/netdev/list/?series=&submitter=&state=&q=&archive=&delegate=34)
> is your queue?  I don't see my patches in the above query, but I do
> see my series here:
> https://patchwork.ozlabs.org/project/netdev/list/?series=188486&state=*
> but they're marked "Not Applicable".  What does that mean?

Hi David,
I read through https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#q-how-often-do-changes-from-these-trees-make-it-to-the-mainline-linus-tree
and noticed http://vger.kernel.org/~davem/net-next.html.  Since the
merge window just opened, it sounds like I'll need to wait 2 weeks for
it to close before resending? Is that correct? Based on:

> IMPORTANT: Do not send new net-next content to netdev during the period during which net-next tree is closed.

Then based on the next section in the doc, it sounds like I was
missing which tree to put the patch in, in the subject? I believe
these patches should target net-next (not net) since they're not
addressing regressions from the most recent cycle.

Do I have all that right?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2 net] bitfield.h cleanups

[Jakub Kicinski]

On Wed, 5 Aug 2020 10:44:30 -0700 Nick Desaulniers wrote:
> Hi David,
> I read through https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#q-how-often-do-changes-from-these-trees-make-it-to-the-mainline-linus-tree
> and noticed http://vger.kernel.org/~davem/net-next.html.  Since the
> merge window just opened, it sounds like I'll need to wait 2 weeks for
> it to close before resending? Is that correct? Based on:
> 
> > IMPORTANT: Do not send new net-next content to netdev during the period during which net-next tree is closed.  
> 
> Then based on the next section in the doc, it sounds like I was
> missing which tree to put the patch in, in the subject? I believe
> these patches should target net-next (not net) since they're not
> addressing regressions from the most recent cycle.
> 
> Do I have all that right?

Nick, please repost the first patch only (to:dave, cc:netdev,...,
subject "[PATCH net resend] ...") and I'm pretty sure Dave will
re-consider it, it's a build fix after all. In any case reviewing 
a patch with a short explanation under the commit message on why it's
resend is easier [1] for a maintainer to process than digging through
conversations.

[1] may be related to the use of patchwork on netdev

Re: [PATCH v2 0/2 net] bitfield.h cleanups

[Jakub Kicinski]

On Thu, 6 Aug 2020 11:13:58 -0700 Jakub Kicinski wrote:
> please repost the first patch only

To be clear the second patch may need to wait for net-next since it's
refactoring. That's why I suggest only posting patch 1 now, i.e. the
fix.