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(-)
On Thu, 25 Apr 2024 at 10:46, Rafael J. Wysocki <[email protected]> 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
On Thu, 25 Apr 2024 at 11:58, Linus Torvalds
<[email protected]> 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
On Thu, 25 Apr 2024 at 11:58, Linus Torvalds
<[email protected]> 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
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
On Thu, Apr 25, 2024 at 9:18 PM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, 25 Apr 2024 at 11:58, Linus Torvalds
> <[email protected]> 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