[PATCH 0/9] bitfield: tidy up bitfield.h

[PATCH 0/9] bitfield: tidy up bitfield.h

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

I noticed some very long (18KB) error messages from the compiler.
Turned out they were errors on lines that passed GENMASK() to FIELD_PREP().
Since most of the #defines are already statement functions the values
can be copied to locals so the actual parameters only get expanded once.

The 'bloat' is reduced further by using a simple test to ensure 'reg'
is large enough, slightly simplifying the test for constant 'val' and
only checking 'reg' and 'val' when the parameters are present.

The first two patches are slightly problematic.

drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c manages to use
a #define that should be an internal to bitfield.h, the changed file
is actually more similar to the previous version.

drivers/thunderbolt/tb.h passes a bifield to FIELD_GET(), these can't
be used with sizeof or __auto_type. The usual solution is to add zero,
but that can't be done in FIELD_GET() because it doesn't want the value
promoted to 'int' (no idea how _Generic() treated it.)
The fix is just to add zero at the call site.
(The bitfield seems to be in a structure rad from hardware - no idea
how that works on BE (or any LE that uses an unusual order for bitfields.)

Both changes may need to to through the same tree as the header file changes.

The changes are based on 'next' and contain the addition of field_prep()
and field_get() for non-constant values.

I also know it is the merge window.
I expect to be generating a v2 in the new year (someone always has a comment).

David Laight (9):
  nfp: Call FIELD_PREP() in NFP_ETH_SET_BIT_CONFIG() wrapper
  thunderblot: Don't pass a bitfield to FIELD_GET
  bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()
  bitfield: Copy #define parameters to locals
  bitfield: FIELD_MODIFY: Only do a single read/write on the target
  bitfield: Update sanity checks
  bitfield: Reduce indentation
  bitfield: Add comment block for the host/fixed endian functions
  bitfield: Update comments for le/be functions

 .../netronome/nfp/nfpcore/nfp_nsp_eth.c       |  16 +-
 drivers/thunderbolt/tb.h                      |   2 +-
 include/linux/bitfield.h                      | 278 ++++++++++--------
 include/linux/hw_bitfield.h                   |  17 +-
 4 files changed, 166 insertions(+), 147 deletions(-)

-- 
2.39.5

Re: [PATCH 0/9] bitfield: tidy up bitfield.h

[Mika Westerberg]

Hi David,

On Mon, Dec 08, 2025 at 10:42:41PM +0000, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> I noticed some very long (18KB) error messages from the compiler.
> Turned out they were errors on lines that passed GENMASK() to FIELD_PREP().
> Since most of the #defines are already statement functions the values
> can be copied to locals so the actual parameters only get expanded once.
> 
> The 'bloat' is reduced further by using a simple test to ensure 'reg'
> is large enough, slightly simplifying the test for constant 'val' and
> only checking 'reg' and 'val' when the parameters are present.
> 
> The first two patches are slightly problematic.
> 
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c manages to use
> a #define that should be an internal to bitfield.h, the changed file
> is actually more similar to the previous version.
> 
> drivers/thunderbolt/tb.h passes a bifield to FIELD_GET(), these can't
> be used with sizeof or __auto_type. The usual solution is to add zero,
> but that can't be done in FIELD_GET() because it doesn't want the value
> promoted to 'int' (no idea how _Generic() treated it.)
> The fix is just to add zero at the call site.
> (The bitfield seems to be in a structure rad from hardware - no idea
> how that works on BE (or any LE that uses an unusual order for bitfields.)

Okay but can you CC me the actual patch too? I only got the cover letter
;-)

> Both changes may need to to through the same tree as the header file changes.
> 
> The changes are based on 'next' and contain the addition of field_prep()
> and field_get() for non-constant values.
> 
> I also know it is the merge window.
> I expect to be generating a v2 in the new year (someone always has a comment).
> 
> David Laight (9):
>   nfp: Call FIELD_PREP() in NFP_ETH_SET_BIT_CONFIG() wrapper
>   thunderblot: Don't pass a bitfield to FIELD_GET
>   bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()
>   bitfield: Copy #define parameters to locals
>   bitfield: FIELD_MODIFY: Only do a single read/write on the target
>   bitfield: Update sanity checks
>   bitfield: Reduce indentation
>   bitfield: Add comment block for the host/fixed endian functions
>   bitfield: Update comments for le/be functions
> 
>  .../netronome/nfp/nfpcore/nfp_nsp_eth.c       |  16 +-
>  drivers/thunderbolt/tb.h                      |   2 +-
>  include/linux/bitfield.h                      | 278 ++++++++++--------
>  include/linux/hw_bitfield.h                   |  17 +-
>  4 files changed, 166 insertions(+), 147 deletions(-)
> 
> -- 
> 2.39.5

Re: [PATCH 0/9] bitfield: tidy up bitfield.h

[Jakub Kicinski]

On Tue, 9 Dec 2025 08:08:06 +0100 Mika Westerberg wrote:
> > drivers/thunderbolt/tb.h passes a bifield to FIELD_GET(), these can't
> > be used with sizeof or __auto_type. The usual solution is to add zero,
> > but that can't be done in FIELD_GET() because it doesn't want the value
> > promoted to 'int' (no idea how _Generic() treated it.)
> > The fix is just to add zero at the call site.
> > (The bitfield seems to be in a structure rad from hardware - no idea
> > how that works on BE (or any LE that uses an unusual order for bitfields.)  
> 
> Okay but can you CC me the actual patch too? I only got the cover letter
> ;-)

Right, David, please fix your CC lists. kernel dev 101..

Re: [PATCH 0/9] bitfield: tidy up bitfield.h

[David Laight]

On Tue, 9 Dec 2025 08:08:06 +0100
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> Hi David,
> 
> On Mon, Dec 08, 2025 at 10:42:41PM +0000, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > I noticed some very long (18KB) error messages from the compiler.
> > Turned out they were errors on lines that passed GENMASK() to FIELD_PREP().
> > Since most of the #defines are already statement functions the values
> > can be copied to locals so the actual parameters only get expanded once.
> > 
> > The 'bloat' is reduced further by using a simple test to ensure 'reg'
> > is large enough, slightly simplifying the test for constant 'val' and
> > only checking 'reg' and 'val' when the parameters are present.
> > 
> > The first two patches are slightly problematic.
> > 
> > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c manages to use
> > a #define that should be an internal to bitfield.h, the changed file
> > is actually more similar to the previous version.
> > 
> > drivers/thunderbolt/tb.h passes a bifield to FIELD_GET(), these can't
> > be used with sizeof or __auto_type. The usual solution is to add zero,
> > but that can't be done in FIELD_GET() because it doesn't want the value
> > promoted to 'int' (no idea how _Generic() treated it.)
> > The fix is just to add zero at the call site.
> > (The bitfield seems to be in a structure rad from hardware - no idea
> > how that works on BE (or any LE that uses an unusual order for bitfields.)  
> 
> Okay but can you CC me the actual patch too? I only got the cover letter
> ;-)

Ah, sorry I'd changed the git settings..
I'll resend it all.

	David

[PATCH 0/9] bitfield: tidy up bitfield.h

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Re-send with patches going to everyone.
(I'd forgotten I'd set 'ccCover = 0'.)

I noticed some very long (18KB) error messages from the compiler.
Turned out they were errors on lines that passed GENMASK() to FIELD_PREP().
Since most of the #defines are already statement functions the values
can be copied to locals so the actual parameters only get expanded once.

The 'bloat' is reduced further by using a simple test to ensure 'reg'
is large enough, slightly simplifying the test for constant 'val' and
only checking 'reg' and 'val' when the parameters are present.

The first two patches are slightly problematic.

drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c manages to use
a #define that should be an internal to bitfield.h, the changed file
is actually more similar to the previous version.

drivers/thunderbolt/tb.h passes a bifield to FIELD_GET(), these can't
be used with sizeof or __auto_type. The usual solution is to add zero,
but that can't be done in FIELD_GET() because it doesn't want the value
promoted to 'int' (no idea how _Generic() treated it.)
The fix is just to add zero at the call site.
(The bitfield seems to be in a structure rad from hardware - no idea
how that works on BE (or any LE that uses an unusual order for bitfields.)

Both changes may need to to through the same tree as the header file changes.

The changes are based on 'next' and contain the addition of field_prep()
and field_get() for non-constant values.

I also know it is the merge window.
I expect to be generating a v2 in the new year (someone always has a comment).

David Laight (9):
  nfp: Call FIELD_PREP() in NFP_ETH_SET_BIT_CONFIG() wrapper
  thunderblot: Don't pass a bitfield to FIELD_GET
  bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()
  bitfield: Copy #define parameters to locals
  bitfield: FIELD_MODIFY: Only do a single read/write on the target
  bitfield: Update sanity checks
  bitfield: Reduce indentation
  bitfield: Add comment block for the host/fixed endian functions
  bitfield: Update comments for le/be functions

 .../netronome/nfp/nfpcore/nfp_nsp_eth.c       |  16 +-
 drivers/thunderbolt/tb.h                      |   2 +-
 include/linux/bitfield.h                      | 278 ++++++++++--------
 include/linux/hw_bitfield.h                   |  17 +-
 4 files changed, 166 insertions(+), 147 deletions(-)

-- 
2.39.5

[PATCH 1/9] nfp: Call FIELD_PREP() in NFP_ETH_SET_BIT_CONFIG() wrapper

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Rather than use a define that should be internal to the implementation
of FIELD_PREP(), pass the shifted 'val' to nfp_eth_set_bit_config()
and change the test for 'value unchanged' to match.

This is a simpler change than the one used to avoid calling both
FIELD_GET() and FIELD_PREP() with non-constant mask values.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 .../ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 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 5cfddc9a5d87..4a71ff47fbef 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
@@ -509,8 +509,7 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed)
 
 static int
 nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
-		       const u64 mask, const unsigned int shift,
-		       u64 val, const u64 ctrl_bit)
+		       const u64 mask, u64 shifted_val, const u64 ctrl_bit)
 {
 	union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
 	unsigned int idx = nfp_nsp_config_idx(nsp);
@@ -527,11 +526,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 == (reg & mask) >> shift)
+	if (shifted_val == (reg & mask))
 		return 0;
 
 	reg &= ~mask;
-	reg |= (val << shift) & mask;
+	reg |= shifted_val;
 	entries[idx].raw[raw_idx] = cpu_to_le64(reg);
 
 	entries[idx].control |= cpu_to_le64(ctrl_bit);
@@ -571,12 +570,9 @@ int nfp_eth_set_idmode(struct nfp_cpp *cpp, unsigned int idx, bool state)
 	return nfp_eth_config_commit_end(nsp);
 }
 
-#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)	  \
+	nfp_eth_set_bit_config(nsp, raw_idx, mask, FIELD_PREP(mask, val), \
+			       ctrl_bit)
 
 /**
  * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
-- 
2.39.5

[PATCH 2/9] thunderblot: Don't pass a bitfield to FIELD_GET

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

FIELD_GET needs to use __auto_type to get the value of the 'reg'
parameter, this can't be used with bifields.

FIELD_GET also want to verify the size of 'reg' so can't add zero
to force the type to int.

So add a zero here.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 drivers/thunderbolt/tb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index e96474f17067..7ca2b5a0f01e 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
  */
 static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
 {
-	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
+	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);
 }
 
 /**
-- 
2.39.5

[PATCH 3/9] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Instead of directly expanding __BF_FIELD_CHECK() (which really ought
not be used outside bitfield) and open-coding the generation of the
masked value, just call FIELD_PREP() and add an extra check for
the mask being at most 16 bits.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 include/linux/hw_bitfield.h | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
index df202e167ce4..d7f21b60449b 100644
--- a/include/linux/hw_bitfield.h
+++ b/include/linux/hw_bitfield.h
@@ -23,15 +23,14 @@
  * register, a bit in the lower half is only updated if the corresponding bit
  * in the upper half is high.
  */
-#define FIELD_PREP_WM16(_mask, _val)					     \
-	({								     \
-		typeof(_val) __val = _val;				     \
-		typeof(_mask) __mask = _mask;				     \
-		__BF_FIELD_CHECK(__mask, ((u16)0U), __val,		     \
-				 "HWORD_UPDATE: ");			     \
-		(((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) | \
-		((__mask) << 16);					     \
-	})
+#define FIELD_PREP_WM16(mask, val)				\
+({								\
+	__auto_type _mask = mask;				\
+	u32 _val = FIELD_PREP(_mask, val);			\
+	BUILD_BUG_ON_MSG(_mask > 0xffffu,			\
+			 "FIELD_PREP_WM16: mask too large");	\
+	_val | (_mask << 16);					\
+})
 
 /**
  * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
-- 
2.39.5

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

[david.laight.linux]

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

[PATCH 5/9] bitfield: FIELD_MODIFY: Only do a single read/write on the target

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Replace the:
        *reg &= ~mask; *reg |= new_value;
with a single assignment.
While the compiler will usually optimise the extra access away
it isn't guaranteed.

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

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 3e013da9ea12..3e0e8533bb66 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -200,8 +200,7 @@
 		__auto_type _val = 1 ? (val) : _mask;					\
 		typecheck_pointer(_reg_p);						\
 		__BF_FIELD_CHECK(_mask, *_reg_p, _val, "FIELD_MODIFY: ");		\
-		*_reg_p &= ~_mask;							\
-		*_reg_p |= ((_val << __bf_shf(_mask)) & _mask);				\
+		*_reg_p = (*_reg_p & ~_mask) | ((_val << __bf_shf(_mask)) & _mask);	\
 	})
 
 extern void __compiletime_error("value doesn't fit into mask")
-- 
2.39.5

[PATCH 6/9] bitfield: Update sanity checks

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Simplify the check for 'reg' being large enough to hold 'mask' using
sizeof (reg) rather than a convoluted scheme to generate an unsigned
type the same size as 'reg'.

There are three places where the mask is checked for being non-zero
and contiguous. Add a simple expression that checks it and use in
all three places.

Three of the five calls to __BF_FIELD_CHECK_MASK() don't have a 'value'
to check, separate out as was done to __BF_FIELD_CHECK_REG().

There is no point checking a 'val' of zero or a 'reg' of 0ULL (both
are placeholders) - remove/change the calls.

There should be a check of __BF_FIELD_CHECK_REG() when __BF_FIELD_GET()
is called from field_get().
Move the check from FIELD_GET() into __BF_FIELD_GET().

Delete the now-unused __BF_FIELD_CHECK().

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

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 3e0e8533bb66..7e8d436b6571 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -45,55 +45,38 @@
 
 #define __bf_shf(x) (__builtin_ffsll(x) - 1)
 
-#define __scalar_type_to_unsigned_cases(type)				\
-		unsigned type:	(unsigned type)0,			\
-		signed type:	(unsigned type)0
+#define __BF_VALIDATE_MASK(mask) \
+	(!(mask) || ((mask) & ((mask) + ((mask) & -(mask)))))
 
-#define __unsigned_scalar_typeof(x) typeof(				\
-		_Generic((x),						\
-			char:	(unsigned char)0,			\
-			__scalar_type_to_unsigned_cases(char),		\
-			__scalar_type_to_unsigned_cases(short),		\
-			__scalar_type_to_unsigned_cases(int),		\
-			__scalar_type_to_unsigned_cases(long),		\
-			__scalar_type_to_unsigned_cases(long long),	\
-			default: (x)))
-
-#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, pfx)				\
+	do {								\
 		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(__BF_VALIDATE_MASK(mask),		\
+				 pfx "mask is zero or not contiguous");	\
+	} while (0)
+
+#define __BF_FIELD_CHECK_VAL(mask, val, pfx)			\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(val) &&		\
+			 ~((mask) >> __bf_shf(mask)) & (val),	\
+			 pfx "value too large for the field")
 
 #define __BF_FIELD_CHECK_REG(mask, reg, pfx)				\
-	BUILD_BUG_ON_MSG(__bf_cast_unsigned(mask, mask) >		\
-			 __bf_cast_unsigned(reg, ~0ull),		\
+	BUILD_BUG_ON_MSG(mask + 0U + 0UL + 0ULL >			\
+			 ~0ULL >> (64 - 8 * sizeof (reg)),		\
 			 pfx "type of reg too small for mask")
 
-#define __BF_FIELD_CHECK(mask, reg, val, pfx)				\
-	({								\
-		__BF_FIELD_CHECK_MASK(mask, val, pfx);			\
-		__BF_FIELD_CHECK_REG(mask, reg, pfx);			\
-	})
-
 #define __BF_FIELD_PREP(mask, val, pfx)					\
 	({								\
-		__BF_FIELD_CHECK_MASK(mask, val, pfx);			\
+		__BF_FIELD_CHECK_MASK(mask, pfx);			\
+		__BF_FIELD_CHECK_VAL(mask, val, pfx);			\
 		((val) << __bf_shf(mask)) & (mask);			\
 	})
 
 #define __BF_FIELD_GET(mask, reg, pfx)					\
 	({								\
-		__BF_FIELD_CHECK_MASK(mask, 0U, pfx);			\
+		__BF_FIELD_CHECK_MASK(mask, pfx);			\
+		__BF_FIELD_CHECK_REG(mask, reg, pfx);			\
 		((reg) & (mask)) >> __bf_shf(mask);			\
 	})
 
@@ -107,7 +90,7 @@
 #define FIELD_MAX(mask)							\
 	({								\
 		__auto_type _mask = mask;				\
-		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: ");	\
+		__BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: ");		\
 		(_mask >> __bf_shf(_mask));				\
 	})
 
@@ -122,7 +105,7 @@
 	({								\
 		__auto_type _mask = mask;				\
 		__auto_type _val = 1 ? (val) : _mask;			\
-		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: ");	\
+		__BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: ");		\
 		!((_val << __bf_shf(_mask)) & ~_mask); 			\
 	})
 
@@ -138,12 +121,9 @@
 	({								\
 		__auto_type _mask = mask;				\
 		__auto_type _val = 1 ? (val) : _mask;			\
-		__BF_FIELD_CHECK_REG(_mask, 0ULL, "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
@@ -158,12 +138,10 @@
  */
 #define FIELD_PREP_CONST(mask, val)					\
 	(								\
-		/* mask must be non-zero */				\
-		BUILD_BUG_ON_ZERO((mask) == 0) +			\
+		/* mask must be non-zero and contiguous */		\
+		BUILD_BUG_ON_ZERO(__BF_VALIDATE_MASK(mask)) +		\
 		/* check if value fits */				\
 		BUILD_BUG_ON_ZERO(~((mask) >> __bf_shf(mask)) & (val)) + \
-		/* check if mask is contiguous */			\
-		__BF_CHECK_POW2((mask) + (1ULL << __bf_shf(mask))) +	\
 		/* and create the value */				\
 		(((typeof(mask))(val) << __bf_shf(mask)) & (mask))	\
 	)
@@ -180,7 +158,6 @@
 	({								\
 		__auto_type _mask = mask;				\
 		__auto_type _reg = reg;					\
-		__BF_FIELD_CHECK_REG(_mask, _reg, "FIELD_GET: ");	\
 		__BF_FIELD_GET(_mask, _reg, "FIELD_GET: ");		\
 	})
 
@@ -198,8 +175,9 @@
 		__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: ");		\
+		__BF_FIELD_CHECK_MASK(_mask, "FIELD_MODIFY: ");				\
+		__BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_MODIFY: ");			\
+		__BF_FIELD_CHECK_REG(_mask, *_reg_p, "FIELD_MODIFY: ");			\
 		*_reg_p = (*_reg_p & ~_mask) | ((_val << __bf_shf(_mask)) & _mask);	\
 	})
 
@@ -209,7 +187,7 @@ extern void __compiletime_error("bad bitfield mask")
 __bad_mask(void);
 static __always_inline u64 field_multiplier(u64 field)
 {
-	if ((field | (field - 1)) & ((field | (field - 1)) + 1))
+	if (__BF_VALIDATE_MASK(field))
 		__bad_mask();
 	return field & -field;
 }
-- 
2.39.5

[PATCH 7/9] bitfield: Reduce indentation

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

There is no need to double indent the body of #defines.
Leave the opening ( and closing ) on their own lines.

Delete extra tabs before continuation markers.

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

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 7e8d436b6571..bfd80ebd25b1 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -48,37 +48,37 @@
 #define __BF_VALIDATE_MASK(mask) \
 	(!(mask) || ((mask) & ((mask) + ((mask) & -(mask)))))
 
-#define __BF_FIELD_CHECK_MASK(mask, pfx)				\
-	do {								\
-		BUILD_BUG_ON_MSG(!__builtin_constant_p(mask),		\
-				 pfx "mask is not constant");		\
-		BUILD_BUG_ON_MSG(__BF_VALIDATE_MASK(mask),		\
-				 pfx "mask is zero or not contiguous");	\
-	} while (0)
+#define __BF_FIELD_CHECK_MASK(mask, pfx)			\
+do {								\
+	BUILD_BUG_ON_MSG(!__builtin_constant_p(mask),		\
+			 pfx "mask is not constant");		\
+	BUILD_BUG_ON_MSG(__BF_VALIDATE_MASK(mask),		\
+			 pfx "mask is zero or not contiguous");	\
+} while (0)
 
 #define __BF_FIELD_CHECK_VAL(mask, val, pfx)			\
 	BUILD_BUG_ON_MSG(__builtin_constant_p(val) &&		\
 			 ~((mask) >> __bf_shf(mask)) & (val),	\
 			 pfx "value too large for the field")
 
-#define __BF_FIELD_CHECK_REG(mask, reg, pfx)				\
-	BUILD_BUG_ON_MSG(mask + 0U + 0UL + 0ULL >			\
-			 ~0ULL >> (64 - 8 * sizeof (reg)),		\
+#define __BF_FIELD_CHECK_REG(mask, reg, pfx)			\
+	BUILD_BUG_ON_MSG(mask + 0U + 0UL + 0ULL >		\
+			 ~0ULL >> (64 - 8 * sizeof (reg)),	\
 			 pfx "type of reg too small for mask")
 
-#define __BF_FIELD_PREP(mask, val, pfx)					\
-	({								\
-		__BF_FIELD_CHECK_MASK(mask, pfx);			\
-		__BF_FIELD_CHECK_VAL(mask, val, pfx);			\
-		((val) << __bf_shf(mask)) & (mask);			\
-	})
+#define __BF_FIELD_PREP(mask, val, pfx)		\
+({						\
+	__BF_FIELD_CHECK_MASK(mask, pfx);	\
+	__BF_FIELD_CHECK_VAL(mask, val, pfx);	\
+	((val) << __bf_shf(mask)) & (mask);	\
+})
 
-#define __BF_FIELD_GET(mask, reg, pfx)					\
-	({								\
-		__BF_FIELD_CHECK_MASK(mask, pfx);			\
-		__BF_FIELD_CHECK_REG(mask, reg, pfx);			\
-		((reg) & (mask)) >> __bf_shf(mask);			\
-	})
+#define __BF_FIELD_GET(mask, reg, pfx)		\
+({						\
+	__BF_FIELD_CHECK_MASK(mask, pfx);	\
+	__BF_FIELD_CHECK_REG(mask, reg, pfx);	\
+	((reg) & (mask)) >> __bf_shf(mask);	\
+})
 
 /**
  * FIELD_MAX() - produce the maximum value representable by a field
@@ -87,12 +87,12 @@
  * FIELD_MAX() returns the maximum value that can be held in the field
  * specified by @mask.
  */
-#define FIELD_MAX(mask)							\
-	({								\
-		__auto_type _mask = mask;				\
-		__BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: ");		\
-		(_mask >> __bf_shf(_mask));				\
-	})
+#define FIELD_MAX(mask)					\
+({							\
+	__auto_type _mask = mask;			\
+	__BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: ");	\
+	(_mask >> __bf_shf(_mask));			\
+})
 
 /**
  * FIELD_FIT() - check if value fits in the field
@@ -101,13 +101,13 @@
  *
  * Return: true if @val can fit inside @mask, false if @val is too big.
  */
-#define FIELD_FIT(mask, val)						\
-	({								\
-		__auto_type _mask = mask;				\
-		__auto_type _val = 1 ? (val) : _mask;			\
-		__BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: ");		\
-		!((_val << __bf_shf(_mask)) & ~_mask); 			\
-	})
+#define FIELD_FIT(mask, val)				\
+({							\
+	__auto_type _mask = mask;			\
+	__auto_type _val = 1 ? (val) : _mask;		\
+	__BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: ");	\
+	!((_val << __bf_shf(_mask)) & ~_mask); 		\
+})
 
 /**
  * FIELD_PREP() - prepare a bitfield element
@@ -117,12 +117,12 @@
  * 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)						\
-	({								\
-		__auto_type _mask = mask;				\
-		__auto_type _val = 1 ? (val) : _mask;			\
-		__BF_FIELD_PREP(_mask, _val, "FIELD_PREP: ");		\
-	})
+#define FIELD_PREP(mask, val)				\
+({							\
+	__auto_type _mask = mask;			\
+	__auto_type _val = 1 ? (val) : _mask;		\
+	__BF_FIELD_PREP(_mask, _val, "FIELD_PREP: ");	\
+})
 
 /**
  * FIELD_PREP_CONST() - prepare a constant bitfield element
@@ -136,15 +136,15 @@
  * 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)					\
-	(								\
-		/* mask must be non-zero and contiguous */		\
-		BUILD_BUG_ON_ZERO(__BF_VALIDATE_MASK(mask)) +		\
-		/* check if value fits */				\
-		BUILD_BUG_ON_ZERO(~((mask) >> __bf_shf(mask)) & (val)) + \
-		/* and create the value */				\
-		(((typeof(mask))(val) << __bf_shf(mask)) & (mask))	\
-	)
+#define FIELD_PREP_CONST(mask, val)				\
+(								\
+	/* mask must be non-zero and contiguous */		\
+	BUILD_BUG_ON_ZERO(__BF_VALIDATE_MASK(mask)) +		\
+	/* check if value fits */				\
+	BUILD_BUG_ON_ZERO(~((mask) >> __bf_shf(mask)) & (val)) + \
+	/* and create the value */				\
+	(((typeof(mask))(val) << __bf_shf(mask)) & (mask))	\
+)
 
 /**
  * FIELD_GET() - extract a bitfield element
@@ -154,12 +154,12 @@
  * 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)						\
-	({								\
-		__auto_type _mask = mask;				\
-		__auto_type _reg = reg;					\
-		__BF_FIELD_GET(_mask, _reg, "FIELD_GET: ");		\
-	})
+#define FIELD_GET(mask, reg)				\
+({							\
+	__auto_type _mask = mask;			\
+	__auto_type _reg = reg;				\
+	__BF_FIELD_GET(_mask, _reg, "FIELD_GET: ");	\
+})
 
 /**
  * FIELD_MODIFY() - modify a bitfield element
@@ -170,16 +170,16 @@
  * 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)							\
-	({										\
-		__auto_type _mask = mask;						\
-		__auto_type _reg_p = reg_p;						\
-		__auto_type _val = 1 ? (val) : _mask;					\
-		__BF_FIELD_CHECK_MASK(_mask, "FIELD_MODIFY: ");				\
-		__BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_MODIFY: ");			\
-		__BF_FIELD_CHECK_REG(_mask, *_reg_p, "FIELD_MODIFY: ");			\
-		*_reg_p = (*_reg_p & ~_mask) | ((_val << __bf_shf(_mask)) & _mask);	\
-	})
+#define FIELD_MODIFY(mask, reg_p, val)						\
+({										\
+	__auto_type _mask = mask;						\
+	__auto_type _reg_p = reg_p;						\
+	__auto_type _val = 1 ? (val) : _mask;					\
+	__BF_FIELD_CHECK_MASK(_mask, "FIELD_MODIFY: ");				\
+	__BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_MODIFY: ");			\
+	__BF_FIELD_CHECK_REG(_mask, *_reg_p, "FIELD_MODIFY: ");			\
+	*_reg_p = (*_reg_p & ~_mask) | ((_val << __bf_shf(_mask)) & _mask);	\
+})
 
 extern void __compiletime_error("value doesn't fit into mask")
 __field_overflow(void);
-- 
2.39.5

[PATCH 8/9] bitfield: Add comment block for the host/fixed endian functions

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Copied almost verbatim from the commit message that added the functions.

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

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index bfd80ebd25b1..9feb489a8da3 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -181,6 +181,49 @@ do {								\
 	*_reg_p = (*_reg_p & ~_mask) | ((_val << __bf_shf(_mask)) & _mask);	\
 })
 
+/*
+ * Primitives for manipulating bitfields both in host- and fixed-endian.
+ *
+ * * u32 le32_get_bits(__le32 val, u32 field) extracts the contents of the
+ *   bitfield specified by @field in little-endian 32bit object @val and
+ *   converts it to host-endian.
+ *
+ * * void le32p_replace_bits(__le32 *p, u32 v, u32 field) replaces
+ *   the contents of the bitfield specified by @field in little-endian
+ *   32bit object pointed to by @p with the value of @v.  New value is
+ *   given in host-endian and stored as little-endian.
+ *
+ * * __le32 le32_replace_bits(__le32 old, u32 v, u32 field) is equivalent to
+ *   ({__le32 tmp = old; le32p_replace_bits(&tmp, v, field); tmp;})
+ *   In other words, instead of modifying an object in memory, it takes
+ *   the initial value and returns the modified one.
+ *
+ * * __le32 le32_encode_bits(u32 v, u32 field) is equivalent to
+ *   le32_replace_bits(0, v, field).  In other words, it returns a little-endian
+ *   32bit object with the bitfield specified by @field containing the
+ *   value of @v and all bits outside that bitfield being zero.
+ *
+ * Such set of helpers is defined for each of little-, big- and host-endian
+ * types; e.g. u64_get_bits(val, field) will return the contents of the bitfield
+ * specified by @field in host-endian 64bit object @val, etc.  Of course, for
+ * host-endian no conversion is involved.
+ *
+ * Fields to access are specified as GENMASK() values - an N-bit field
+ * starting at bit #M is encoded as GENMASK(M + N - 1, M).  Note that
+ * bit numbers refer to endianness of the object we are working with -
+ * e.g. GENMASK(11, 0) in __be16 refers to the second byte and the lower
+ * 4 bits of the first byte.  In __le16 it would refer to the first byte
+ * and the lower 4 bits of the second byte, etc.
+ *
+ * Field specification must be a constant; __builtin_constant_p() doesn't
+ * have to be true for it, but compiler must be able to evaluate it at
+ * build time.  If it cannot or if the value does not encode any bitfield,
+ * the build will fail.
+ *
+ * If the value being stored in a bitfield is a constant that does not fit
+ * into that bitfield, a warning will be generated at compile time.
+ */
+
 extern void __compiletime_error("value doesn't fit into mask")
 __field_overflow(void);
 extern void __compiletime_error("bad bitfield mask")
-- 
2.39.5

[PATCH 9/9] bitfield: Update comments for le/be functions

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Make it clear the the values are converted to host order before
being acted on.
Order the explantions with the simple functions first.

These still need converting to kerneldoc format.

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

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 9feb489a8da3..dcc4809b4706 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -184,24 +184,24 @@ do {								\
 /*
  * Primitives for manipulating bitfields both in host- and fixed-endian.
  *
- * * u32 le32_get_bits(__le32 val, u32 field) extracts the contents of the
- *   bitfield specified by @field in little-endian 32bit object @val and
- *   converts it to host-endian.
  *
- * * void le32p_replace_bits(__le32 *p, u32 v, u32 field) replaces
- *   the contents of the bitfield specified by @field in little-endian
- *   32bit object pointed to by @p with the value of @v.  New value is
- *   given in host-endian and stored as little-endian.
+ * * u32 le32_get_bits(__le32 val, u32 field) converts the little-endian 32bit
+ *   object @val to host-endian then extracts the contents of the bitfield
+ *   specified by @field.
+ *
+ * * __le32 le32_encode_bits(u32 v, u32 field) encodes the value of @v into
+ *   the bitfield specified by @field then converts the value to little-endian.
+ *   All the bits outside that bitfield being zero.
  *
- * * __le32 le32_replace_bits(__le32 old, u32 v, u32 field) is equivalent to
- *   ({__le32 tmp = old; le32p_replace_bits(&tmp, v, field); tmp;})
- *   In other words, instead of modifying an object in memory, it takes
- *   the initial value and returns the modified one.
+ * * __le32 le32_replace_bits(__le32 old, u32 v, u32 field) converts the
+ *   little-endian 32bit object @old to host order, replaces the contents
+ *   of the bitfield specified by @field with @v, then returns the value
+ *   converted back to little-endian.
  *
- * * __le32 le32_encode_bits(u32 v, u32 field) is equivalent to
- *   le32_replace_bits(0, v, field).  In other words, it returns a little-endian
- *   32bit object with the bitfield specified by @field containing the
- *   value of @v and all bits outside that bitfield being zero.
+ * * void le32p_replace_bits(__le32 *p, u32 v, u32 field) replaces
+ *   the contents of the bitfield specified by @field in little-endian
+ *   32bit object pointed to by @p with the value of @v.
+ *   Equivalent to *p = le32_replace_bits(*p, v, field).
  *
  * Such set of helpers is defined for each of little-, big- and host-endian
  * types; e.g. u64_get_bits(val, field) will return the contents of the bitfield
@@ -210,15 +210,13 @@ do {								\
  *
  * Fields to access are specified as GENMASK() values - an N-bit field
  * starting at bit #M is encoded as GENMASK(M + N - 1, M).  Note that
- * bit numbers refer to endianness of the object we are working with -
+ * bit numbers refer to the value after being converted to host order -
  * e.g. GENMASK(11, 0) in __be16 refers to the second byte and the lower
  * 4 bits of the first byte.  In __le16 it would refer to the first byte
  * and the lower 4 bits of the second byte, etc.
  *
- * Field specification must be a constant; __builtin_constant_p() doesn't
- * have to be true for it, but compiler must be able to evaluate it at
- * build time.  If it cannot or if the value does not encode any bitfield,
- * the build will fail.
+ * Field specification must be a non-zero constant, otherwise the build
+ * will fail.
  *
  * If the value being stored in a bitfield is a constant that does not fit
  * into that bitfield, a warning will be generated at compile time.
-- 
2.39.5

Re: [PATCH 3/9] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

[Andy Shevchenko]

On Tue, Dec 09, 2025 at 10:03:07AM +0000, david.laight.linux@gmail.com wrote:

> Instead of directly expanding __BF_FIELD_CHECK() (which really ought
> not be used outside bitfield) and open-coding the generation of the
> masked value, just call FIELD_PREP() and add an extra check for
> the mask being at most 16 bits.

...

> +#define FIELD_PREP_WM16(mask, val)				\
> +({								\
> +	__auto_type _mask = mask;				\
> +	u32 _val = FIELD_PREP(_mask, val);			\

> +	BUILD_BUG_ON_MSG(_mask > 0xffffu,			\
> +			 "FIELD_PREP_WM16: mask too large");	\

Can it be static_assert() instead?

> +	_val | (_mask << 16);					\
> +})

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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 8/9] bitfield: Add comment block for the host/fixed endian functions

[Andy Shevchenko]

On Tue, Dec 09, 2025 at 10:03:12AM +0000, david.laight.linux@gmail.com wrote:

> Copied almost verbatim from the commit message that added the functions.

...

> +/*

Can it be a global DOC for being processed by kernel-doc?

> + * Primitives for manipulating bitfields both in host- and fixed-endian.
> + *
> + * * u32 le32_get_bits(__le32 val, u32 field) extracts the contents of the
> + *   bitfield specified by @field in little-endian 32bit object @val and
> + *   converts it to host-endian.
> + *
> + * * void le32p_replace_bits(__le32 *p, u32 v, u32 field) replaces
> + *   the contents of the bitfield specified by @field in little-endian
> + *   32bit object pointed to by @p with the value of @v.  New value is
> + *   given in host-endian and stored as little-endian.
> + *
> + * * __le32 le32_replace_bits(__le32 old, u32 v, u32 field) is equivalent to
> + *   ({__le32 tmp = old; le32p_replace_bits(&tmp, v, field); tmp;})
> + *   In other words, instead of modifying an object in memory, it takes
> + *   the initial value and returns the modified one.
> + *
> + * * __le32 le32_encode_bits(u32 v, u32 field) is equivalent to
> + *   le32_replace_bits(0, v, field).  In other words, it returns a little-endian
> + *   32bit object with the bitfield specified by @field containing the
> + *   value of @v and all bits outside that bitfield being zero.
> + *
> + * Such set of helpers is defined for each of little-, big- and host-endian
> + * types; e.g. u64_get_bits(val, field) will return the contents of the bitfield
> + * specified by @field in host-endian 64bit object @val, etc.  Of course, for
> + * host-endian no conversion is involved.
> + *
> + * Fields to access are specified as GENMASK() values - an N-bit field
> + * starting at bit #M is encoded as GENMASK(M + N - 1, M).  Note that
> + * bit numbers refer to endianness of the object we are working with -
> + * e.g. GENMASK(11, 0) in __be16 refers to the second byte and the lower
> + * 4 bits of the first byte.  In __le16 it would refer to the first byte
> + * and the lower 4 bits of the second byte, etc.
> + *
> + * Field specification must be a constant; __builtin_constant_p() doesn't
> + * have to be true for it, but compiler must be able to evaluate it at
> + * build time.  If it cannot or if the value does not encode any bitfield,
> + * the build will fail.
> + *
> + * If the value being stored in a bitfield is a constant that does not fit
> + * into that bitfield, a warning will be generated at compile time.
> + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/9] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

[David Laight]

On Tue, 9 Dec 2025 17:46:21 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 09, 2025 at 10:03:07AM +0000, david.laight.linux@gmail.com wrote:
> 
> > Instead of directly expanding __BF_FIELD_CHECK() (which really ought
> > not be used outside bitfield) and open-coding the generation of the
> > masked value, just call FIELD_PREP() and add an extra check for
> > the mask being at most 16 bits.  
> 
> ...
> 
> > +#define FIELD_PREP_WM16(mask, val)				\
> > +({								\
> > +	__auto_type _mask = mask;				\
> > +	u32 _val = FIELD_PREP(_mask, val);			\  
> 
> > +	BUILD_BUG_ON_MSG(_mask > 0xffffu,			\
> > +			 "FIELD_PREP_WM16: mask too large");	\  
> 
> Can it be static_assert() instead?

No, they have to be 'integer constant expressions' not just
'compile time constants'.
Pretty useless for anything non-trivial.

	David

> 
> > +	_val | (_mask << 16);					\
> > +})  
> 

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

[David Laight]

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

[Andy Shevchenko]

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

Re: [PATCH 2/9] thunderblot: Don't pass a bitfield to FIELD_GET

[Mika Westerberg]

$subject has typo: thunderblot -> thunderbolt ;-)

On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> FIELD_GET needs to use __auto_type to get the value of the 'reg'
> parameter, this can't be used with bifields.
> 
> FIELD_GET also want to verify the size of 'reg' so can't add zero
> to force the type to int.
> 
> So add a zero here.
> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>
> ---
>  drivers/thunderbolt/tb.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index e96474f17067..7ca2b5a0f01e 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
>   */
>  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
>  {
> -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);

Can't this use a cast instead? If not then can you also add a comment here
because next someone will send a patch "fixing" the unnecessary addition.

>  }
>  
>  /**
> -- 
> 2.39.5

Re: [PATCH 8/9] bitfield: Add comment block for the host/fixed endian functions

[Jakub Kicinski]

On Tue,  9 Dec 2025 10:03:12 +0000 david.laight.linux@gmail.com wrote:
> + * * u32 le32_get_bits(__le32 val, u32 field) extracts the contents of the
> + *   bitfield specified by @field in little-endian 32bit object @val and
> + *   converts it to host-endian.

possibly also add declarations for these? So that ctags and co. sees
them?

Re: [PATCH 1/9] nfp: Call FIELD_PREP() in NFP_ETH_SET_BIT_CONFIG() wrapper

[Jakub Kicinski]

On Tue,  9 Dec 2025 10:03:05 +0000 david.laight.linux@gmail.com wrote:
> Rather than use a define that should be internal to the implementation
> of FIELD_PREP(), pass the shifted 'val' to nfp_eth_set_bit_config()
> and change the test for 'value unchanged' to match.
> 
> This is a simpler change than the one used to avoid calling both
> FIELD_GET() and FIELD_PREP() with non-constant mask values.

I'd like this code to be left out of the subjective churn please.
I like it the way I wrote it. I also liked the bitfield.h the way
I wrote it but I guess that part "belongs" to the community at large.

FWIW - thumbs up for patch 8, no opinion on the rest.

Re: [PATCH 2/9] thunderblot: Don't pass a bitfield to FIELD_GET

[David Laight]

On Wed, 10 Dec 2025 06:56:17 +0100
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> $subject has typo: thunderblot -> thunderbolt ;-)
> 
> On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > parameter, this can't be used with bifields.
> > 
> > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > to force the type to int.
> > 
> > So add a zero here.
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> >  drivers/thunderbolt/tb.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > index e96474f17067..7ca2b5a0f01e 100644
> > --- a/drivers/thunderbolt/tb.h
> > +++ b/drivers/thunderbolt/tb.h
> > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> >   */
> >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> >  {
> > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);  
> 
> Can't this use a cast instead? If not then can you also add a comment here
> because next someone will send a patch "fixing" the unnecessary addition.

A cast can do other (possibly incorrect) conversions, adding zero is never going
to so any 'damage' - even if it looks a bit odd.

Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
just do:
	/* The major version is in the top 3 bits */
	return sw->config.thunderbolt_version > 5;

The only other uses of thunderbolt_version are debug prints (in decimal).

	David

> 
> >  }
> >  
> >  /**
> > -- 
> > 2.39.5  

Re: [PATCH 2/9] thunderblot: Don't pass a bitfield to FIELD_GET

[Mika Westerberg]

On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:
> On Wed, 10 Dec 2025 06:56:17 +0100
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > $subject has typo: thunderblot -> thunderbolt ;-)
> > 
> > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:
> > > From: David Laight <david.laight.linux@gmail.com>
> > > 
> > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > parameter, this can't be used with bifields.
> > > 
> > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > to force the type to int.
> > > 
> > > So add a zero here.
> > > 
> > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > ---
> > >  drivers/thunderbolt/tb.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > index e96474f17067..7ca2b5a0f01e 100644
> > > --- a/drivers/thunderbolt/tb.h
> > > +++ b/drivers/thunderbolt/tb.h
> > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > >   */
> > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > >  {
> > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);  
> > 
> > Can't this use a cast instead? If not then can you also add a comment here
> > because next someone will send a patch "fixing" the unnecessary addition.
> 
> A cast can do other (possibly incorrect) conversions, adding zero is never going
> to so any 'damage' - even if it looks a bit odd.
> 
> Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> just do:
> 	/* The major version is in the top 3 bits */
> 	return sw->config.thunderbolt_version > 5;

You mean 

	return sw->config.thunderbolt_version >> 5;

?

Yes that works but I prefer then:

	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;

> 
> The only other uses of thunderbolt_version are debug prints (in decimal).
> 
> 	David
> 
> > 
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.39.5  

Re: [PATCH 1/9] nfp: Call FIELD_PREP() in NFP_ETH_SET_BIT_CONFIG() wrapper

[David Laight]

On Wed, 10 Dec 2025 18:29:47 +0900
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue,  9 Dec 2025 10:03:05 +0000 david.laight.linux@gmail.com wrote:
> > Rather than use a define that should be internal to the implementation
> > of FIELD_PREP(), pass the shifted 'val' to nfp_eth_set_bit_config()
> > and change the test for 'value unchanged' to match.
> > 
> > This is a simpler change than the one used to avoid calling both
> > FIELD_GET() and FIELD_PREP() with non-constant mask values.  
> 
> I'd like this code to be left out of the subjective churn please.
> I like it the way I wrote it.

The 'problem' is that I want to remove __BF_FIELD_CHECK().
It has already been split into two (for 6.19) but it makes sense
to split into three (to avoid code-bloat in the cpp output).

IMHO Using a define that is part of the implementation of FIELD_xxxx()
is wrong anyway.

> I also liked the bitfield.h the way
> I wrote it but I guess that part "belongs" to the community at large.

There are already significant changes there for 6.19-rc1

	David

> 
> FWIW - thumbs up for patch 8, no opinion on the rest.

Re: [PATCH 8/9] bitfield: Add comment block for the host/fixed endian functions

[David Laight]

On Wed, 10 Dec 2025 18:23:00 +0900
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue,  9 Dec 2025 10:03:12 +0000 david.laight.linux@gmail.com wrote:
> > + * * u32 le32_get_bits(__le32 val, u32 field) extracts the contents of the
> > + *   bitfield specified by @field in little-endian 32bit object @val and
> > + *   converts it to host-endian.  
> 
> possibly also add declarations for these? So that ctags and co. sees
> them?

The functions are bulk-generated using a #define, ctags is never going to
find definitions.

Adding kerneldoc comments is also painful.
I don't think it lets you use a single comment for multiple functions.

	David

Re: [PATCH 2/9] thunderblot: Don't pass a bitfield to FIELD_GET

[David Laight]

On Wed, 10 Dec 2025 10:41:02 +0100
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:
> > On Wed, 10 Dec 2025 06:56:17 +0100
> > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> >   
> > > $subject has typo: thunderblot -> thunderbolt ;-)
> > > 
> > > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:  
> > > > From: David Laight <david.laight.linux@gmail.com>
> > > > 
> > > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > > parameter, this can't be used with bifields.
> > > > 
> > > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > > to force the type to int.
> > > > 
> > > > So add a zero here.
> > > > 
> > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > ---
> > > >  drivers/thunderbolt/tb.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > > index e96474f17067..7ca2b5a0f01e 100644
> > > > --- a/drivers/thunderbolt/tb.h
> > > > +++ b/drivers/thunderbolt/tb.h
> > > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > > >   */
> > > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > > >  {
> > > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);    
> > > 
> > > Can't this use a cast instead? If not then can you also add a comment here
> > > because next someone will send a patch "fixing" the unnecessary addition.  
> > 
> > A cast can do other (possibly incorrect) conversions, adding zero is never going
> > to so any 'damage' - even if it looks a bit odd.
> > 
> > Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> > just do:
> > 	/* The major version is in the top 3 bits */
> > 	return sw->config.thunderbolt_version > 5;  
> 
> You mean 
> 
> 	return sw->config.thunderbolt_version >> 5;
> 
> ?
> 
> Yes that works but I prefer then:
> 
> 	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;

I've put that in for the next version (without the comment line).

	David

> 
> > 
> > The only other uses of thunderbolt_version are debug prints (in decimal).
> > 
> > 	David
> >   
> > >   
> > > >  }
> > > >  
> > > >  /**
> > > > -- 
> > > > 2.39.5    
> 

Re: [PATCH 2/9] thunderblot: Don't pass a bitfield to FIELD_GET

[Yury Norov]

On Wed, Dec 10, 2025 at 10:18:42AM +0000, David Laight wrote:
> On Wed, 10 Dec 2025 10:41:02 +0100
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:
> > > On Wed, 10 Dec 2025 06:56:17 +0100
> > > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> > >   
> > > > $subject has typo: thunderblot -> thunderbolt ;-)
> > > > 
> > > > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:  
> > > > > From: David Laight <david.laight.linux@gmail.com>
> > > > > 
> > > > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > > > parameter, this can't be used with bifields.
> > > > > 
> > > > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > > > to force the type to int.
> > > > > 
> > > > > So add a zero here.
> > > > > 
> > > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > > ---
> > > > >  drivers/thunderbolt/tb.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > > > index e96474f17067..7ca2b5a0f01e 100644
> > > > > --- a/drivers/thunderbolt/tb.h
> > > > > +++ b/drivers/thunderbolt/tb.h
> > > > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > > > >   */
> > > > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > > > >  {
> > > > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);    
> > > > 
> > > > Can't this use a cast instead? If not then can you also add a comment here
> > > > because next someone will send a patch "fixing" the unnecessary addition.  
> > > 
> > > A cast can do other (possibly incorrect) conversions, adding zero is never going
> > > to so any 'damage' - even if it looks a bit odd.
> > > 
> > > Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> > > just do:
> > > 	/* The major version is in the top 3 bits */
> > > 	return sw->config.thunderbolt_version > 5;  
> > 
> > You mean 
> > 
> > 	return sw->config.thunderbolt_version >> 5;
> > 
> > ?
> > 
> > Yes that works but I prefer then:
> > 
> > 	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;
> 
> I've put that in for the next version (without the comment line).

FIELD_GET() is here exactly to let people to not opencode this
error-prone bit manipulation. So, let's continue using it.

David, can you explain in details why this code needs to be fixed? Why
and when typecast wouldn't work so that you have to use an ugly '+0'
hack, or even drop the FIELD_GET().

My current understanding is that the existing FIELD_GET()
implementation works well with any data types, including bitfields,
and what you suggested in this series - does not.

If it's correct, I don't think that switching to your version is
well-justified.

Thanks,
Yury

Re: [PATCH 0/9] bitfield: tidy up bitfield.h

[Yury Norov]

On Tue, Dec 09, 2025 at 10:03:04AM +0000, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> Re-send with patches going to everyone.
> (I'd forgotten I'd set 'ccCover = 0'.)

And this one again appeared in my spambox. Have you any ideas why?
 
> I noticed some very long (18KB) error messages from the compiler.
> Turned out they were errors on lines that passed GENMASK() to FIELD_PREP().
> Since most of the #defines are already statement functions the values
> can be copied to locals so the actual parameters only get expanded once.
> 
> The 'bloat' is reduced further by using a simple test to ensure 'reg'
> is large enough, slightly simplifying the test for constant 'val' and
> only checking 'reg' and 'val' when the parameters are present.

So, can you share the before/after?

> The first two patches are slightly problematic.
> 
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c manages to use
> a #define that should be an internal to bitfield.h, the changed file
> is actually more similar to the previous version.
> 
> drivers/thunderbolt/tb.h passes a bifield to FIELD_GET(), these can't
> be used with sizeof or __auto_type. The usual solution is to add zero,
> but that can't be done in FIELD_GET() because it doesn't want the value
> promoted to 'int' (no idea how _Generic() treated it.)
> The fix is just to add zero at the call site.
> (The bitfield seems to be in a structure rad from hardware - no idea
> how that works on BE (or any LE that uses an unusual order for bitfields.)
> 
> Both changes may need to to through the same tree as the header file changes.
> 
> The changes are based on 'next' and contain the addition of field_prep()
> and field_get() for non-constant values.
> 
> I also know it is the merge window.
> I expect to be generating a v2 in the new year (someone always has a comment).
> 
> David Laight (9):
>   nfp: Call FIELD_PREP() in NFP_ETH_SET_BIT_CONFIG() wrapper
>   thunderblot: Don't pass a bitfield to FIELD_GET
>   bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()
>   bitfield: Copy #define parameters to locals
>   bitfield: FIELD_MODIFY: Only do a single read/write on the target
>   bitfield: Update sanity checks
>   bitfield: Reduce indentation
>   bitfield: Add comment block for the host/fixed endian functions
>   bitfield: Update comments for le/be functions
> 
>  .../netronome/nfp/nfpcore/nfp_nsp_eth.c       |  16 +-
>  drivers/thunderbolt/tb.h                      |   2 +-
>  include/linux/bitfield.h                      | 278 ++++++++++--------
>  include/linux/hw_bitfield.h                   |  17 +-
>  4 files changed, 166 insertions(+), 147 deletions(-)
> 
> -- 
> 2.39.5

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

[Yury Norov]

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 3/9] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

[Nicolas Frattaroli]

On Tuesday, 9 December 2025 11:03:07 Central European Standard Time david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> Instead of directly expanding __BF_FIELD_CHECK() (which really ought
> not be used outside bitfield) and open-coding the generation of the
> masked value, just call FIELD_PREP() and add an extra check for
> the mask being at most 16 bits.
> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>
> ---
>  include/linux/hw_bitfield.h | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
> index df202e167ce4..d7f21b60449b 100644
> --- a/include/linux/hw_bitfield.h
> +++ b/include/linux/hw_bitfield.h
> @@ -23,15 +23,14 @@
>   * register, a bit in the lower half is only updated if the corresponding bit
>   * in the upper half is high.
>   */
> -#define FIELD_PREP_WM16(_mask, _val)					     \
> -	({								     \
> -		typeof(_val) __val = _val;				     \
> -		typeof(_mask) __mask = _mask;				     \
> -		__BF_FIELD_CHECK(__mask, ((u16)0U), __val,		     \
> -				 "HWORD_UPDATE: ");			     \
> -		(((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) | \
> -		((__mask) << 16);					     \
> -	})
> +#define FIELD_PREP_WM16(mask, val)				\
> +({								\
> +	__auto_type _mask = mask;				\
> +	u32 _val = FIELD_PREP(_mask, val);			\
> +	BUILD_BUG_ON_MSG(_mask > 0xffffu,			\
> +			 "FIELD_PREP_WM16: mask too large");	\
> +	_val | (_mask << 16);					\
> +})
>  
>  /**
>   * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
> 

This breaks the build for at least one driver that uses
FIELD_PREP_WM16, namely phy-rockchip-emmc.c:

drivers/phy/rockchip/phy-rockchip-emmc.c:109:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  109 |                      HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/phy/rockchip/phy-rockchip-emmc.c:114:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  114 |                      HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/phy/rockchip/phy-rockchip-emmc.c:167:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  167 |                      HIWORD_UPDATE(PHYCTRL_PDB_PWR_ON,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/phy/rockchip/phy-rockchip-emmc.c:190:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  190 |                      HIWORD_UPDATE(freqsel, PHYCTRL_FREQSEL_MASK,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/phy/rockchip/phy-rockchip-emmc.c:196:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  196 |                      HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/phy/rockchip/phy-rockchip-emmc.c:291:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  291 |                      HIWORD_UPDATE(rk_phy->drive_impedance,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/phy/rockchip/phy-rockchip-emmc.c:298:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  298 |                      HIWORD_UPDATE(PHYCTRL_OTAPDLYENA,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/phy/rockchip/phy-rockchip-emmc.c:305:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  305 |                      HIWORD_UPDATE(rk_phy->output_tapdelay_select,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/phy/rockchip/phy-rockchip-emmc.c:312:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  312 |                      HIWORD_UPDATE(rk_phy->enable_strobe_pulldown,
      |                      ^
drivers/phy/rockchip/phy-rockchip-emmc.c:25:4: note: expanded from macro 'HIWORD_UPDATE'
   25 |                 (FIELD_PREP_WM16((mask) << (shift), (val)))
      |                  ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^

Maybe the wrapping in HIWORD_UPDATE (which was done to make the
transitionary patch easier) is playing a role here.

pcie-dw-rockchip.c is similarly broken by this change, except
without the superfluous wrapper:

drivers/pci/controller/dwc/pcie-dw-rockchip.c:191:37: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  191 |         rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM,
      |                                            ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:43:35: note: expanded from macro 'PCIE_CLIENT_ENABLE_LTSSM'
   43 | #define  PCIE_CLIENT_ENABLE_LTSSM       FIELD_PREP_WM16(BIT(2), 1)
      |                                         ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:197:37: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  197 |         rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DISABLE_LTSSM,
      |                                            ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:44:36: note: expanded from macro 'PCIE_CLIENT_DISABLE_LTSSM'
   44 | #define  PCIE_CLIENT_DISABLE_LTSSM      FIELD_PREP_WM16(BIT(2), 0)
      |                                         ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:222:38: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  222 |                 rockchip_pcie_writel_apb(rockchip, PCIE_CLKREQ_READY,
      |                                                    ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:67:29: note: expanded from macro 'PCIE_CLKREQ_READY'
   67 | #define  PCIE_CLKREQ_READY              FIELD_PREP_WM16(BIT(0), 1)
      |                                         ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:234:6: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  234 |                                  PCIE_CLKREQ_PULL_DOWN | PCIE_CLKREQ_NOT_READY,
      |                                  ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:69:33: note: expanded from macro 'PCIE_CLKREQ_PULL_DOWN'
   69 | #define  PCIE_CLKREQ_PULL_DOWN          FIELD_PREP_WM16(GENMASK(13, 12), 1)
      |                                         ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:234:30: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  234 |                                  PCIE_CLKREQ_PULL_DOWN | PCIE_CLKREQ_NOT_READY,
      |                                                          ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:68:33: note: expanded from macro 'PCIE_CLKREQ_NOT_READY'
   68 | #define  PCIE_CLKREQ_NOT_READY          FIELD_PREP_WM16(BIT(0), 0)
      |                                         ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:535:9: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  535 |                 val = FIELD_PREP_WM16(PCIE_LTSSM_APP_DLY2_DONE, 1);
      |                       ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:574:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  574 |         val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1);
      |               ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:578:6: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  578 |                                  PCIE_CLIENT_SET_MODE(PCIE_CLIENT_MODE_RC),
      |                                  ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:41:34: note: expanded from macro 'PCIE_CLIENT_SET_MODE'
   41 | #define  PCIE_CLIENT_SET_MODE(x)        FIELD_PREP_WM16(PCIE_CLIENT_MODE_MASK, (x))
      |                                         ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:592:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  592 |         val = FIELD_PREP_WM16(PCIE_RDLH_LINK_UP_CHGED, 0);
      |               ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:624:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  624 |         val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1) |
      |               ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:625:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  625 |               FIELD_PREP_WM16(PCIE_LTSSM_APP_DLY2_EN, 1);
      |               ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:629:6: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  629 |                                  PCIE_CLIENT_SET_MODE(PCIE_CLIENT_MODE_EP),
      |                                  ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:41:34: note: expanded from macro 'PCIE_CLIENT_SET_MODE'
   41 | #define  PCIE_CLIENT_SET_MODE(x)        FIELD_PREP_WM16(PCIE_CLIENT_MODE_MASK, (x))
      |                                         ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:653:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  653 |         val = FIELD_PREP_WM16(PCIE_RDLH_LINK_UP_CHGED, 0) |
      |               ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
drivers/pci/controller/dwc/pcie-dw-rockchip.c:654:8: error: variable '_mask' declared with deduced type '__auto_type' cannot appear in its own initializer
  654 |               FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
      |               ^
include/linux/hw_bitfield.h:29:24: note: expanded from macro 'FIELD_PREP_WM16'
   29 |         u32 _val = FIELD_PREP(_mask, val);                      \
      |                               ^
14 errors generated.

Kind regards,
Nicolas Frattaroli

Re: [PATCH 2/9] thunderblot: Don't pass a bitfield to FIELD_GET

[David Laight]

On Wed, 10 Dec 2025 13:13:55 -0500
Yury Norov <yury.norov@gmail.com> wrote:

> On Wed, Dec 10, 2025 at 10:18:42AM +0000, David Laight wrote:
> > On Wed, 10 Dec 2025 10:41:02 +0100
> > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> >   
> > > On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:  
> > > > On Wed, 10 Dec 2025 06:56:17 +0100
> > > > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> > > >     
> > > > > $subject has typo: thunderblot -> thunderbolt ;-)
> > > > > 
> > > > > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:    
> > > > > > From: David Laight <david.laight.linux@gmail.com>
> > > > > > 
> > > > > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > > > > parameter, this can't be used with bifields.
> > > > > > 
> > > > > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > > > > to force the type to int.
> > > > > > 
> > > > > > So add a zero here.
> > > > > > 
> > > > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > > > ---
> > > > > >  drivers/thunderbolt/tb.h | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > > > > index e96474f17067..7ca2b5a0f01e 100644
> > > > > > --- a/drivers/thunderbolt/tb.h
> > > > > > +++ b/drivers/thunderbolt/tb.h
> > > > > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > > > > >   */
> > > > > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > > > > >  {
> > > > > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > > > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);      
> > > > > 
> > > > > Can't this use a cast instead? If not then can you also add a comment here
> > > > > because next someone will send a patch "fixing" the unnecessary addition.    
> > > > 
> > > > A cast can do other (possibly incorrect) conversions, adding zero is never going
> > > > to so any 'damage' - even if it looks a bit odd.
> > > > 
> > > > Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> > > > just do:
> > > > 	/* The major version is in the top 3 bits */
> > > > 	return sw->config.thunderbolt_version > 5;    
> > > 
> > > You mean 
> > > 
> > > 	return sw->config.thunderbolt_version >> 5;
> > > 
> > > ?
> > > 
> > > Yes that works but I prefer then:
> > > 
> > > 	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;  
> > 
> > I've put that in for the next version (without the comment line).  
> 
> FIELD_GET() is here exactly to let people to not opencode this
> error-prone bit manipulation. So, let's continue using it.
> 
> David, can you explain in details why this code needs to be fixed? Why
> and when typecast wouldn't work so that you have to use an ugly '+0'
> hack, or even drop the FIELD_GET().
> 
> My current understanding is that the existing FIELD_GET()
> implementation works well with any data types, including bitfields,
> and what you suggested in this series - does not.

The underlying issue is that FIELD_GET() does a check that the supplied 'reg'
field is large enough for the mask.
In this case the 'reg' is a bitfield and you can't use sizeof(), typeof() or
__auto_type on a bitfield.

I don't want to (for example) add zero inside FIELD_GET() (as is done
for 'val') because that would promote u8/u16 to 32 bits - making the
size check less useful.

Ok, the current version relies on how the compiler happens to treat:
	_Generic(foo->bitfield, ...)
clang seems to treat a bitfield as 'int' (so sizeof() the result is 4)
regardless of the width.
OTOH gcc treats 'u32 bits:8' as 'unsigned char', but bits:7 selects
the 'default' and then __unsigned_scaler_typeof() fails.

I don't know what the standard says - but at least one of the compilers
is buggy.

So the current code doesn't really work with bitfields at all.

An alternate change would be to make the 4 fields in DWORD (sic) 4
just u8 instead of u32 xxx:8.
This would be similar to the two u16 at the top.

Perhaps that is the best fix.

	David

> 
> If it's correct, I don't think that switching to your version is
> well-justified.
> 
> Thanks,
> Yury

Re: [PATCH 3/9] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

[David Laight]

On Wed, 10 Dec 2025 20:18:30 +0100
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> On Tuesday, 9 December 2025 11:03:07 Central European Standard Time david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > Instead of directly expanding __BF_FIELD_CHECK() (which really ought
> > not be used outside bitfield) and open-coding the generation of the
> > masked value, just call FIELD_PREP() and add an extra check for
> > the mask being at most 16 bits.
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> >  include/linux/hw_bitfield.h | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
> > index df202e167ce4..d7f21b60449b 100644
> > --- a/include/linux/hw_bitfield.h
> > +++ b/include/linux/hw_bitfield.h
> > @@ -23,15 +23,14 @@
> >   * register, a bit in the lower half is only updated if the corresponding bit
> >   * in the upper half is high.
> >   */
> > -#define FIELD_PREP_WM16(_mask, _val)					     \
> > -	({								     \
> > -		typeof(_val) __val = _val;				     \
> > -		typeof(_mask) __mask = _mask;				     \
> > -		__BF_FIELD_CHECK(__mask, ((u16)0U), __val,		     \
> > -				 "HWORD_UPDATE: ");			     \
> > -		(((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) | \
> > -		((__mask) << 16);					     \
> > -	})
> > +#define FIELD_PREP_WM16(mask, val)				\
> > +({								\
> > +	__auto_type _mask = mask;				\
> > +	u32 _val = FIELD_PREP(_mask, val);			\
> > +	BUILD_BUG_ON_MSG(_mask > 0xffffu,			\
> > +			 "FIELD_PREP_WM16: mask too large");	\
> > +	_val | (_mask << 16);					\
> > +})
> >  
> >  /**
> >   * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
> >   
> 
> This breaks the build for at least one driver that uses
> FIELD_PREP_WM16, namely phy-rockchip-emmc.c:

Not in my allmodconfig build.
... 
> pcie-dw-rockchip.c is similarly broken by this change, except
> without the superfluous wrapper:

That one did get built.

The problem is that FIELD_PREP_WM16() needs to use different 'local'
variables than FIELD_PREP().
The 'proper' fix is to use unique names (as min() and max() do), but that
makes the whole thing unreadable and is best avoided unless nesting is
likely.
In this case s/mask/wm16_mask/ and s/val/wm16_val/ might be best.

	David

Re: [PATCH 0/9] bitfield: tidy up bitfield.h

[David Laight]

On Wed, 10 Dec 2025 13:20:16 -0500
Yury Norov <yury.norov@gmail.com> wrote:

> On Tue, Dec 09, 2025 at 10:03:04AM +0000, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > Re-send with patches going to everyone.
> > (I'd forgotten I'd set 'ccCover = 0'.)  
> 
> And this one again appeared in my spambox. Have you any ideas why?

I'm getting the copies sent to my gmail account, but gmail has a mind of its own.
It bounces and spams a lot of list emails (never might the emails/minute limit
that meany you only get 'some of' lkml).

The email headers are a slight lie, the From: doesn't match the 'envelope from'.
Basically you can't send more than 100 emails/day (count of To: and Cc:) from a
'free' gmail address, so they are sent from elsewhere (with a 500/day limit).
But I don't want that address appearing in the emails - hence the subterfuge.
But the 'envelope from' is correct for where the emails come from.
(A test email to xxx.gmail.com with an envelope from of xxx.gmail.com sent
from a different smtp server confused gmail! The mail was errored, but it sent
the bounce to itself!)

	David

Re: [PATCH 8/9] bitfield: Add comment block for the host/fixed endian functions

[Jakub Kicinski]

On Wed, 10 Dec 2025 10:08:46 +0000 David Laight wrote:
> > possibly also add declarations for these? So that ctags and co. sees
> > them?  
> 
> The functions are bulk-generated using a #define, ctags is never going to
> find definitions.

I know. That's why I said declarations. But for code completions etc
decl is enough.

Re: [PATCH 0/9] bitfield: tidy up bitfield.h

[David Laight]

On Wed, 10 Dec 2025 13:20:16 -0500
Yury Norov <yury.norov@gmail.com> wrote:

> On Tue, Dec 09, 2025 at 10:03:04AM +0000, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
...
> > I noticed some very long (18KB) error messages from the compiler.
> > Turned out they were errors on lines that passed GENMASK() to FIELD_PREP().
> > Since most of the #defines are already statement functions the values
> > can be copied to locals so the actual parameters only get expanded once.
> > 
> > The 'bloat' is reduced further by using a simple test to ensure 'reg'
> > is large enough, slightly simplifying the test for constant 'val' and
> > only checking 'reg' and 'val' when the parameters are present.  
> 
> So, can you share the before/after?

Not that hard to generate since the kernel makefiles will create foo.i

I would have fed the .i file though xargs - but someone broke it
(there is no option to ignore quotes, and -s70 isn't allowed).
So I used:
tr ' ' '\n' foo.i | (ll=; while read -r l; do ll1="$ll $l"; [ ${#l} = 0 -o ${#ll1} -ge 70 ] && { echo "$ll"; ll="$l";} || ll="$ll1"; done; echo "$ll")

GENMASK(hi, lo)
    ((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))

A chunk of that would be removed by changing type_max() to do
2 * (n - 1) + 1 instead of (n - 1) + n, but for unsigned types
it isn't needed.
Changing type_max(t) to (t)-1 GENMASK(hi, lo) becomes
(patch not posted...)
    ((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) + ((unsigned long)-1
    << (lo) & (unsigned long)-1 >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))
There are still four expansions of both lo and hi - but they are
usually trivial. 

You asked for this one :-)
old FIELD_GET(GENMASK(hi, lo), reg)
    ({ do { __attribute__((__noreturn__)) extern void
    __compiletime_assert_769(void) __attribute__((__error__("FIELD_GET: "
    "type of reg too small for mask"))); if (!(!(((typeof(
    _Generic((((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))), char: (unsigned
    char)0, unsigned char: (unsigned char)0, signed char: (unsigned
    char)0, unsigned short: (unsigned short)0, signed short: (unsigned
    short)0, unsigned int: (unsigned int)0, signed int: (unsigned int)0,
    unsigned long: (unsigned long)0, signed long: (unsigned long)0,
    unsigned long long: (unsigned long long)0, signed long long:
    (unsigned long long)0, default: (((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))))))(((unsigned
    long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi))))))) > ((typeof(
    _Generic((reg), char: (unsigned char)0, unsigned char: (unsigned
    char)0, signed char: (unsigned char)0, unsigned short: (unsigned
    short)0, signed short: (unsigned short)0, unsigned int: (unsigned
    int)0, signed int: (unsigned int)0, unsigned long: (unsigned long)0,
    signed long: (unsigned long)0, unsigned long long: (unsigned long
    long)0, signed long long: (unsigned long long)0, default:
    (reg))))(~0ull))))) __compiletime_assert_769(); } while (0); ({ ({ do
    { __attribute__((__noreturn__)) extern void
    __compiletime_assert_770(void) __attribute__((__error__("FIELD_GET: "
    "mask is not constant"))); if (!(!(!__builtin_constant_p(((unsigned
    long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))))))
    __compiletime_assert_770(); } while (0); do {
    __attribute__((__noreturn__)) extern void
    __compiletime_assert_771(void) __attribute__((__error__("FIELD_GET: "
    "mask is zero"))); if (!(!((((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) == 0)))
    __compiletime_assert_771(); } while (0); do {
    __attribute__((__noreturn__)) extern void
    __compiletime_assert_772(void) __attribute__((__error__("FIELD_GET: "
    "value too large for the field"))); if (!(!(__builtin_constant_p(0U)
    ? ~((((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) >>
    (__builtin_ffsll(((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) - 1)) & (0 + (0U)) :
    0))) __compiletime_assert_772(); } while (0); do {
    __attribute__((__noreturn__)) extern void
    __compiletime_assert_773(void) __attribute__((__error__("BUILD_BUG_ON
    failed: " "(((((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), \"const_true((lo) > (hi))\" \" is true\");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) + (1ULL <<
    (__builtin_ffsll(((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), \"const_true((lo) > (hi))\" \" is true\");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) - 1))) &
    (((((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), \"const_true((lo) > (hi))\" \" is true\");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) + (1ULL <<
    (__builtin_ffsll(((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), \"const_true((lo) > (hi))\" \" is true\");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) - 1))) - 1)) !=
    0"))); if (!(!((((((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) + (1ULL <<
    (__builtin_ffsll(((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) - 1))) &
    (((((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) + (1ULL <<
    (__builtin_ffsll(((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) - 1))) - 1)) != 0)))
    __compiletime_assert_773(); } while (0); }); (typeof(((unsigned
    long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))))(((reg) & (((unsigned
    long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi))))))) >>
    (__builtin_ffsll(((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi)))))) - 1)); }); })

new FIELD_GET(GENMASK(hi, lo), reg)
    ({ __auto_type _mask = ((unsigned long)(((int)sizeof(struct
    {_Static_assert(!(__builtin_choose_expr((sizeof(int) == sizeof(*(8 ?
    ((void *)((long)((lo) > (hi)) * 0l)) : (int *)8))), (lo) > (hi),
    false)), "const_true((lo) > (hi))" " is true");})) +
    (((typeof(unsigned long))((((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))) - 1) + ((typeof(unsigned long))1 <<
    (8*sizeof(typeof(unsigned long)) - 1 - (((typeof(unsigned long))(-1))
    < ( typeof(unsigned long))1))))) << (lo) & ((typeof(unsigned
    long))((((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long))
    - 1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1))) -
    1) + ((typeof(unsigned long))1 << (8*sizeof(typeof(unsigned long)) -
    1 - (((typeof(unsigned long))(-1)) < ( typeof(unsigned long))1)))))
    >> ((sizeof(unsigned long) * 8) - 1 - (hi))))); __auto_type _reg =
    reg; ({ do { do { __attribute__((__noreturn__)) extern void
    __compiletime_assert_697(void) __attribute__((__error__("FIELD_GET: "
    "mask is not constant"))); if (!(!(!__builtin_constant_p(_mask))))
    __compiletime_assert_697(); } while (0); do {
    __attribute__((__noreturn__)) extern void
    __compiletime_assert_698(void) __attribute__((__error__("FIELD_GET: "
    "mask is zero or not contiguous"))); if (!(!((!(_mask) || ((_mask) &
    ((_mask) + ((_mask) & -(_mask)))))))) __compiletime_assert_698(); }
    while (0); } while (0); do { __attribute__((__noreturn__)) extern
    void __compiletime_assert_699(void)
    __attribute__((__error__("FIELD_GET: " "type of reg too small for
    mask"))); if (!(!(_mask + 0U + 0UL + 0ULL > ~0ULL >> (64 - 8 * sizeof
    (_reg))))) __compiletime_assert_699(); } while (0); ((_reg) &
    (_mask)) >> (__builtin_ffsll(_mask) - 1); }); })

	David

Re: [PATCH 3/9] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

[Nicolas Frattaroli]

On Wednesday, 10 December 2025 21:59:15 Central European Standard Time David Laight wrote:
> On Wed, 10 Dec 2025 20:18:30 +0100
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> 
> > On Tuesday, 9 December 2025 11:03:07 Central European Standard Time david.laight.linux@gmail.com wrote:
> > > From: David Laight <david.laight.linux@gmail.com>
> > > 
> > > Instead of directly expanding __BF_FIELD_CHECK() (which really ought
> > > not be used outside bitfield) and open-coding the generation of the
> > > masked value, just call FIELD_PREP() and add an extra check for
> > > the mask being at most 16 bits.
> > > 
> > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > ---
> > >  include/linux/hw_bitfield.h | 17 ++++++++---------
> > >  1 file changed, 8 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
> > > index df202e167ce4..d7f21b60449b 100644
> > > --- a/include/linux/hw_bitfield.h
> > > +++ b/include/linux/hw_bitfield.h
> > > @@ -23,15 +23,14 @@
> > >   * register, a bit in the lower half is only updated if the corresponding bit
> > >   * in the upper half is high.
> > >   */
> > > -#define FIELD_PREP_WM16(_mask, _val)					     \
> > > -	({								     \
> > > -		typeof(_val) __val = _val;				     \
> > > -		typeof(_mask) __mask = _mask;				     \
> > > -		__BF_FIELD_CHECK(__mask, ((u16)0U), __val,		     \
> > > -				 "HWORD_UPDATE: ");			     \
> > > -		(((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) | \
> > > -		((__mask) << 16);					     \
> > > -	})
> > > +#define FIELD_PREP_WM16(mask, val)				\
> > > +({								\
> > > +	__auto_type _mask = mask;				\
> > > +	u32 _val = FIELD_PREP(_mask, val);			\
> > > +	BUILD_BUG_ON_MSG(_mask > 0xffffu,			\
> > > +			 "FIELD_PREP_WM16: mask too large");	\
> > > +	_val | (_mask << 16);					\
> > > +})
> > >  
> > >  /**
> > >   * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
> > >   
> > 
> > This breaks the build for at least one driver that uses
> > FIELD_PREP_WM16, namely phy-rockchip-emmc.c:
> 
> Not in my allmodconfig build.
> ... 
> > pcie-dw-rockchip.c is similarly broken by this change, except
> > without the superfluous wrapper:
> 
> That one did get built.

I build with clang 21.1.6 for arm64, in case that's any help.
I don't see how pcie-dw-rockchip.c built for you if FIELD_PREP
and FIELD_PREP_WM16 have conflicting symbol names?

> 
> The problem is that FIELD_PREP_WM16() needs to use different 'local'
> variables than FIELD_PREP().
> The 'proper' fix is to use unique names (as min() and max() do), but that
> makes the whole thing unreadable and is best avoided unless nesting is
> likely.
> In this case s/mask/wm16_mask/ and s/val/wm16_val/ might be best.
> 
> 	David
> 
> 

Re: [PATCH 3/9] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

[David Laight]

On Thu, 11 Dec 2025 13:50:28 +0100
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> On Wednesday, 10 December 2025 21:59:15 Central European Standard Time David Laight wrote:
> > On Wed, 10 Dec 2025 20:18:30 +0100
> > Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> >   
> > > On Tuesday, 9 December 2025 11:03:07 Central European Standard Time david.laight.linux@gmail.com wrote:  
> > > > From: David Laight <david.laight.linux@gmail.com>
> > > > 
> > > > Instead of directly expanding __BF_FIELD_CHECK() (which really ought
> > > > not be used outside bitfield) and open-coding the generation of the
> > > > masked value, just call FIELD_PREP() and add an extra check for
> > > > the mask being at most 16 bits.
> > > > 
> > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > ---
> > > >  include/linux/hw_bitfield.h | 17 ++++++++---------
> > > >  1 file changed, 8 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
> > > > index df202e167ce4..d7f21b60449b 100644
> > > > --- a/include/linux/hw_bitfield.h
> > > > +++ b/include/linux/hw_bitfield.h
> > > > @@ -23,15 +23,14 @@
> > > >   * register, a bit in the lower half is only updated if the corresponding bit
> > > >   * in the upper half is high.
> > > >   */
> > > > -#define FIELD_PREP_WM16(_mask, _val)					     \
> > > > -	({								     \
> > > > -		typeof(_val) __val = _val;				     \
> > > > -		typeof(_mask) __mask = _mask;				     \
> > > > -		__BF_FIELD_CHECK(__mask, ((u16)0U), __val,		     \
> > > > -				 "HWORD_UPDATE: ");			     \
> > > > -		(((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) | \
> > > > -		((__mask) << 16);					     \
> > > > -	})
> > > > +#define FIELD_PREP_WM16(mask, val)				\
> > > > +({								\
> > > > +	__auto_type _mask = mask;				\
> > > > +	u32 _val = FIELD_PREP(_mask, val);			\
> > > > +	BUILD_BUG_ON_MSG(_mask > 0xffffu,			\
> > > > +			 "FIELD_PREP_WM16: mask too large");	\
> > > > +	_val | (_mask << 16);					\
> > > > +})
> > > >  
> > > >  /**
> > > >   * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
> > > >     
> > > 
> > > This breaks the build for at least one driver that uses
> > > FIELD_PREP_WM16, namely phy-rockchip-emmc.c:  
> > 
> > Not in my allmodconfig build.
> > ...   
> > > pcie-dw-rockchip.c is similarly broken by this change, except
> > > without the superfluous wrapper:  
> > 
> > That one did get built.  
> 
> I build with clang 21.1.6 for arm64, in case that's any help.
> I don't see how pcie-dw-rockchip.c built for you if FIELD_PREP
> and FIELD_PREP_WM16 have conflicting symbol names?

I would expect gcc to have found that 'silly' error.
I'll generate a v2 soon.

	David

> 
> > 
> > The problem is that FIELD_PREP_WM16() needs to use different 'local'
> > variables than FIELD_PREP().
> > The 'proper' fix is to use unique names (as min() and max() do), but that
> > makes the whole thing unreadable and is best avoided unless nesting is
> > likely.
> > In this case s/mask/wm16_mask/ and s/val/wm16_val/ might be best.
> > 
> > 	David
> > 
> >   
> 
> 
> 
> 
>