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

["zhenglifeng (A)" <zhenglifeng1@huawei.com>]

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
>