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

[Christian Loehle]

On 8/26/26 14:59, Sumit Gupta wrote:
> 
> On 26/08/26 12:00, 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.
>>
>> 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.
>>
>> Performance Limited status is not required for CPPC control. If firmware
>> describes it using an access which Linux cannot read safely, 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>
>> ---
> 
> Performance Limited is readable but not clear-able on my test platform,
> with a warning on every CPU:
> 
>   ACPI CPPC: CPU0: Performance Limited register cannot be cleared safely; keeping it readable
> 
> This occurs with both _CPC revision 3 and revision 4.
> 
> Could clearing remain supported?
> cppc_set_perf_limited() now builds the W0C value without reading the
> register first. Here the field is 2 bits at Bit Offset 0 in a 32-bit
> access unit. If Section 4.6.1 allows bits 31:2 to be treated as
> reserved status bits, the value could be issued as a single 32-bit write
> with those bits zero. That avoids RMW.

Thanks. I agree that a single DWord write would avoid the RMW need but I
don't think we can safely infer that bits 31:2 are reserved.

Section 5.2.3.2 defines Register Bit Width as the size of the given
register, while Access Size only specifies the transaction size.
With Bit Width 2, Table 8.26 designates reserved bits only within that
described register, it does not make the remaining bits of the 32-bit
access unit part of Performance Limited. Those bits could contain
unrelated state which must (generally) be preserved.

> 
> A full-width description already writes zero to the reserved bits.
> Is the same treatment valid for bits outside the GAS Bit Width when
> Bit Offset is zero?
> 

The full-width case is different: Bit Width 32 makes bits 31:2 part of the
Performance Limited register, so Table 8.26 identifies them as reserved and
Section 4.6.1 permits writing zero.

Therefore I think the generic driver must keep a 2-bit/DWord descriptor
readable but not clearable. If this platform guarantees that bits 31:2
have no side effects, firmware should just describe the register with
Bit Width 32? Then the current code would clear it using one DWord write.

Does that make sense? Did I miss anything?

> Apart from this, cppc_cpufreq probes and basic frequency scaling work
> on this platform.
> 

Good to hear and thanks for testing!
(That being said, I'll drop your tags for v5 unfortunately as I have changes
in the middle of the series :/ )