Received-SPF: pass (google.com: domain of linux-kernel+bounces-159048-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <CAJZ5v0iUUmMUo86vBzYJjL4NjoFzpDwD1+c292aP+T++PLv6vQ@mail.gmail.com>
In-Reply-To: <CAJZ5v0iUUmMUo86vBzYJjL4NjoFzpDwD1+c292aP+T++PLv6vQ@mail.gmail.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 25 Apr 2024 11:58:16 -0700
Message-ID: <CAHk-=wj8J78-12QfAoKaLdRi2g1=_U7sv02POShjotcJ6t4nzw@mail.gmail.com>
Subject: Re: [GIT PULL] ACPI fixes for v6.9-rc6
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: ACPI Devel Maling List <linux-acpi@vger.kernel.org>, Linux PM <linux-pm@vger.kernel.org>, 
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"

On Thu, 25 Apr 2024 at 10:46, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
>  - Fix bit offset computation in MASK_VAL() macro used for applying
>    a bitmask to a new CPPC register value (Jarred White).

Honestly, that code should never have used GENMASK() in the first place.

When a helper macro is more complicated than just doing the obvious
thing without it, it's not a helper macro any more.

Doing

    GENMASK(((reg)->bit_width) - 1, 0)

is literally more work than just doing the obvious thing

    ((1ul << (reg)->bit_width) - 1)

and using that "helper" macro was actually more error-prone too as
shown by this example, because of the whole "inclusive or not" issue.

BUT!

Even with that simpler model, that's still entirely buggy, since 'val'
is 64-bit, and these GENMASK tricks only work on 'long'.

Which happens to be ok on x86-64, of course, and maybe in practice all
fields are less than 32 bits in width anyway so maybe it even works on
32-bit, but this all smells HORRIBLY WRONG.

And no, the fix is *NOT* to make that GENVAL() mindlessly just be
GENVAL_ULL().  That fixes the immediate bug, but it shows - once again
- how mindlessly using "helper macros" is not the right thing to do.

When that macro now has had TWO independent bugs, how about you just
write it out with explicit types and without any broken "helpers":

   static inline u64 MASK_VAL(const struct cpc_reg *reg, u64 val)
   {
        u64 mask = (1ull << reg->bit_width)-1;
        return (val >> reg->bit_offset) & mask;
   }

which is a few more lines, but doesn't that make it a whole lot more readable?

And maybe this time, it's not a buggy mess?

               Linus