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

On 09/08/26 11:55, 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.
>
> Reject SystemMemory descriptions which require read-modify-write to
> preserve the containing access unit, because the per-descriptor spinlock
> cannot interlock that RMW with platform updates. 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.
>
> Performance Limited status is not required for CPPC control. If firmware
> describes it using an access that Linux cannot interlock safely, disable
> that status register instead of rejecting the processor's otherwise usable
> _CPC package.
>
> 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 | 33 ++++++++++++++++++++++-----------
>   1 file changed, 22 insertions(+), 11 deletions(-)

On my test platform Performance Limited is a 2-bit field at offset 0 with
Access Size 3 (DWord), so bit_width (2) != access_width (32). This is a
valid GAS description. The mismatch only means that the generic writer
would use RMW.

I agree with removing the stale read. However, even if writes are rejected
for this partial width layout, reads do not require RMW. Could the register
remain readable instead of being disabled completely?

If the register must be disabled, the current fallback is reported
incorrectly. Performance Limited is mandatory in _CPC. cpc_disable_reg()
replaces it with an Integer 0, but cppc_get_reg_val() only recognizes
that as unsupported for registers in REG_OPTIONAL, which does not
include PERF_LIMITED. Reads therefore succeed with zero while writes
return -EOPNOTSUPP, so userspace sees "never limited" instead of
"<unsupported>". This hides real events on a platform that can signal
them.

Also, pr_err followed by pr_warn emits two messages per CPU for this
nonfatal fallback. Could these messages be consolidated?

Thanks,
Sumit
....