[PATCH 4/9] bitfield: Copy #define parameters to locals

david.laight.linux@gmail.com posted 9 patches 2 months ago
There is a newer version of this series
[PATCH 4/9] bitfield: Copy #define parameters to locals
Posted by david.laight.linux@gmail.com 2 months ago
From: David Laight <david.laight.linux@gmail.com>

Use __auto_type to take copies of parameters to both ensure they are
evaluated only once and to avoid bloating the pre-processor output.
In particular 'mask' is likely to be GENMASK() and the expension
of FIELD_GET() is then about 18KB.

Remove any extra (), update kerneldoc.

Consistently use xxx for #define formal parameters and _xxx for
local variables.

Rather than use (typeof(mask))(val) to ensure bits aren't lost when
val is shifted left, use '__auto_type _val = 1 ? (val) : _mask;'
relying on the ?: operator to generate a type that is large enough.

Remove the (typeof(mask)) cast from __FIELD_GET(), it can only make
a difference if 'reg' is larger than 'mask' and the caller cares about
the actual type.
Note that mask usually comes from GENMASK() and is then 'unsigned long'.

Rename the internal defines __FIELD_PREP to __BF_FIELD_PREP and
__FIELD_GET to __BF_FIELD_GET.

Now that field_prep() and field_get() copy their parameters there is
no need for the __field_prep() and __field_get() defines.
But add a define to generate the required 'shift' to use in both defines.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 include/linux/bitfield.h | 150 ++++++++++++++++++++-------------------
 1 file changed, 78 insertions(+), 72 deletions(-)

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 126dc5b380af..3e013da9ea12 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -61,17 +61,17 @@
 
 #define __bf_cast_unsigned(type, x)	((__unsigned_scalar_typeof(type))(x))
 
-#define __BF_FIELD_CHECK_MASK(_mask, _val, _pfx)			\
+#define __BF_FIELD_CHECK_MASK(mask, val, 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_MSG(__builtin_constant_p(_val) ?		\
-				 ~((_mask) >> __bf_shf(_mask)) &	\
-					(0 + (_val)) : 0,		\
-				 _pfx "value too large for the field"); \
-		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
-					      (1ULL << __bf_shf(_mask))); \
+		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_MSG(__builtin_constant_p(val) ?		\
+				 ~((mask) >> __bf_shf(mask)) &		\
+					(0 + (val)) : 0,		\
+				 pfx "value too large for the field");	\
+		__BUILD_BUG_ON_NOT_POWER_OF_2((mask) +			\
+					      (1ULL << __bf_shf(mask))); \
 	})
 
 #define __BF_FIELD_CHECK_REG(mask, reg, pfx)				\
@@ -85,64 +85,69 @@
 		__BF_FIELD_CHECK_REG(mask, reg, pfx);			\
 	})
 
-#define __FIELD_PREP(mask, val, pfx)					\
+#define __BF_FIELD_PREP(mask, val, pfx)					\
 	({								\
 		__BF_FIELD_CHECK_MASK(mask, val, pfx);			\
-		((typeof(mask))(val) << __bf_shf(mask)) & (mask);	\
+		((val) << __bf_shf(mask)) & (mask);			\
 	})
 
-#define __FIELD_GET(mask, reg, pfx)					\
+#define __BF_FIELD_GET(mask, reg, pfx)					\
 	({								\
 		__BF_FIELD_CHECK_MASK(mask, 0U, pfx);			\
-		(typeof(mask))(((reg) & (mask)) >> __bf_shf(mask));	\
+		((reg) & (mask)) >> __bf_shf(mask);			\
 	})
 
 /**
  * FIELD_MAX() - produce the maximum value representable by a field
- * @_mask: shifted mask defining the field's length and position
+ * @mask: shifted mask defining the field's length and position
  *
  * FIELD_MAX() returns the maximum value that can be held in the field
- * specified by @_mask.
+ * specified by @mask.
  */
-#define FIELD_MAX(_mask)						\
+#define FIELD_MAX(mask)							\
 	({								\
+		__auto_type _mask = mask;				\
 		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: ");	\
-		(typeof(_mask))((_mask) >> __bf_shf(_mask));		\
+		(_mask >> __bf_shf(_mask));				\
 	})
 
 /**
  * FIELD_FIT() - check if value fits in the field
- * @_mask: shifted mask defining the field's length and position
- * @_val:  value to test against the field
+ * @mask: shifted mask defining the field's length and position
+ * @val:  value to test against the field
  *
- * Return: true if @_val can fit inside @_mask, false if @_val is too big.
+ * Return: true if @val can fit inside @mask, false if @val is too big.
  */
-#define FIELD_FIT(_mask, _val)						\
+#define FIELD_FIT(mask, val)						\
 	({								\
+		__auto_type _mask = mask;				\
+		__auto_type _val = 1 ? (val) : _mask;			\
 		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: ");	\
-		!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
+		!((_val << __bf_shf(_mask)) & ~_mask); 			\
 	})
 
 /**
  * FIELD_PREP() - prepare a bitfield element
- * @_mask: shifted mask defining the field's length and position
- * @_val:  value to put in the field
+ * @mask: shifted mask defining the field's length and position
+ * @val:  value to put in the field
  *
  * FIELD_PREP() masks and shifts up the value.  The result should
  * be combined with other fields of the bitfield using logical OR.
  */
-#define FIELD_PREP(_mask, _val)						\
+#define FIELD_PREP(mask, val)						\
 	({								\
+		__auto_type _mask = mask;				\
+		__auto_type _val = 1 ? (val) : _mask;			\
 		__BF_FIELD_CHECK_REG(_mask, 0ULL, "FIELD_PREP: ");	\
-		__FIELD_PREP(_mask, _val, "FIELD_PREP: ");		\
+		__BF_FIELD_PREP(_mask, _val, "FIELD_PREP: ");		\
 	})
 
 #define __BF_CHECK_POW2(n)	BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0)
 
 /**
  * FIELD_PREP_CONST() - prepare a constant bitfield element
- * @_mask: shifted mask defining the field's length and position
- * @_val:  value to put in the field
+ * @mask: shifted mask defining the field's length and position
+ * @val:  value to put in the field
  *
  * FIELD_PREP_CONST() masks and shifts up the value.  The result should
  * be combined with other fields of the bitfield using logical OR.
@@ -151,47 +156,52 @@
  * be used in initializers. Error checking is less comfortable for this
  * version, and non-constant masks cannot be used.
  */
-#define FIELD_PREP_CONST(_mask, _val)					\
+#define FIELD_PREP_CONST(mask, val)					\
 	(								\
 		/* mask must be non-zero */				\
-		BUILD_BUG_ON_ZERO((_mask) == 0) +			\
+		BUILD_BUG_ON_ZERO((mask) == 0) +			\
 		/* check if value fits */				\
-		BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
+		BUILD_BUG_ON_ZERO(~((mask) >> __bf_shf(mask)) & (val)) + \
 		/* check if mask is contiguous */			\
-		__BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) +	\
+		__BF_CHECK_POW2((mask) + (1ULL << __bf_shf(mask))) +	\
 		/* and create the value */				\
-		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
+		(((typeof(mask))(val) << __bf_shf(mask)) & (mask))	\
 	)
 
 /**
  * FIELD_GET() - extract a bitfield element
- * @_mask: shifted mask defining the field's length and position
- * @_reg:  value of entire bitfield
+ * @mask: shifted mask defining the field's length and position
+ * @reg:  value of entire bitfield
  *
- * FIELD_GET() extracts the field specified by @_mask from the
- * bitfield passed in as @_reg by masking and shifting it down.
+ * FIELD_GET() extracts the field specified by @mask from the
+ * bitfield passed in as @reg by masking and shifting it down.
  */
-#define FIELD_GET(_mask, _reg)						\
+#define FIELD_GET(mask, reg)						\
 	({								\
+		__auto_type _mask = mask;				\
+		__auto_type _reg = reg;					\
 		__BF_FIELD_CHECK_REG(_mask, _reg, "FIELD_GET: ");	\
-		__FIELD_GET(_mask, _reg, "FIELD_GET: ");		\
+		__BF_FIELD_GET(_mask, _reg, "FIELD_GET: ");		\
 	})
 
 /**
  * FIELD_MODIFY() - modify a bitfield element
- * @_mask: shifted mask defining the field's length and position
- * @_reg_p: pointer to the memory that should be updated
- * @_val: value to store in the bitfield
+ * @mask: shifted mask defining the field's length and position
+ * @reg_p: pointer to the memory that should be updated
+ * @val: value to store in the bitfield
  *
- * FIELD_MODIFY() modifies the set of bits in @_reg_p specified by @_mask,
- * by replacing them with the bitfield value passed in as @_val.
+ * FIELD_MODIFY() modifies the set of bits in @reg_p specified by @mask,
+ * by replacing them with the bitfield value passed in as @val.
  */
-#define FIELD_MODIFY(_mask, _reg_p, _val)						\
+#define FIELD_MODIFY(mask, reg_p, val)							\
 	({										\
+		__auto_type _mask = mask;						\
+		__auto_type _reg_p = reg_p;						\
+		__auto_type _val = 1 ? (val) : _mask;					\
 		typecheck_pointer(_reg_p);						\
-		__BF_FIELD_CHECK(_mask, *(_reg_p), _val, "FIELD_MODIFY: ");		\
-		*(_reg_p) &= ~(_mask);							\
-		*(_reg_p) |= (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask));	\
+		__BF_FIELD_CHECK(_mask, *_reg_p, _val, "FIELD_MODIFY: ");		\
+		*_reg_p &= ~_mask;							\
+		*_reg_p |= ((_val << __bf_shf(_mask)) & _mask);				\
 	})
 
 extern void __compiletime_error("value doesn't fit into mask")
@@ -241,23 +251,9 @@ __MAKE_OP(64)
 #undef __MAKE_OP
 #undef ____MAKE_OP
 
-#define __field_prep(mask, val)						\
-	({								\
-		__auto_type __mask = (mask);				\
-		typeof(__mask) __val = (val);				\
-		unsigned int __shift = BITS_PER_TYPE(__mask) <= 32 ?	\
-				       __ffs(__mask) : __ffs64(__mask);	\
-		(__val << __shift) & __mask;				\
-	})
-
-#define __field_get(mask, reg)						\
-	({								\
-		__auto_type __mask = (mask);				\
-		typeof(__mask) __reg =  (reg);				\
-		unsigned int __shift = BITS_PER_TYPE(__mask) <= 32 ?	\
-				       __ffs(__mask) : __ffs64(__mask);	\
-		(__reg & __mask) >> __shift;				\
-	})
+/* As __bf_shf() but for non-zero variables */
+#define __BF_SHIFT(mask) \
+	(BITS_PER_TYPE(_mask) <= 32 ? __ffs(_mask) : __ffs64(_mask))
 
 /**
  * field_prep() - prepare a bitfield element
@@ -275,9 +271,14 @@ __MAKE_OP(64)
  * If you want to ensure that @mask is a compile-time constant, please use
  * FIELD_PREP() directly instead.
  */
-#define field_prep(mask, val)						\
-	(__builtin_constant_p(mask) ? __FIELD_PREP(mask, val, "field_prep: ") \
-				    : __field_prep(mask, val))
+#define field_prep(mask, val)					\
+({								\
+	__auto_type _mask = mask;				\
+	__auto_type _val = 1 ? (val) : _mask;			\
+	__builtin_constant_p(_mask) ?				\
+		__BF_FIELD_PREP(_mask, _val, "field_prep: ") :	\
+		(_val << __BF_SHIFT(_mask)) & _mask;		\
+})
 
 /**
  * field_get() - extract a bitfield element
@@ -295,8 +296,13 @@ __MAKE_OP(64)
  * If you want to ensure that @mask is a compile-time constant, please use
  * FIELD_GET() directly instead.
  */
-#define field_get(mask, reg)						\
-	(__builtin_constant_p(mask) ? __FIELD_GET(mask, reg, "field_get: ") \
-				    : __field_get(mask, reg))
+#define field_get(mask, reg)					\
+({								\
+	__auto_type _mask = mask;				\
+	__auto_type _reg = reg;					\
+	__builtin_constant_p(_mask) ?				\
+		__BF_FIELD_GET(_mask, _reg, "field_get: ") :	\
+		(_reg & _mask) >> __BF_SHIFT(_mask);		\
+})
 
 #endif
-- 
2.39.5
Re: [PATCH 4/9] bitfield: Copy #define parameters to locals
Posted by Andy Shevchenko 2 months ago
On Tue, Dec 09, 2025 at 10:03:08AM +0000, david.laight.linux@gmail.com wrote:

> Use __auto_type to take copies of parameters to both ensure they are
> evaluated only once and to avoid bloating the pre-processor output.
> In particular 'mask' is likely to be GENMASK() and the expension
> of FIELD_GET() is then about 18KB.
> 
> Remove any extra (), update kerneldoc.

> Consistently use xxx for #define formal parameters and _xxx for
> local variables.

Okay, I commented below, and I think this is too huge to be in this commit.
Can we make it separate?

> Rather than use (typeof(mask))(val) to ensure bits aren't lost when
> val is shifted left, use '__auto_type _val = 1 ? (val) : _mask;'
> relying on the ?: operator to generate a type that is large enough.
> 
> Remove the (typeof(mask)) cast from __FIELD_GET(), it can only make
> a difference if 'reg' is larger than 'mask' and the caller cares about
> the actual type.
> Note that mask usually comes from GENMASK() and is then 'unsigned long'.
> 
> Rename the internal defines __FIELD_PREP to __BF_FIELD_PREP and
> __FIELD_GET to __BF_FIELD_GET.
> 
> Now that field_prep() and field_get() copy their parameters there is
> no need for the __field_prep() and __field_get() defines.
> But add a define to generate the required 'shift' to use in both defines.

...

> -#define __BF_FIELD_CHECK_MASK(_mask, _val, _pfx)			\
> +#define __BF_FIELD_CHECK_MASK(mask, val, 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_MSG(__builtin_constant_p(_val) ?		\
> -				 ~((_mask) >> __bf_shf(_mask)) &	\
> -					(0 + (_val)) : 0,		\
> -				 _pfx "value too large for the field"); \
> -		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
> -					      (1ULL << __bf_shf(_mask))); \
> +		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_MSG(__builtin_constant_p(val) ?		\
> +				 ~((mask) >> __bf_shf(mask)) &		\
> +					(0 + (val)) : 0,		\
> +				 pfx "value too large for the field");	\
> +		__BUILD_BUG_ON_NOT_POWER_OF_2((mask) +			\
> +					      (1ULL << __bf_shf(mask))); \
>  	})

I looks like renaming parameters without any benefit, actually the opposite
it's very hard to see if there is any interesting change here. Please, drop
this or make it clear to focus only on the things that needs to be changed.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH 4/9] bitfield: Copy #define parameters to locals
Posted by Yury Norov 2 months ago
On Tue, Dec 09, 2025 at 05:51:48PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 09, 2025 at 10:03:08AM +0000, david.laight.linux@gmail.com wrote:
> 
> > Use __auto_type to take copies of parameters to both ensure they are
> > evaluated only once and to avoid bloating the pre-processor output.
> > In particular 'mask' is likely to be GENMASK() and the expension
> > of FIELD_GET() is then about 18KB.
> > 
> > Remove any extra (), update kerneldoc.
> 
> > Consistently use xxx for #define formal parameters and _xxx for
> > local variables.
> 
> Okay, I commented below, and I think this is too huge to be in this commit.
> Can we make it separate?

I'm next to Andy. The commit message covers 6 or 7 independent
changes, and patch body itself seems to be above my abilities to
review. This should look like a series if nice cleanups, now it looks
like a patch bomb.
 
> > Rather than use (typeof(mask))(val) to ensure bits aren't lost when
> > val is shifted left, use '__auto_type _val = 1 ? (val) : _mask;'
> > relying on the ?: operator to generate a type that is large enough.
> > 
> > Remove the (typeof(mask)) cast from __FIELD_GET(), it can only make
> > a difference if 'reg' is larger than 'mask' and the caller cares about
> > the actual type.
> > Note that mask usually comes from GENMASK() and is then 'unsigned long'.
> > 
> > Rename the internal defines __FIELD_PREP to __BF_FIELD_PREP and
> > __FIELD_GET to __BF_FIELD_GET.
> > 
> > Now that field_prep() and field_get() copy their parameters there is
> > no need for the __field_prep() and __field_get() defines.
> > But add a define to generate the required 'shift' to use in both defines.
> 
> ...
> 
> > -#define __BF_FIELD_CHECK_MASK(_mask, _val, _pfx)			\
> > +#define __BF_FIELD_CHECK_MASK(mask, val, 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_MSG(__builtin_constant_p(_val) ?		\
> > -				 ~((_mask) >> __bf_shf(_mask)) &	\
> > -					(0 + (_val)) : 0,		\
> > -				 _pfx "value too large for the field"); \
> > -		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
> > -					      (1ULL << __bf_shf(_mask))); \
> > +		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_MSG(__builtin_constant_p(val) ?		\
> > +				 ~((mask) >> __bf_shf(mask)) &		\
> > +					(0 + (val)) : 0,		\
> > +				 pfx "value too large for the field");	\
> > +		__BUILD_BUG_ON_NOT_POWER_OF_2((mask) +			\
> > +					      (1ULL << __bf_shf(mask))); \
> >  	})
> 
> I looks like renaming parameters without any benefit, actually the opposite
> it's very hard to see if there is any interesting change here. Please, drop
> this or make it clear to focus only on the things that needs to be changed.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>
Re: [PATCH 4/9] bitfield: Copy #define parameters to locals
Posted by David Laight 2 months ago
On Tue, 9 Dec 2025 17:51:48 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 09, 2025 at 10:03:08AM +0000, david.laight.linux@gmail.com wrote:
> 
> > Use __auto_type to take copies of parameters to both ensure they are
> > evaluated only once and to avoid bloating the pre-processor output.
> > In particular 'mask' is likely to be GENMASK() and the expension
> > of FIELD_GET() is then about 18KB.
> > 
> > Remove any extra (), update kerneldoc.  
> 
> > Consistently use xxx for #define formal parameters and _xxx for
> > local variables.  
> 
> Okay, I commented below, and I think this is too huge to be in this commit.
> Can we make it separate?

I originally wrote a much longer patch set, then merged some to reduce
the number of patches - you can't win really.

> 
> > Rather than use (typeof(mask))(val) to ensure bits aren't lost when
> > val is shifted left, use '__auto_type _val = 1 ? (val) : _mask;'
> > relying on the ?: operator to generate a type that is large enough.
> > 
> > Remove the (typeof(mask)) cast from __FIELD_GET(), it can only make
> > a difference if 'reg' is larger than 'mask' and the caller cares about
> > the actual type.
> > Note that mask usually comes from GENMASK() and is then 'unsigned long'.
> > 
> > Rename the internal defines __FIELD_PREP to __BF_FIELD_PREP and
> > __FIELD_GET to __BF_FIELD_GET.
> > 
> > Now that field_prep() and field_get() copy their parameters there is
> > no need for the __field_prep() and __field_get() defines.
> > But add a define to generate the required 'shift' to use in both defines.  
> 
> ...
> 
> > -#define __BF_FIELD_CHECK_MASK(_mask, _val, _pfx)			\
> > +#define __BF_FIELD_CHECK_MASK(mask, val, 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_MSG(__builtin_constant_p(_val) ?		\
> > -				 ~((_mask) >> __bf_shf(_mask)) &	\
> > -					(0 + (_val)) : 0,		\
> > -				 _pfx "value too large for the field"); \
> > -		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
> > -					      (1ULL << __bf_shf(_mask))); \
> > +		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_MSG(__builtin_constant_p(val) ?		\
> > +				 ~((mask) >> __bf_shf(mask)) &		\
> > +					(0 + (val)) : 0,		\
> > +				 pfx "value too large for the field");	\
> > +		__BUILD_BUG_ON_NOT_POWER_OF_2((mask) +			\
> > +					      (1ULL << __bf_shf(mask))); \
> >  	})  
> 
> I looks like renaming parameters without any benefit, actually the opposite
> it's very hard to see if there is any interesting change here. Please, drop
> this or make it clear to focus only on the things that needs to be changed.

I'm pretty sure there are no other changes in that bit.
(The entire define is pretty much re-written in a later patch and I
did want to separate the changes.)

I wanted to the file to be absolutely consistent with the parameter/variable
names.
Plausibly the scheme could be slightly different:
'user' parameters are 'xxx', '__auto_type' variables are '_xxx'.
But internal defines that evaluate/expand parameters more than once are
'_xxx' and must be 'copied' by an outer define.

	David
Re: [PATCH 4/9] bitfield: Copy #define parameters to locals
Posted by Andy Shevchenko 2 months ago
On Tue, Dec 09, 2025 at 07:11:48PM +0000, David Laight wrote:
> On Tue, 9 Dec 2025 17:51:48 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Dec 09, 2025 at 10:03:08AM +0000, david.laight.linux@gmail.com wrote:

...

> > > -#define __BF_FIELD_CHECK_MASK(_mask, _val, _pfx)			\
> > > +#define __BF_FIELD_CHECK_MASK(mask, val, 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_MSG(__builtin_constant_p(_val) ?		\
> > > -				 ~((_mask) >> __bf_shf(_mask)) &	\
> > > -					(0 + (_val)) : 0,		\
> > > -				 _pfx "value too large for the field"); \
> > > -		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
> > > -					      (1ULL << __bf_shf(_mask))); \
> > > +		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_MSG(__builtin_constant_p(val) ?		\
> > > +				 ~((mask) >> __bf_shf(mask)) &		\
> > > +					(0 + (val)) : 0,		\
> > > +				 pfx "value too large for the field");	\
> > > +		__BUILD_BUG_ON_NOT_POWER_OF_2((mask) +			\
> > > +					      (1ULL << __bf_shf(mask))); \
> > >  	})  
> > 
> > I looks like renaming parameters without any benefit, actually the opposite
> > it's very hard to see if there is any interesting change here. Please, drop
> > this or make it clear to focus only on the things that needs to be changed.
> 
> I'm pretty sure there are no other changes in that bit.

Yes, but the rule of thumb to avoid putting several logical changes into a
single patch and here AFAICT the renaming should be avoided  / split to a
precursor or do it after this.

> (The entire define is pretty much re-written in a later patch and I
> did want to separate the changes.)

Then probably don't do the change at all (renaming), as it's useless here?

> I wanted to the file to be absolutely consistent with the parameter/variable
> names.

No objection on this.

> Plausibly the scheme could be slightly different:
> 'user' parameters are 'xxx', '__auto_type' variables are '_xxx'.
> But internal defines that evaluate/expand parameters more than once are
> '_xxx' and must be 'copied' by an outer define.

-- 
With Best Regards,
Andy Shevchenko