Re: [PATCH v3 2/2] ACPI: CPPC: Add ospm_nominal_perf support

On 22/05/26 22:50, Rafael J. Wysocki wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, May 14, 2026 at 9:49 PM Sumit Gupta <[email protected]> wrote:
>> Expose the OSPM Nominal Performance register (ACPI 6.6, Section
>> 8.4.6.1.2.6), which conveys the desired nominal performance level
>> at which the platform may run. Unlike the existing read-only
>> Nominal Performance register, it is writable and lets OSPM
>> request a lower nominal level than the platform-reported nominal.
>> The platform classifies performance above this level as boosted
>> and below as throttled for its power/thermal decisions.
>>
>> It is exposed as a per-policy cpufreq sysfs attribute in kHz, to
>> match the cpufreq sysfs unit convention:
>>
>>    /sys/devices/system/cpu/cpufreq/policyN/ospm_nominal_freq
>>
>> The attribute is documented in
>> Documentation/ABI/testing/sysfs-devices-system-cpu.
>>
>> Writes are converted to perf via cppc_khz_to_perf(), validated
>> against [Lowest Performance, Nominal Performance], and applied to
>> every CPU in policy->cpus.
>>
>> The register is write-only; the kernel caches the last written
>> value in struct cppc_cpudata for sysfs readback (returns 0 until
>> userspace writes a value).
>>
>> Signed-off-by: Sumit Gupta <[email protected]>
> Please see sashiko.dev's feedback on this patch:
>
> https://sashiko.dev/#/patchset/20260514194822.1841748-1-sumitg%40nvidia.com
>
> and address it, or let me know why you think that addressing it is not
> necessary.


Hi Rafael,

Sure, going through the Sashiko comments:

Patch 1:
- Comments #1 and #2 were also raised in the earlier Sashiko review.
   They are pre-existing issues with rare occurrence.
   Will address them in a separate hardening patch.

Patch 2:
 > Is it possible for a platform to define lowest_perf as 0?
....
Will fix by adding a bool cpu_data->ospm_nominal_perf_set to
distinguish an unwritten cpu_data->ospm_nominal_perf cache
from a valid 0 perf write.

 > Does passing the 64-bit freq_khz to cppc_khz_to_perf() cause a silent
 > truncation?
....
In v4, will switch freq_khz to 'unsigned int' and use kstrtouint() instead
of kstrtou64().

 > If cppc_set_ospm_nominal_perf() succeeds for the first CPU but fails on
 > a subsequent sibling, does this leave the policy out of sync?
In v4, will add rollback in store_ospm_nominal_freq() to restore the
previous value on siblings already updated before the failure.

Thank you,
Sumit Gupta


>
>> ---
>>   .../ABI/testing/sysfs-devices-system-cpu      | 17 ++++++++
>>   drivers/acpi/cppc_acpi.c                      | 35 ++++++++++++++++
>>   drivers/cpufreq/cppc_cpufreq.c                | 40 +++++++++++++++++++
>>   include/acpi/cppc_acpi.h                      |  7 ++++
>>   4 files changed, 99 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
>> index 82d10d556cc8..ac1bf1b89ac4 100644
>> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
>> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
>> @@ -346,6 +346,23 @@ Description:       Performance Limited
>>
>>                  This file is only present if the cppc-cpufreq driver is in use.
>>
>> +What:          /sys/devices/system/cpu/cpuX/cpufreq/ospm_nominal_freq
>> +Date:          May 2026
>> +Contact:       [email protected]
>> +Description:   OSPM Nominal Performance (kHz)
>> +
>> +               OSPM uses this attribute to request a nominal performance
>> +               level lower than the platform-reported nominal. The
>> +               platform treats performance above this level as boost
>> +               and below as throttle for power and thermal decisions.
>> +
>> +               Read returns the last written value in kHz, or 0 if no
>> +               value has been written. Write a kHz value in the range
>> +               [lowest_freq, nominal_freq].
>> +
>> +               This file is only present if the cppc-cpufreq driver is
>> +               in use.
>> +
>>   What:          /sys/devices/system/cpu/cpu*/cache/index3/cache_disable_{0,1}
>>   Date:          August 2008
>>   KernelVersion: 2.6.27
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index c76cfafa3589..ad6ece16c30d 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1682,6 +1682,41 @@ int cppc_set_epp(int cpu, u64 epp_val)
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_set_epp);
>>
>> +/**
>> + * cppc_set_ospm_nominal_perf() - Write OSPM Nominal Performance register.
>> + * @cpu: CPU on which to write register.
>> + * @ospm_nominal_perf: Value to write to the OSPM Nominal Performance register.
>> + *
>> + * OSPM Nominal Performance conveys the desired nominal performance level
>> + * at which the platform may run. Per ACPI 6.6, s8.4.6.1.2.6, the value
>> + * must lie within [Lowest Performance, Nominal Performance] and may be
>> + * set independently of Minimum, Maximum and Desired performance.
>> + *
>> + * Return: 0 on success or negative error code.
>> + */
>> +int cppc_set_ospm_nominal_perf(int cpu, u64 ospm_nominal_perf)
>> +{
>> +       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> +       struct cppc_perf_caps caps;
>> +       int ret;
>> +
>> +       if (!cpc_desc) {
>> +               pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>> +               return -ENODEV;
>> +       }
>> +
>> +       ret = cppc_get_perf_caps(cpu, &caps);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (ospm_nominal_perf < caps.lowest_perf ||
>> +           ospm_nominal_perf > caps.nominal_perf)
>> +               return -EINVAL;
>> +
>> +       return cppc_set_reg_val(cpu, OSPM_NOMINAL_PERF, ospm_nominal_perf);
>> +}
>> +EXPORT_SYMBOL_GPL(cppc_set_ospm_nominal_perf);
>> +
>>   /**
>>    * cppc_get_auto_act_window() - Read autonomous activity window register.
>>    * @cpu: CPU from which to read register.
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 7e7f9dfb7a24..6379b7ceee34 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -985,11 +985,50 @@ store_energy_performance_preference_val(struct cpufreq_policy *policy,
>>   CPPC_CPUFREQ_ATTR_RW_U64(perf_limited, cppc_get_perf_limited,
>>                           cppc_set_perf_limited)
>>
>> +static ssize_t show_ospm_nominal_freq(struct cpufreq_policy *policy, char *buf)
>> +{
>> +       struct cppc_cpudata *cpu_data = policy->driver_data;
>> +       unsigned int freq_khz;
>> +
>> +       if (!cpu_data->ospm_nominal_perf)
>> +               return sysfs_emit(buf, "0\n");
>> +
>> +       freq_khz = cppc_perf_to_khz(&cpu_data->perf_caps,
>> +                                   cpu_data->ospm_nominal_perf);
>> +       return sysfs_emit(buf, "%u\n", freq_khz);
>> +}
>> +
>> +static ssize_t store_ospm_nominal_freq(struct cpufreq_policy *policy,
>> +                                      const char *buf, size_t count)
>> +{
>> +       struct cppc_cpudata *cpu_data = policy->driver_data;
>> +       unsigned int sib;
>> +       u64 freq_khz;
>> +       u32 perf;
>> +       int ret;
>> +
>> +       ret = kstrtou64(buf, 0, &freq_khz);
>> +       if (ret)
>> +               return ret;
>> +
>> +       perf = cppc_khz_to_perf(&cpu_data->perf_caps, freq_khz);
>> +
>> +       for_each_cpu(sib, policy->cpus) {
>> +               ret = cppc_set_ospm_nominal_perf(sib, perf);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       cpu_data->ospm_nominal_perf = perf;
>> +       return count;
>> +}
>> +
>>   cpufreq_freq_attr_ro(freqdomain_cpus);
>>   cpufreq_freq_attr_rw(auto_select);
>>   cpufreq_freq_attr_rw(auto_act_window);
>>   cpufreq_freq_attr_rw(energy_performance_preference_val);
>>   cpufreq_freq_attr_rw(perf_limited);
>> +cpufreq_freq_attr_rw(ospm_nominal_freq);
>>
>>   static struct freq_attr *cppc_cpufreq_attr[] = {
>>          &freqdomain_cpus,
>> @@ -997,6 +1036,7 @@ static struct freq_attr *cppc_cpufreq_attr[] = {
>>          &auto_act_window,
>>          &energy_performance_preference_val,
>>          &perf_limited,
>> +       &ospm_nominal_freq,
>>          NULL,
>>   };
>>
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 8693890a7275..0b1dcdbea10a 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -153,6 +153,8 @@ struct cppc_cpudata {
>>          struct cppc_perf_fb_ctrs perf_fb_ctrs;
>>          unsigned int shared_type;
>>          cpumask_var_t shared_cpu_map;
>> +       /* Cached OSPM Nominal Performance value (write-only register). */
>> +       u32 ospm_nominal_perf;
>>   };
>>
>>   #ifdef CONFIG_ACPI_CPPC_LIB
>> @@ -180,6 +182,7 @@ extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
>>   extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
>>   extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
>>   extern int cppc_set_epp(int cpu, u64 epp_val);
>> +extern int cppc_set_ospm_nominal_perf(int cpu, u64 ospm_nominal_perf);
>>   extern int cppc_get_auto_act_window(int cpu, u64 *auto_act_window);
>>   extern int cppc_set_auto_act_window(int cpu, u64 auto_act_window);
>>   extern int cppc_get_auto_sel(int cpu, bool *enable);
>> @@ -266,6 +269,10 @@ static inline int cppc_set_epp(int cpu, u64 epp_val)
>>   {
>>          return -EOPNOTSUPP;
>>   }
>> +static inline int cppc_set_ospm_nominal_perf(int cpu, u64 ospm_nominal_perf)
>> +{
>> +       return -EOPNOTSUPP;
>> +}
>>   static inline int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
>>   {
>>          return -EOPNOTSUPP;
>> --
>> 2.34.1
>>
>>