[04/14] cpufreq/amd-pstate: Overhaul locking

From: Mario Limonciello <mario.limonciello@amd.com>

amd_pstate_cpu_boost_update() and refresh_frequency_limits() both
update the policy state and have nothing to do with the amd-pstate
driver itself.

A global "limits" lock doesn't make sense because each CPU can have
policies changed independently.  Instead introduce locks into to the
cpudata structure and lock each CPU independently.

The remaining "global" driver lock is used to ensure that only one
entity can change driver modes at a given time.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 27 +++++++++++++++++----------
 drivers/cpufreq/amd-pstate.h |  2 ++
 2 files changed, 19 insertions(+), 10 deletions(-)

On 2/11/2025 23:15, Dhananjay Ugwekar wrote:
> On 2/12/2025 3:24 AM, Mario Limonciello wrote:
>> On 2/10/2025 23:02, Dhananjay Ugwekar wrote:
>>> On 2/7/2025 3:26 AM, Mario Limonciello wrote:
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> amd_pstate_cpu_boost_update() and refresh_frequency_limits() both
>>>> update the policy state and have nothing to do with the amd-pstate
>>>> driver itself.
>>>>
>>>> A global "limits" lock doesn't make sense because each CPU can have
>>>> policies changed independently.  Instead introduce locks into to the
>>>> cpudata structure and lock each CPU independently.
>>>>
>>>> The remaining "global" driver lock is used to ensure that only one
>>>> entity can change driver modes at a given time.
>>>>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/cpufreq/amd-pstate.c | 27 +++++++++++++++++----------
>>>>    drivers/cpufreq/amd-pstate.h |  2 ++
>>>>    2 files changed, 19 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>>>> index 77bc6418731ee..dd230ed3b9579 100644
>>>> --- a/drivers/cpufreq/amd-pstate.c
>>>> +++ b/drivers/cpufreq/amd-pstate.c
>>>> @@ -196,7 +196,6 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
>>>>        return -EINVAL;
>>>>    }
>>>>    -static DEFINE_MUTEX(amd_pstate_limits_lock);
>>>>    static DEFINE_MUTEX(amd_pstate_driver_lock);
>>>>      static u8 msr_get_epp(struct amd_cpudata *cpudata)
>>>> @@ -283,6 +282,8 @@ static int msr_set_epp(struct amd_cpudata *cpudata, u8 epp)
>>>>        u64 value, prev;
>>>>        int ret;
>>>>    +    lockdep_assert_held(&cpudata->lock);
>>>
>>> After making the perf_cached variable writes atomic, do we still need a cpudata->lock ?
>>
>> My concern was specifically that userspace could interact with multiple sysfs files that influence the atomic perf variable (and the HW) at the same time.  So you would not have a deterministic behavior if they raced.  But if you take the mutex on all the paths that this could happen it will be a FIFO.
> 
> I guess, the lock still wont guarantee the ordering right? It will just ensure that one thread executes
> that code path for a specific CPU at a time. And do we even care about the ordering ? I'm having a hard
> time thinking of a scenario where we'll need the lock. Can you or Gautham think of any such scenario?
> 

You're right; I can't really think of one either.  Let me take out the 
private lock for the per-cpu device and I'll just overhaul the global 
lock locations.

>>
>>>
>>> Regards,
>>> Dhananjay
>>>
>>>> +
>>>>        value = prev = READ_ONCE(cpudata->cppc_req_cached);
>>>>        value &= ~AMD_CPPC_EPP_PERF_MASK;
>>>>        value |= FIELD_PREP(AMD_CPPC_EPP_PERF_MASK, epp);
>>>> @@ -315,6 +316,8 @@ static int shmem_set_epp(struct amd_cpudata *cpudata, u8 epp)
>>>>        int ret;
>>>>        struct cppc_perf_ctrls perf_ctrls;
>>>>    +    lockdep_assert_held(&cpudata->lock);
>>>> +
>>>>        if (epp == cpudata->epp_cached)
>>>>            return 0;
>>>>    @@ -335,6 +338,8 @@ static int amd_pstate_set_energy_pref_index(struct cpufreq_policy *policy,
>>>>        struct amd_cpudata *cpudata = policy->driver_data;
>>>>        u8 epp;
>>>>    +    guard(mutex)(&cpudata->lock);
>>>> +
>>>>        if (!pref_index)
>>>>            epp = cpudata->epp_default;
>>>>        else
>>>> @@ -750,7 +755,6 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state)
>>>>            pr_err("Boost mode is not supported by this processor or SBIOS\n");
>>>>            return -EOPNOTSUPP;
>>>>        }
>>>> -    guard(mutex)(&amd_pstate_driver_lock);
>>>>          ret = amd_pstate_cpu_boost_update(policy, state);
>>>>        refresh_frequency_limits(policy);
>>>> @@ -973,6 +977,9 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>>>>          cpudata->cpu = policy->cpu;
>>>>    +    mutex_init(&cpudata->lock);
>>>> +    guard(mutex)(&cpudata->lock);
>>>> +
>>>>        ret = amd_pstate_init_perf(cpudata);
>>>>        if (ret)
>>>>            goto free_cpudata1;
>>>> @@ -1179,8 +1186,6 @@ static ssize_t store_energy_performance_preference(
>>>>        if (ret < 0)
>>>>            return -EINVAL;
>>>>    -    guard(mutex)(&amd_pstate_limits_lock);
>>>> -
>>>>        ret = amd_pstate_set_energy_pref_index(policy, ret);
>>>>          return ret ? ret : count;
>>>> @@ -1353,8 +1358,10 @@ int amd_pstate_update_status(const char *buf, size_t size)
>>>>        if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX)
>>>>            return -EINVAL;
>>>>    -    if (mode_state_machine[cppc_state][mode_idx])
>>>> +    if (mode_state_machine[cppc_state][mode_idx]) {
>>>> +        guard(mutex)(&amd_pstate_driver_lock);
>>>>            return mode_state_machine[cppc_state][mode_idx](mode_idx);
>>>> +    }
>>>>          return 0;
>>>>    }
>>>> @@ -1375,7 +1382,6 @@ static ssize_t status_store(struct device *a, struct device_attribute *b,
>>>>        char *p = memchr(buf, '\n', count);
>>>>        int ret;
>>>>    -    guard(mutex)(&amd_pstate_driver_lock);
>>>>        ret = amd_pstate_update_status(buf, p ? p - buf : count);
>>>>          return ret < 0 ? ret : count;
>>>> @@ -1472,6 +1478,9 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>>>>          cpudata->cpu = policy->cpu;
>>>>    +    mutex_init(&cpudata->lock);
>>>> +    guard(mutex)(&cpudata->lock);
>>>> +
>>>>        ret = amd_pstate_init_perf(cpudata);
>>>>        if (ret)
>>>>            goto free_cpudata1;
>>>> @@ -1558,6 +1567,8 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>>>>        union perf_cached perf;
>>>>        u8 epp;
>>>>    +    guard(mutex)(&cpudata->lock);
>>>> +
>>>>        amd_pstate_update_min_max_limit(policy);
>>>>          if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>>>> @@ -1646,8 +1657,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>>>>        if (cpudata->suspended)
>>>>            return 0;
>>>>    -    guard(mutex)(&amd_pstate_limits_lock);
>>>> -
>>>>        if (trace_amd_pstate_epp_perf_enabled()) {
>>>>            trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
>>>>                          AMD_CPPC_EPP_BALANCE_POWERSAVE,
>>>> @@ -1684,8 +1693,6 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
>>>>        struct amd_cpudata *cpudata = policy->driver_data;
>>>>          if (cpudata->suspended) {
>>>> -        guard(mutex)(&amd_pstate_limits_lock);
>>>> -
>>>>            /* enable amd pstate from suspend state*/
>>>>            amd_pstate_epp_reenable(policy);
>>>>    diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
>>>> index a140704b97430..6d776c3e5712a 100644
>>>> --- a/drivers/cpufreq/amd-pstate.h
>>>> +++ b/drivers/cpufreq/amd-pstate.h
>>>> @@ -96,6 +96,8 @@ struct amd_cpudata {
>>>>        bool    boost_supported;
>>>>        bool    hw_prefcore;
>>>>    +    struct mutex    lock;
>>>> +
>>>>        /* EPP feature related attributes*/
>>>>        u8    epp_cached;
>>>>        u32    policy;
>>>
>>
>