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

[Yury Norov]

On Thu, Apr 02, 2026 at 10:27:17AM +0100, David Laight wrote:
> 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.

Have you any numbers? Can you send a patch?

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

Yes, but what this _fw means? Here it could be just auto __mask = mask.
That is what the underscores are used.

> > +               __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().

For __BF_FIELD_CHECK_MASK() check, I guess.