Message-ID: <CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com>
Date: Thu, 2 Oct 2025 09:46:01 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Yury Norov <yury.norov@...il.com>
Cc: linux-kernel@...r.kernel.org, NVIDIA <YuryNorovyury.norov@...il.com>, 
	Alice Ryhl <aliceryhl@...gle.com>, Burak Emir <bqe@...gle.com>, 
	Miguel Ojeda <miguel.ojeda.sandonis@...il.com>, 
	Nicolas Frattaroli <nicolas.frattaroli@...labora.com>, Jakub Kicinski <kuba@...nel.org>, 
	Heiko Stuebner <heiko@...ech.de>
Subject: Re: [GIT PULL] bitmap for 6.18

On Mon, 29 Sept 2025 at 13:07, Yury Norov <yury.norov@...il.com> wrote:
>
>  - almost complete consolidation for HIWORD_UPDATE()-like macros from Nicolas;

Argh. I don't love this, but I've pulled it.

That new interface is a bit odd, and I had to wrap my head around it a
bit, but it actually looks fine when it's a single bit or it already
has a mask defined for it, and the end result is something like

        FIELD_PREP_WM16(BIT(1), val)

Yes, this is often longer than what it replaces, but quite readable,
and I think it's a good thing.

But then we have GENMASK.

The macro from hell, that was a mistake from day 0, but then took over
despite always being illogical.

It was so illogical that it had to have lots of error checking,
because it always had the wrong interface and as a result lots of
people got it wrong.

So now it has compile-time checking of the bits to make sure people
get notified when they invariably get things wrong.

The only saving grace of that thing is that checking. I feel that
often the *only* reason to use GENMASK() over any other alternative is
literally that it checks the arguments so much because the interface
is so horrific.

It does "high, low", which is often very unintuitive, and in fact the
very commit that introduced this thing from hell had to convert the
sane "low,high" cases to the other way around.

See commit 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK
macro"), and notice how ALMOST ALL use cases were switched around to
the illogical "high,low" format by that introductory phase.

And yes, I understand why that person did it: many datasheets show
bits in a register graphically, and then you see that "high .. low"
thing in a rectangle that describes the register, and that ordering
them makes 100% sense IN THAT CONTEXT.

But it damn well does not make sense in most other contexts.

In fact, even in the context of generating mask #defines, it actually
reads oddly, because you end up having things like

  /* Status register (SR) */
  #define I2C_SR_OP               GENMASK(1, 0)   /* Operation */
  #define I2C_SR_STATUS           GENMASK(3, 2)   /* controller status */
  #define I2C_SR_CAUSE            GENMASK(6, 4)   /* Abort cause */
  #define I2C_SR_TYPE             GENMASK(8, 7)   /* Receive type */
  #define I2C_SR_LENGTH           GENMASK(19, 9)  /* Transfer length */

(Yes, that's a real example from the kernel), and notice how *oddly*
the numbers flow in that series: instead of being a logical
progression of 0 .. 1 .. 2 .. 3 etc, you have 1 .. 0 .. 3 .. 2 .. 6 ..
4 etc)

I really have an almost irrational hatred of GENMASK (note "almost". I
think it's rational). I hate it so much that this almost made me not
pull the end result just because a few conversions were just
horrendous.

The very first conversion in that series does this:

-               mci_writel(host, TIMING_CON1, HIWORD_UPDATE(raw_value,
0x07ff, 1));
+               mci_writel(host, TIMING_CON1,
+                          FIELD_PREP_WM16(GENMASK(11, 1), raw_value));

and no, that new version is *NOT* more readable than the old code. I
had to really read it several times just to understand what was going
on, admittedly because the old HIWORD_UPDATE() macro was also odd.

But at least that old HIWORD_UPDATE() was odd in a "natural" way.

And it's all because GENMASK() is a horrible horrible interface, and
should only be used for generating other #defines for actual masks
when you read the datasheet.

Anyway. I just wanted to state how much I hate GENMASK(). It's almost
fine for when it is appropriate, but this is an example of where it's
very much not appropriate.

I wish we had a saner form for generating bitmasks with a better
interface. The "high,low" thing is horrendous when what you want is "I
want X bits starting at X":

Maybe we could introduce a new macro to go with BIT(x), and call it
"BITS(low,high)". Yes, we'd have to replace a few existing driver uses
of BITS(), but not very many.

Am I the only person who would find "BITS(1,11)" to be much easier to
understand than "GENMASK(11,1)"?

Random "Linus rants" email over.

             Linus

Powered by blists - more mailing lists