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

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

[Johannes Berg]

On Fri, 2026-04-24 at 12:35 -0400, Yury Norov wrote:
> > I (personally) tend to prefer the "__MAKE_OP" versions (*_get_bits()
> > etc.), in particular because WiFi and firmware interfaces deal a lot
> > with fixed endian fields.
> 
> I don't like that __MAKE_OP magic because whatever it generates is not
> greppable. And because we disable strict type checks for kernel, but
> this API claims to typecheck the parameters for the user. So, the
> following compiles well:
> 
>         u64 val = 0;
>         ret = le16_get_bits(val, GENMASK(15, 10));
> 
> I don't like autogeneration in general. We generate, for example,
> be32_get_bits(), but never use it.

That's a lot of "I don't like", but whatever.


> We don't even know the level of the bloat.

These are static inlines so there's no binary cost, and given that
you're complaining about them being generated you can't really *also*
complain about too much code...
 
> > Any chance it'd be simple to generate u32_get_bits_signed() etc.? Could
> > be especially useful for le32_get_bits_signed() for example, to have the
> > endian conversion built-in unlike FIELD_GET_SIGNED().
> 
> Maybe this:
> 
>         FIELD_GET_SIGNED(mask, le32_to_cpu(reg))

Awful. "I don't like". But we rarely deal with bit-packed signed values
anyway.

johannes

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

[David Laight]

On Mon, 27 Apr 2026 10:29:21 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> On Fri, 2026-04-24 at 12:35 -0400, Yury Norov wrote:
> > > I (personally) tend to prefer the "__MAKE_OP" versions (*_get_bits()
> > > etc.), in particular because WiFi and firmware interfaces deal a lot
> > > with fixed endian fields.  
> > 
> > I don't like that __MAKE_OP magic because whatever it generates is not
> > greppable. And because we disable strict type checks for kernel, but
> > this API claims to typecheck the parameters for the user. So, the
> > following compiles well:
> > 
> >         u64 val = 0;
> >         ret = le16_get_bits(val, GENMASK(15, 10));
> > 
> > I don't like autogeneration in general. We generate, for example,
> > be32_get_bits(), but never use it.  
> 
> That's a lot of "I don't like", but whatever.
> 
> 
> > We don't even know the level of the bloat.  
> 
> These are static inlines so there's no binary cost, and given that
> you're complaining about them being generated you can't really *also*
> complain about too much code...

There is a measurable compile-time cost for processing the definitions
of static inlines even when they aren't used.
While processing the definitions of #defines is cheap, processing the
expansions can be measurable. Particularly when expansions get nested
as often happens when GENMASK() is used as an argument to FIELD_xxx().

Even trivial #defines can affect compile times.
The 'size' check in READ_ONCE() (etc) costs about 1%.
(1% may not sound much, but find 10 of them and it becomes significant.)
I suspect a lot of that is adding the extra external function to hold
the error message.

	David

>  
> > > Any chance it'd be simple to generate u32_get_bits_signed() etc.? Could
> > > be especially useful for le32_get_bits_signed() for example, to have the
> > > endian conversion built-in unlike FIELD_GET_SIGNED().  
> > 
> > Maybe this:
> > 
> >         FIELD_GET_SIGNED(mask, le32_to_cpu(reg))  
> 
> Awful. "I don't like". But we rarely deal with bit-packed signed values
> anyway.
> 
> johannes
>