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

[Sumit Gupta <sumitg@nvidia.com>]

>>> 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.
>

Sure. I will just remove the mutex, keeping it consistent with other 
cpufreq drivers.

Thank you,
Sumit Gupta


>
>> --------
>> 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
>>