[GIT PULL] ACPI fixes for v6.9-rc6

[GIT PULL] ACPI fixes for v6.9-rc6

[Rafael J. Wysocki]

Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-6.9-rc6

with top-most commit 2ad984673beef7c3dbe9e3d2cabf046f338fdffc

 Merge branch 'acpi-cppc'

on top of commit ed30a4a51bb196781c8058073ea720133a65596f

 Linux 6.9-rc5

to receive ACPI fixes for 6.9-rc6.

These fix three recent regressions, one introduced while enabling a new
platform firmware feature for power management, and two introduced by
a recent CPPC library update.

Specifics:

 - Allow two overlapping Low-Power S0 Idle _DSM function sets to be used
   at the same time (Rafael Wysocki).

 - Fix bit offset computation in MASK_VAL() macro used for applying
   a bitmask to a new CPPC register value (Jarred White).

 - Fix access width field usage for PCC registers in CPPC (Vanshidhar
   Konda).

Thanks!


---------------

Jarred White (1):
      ACPI: CPPC: Fix bit_offset shift in MASK_VAL() macro

Rafael J. Wysocki (1):
      ACPI: PM: s2idle: Evaluate all Low-Power S0 Idle _DSM functions

Vanshidhar Konda (1):
      ACPI: CPPC: Fix access width used for PCC registers

---------------

 drivers/acpi/cppc_acpi.c  | 57 ++++++++++++++++++++++++++++++++---------------
 drivers/acpi/x86/s2idle.c |  8 +++----
 2 files changed, 42 insertions(+), 23 deletions(-)

Re: [GIT PULL] ACPI fixes for v6.9-rc6

[Linus Torvalds]

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

Re: [GIT PULL] ACPI fixes for v6.9-rc6

[Linus Torvalds]

On Thu, 25 Apr 2024 at 11:58, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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?

Just to clarify: that was written in the MUA, and entirely untested.
Somebody should still verify it, but really, with already now two
bugs, that macro needs fixing for good, and the "for good" should be
looking at least _something_ like the above.

And despite needing fixing, I've done the pull, since bug #2 is at
least less bad than bug#1 was.

                   Linus

Re: [GIT PULL] ACPI fixes for v6.9-rc6

[Linus Torvalds]

On Thu, 25 Apr 2024 at 11:58, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And maybe this time, it's not a buggy mess?

Actually, even with MASK_VAL() fixed, I think it's *STILL* a buggy mess.

Why? Beuse the *uses* of MASK_VAL() seem entirely bogus.

In particular, we have this in cpc_write():

        if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
                val = MASK_VAL(reg, val);

        switch (size) {
        case 8:
                writeb_relaxed(val, vaddr);
                break;
        case 16:
                writew_relaxed(val, vaddr);
                break;
        ...

and I strongly suspect that it needs to update the 'vaddr' too. Something like

        if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
                val = MASK_VAL(reg, val);
  #ifdef __LITTLE_ENDIAN
                vaddr += reg->bit_offset >> 3;
                if (reg->bit_offset & 7)
                        return -EFAULT;
  #else
                /* Fixme if we ever care */
                if (reg->bit_offset)
                        return -EFAULT;
  #endif
        }

*might* be changing this in the right direction, but it's unclear and
I neither know that CPC rules, nor did I think _that_ much about it.

Anyway, the take-away should be that all this code is entirely broken
and somebody didn't think enough about it.

It's possible that that whole cpc_write() ACPI_ADR_SPACE_SYSTEM_MEMORY
case should be done as a 64-bit "read-mask-write" sequence.

Possibly with "reg->bit_offset == 0" and the 8/16/32/64-bit cases as a
special case for "just do the write".

Or, maybe writes with a non-zero bit offset shouldn't be allowed at
all, and there are CPC rules that aren't checked. I don't know. I only
know that the current code is seriously broken.

                   Linus

Re: [GIT PULL] ACPI fixes for v6.9-rc6

[pr-tracker-bot]

The pull request you sent on Thu, 25 Apr 2024 19:45:50 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-6.9-rc6

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a93289b830ce783955b22fbe5d1274a464c05acf

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] ACPI fixes for v6.9-rc6

[Rafael J. Wysocki]

On Thu, Apr 25, 2024 at 9:18 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, 25 Apr 2024 at 11:58, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > And maybe this time, it's not a buggy mess?
>
> Actually, even with MASK_VAL() fixed, I think it's *STILL* a buggy mess.
>
> Why? Beuse the *uses* of MASK_VAL() seem entirely bogus.
>
> In particular, we have this in cpc_write():
>
>         if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
>                 val = MASK_VAL(reg, val);
>
>         switch (size) {
>         case 8:
>                 writeb_relaxed(val, vaddr);
>                 break;
>         case 16:
>                 writew_relaxed(val, vaddr);
>                 break;
>         ...
>
> and I strongly suspect that it needs to update the 'vaddr' too. Something like
>
>         if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>                 val = MASK_VAL(reg, val);
>   #ifdef __LITTLE_ENDIAN
>                 vaddr += reg->bit_offset >> 3;
>                 if (reg->bit_offset & 7)
>                         return -EFAULT;
>   #else
>                 /* Fixme if we ever care */
>                 if (reg->bit_offset)
>                         return -EFAULT;
>   #endif
>         }
>
> *might* be changing this in the right direction, but it's unclear and
> I neither know that CPC rules, nor did I think _that_ much about it.

This is a very nice catch, thank you!

> Anyway, the take-away should be that all this code is entirely broken
> and somebody didn't think enough about it.
>
> It's possible that that whole cpc_write() ACPI_ADR_SPACE_SYSTEM_MEMORY
> case should be done as a 64-bit "read-mask-write" sequence.
>
> Possibly with "reg->bit_offset == 0" and the 8/16/32/64-bit cases as a
> special case for "just do the write".
>
> Or, maybe writes with a non-zero bit offset shouldn't be allowed at
> all, and there are CPC rules that aren't checked. I don't know. I only
> know that the current code is seriously broken.

In any case, this needs to be taken care of (Jared?).

Thanks,
Rafael