Re: [PATCH v6 15/15] ACPI: CPPC: Clear Performance Limited without a stale read
From: Sumit Gupta
Date: Mon Sep 07 2026 - 06:34:55 EST
Hi Christian,
Continuing the discussion from v4 [1].
On 30/08/26 17:26, Christian Loehle wrote:
External email: Use caution opening links or attachments
The Performance Limited status bits are sticky and write-zero-to-clear.
ACPI 6.6 Section 8.4.6.1.3.2 also requires both entities to use interlocked
accesses.
cppc_set_perf_limited() currently reads the register, computes a new value,
and writes it in a separate transaction. If the platform reports another
excursion between those transactions, the stale write can clear that new
event.
Write zero to the requested bits and one to the other defined status bits
directly. Keep reserved bits zero as required for hardware status registers
by ACPI 6.6 Section 4.6.1. This removes the stale read window.
A partial SystemMemory field would still make the generic writer perform a
read-modify-write to preserve the containing access unit. The
per-descriptor spinlock cannot interlock that RMW with platform updates, so
reject clears of such a field. Keep the descriptor mapped and readable,
because reading the containing access unit once and extracting the field
does not require RMW.
Classify a field as a writer during overlap validation only when its _CPC
semantics permit writes and its validated resource remains writable. This
allows partial Performance Limited fields whose clear path was disabled to
share an access unit with other read-only fields, while still rejecting an
actual writer in that access unit.
Also reject another writable SystemMemory field sharing Performance
Limited's access unit. Its RMW could similarly replay stale status bits,
and an OSPM lock cannot serialize against the platform.
Also reject 64-bit SystemMemory descriptions on 32-bit kernels, where
generic readq()/writeq() may be split into two 32-bit operations and cannot
provide the required portable interlocked access. A naturally aligned
full-width QWord remains supported on 64-bit kernels, where the
architecture provides a native 64-bit MMIO accessor.
Retain any inaccessible Performance Limited descriptor whose conservative
physical range is still locatable, while marking both reads and writes
unsupported. This includes a QWord on a 32-bit kernel. Skip its mapping and
the flexible-address-space capability gate, because Linux will issue no
access, without hiding the asynchronous status range from
neighbouring-writer validation. Both the interval registry and pairwise
overlap test use the larger of the access unit and logical field span, so a
malformed field extending beyond its nominal access unit remains covered.
Performance Limited status is not required for CPPC control. If firmware
describes it without even a locatable physical range, disable that status
register instead of rejecting the processor's otherwise usable _CPC
package. Report reads as unsupported rather than returning a synthetic
zero, and emit a single warning for each nonfatal fallback.
Fixes: 13c45a26635f ("ACPI: CPPC: add APIs and sysfs interface for perf_limited")
Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
Link: https://sashiko.dev/#/patchset/20260807111303.1062391-1-christian.loehle%40arm.com
Signed-off-by: Christian Loehle <christian.loehle@xxxxxxx>
---
drivers/acpi/cppc_acpi.c | 76 +++++++++++++++++++++++++++++-----------
1 file changed, 56 insertions(+), 20 deletions(-)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 25bccd4cfb34..a07440ed7f80 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -443,6 +443,22 @@ static int cpc_validate_sysmem_reg(struct cpc_desc *cpc_desc,
if (!cpc_reg_access_aligned(gas, access_size))
goto invalid;
+ if (reg_idx == PERF_LIMITED) {
+ if (access_width == 64 && !IS_ENABLED(CONFIG_64BIT)) {
+ pr_warn("CPU%d: Performance Limited register cannot be accessed atomically; keeping its range reserved\n",
+ cpc_desc->cpu_id);
+ cpc_desc->cpc_regs[reg_idx].cpc_entry.read_unsupported = true;
+ cpc_desc->cpc_regs[reg_idx].cpc_entry.write_unsupported = true;
+ return 0;
+ }
+
+ if (gas->bit_offset || gas->bit_width != access_width) {
+ pr_warn("CPU%d: Performance Limited register cannot be cleared safely; keeping it readable\n",
+ cpc_desc->cpu_id);
+ cpc_desc->cpc_regs[reg_idx].cpc_entry.write_unsupported = true;
+ }
+ }
Agreed on the generic behavior. With Bit Width 2 the remaining bits are
not part of the register, and the driver cannot treat them as reserved.
On the firmware option, I confirmed with the hardware team that
bits 31:2 here are unimplemented. They read as zero, have no side
effects when written, and are unused elsewhere.
Future firmware can describe the register with Bit Width 32, but systems
already shipped cannot be updated. For those I have a patch which widens
the descriptor to the access width, so the clear becomes the single
DWord write you describe. It is pasted below and same attached.
Testing with that applied uncovered a second issue.
The commit description says the status bits are write-zero-to-clear.
I could not find where that is specified, have I missed something?
ACPI spec describes the register as Read/Write and requires interlocked
operations, which reads as an expectation of read-modify-write,
but I found nothing defining what a written one does.
Here a written one sets the bit, and the hardware team confirmed the
register is plain Read/Write on my test platform.
Writing CPPC_PERF_LIMITED_MASK & ~bits_to_clear therefore sets the bit
which is not being cleared, and Linux reports an excursion the platform
never signalled:
#cat /sys/devices/system/cpu/cpu0/cpufreq/perf_limited
0
#echo 0x1 > /sys/devices/system/cpu/cpu0/cpufreq/perf_limited
#cat /sys/devices/system/cpu/cpu0/cpufreq/perf_limited
2
The read before the write avoided this, at the cost of the stale read
race you describe. Clearing both bits would still need no read at all,
because the value written is zero in either case.
How would you prefer to handle that?
Thanks,
Sumit
[1] https://lore.kernel.org/lkml/d5f1ea9b-53b7-4db2-983a-b5be8e71a371@xxxxxxx/
-- >8 --
From: Sumit Gupta <sumitg@xxxxxxxxxx>
Date: Thu, 3 Sep 2026 20:17:42 +0530
Subject: [PATCH 1/1] ACPI: CPPC: Keep Performance Limited clearable where it
owns its unit
Firmware may describe Performance Limited as a field narrower than the
access unit given by its Access Size. Clearing such a field needs a
read-modify-write to preserve the rest of the unit. That cannot be
interlocked against the platform setting further status bits. The clear
is therefore disabled, and writes to the perf_limited attribute return
-EOPNOTSUPP.
The generic code cannot do better. Per ACPI 6.6 Section 5.2.3.2, Bit
Width is the size of the register while Access Size only describes the
transaction. Bits beyond Bit Width are not part of the register, so they
may hold unrelated state and Table 8.26 says nothing about them.
Some platforms do implement the register alone in its access unit, with
the remaining bits unimplemented, reading as zero and without side
effects when written. Firmware conveys that by declaring Bit Width 32,
and _CPC offers no other way to express it. Describe the register as
owning the unit on those platforms. The clear then becomes a single
interlocked write with no read, and the status bits stay clearable.
Add the NVIDIA platforms with that property, matched on the OEM ID and
OEM Table ID of the DSDT. Only a descriptor narrower than its access
unit is widened, so firmware which already describes the register
accurately is left alone and the fixup lapses once such firmware ships.
Reads now return the whole unit, which is correct here because those
bits read as zero. Overlap validation is unaffected, since its range
derives from Access Size and already covered the complete unit.
Amend the descriptor before it is copied, so layout validation, the
read-modify-write lock decision and overlap checking all see the
corrected width. The descriptor lives in the _CPC output buffer, which
this function allocates and frees, so amending it in place is safe.
Change-Id: I4817d3d0a08a02603d925819b733428f96ecf0d8
Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx>
---
drivers/acpi/cppc_acpi.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index a07440ed7f80..1985e19f9eb0 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -330,6 +330,44 @@ static unsigned int cpc_reg_access_width(const struct cpc_reg *reg)
return reg->bit_width;
}
+/*
+ * Platforms which implement Performance Limited alone in its access unit, with
+ * the remaining bits unimplemented, reading as zero and without side effects
+ * when written.
+ */
+static const struct acpi_platform_list cpc_perf_limited_owns_unit[] = {
+ { "NVIDIA", "T41", 0, ACPI_SIG_DSDT, all_versions },
+ { }
+};
+
+/*
+ * A Performance Limited field narrower than its access unit cannot be
+ * cleared, because preserving the rest of the unit needs a read-modify-write
+ * and an OSPM lock cannot interlock that against the platform. Where the
+ * register owns the whole unit, describe it that way so the clear becomes a
+ * single interlocked write.
+ *
+ * Widen only a field at Bit Offset 0, so the widened field still describes
+ * exactly the access unit.
+ */
+static void cpc_fixup_perf_limited_width(struct cpc_reg *gas,
+ unsigned int reg_idx)
+{
+ unsigned int access_width = cpc_reg_access_width(gas);
+
+ if (reg_idx != PERF_LIMITED ||
+ gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY ||
+ gas->bit_offset || gas->bit_width >= access_width)
+ return;
+
+ if (acpi_match_platform_list(cpc_perf_limited_owns_unit) < 0)
+ return;
+
+ pr_info_once("Performance Limited owns its access unit, using Bit Width %u\n",
+ access_width);
+ gas->bit_width = access_width;
+}
+
static u64 cpc_sysmem_access_size(const struct cpc_register_resource *reg)
{
unsigned int width = cpc_reg_access_width(®->cpc_entry.reg);
@@ -1861,6 +1899,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
goto out_free;
}
+ cpc_fixup_perf_limited_width(gas_t, i - 2);
+
cpc_ptr->cpc_regs[i - 2].type = ACPI_TYPE_BUFFER;
memcpy(&cpc_ptr->cpc_regs[i - 2].cpc_entry.reg, gas_t,
sizeof(*gas_t));
--
2.34.1
....From 82287a99415fbfa53ef4fd107dc6d16bbf86d6d1 Mon Sep 17 00:00:00 2001
From: Sumit Gupta <sumitg@xxxxxxxxxx>
Date: Thu, 3 Sep 2026 20:17:42 +0530
Subject: [PATCH 1/1] ACPI: CPPC: Keep Performance Limited clearable where it
owns its unit
X-NVConfidentiality: public
Firmware may describe Performance Limited as a field narrower than the
access unit given by its Access Size. Clearing such a field needs a
read-modify-write to preserve the rest of the unit. That cannot be
interlocked against the platform setting further status bits. The clear
is therefore disabled, and writes to the perf_limited attribute return
-EOPNOTSUPP.
The generic code cannot do better. Per ACPI 6.6 Section 5.2.3.2, Bit
Width is the size of the register while Access Size only describes the
transaction. Bits beyond Bit Width are not part of the register, so they
may hold unrelated state and Table 8.26 says nothing about them.
Some platforms do implement the register alone in its access unit, with
the remaining bits unimplemented, reading as zero and without side
effects when written. Firmware conveys that by declaring Bit Width 32,
and _CPC offers no other way to express it. Describe the register as
owning the unit on those platforms. The clear then becomes a single
interlocked write with no read, and the status bits stay clearable.
Add the NVIDIA platforms with that property, matched on the OEM ID and
OEM Table ID of the DSDT. Only a descriptor narrower than its access
unit is widened, so firmware which already describes the register
accurately is left alone and the fixup lapses once such firmware ships.
Reads now return the whole unit, which is correct here because those
bits read as zero. Overlap validation is unaffected, since its range
derives from Access Size and already covered the complete unit.
Amend the descriptor before it is copied, so layout validation, the
read-modify-write lock decision and overlap checking all see the
corrected width. The descriptor lives in the _CPC output buffer, which
this function allocates and frees, so amending it in place is safe.
Change-Id: I4817d3d0a08a02603d925819b733428f96ecf0d8
Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx>
---
drivers/acpi/cppc_acpi.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index a07440ed7f80..1985e19f9eb0 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -330,6 +330,44 @@ static unsigned int cpc_reg_access_width(const struct cpc_reg *reg)
return reg->bit_width;
}
+/*
+ * Platforms which implement Performance Limited alone in its access unit, with
+ * the remaining bits unimplemented, reading as zero and without side effects
+ * when written.
+ */
+static const struct acpi_platform_list cpc_perf_limited_owns_unit[] = {
+ { "NVIDIA", "T41", 0, ACPI_SIG_DSDT, all_versions },
+ { }
+};
+
+/*
+ * A Performance Limited field narrower than its access unit cannot be
+ * cleared, because preserving the rest of the unit needs a read-modify-write
+ * and an OSPM lock cannot interlock that against the platform. Where the
+ * register owns the whole unit, describe it that way so the clear becomes a
+ * single interlocked write.
+ *
+ * Widen only a field at Bit Offset 0, so the widened field still describes
+ * exactly the access unit.
+ */
+static void cpc_fixup_perf_limited_width(struct cpc_reg *gas,
+ unsigned int reg_idx)
+{
+ unsigned int access_width = cpc_reg_access_width(gas);
+
+ if (reg_idx != PERF_LIMITED ||
+ gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY ||
+ gas->bit_offset || gas->bit_width >= access_width)
+ return;
+
+ if (acpi_match_platform_list(cpc_perf_limited_owns_unit) < 0)
+ return;
+
+ pr_info_once("Performance Limited owns its access unit, using Bit Width %u\n",
+ access_width);
+ gas->bit_width = access_width;
+}
+
static u64 cpc_sysmem_access_size(const struct cpc_register_resource *reg)
{
unsigned int width = cpc_reg_access_width(®->cpc_entry.reg);
@@ -1861,6 +1899,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
goto out_free;
}
+ cpc_fixup_perf_limited_width(gas_t, i - 2);
+
cpc_ptr->cpc_regs[i - 2].type = ACPI_TYPE_BUFFER;
memcpy(&cpc_ptr->cpc_regs[i - 2].cpc_entry.reg, gas_t,
sizeof(*gas_t));
--
2.34.1