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.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.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;
+
+	/* 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();
+	if (!ret)
+		return 0;
 
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 	cpufreq_driver->boost_enabled = !state;
-- 
2.33.0

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.
> 
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.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;
> +
> +	/* Return -EINVAL if no policy is boost supported. */

Remove the comment and blank line.

> +	int ret = -EINVAL;

EOPNOTSUPP instead ?

-- 
viresh

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@huawei.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

On 2025/11/26 14:29, Viresh Kumar wrote:
> 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.

Yes. So I think return an error is more reasonable when try to 'echo 1 >
boost' in this situation.

> Also, cpufreq_table_validate_and_sort()
> sets boost supported only if at least one policy supports it.

Sorry, I don't see any connection to cpufreq_table_validate_and_sort().

> 
> 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.

Make sense. But it is also confusing when setting 1 to global boost success
but the platform doesn't support boost at all. It seems like there is no
way to distinguish between these two scenarios: the platform does not
support turbo or the turbo-supporting cores are not yet online.

> 
> Having said that, there are a few things I would like to point.
> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.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.

OK. I'll split that to another patch in the next version. Thanks!

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

On 28-11-25, 12:02, zhenglifeng (A) wrote:
> On 2025/11/26 14:29, Viresh Kumar wrote:
> > 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.

Drivers like cpufreq-dt actually set the boost callback unconditionally, which
can lead to the case you mentioned. None of the policies support boost, but it
is configurable.

> Yes. So I think return an error is more reasonable when try to 'echo 1 >
> boost' in this situation.

I am inclining towards this now.

> > Also, cpufreq_table_validate_and_sort()
> > sets boost supported only if at least one policy supports it.
> 
> Sorry, I don't see any connection to cpufreq_table_validate_and_sort().

Yeah, I misread, we are only configuring policy's boost flag there, not
driver's.

-- 
viresh