diff mbox series

cpufreq: add a check for unsupported CPU frequencies

Message ID 20240515022037.818078-1-chizhiling@163.com
State New
Headers show
Series cpufreq: add a check for unsupported CPU frequencies | expand

Commit Message

Chi Zhiling May 15, 2024, 2:20 a.m. UTC
From: Chi Zhiling <chizhiling@kylinos.cn>

When user wants to control the CPU frequency on their own,
if they write an unsupported frequency to the
scaling_min_freq/scaling_max_freq node, the execution will not report an
error, which will make the user think that the execution is successful.

So, this patch add a check to return an error if an unsupported frequency
is written.

Testing:
CPU supported frequency [min, max] = [800000, 4600000]

before patch:
root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
root:

after patch:
root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
-bash: echo: Invalid argument
root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
-bash: echo: Invalid argument
root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
-bash: echo: Invalid argument
root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
-bash: echo: Invalid argument
root:

Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
---
 drivers/cpufreq/freq_table.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Viresh Kumar May 20, 2024, 7:55 a.m. UTC | #1
On 15-05-24, 10:20, chizhiling@163.com wrote:
> From: Chi Zhiling <chizhiling@kylinos.cn>
> 
> When user wants to control the CPU frequency on their own,
> if they write an unsupported frequency to the
> scaling_min_freq/scaling_max_freq node, the execution will not report an
> error, which will make the user think that the execution is successful.
> 
> So, this patch add a check to return an error if an unsupported frequency
> is written.
> 
> Testing:
> CPU supported frequency [min, max] = [800000, 4600000]
> 
> before patch:
> root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
> root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
> root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
> root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
> root:
> 
> after patch:
> root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
> -bash: echo: Invalid argument
> root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
> -bash: echo: Invalid argument
> root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
> -bash: echo: Invalid argument
> root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
> -bash: echo: Invalid argument
> root:
> 
> Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
> ---
>  drivers/cpufreq/freq_table.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index 10e80d912b8d..416826d582a4 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -76,6 +76,11 @@ 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);
>  
> +	if (policy->min > policy->max ||
> +	    policy->max > policy->cpuinfo.max_freq ||
> +	    policy->min < policy->cpuinfo.min_freq)
> +		return -EINVAL;
> +

I think the current behavior (of not reporting errors) is what we
really wanted and that's why it is written that way. The kernel
doesn't want to enforce any min/max that the user can set, the kernel
will just get it in line with the current hardware limits.

For example consider this case for a platform with following frequency
range, 1 ghz, 1.1 ghz, 1.2 ghz, 1.3 ghz (boost only).

Lets say boost is disabled, at this point cpuinfo.max_freq is 1.2 ghz.
The user does this:

root: echo 1300000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq

With your change this will fail, but we really want to record this
into policy->max. As the user can enable the boost frequency now,
which will make cpuinfo.max_freq to 1.3 ghz and user isn't required to
set scaling_max_freq again.
Chi Zhiling May 21, 2024, 6:18 a.m. UTC | #2
On 20 May 2024 13:25:58, Viresh Kumar wrote:
> On 15-05-24, 10:20, chizhiling@163.com wrote:
> > From: Chi Zhiling <chizhiling@kylinos.cn>
> > 
> > When user wants to control the CPU frequency on their own,
> > if they write an unsupported frequency to the
> > scaling_min_freq/scaling_max_freq node, the execution will not report an
> > error, which will make the user think that the execution is successful.
> > 
> > So, this patch add a check to return an error if an unsupported frequency
> > is written.
> > 
> > Testing:
> > CPU supported frequency [min, max] = [800000, 4600000]
> > 
> > before patch:
> > root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
> > root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
> > root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
> > root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
> > root:
> > 
> > after patch:
> > root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
> > -bash: echo: Invalid argument
> > root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_min_freq
> > -bash: echo: Invalid argument
> > root: echo 0 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
> > -bash: echo: Invalid argument
> > root: echo 5000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
> > -bash: echo: Invalid argument
> > root:
> > 
> > Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
> > ---
> >  drivers/cpufreq/freq_table.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> > index 10e80d912b8d..416826d582a4 100644
> > --- a/drivers/cpufreq/freq_table.c
> > +++ b/drivers/cpufreq/freq_table.c
> > @@ -76,6 +76,11 @@ 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);
> >  
> > +	if (policy->min > policy->max ||
> > +	    policy->max > policy->cpuinfo.max_freq ||
> > +	    policy->min < policy->cpuinfo.min_freq)
> > +		return -EINVAL;
> > +
> 
> I think the current behavior (of not reporting errors) is what we
> really wanted and that's why it is written that way. The kernel
> doesn't want to enforce any min/max that the user can set, the kernel
> will just get it in line with the current hardware limits.
> 
> For example consider this case for a platform with following frequency
> range, 1 ghz, 1.1 ghz, 1.2 ghz, 1.3 ghz (boost only).
> 
> Lets say boost is disabled, at this point cpuinfo.max_freq is 1.2 ghz.
> The user does this:
> 
> root: echo 1300000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> 
> With your change this will fail, but we really want to record this
> into policy->max. As the user can enable the boost frequency now,
> which will make cpuinfo.max_freq to 1.3 ghz and user isn't required to
> set scaling_max_freq again.


You explained it very clearly, thank you for your reply.
diff mbox series

Patch

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 10e80d912b8d..416826d582a4 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -76,6 +76,11 @@  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);
 
+	if (policy->min > policy->max ||
+	    policy->max > policy->cpuinfo.max_freq ||
+	    policy->min < policy->cpuinfo.min_freq)
+		return -EINVAL;
+
 	cpufreq_verify_within_cpu_limits(policy);
 
 	cpufreq_for_each_valid_entry(pos, table) {