Re: [PATCH] cpufreq: Correct a freq check in cpufreq_set_policy

[Pan Xinhui]

hi, Viresh
	thanks for your quick reply! :)
On 2015å07æ28æ 12:41, Viresh Kumar wrote:
> On 28-07-15, 11:34, Pan Xinhui wrote:
>> From: Pan Xinhui <xinhuix.pan@xxxxxxxxx>
>>
>> This check was originally added by commit 9c9a43ed2734 ("[CPUFREQ]
>> return error when failing to set minfreq").It attempt to return an error
>> on obviously incorrect limits when we echo xxx >.../scaling_max,min_freq
>> Actually we just need check if new_policy->min > new_policy->max.
>> Because at least one of max/min is copied from cpufreq_get_policy().
>>
>> For example, when we echo xxx > .../scaling_min_freq, new_policy is
>> copied from policy in cpufreq_get_policy. new_policy->max is same with
>> policy->max. new_policy->min is set to a new value.
>>
>> Let me explain it in deduction method, first statment in if ():
>> new_policy->min > policy->max
>> policy->max == new_policy->max
>> ==> new_policy->min > new_policy->max
>>
>> second statment in if():
>> new_policy->max < policy->min
>> policy->max < policy->min
>> ==>new_policy->min > new_policy->max (induction method)
>>
>> So we have proved that we only need check if new_policy->min >
>> new_policy->max.
>>
>> After apply this patch, we can also modify ->min and ->max in same time
>> if new freq range is very much different from current freq range. For
>> example, if current freq range is 480000-960000, then we want to set
>> this range to 1120000-2240000, we would fail in the past because
>> new_policy->min > policy->max. As long as the cpufreq range is valid, we
>> has no reason to reject the user. So correct the check.
>>
>> Signed-off-by: Pan Xinhui <xinhuix.pan@xxxxxxxxx>
> 
> Does this patch depend on the other patch you sent where you are
> trying to update both min/max in the same call to
> cpufreq_set_policy()? If so, they should have been part of the same
> series in proper order, as you have sent them as separate patches.
>

Thanks for pointing out my mistakes. I will send them in a same series with proper order.
Sorry for that.
 
> Now, if we don't consider your first patch at all, then this patch is
> obviously wrong. We need to take care of both the checks.
> 
Agree, we need take care of every checks. BUT, As We have proved, it's equal to check if (new_policy->min > new_policy->max). I don't why it's wrong.
with/without this patch, echo 0 > .../scaling_min_freq has no error. min freq is just set to the limit min freq. I prefer to treat it as a feature. :)
So I don't add new_policy->min < policy->cpuinfo.min_freq || new_policy->max > policy->cpuinfo.max_freq.

We have ->verify callback, no need to worry about that an out-of-limit cpufreq will harm kernel.
This check is just to tell userspace that *the cpufreq you are trying to set is wrong, pls double check.*

thanks
xinhui
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/