linux-kernel - Re: [PATCH v4 05/10] PM / devfreq: Use scope-based cleanup helper

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: <20250905110132.00003987@huawei.com>
Date: Fri, 5 Sep 2025 11:01:32 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Zihuan Zhang <zhangzihuan@...inos.cn>
CC: "Rafael J . wysocki" <rafael@...nel.org>, Viresh Kumar
	<viresh.kumar@...aro.org>, Catalin Marinas <catalin.marinas@....com>, "Will
 Deacon" <will@...nel.org>, Borislav Petkov <bp@...en8.de>, Dave Hansen
	<dave.hansen@...ux.intel.com>, Srinivas Pandruvada
	<srinivas.pandruvada@...ux.intel.com>, Michael Ellerman <mpe@...erman.id.au>,
	Krzysztof Kozlowski <krzk@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>,
	Thierry Reding <thierry.reding@...il.com>, MyungJoo Ham
	<myungjoo.ham@...sung.com>, Kyungmin Park <kyungmin.park@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>, "Jani Nikula"
	<jani.nikula@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>, Tvrtko
 Ursulin <tursulin@...ulin.net>, "David Airlie" <airlied@...il.com>, Simona
 Vetter <simona@...ll.ch>, Daniel Lezcano <daniel.lezcano@...nel.org>, Sascha
 Hauer <s.hauer@...gutronix.de>, "Shawn Guo" <shawnguo@...nel.org>, Eduardo
 Valentin <edubezval@...il.com>, Keerthy <j-keerthy@...com>, Ben Horgan
	<ben.horgan@....com>, zhenglifeng <zhenglifeng1@...wei.com>, Zhang Rui
	<rui.zhang@...el.com>, Len Brown <lenb@...nel.org>, Lukasz Luba
	<lukasz.luba@....com>, "Pengutronix Kernel Team" <kernel@...gutronix.de>,
	Beata Michalska <beata.michalska@....com>, Fabio Estevam
	<festevam@...il.com>, Pavel Machek <pavel@...nel.org>, "Sumit Gupta"
	<sumitg@...dia.com>, Prasanna Kumar T S M <ptsm@...ux.microsoft.com>, Sudeep
 Holla <sudeep.holla@....com>, Yicong Yang <yangyicong@...ilicon.com>,
	<linux-pm@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
	<linuxppc-dev@...ts.ozlabs.org>, <linux-arm-kernel@...ts.infradead.org>,
	<intel-gfx@...ts.freedesktop.org>, <dri-devel@...ts.freedesktop.org>,
	<imx@...ts.linux.dev>, <linux-omap@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 05/10] PM / devfreq: Use scope-based cleanup helper

On Wed,  3 Sep 2025 21:17:28 +0800
Zihuan Zhang <zhangzihuan@...inos.cn> wrote:

> Replace the manual cpufreq_cpu_put() with __free(put_cpufreq_policy)
> annotation for policy references. This reduces the risk of reference
> counting mistakes and aligns the code with the latest kernel style.
> 
> No functional change intended.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@...inos.cn>

This falls into the mess of mixing gotos with cleanup.h usage.

The guidance in cleanup.h IIRC say don't do this.  It isn't (I think) buggy here
but it does make things harder to reason about and generally removes
the point of doing __free.  So I think if you are going to do this one
you need to do it fully which is a little more complex.
Need to deal with parent_cpu_data which isn't that hard.

If you mix the two, Linus may get grumpy!

> ---
>  drivers/devfreq/governor_passive.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 953cf9a1e9f7..a035cf44bdb8 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c

> @@ -256,7 +253,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  	struct device *dev = devfreq->dev.parent;
>  	struct opp_table *opp_table = NULL;
>  	struct devfreq_cpu_data *parent_cpu_data;
> -	struct cpufreq_policy *policy;
>  	struct device *cpu_dev;
>  	unsigned int cpu;
>  	int ret;
> @@ -273,23 +269,23 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  	}
>  
>  	for_each_possible_cpu(cpu) {
> -		policy = cpufreq_cpu_get(cpu);
> +		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +			cpufreq_cpu_get(cpu);
> +
>  		if (!policy) {
>  			ret = -EPROBE_DEFER;
>  			goto err;
Return directly here (and after changes below, in all error paths.
>  		}
>  
>  		parent_cpu_data = get_parent_cpu_data(p_data, policy);
> -		if (parent_cpu_data) {
> -			cpufreq_cpu_put(policy);
> +		if (parent_cpu_data)
>  			continue;
This is the first use of parent_cpu_data. If it's set at this point
we don't use it at all.  So step 1. Rename this to split this
use from the one that follows.


> -		}
>  
>  		parent_cpu_data = kzalloc(sizeof(*parent_cpu_data),
>  						GFP_KERNEL);
This one needs to be
		struct devfreq_cpu_data *parent_cpu_data __free(kfree) =
			kzalloc(sizeof(*parent_cpu_data), GFP_KERNEL);

		
>  		if (!parent_cpu_data) {
>  			ret = -ENOMEM;
> -			goto err_put_policy;
> +			goto err;
>  		}
>  
>  		cpu_dev = get_cpu_device(cpu);
> @@ -314,7 +310,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  		parent_cpu_data->max_freq = policy->cpuinfo.max_freq;
>  
>  		list_add_tail(&parent_cpu_data->node, &p_data->cpu_data_list);

then here we need to ensure we don't free parent_cpu_data. Hence

		list_add_tail(&(no_free_ptr(parent_cpu_data)->node,
			      &p_data->cpu_data_list);

That that point we have passed ownership of the data to the list.

> -		cpufreq_cpu_put(policy);
>  	}
>  
>  	mutex_lock(&devfreq->lock);
> @@ -327,8 +322,6 @@ static int cpufreq_passive_register_notifier(struct devfreq *devfreq)
>  
>  err_free_cpu_data:
>  	kfree(parent_cpu_data);
And all this error block goes away.

> -err_put_policy:
> -	cpufreq_cpu_put(policy);
>  err:
>  
>  	return ret;