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

On 8/25/26 10:15, Sumit Gupta wrote:
> 
> 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?

Ack, I can turn this into read-only if not writable by the kernel.

> 
> 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.

Ack, I've fixed the getter.

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

Ack

> 
> Thanks,
> Sumit
> ....
> 
>