Message-ID: <8170477.EvYhyI6sBW@workhorse>
Date: Thu, 02 Oct 2025 22:31:57 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Yury Norov <yury.norov@...il.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>
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>,
 Jakub Kicinski <kuba@...nel.org>, Heiko Stuebner <heiko@...ech.de>
Subject: Re: [GIT PULL] bitmap for 6.18

On Thursday, 2 October 2025 18:46:01 Central European Summer Time Linus Torvalds wrote:
> 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.

Hi, that person here.

My datasheets indeed show registers in the probably verilog-derived
high:low notation. An example of what that looks like is publicly
available[1]. The PWM hardware there is the same as the one I'm
working on for the RK3576, where I was motivated to finally address
the HIWORD_UPDATE copypaste throughout drivers. During this work I
discovered, and was told by others, that this is in fact more than
just a Rockchip-specific convention.

The HIWORD_UPDATE macros specifically had the issue that they liked
to deviate from each other, e.g. in argument order. This made jumping
between drivers awkward, because the same symbol could mean two different
things.

FWIW, I am also not a fan of GENMASK's high-to-low order. I do
however prefer it over bare hex values, but that's likely because I
can't really do hex math in my head. I don't know what 0x7ff << 1
is by heart, and I wouldn't know what bit would end up being the
first set bit there.

Since this new macro is decoupled from GENMASK however, I think
people other than me will find it to be useful. Macros that make you
think about whether your mask can be statically checked at compile
time will probably result in fewer people shifting a sign bit into an
awkward place without knowing. It already stopped me from doing
precisely that.

> 
> 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)"?

I'm with you on that, but others may disagree. This is may be an
artefact of Latin writing going left to right while our Arabic
numerals go right to left, I think. Different people will have
different cutoffs for where numerals conceptually start for them
and where text ends.

But then again I also think the writel(var, addr) order is unexpected,
and `ln -s target link_name` is off, and if find(1) tells me paths must
precede expression one more time I'm gonna lose it. So maybe taste in
matters like this isn't tied to numerals at all.

> 
> Random "Linus rants" email over.
> 
>              Linus
> 

Kind regards,
Nicolas Frattaroli

Link: https://opensource.rock-chips.com/images/3/36/Rockchip_RK3506_TRM_Part_1_V1.2-20250811.pdf [1]

Powered by blists - more mailing lists