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

[David Laight]

On Thu, 2 Apr 2026 01:00:20 -0300
Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx> 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?
> 
> > > +     ({                                                              \
> > > +             __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?

All of the calls with the 0ULL placeholder (especially for the register)
should really be removed.
Last time I looked there where some calls that only had placeholders.
They just bloat the pre-processor output and slow down compilation.

> 
> 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)
> 
> #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)                                             \
> +       ({                                                              \

You ought to have:
		auto _fw_mask = mask;
here. While _mask has to be a constant, if it comes from GENMASK()
it is very long.

> +               __BF_FIELD_CHECK_MASK(_mask, 0ULL, "FIELD_WIDTH: ");   \
> +               (typeof(_mask))__bf_hweight(_mask);                     \

Why the cast of the result?
They are everywhere in that file, and many are pointless.
But there is no point adding another one.

I'm not even sure you need the extra define.
Just use __builtin_popcountll().

	David

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