Message-ID: <55B71050.7070707@intel.com>
Date: Tue, 28 Jul 2015 13:17:04 +0800
From: Pan Xinhui <xinhuix.pan@intel.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0
MIME-Version: 1.0
To: Viresh Kumar <viresh.kumar@linaro.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
        "rjw@rjwysocki.net" <rjw@rjwysocki.net>,
        "mnipxh@163.com" <mnipxh@163.com>,
        "yanmin_zhang@linux.intel.com" <yanmin_zhang@linux.intel.com>
Subject: Re: [PATCH] cpufreq: Correct a freq check in cpufreq_set_policy
References: <55B6F83D.3040901@intel.com> <20150728044137.GF1229@linux>
In-Reply-To: <20150728044137.GF1229@linux>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3023
Lines: 64

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@intel.com>
>>
>> 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@intel.com>
> 
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/