| Message ID | 20240515022037.818078-1-chizhiling@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: add a check for unsupported CPU frequencies | expand |
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.
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 --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) {