[PATCH 00/21] lib: add alternatives for GENMASK()

Yury Norov (NVIDIA) posted 21 patches 3 months, 2 weeks ago
Documentation/process/coding-style.rst        | 48 ++++++++++++++
arch/arc/include/asm/disasm.h                 | 62 +++++++++----------
arch/arc/kernel/disasm.c                      | 46 +++++++-------
drivers/gpu/drm/rockchip/rockchip_lvds.h      |  2 +-
drivers/gpu/drm/rockchip/rockchip_vop2_reg.c  |  4 +-
drivers/i2c/busses/i2c-nomadik.c              | 44 ++++++-------
.../platform/synopsys/hdmirx/snps_hdmirx.h    |  4 +-
drivers/mfd/db8500-prcmu-regs.h               |  2 -
drivers/mmc/host/dw_mmc-rockchip.c            |  4 +-
.../net/wireless/intel/iwlwifi/fw/api/coex.h  |  2 -
drivers/soc/rockchip/grf.c                    |  4 +-
fs/erofs/internal.h                           |  2 +-
fs/f2fs/data.c                                |  2 +-
fs/f2fs/inode.c                               |  2 +-
fs/f2fs/segment.c                             |  2 +-
fs/f2fs/super.c                               |  2 +-
fs/proc/task_mmu.c                            |  2 +-
fs/resctrl/pseudo_lock.c                      |  2 +-
fs/select.c                                   |  6 +-
include/asm-generic/fprobe.h                  |  7 +--
include/linux/bitmap.h                        | 11 ++--
include/linux/bits.h                          | 11 ++++
include/linux/f2fs_fs.h                       |  2 +-
include/linux/find.h                          | 34 +++++-----
include/linux/fortify-string.h                |  4 +-
kernel/bpf/verifier.c                         |  4 +-
kernel/kcsan/encoding.h                       |  4 +-
kernel/trace/fgraph.c                         | 10 +--
kernel/trace/trace_probe.h                    |  2 +-
lib/842/842_compress.c                        |  2 +-
lib/842/842_decompress.c                      |  2 +-
lib/bitmap.c                                  |  2 +-
lib/test_bitmap.c                             | 14 ++---
lib/zlib_inflate/inflate.c                    | 48 +++++++-------
mm/debug_vm_pgtable.c                         |  2 +-
sound/core/oss/rate.c                         | 12 ++--
sound/soc/rockchip/rockchip_i2s_tdm.h         |  2 +-
37 files changed, 235 insertions(+), 180 deletions(-)
[PATCH 00/21] lib: add alternatives for GENMASK()
Posted by Yury Norov (NVIDIA) 3 months, 2 weeks ago
GENMASK(high, low) notation reflects a common pattern to describe
hardware registers. However, out of drivers context it's confusing and
error-prone. 

This series introduces alternatives for GENMASK():

 - BITS(low, high);
 - FIRST_BITS(nbits);
 - LAST_BITS(start);

Patches 1-6 and 8 rename existing local BITS(). Patches 7 and 9 introduce
new generic macros. Patches 10-18 switch GENMASK() usage in core files to
better alternatives. Patch 19 adds Documentation section describing
preferred parameters ordering.

The series is inspired by:

https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/

Yury Norov (NVIDIA) (21):
  arc: disasm: rename BITS() for FIELD()
  iwlwifi: drop unused BITS()
  select: rename BITS() to FDS_BITS()
  ALSA: rename BITS to R_BITS
  zlib: BITS() to LOWBITS()
  mfd: prepare to generalize BITS() macro
  bits: Add BITS() macro
  mfd: drop local BITS() macro
  bits: generalize BITMAP_{FIRST,LAST}_WORD_MASK
  i2c: nomadik: don't use GENMASK()
  drivers: don't use GENMASK() in FIELD_PREP_WM16()
  bitmap: don't use GENMASK()
  trace: don't use GENMASK()
  lib: 842: don't use GENMASK_ULL()
  bpf: don't use GENMASK()
  kcsan: don't use GENMASK()
  mm: don't use GENMASK()
  fprobe: don't use GENMASK()
  fs: don't use GENMASK()
  fortify-string: don't use GENMASK()
  Docs: add Functions parameters order section

 Documentation/process/coding-style.rst        | 48 ++++++++++++++
 arch/arc/include/asm/disasm.h                 | 62 +++++++++----------
 arch/arc/kernel/disasm.c                      | 46 +++++++-------
 drivers/gpu/drm/rockchip/rockchip_lvds.h      |  2 +-
 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c  |  4 +-
 drivers/i2c/busses/i2c-nomadik.c              | 44 ++++++-------
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |  4 +-
 drivers/mfd/db8500-prcmu-regs.h               |  2 -
 drivers/mmc/host/dw_mmc-rockchip.c            |  4 +-
 .../net/wireless/intel/iwlwifi/fw/api/coex.h  |  2 -
 drivers/soc/rockchip/grf.c                    |  4 +-
 fs/erofs/internal.h                           |  2 +-
 fs/f2fs/data.c                                |  2 +-
 fs/f2fs/inode.c                               |  2 +-
 fs/f2fs/segment.c                             |  2 +-
 fs/f2fs/super.c                               |  2 +-
 fs/proc/task_mmu.c                            |  2 +-
 fs/resctrl/pseudo_lock.c                      |  2 +-
 fs/select.c                                   |  6 +-
 include/asm-generic/fprobe.h                  |  7 +--
 include/linux/bitmap.h                        | 11 ++--
 include/linux/bits.h                          | 11 ++++
 include/linux/f2fs_fs.h                       |  2 +-
 include/linux/find.h                          | 34 +++++-----
 include/linux/fortify-string.h                |  4 +-
 kernel/bpf/verifier.c                         |  4 +-
 kernel/kcsan/encoding.h                       |  4 +-
 kernel/trace/fgraph.c                         | 10 +--
 kernel/trace/trace_probe.h                    |  2 +-
 lib/842/842_compress.c                        |  2 +-
 lib/842/842_decompress.c                      |  2 +-
 lib/bitmap.c                                  |  2 +-
 lib/test_bitmap.c                             | 14 ++---
 lib/zlib_inflate/inflate.c                    | 48 +++++++-------
 mm/debug_vm_pgtable.c                         |  2 +-
 sound/core/oss/rate.c                         | 12 ++--
 sound/soc/rockchip/rockchip_i2s_tdm.h         |  2 +-
 37 files changed, 235 insertions(+), 180 deletions(-)

-- 
2.43.0
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by Linus Torvalds 3 months, 2 weeks ago
On Sat, 25 Oct 2025 at 09:40, Yury Norov (NVIDIA) <yury.norov@gmail.com> wrote:
>
> GENMASK(high, low) notation reflects a common pattern to describe
> hardware registers. However, out of drivers context it's confusing and
> error-prone.

So I obviously approve of the BITS() syntax, and am not a huge fan of
the odd "GENMASK()" argument ordering.

That said, I'm not convinced about adding the other helpers. I don't
think "FIRST_BITS(8)" is any more readable than "BITS(0,7)", and I
think there's a real danger to having lots of specialized macros that
people then have to know what they mean.

I have an absolutely *disgusting* trick to use the syntax

    BITS(3 ... 25)

to do this all, but it's so disgusting and limited that I don't think
it's actually reasonable.

In case somebody can come up with a cleaner model, here's the trick:

   #define LOWRANGE_0 0,
   #define LOWRANGE_1 1,
   #define LOWRANGE_2 2,
   #define LOWRANGE_3 3,
   #define LOWRANGE_4 4,
   #define LOWRANGE_5 5,
   // ..and so on

   #define _HIGH_VAL(x) (sizeof((char[]){[x]=1})-1)
   #define __FIRST(x, ...) x
   #define ___LOW_VAL(x) __FIRST(x)
   #define __LOW_VAL(x) ___LOW_VAL(LOWRANGE_ ##x)
   #define _LOW_VAL(x) __LOW_VAL(x)

   #define BITS(x) GENMASK(_HIGH_VAL(x), _LOW_VAL(x))

   #define TESTVAL 5
   unsigned long val1 = BITS(3 ... 25);
   unsigned long val2 = BITS(4);
   unsigned long val3 = BITS(TESTVAL ... 31);

and that syntax with either "3 ... 25" or just a plain "4" does
actually work. But only with fairly simple numbers.

It doesn't work with more complex expressions (due to the nasty
preprocessor pasting hack), and I couldn't figure out a way to make it
do so.

I also suspect that we shouldn't do the core code conversions without
having acks from maintainers. Some people may prefer the odd
"high,low" ordering due to it matching some equally odd hardware
documentation.

               Linus
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by Rasmus Villemoes 2 months, 1 week ago
On Sat, Oct 25 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 25 Oct 2025 at 09:40, Yury Norov (NVIDIA) <yury.norov@gmail.com> wrote:
>>
>> GENMASK(high, low) notation reflects a common pattern to describe
>> hardware registers. However, out of drivers context it's confusing and
>> error-prone.
>
> So I obviously approve of the BITS() syntax, and am not a huge fan of
> the odd "GENMASK()" argument ordering.
>
> That said, I'm not convinced about adding the other helpers. I don't
> think "FIRST_BITS(8)" is any more readable than "BITS(0,7)", and I
> think there's a real danger to having lots of specialized macros that
> people then have to know what they mean.
>
> I have an absolutely *disgusting* trick to use the syntax
>
>     BITS(3 ... 25)
>
> to do this all, but it's so disgusting and limited that I don't think
> it's actually reasonable.
>
> In case somebody can come up with a cleaner model, here's the trick:
>
>    #define LOWRANGE_0 0,
>    #define LOWRANGE_1 1,
>    #define LOWRANGE_2 2,
>    #define LOWRANGE_3 3,
>    #define LOWRANGE_4 4,
>    #define LOWRANGE_5 5,
>    // ..and so on
>
>    #define _HIGH_VAL(x) (sizeof((char[]){[x]=1})-1)
>    #define __FIRST(x, ...) x
>    #define ___LOW_VAL(x) __FIRST(x)
>    #define __LOW_VAL(x) ___LOW_VAL(LOWRANGE_ ##x)
>    #define _LOW_VAL(x) __LOW_VAL(x)
>
>    #define BITS(x) GENMASK(_HIGH_VAL(x), _LOW_VAL(x))
>
>    #define TESTVAL 5
>    unsigned long val1 = BITS(3 ... 25);
>    unsigned long val2 = BITS(4);
>    unsigned long val3 = BITS(TESTVAL ... 31);
>
> and that syntax with either "3 ... 25" or just a plain "4" does
> actually work. But only with fairly simple numbers.
>
> It doesn't work with more complex expressions (due to the nasty
> preprocessor pasting hack), and I couldn't figure out a way to make it
> do so.

It's cute, but no, I also don't know how to make it work without the
preprocessor concatenation stuff.

There is, however, an alternative that resembles the syntax in data
sheets even more closely:

#define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))

That'll work for

unsigned long val1 = BITS(3:25);
unsigned long val3 = BITS(TESTVAL:31);

for most of the things TESTVAL might expand to - I'm not sure what would
happen if one of the lo/hi values contains ternaries and isn't properly
parenthesized, but nobody writes stuff like

#define RESET_BIT(rev) rev == 0xaa ? 7 : 9

It doesn't work for the single-bit case, but I don't think it's so bad
to have to say

unsigned long val2 = BIT(4);

and obviously BITS(4:4) would work as well.

It also doesn't do anything to prevent the hi-lo 11:7 order.

Rasmus
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by Linus Torvalds 2 months, 1 week ago
On Wed, 26 Nov 2025 at 01:07, Rasmus Villemoes <ravi@prevas.dk> wrote:
>
> There is, however, an alternative that resembles the syntax in data
> sheets even more closely:
>
> #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))

Oh, me likey. That's a much better idea than my crazy syntax.

            Linus
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by david laight 2 months, 1 week ago
On Wed, 26 Nov 2025 11:44:59 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 26 Nov 2025 at 01:07, Rasmus Villemoes <ravi@prevas.dk> wrote:
> >
> > There is, however, an alternative that resembles the syntax in data
> > sheets even more closely:
> >
> > #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))  
> 
> Oh, me likey. That's a much better idea than my crazy syntax.
> 
>             Linus
> 

I've had a couple of further thoughts...

1) GENMASK() could just swap the two arguments if they are constant and
in the wrong order, so GENMASK(2, 4) and GENMASK(4, 2) are equivalent.
Easily done without too much bloat in a statement expression.

2) It a lot of cases the result from GENMASK() is only ever passed to FIELD_XXX().
The latter really don't want the mask - they want the two bit numbers.
(In particular the low bit number.)

So you could change the headers to have (eg):
#define REGISTER_BITS_FOR_XXX (5:7)

Then the expected calling 'convention' would be FIELD_XXX((lo:hi), val).
All done with:
#define _BIT_LO(lo_hi) (1 ? lo_hi)
#define _BIT_HI(lo_hi) (0 ? lo_hi)
#define FIELD_XXX(lo_hi, ...) ({
	u32 _lo = _BIT_LO lo_hi;
	u32 _hi = _BIT_HI lo_hi;
	...

Any attempted use without the 'magic macros' will be a nice syntax error.

	David
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by david laight 2 months, 1 week ago
On Wed, 26 Nov 2025 11:44:59 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 26 Nov 2025 at 01:07, Rasmus Villemoes <ravi@prevas.dk> wrote:
> >
> > There is, however, an alternative that resembles the syntax in data
> > sheets even more closely:
> >
> > #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))  
> 
> Oh, me likey. That's a much better idea than my crazy syntax.

Mark B. will accuse you of abusing ?: :-)

I've just looked at a .i file.
GENMASK() currently expands to 855 chars plus four copies of each argument.
Both FIELD_PREP/GET(GENMASK(), v) are about 18k plus three copies of v.
FIELD_GET(mask,v) has 18 expansions of 'mask'.

HWEIHT32(GENMASK(15,0)) is about 30k - don't ask why anyone would do it.

They all look excessive.

	David
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by david laight 2 months, 1 week ago
On Wed, 26 Nov 2025 22:17:18 +0000
david laight <david.laight@runbox.com> wrote:

...
> Both FIELD_PREP/GET(GENMASK(), v) are about 18k plus three copies of v.
> FIELD_GET(mask,v) has 18 expansions of 'mask'.

All the FIELD_XXX() are already statement expressions.
I think that means they should 'just work' if the parameters are
copied to locals - annoyingly there are a few bitfields...

Even after that a shed-load (or two) of bloat comes from __BF_FIELD_CHECK().
Not helped by expanding pointless checks because the same define is
used lots of times.

I'm not really sure the expensive test that uses __bf_cast_unsigned()
is actually worth doing - unless a trivial equivalent can be found.
Actually the 'mask' side can just be 'mask + 0u + 0ul + 0ull' which
zero-extends 'mask';
the other just '~0ull >> (64 - sizeof(_reg) * 8)'.

I see a patch lurking...

	David
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by Linus Torvalds 2 months, 1 week ago
On Wed, 26 Nov 2025 at 14:17, david laight <david.laight@runbox.com> wrote:
>
> Mark B. will accuse you of abusing ?: :-)

Compared to '__is_constexpr()', this is child's play.  Not *that* is
abusing the ternary op.

> I've just looked at a .i file.
> GENMASK() currently expands to 855 chars plus four copies of each argument.

Yeah, I don't love it. It's a horrid macro. But because of the odd
order of arguments, it needs all that crazy checking.

That said, I do think it could be simplified. Particularly if we just
make the rule be that GENMASK _has_ to take just constant values.

Right now, I think 99.9% of all users are constants, and we spend a
lot of effort on the 0.1% that isn't.

               Linus
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by david laight 2 months, 1 week ago
On Wed, 26 Nov 2025 15:47:28 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 26 Nov 2025 at 14:17, david laight <david.laight@runbox.com> wrote:
> >
> > Mark B. will accuse you of abusing ?: :-)  
> 
> Compared to '__is_constexpr()', this is child's play.  Not *that* is
> abusing the ternary op.
> 
> > I've just looked at a .i file.
> > GENMASK() currently expands to 855 chars plus four copies of each argument.  
> 
> Yeah, I don't love it. It's a horrid macro. But because of the odd
> order of arguments, it needs all that crazy checking.
> 
> That said, I do think it could be simplified. Particularly if we just
> make the rule be that GENMASK _has_ to take just constant values.
> 
> Right now, I think 99.9% of all users are constants, and we spend a
> lot of effort on the 0.1% that isn't.

There is the other 0.1% that actually need an 'integer constant expression'.
They stop you using ({ ...}).
I doubt there are many (if any) for FIELD_PREP() and FIELD_GET() and using
	auto _mask = mask;
would shrink those massively.
(If there are any I suspect you are the only person who can fix them
in one release cycle.)

I've not looked closely at the expansions, but as well as some is_constexpr()
(that could probably be 'statically_true()') there is the _Generic() that
generates u8/u16/u32/u64 based on 'something'.
That is mostly entirely pointless, you just need to pick u32 or u64, the u8/u16
get promoted to 'int' as soon as they are used - so it only makes a difference
to code that looks at the type of the variable/result.

In spite of the faffing, the type of FIELD_GET() is u64.

	David

> 
>                Linus
>
Re: [PATCH 00/21] lib: add alternatives for GENMASK()
Posted by david laight 2 months, 1 week ago
On Wed, 26 Nov 2025 10:07:34 +0100
Rasmus Villemoes <ravi@prevas.dk> wrote:

> On Sat, Oct 25 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Sat, 25 Oct 2025 at 09:40, Yury Norov (NVIDIA) <yury.norov@gmail.com> wrote:  
> >>
> >> GENMASK(high, low) notation reflects a common pattern to describe
> >> hardware registers. However, out of drivers context it's confusing and
> >> error-prone.  
> >
> > So I obviously approve of the BITS() syntax, and am not a huge fan of
> > the odd "GENMASK()" argument ordering.
> >
> > That said, I'm not convinced about adding the other helpers. I don't
> > think "FIRST_BITS(8)" is any more readable than "BITS(0,7)", and I
> > think there's a real danger to having lots of specialized macros that
> > people then have to know what they mean.
> >
> > I have an absolutely *disgusting* trick to use the syntax
> >
> >     BITS(3 ... 25)
> >
> > to do this all, but it's so disgusting and limited that I don't think
> > it's actually reasonable.
> >
> > In case somebody can come up with a cleaner model, here's the trick:
> >
> >    #define LOWRANGE_0 0,
> >    #define LOWRANGE_1 1,
> >    #define LOWRANGE_2 2,
> >    #define LOWRANGE_3 3,
> >    #define LOWRANGE_4 4,
> >    #define LOWRANGE_5 5,
> >    // ..and so on
> >
> >    #define _HIGH_VAL(x) (sizeof((char[]){[x]=1})-1)
> >    #define __FIRST(x, ...) x
> >    #define ___LOW_VAL(x) __FIRST(x)
> >    #define __LOW_VAL(x) ___LOW_VAL(LOWRANGE_ ##x)
> >    #define _LOW_VAL(x) __LOW_VAL(x)
> >
> >    #define BITS(x) GENMASK(_HIGH_VAL(x), _LOW_VAL(x))
> >
> >    #define TESTVAL 5
> >    unsigned long val1 = BITS(3 ... 25);
> >    unsigned long val2 = BITS(4);
> >    unsigned long val3 = BITS(TESTVAL ... 31);
> >
> > and that syntax with either "3 ... 25" or just a plain "4" does
> > actually work. But only with fairly simple numbers.
> >
> > It doesn't work with more complex expressions (due to the nasty
> > preprocessor pasting hack), and I couldn't figure out a way to make it
> > do so.  
> 
> It's cute, but no, I also don't know how to make it work without the
> preprocessor concatenation stuff.
> 
> There is, however, an alternative that resembles the syntax in data
> sheets even more closely:
> 
> #define BITS(low_high) GENMASK((0 ? low_high), (1 ? low_high))

That is both beautiful and horrid...
 
> 
> That'll work for
> 
> unsigned long val1 = BITS(3:25);
> unsigned long val3 = BITS(TESTVAL:31);
> 
> for most of the things TESTVAL might expand to - I'm not sure what would
> happen if one of the lo/hi values contains ternaries and isn't properly
> parenthesized, but nobody writes stuff like
> 
> #define RESET_BIT(rev) rev == 0xaa ? 7 : 9
> 
> It doesn't work for the single-bit case, but I don't think it's so bad
> to have to say
> 
> unsigned long val2 = BIT(4);
> 
> and obviously BITS(4:4) would work as well.
> 
> It also doesn't do anything to prevent the hi-lo 11:7 order.

For constants that gets trapped.
If the correct expression in used in GENMASK() the compiler will reject
the negative shift - so doesn't need an explicit test.

Not directly related...

GENMASK() and the FIELD_GET() family need to go on massive diets.
Nest them and the pre-processor generates multi-kb lines.

Somewhere there is a (probably pointless) _Generic() for all the integer types.
(Especially since FIELD_GET() is always u64.)

FIELD_GET(mask, val) is just (val & mask) / (mask & -mask), but even that
really needs mask assigned to a local (just to reduce the line length).
For variable 'mask' you might worry about the cost of the 'divide by power of 2'
but for constants it doesn't matter.

FIELD_PREP(mask, val) is (val * (mask & -mask)) & mask

'Bloat city' comes from:
#define MASK GENMASK(9, 0);
	x = FIELD_GET(MASK, MASK);

I've just looked at bitfield.h - no wonder it bloats the world.
95% of it needs deleting :-)

	David

> 
> Rasmus
>