Re: [PATCH v7 0/20] ACPI: CPPC: Fix register access and lifetime bugs

[Sumit Gupta <sumitg@nvidia.com>]

Hi Christian,


On 16/09/26 21:57, Christian Loehle wrote:
> External email: Use caution opening links or attachments
> 
> 
> This series fixes malformed _CPC handling, control-write error propagation,
> PCC ownership and cleanup, CPC sysfs lifetime, register-field access,
> cross-processor aliases, and Performance Limited clearing.
> 
> Series structure
> ================
> 
> Patches 1-7 cover parsing and control semantics, error propagation,
> PCC update serialization, and descriptor/PCC cleanup. Patches 8-15
> validate the register layouts and aliases which the existing accessors and
> locking can safely support, and correct Performance Limited clearing.
> Patch 16 bounds x86 and arm64 FFH fields before accessing hardware, checks
> x86 MSR numbers, and dispatches FFH before generic Access Size decoding.
> Patch 17 propagates failed cross-CPU FFH calls on arm64 and RISC-V before
> using callback output, including counter reads for an offline CPU.
> Patch 18 accepts a request to retain immutable Autonomous Selection as a
> no-op, rejects unsupported amd-pstate transitions to passive mode before
> changing state or removing the driver, and restores cppc-cpufreq's previous
> mode if a later bounds update fails. The immutable-capability query is
> serialized with descriptor publication and removal.
> Patch 19 selects the frequency-invariance callback per CPU, avoiding
> uninitialized deferred work in policies mixing PCC and direct counters.
> Patch 20 creates the FIE worker when policy initialization encounters PCC
> counters, before publishing callbacks. This covers offline and later-added
> PCC members without making direct-counter FIE depend on worker setup.
> 
> The layout fixes are more substantial: a physical access unit can contain
> several fields or be shared across processors and _PSD domains. Probe-time
> interval registries enforce the assumptions of the existing
> descriptor-local locking model. They add no lookup to the scheduler path;
> standalone full-width SystemMemory writes remain lockless. Disjoint fields
> within one descriptor retain their shared lock when their access units
> overlap, including a full-width byte beside a wider partial-field access.
> 
> Behavior description
> ==================
> 
> The parser validates the package before indexing its entries and the
> Generic Register descriptor consumed by the driver. It tolerates trailing
> package/ResourceTemplate data and legacy Integer-zero absent controls.
> Buffer-backed capabilities and bound readbacks are checked before narrowing
> to u32. Minimum and Maximum Performance must form a usable pair, and
> an explicit minimum-valid flag preserves a zero Minimum Performance
> readback across cpufreq initialization and exit. Maximum Performance and
> amd-pstate's unsaved firmware-minimum restore requests retain their existing
> zero-means-omitted convention.
> 
> Lowest Performance must remain nonzero, as before the series. Accepting zero
> needs separate changes to consumers which assume a zero frequency origin
> or use a zero capability as an absence sentinel. This does not change the
> explicit zero Minimum Performance update used to remove a lower bound.
> Invalid firmware-provided Lowest/Nominal Frequency values remain an error.
> 
> PCC ownership is acquired before staging payload updates. Direct-only,
> batched PCC, and mixed-address-space performance updates retain their
> separate ordering requirements. PCC lifecycle locking and payload-copy
> serialization protect different state.
> 
> Frequency-invariance policy teardown remembers which PCC work was
> initialized, so it can drain that work after processor removal unpublishes
> the CPC descriptor. Callback selection uses that same per-CPU initialization
> state rather than looking up the potentially removed descriptor again.
> Worker creation is serialized and occurs only when initializing PCC
> counters, before that policy publishes callbacks. Setup failure leaves
> that policy without FIE, but does not disable direct-counter or already
> active policies. Direct-only systems create no worker or deadline
> reservation. Direct-counter CPUs continue updating in the tick.
> This does not solve the separate runtime-accessor lifetime races with
> processor removal (which I will post in a later series).
> 
> SystemMemory permits read-only overlaps, supported exact writable aliases,
> and safe descriptor-local partial writes. A full-width writer may share a
> larger read access unit across descriptors when their logical fields are
> disjoint. Only controls written by the CPPC library count as competing
> writers (i.e. OSPM Nominal Performance is not written). Its write-only
> readback protections still apply. The validation rejects cross-descriptor
> partial writers which its locks cannot serialize and accesses which replay
> a write-only neighbour's undefined readback. SystemIO permits read-only
> aliases with write-only controls, but not with readable controls whose
> readback would select the wrong register. Unsupported optional controls are
> disabled only where doing so cannot silently change the operating mode.
> Present but inaccessible Enable or Autonomous Selection controls fail
> probe, including sub-byte PCC forms which the existing writer cannot
> program. This may reject firmware-enabled configurations that previously
> worked until the driver attempted an unsupported control access.
> Optional-writer fallback only changes unpublished descriptors. A conflict
> with an already-published writer can still fail probe; making recovery
> independent of CPU discovery order requires runtime quiescence support.
> 
> Performance Limited is sticky, but ACPI does not define the effect of
> writing one. A selective clear can either replay stale status or set a bit
> on a plain read/write implementation. Write 0x3 to the perf_limited
> attribute to clear both bits with one literal-zero register write;
> selective clears return -EOPNOTSUPP. Partial SystemMemory/SystemIO status
> fields remain readable but cannot be cleared generically. This is a status
> acknowledgement, not a lossless event log.
> 
> Inaccessible status ranges remain visible to overlap validation without
> being accessed. Sumit Gupta's NVIDIA T41 quirk widens only the verified
> two-bit, offset-zero DWord descriptor whose remaining bits are unimplemented,
> allowing that platform to use the safe clear-all operation.
> 
> The general SystemMemory/SystemIO alignment checks retain the x86 exception
> for unaligned accesses. Performance Limited currently inherits that policy.
> This is not a guarantee of an indivisible device transaction at an unaligned
> address; the stricter status-atomicity question remains separate work. No
> torn access or affected platform has been demonstrated by the reviews.
> 
> Testing
> =======
> 
> An earlier iteration was built and booted on Orion O6-01, with CPPC sysfs,
> frequency changes, advancing counters and module reloads checked. The
> subsequent AMD transition guard and FIE fixes have passed source/model
> checks and checkpatch, but this tip has not yet been built or booted.
> Those checks are not AMD hardware, mixed-PCC hotplug or concurrency tests.
> 
> Changes since v6
> ================
> 
> Patch 1: Validate the _CPC package header
>    Unchanged from v6.
> 
> Patch 2: Validate _CPC entry and control semantics
>    Allow the full 64-bit Integer Counter Wraparound Time and emit FW_BUG
>    once for tolerated legacy Integer-zero Buffer placeholders. (Rafael)
>    Drop v6's zero Lowest Performance support: nonzero frequency anchors do
>    not make it safe for all consumers. Retain the pre-series nonzero check.
>    Reject Minimum/Maximum readbacks above U32_MAX before narrowing them.
>    Keep only min_perf_valid: an explicit zero minimum removes a lower bound,
>    whereas zero Maximum Performance retains its legacy omitted-update
>    meaning. Document the flag and leave it clear for amd-pstate's unsaved
>    firmware-minimum restore requests.
> 
> Patch 3: Propagate performance-control write errors
>    Adjust for patch 2's minimum-only validity flag; a zero maximum remains
>    an omitted update.
> 
> Patch 4: Serialize PCC single-register payload updates (v6 patch 5)
>    Reject unsupported PCC widths before taking the exclusive lock, so a
>    malformed request cannot abort an older valid performance batch.
>    Match both subspace and generation when completing failed PCC writes;
>    equal generation numbers in another subspace must not receive the error.
> 
> Patch 5: Serialize PCC EPP payload updates (v6 patch 6)
>    Preflight PCC widths and SystemIO writer geometry before changing any
>    direct control. This keeps the patch independently safe before the later
>    probe-time layout validation. Pass the subspace to error completion.
>    Patches 10 and 11 remove these preflights once probe guarantees them.
> 
> Patch 6: Release CPC descriptors through kobject (v6 patch 7)
>    Track initialized PCC frequency-invariance work independently of the CPC
>    descriptor, so policy exit still drains it after descriptor unpublication.
>    Make the counter-transport query tolerate a missing descriptor. Broader
>    runtime-accessor lifetime protection remains deferred.
> 
> Patch 7: Release PCC data after probe failures (v6 patch 8)
>    No functional changes from v6. Only clarify the channel-reuse comment.
> 
> Patch 8: Reject unsafe cross-CPU SystemMemory RMW (v6 patch 9)
>    Mark successful relaxed MMIO writes pending for lock-handoff ordering.
>    Use the interval walk to lock both same-descriptor overlapping accesses,
>    including mixed-width full/partial fields, without another overlap pass.
>    Permit a full-width writer beside a disjoint logical read-only field,
>    including across descriptors when the reader uses a larger access unit.
>    Do not count OSPM Nominal Performance as a competing writer: Linux does
>    not write it. Its write-only readback protections remain in patch 9.
>    Exact-alias coalescing and representative promotion were already in v6
>    and remain present; they are not additions in this version.
> 
> Patch 9: Reject direct reads of write-only controls (v6 patch 10)
>    Also reject readable SystemMemory fields whose logical bits overlap a
>    write-only control, including retained inaccessible fields. Treat a
>    retained zero-width field conservatively before the no-writer shortcut.
>    Reuse the logical-field overlap check instead of a separate full-width
>    ownership helper. Remove the duplicate Desired Performance getter check;
>    the common getter already enforces write-only semantics.
> 
> Patch 10: Validate and access PCC register layouts (v6 patch 11)
>    Fail probe for a present inaccessible Autonomous Selection control,
>    instead of hiding it without establishing the hardware's operating mode.
>    Separate the final bound-pair check from mandatory-control validation.
>    Remove setter PCC-width preflights once probe validates every published
>    writer, and remove unreachable PCC branches in the memory accessors.
>    Keep actual access-error handling and ownership serialization.
> 
> Patch 11: Validate SystemIO register layouts (v6 patch 12)
>    Allow a read-only port alias with a write-only control, but reject aliases
>    with readable controls whose readback would select the wrong register.
>    Remove the EPP setter's duplicate SystemIO geometry preflight once probe
>    enforces it. Read-only partial-field support and the x86 exception to
>    natural alignment were already in v6 and remain unchanged.
> 
> Patch 12: Validate PCC overlaps across processors (v6 patch 13)
>    Replace the separate same-descriptor overlap pass with registry checks.
>    Drop v6's expansion to additional byte-multiple PCC writer widths; retain
>    the existing 8/16/32/64-bit widths, subject to control-specific limits.
>    Keep one interval record per descriptor entry; PCC alias coalescing is
>    deferred. Correct Fixes to 80b8286aeec0 (CPPC request batching).
> 
> Patch 13: Validate SystemIO overlaps across processors (v6 patch 14)
>    Apply patch 11's direction-aware alias policy across processors and
>    remove the now-redundant same-descriptor overlap pass. Retain per-entry
>    interval records; SystemIO alias coalescing remains deferred.
> 
> Patch 14: Clear Performance Limited without a stale read (v6 patch 15)
>    Stop assuming write-zero-to-clear semantics for written ones. Reject
>    selective clears and implement clear-all (sysfs 0x3) with one literal-zero
>    register write, avoiding both stale readback and fabricated status. (Sumit)
>    Extend the readable-but-not-clearable fallback to partial SystemIO fields.
>    Extend inaccessible-range retention to PCC and SystemIO, including status
>    crossing the PCC payload end or port 0xffff. PCC claims retain their own
>    subspace without selecting or retaining an unnecessary access channel.
>    Permit disabling only unpublished, disjoint optional SystemMemory writers
>    which share a status access unit; never hide a required or Autonomous
>    Selection control. Published writers are not changed during another probe.
>    Compare bit geometry as well as byte ranges for exact PCC/SystemIO aliases,
>    and count only accessible controls as writers in those registries.
>    Make status fallback warnings once-only and document the clear-all and
>    unsupported-access behavior in the sysfs ABI.
> 
> Patch 15: Keep Performance Limited clearable on NVIDIA T41 (new)
>    Add Sumit's quirk for the verified two-bit, offset-zero DWord layout.
>    Use a cached DSDT match and quirk flags, applying the fixup to the private
>    GAS copy before validation and mapping. Distinguish lookup failures from
>    non-matches and leave failures retryable. Keep all quirk plumbing here,
>    so patch 14 remains independently buildable without unused declarations.
> 
> Patch 16: Validate FFH register fields before hardware access (new)
>    Reject invalid x86 field geometry and GAS addresses that would truncate
>    to another 32-bit MSR number. Bound arm64 AMU fields in both single and
>    paired readers. Dispatch FFH before generic Access Size decoding can
>    shift by an invalid amount using an architecture-specific field.
> 
> Patch 17: Propagate errors from cross-CPU FFH calls (new)
>    Return failed SMP-call errors on arm64 and RISC-V before consuming
>    uninitialized callback output. Also check RISC-V callback errors before
>    copying a read value. Keep this as a separate backportable error-path fix.
> 
> Patch 18: Accept requests to retain immutable autonomous selection (new)
>    Treat setting immutable Autonomous Selection Integer 1 to one as a
>    successful no-op; disabling it still fails. Distinguish immutable-one
>    descriptors from writable controls currently reading one. Reject an
>    unsupported amd-pstate transition to passive before changing the mode or
>    unregistering the working driver, checking known offline CPUs too.
>    Serialize the immutable query with descriptor publication/removal,
>    including probe failure, since CPU-hotplug locking does not pin it.
>    On a later bounds-update failure, restore cppc-cpufreq's previous mode
>    rather than always disabling selection.
> 
> Patch 19: Select the frequency-invariance callback per CPU (new)
>    Register the PCC callback only on CPUs with PCC counters. A shared policy
>    can also contain direct-counter CPUs, whose irq_work is not initialized.
>    Select from the recorded work-initialization state, so descriptor removal
>    between initialization and publication cannot change the callback choice.
>    Keep callback registration after the complete counter-initialization
>    pass, so an online CPU's failed initial read publishes no CPPC callbacks.
> 
> Patch 20: Create the FIE worker before enabling PCC callbacks (new)
>    Create the worker when initializing PCC counters, rather than relying
>    on an online-only startup scan. Serialize creation and reuse it until
>    driver teardown. Offline members and later PCC policies are covered.
>    On setup failure, skip FIE for that policy without changing the global
>    setting or affecting existing policies. Direct-only systems do not
>    allocate an unused worker or consume deadline admission bandwidth.
> 
> Dropped v6 patch 4: Use 64-bit masks for register fields
>    All supported CPPC configurations are already 64-bit, so this is only
>    a cleanup I'll submit later on.
> 
> Separate lifetime work
> ======================
> 
> The PCC mailbox teardown fixes are posted separately: [1] is their cover,
> [2] frees the channel before unmapping its shared memory, and [3] serializes
> channel updates with shared-memory teardown. They are not included here;
> this series alone does not close the mailbox IRQ-teardown races.
> 
> [1] https://lore.kernel.org/all/20260903111328.805352-1-christian.loehle@arm.com/
> [2] https://lore.kernel.org/all/20260903111328.805352-2-christian.loehle@arm.com/
> [3] https://lore.kernel.org/all/20260903111328.805352-3-christian.loehle@arm.com/
> 
> Christian Loehle (19):
>    ACPI: CPPC: Validate the _CPC package header
>    ACPI: CPPC: Validate _CPC entry and control semantics
>    ACPI: CPPC: Propagate performance-control write errors
>    ACPI: CPPC: Serialize PCC single-register payload updates
>    ACPI: CPPC: Serialize PCC EPP payload updates
>    ACPI: CPPC: Release CPC descriptors through kobject
>    ACPI: CPPC: Release PCC data after probe failures
>    ACPI: CPPC: Reject unsafe cross-CPU SystemMemory RMW
>    ACPI: CPPC: Reject direct reads of write-only controls
>    ACPI: CPPC: Validate and access PCC register layouts
>    ACPI: CPPC: Validate SystemIO register layouts
>    ACPI: CPPC: Validate PCC overlaps across processors
>    ACPI: CPPC: Validate SystemIO overlaps across processors
>    ACPI: CPPC: Clear Performance Limited without a stale read
>    ACPI: CPPC: Validate FFH register fields before hardware access
>    ACPI: CPPC: Propagate errors from cross-CPU FFH calls
>    ACPI: CPPC: Accept requests to retain immutable autonomous selection
>    cpufreq: CPPC: Select the frequency-invariance callback per CPU
>    cpufreq: CPPC: Create the FIE worker before enabling PCC callbacks
> 
> Sumit Gupta (1):
>    ACPI: CPPC: Keep Performance Limited clearable on NVIDIA T41
> 
>   Documentation/ABI/testing/sysfs-devices-system-cpu |   16 +-
>   arch/arm64/kernel/topology.c                       |   14 +-
>   arch/x86/kernel/acpi/cppc.c                        |   14 +
>   drivers/acpi/cppc_acpi.c                           | 2209 +++++++++++++++++---
>   drivers/acpi/riscv/cppc.c                          |   26 +-
>   drivers/acpi/utils.c                               |   13 +-
>   drivers/cpufreq/amd-pstate.c                       |   31 +-
>   drivers/cpufreq/cppc_cpufreq.c                     |   73 +-
>   include/acpi/cppc_acpi.h                           |   18 +-
>   9 files changed, 2024 insertions(+), 390 deletions(-)
> 
> base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
> --
> 2.34.1


Tested the v7 on v7.3-rc3, with both _CPC rev 3 and 4.

For the entire series:
Tested-by: Sumit Gupta <sumitg@nvidia.com>


Patch 20/20 had a conflict in drivers/cpufreq/cppc_cpufreq.c with
linux-next 20260916.

The series applied cleanly to the base commit specified in the cover
letter (fd73f4a66598, tag v7.3-rc3), so patch 20/20 may need to be
rebased for the current linux-next.

Thanks,
Sumit