Re: [PATCH 12/14] cpufreq/amd-pstate: Cache a pointer to policy in cpudata

[Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>]

On 2/12/2025 12:47 AM, Mario Limonciello wrote:
> On 2/11/2025 07:13, Dhananjay Ugwekar wrote:
>> On 2/7/2025 3:26 AM, Mario Limonciello wrote:
>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>
>>> In order to access the policy from a notification block it will
>>> need to be stored in cpudata.
>>
>> This might break the cpufreq_policy ref counting right?, if we cache the pointer
>> and use it independent of the ref counting framework.
> 
> Would it be reasonable to bump the ref count when we take the pointer?
> 
> I'm not sure if this will work properly.

One doubt, why cant we get the policy ref normally using the cpufreq_cpu_get(cpudata->cpu) 
in the notification block ? I'm not aware of that code, if there is any restriction on this.

> 
>>
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/cpufreq/amd-pstate.c | 13 +++++++------
>>>   drivers/cpufreq/amd-pstate.h |  3 ++-
>>>   2 files changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>>> index 689de385d06da..5945b6c7f7e56 100644
>>> --- a/drivers/cpufreq/amd-pstate.c
>>> +++ b/drivers/cpufreq/amd-pstate.c
>>> @@ -388,7 +388,7 @@ static int amd_pstate_set_energy_pref_index(struct cpufreq_policy *policy,
>>>       else
>>>           epp = epp_values[pref_index];
>>>   -    if (epp > 0 && cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
>>> +    if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) {
>>>           pr_debug("EPP cannot be set under performance policy\n");
>>>           return -EBUSY;
>>>       }
>>> @@ -689,7 +689,7 @@ static void amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
>>>       perf.max_limit_perf = freq_to_perf(perf, cpudata->nominal_freq, policy->max);
>>>       perf.min_limit_perf = freq_to_perf(perf, cpudata->nominal_freq, policy->min);
>>>   -    if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>>> +    if (policy->policy == CPUFREQ_POLICY_PERFORMANCE)
>>>           perf.min_limit_perf = min(perf.nominal_perf, perf.max_limit_perf);
>>>         WRITE_ONCE(cpudata->perf, perf);
>>> @@ -1042,6 +1042,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>>>           return -ENOMEM;
>>>         cpudata->cpu = policy->cpu;
>>> +    cpudata->policy = policy;
>>>         mutex_init(&cpudata->lock);
>>>       guard(mutex)(&cpudata->lock);
>>> @@ -1224,9 +1225,8 @@ static ssize_t show_energy_performance_available_preferences(
>>>   {
>>>       int i = 0;
>>>       int offset = 0;
>>> -    struct amd_cpudata *cpudata = policy->driver_data;
>>>   -    if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>>> +    if (policy->policy == CPUFREQ_POLICY_PERFORMANCE)
>>>           return sysfs_emit_at(buf, offset, "%s\n",
>>>                   energy_perf_strings[EPP_INDEX_PERFORMANCE]);
>>>   @@ -1543,6 +1543,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>>>           return -ENOMEM;
>>>         cpudata->cpu = policy->cpu;
>>> +    cpudata->policy = policy;
>>>         mutex_init(&cpudata->lock);
>>>       guard(mutex)(&cpudata->lock);
>>> @@ -1632,7 +1633,7 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>>>         amd_pstate_update_min_max_limit(policy);
>>>   -    if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>>> +    if (policy->policy == CPUFREQ_POLICY_PERFORMANCE)
>>>           epp = 0;
>>>       else
>>>           epp = READ_ONCE(cpudata->epp_cached);
>>> @@ -1651,7 +1652,7 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>>>       if (!policy->cpuinfo.max_freq)
>>>           return -ENODEV;
>>>   -    cpudata->policy = policy->policy;
>>> +    cpudata->policy = policy;
>>>         ret = amd_pstate_epp_update_limit(policy);
>>>       if (ret)
>>> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
>>> index 7501d30db9953..16ce631a6c3d5 100644
>>> --- a/drivers/cpufreq/amd-pstate.h
>>> +++ b/drivers/cpufreq/amd-pstate.h
>>> @@ -97,9 +97,10 @@ struct amd_cpudata {
>>>         struct mutex    lock;
>>>   +    struct cpufreq_policy *policy;
>>> +
>>>       /* EPP feature related attributes*/
>>>       u8    epp_cached;
>>> -    u32    policy;
>>>       bool    suspended;
>>>       u8    epp_default;
>>>   };
>>
>