Re: [net-next PATCH 04/10] bitfield.h: add FIELD_WIDTH()

[Yury Norov]

On Thu, Apr 02, 2026 at 01:00:20AM -0300, Luiz Angelo Daros de Luca wrote:
> > > +/**
> > > + * FIELD_WIDTH() - return the width of a bitfield
> > > + * @_mask: shifted mask defining the field's length and position
> > > + *
> > > + * Returns the number of contiguous bits covered by @_mask.
> > > + * This corresponds to the bit width of FIELD_MAX(@_mask).
> > > + */
> > > +#define FIELD_WIDTH(_mask)                                           \
> >
> > Please no underscored names unless necessary.
> 
> I used _mask to maintain consistency with the existing public macros
> in this file, such as FIELD_GET, FIELD_PREP, FIELD_MAX, and FIELD_FIT.
> All of them use the underscore prefix for parameters. Should I diverge
> from them?

There's no consistency, but if you look at the recently added code,
you'll find it doesn't add those underscored prefixes. If you want
them, it's OK. Just explain what the hell do they mean?
 
> > > +     ({                                                              \
> > > +             __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_WIDTH: ");   \
> > > +             __bf_shf(~FIELD_MAX(_mask));                            \
> > > +     })
> >
> > I believe, this should be:
> >
> >   #define FIELD_WIDTH(mask) ({                                  \
> >         __BF_FIELD_CHECK_MASK(mask, 0ULL, "FIELD_WIDTH: ");     \
> >         HWEIGHT(mask);                                          \
> >   })
> 
> HWEIGHT() is indeed much cleaner. However, to keep bitfield.h
> self-contained and avoid adding more includes, I'll try
> __builtin_popcountll() in a similar way __builtin_ffsll is already
> used.
> 
> I also noticed the suggestion to use __BF_FIELD_CHECK_MASK instead of
> __BF_FIELD_CHECK. Both FIELD_MAX and FIELD_FIT currently use the full
> __BF_FIELD_CHECK with 0ULL as a dummy register to ensure the mask fits
> within the header's supported limits. If __BF_FIELD_CHECK_MASK is
> preferred for FIELD_WIDTH, I can certainly use it, but should we also
> update FIELD_MAX and FIELD_FIT for consistency?
> 
> I intend to send a v2 with the following implementation:
> 
> #define __bf_shf(x) (__builtin_ffsll(x) - 1)
> +#define __bf_hweight(x) __builtin_popcountll(x)

So, we've got only 2 __bf_xxx() functions here:

 #define __bf_shf(x) (__builtin_ffsll(x) - 1)
 #define __bf_cast_unsigned(type, x)     ((__unsigned_scalar_typeof(type))(x))

They both do something with the builtins. Your __bf_hweight() is a
pure redefinition. What for do you need it? Why not just use that
compiler-provided popcount() that everybody knows?


> #define __scalar_type_to_unsigned_cases(type)                          \
>                unsigned type:  (unsigned type)0,                       \
> @@ -111,6 +112,19 @@
>                (typeof(_mask))((_mask) >> __bf_shf(_mask));            \
>        })
> 
> +/**
> + * FIELD_WIDTH() - return the width of a bitfield
> + * @_mask: shifted mask defining the field's length and position
> + *
> + * Returns the number of contiguous bits covered by @_mask.
> + * This corresponds to the bit width of FIELD_MAX(@_mask).
> + */
> +#define FIELD_WIDTH(_mask)                                             \
> +       ({                                                              \
> +               __BF_FIELD_CHECK_MASK(_mask, 0ULL, "FIELD_WIDTH: ");   \
> +               (typeof(_mask))__bf_hweight(_mask);                     \

No need to typecast the result.

> +       })
> +
> 
> Regards,
> 
> Luiz