Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Vincent Mailhol]

On 31/01/2025 at 22:46, Geert Uytterhoeven wrote:
> The existing FIELD_{GET,PREP}() macros are limited to compile-time
> constants.  However, it is very common to prepare or extract bitfield
> elements where the bitfield mask is not a compile-time constant.

Why is it that the existing FIELD_{GET,PREP}() macros must be limited to
compile time constants? Instead of creating another variant for
non-constant bitfields, wouldn't it be better to make the existing macro
accept both?

As far as I can see, only __BUILD_BUG_ON_NOT_POWER_OF_2()  and
__BF_FIELD_CHECK() need to be adjusted. I am thinking of this:

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 63928f173223..c6bedab862d1 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -8,6 +8,7 @@
 #define _LINUX_BITFIELD_H

 #include <linux/build_bug.h>
+#include <linux/compiler.h>
 #include <asm/byteorder.h>

 /*
@@ -62,15 +63,13 @@

 #define __BF_FIELD_CHECK(_mask, _reg, _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,               \
+               BUILD_BUG_ON_MSG(statically_true((_mask) == 0),         \
+                                _pfx "mask is zero");                  \
+               BUILD_BUG_ON_MSG(statically_true(~((_mask) >>
__bf_shf(_mask)) & \
+                                                (0 + (_val))),         \
                                 _pfx "value too large for the field"); \
-               BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >     \
-                                __bf_cast_unsigned(_reg, ~0ull),       \
+
BUILD_BUG_ON_MSG(statically_true(__bf_cast_unsigned(_mask, _mask) > \
+
__bf_cast_unsigned(_reg, ~0ull)), \
                                 _pfx "type of reg too small for mask"); \
                __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +                 \
                                              (1ULL << __bf_shf(_mask))); \
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index 3aa3640f8c18..3b8055ebb55f 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -18,9 +18,9 @@

 /* Force a compilation error if a constant expression is not a power of
2 */
 #define __BUILD_BUG_ON_NOT_POWER_OF_2(n)       \
-       BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
+       BUILD_BUG_ON(statically_true((n) & ((n) - 1)))
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n)                 \
-       BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
+       BUILD_BUG_ON(statically_true(!(n) || ((n) & ((n) - 1))))

 /*
  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the


> To avoid this limitation, the AT91 clock driver and several other
> drivers already have their own non-const field_{prep,get}() macros.
> Make them available for general use by consolidating them in
> <linux/bitfield.h>, and improve them slightly:
>   1. Avoid evaluating macro parameters more than once,
>   2. Replace "ffs() - 1" by "__ffs()",
>   3. Support 64-bit use on 32-bit architectures.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

(...)


Yours sincerely,
Vincent Mailhol