Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()

[David Laight]

On Mon, 20 Apr 2026 13:54:57 -0400
Yury Norov <ynorov@xxxxxxxxxx> wrote:

> On Mon, Apr 20, 2026 at 01:19:40PM +0200, Peter Zijlstra wrote:
> > On Fri, Apr 17, 2026 at 01:36:12PM -0400, Yury Norov wrote:  
> > > The bitfields are designed in assumption that fields contain unsigned
> > > integer values, thus extracting the values from the field implies
> > > zero-extending.
> > > 
> > > Some drivers need to sign-extend their fields, and currently do it like:
> > > 
> > > 	dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > > 	dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
> > > 
> > > It's error-prone because it relies on user to provide the correct
> > > index of the most significant bit and proper 32 vs 64 function flavor.
> > > 
> > > Thus, introduce a FIELD_GET_SIGNED() macro, which is the more
> > > convenient and compiles (on x86_64) to just a couple instructions:
> > > shl and sar.
> > > 
> > > Signed-off-by: Yury Norov <ynorov@xxxxxxxxxx>
> > > ---
> > >  include/linux/bitfield.h | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > > index 54aeeef1f0ec..35ef63972810 100644
> > > --- a/include/linux/bitfield.h
> > > +++ b/include/linux/bitfield.h
> > > @@ -178,6 +178,22 @@
> > >  		__FIELD_GET(_mask, _reg, "FIELD_GET: ");		\
> > >  	})
> > >  
> > > +/**
> > > + * FIELD_GET_SIGNED() - extract a signed bitfield element
> > > + * @mask: shifted mask defining the field's length and position
> > > + * @reg:  value of entire bitfield
> > > + *
> > > + * Returns the sign-extended field specified by @_mask from the
> > > + * bitfield passed in as @_reg by masking and shifting it down.
> > > + */
> > > +#define FIELD_GET_SIGNED(mask, reg)					\
> > > +	({								\
> > > +		__BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: ");	\
> > > +		 ((__signed_scalar_typeof(mask))((long long)(reg) <<	\
> > > +		 __builtin_clzll(mask) >> (__builtin_clzll(mask) +	\
> > > +						__builtin_ctzll(mask))));\
> > > +	})  
> > 
> > IIRC clz is count-leading-zeros and ctz is count-trailing-zeros. Most of
> > the other FIELD things use __bf_shf() which is defined in terms of ffs -
> > 1 (which is another way of writing ctz).
> > 
> > So how about you start by redefining __bf_shf() in ctz, and then add
> > another helper for the clz and write the thing something like:
> > 
> > 	((long long)(reg) << __bf_clz(mask)) >> (__bf_clz(mask) + __bf_shf(mask));  
> 
> So...
> 
> I like the shorter form, but whatever we add in the bitfield.h - we'll
> have to support it.
> 
> For example, __bf_shf() wasn't intended to be used outsize of the
> header, thus double underscored. But there's over 100 external users
> now. And to make it worse, it's broken for GCC 14 and earlier:

For anyone who hasn't followed the gory details it isn't 'very broken'.
Basically __builtin_ffsll() doesn't always generate an 'integer constant
expression' from constant input so you can get a compile fail.

> https://lore.kernel.org/all/20260409-field-prep-fix-v1-1-f0e9ae64f63c@xxxxxxxxxx/
> 
> So needs to get fixed.
> 
> The bitfield.h has two __bf macros: __bf_shf() and __bf_cast_unsigned().
> They are thin wrappers,

__bf_cast_unsigned() isn't exactly thin.

	David

> but after all do something with the corresponding
> builtins output. The __bf_cls() would be a pure renaming. I'm OK with
> that, but some people don't:
> 
> https://lore.kernel.org/all/20260303182845.250bb2de@xxxxxxxxxx/
> 
> That's why I didn't make FIELD_GET_SIGNED() implementation looking nicer.
> If you strongly prefer the shorter version, I can do that in v2.
>  
> > Also, since the order of the shifts is rather important, I think it
> > makes sense to add this extra pair of (), even when not strictly needed,
> > just to make it easier to read.  
> 
> Sure, will do.
>