linux-kernel - Re: [PATCH 11/12] cpufreq/amd-pstate: Use scope based cleanup for cpufreq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30e56a47-4201-4f2b-aa6d-0547c0d09c38@amd.com>
Date: Wed, 5 Feb 2025 11:50:38 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Dhananjay Ugwekar <dhananjay.ugwekar@....com>, gautham.shenoy@....com,
 rafael@...nel.org, viresh.kumar@...aro.org
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 11/12] cpufreq/amd-pstate: Use scope based cleanup for
 cpufreq_policy refs

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> There have been instances in past where refcount decrementing is missed
> while exiting a function. Use automatic scope based cleanup to avoid
> such errors.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@....com>

Reviewed-by: Mario Limonciello <mario.limonciello@....com>

> ---
>   drivers/cpufreq/amd-pstate.c | 25 ++++++++-----------------
>   include/linux/cpufreq.h      |  3 +++
>   2 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 6a604f0797d9..ee7e3f0a4c0a 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -548,7 +548,7 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
>   static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>   			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
>   {
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
>   	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>   
>   	if (!policy)
> @@ -574,8 +574,6 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>   	}
>   
>   	amd_pstate_update_perf(cpudata, min_perf, des_perf, max_perf, 0, fast_switch);
> -
> -	cpufreq_cpu_put(policy);
>   }
>   
>   static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
> @@ -587,7 +585,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>   	 * amd-pstate qos_requests.
>   	 */
>   	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE) {
> -		struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
> +		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +					      cpufreq_cpu_get(policy_data->cpu);
>   		struct amd_cpudata *cpudata;
>   
>   		if (!policy)
> @@ -595,7 +594,6 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>   
>   		cpudata = policy->driver_data;
>   		policy_data->min = cpudata->lowest_nonlinear_freq;
> -		cpufreq_cpu_put(policy);
>   	}
>   
>   	cpufreq_verify_within_cpu_limits(policy_data);
> @@ -678,7 +676,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   				   unsigned long capacity)
>   {
>   	u8 max_perf, min_perf, des_perf, cap_perf, min_limit_perf;
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>   	struct amd_cpudata *cpudata;
>   
>   	if (!policy)
> @@ -710,7 +708,6 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   
>   	amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true,
>   			policy->governor->flags);
> -	cpufreq_cpu_put(policy);
>   }
>   
>   static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
> @@ -824,28 +821,23 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>   
>   static void amd_pstate_update_limits(unsigned int cpu)
>   {
> -	struct cpufreq_policy *policy = NULL;
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>   	struct amd_cpudata *cpudata;
>   	u32 prev_high = 0, cur_high = 0;
> -	int ret;
>   	bool highest_perf_changed = false;
>   
>   	if (!amd_pstate_prefcore)
>   		return;
>   
> -	policy = cpufreq_cpu_get(cpu);
>   	if (!policy)
>   		return;
>   
> -	cpudata = policy->driver_data;
> -
>   	guard(mutex)(&amd_pstate_driver_lock);
>   
> -	ret = amd_get_highest_perf(cpu, &cur_high);
> -	if (ret) {
> -		cpufreq_cpu_put(policy);
> +	if (amd_get_highest_perf(cpu, &cur_high))
>   		return;
> -	}
> +
> +	cpudata = policy->driver_data;
>   
>   	prev_high = READ_ONCE(cpudata->prefcore_ranking);
>   	highest_perf_changed = (prev_high != cur_high);
> @@ -855,7 +847,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>   		if (cur_high < CPPC_MAX_PERF)
>   			sched_set_itmt_core_prio((int)cur_high, cpu);
>   	}
> -	cpufreq_cpu_put(policy);
>   }
>   
>   /*
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 7fe0981a7e46..dde5212d256c 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -210,6 +210,9 @@ static inline struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
>   static inline void cpufreq_cpu_put(struct cpufreq_policy *policy) { }
>   #endif
>   
> +/* Scope based cleanup macro for cpufreq_policy kobject reference counting */
> +DEFINE_FREE(put_cpufreq_policy, struct cpufreq_policy *, if (_T) cpufreq_cpu_put(_T))
> +
>   static inline bool policy_is_inactive(struct cpufreq_policy *policy)
>   {
>   	return cpumask_empty(policy->cpus);