[PATCH] cpufreq: respect the min/max settings from user space

[PATCH] cpufreq: respect the min/max settings from user space

[Vince Hsu]

When the user space tries to set scaling_(max|min)_freq through
sysfs, the cpufreq_set_policy() asks other driver's opinions
for the max/min frequencies. Some device drivers, like Tegra
CPU EDP which is not upstreamed yet though, may constrain the
CPU maximum frequency dynamically because of board design.
So if the user space access happens and some driver is capping
the cpu frequency at the same time, the user_policy->(max|min)
is overridden by the capped value, and that's not expected by
the user space. And if the user space is not invoked again,
the CPU will always be capped by the user_policy->(max|min)
even no drivers limit the CPU frequency any more.

This patch preserves the user specified min/max settings, so that
every time the cpufreq policy is updated, the new max/min can
be re-evaluated correctly based on the user's expection and
the present device drivers' status.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
Hi,

I'm not sure if any platform that is supported mainlin might have this
issue, and this patch is complie tested only. We hit the problem when 
the laptop_mode tool configures the scaling_max_freq and the Tegra
CPU EDP driver is limiting the CPU maximum frequency.

 drivers/cpufreq/cpufreq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 24bf76fba141..c007cf2a3d2a 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -524,7 +524,7 @@ 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;							\
+	int ret, temp;						\
 	struct cpufreq_policy new_policy;				\
 									\
 	ret = cpufreq_get_policy(&new_policy, policy->cpu);		\
@@ -535,8 +535,10 @@ static ssize_t store_##file_name					\
 	if (ret != 1)							\
 		return -EINVAL;						\
 									\
+	temp = new_policy.object;					\
 	ret = cpufreq_set_policy(policy, &new_policy);		\
-	policy->user_policy.object = policy->object;			\
+	if (!ret)							\
+		policy->user_policy.object = temp;			\
 									\
 	return ret ? ret : count;					\
 }
-- 
1.9.1

Re: [PATCH] cpufreq: respect the min/max settings from user space

[Viresh Kumar]

On 2 October 2014 12:25, Vince Hsu <vinceh@nvidia.com> wrote:
> When the user space tries to set scaling_(max|min)_freq through
> sysfs, the cpufreq_set_policy() asks other driver's opinions
> for the max/min frequencies. Some device drivers, like Tegra
> CPU EDP which is not upstreamed yet though, may constrain the
> CPU maximum frequency dynamically because of board design.
> So if the user space access happens and some driver is capping
> the cpu frequency at the same time, the user_policy->(max|min)
> is overridden by the capped value, and that's not expected by
> the user space. And if the user space is not invoked again,
> the CPU will always be capped by the user_policy->(max|min)
> even no drivers limit the CPU frequency any more.
>
> This patch preserves the user specified min/max settings, so that
> every time the cpufreq policy is updated, the new max/min can
> be re-evaluated correctly based on the user's expection and
> the present device drivers' status.
>
> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> ---
> Hi,
>
> I'm not sure if any platform that is supported mainlin might have this
> issue, and this patch is complie tested only.

Why only compiled tested? Why haven't you tested it on tegra?

>  drivers/cpufreq/cpufreq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 24bf76fba141..c007cf2a3d2a 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -524,7 +524,7 @@ 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;                                                        \
> +       int ret, temp;                                          \
>         struct cpufreq_policy new_policy;                               \
>                                                                         \
>         ret = cpufreq_get_policy(&new_policy, policy->cpu);             \
> @@ -535,8 +535,10 @@ static ssize_t store_##file_name                                   \
>         if (ret != 1)                                                   \
>                 return -EINVAL;                                         \
>                                                                         \
> +       temp = new_policy.object;                                       \
>         ret = cpufreq_set_policy(policy, &new_policy);          \
> -       policy->user_policy.object = policy->object;                    \
> +       if (!ret)                                                       \
> +               policy->user_policy.object = temp;                      \
>                                                                         \
>         return ret ? ret : count;                                       \
>  }

Looks fine otherwise.

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

Re: [PATCH] cpufreq: respect the min/max settings from user space

[Vince Hsu]

Hi Viresh,

On 10/06/2014 12:45 PM, Viresh Kumar wrote:
> On 2 October 2014 12:25, Vince Hsu <vinceh@nvidia.com> wrote:
>> When the user space tries to set scaling_(max|min)_freq through
>> sysfs, the cpufreq_set_policy() asks other driver's opinions
>> for the max/min frequencies. Some device drivers, like Tegra
>> CPU EDP which is not upstreamed yet though, may constrain the
>> CPU maximum frequency dynamically because of board design.
>> So if the user space access happens and some driver is capping
>> the cpu frequency at the same time, the user_policy->(max|min)
>> is overridden by the capped value, and that's not expected by
>> the user space. And if the user space is not invoked again,
>> the CPU will always be capped by the user_policy->(max|min)
>> even no drivers limit the CPU frequency any more.
>>
>> This patch preserves the user specified min/max settings, so that
>> every time the cpufreq policy is updated, the new max/min can
>> be re-evaluated correctly based on the user's expection and
>> the present device drivers' status.
>>
>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
>> ---
>> Hi,
>>
>> I'm not sure if any platform that is supported mainlin might have this
>> issue, and this patch is complie tested only.
> Why only compiled tested? Why haven't you tested it on tegra?
I did test with Chrome kernel on Tegra platform. I can't do that with 
mainline kernel because we haven't had the CPU EDP driver upstream yet.

Thanks,
Vince

>
>>   drivers/cpufreq/cpufreq.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 24bf76fba141..c007cf2a3d2a 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -524,7 +524,7 @@ 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;                                                        \
>> +       int ret, temp;                                          \
>>          struct cpufreq_policy new_policy;                               \
>>                                                                          \
>>          ret = cpufreq_get_policy(&new_policy, policy->cpu);             \
>> @@ -535,8 +535,10 @@ static ssize_t store_##file_name                                   \
>>          if (ret != 1)                                                   \
>>                  return -EINVAL;                                         \
>>                                                                          \
>> +       temp = new_policy.object;                                       \
>>          ret = cpufreq_set_policy(policy, &new_policy);          \
>> -       policy->user_policy.object = policy->object;                    \
>> +       if (!ret)                                                       \
>> +               policy->user_policy.object = temp;                      \
>>                                                                          \
>>          return ret ? ret : count;                                       \
>>   }
> Looks fine otherwise.
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH] cpufreq: respect the min/max settings from user space

[Viresh Kumar]

On 6 October 2014 10:20, Vince Hsu <vinceh@nvidia.com> wrote:
> I did test with Chrome kernel on Tegra platform. I can't do that with
> mainline kernel because we haven't had the CPU EDP driver upstream yet.

You should have mentioned this clearly :)

Looks fine to me then.

Re: [PATCH] cpufreq: respect the min/max settings from user space

[Vince Hsu]

Hi Viresh,

Could you remind me where can I find this patch upstream? It seems this 
was missed?

Thanks,
Vince

On 10/06/2014 12:50 PM, Vince Hsu wrote:
> Hi Viresh,
>
> On 10/06/2014 12:45 PM, Viresh Kumar wrote:
>> On 2 October 2014 12:25, Vince Hsu <vinceh@nvidia.com> wrote:
>>> When the user space tries to set scaling_(max|min)_freq through
>>> sysfs, the cpufreq_set_policy() asks other driver's opinions
>>> for the max/min frequencies. Some device drivers, like Tegra
>>> CPU EDP which is not upstreamed yet though, may constrain the
>>> CPU maximum frequency dynamically because of board design.
>>> So if the user space access happens and some driver is capping
>>> the cpu frequency at the same time, the user_policy->(max|min)
>>> is overridden by the capped value, and that's not expected by
>>> the user space. And if the user space is not invoked again,
>>> the CPU will always be capped by the user_policy->(max|min)
>>> even no drivers limit the CPU frequency any more.
>>>
>>> This patch preserves the user specified min/max settings, so that
>>> every time the cpufreq policy is updated, the new max/min can
>>> be re-evaluated correctly based on the user's expection and
>>> the present device drivers' status.
>>>
>>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
>>> ---
>>> Hi,
>>>
>>> I'm not sure if any platform that is supported mainlin might have this
>>> issue, and this patch is complie tested only.
>> Why only compiled tested? Why haven't you tested it on tegra?
> I did test with Chrome kernel on Tegra platform. I can't do that with 
> mainline kernel because we haven't had the CPU EDP driver upstream yet.
>
> Thanks,
> Vince
>
>>
>>>   drivers/cpufreq/cpufreq.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>> index 24bf76fba141..c007cf2a3d2a 100644
>>> --- a/drivers/cpufreq/cpufreq.c
>>> +++ b/drivers/cpufreq/cpufreq.c
>>> @@ -524,7 +524,7 @@ 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;                                                        \
>>> +       int ret, temp;                                          \
>>>          struct cpufreq_policy 
>>> new_policy;                               \
>>> \
>>>          ret = cpufreq_get_policy(&new_policy, 
>>> policy->cpu);             \
>>> @@ -535,8 +535,10 @@ static ssize_t 
>>> store_##file_name                                   \
>>>          if (ret != 
>>> 1)                                                   \
>>>                  return 
>>> -EINVAL;                                         \
>>> \
>>> +       temp = 
>>> new_policy.object;                                       \
>>>          ret = cpufreq_set_policy(policy, &new_policy);          \
>>> -       policy->user_policy.object = 
>>> policy->object;                    \
>>> +       if 
>>> (!ret)                                                       \
>>> +               policy->user_policy.object = 
>>> temp;                      \
>>> \
>>>          return ret ? ret : 
>>> count;                                       \
>>>   }
>> Looks fine otherwise.
>>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cpufreq: respect the min/max settings from user space

[Viresh Kumar]

On 28 October 2014 08:55, Vince Hsu <vinceh@nvidia.com> wrote:
> Hi Viresh,
>
> Could you remind me where can I find this patch upstream? It seems this was
> missed?

Rafael hasn't picked it up. You can normally look at the tree Rafael
manages:

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git

Can you please resend it to Rafael ?

Re: [PATCH] cpufreq: respect the min/max settings from user space

[Vince Hsu]

Hi Viresh,

Just sent v2 with your ACK. :)

Hi Rafael,

Could you please apply the v2? Thanks!

Vince

On 11/10/2014 01:09 PM, Viresh Kumar wrote:
> On 28 October 2014 08:55, Vince Hsu <vinceh@nvidia.com> wrote:
>> Hi Viresh,
>>
>> Could you remind me where can I find this patch upstream? It seems this was
>> missed?
> Rafael hasn't picked it up. You can normally look at the tree Rafael
> manages:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
>
> Can you please resend it to Rafael ?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html