[PATCH] cpufreq: Restore policy min/max limits on CPU online

[PATCH] cpufreq: Restore policy min/max limits on CPU online

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

On CPU online the cpufreq core restores the previous governor (or
the previous "policy" setting for ->setpolicy drivers), but it does
not restore the min/max limits at the same time, which is confusing,
inconsistent and real pain for users who set the limits and then
suspend/resume the system (using full suspend), in which case the
limits are reset on all CPUs except for the boot one.

Fix this by making cpufreq_init_policy() restore the limits when it
sees that this is CPU online and not initialization from scratch.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -979,6 +979,8 @@ static int cpufreq_init_policy(struct cp
 	/* Update governor of new_policy to the governor used before hotplug */
 	gov = find_governor(policy->last_governor);
 	if (gov) {
+		new_policy.min = policy->user_policy.min;
+		new_policy.max = policy->user_policy.max;
 		pr_debug("Restoring governor %s for cpu %d\n",
 				policy->governor->name, policy->cpu);
 	} else {
@@ -991,11 +993,14 @@ static int cpufreq_init_policy(struct cp
 
 	/* Use the default policy if there is no last_policy. */
 	if (cpufreq_driver->setpolicy) {
-		if (policy->last_policy)
+		if (policy->last_policy) {
 			new_policy.policy = policy->last_policy;
-		else
+			new_policy.min = policy->user_policy.min;
+			new_policy.max = policy->user_policy.max;
+		} else {
 			cpufreq_parse_governor(gov->name, &new_policy.policy,
 					       NULL);
+		}
 	}
 	/* set default policy */
 	return cpufreq_set_policy(policy, &new_policy);

Re: [PATCH] cpufreq: Restore policy min/max limits on CPU online

[Viresh Kumar]

On 16-03-17, 23:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> On CPU online the cpufreq core restores the previous governor (or
> the previous "policy" setting for ->setpolicy drivers), but it does
> not restore the min/max limits at the same time, which is confusing,
> inconsistent and real pain for users who set the limits and then
> suspend/resume the system (using full suspend), in which case the
> limits are reset on all CPUs except for the boot one.
> 
> Fix this by making cpufreq_init_policy() restore the limits when it
> sees that this is CPU online and not initialization from scratch.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -979,6 +979,8 @@ static int cpufreq_init_policy(struct cp
>  	/* Update governor of new_policy to the governor used before hotplug */
>  	gov = find_governor(policy->last_governor);
>  	if (gov) {
> +		new_policy.min = policy->user_policy.min;
> +		new_policy.max = policy->user_policy.max;
>  		pr_debug("Restoring governor %s for cpu %d\n",
>  				policy->governor->name, policy->cpu);
>  	} else {
> @@ -991,11 +993,14 @@ static int cpufreq_init_policy(struct cp
>  
>  	/* Use the default policy if there is no last_policy. */
>  	if (cpufreq_driver->setpolicy) {
> -		if (policy->last_policy)
> +		if (policy->last_policy) {
>  			new_policy.policy = policy->last_policy;
> -		else
> +			new_policy.min = policy->user_policy.min;
> +			new_policy.max = policy->user_policy.max;
> +		} else {
>  			cpufreq_parse_governor(gov->name, &new_policy.policy,
>  					       NULL);
> +		}
>  	}
>  	/* set default policy */
>  	return cpufreq_set_policy(policy, &new_policy);

What about something like this instead ?

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b8ff617d449d..5dbdd261aa73 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1184,6 +1184,9 @@ static int cpufreq_online(unsigned int cpu)
                for_each_cpu(j, policy->related_cpus)
                        per_cpu(cpufreq_cpu_data, j) = policy;
                write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+       } else {
+               policy->min = policy->user_policy.min;
+               policy->max = policy->user_policy.max;
        }
 
        if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {


-- 
viresh

Re: [PATCH] cpufreq: Restore policy min/max limits on CPU online

[Rafael J. Wysocki]

On Fri, Mar 17, 2017 at 4:20 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16-03-17, 23:42, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> On CPU online the cpufreq core restores the previous governor (or
>> the previous "policy" setting for ->setpolicy drivers), but it does
>> not restore the min/max limits at the same time, which is confusing,
>> inconsistent and real pain for users who set the limits and then
>> suspend/resume the system (using full suspend), in which case the
>> limits are reset on all CPUs except for the boot one.
>>
>> Fix this by making cpufreq_init_policy() restore the limits when it
>> sees that this is CPU online and not initialization from scratch.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  drivers/cpufreq/cpufreq.c |    9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
>> +++ linux-pm/drivers/cpufreq/cpufreq.c
>> @@ -979,6 +979,8 @@ static int cpufreq_init_policy(struct cp
>>       /* Update governor of new_policy to the governor used before hotplug */
>>       gov = find_governor(policy->last_governor);
>>       if (gov) {
>> +             new_policy.min = policy->user_policy.min;
>> +             new_policy.max = policy->user_policy.max;
>>               pr_debug("Restoring governor %s for cpu %d\n",
>>                               policy->governor->name, policy->cpu);
>>       } else {
>> @@ -991,11 +993,14 @@ static int cpufreq_init_policy(struct cp
>>
>>       /* Use the default policy if there is no last_policy. */
>>       if (cpufreq_driver->setpolicy) {
>> -             if (policy->last_policy)
>> +             if (policy->last_policy) {
>>                       new_policy.policy = policy->last_policy;
>> -             else
>> +                     new_policy.min = policy->user_policy.min;
>> +                     new_policy.max = policy->user_policy.max;
>> +             } else {
>>                       cpufreq_parse_governor(gov->name, &new_policy.policy,
>>                                              NULL);
>> +             }
>>       }
>>       /* set default policy */
>>       return cpufreq_set_policy(policy, &new_policy);
>
> What about something like this instead ?

It generally would work, but I don't like it.

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index b8ff617d449d..5dbdd261aa73 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1184,6 +1184,9 @@ static int cpufreq_online(unsigned int cpu)
>                 for_each_cpu(j, policy->related_cpus)
>                         per_cpu(cpufreq_cpu_data, j) = policy;
>                 write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> +       } else {
> +               policy->min = policy->user_policy.min;
> +               policy->max = policy->user_policy.max;
>         }
>
>         if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
>
>
> --

IMO if we are not going to restore the governor, we also should not
restore the limits as those things are related.  Now, the governor can
be unloaded while the CPU is offline.

Code duplication can be addressed by adding a helper function to do
the copying.  I can do that later if you insist.

Thanks,
Rafael

Re: [PATCH] cpufreq: Restore policy min/max limits on CPU online

[Viresh Kumar]

On 17 March 2017 at 22:01, Rafael J. Wysocki <rafael@kernel.org> wrote:

> IMO if we are not going to restore the governor, we also should not
> restore the limits as those things are related.  Now, the governor can
> be unloaded while the CPU is offline.

I thought about it earlier but then governor and policy min/max
looked independent to me. Why do you think they are related?

Re: [PATCH] cpufreq: Restore policy min/max limits on CPU online

[Rafael J. Wysocki]

On Fri, Mar 17, 2017 at 5:43 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 17 March 2017 at 22:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
>> IMO if we are not going to restore the governor, we also should not
>> restore the limits as those things are related.  Now, the governor can
>> be unloaded while the CPU is offline.
>
> I thought about it earlier but then governor and policy min/max
> looked independent to me. Why do you think they are related?

They are parts of one set of settings.

If the governor is not restored, the policy starts with the default
one, so why would it not start with the default limits then?

My opinion is that either we restore everything the way it was, or we
start afresh entirely.

Thanks,
Rafael

[PATCH v2] cpufreq: Restore policy min/max limits on CPU online

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] cpufreq: Restore policy min/max limits on CPU online

On CPU online the cpufreq core restores the previous governor (or
the previous "policy" setting for ->setpolicy drivers), but it does
not restore the min/max limits at the same time, which is confusing,
inconsistent and real pain for users who set the limits and then
suspend/resume the system (using full suspend), in which case the
limits are reset for all CPUs except for the boot one (on systems
with one CPU per policy, which means the vast majority of laptops,
for example).

Fix this by making cpufreq_online() restore the limits when it
sees that this is CPU online and not initialization from scratch.

Also move the governor restoration code to the place where the limits
are restored for consistency.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Here it goes for completeness:

-> v2: Avoid code duplication by adding a helper function for copying values.

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

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -969,6 +969,13 @@ __weak struct cpufreq_governor *cpufreq_
 	return NULL;
 }
 
+static void policy_copy_user_min_max(struct cpufreq_policy *to,
+				     struct cpufreq_policy *from)
+{
+	to->min = from->user_policy.min;
+	to->max = from->user_policy.max;
+}
+
 static int cpufreq_init_policy(struct cpufreq_policy *policy)
 {
 	struct cpufreq_governor *gov = NULL;
@@ -979,6 +986,7 @@ static int cpufreq_init_policy(struct cp
 	/* Update governor of new_policy to the governor used before hotplug */
 	gov = find_governor(policy->last_governor);
 	if (gov) {
+		policy_copy_user_min_max(&new_policy, policy);
 		pr_debug("Restoring governor %s for cpu %d\n",
 				policy->governor->name, policy->cpu);
 	} else {
@@ -991,11 +999,13 @@ static int cpufreq_init_policy(struct cp
 
 	/* Use the default policy if there is no last_policy. */
 	if (cpufreq_driver->setpolicy) {
-		if (policy->last_policy)
+		if (policy->last_policy) {
 			new_policy.policy = policy->last_policy;
-		else
+			policy_copy_user_min_max(&new_policy, policy);
+		} else {
 			cpufreq_parse_governor(gov->name, &new_policy.policy,
 					       NULL);
+		}
 	}
 	/* set default policy */
 	return cpufreq_set_policy(policy, &new_policy);
@@ -2263,8 +2273,7 @@ void cpufreq_update_policy(unsigned int
 
 	pr_debug("updating policy for CPU %u\n", cpu);
 	memcpy(&new_policy, policy, sizeof(*policy));
-	new_policy.min = policy->user_policy.min;
-	new_policy.max = policy->user_policy.max;
+	policy_copy_user_min_max(&new_policy, policy);
 
 	/*
 	 * BIOS might change freq behind our back

Re: [PATCH] cpufreq: Restore policy min/max limits on CPU online

[Viresh Kumar]

On 17-03-17, 18:40, Rafael J. Wysocki wrote:
> On Fri, Mar 17, 2017 at 5:43 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 17 March 2017 at 22:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> >> IMO if we are not going to restore the governor, we also should not
> >> restore the limits as those things are related.  Now, the governor can
> >> be unloaded while the CPU is offline.
> >
> > I thought about it earlier but then governor and policy min/max
> > looked independent to me. Why do you think they are related?
> 
> They are parts of one set of settings.
> 
> If the governor is not restored, the policy starts with the default
> one, so why would it not start with the default limits then?

Do we reset the limits when we change governor's normally? No. Then
why should we consider suspend/resume special in that sense? These are
completely different and independent settings which user has done and
we don't really need to relate them.

> My opinion is that either we restore everything the way it was, or we
> start afresh entirely.

What about fields like: policy->user_policy.*? They aren't reset for
existing policies if the last governor isn't found. And there are
drivers which call cpufreq_update_policy(), and that would mean that
the CPU will come back to user defined policies before system
suspended. And that kind of defeats whatever you were trying to do in
this patch. Isn't it?

-- 
viresh

Re: [PATCH] cpufreq: Restore policy min/max limits on CPU online

[Rafael J. Wysocki]

On Monday, March 20, 2017 08:51:56 AM Viresh Kumar wrote:
> On 17-03-17, 18:40, Rafael J. Wysocki wrote:
> > On Fri, Mar 17, 2017 at 5:43 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 17 March 2017 at 22:01, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > >> IMO if we are not going to restore the governor, we also should not
> > >> restore the limits as those things are related.  Now, the governor can
> > >> be unloaded while the CPU is offline.
> > >
> > > I thought about it earlier but then governor and policy min/max
> > > looked independent to me. Why do you think they are related?
> > 
> > They are parts of one set of settings.
> > 
> > If the governor is not restored, the policy starts with the default
> > one, so why would it not start with the default limits then?
> 
> Do we reset the limits when we change governor's normally? No. Then
> why should we consider suspend/resume special in that sense? These are
> completely different and independent settings which user has done and
> we don't really need to relate them.
> 
> > My opinion is that either we restore everything the way it was, or we
> > start afresh entirely.
> 
> What about fields like: policy->user_policy.*? They aren't reset for
> existing policies if the last governor isn't found. And there are
> drivers which call cpufreq_update_policy(), and that would mean that
> the CPU will come back to user defined policies before system
> suspended. And that kind of defeats whatever you were trying to do in
> this patch. Isn't it?

OK, it looks like I don't care as much as you do. :-)

Send the patch with a changelog.

Thanks,
Rafael