Currently, cpufreq_frequency_table_verify() simply returns when the driver’s
frequency table is missing (policy->freq_table == NULL).
This means that cpufreq_verify_within_cpu_limits() is not invoked in such
cases, leaving policy->min and policy->max unchecked.

Some cpufreq drivers handle this manually by calling
cpufreq_verify_within_cpu_limits() even when no frequency table is present,
in order to ensure the policy stays within CPU limits.

To avoid this inconsistency and potential misuse, make
cpufreq_generic_frequency_table_verify() always call
cpufreq_verify_within_cpu_limits(), regardless of whether policy->freq_table
is available. This unifies the behavior across all drivers and makes the helper
safe to use universally.

Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 drivers/cpufreq/freq_table.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index d5111ee56e38..f4b05dcc479b 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -105,6 +105,7 @@ EXPORT_SYMBOL_GPL(cpufreq_frequency_table_verify);
  */
 int cpufreq_generic_frequency_table_verify(struct cpufreq_policy_data *policy)
 {
+	cpufreq_verify_within_cpu_limits(policy);
 	if (!policy->freq_table)
 		return -ENODEV;
 
-- 
2.25.1

On 04-09-25, 11:22, Zihuan Zhang wrote:
> Currently, cpufreq_frequency_table_verify() simply returns when the driver’s
> frequency table is missing (policy->freq_table == NULL).
> This means that cpufreq_verify_within_cpu_limits() is not invoked in such
> cases, leaving policy->min and policy->max unchecked.
> 
> Some cpufreq drivers handle this manually by calling
> cpufreq_verify_within_cpu_limits() even when no frequency table is present,
> in order to ensure the policy stays within CPU limits.
> 
> To avoid this inconsistency and potential misuse, make
> cpufreq_generic_frequency_table_verify() always call
> cpufreq_verify_within_cpu_limits(), regardless of whether policy->freq_table
> is available. This unifies the behavior across all drivers and makes the helper
> safe to use universally.
> 
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> ---
>  drivers/cpufreq/freq_table.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index d5111ee56e38..f4b05dcc479b 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -105,6 +105,7 @@ EXPORT_SYMBOL_GPL(cpufreq_frequency_table_verify);
>   */
>  int cpufreq_generic_frequency_table_verify(struct cpufreq_policy_data *policy)
>  {
> +	cpufreq_verify_within_cpu_limits(policy);

So if we have a freq-table, we will call this twice now. Why make it
bad for the existing users ?

And then the name of this function, it is all about freq-table. If it
isn't there, not sure we should call it at all.

>  	if (!policy->freq_table)
>  		return -ENODEV;
>  
> -- 
> 2.25.1

-- 
viresh

在 2025/9/4 12:48, Viresh Kumar 写道:
> On 04-09-25, 11:22, Zihuan Zhang wrote:
>> Currently, cpufreq_frequency_table_verify() simply returns when the driver’s
>> frequency table is missing (policy->freq_table == NULL).
>> This means that cpufreq_verify_within_cpu_limits() is not invoked in such
>> cases, leaving policy->min and policy->max unchecked.
>>
>> Some cpufreq drivers handle this manually by calling
>> cpufreq_verify_within_cpu_limits() even when no frequency table is present,
>> in order to ensure the policy stays within CPU limits.
>>
>> To avoid this inconsistency and potential misuse, make
>> cpufreq_generic_frequency_table_verify() always call
>> cpufreq_verify_within_cpu_limits(), regardless of whether policy->freq_table
>> is available. This unifies the behavior across all drivers and makes the helper
>> safe to use universally.
>>
>> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
>> ---
>>   drivers/cpufreq/freq_table.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
>> index d5111ee56e38..f4b05dcc479b 100644
>> --- a/drivers/cpufreq/freq_table.c
>> +++ b/drivers/cpufreq/freq_table.c
>> @@ -105,6 +105,7 @@ EXPORT_SYMBOL_GPL(cpufreq_frequency_table_verify);
>>    */
>>   int cpufreq_generic_frequency_table_verify(struct cpufreq_policy_data *policy)
>>   {
>> +	cpufreq_verify_within_cpu_limits(policy);
> So if we have a freq-table, we will call this twice now. Why make it
> bad for the existing users ?


Just to clarify, in the third patch of this series we remove
cpufreq_generic_frequency_table_verify() from the table_verify path,
so cpufreq_verify_within_cpu_limits() is now only called here. There
won’t be any duplicate invocation for drivers that already have a
frequency table.

This also resolves the semantic concern, since the helper is no
longer invoked outside of this context.

Thanks!


> And then the name of this function, it is all about freq-table. If it
> isn't there, not sure we should call it at all.


As it stands, only drivers that have a frequency table actually call
cpufreq_generic_frequency_table_verify(). Drivers without a table
implement their own verification logic. So in practice, this helper
is still only used in the context of a frequency table, keeping the
semantics consistent with its name.

>>   	if (!policy->freq_table)
>>   		return -ENODEV;
>>   
>> -- 
>> 2.25.1

On 04-09-25, 13:23, Zihuan Zhang wrote:
> 在 2025/9/4 12:48, Viresh Kumar 写道:
> > On 04-09-25, 11:22, Zihuan Zhang wrote:
> > >   int cpufreq_generic_frequency_table_verify(struct cpufreq_policy_data *policy)
> > >   {
> > > +	cpufreq_verify_within_cpu_limits(policy);
> > So if we have a freq-table, we will call this twice now. Why make it
> > bad for the existing users ?
> 
> 
> Just to clarify, in the third patch of this series we remove
> cpufreq_generic_frequency_table_verify() from the table_verify path,
> so cpufreq_verify_within_cpu_limits() is now only called here. There
> won’t be any duplicate invocation for drivers that already have a
> frequency table.

Maybe I wasn't clear enough.

int cpufreq_frequency_table_verify(...)
{
	cpufreq_verify_within_cpu_limits(...);
        ...
}

int cpufreq_generic_frequency_table_verify(...)
{
        cpufreq_verify_within_cpu_limits(...);
        cpufreq_frequency_table_verify(...);
        ...
}

For a driver with a valid freq-table, we will call
cpufreq_verify_within_cpu_limits() unnecessarily, isn't it ?

-- 
viresh

在 2025/9/4 13:37, Viresh Kumar 写道:
> On 04-09-25, 13:23, Zihuan Zhang wrote:
>> 在 2025/9/4 12:48, Viresh Kumar 写道:
>>> On 04-09-25, 11:22, Zihuan Zhang wrote:
>>>>    int cpufreq_generic_frequency_table_verify(struct cpufreq_policy_data *policy)
>>>>    {
>>>> +	cpufreq_verify_within_cpu_limits(policy);
>>> So if we have a freq-table, we will call this twice now. Why make it
>>> bad for the existing users ?
>>
>> Just to clarify, in the third patch of this series we remove
>> cpufreq_generic_frequency_table_verify() from the table_verify path,
>> so cpufreq_verify_within_cpu_limits() is now only called here. There
>> won’t be any duplicate invocation for drivers that already have a
>> frequency table.
> Maybe I wasn't clear enough.
>
> int cpufreq_frequency_table_verify(...)
> {
> 	cpufreq_verify_within_cpu_limits(...);
>          ...
> }
>
> int cpufreq_generic_frequency_table_verify(...)
> {
>          cpufreq_verify_within_cpu_limits(...);
>          cpufreq_frequency_table_verify(...);
>          ...
> }
>
> For a driver with a valid freq-table, we will call
> cpufreq_verify_within_cpu_limits() unnecessarily, isn't it ?

I understand your point about the potential duplicate call to
cpufreq_verify_within_cpu_limits() for drivers with a valid freq-table.
However, in the third patch of this series, we removed the call to
cpufreq_generic_frequency_table_verify() from the table_verify path.


In the Third patch:

-int cpufreq_frequency_table_verify(struct cpufreq_policy_data *policy)
+static int cpufreq_frequency_table_verify(struct cpufreq_policy_data *policy)
  {
  	struct cpufreq_frequency_table *pos, *table = policy->freq_table;
  	unsigned int freq, prev_smaller = 0;
@@ -73,8 +73,6 @@ int cpufreq_frequency_table_verify(struct cpufreq_policy_data *policy)
  	pr_debug("request for verification of policy (%u - %u kHz) for cpu %u\n",
  					policy->min, policy->max, policy->cpu);
  
-	cpufreq_verify_within_cpu_limits(policy);
-
  	cpufreq_for_each_valid_entry(pos, table) {


Now, the verification and the CPU limits check are unified in the
generic function, so there is no longer a redundant invocation for
drivers with a frequency table.

Thanks!

On 04-09-25, 13:48, Zihuan Zhang wrote:
> I understand your point about the potential duplicate call to
> cpufreq_verify_within_cpu_limits() for drivers with a valid freq-table.
> However, in the third patch of this series, we removed the call to
> cpufreq_generic_frequency_table_verify() from the table_verify path.

Yeah, I missed that.

-- 
viresh

在 2025/9/8 14:13, Viresh Kumar 写道:
> On 04-09-25, 13:48, Zihuan Zhang wrote:
>> I understand your point about the potential duplicate call to
>> cpufreq_verify_within_cpu_limits() for drivers with a valid freq-table.
>> However, in the third patch of this series, we removed the call to
>> cpufreq_generic_frequency_table_verify() from the table_verify path.
> Yeah, I missed that.
>

We are currently considering moving the check that ensures a driver 
providing a freq_table also implements target_index() into the driver 
registration path.

This way, freq_table.c no longer needs to defensively check for NULL 
pointers.

Additionally, we are thinking about merging the two related APIs into a 
single one. Do you think this is a good idea?

On 08-09-25, 14:51, Zihuan Zhang wrote:
> We are currently considering moving the check that ensures a driver
> providing a freq_table also implements target_index() into the driver
> registration path.

That won't work AFAIU. The freq table is initialized during
policy->init and that's not done at the time of registration.

> This way, freq_table.c no longer needs to defensively check for NULL
> pointers.
> 
> Additionally, we are thinking about merging the two related APIs into a
> single one. Do you think this is a good idea?

Which ones ? target/target_index ? I am not sure if that can be done.

We are fine with improvements generally, but please make sure whatever
you send doesn't break existing users. That will help saving some of
our review time.

-- 
viresh

在 2025/9/8 14:55, Viresh Kumar 写道:
> On 08-09-25, 14:51, Zihuan Zhang wrote:
>> We are currently considering moving the check that ensures a driver
>> providing a freq_table also implements target_index() into the driver
>> registration path.
> That won't work AFAIU. The freq table is initialized during
> policy->init and that's not done at the time of registration.

One idea we are considering is to check whether driver->verify points to 
cpufreq_generic_frequency_table_verify and use that as a heuristic to 
enforce the presence of target_index():

  ((driver_data->verify == cpufreq_generic_frequency_table_verify) != 
!!driver_data->target_index)

I haven’t tested this approach yet, so I’m not sure if it will be fully 
reliable.


>> This way, freq_table.c no longer needs to defensively check for NULL
>> pointers.
>>
>> Additionally, we are thinking about merging the two related APIs into a
>> single one. Do you think this is a good idea?
> Which ones ? target/target_index ? I am not sure if that can be done.

If this approach 
works,  cpufreq_generic_frequency_table_verify and cpufreq_frequency_table_verify 
should be the same because we dont't need to check pointer is NULL  in 
freq_table.c.

> We are fine with improvements generally, but please make sure whatever
> you send doesn't break existing users. That will help saving some of
> our review time.

On 08-09-25, 15:08, Zihuan Zhang wrote:
> One idea we are considering is to check whether driver->verify points to
> cpufreq_generic_frequency_table_verify and use that as a heuristic to
> enforce the presence of target_index():
> 
>  ((driver_data->verify == cpufreq_generic_frequency_table_verify) !=
> !!driver_data->target_index)
> 
> I haven’t tested this approach yet, so I’m not sure if it will be fully
> reliable.

I don't this is a good idea. It isn't necessary for any driver to use
the generic functions.

-- 
viresh

在 2025/9/8 15:19, Viresh Kumar 写道:
> On 08-09-25, 15:08, Zihuan Zhang wrote:
>> One idea we are considering is to check whether driver->verify points to
>> cpufreq_generic_frequency_table_verify and use that as a heuristic to
>> enforce the presence of target_index():
>>
>>   ((driver_data->verify == cpufreq_generic_frequency_table_verify) !=
>> !!driver_data->target_index)
>>
>> I haven’t tested this approach yet, so I’m not sure if it will be fully
>> reliable.
> I don't this is a good idea. It isn't necessary for any driver to use
> the generic functions.
>
Understood, I thinks there is some reason that the two separate 
verification functions exist.

By the way, Do you think it’s necessary to add some defensive checks 
during driver registration?

For instance, we could enforce that a driver cannot implement both 
has_target and has_target_index at the same time.

On 08-09-25, 15:36, Zihuan Zhang wrote:
> For instance, we could enforce that a driver cannot implement both
> has_target and has_target_index at the same time.

Can be added.

-- 
viresh