Re: [PATCH v6 6/9] ACPI: CPPC: add APIs and sysfs interface for min/max_perf

[Pierre Gondois <pierre.gondois@arm.com>]

On 1/24/26 21:32, Sumit Gupta wrote:
>
> On 22/01/26 17:06, Pierre Gondois wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 1/20/26 15:56, Sumit Gupta wrote:
>>> Add cppc_get/set_min_perf() and cppc_get/set_max_perf() APIs to read 
>>> and
>>> write the MIN_PERF and MAX_PERF registers.
>>>
>>> Also add sysfs interfaces (min_perf, max_perf) in cppc_cpufreq driver
>>> to expose these controls to userspace. The sysfs values are in 
>>> frequency
>>> (kHz) for consistency with other cpufreq sysfs files.
>>>
>>> A mutex is used to serialize sysfs store operations to ensure hardware
>>> register writes and perf_ctrls updates are atomic.
>>>
>>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>>> ---
>>>   drivers/acpi/cppc_acpi.c       |  44 +++++++++
>>>   drivers/cpufreq/cppc_cpufreq.c | 157 
>>> +++++++++++++++++++++++++++++++++
>>>   include/acpi/cppc_acpi.h       |  20 +++++
>>>   3 files changed, 221 insertions(+)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index 45c6bd6ec24b..46bf45f8b0f3 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -1743,6 +1743,50 @@ int cppc_set_auto_sel(int cpu, bool enable)
>>>   }
>>>   EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
>>>
>>> +/**
>>> + * cppc_get_min_perf - Read minimum performance register.
>>> + * @cpu: CPU from which to read register.
>>> + * @min_perf: Return address.
>>> + */
>>> +int cppc_get_min_perf(int cpu, u64 *min_perf)
>>> +{
>>> +     return cppc_get_reg_val(cpu, MIN_PERF, min_perf);
>>> +}
>>> +EXPORT_SYMBOL_GPL(cppc_get_min_perf);
>>> +
>>> +/**
>>> + * cppc_set_min_perf - Write minimum performance register.
>>> + * @cpu: CPU to which to write register.
>>> + * @min_perf: the desired minimum performance value to be updated.
>>> + */
>>> +int cppc_set_min_perf(int cpu, u32 min_perf)
>>> +{
>>> +     return cppc_set_reg_val(cpu, MIN_PERF, min_perf);
>>> +}
>>> +EXPORT_SYMBOL_GPL(cppc_set_min_perf);
>>> +
>>> +/**
>>> + * cppc_get_max_perf - Read maximum performance register.
>>> + * @cpu: CPU from which to read register.
>>> + * @max_perf: Return address.
>>> + */
>>> +int cppc_get_max_perf(int cpu, u64 *max_perf)
>>> +{
>>> +     return cppc_get_reg_val(cpu, MAX_PERF, max_perf);
>>> +}
>>> +EXPORT_SYMBOL_GPL(cppc_get_max_perf);
>>> +
>>> +/**
>>> + * cppc_set_max_perf - Write maximum performance register.
>>> + * @cpu: CPU to which to write register.
>>> + * @max_perf: the desired maximum performance value to be updated.
>>> + */
>>> +int cppc_set_max_perf(int cpu, u32 max_perf)
>>> +{
>>> +     return cppc_set_reg_val(cpu, MAX_PERF, max_perf);
>>> +}
>>> +EXPORT_SYMBOL_GPL(cppc_set_max_perf);
>>> +
>>>   /**
>>>    * cppc_set_enable - Set to enable CPPC on the processor by 
>>> writing the
>>>    * Continuous Performance Control package EnableRegister field.
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c 
>>> b/drivers/cpufreq/cppc_cpufreq.c
>>> index 229880c4eedb..66e183b45fb0 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -28,6 +28,8 @@
>>>
>>>   static struct cpufreq_driver cppc_cpufreq_driver;
>>>
>>> +static DEFINE_MUTEX(cppc_cpufreq_autonomous_lock);
>>> +
>>
>> Shouldn't concurrent access be handled by the policy->rwsem ?
>>
>> I think this can be checked using either:
>> - lockdep_assert_held_write(&policy->rwsem)
>> - lockdep_assert_held_read(&policy->rwsem)
>>
>> in store/show_max_perf() for instance.
>>
>
> You're right. The cpufreq sysfs already holds policy->rwsem for
> show/store callbacks. I'll remove the mutex and add lockdep
> assertions for the expected locking.

I think it's ok not to have lockde assertions.

It seems that it is a common assumption sysfs files cannot be modified

concurrently. None of the cpufreq driver seems to use lockdep assertion.


> --------
> File: drivers/cpufreq/cpufreq.c
>   static ssize_t store(struct kobject *kobj, struct attribute *attr,
>                      const char *buf, size_t count)
>   {
>       struct cpufreq_policy *policy = to_policy(kobj);
>       ....
>       guard(cpufreq_policy_write)(policy);
> --------
>
> Thank you,
> Sumit Gupta
>