[PATCH] cpufreq: Add scaling frequency range support

[PATCH] cpufreq: Add scaling frequency range support

[Pan Xinhui]

From: Pan Xinhui <xinhuix.pan@intel.com>

Userspace at most time do cpufreq tests very much inconveniently.
Currently they have to echo min and max cpu freq separately like below:
echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq

Add scaling_freq_range cpufreq attr to support userspace's demand.
Therefore it's easier for testers to write readable scripts like below: 
echo 480000-2240000 >
/sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
 drivers/cpufreq/cpufreq.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 26063af..6424e05 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -812,6 +812,35 @@ static ssize_t show_bios_limit(struct cpufreq_policy *policy, char *buf)
 	return sprintf(buf, "%u\n", policy->cpuinfo.max_freq);
 }
 
+static ssize_t store_scaling_freq_range(struct cpufreq_policy *policy,
+					char *buf, size_t count)
+{
+	int ret, temp_min, temp_max;
+	struct cpufreq_policy new_policy;
+
+	ret = cpufreq_get_policy(&new_policy, policy->cpu);
+	if (ret)
+		return -EINVAL;
+
+	ret = sscanf(buf, "%u-%u", &new_policy.min, &new_policy.max);
+	if (ret != 2)
+		return -EINVAL;
+
+	temp_min = new_policy.min;
+	temp_max = new_policy.max;
+	ret = cpufreq_set_policy(policy, &new_policy);
+	if (!ret) {
+		policy->user_policy.min = temp_min;
+		policy->user_policy.max = temp_max;
+	}
+	return ret ? ret : count;
+}
+
+static ssize_t show_scaling_freq_range(struct cpufreq_policy *policy, char *buf)
+{
+	return sprintf(buf, "%u-%u\n", policy->min, policy->max);
+}
+
 cpufreq_freq_attr_ro_perm(cpuinfo_cur_freq, 0400);
 cpufreq_freq_attr_ro(cpuinfo_min_freq);
 cpufreq_freq_attr_ro(cpuinfo_max_freq);
@@ -826,6 +855,7 @@ cpufreq_freq_attr_rw(scaling_min_freq);
 cpufreq_freq_attr_rw(scaling_max_freq);
 cpufreq_freq_attr_rw(scaling_governor);
 cpufreq_freq_attr_rw(scaling_setspeed);
+cpufreq_freq_attr_rw(scaling_freq_range);
 
 static struct attribute *default_attrs[] = {
 	&cpuinfo_min_freq.attr,
@@ -839,6 +869,7 @@ static struct attribute *default_attrs[] = {
 	&scaling_driver.attr,
 	&scaling_available_governors.attr,
 	&scaling_setspeed.attr,
+	&scaling_freq_range.attr,
 	NULL
 };
 
-- 
1.9.1

Re: [PATCH] cpufreq: Add scaling frequency range support

[Viresh Kumar]

On 28-07-15, 11:32, Pan Xinhui wrote:
> From: Pan Xinhui <xinhuix.pan@intel.com>
> 
> Userspace at most time do cpufreq tests very much inconveniently.
> Currently they have to echo min and max cpu freq separately like below:
> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> 
> Add scaling_freq_range cpufreq attr to support userspace's demand.
> Therefore it's easier for testers to write readable scripts like below: 
> echo 480000-2240000 >
> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range

I don't think this brings any good change, we already have support for
that with min/max freqs and I don't see how scripts can be less
readable with that.

So, why to add redundant files at all? Also note that we can't remove
the old interface as that will break the ABI.

-- 
viresh

Re: [PATCH] cpufreq: Add scaling frequency range support

[Pan Xinhui]

hi, Viresh
	thanks for your reply :)
On 2015年07月28日 12:29, Viresh Kumar wrote:
> On 28-07-15, 11:32, Pan Xinhui wrote:
>> From: Pan Xinhui <xinhuix.pan@intel.com>
>>
>> Userspace at most time do cpufreq tests very much inconveniently.
>> Currently they have to echo min and max cpu freq separately like below:
>> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
>> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>>
>> Add scaling_freq_range cpufreq attr to support userspace's demand.
>> Therefore it's easier for testers to write readable scripts like below: 
>> echo 480000-2240000 >
>> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range
> 
> I don't think this brings any good change, we already have support for
> that with min/max freqs and I don't see how scripts can be less
> readable with that.
> 
yes, min/max are supported, however it is inconvenient. sometime it's very easy to cause obscure bugs.
For example, some one might write a script like below.
echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
echo 960000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
.....//other works
echo 1120000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
...//other works

But it did not work when we echo 112000 to min-freq, as the current max freq is smaller than it.
It's hard to figure it out in a big script... we have many such scripts.
I admit this is a bug in script. If we can support *set freq range*, it's a very good option for usesapce to change the cpufreq.
People working in useespace will be thankful to us :)

> So, why to add redundant files at all? Also note that we can't remove
> the old interface as that will break the ABI.
> 
fully agree! we can't break the ABI. So i just add this feature which is very helpful. :)


thanks
xinhui

Re: [PATCH] cpufreq: Add scaling frequency range support

[Rafael J. Wysocki]

On Tuesday, July 28, 2015 12:53:33 PM Pan Xinhui wrote:
> hi, Viresh
> 	thanks for your reply :)
> On 2015年07月28日 12:29, Viresh Kumar wrote:
> > On 28-07-15, 11:32, Pan Xinhui wrote:
> >> From: Pan Xinhui <xinhuix.pan@intel.com>
> >>
> >> Userspace at most time do cpufreq tests very much inconveniently.
> >> Currently they have to echo min and max cpu freq separately like below:
> >> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> >> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> >>
> >> Add scaling_freq_range cpufreq attr to support userspace's demand.
> >> Therefore it's easier for testers to write readable scripts like below: 
> >> echo 480000-2240000 >
> >> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range
> > 
> > I don't think this brings any good change, we already have support for
> > that with min/max freqs and I don't see how scripts can be less
> > readable with that.
> > 
> yes, min/max are supported, however it is inconvenient. sometime it's very easy to cause obscure bugs.
> For example, some one might write a script like below.
> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> echo 960000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> .....//other works
> echo 1120000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> ...//other works
> 
> But it did not work when we echo 112000 to min-freq, as the current max freq is smaller than it.
> It's hard to figure it out in a big script... we have many such scripts.

Fix them, then, pretty please.

And adding this attribute is not going to magically fix them, is it?

Thanks,
Rafael

Re: [PATCH] cpufreq: Add scaling frequency range support

[Pan Xinhui]

hi, Rafael
	thanks for you reply.

On 2015年07月29日 08:18, Rafael J. Wysocki wrote:
> On Tuesday, July 28, 2015 12:53:33 PM Pan Xinhui wrote:
>> hi, Viresh
>> 	thanks for your reply :)
>> On 2015年07月28日 12:29, Viresh Kumar wrote:
>>> On 28-07-15, 11:32, Pan Xinhui wrote:
>>>> From: Pan Xinhui <xinhuix.pan@intel.com>
>>>>
>>>> Userspace at most time do cpufreq tests very much inconveniently.
>>>> Currently they have to echo min and max cpu freq separately like below:
>>>> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
>>>> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>>>>
>>>> Add scaling_freq_range cpufreq attr to support userspace's demand.
>>>> Therefore it's easier for testers to write readable scripts like below: 
>>>> echo 480000-2240000 >
>>>> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range
>>>
>>> I don't think this brings any good change, we already have support for
>>> that with min/max freqs and I don't see how scripts can be less
>>> readable with that.
>>>
>> yes, min/max are supported, however it is inconvenient. sometime it's very easy to cause obscure bugs.
>> For example, some one might write a script like below.
>> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
>> echo 960000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>> .....//other works
>> echo 1120000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
>> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>> ...//other works
>>
>> But it did not work when we echo 112000 to min-freq, as the current max freq is smaller than it.
>> It's hard to figure it out in a big script... we have many such scripts.
> 
> Fix them, then, pretty please.
> 
of course we will fix them. :)

> And adding this attribute is not going to magically fix them, is it?
> 
yes, this patch can not fix them without changing the script. BUT I have another patch which could magically fix them. :)

These two attribute files are very tricky. they are related with each other.
Not like some other attribute file in other part of kernel, for example, proc/sys/fs/file-max.
As the file-min is always zero. It's very reasonable to only support file-max attribute file.

The sequence we echoing value to min/max_freq is very important. Maybe we can also assume they have *state*.
Just like a developer writes a buf to a file. he should do in this way below.
fp = fopen(..)
 => fwrite(...)
  => fclose(...)

The script I mentioned above did not follow the right sequence. when script wants to set the min higher, we need set the max first to avoid min > max issue...
So max/min_freq have *state*. just like TCP Three-way handshake, SYN, ACK&SYN, ACK. the sequence(this is so-called state) is very important.

Now I want to offer a non-state attribute to user-space :)
This is a design/engineering problem. It's okay for kernel to not offer such attribute. But user-space will do more work.
For example, In the worst case, we need system call four times.
read min/max_freq (system call two times)
might set min or max freq first to avoid min > max issue (system call one time)
set min/max a new value (system call one time)

What if we offer *set freq range* attribute? just once. :)
set freq range (system call one time)

From performance point, It's a good idea to offer such attribute.

There is another reason for why it's good to apply this patch.
If cpufreg range is 480000-960000, we call it powersave, 480000-2240000 is normal, 1920000-2240000 is performance.
Assume current cpufreq range is powersave, then user want to set it to performance because user wants to play a 3D game.
BUT user have to set it to normal first, then set it to performance because min(performance) > max(powersave).....
I don't know how people(end-user) would think about such behavior.... why we must be back to normal first, then performance?

As for the patch I mentioned above which could magically fix them.
The solution is: change store_scaling_max_freq and store_scaling_min_freq sysfs callback, let them have *state*.
Always keep the value from user-space.

patch like:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8772346..00e6965 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -615,6 +615,14 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
 static int cpufreq_set_policy(struct cpufreq_policy *policy,
                                struct cpufreq_policy *new_policy);
 
+static void
+cpufreq_get_user_policy_freq(struct cpufreq_real_policy *user_policy,
+                               struct cpufreq_policy *policy)
+{
+       policy->min = user_policy->min;
+       policy->max = user_policy->max;
+}
+
 /**
  * cpufreq_per_cpu_attr_write() / store_##file_name() - sysfs write access
  */
@@ -622,21 +630,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 static ssize_t store_##file_name                                       \
 (struct cpufreq_policy *policy, const char *buf, size_t count)         \
 {                                                                      \
-       int ret, temp;                                                  \
+       int ret;                                                        \
        struct cpufreq_policy new_policy;                               \
                                                                        \
        ret = cpufreq_get_policy(&new_policy, policy->cpu);             \
        if (ret)                                                        \
                return -EINVAL;                                         \
                                                                        \
+       cpufreq_get_user_policy_freq(&policy->user_policy, &new_policy);\
        ret = sscanf(buf, "%u", &new_policy.object);                    \
        if (ret != 1)                                                   \
                return -EINVAL;                                         \
                                                                        \
-       temp = new_policy.object;                                       \
-       ret = cpufreq_set_policy(policy, &new_policy);          \
-       if (!ret)                                                       \
-               policy->user_policy.object = temp;                      \
+       policy->user_policy.object = policy->object;                    \
+       ret = cpufreq_set_policy(policy, &new_policy);                  \
                                                                        \
        return ret ? ret : count;                                       \
 }




Thanks
xinhui

> Thanks,
> Rafael
> 

Re: [PATCH] cpufreq: Add scaling frequency range support

[Pan Xinhui]

On 2015年07月29日 17:59, Pan Xinhui wrote:
> hi, Rafael
> 	thanks for you reply.
> 
> On 2015年07月29日 08:18, Rafael J. Wysocki wrote:
>> On Tuesday, July 28, 2015 12:53:33 PM Pan Xinhui wrote:
>>> hi, Viresh
>>> 	thanks for your reply :)
>>> On 2015年07月28日 12:29, Viresh Kumar wrote:
>>>> On 28-07-15, 11:32, Pan Xinhui wrote:
>>>>> From: Pan Xinhui <xinhuix.pan@intel.com>
>>>>>
>>>>> Userspace at most time do cpufreq tests very much inconveniently.
>>>>> Currently they have to echo min and max cpu freq separately like below:
>>>>> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
>>>>> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>>>>>
>>>>> Add scaling_freq_range cpufreq attr to support userspace's demand.
>>>>> Therefore it's easier for testers to write readable scripts like below: 
>>>>> echo 480000-2240000 >
>>>>> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range
>>>>
>>>> I don't think this brings any good change, we already have support for
>>>> that with min/max freqs and I don't see how scripts can be less
>>>> readable with that.
>>>>
>>> yes, min/max are supported, however it is inconvenient. sometime it's very easy to cause obscure bugs.
>>> For example, some one might write a script like below.
>>> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
>>> echo 960000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>>> .....//other works
>>> echo 1120000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
>>> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>>> ...//other works
>>>
>>> But it did not work when we echo 112000 to min-freq, as the current max freq is smaller than it.
>>> It's hard to figure it out in a big script... we have many such scripts.
>>
>> Fix them, then, pretty please.
>>
> of course we will fix them. :)
> 
>> And adding this attribute is not going to magically fix them, is it?
>>
> yes, this patch can not fix them without changing the script. BUT I have another patch which could magically fix them. :)
> 
> These two attribute files are very tricky. they are related with each other.
> Not like some other attribute file in other part of kernel, for example, proc/sys/fs/file-max.
> As the file-min is always zero. It's very reasonable to only support file-max attribute file.
> 
> The sequence we echoing value to min/max_freq is very important. Maybe we can also assume they have *state*.
> Just like a developer writes a buf to a file. he should do in this way below.
> fp = fopen(..)
>  => fwrite(...)
>   => fclose(...)
> 
> The script I mentioned above did not follow the right sequence. when script wants to set the min higher, we need set the max first to avoid min > max issue...
> So max/min_freq have *state*. just like TCP Three-way handshake, SYN, ACK&SYN, ACK. the sequence(this is so-called state) is very important.
> 
> Now I want to offer a non-state attribute to user-space :)
> This is a design/engineering problem. It's okay for kernel to not offer such attribute. But user-space will do more work.
> For example, In the worst case, we need system call four times.
> read min/max_freq (system call two times)
> might set min or max freq first to avoid min > max issue (system call one time)
> set min/max a new value (system call one time)
> 
> What if we offer *set freq range* attribute? just once. :)
> set freq range (system call one time)
> 
> From performance point, It's a good idea to offer such attribute.
> 
> There is another reason for why it's good to apply this patch.
> If cpufreg range is 480000-960000, we call it powersave, 480000-2240000 is normal, 1920000-2240000 is performance.
> Assume current cpufreq range is powersave, then user want to set it to performance because user wants to play a 3D game.
> BUT user have to set it to normal first, then set it to performance because min(performance) > max(powersave).....
> I don't know how people(end-user) would think about such behavior.... why we must be back to normal first, then performance?
> 
> As for the patch I mentioned above which could magically fix them.
> The solution is: change store_scaling_max_freq and store_scaling_min_freq sysfs callback, let them have *state*.
> Always keep the value from user-space.
> 
> patch like:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 8772346..00e6965 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -615,6 +615,14 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
>  static int cpufreq_set_policy(struct cpufreq_policy *policy,
>                                 struct cpufreq_policy *new_policy);
>  
> +static void
> +cpufreq_get_user_policy_freq(struct cpufreq_real_policy *user_policy,
> +                               struct cpufreq_policy *policy)
> +{
> +       policy->min = user_policy->min;
> +       policy->max = user_policy->max;
> +}
> +
>  /**
>   * cpufreq_per_cpu_attr_write() / store_##file_name() - sysfs write access
>   */
> @@ -622,21 +630,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>  static ssize_t store_##file_name                                       \
>  (struct cpufreq_policy *policy, const char *buf, size_t count)         \
>  {                                                                      \
> -       int ret, temp;                                                  \
> +       int ret;                                                        \
>         struct cpufreq_policy new_policy;                               \
>                                                                         \
>         ret = cpufreq_get_policy(&new_policy, policy->cpu);             \
>         if (ret)                                                        \
>                 return -EINVAL;                                         \
>                                                                         \
> +       cpufreq_get_user_policy_freq(&policy->user_policy, &new_policy);\
>         ret = sscanf(buf, "%u", &new_policy.object);                    \
>         if (ret != 1)                                                   \
>                 return -EINVAL;                                         \
>                                                                         \
> -       temp = new_policy.object;                                       \
> -       ret = cpufreq_set_policy(policy, &new_policy);          \
> -       if (!ret)                                                       \
> -               policy->user_policy.object = temp;                      \
> +       policy->user_policy.object = policy->object;                    \
should be 
+	policy->user_policy.object = new_policy.object;		\
sorry for that.
> +       ret = cpufreq_set_policy(policy, &new_policy);                  \
>                                                                         \
>         return ret ? ret : count;                                       \
>  }
> 
>

> 
> 
> Thanks
> xinhui
> 
>> Thanks,
>> Rafael
>>

Re: [PATCH] cpufreq: Add scaling frequency range support

[Viresh Kumar]

On 29-07-15, 18:04, Pan Xinhui wrote:
> > @@ -622,21 +630,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> >  static ssize_t store_##file_name                                       \
> >  (struct cpufreq_policy *policy, const char *buf, size_t count)         \
> >  {                                                                      \
> > -       int ret, temp;                                                  \
> > +       int ret;                                                        \
> >         struct cpufreq_policy new_policy;                               \
> >                                                                         \
> >         ret = cpufreq_get_policy(&new_policy, policy->cpu);             \
> >         if (ret)                                                        \
> >                 return -EINVAL;                                         \
> >                                                                         \
> > +       cpufreq_get_user_policy_freq(&policy->user_policy, &new_policy);\
> >         ret = sscanf(buf, "%u", &new_policy.object);                    \
> >         if (ret != 1)                                                   \
> >                 return -EINVAL;                                         \
> >                                                                         \
> > -       temp = new_policy.object;                                       \
> > -       ret = cpufreq_set_policy(policy, &new_policy);          \
> > -       if (!ret)                                                       \
> > -               policy->user_policy.object = temp;                      \
> > +       policy->user_policy.object = policy->object;                    \
> should be 
> +	policy->user_policy.object = new_policy.object;		\
> sorry for that.
> > +       ret = cpufreq_set_policy(policy, &new_policy);                  \

This is wrong because we save user-preference, even when we failed. So that's
surely bad.

-- 
viresh

Re: [PATCH] cpufreq: Add scaling frequency range support

[Rafael J. Wysocki]

On Wednesday, July 29, 2015 05:59:14 PM Pan Xinhui wrote:
> hi, Rafael
> 	thanks for you reply.
> 
> On 2015年07月29日 08:18, Rafael J. Wysocki wrote:
> > On Tuesday, July 28, 2015 12:53:33 PM Pan Xinhui wrote:
> >> hi, Viresh
> >> 	thanks for your reply :)
> >> On 2015年07月28日 12:29, Viresh Kumar wrote:
> >>> On 28-07-15, 11:32, Pan Xinhui wrote:
> >>>> From: Pan Xinhui <xinhuix.pan@intel.com>
> >>>>
> >>>> Userspace at most time do cpufreq tests very much inconveniently.
> >>>> Currently they have to echo min and max cpu freq separately like below:
> >>>> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> >>>> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> >>>>
> >>>> Add scaling_freq_range cpufreq attr to support userspace's demand.
> >>>> Therefore it's easier for testers to write readable scripts like below: 
> >>>> echo 480000-2240000 >
> >>>> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range
> >>>
> >>> I don't think this brings any good change, we already have support for
> >>> that with min/max freqs and I don't see how scripts can be less
> >>> readable with that.
> >>>
> >> yes, min/max are supported, however it is inconvenient. sometime it's very easy to cause obscure bugs.
> >> For example, some one might write a script like below.
> >> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> >> echo 960000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> >> .....//other works
> >> echo 1120000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> >> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> >> ...//other works
> >>
> >> But it did not work when we echo 112000 to min-freq, as the current max freq is smaller than it.
> >> It's hard to figure it out in a big script... we have many such scripts.
> > 
> > Fix them, then, pretty please.
> > 
> of course we will fix them. :)
> 
> > And adding this attribute is not going to magically fix them, is it?
> > 
> yes, this patch can not fix them without changing the script. BUT I have another patch which could magically fix them. :)
> 
> These two attribute files are very tricky. they are related with each other.
> Not like some other attribute file in other part of kernel, for example, proc/sys/fs/file-max.
> As the file-min is always zero. It's very reasonable to only support file-max attribute file.
> 
> The sequence we echoing value to min/max_freq is very important. Maybe we can also assume they have *state*.
> Just like a developer writes a buf to a file. he should do in this way below.
> fp = fopen(..)
>  => fwrite(...)
>   => fclose(...)
> 
> The script I mentioned above did not follow the right sequence. when script wants to set the min higher, we need set the max first to avoid min > max issue...
> So max/min_freq have *state*. just like TCP Three-way handshake, SYN, ACK&SYN, ACK. the sequence(this is so-called state) is very important.

No, this isn't like that.  The rule is simple: whatever is in one of the
attributes needs to be a smaller value than the one from the other attribute
at any time.  So there is a correlation between them, but the only "state" is
those numbers written to them previously (which they preserve quite as expected).

And the algorithm is: look for what's in min and write a number which is not
less then that to max.  And the other way around.

Again, please fix your scripts and don't litter the kernel with stuff which
only is needed because user space developers can't get their act together.

Case dismissed.

Thanks,
Rafael

Re: [PATCH] cpufreq: Add scaling frequency range support

[Viresh Kumar]

On 28-07-15, 11:32, Pan Xinhui wrote:
> From: Pan Xinhui <xinhuix.pan@intel.com>
> 
> Userspace at most time do cpufreq tests very much inconveniently.
> Currently they have to echo min and max cpu freq separately like below:
> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> 
> Add scaling_freq_range cpufreq attr to support userspace's demand.
> Therefore it's easier for testers to write readable scripts like below: 
> echo 480000-2240000 >
> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range
> 
> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)

Okay, code looks fine but please updates Documentation as well.

-- 
viresh

[PATCH V2] cpufreq: Add scaling frequency range support

[Pan Xinhui]

From: Pan Xinhui <xinhuix.pan@intel.com>

Userspace at most time do cpufreq tests very much inconveniently.
Currently they have to echo min and max cpu freq separately like below:
echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq

Add scaling_freq_range cpufreq attr to support userspace's demand.
Therefore it's easier for testers to write readable scripts like below:
echo 480000-2240000 >
/sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
Change from V1:
	update cpu-freq documentation.
---
 Documentation/cpu-freq/user-guide.txt |  7 +++++++
 drivers/cpufreq/cpufreq.c             | 31 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/Documentation/cpu-freq/user-guide.txt b/Documentation/cpu-freq/user-guide.txt
index 109e97b..ac6dd7b 100644
--- a/Documentation/cpu-freq/user-guide.txt
+++ b/Documentation/cpu-freq/user-guide.txt
@@ -212,6 +212,13 @@ bios_limit :			If the BIOS tells the OS to limit a CPU to
 				which can be detected through the generic
 				thermal driver.
 
+scaling_freq_range :	show the current cpufreq range of policy (in
+				kHz) with format "%u-%u". In other words it shows the
+				scaling_min_freq and scaling_max_freq at same time. By
+				echoing new values with format "%u-%u" into this file,
+				you can change this range. first %u stands for
+				scaling_min_freq, second %u stands for scaling_max_freq.
+
 If you have selected the "userspace" governor which allows you to
 set the CPU operating frequency to a specific value, you can read out
 the current frequency in
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 26063af..6424e05 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -812,6 +812,35 @@ static ssize_t show_bios_limit(struct cpufreq_policy *policy, char *buf)
 	return sprintf(buf, "%u\n", policy->cpuinfo.max_freq);
 }
 
+static ssize_t store_scaling_freq_range(struct cpufreq_policy *policy,
+					char *buf, size_t count)
+{
+	int ret, temp_min, temp_max;
+	struct cpufreq_policy new_policy;
+
+	ret = cpufreq_get_policy(&new_policy, policy->cpu);
+	if (ret)
+		return -EINVAL;
+
+	ret = sscanf(buf, "%u-%u", &new_policy.min, &new_policy.max);
+	if (ret != 2)
+		return -EINVAL;
+
+	temp_min = new_policy.min;
+	temp_max = new_policy.max;
+	ret = cpufreq_set_policy(policy, &new_policy);
+	if (!ret) {
+		policy->user_policy.min = temp_min;
+		policy->user_policy.max = temp_max;
+	}
+	return ret ? ret : count;
+}
+
+static ssize_t show_scaling_freq_range(struct cpufreq_policy *policy, char *buf)
+{
+	return sprintf(buf, "%u-%u\n", policy->min, policy->max);
+}
+
 cpufreq_freq_attr_ro_perm(cpuinfo_cur_freq, 0400);
 cpufreq_freq_attr_ro(cpuinfo_min_freq);
 cpufreq_freq_attr_ro(cpuinfo_max_freq);
@@ -826,6 +855,7 @@ cpufreq_freq_attr_rw(scaling_min_freq);
 cpufreq_freq_attr_rw(scaling_max_freq);
 cpufreq_freq_attr_rw(scaling_governor);
 cpufreq_freq_attr_rw(scaling_setspeed);
+cpufreq_freq_attr_rw(scaling_freq_range);
 
 static struct attribute *default_attrs[] = {
 	&cpuinfo_min_freq.attr,
@@ -839,6 +869,7 @@ static struct attribute *default_attrs[] = {
 	&scaling_driver.attr,
 	&scaling_available_governors.attr,
 	&scaling_setspeed.attr,
+	&scaling_freq_range.attr,
 	NULL
 };
 
-- 
1.9.1

Re: [PATCH V2] cpufreq: Add scaling frequency range support

[Viresh Kumar]

On 28-07-15, 15:02, Pan Xinhui wrote:
> From: Pan Xinhui <xinhuix.pan@intel.com>
> 
> Userspace at most time do cpufreq tests very much inconveniently.
> Currently they have to echo min and max cpu freq separately like below:
> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
> 
> Add scaling_freq_range cpufreq attr to support userspace's demand.
> Therefore it's easier for testers to write readable scripts like below:
> echo 480000-2240000 >
> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range
> 
> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
> ---
> Change from V1:
> 	update cpu-freq documentation.
> ---
>  Documentation/cpu-freq/user-guide.txt |  7 +++++++
>  drivers/cpufreq/cpufreq.c             | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH] cpufreq: Add scaling frequency range support

[Rafael J. Wysocki]

On Tuesday, July 28, 2015 11:32:19 AM Pan Xinhui wrote:
> From: Pan Xinhui <xinhuix.pan@intel.com>
> 
> Userspace at most time do cpufreq tests very much inconveniently.
> Currently they have to echo min and max cpu freq separately like below:
> echo 480000  > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq
> echo 2240000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq

And why is this a problem?

> Add scaling_freq_range cpufreq attr to support userspace's demand.
> Therefore it's easier for testers to write readable scripts like below: 
> echo 480000-2240000 >
> /sys/devices/system/cpu/cpu0/cpufreq/scaling_freq_range

I'm not sure I agree with the statement that this is more readable.

Not applied.

Thanks,
Rafael