[PATCH v2 08/16] bitfield: Simplify __BF_FIELD_CHECK_REG()

david.laight.linux@gmail.com posted 16 patches 1 month, 3 weeks ago
[PATCH v2 08/16] bitfield: Simplify __BF_FIELD_CHECK_REG()
Posted by david.laight.linux@gmail.com 1 month, 3 weeks ago
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'.

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

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index c354ca2ef1a0..7160b762c979 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -45,22 +45,6 @@
 
 #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 __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)			\
 	({								\
 		BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),		\
@@ -75,8 +59,8 @@
 	})
 
 #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)				\
-- 
2.39.5
Re: [PATCH v2 08/16] bitfield: Simplify __BF_FIELD_CHECK_REG()
Posted by Jonathan Cameron 1 month, 3 weeks ago
On Fri, 12 Dec 2025 19:37:13 +0000
david.laight.linux@gmail.com wrote:

> 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'.
> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>
Hi David,

Just one really trivial comment inline. Feel free to ignore.

Jonathan

> ---
> @@ -75,8 +59,8 @@
>  	})
>  
>  #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)),		\

Trivial.  sizeof(reg) is much more comment syntax in kernel code.

>  			 pfx "type of reg too small for mask")
>  
>  #define __BF_FIELD_CHECK(mask, reg, val, pfx)				\
Re: [PATCH v2 08/16] bitfield: Simplify __BF_FIELD_CHECK_REG()
Posted by David Laight 1 month, 3 weeks ago
On Wed, 17 Dec 2025 10:26:18 +0000
Jonathan Cameron <jonathan.cameron@huawei.com> wrote:

> On Fri, 12 Dec 2025 19:37:13 +0000
> david.laight.linux@gmail.com wrote:
> 
> > 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'.
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>  
> Hi David,
> 
> Just one really trivial comment inline. Feel free to ignore.
> 
> Jonathan
> 
> > ---
> > @@ -75,8 +59,8 @@
> >  	})
> >  
> >  #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)),		\  
> 
> Trivial.  sizeof(reg) is much more comment syntax in kernel code.
                                     (common)

Hmm. sizeof is an operator not a function.
Its argument is either a variable/expression or a bracketed type
(I don't usually put variables in brackets).
So 'sizeof(reg)' is nearly as bad as 'return(reg)'.

	David

> 
> >  			 pfx "type of reg too small for mask")
> >  
> >  #define __BF_FIELD_CHECK(mask, reg, val, pfx)				\  
>
Re: [PATCH v2 08/16] bitfield: Simplify __BF_FIELD_CHECK_REG()
Posted by Andy Shevchenko 1 month, 1 week ago
On Wed, Dec 17, 2025 at 10:31:55PM +0000, David Laight wrote:
> On Wed, 17 Dec 2025 10:26:18 +0000
> Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> > On Fri, 12 Dec 2025 19:37:13 +0000
> > david.laight.linux@gmail.com wrote:

...

> > > +	BUILD_BUG_ON_MSG((mask) + 0U + 0UL + 0ULL >			\
> > > +			 ~0ULL >> (64 - 8 * sizeof (reg)),		\  
> > 
> > Trivial.  sizeof(reg) is much more comment syntax in kernel code.
>                                      (common)
> 
> Hmm. sizeof is an operator not a function.
> Its argument is either a variable/expression or a bracketed type
> (I don't usually put variables in brackets).
> So 'sizeof(reg)' is nearly as bad as 'return(reg)'.

Yet, it's a style used de facto in the Linux kernel. I am with Jonathan on this.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 08/16] bitfield: Simplify __BF_FIELD_CHECK_REG()
Posted by David Laight 1 month, 1 week ago
On Sun, 28 Dec 2025 20:53:45 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Dec 17, 2025 at 10:31:55PM +0000, David Laight wrote:
> > On Wed, 17 Dec 2025 10:26:18 +0000
> > Jonathan Cameron <jonathan.cameron@huawei.com> wrote:  
> > > On Fri, 12 Dec 2025 19:37:13 +0000
> > > david.laight.linux@gmail.com wrote:  
> 
> ...
> 
> > > > +	BUILD_BUG_ON_MSG((mask) + 0U + 0UL + 0ULL >			\
> > > > +			 ~0ULL >> (64 - 8 * sizeof (reg)),		\    
> > > 
> > > Trivial.  sizeof(reg) is much more comment syntax in kernel code.  
> >                                      (common)
> > 
> > Hmm. sizeof is an operator not a function.
> > Its argument is either a variable/expression or a bracketed type
> > (I don't usually put variables in brackets).
> > So 'sizeof(reg)' is nearly as bad as 'return(reg)'.  
> 
> Yet, it's a style used de facto in the Linux kernel. I am with Jonathan on this.

Not hard to change :-)

There is a more interesting question as to whether that check is
needed at all?
It is only really the sanity checks (and the fact that __builtin_ffs() is
sub-optimal) that stop the functions being 'reasonable' for non-constant
values.

	David
Re: [PATCH v2 08/16] bitfield: Simplify __BF_FIELD_CHECK_REG()
Posted by Yury Norov 1 month, 3 weeks ago
On Wed, Dec 17, 2025 at 10:31:55PM +0000, David Laight wrote:
> On Wed, 17 Dec 2025 10:26:18 +0000
> Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> 
> > On Fri, 12 Dec 2025 19:37:13 +0000
> > david.laight.linux@gmail.com wrote:
> > 
> > > From: David Laight <david.laight.linux@gmail.com>

...

> > > ---
> > > @@ -75,8 +59,8 @@
> > >  	})
> > >  
> > >  #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)),		\  
> > 
> > Trivial.  sizeof(reg) is much more comment syntax in kernel code.
>                                      (common)
> 
> Hmm. sizeof is an operator not a function.
> Its argument is either a variable/expression or a bracketed type
> (I don't usually put variables in brackets).
> So 'sizeof(reg)' is nearly as bad as 'return(reg)'.

Please re-read Documentation/process/coding-style.rst:

3.1) Spaces
***********

Linux kernel style for use of spaces depends (mostly) on
function-versus-keyword usage.  Use a space after (most) keywords.  The
notable exceptions are sizeof, typeof, alignof, and __attribute__, which look
somewhat like functions (and are usually used with parentheses in Linux,
although they are not required in the language, as in: ``sizeof info`` after
``struct fileinfo info;`` is declared).

So use a space after these keywords::

        if, switch, case, for, do, while

but not with sizeof, typeof, alignof, or __attribute__.  E.g.,

.. code-block:: c


        s = sizeof(struct file);

Do not add spaces around (inside) parenthesized expressions.  This example is
**bad**:

.. code-block:: c


        s = sizeof( struct file );