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

[Yury Norov <yury.norov@gmail.com>]

On Sun, Feb 02, 2025 at 05:26:04PM +0900, Vincent Mailhol wrote:
> 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?

I guess, for historical reasons?

> Instead of creating another variant for
> non-constant bitfields, wouldn't it be better to make the existing macro
> accept both?

Yes, it would definitely be better IMO.

> 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) >>

This should be a const_true(), because statically_true() may be OK
with something like:
        ((runtime_var << 1) & 1 == 0)

I think it's your own patch that adds const_true(): 4f3d1be4c2f8a :)

Thanks,
Yury