* Re: [PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant
2026-04-10 16:55 ` Yury Norov
@ 2026-04-10 18:45 ` David Laight
0 siblings, 0 replies; 3+ messages in thread
From: David Laight @ 2026-04-10 18:45 UTC (permalink / raw)
To: Yury Norov
Cc: Geert Uytterhoeven, Alexandre Belloni, Jonathan Cameron, Crt Mori,
Nuno Sá, Richard Genoud, Andy Shevchenko, Yury Norov,
Rasmus Villemoes, Matt Coster, open list
On Fri, 10 Apr 2026 12:55:25 -0400
Yury Norov <ynorov@nvidia.com> wrote:
> + Matt Coster
>
> On Fri, Apr 10, 2026 at 10:09:27AM +0100, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> >
> > Some versions of gcc report that some expressions involving
> > __builtin_ffsll(1ull << 63) are not integer constant expressions.
> >
> > Rework FIELD_PREP_CONST() to avoid the issue.
> >
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
>
> Thanks, David.
>
> So, the original patch that spotted the problem is:
>
> https://lore.kernel.org/all/20260409-field-prep-fix-v1-1-f0e9ae64f63c@imgtec.com/
That isn't the patch that spotted the problem.
That is a suggested fix.
I've not seen the code (or patch) that actually exhibited the issue.
I think someone changed (drivers/gpu/drm/imagination/pvr_device.h):
#define PVR_PACKED_BVNC(b, v, n, c) \
((((u64)(b) & GENMASK_ULL(15, 0)) << 48) | \
(((u64)(v) & GENMASK_ULL(15, 0)) << 32) | \
(((u64)(n) & GENMASK_ULL(15, 0)) << 16) | \
(((u64)(c) & GENMASK_ULL(15, 0)) << 0))
to use FIELD_PREP_CONST().
The top line might be FIELD_PREP_CONST(GENMASK_ULL(63, 48), b))
which has the MSB set on the call to __builtin_ffsll() which
seems to be necessary (but not sufficient) to fail.
The expansion of that is used (pvr_device.c) in:
static enum pvr_gpu_support_level
pvr_gpu_support_level(const struct pvr_gpu_id *gpu_id)
{
switch (pvr_gpu_id_to_packed_bvnc(gpu_id)) {
case PVR_PACKED_BVNC(33, 15, 11, 3):
case PVR_PACKED_BVNC(36, 53, 104, 796):
return PVR_GPU_SUPPORTED;
case PVR_PACKED_BVNC(36, 52, 104, 182):
return PVR_GPU_EXPERIMENTAL;
default:
return PVR_GPU_UNKNOWN;
}
}
>
> Let's keep Matt in the loop, at least.
I thought I'd typed his address in - somewhere it got lost.
> Please keep the details from the original patch. "Some versions of
> some expressions" style is too uncertain.
But that is pretty much what it is.
I could give a failing test case, but it all reminds me of a bug in ash where:
x=$(command args)
could lose the last byte of output.
Fiddling with the shell script would fix it (never mind changing the
compiler options for the shell itself).
(Basically is was scanning the on-stack temporary buffer when removing
trailing '\n' and could access buffer[-1], if that happened to be 0xa
then the length would be decremented. Only likely to happen on BE.)
In this case minor tweaks to the expression change the behaviour.
This one fails:
int t : (__builtin_ffsll((1ull << 63)) < 3u) ? 1 : 2;
change the 3u to 3 or the 63 to 62 and it is ok.
Until someone supporting gcc actually identifies the underlying bug
there really isn't much to say.
>
> > ---
> >
> > Note that when 'val' is a variable 'val << constant' is likely
> > to execute faster than 'val * (1 << constant)'.
> > So the normal FIELD_PREP() is best left alone.
>
> Do you have any numbers? I'd prefer to have the codebase consistent
> when possible.
I think the multiply instruction will have a higher latency than the shift.
So you are talking about a very small number of clocks if the expression
is in the critical register dependency path.
However FIELD_GET() would need to use a divide - and that would be a lot
worse.
Having written that, ISTR that 'mask' is required to be a constant.
So the compiler may use a shift anyway - if the divide is unsigned.
But for non-constant mask you definitely want a 'shift right'.
While you might think that it only makes sense to use unsigned values,
I've found one piece of code (IIRC in the x86 fault handler) that
passes a signed value to FIELD_GET() and needs the result sign extended.
So, unless that is changed, FIELD_GET() must use an explicit right shift.
(Of course, right shift of negative values is probably UB...)
David
>
> Thanks,
> Yury
>
> > include/linux/bitfield.h | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 54aeeef1f0ec..f21765851d5c 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -45,6 +45,7 @@
> > */
> >
> > #define __bf_shf(x) (__builtin_ffsll(x) - 1)
> > +#define __bf_low_bit(mask) ((mask) & (~(mask) + 1))
> >
> > #define __scalar_type_to_unsigned_cases(type) \
> > unsigned type: (unsigned type)0, \
> > @@ -138,8 +139,6 @@
> > __FIELD_PREP(_mask, _val, "FIELD_PREP: "); \
> > })
> >
> > -#define __BF_CHECK_POW2(n) BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0)
> > -
> > /**
> > * FIELD_PREP_CONST() - prepare a constant bitfield element
> > * @_mask: shifted mask defining the field's length and position
> > @@ -157,11 +156,11 @@
> > /* mask must be non-zero */ \
> > BUILD_BUG_ON_ZERO((_mask) == 0) + \
> > /* check if value fits */ \
> > - BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
> > + BUILD_BUG_ON_ZERO(~((_mask) / __bf_low_bit(_mask)) & (_val)) + \
> > /* check if mask is contiguous */ \
> > - __BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) + \
> > + BUILD_BUG_ON_ZERO((_mask) & ((_mask) + __bf_low_bit(_mask))) + \
> > /* and create the value */ \
> > - (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)) \
> > + (((_val) * __bf_low_bit(_mask)) & (_mask)) \
> > )
> >
> > /**
> > --
> > 2.39.5
^ permalink raw reply [flat|nested] 3+ messages in thread