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

Peter Zijlstra
2026-04-20T11:20:10Z

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@nvidia.com>
> ---
> 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));

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.