Re: [PATCH v2 0/2] ACPI: CPPC: Add CPPC v4 support (ACPI 6.6)

[Sumit Gupta]

Hi Rafael,


On 09/05/26 00:31, Rafael J. Wysocki wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, Apr 30, 2026 at 4:25 PM Sumit Gupta <sumitg@xxxxxxxxxx> wrote:
> > Add initial kernel support for CPPC v4 (ACPI 6.6, Section 8.4.6),
> > which extends the _CPC package from 23 to 25 entries with two
> > optional fields:
> >
> >    - OSPM Nominal Performance (8.4.6.1.2.6): register used by OSPM
> >      to tell the platform what it considers nominal. The platform
> >      classifies performance above this as boost and below as
> >      throttle for power/thermal decisions.
> >
> >    - Resource Priority (8.4.6.1.2.7): Package of Resource Priority
> >      Register Descriptor sub-packages. Full parsing is not yet
> >      implemented; such entries are marked as unsupported.
> >
> > Patch 1: Add v4 _CPC parsing - validate the 25-entry layout,
> > handle the Resource Priority package, and mark the two new
> > registers optional.
> >
> > Patch 2: Add acpi_cppc/ospm_nominal_perf as a read-write sysfs
> > attribute, and initialize it to the platform nominal value
> > during cppc_cpufreq policy init.
> >
> > ---
> > v1[1] -> v2:
> > - Patch 1: added Reviewed-by from Mario Limonciello.
> > - Patch 2:
> >    - Make ospm_nominal_perf sysfs read-write; cache last write in
> >      cpc_desc and skip redundant register writes.
> >    - Validate input in cppc_set_ospm_nominal_perf.
> >
> > Sumit Gupta (2):
> >    ACPI: CPPC: Add support for CPPC v4
> >    ACPI: CPPC: Add ospm_nominal_perf support
> >
> >   drivers/acpi/cppc_acpi.c       | 93 +++++++++++++++++++++++++++++++---
> >   drivers/cpufreq/cppc_cpufreq.c | 10 ++++
> >   include/acpi/cppc_acpi.h       | 14 ++++-
> >   3 files changed, 109 insertions(+), 8 deletions(-)
> >
> > [1] https://lore.kernel.org/lkml/20260427051823.280419-1-sumitg@xxxxxxxxxx/
> >
> > --
> Can you please see the sashiko.dev feedback on this set:
>
> https://sashiko.dev/#/patchset/20260430142430.755437-1-sumitg%40nvidia.com
>
> and let me know what you think?  Especially regarding the second patch?


Thank you for sharing this.

Patch 1:
- Comments #1 and #2 are pre-existing issues with rare occurrence.
  I will address them in a separate hardening patch.

- Comment #3: In v3, will limit the ACPI_TYPE_PACKAGE handling to the
  RESOURCE_PRIORITY entry. So a Package at any other slot will be
  treated as invalid and abort probe, as it did before this patch.

----------------

Patch 2:
Discussed the changes for v3 in some detail on this thread already
which address most of the points (Please see my reply to Pierre [1]).

Summary of how each point will be addressed below:


> The commit message states the valid range is [Lowest Performance,
> Nominal Performance]. Does this code allow writing arbitrary values
> outside that range by only checking against U32_MAX, without fetching
> the CPU's capabilities to validate the input?
Will fetch the bounds via cppc_get_perf_caps() and reject values
outside [lowest_perf, nominal_perf] in v3.


> If the hardware loses state during a logical CPU hotplug or system
> suspend, but the software cache is not invalidated, will this check
> prevent the register from being correctly re-initialized when the CPU
> comes back online?
The redundant write check will be removed in v3, so the stale cache
failure mode won't be possible.


> Can concurrent sysfs writes permanently desynchronize the software
> cache from the hardware register?
> ...
> Is a lock needed around the read-modify-write cycle?
This will not occur in v3 since concurrent calls for the same
policy are serialized by policy->rwsem at the cpufreq layer (see [1]).


> Additionally, can a time-of-check to time-of-use race lead to a NULL
> pointer dereference if cpc_desc_ptr is initialized concurrently?
> ...
> Would this cause the WRITE_ONCE() to dereference the locally fetched NULL
> cpc_desc pointer? Should this explicitly return -ENODEV early if 
!cpc_desc?
Will add the early -ENODEV return at the top of the function in v3,
eliminating the NULL cpc_desc race.


> For shared cpufreq policies where policy->cpus contains multiple
> logical cores (such as CPUFREQ_SHARED_TYPE_ANY), does this skip
> initializing the secondary CPUs in the domain?
>
> If they are uninitialized, will their local cache remain 0, causing
> sysfs reads for those secondary CPUs to incorrectly return -ENODATA?
Will move the rw sysfs from the per-CPU acpi_cppc interface to a
per-policy cpufreq interface in v3, and write the register on every
CPU in policy->cpus/domain.
The -ENODATA on unwritten read path will go away with the per-CPU node,
and the per-policy show returns 0 until user-space writes a value. See [1].


> Also, since the sysfs attribute is tied to the physical CPU device
> lifetime and persists independently of cpufreq policy teardowns, will
> unconditionally setting the nominal performance here silently clobber
> any persistent userspace configurations when a CPU is taken offline
> and online?
Will drop the unconditional cpu_init write in v3, so the user-set
value won't be overwritten on CPU hotplug.

[1] 
https://lore.kernel.org/all/9c32f75a-294f-4cea-810e-c011c4dd91ab@xxxxxxxxxx/

Thank you,
Sumit Gupta