Re: [PATCH v6 15/15] ACPI: CPPC: Clear Performance Limited without a stale read

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 <[email protected]>
> Link: https://sashiko.dev/#/patchset/20260807111303.1062391-1-christian.loehle%40arm.com
> Signed-off-by: Christian Loehle <[email protected]>
> ---
>   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/[email protected]/


-- >8 --

From: Sumit Gupta <[email protected]>
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 <[email protected]>
---
  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(&reg->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 <[email protected]>
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 <[email protected]>
---
 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(&reg->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