linux-kernel - Re: [PATCH] cpufreq: Return -EINVAL if no policy is boost supported

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] [day] [month] [year] [list]

Message-ID: <wnnfdvp3r3bg5wztazoijei2uji5xypl4b4wlvlxuwxaizu6g7@2xxyrk7kdxhf>
Date: Wed, 26 Nov 2025 11:59:58 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Lifeng Zheng <zhenglifeng1@...wei.com>
Cc: rafael@...nel.org, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linuxarm@...wei.com, jonathan.cameron@...wei.com, 
	zhanjie9@...ilicon.com, lihuisong@...wei.com, yubowen8@...wei.com, 
	zhangpengjie2@...wei.com, wangzhi12@...wei.com, linhongye@...artners.com
Subject: Re: [PATCH] cpufreq: Return -EINVAL if no policy is boost supported

On 26-11-25, 11:19, Lifeng Zheng wrote:
> In cpufreq_boost_trigger_state(), if all the policies are boost
> unsupported, policy_set_boost() will not be called and this function will
> return 0. But it is better to return an error to indicate that the platform
> doesn't support boost.

I am not sure if it is a good idea. If boost isn't supported by any policy then
the driver shouldn't enable it at all. Also, cpufreq_table_validate_and_sort()
sets boost supported only if at least one policy supports it.

We can still have this case where the policy that supports boost is offline, but
we shouldn't be returning error there and confuse userspace.

Having said that, there are a few things I would like to point.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@...wei.com>
> ---
>  drivers/cpufreq/cpufreq.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e8d7544b77b8..2df714b24074 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2806,7 +2806,9 @@ static int cpufreq_boost_trigger_state(int state)
>  {
>  	struct cpufreq_policy *policy;
>  	unsigned long flags;
> -	int ret = 0;

In the current code, `ret` isn't required to be initialized to 0.

> +
> +	/* Return -EINVAL if no policy is boost supported. */
> +	int ret = -EINVAL;
>  
>  	/*
>  	 * Don't compare 'cpufreq_driver->boost_enabled' with 'state' here to
> @@ -2824,14 +2826,12 @@ static int cpufreq_boost_trigger_state(int state)
>  
>  		ret = policy_set_boost(policy, state);
>  		if (ret)
> -			goto err_reset_state;
> +			break;
>  	}
>  	cpus_read_unlock();
>  
> -	return 0;
> -
> -err_reset_state:
> -	cpus_read_unlock();

It was a bad idea to mix two things in a single patch. You should have avoided
optimizing the use of cpus_read_unlock() in this patch.

> +	if (!ret)
> +		return 0;
>  
>  	write_lock_irqsave(&cpufreq_driver_lock, flags);
>  	cpufreq_driver->boost_enabled = !state;
> -- 
> 2.33.0

-- 
viresh