[v2] bitfield: tidy up bitfield.h

[PATCH v2 03/16] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

Posted by david.laight.linux@gmail.com 1 month, 3 weeks ago

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.
The extra check is added after calling FIELD_PREP() to get a sane
error message if 'mask' isn't constant.

Remove the leading _ from the formal parameter names.
Prefix the local variables with _wm16_ to hopefully make them
unique.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---

Changes for v2:
- Update kerneldoc to match changed formal parameter names.
- Change local variables to not collide with those in FIELD_PREP().

Most of the examples are constants and get optimised away.

 include/linux/hw_bitfield.h | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
index df202e167ce4..0bd1040a5f93 100644
--- a/include/linux/hw_bitfield.h
+++ b/include/linux/hw_bitfield.h
@@ -12,8 +12,8 @@
 
 /**
  * FIELD_PREP_WM16() - prepare a bitfield element with a mask in the upper half
- * @_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_WM16() masks and shifts up the value, as well as bitwise ORs the
  * result with the mask shifted up by 16.
@@ -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 _wm16_mask = mask;				\
+	u32 _wm16_val = FIELD_PREP(_wm16_mask, val);		\
+	BUILD_BUG_ON_MSG(_wm16_mask > 0xffffu,			\
+			 "FIELD_PREP_WM16: mask too large");	\
+	_wm16_val | (_wm16_mask << 16);				\
+})
 
 /**
  * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
-- 
2.39.5

Re: [PATCH v2 03/16] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

Posted by Geert Uytterhoeven 2 weeks, 3 days ago

On Fri, 12 Dec 2025 at 20:38, <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.
> The extra check is added after calling FIELD_PREP() to get a sane
> error message if 'mask' isn't constant.
>
> Remove the leading _ from the formal parameter names.
> Prefix the local variables with _wm16_ to hopefully make them
> unique.
>
> Signed-off-by: David Laight <david.laight.linux@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 03/16] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

Posted by Geert Uytterhoeven 2 weeks, 3 days ago

On Wed, 21 Jan 2026 at 17:50, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, 12 Dec 2025 at 20:38, <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.
> > The extra check is added after calling FIELD_PREP() to get a sane
> > error message if 'mask' isn't constant.
> >
> > Remove the leading _ from the formal parameter names.
> > Prefix the local variables with _wm16_ to hopefully make them
> > unique.
> >
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Oops, just noticed  commit 2fb6915fa22dc552
("compiler_types.h: add "auto" as a macro for "__auto_type""),
so you want to do s/__auto_type/auto/g.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 03/16] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

Posted by David Laight 2 weeks, 2 days ago

On Wed, 21 Jan 2026 17:52:29 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Wed, 21 Jan 2026 at 17:50, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, 12 Dec 2025 at 20:38, <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.
> > > The extra check is added after calling FIELD_PREP() to get a sane
> > > error message if 'mask' isn't constant.
> > >
> > > Remove the leading _ from the formal parameter names.
> > > Prefix the local variables with _wm16_ to hopefully make them
> > > unique.
> > >
> > > Signed-off-by: David Laight <david.laight.linux@gmail.com>  
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>  
> 
> Oops, just noticed  commit 2fb6915fa22dc552
> ("compiler_types.h: add "auto" as a macro for "__auto_type""),
> so you want to do s/__auto_type/auto/g.

That wasn't there when I was writing the series.
I knew it was coming but didn't want build breakages.

	David

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>

Re: [PATCH v2 03/16] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

Posted by Nicolas Frattaroli 1 month, 3 weeks ago

On Friday, 12 December 2025 20:37:08 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.
> The extra check is added after calling FIELD_PREP() to get a sane
> error message if 'mask' isn't constant.
> 
> Remove the leading _ from the formal parameter names.
> Prefix the local variables with _wm16_ to hopefully make them
> unique.
> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>
> ---
> 
> Changes for v2:
> - Update kerneldoc to match changed formal parameter names.
> - Change local variables to not collide with those in FIELD_PREP().
> 
> Most of the examples are constants and get optimised away.
> 
>  include/linux/hw_bitfield.h | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
> index df202e167ce4..0bd1040a5f93 100644
> --- a/include/linux/hw_bitfield.h
> +++ b/include/linux/hw_bitfield.h
> @@ -12,8 +12,8 @@
>  
>  /**
>   * FIELD_PREP_WM16() - prepare a bitfield element with a mask in the upper half
> - * @_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_WM16() masks and shifts up the value, as well as bitwise ORs the
>   * result with the mask shifted up by 16.
> @@ -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 _wm16_mask = mask;				\
> +	u32 _wm16_val = FIELD_PREP(_wm16_mask, val);		\
> +	BUILD_BUG_ON_MSG(_wm16_mask > 0xffffu,			\
> +			 "FIELD_PREP_WM16: mask too large");	\
> +	_wm16_val | (_wm16_mask << 16);				\
> +})
>  
>  /**
>   * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
> 

Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Compiled it with my usual config and booted it on a board that uses
drivers that make use of these macros, and checked that things are
working.

Thank you!

Kind regards,
Nicolas Frattaroli

Re: [PATCH v2 03/16] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

Posted by Yury Norov 1 month, 3 weeks ago

On Wed, Dec 17, 2025 at 02:22:32PM +0100, Nicolas Frattaroli wrote:
> On Friday, 12 December 2025 20:37:08 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.
> > The extra check is added after calling FIELD_PREP() to get a sane
> > error message if 'mask' isn't constant.
> > 
> > Remove the leading _ from the formal parameter names.
> > Prefix the local variables with _wm16_ to hopefully make them
> > unique.
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> > 
> > Changes for v2:
> > - Update kerneldoc to match changed formal parameter names.
> > - Change local variables to not collide with those in FIELD_PREP().
> > 
> > Most of the examples are constants and get optimised away.
> > 
> >  include/linux/hw_bitfield.h | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
> > index df202e167ce4..0bd1040a5f93 100644
> > --- a/include/linux/hw_bitfield.h
> > +++ b/include/linux/hw_bitfield.h
> > @@ -12,8 +12,8 @@
> >  
> >  /**
> >   * FIELD_PREP_WM16() - prepare a bitfield element with a mask in the upper half
> > - * @_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_WM16() masks and shifts up the value, as well as bitwise ORs the
> >   * result with the mask shifted up by 16.
> > @@ -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 _wm16_mask = mask;				\
> > +	u32 _wm16_val = FIELD_PREP(_wm16_mask, val);		\
> > +	BUILD_BUG_ON_MSG(_wm16_mask > 0xffffu,			\
> > +			 "FIELD_PREP_WM16: mask too large");	\
> > +	_wm16_val | (_wm16_mask << 16);				\
> > +})
> >  
> >  /**
> >   * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
> > 
> 
> Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 
> Compiled it with my usual config and booted it on a board that uses
> drivers that make use of these macros, and checked that things are
> working.

Nicolas, thanks for testing! Would you also want to add an explicit
ack or review tag?

David, I'm OK with this change. Please add bloat-o-meter and code
generation examples, and minimize the diff as I asked in v1, before I
can merge it.

Thanks,
Yury

Re: [PATCH v2 03/16] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

Posted by David Laight 1 month, 3 weeks ago

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

> On Wed, Dec 17, 2025 at 02:22:32PM +0100, Nicolas Frattaroli wrote:
> > On Friday, 12 December 2025 20:37:08 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.
> > > The extra check is added after calling FIELD_PREP() to get a sane
> > > error message if 'mask' isn't constant.
> > > 
> > > Remove the leading _ from the formal parameter names.
> > > Prefix the local variables with _wm16_ to hopefully make them
> > > unique.
> > > 
> > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > ---
> > > 
> > > Changes for v2:
> > > - Update kerneldoc to match changed formal parameter names.
> > > - Change local variables to not collide with those in FIELD_PREP().
> > > 
> > > Most of the examples are constants and get optimised away.
> > > 
> > >  include/linux/hw_bitfield.h | 21 ++++++++++-----------
> > >  1 file changed, 10 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
> > > index df202e167ce4..0bd1040a5f93 100644
> > > --- a/include/linux/hw_bitfield.h
> > > +++ b/include/linux/hw_bitfield.h
> > > @@ -12,8 +12,8 @@
> > >  
> > >  /**
> > >   * FIELD_PREP_WM16() - prepare a bitfield element with a mask in the upper half
> > > - * @_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_WM16() masks and shifts up the value, as well as bitwise ORs the
> > >   * result with the mask shifted up by 16.
> > > @@ -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 _wm16_mask = mask;				\
> > > +	u32 _wm16_val = FIELD_PREP(_wm16_mask, val);		\
> > > +	BUILD_BUG_ON_MSG(_wm16_mask > 0xffffu,			\
> > > +			 "FIELD_PREP_WM16: mask too large");	\
> > > +	_wm16_val | (_wm16_mask << 16);				\
> > > +})
> > >  
> > >  /**
> > >   * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
> > >   
> > 
> > Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > 
> > Compiled it with my usual config and booted it on a board that uses
> > drivers that make use of these macros, and checked that things are
> > working.  
> 
> Nicolas, thanks for testing! Would you also want to add an explicit
> ack or review tag?
> 
> David, I'm OK with this change. Please add bloat-o-meter and code
> generation examples, and minimize the diff as I asked in v1, before I
> can merge it.

That is pretty much the minimal diff.
By the time you've changed the three lines that do anything 'real'
and realigned the continuation markers all the lines have changed.

I did look at the generated code, but since 'mask' is a constant
the whole thing is just ((val << const_a) | const_b) pretty much
regardless of the actual source - most of which is compile-time checks.
There isn't anything for the bloat-o-meter to find.
	
	David


> 
> Thanks,
> Yury

Re: [PATCH v2 03/16] bitmap: Use FIELD_PREP() in expansion of FIELD_PREP_WM16()

Posted by Nicolas Frattaroli 1 month, 2 weeks ago

On Thursday, 18 December 2025 01:16:10 Central European Standard Time Yury Norov wrote:
> On Wed, Dec 17, 2025 at 02:22:32PM +0100, Nicolas Frattaroli wrote:
> > On Friday, 12 December 2025 20:37:08 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.
> > > The extra check is added after calling FIELD_PREP() to get a sane
> > > error message if 'mask' isn't constant.
> > > 
> > > Remove the leading _ from the formal parameter names.
> > > Prefix the local variables with _wm16_ to hopefully make them
> > > unique.
> > > 
> > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > ---
> > > 
> > > Changes for v2:
> > > - Update kerneldoc to match changed formal parameter names.
> > > - Change local variables to not collide with those in FIELD_PREP().
> > > 
> > > Most of the examples are constants and get optimised away.
> > > 
> > >  include/linux/hw_bitfield.h | 21 ++++++++++-----------
> > >  1 file changed, 10 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/include/linux/hw_bitfield.h b/include/linux/hw_bitfield.h
> > > index df202e167ce4..0bd1040a5f93 100644
> > > --- a/include/linux/hw_bitfield.h
> > > +++ b/include/linux/hw_bitfield.h
> > > @@ -12,8 +12,8 @@
> > >  
> > >  /**
> > >   * FIELD_PREP_WM16() - prepare a bitfield element with a mask in the upper half
> > > - * @_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_WM16() masks and shifts up the value, as well as bitwise ORs the
> > >   * result with the mask shifted up by 16.
> > > @@ -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 _wm16_mask = mask;				\
> > > +	u32 _wm16_val = FIELD_PREP(_wm16_mask, val);		\
> > > +	BUILD_BUG_ON_MSG(_wm16_mask > 0xffffu,			\
> > > +			 "FIELD_PREP_WM16: mask too large");	\
> > > +	_wm16_val | (_wm16_mask << 16);				\
> > > +})
> > >  
> > >  /**
> > >   * FIELD_PREP_WM16_CONST() - prepare a constant bitfield element with a mask in
> > > 
> > 
> > Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > 
> > Compiled it with my usual config and booted it on a board that uses
> > drivers that make use of these macros, and checked that things are
> > working.
> 
> Nicolas, thanks for testing! Would you also want to add an explicit
> ack or review tag?

Sure!

Reviewed-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

(not sure if an ack is meaningful for me, since I'm technically not
the maintainer, but:)

Acked-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

I've taken some more time to compare the binary outputs with and
without the change, and they're the same. I've also looked at the
code itself, and it appears correct (and more correct than the
original in fact, since I used "HWORD_UPDATE" by accident in the
error message).

Kind regards,
Nicolas Frattaroli

> 
> David, I'm OK with this change. Please add bloat-o-meter and code
> generation examples, and minimize the diff as I asked in v1, before I
> can merge it.
> 
> Thanks,
> Yury
>