Linux Tegra architecture development
 help / color / mirror / Atom feed
* Re: [PATCH v7 15/20] ACPI: CPPC: Keep Performance Limited clearable on NVIDIA T41
       [not found]     ` <CAJZ5v0g-ipGKWyEUKrBGTo+B6RYqWxba4KhK9RaCJmh2MRLtLA@mail.gmail.com>
@ 2026-09-17 18:20       ` Sumit Gupta
  0 siblings, 0 replies; 3+ messages in thread
From: Sumit Gupta @ 2026-09-17 18:20 UTC (permalink / raw)
  To: Rafael J. Wysocki (Intel), Christian Loehle
  Cc: Viresh Kumar, linux-pm, linux-acpi, linux-kernel, Jie Zhan,
	Lifeng Zheng, Pierre Gondois, Sudeep Holla, Ionela Voinescu,
	zhongqiu.han, linux-tegra@vger.kernel.org



On 17/09/26 18:53, Rafael J. Wysocki (Intel) wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Sep 17, 2026 at 2:59 PM Christian Loehle
> <christian.loehle@arm.com> wrote:
>>
>> On 9/16/26 17:28, Christian Loehle wrote:
>>> From: Sumit Gupta <sumitg@nvidia.com>
>>>
>>> NVIDIA T41 firmware describes Performance Limited as a two-bit field at
>>> offset zero in a DWord SystemMemory access unit. Generic CPPC code must
>>> keep such a field read-only because preserving the remainder when clearing
>>> it requires a read-modify-write which cannot be interlocked with platform
>>> updates.
>>>
>>> The remaining bits of this access unit are unimplemented on T41: they read
>>> as zero, writes have no side effects, and no other register uses them. The
>>> Performance Limited register therefore owns the complete access unit, but
>>> shipped firmware does not describe that property accurately.
>>>
>>> Add a CPPC platform-quirk table keyed by the DSDT header and carry quirk
>>> behavior through explicit flags. Cache a successful table lookup, copy each
>>> GAS into the driver's private descriptor, and apply fixups before layout
>>> validation, mapping and overlap registration.
>>>
>>> Distinguish a genuine non-match from a table-header lookup failure in
>>> acpi_match_platform_list(). Propagate lookup errors from CPPC probe without
>>> caching them, so a transient mapping failure cannot disable the workaround
>>> for every later processor. Existing matcher callers still treat all
>>> negative results as no match.
>>>
>>> For the known T41 layout only, widen a two-bit Performance Limited field at
>>> offset zero to its 32-bit access width. Clearing both status bits can then
>>> be issued as one DWord write of zero without a stale read. Corrected
>>> firmware which reports the full width is unchanged. The workaround
>>> therefore lapses automatically when corrected firmware ships.
>>>
>>> Link: https://lore.kernel.org/lkml/d5f1ea9b-53b7-4db2-983a-b5be8e71a371@arm.com/
>>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>>> [ Rework quirk matching and fixup placement; propagate lookup failures
>>>    without caching them. ]
>>> Signed-off-by: Christian Loehle <christian.loehle@arm.com>
>>> ---
>>>   drivers/acpi/cppc_acpi.c | 72 ++++++++++++++++++++++++++++++++++++++++
>>>   drivers/acpi/utils.c     | 13 ++++++--
>>>   2 files changed, 82 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index 6d130381245e..0e218f2be0fe 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -330,6 +330,68 @@ static unsigned int cpc_reg_access_width(const struct cpc_reg *reg)
>>>        return reg->bit_width;
>>>   }
>>>
>>> +enum cpc_platform_quirk {
>>> +     CPC_QUIRK_PERF_LIMITED_OWNS_UNIT = BIT(0),
>>> +};
>>> +
>>> +static const struct acpi_platform_list cpc_platform_quirk_list[] = {
>>> +     {
>>> +             .oem_id = "NVIDIA",
>>> +             .oem_table_id = "T41",
>>
>> Sashiko:
>> "Will this quirk successfully match a standard ACPI table header?
>> The ACPI specification requires the OEM Table ID to be exactly 8 bytes long,
>> typically padded with trailing spaces by compliant firmware (e.g.,
>> "T41     ").
>> Looking at acpi_match_platform_list(), it compares the IDs using:
>> strncmp(plat->oem_table_id, hdr.oem_table_id, ACPI_OEM_TABLE_ID_SIZE)
>> Because "T41" is a null-terminated 3-character string, strncmp() will
>> compare the 4th character ('\0' from the quirk definition vs ' ' from the
>> ACPI table header) and immediately report a mismatch, causing the quirk to
>> silently fail on compliant firmware.
>> Should this be padded with spaces (e.g., "T41     ") to ensure it matches
>> the firmware's table correctly?"
>>
>> non-padded "T41" matches exactly what Sumit proposed and was discussed in v6:
>> https://lore.kernel.org/lkml/55a5c9fa-cfd3-4000-b3cc-52c343841c9f@nvidia.com/
>> So once Sumit adds Tested-by: this should be fine.
> 
> Sure, thanks!
> 
> Sumit, any concerns?

Hi Rafael,

No concerns.
"T41" without space padding is correct because this firmware NUL pads
the eight-byte field:

  # od -An -tc -j 16 -N 8 /sys/firmware/acpi/tables/DSDT
    T   4   1  \0  \0  \0  \0  \0

The quirk matched, producing the following message during boot:

  ] ACPI CPPC: firmware quirk: Performance Limited owns its access unit, 
using Bit Width 32

Thanks,
Sumit


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v7 0/20] ACPI: CPPC: Fix register access and lifetime bugs
       [not found] <20260916162805.1039247-1-christian.loehle@arm.com>
       [not found] ` <20260916162805.1039247-16-christian.loehle@arm.com>
@ 2026-09-17 18:30 ` Sumit Gupta
  2026-09-18 15:30   ` Rafael J. Wysocki (Intel)
  1 sibling, 1 reply; 3+ messages in thread
From: Sumit Gupta @ 2026-09-17 18:30 UTC (permalink / raw)
  To: Christian Loehle, Rafael J . Wysocki, Viresh Kumar
  Cc: linux-pm, linux-acpi, linux-kernel, Len Brown, Jie Zhan,
	Lifeng Zheng, Pierre Gondois, Sudeep Holla, Ionela Voinescu,
	zhongqiu.han, linux-tegra@vger.kernel.org

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



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v7 0/20] ACPI: CPPC: Fix register access and lifetime bugs
  2026-09-17 18:30 ` [PATCH v7 0/20] ACPI: CPPC: Fix register access and lifetime bugs Sumit Gupta
@ 2026-09-18 15:30   ` Rafael J. Wysocki (Intel)
  0 siblings, 0 replies; 3+ messages in thread
From: Rafael J. Wysocki (Intel) @ 2026-09-18 15:30 UTC (permalink / raw)
  To: Sumit Gupta, Christian Loehle
  Cc: Viresh Kumar, linux-pm, linux-acpi, linux-kernel, Jie Zhan,
	Lifeng Zheng, Pierre Gondois, Sudeep Holla, Ionela Voinescu,
	zhongqiu.han, linux-tegra@vger.kernel.org

On Thu, Sep 17, 2026 at 8:31 PM Sumit Gupta <sumitg@nvidia.com> wrote:
>
> 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.

Thank you both!

Series applied as 7.4 material, thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-09-18 15:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260916162805.1039247-1-christian.loehle@arm.com>
     [not found] ` <20260916162805.1039247-16-christian.loehle@arm.com>
     [not found]   ` <9ad67536-7b6e-4633-8cc7-52b0d68196fc@arm.com>
     [not found]     ` <CAJZ5v0g-ipGKWyEUKrBGTo+B6RYqWxba4KhK9RaCJmh2MRLtLA@mail.gmail.com>
2026-09-17 18:20       ` [PATCH v7 15/20] ACPI: CPPC: Keep Performance Limited clearable on NVIDIA T41 Sumit Gupta
2026-09-17 18:30 ` [PATCH v7 0/20] ACPI: CPPC: Fix register access and lifetime bugs Sumit Gupta
2026-09-18 15:30   ` Rafael J. Wysocki (Intel)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox