[PATCH 0/3] cpufreq: Governor start handling unification and cpufreq_resume() cleanup

[PATCH 0/3] cpufreq: Governor start handling unification and cpufreq_resume() cleanup

[Rafael J. Wysocki]

Hi,

As per the subject, the following patches unify the handling of governor
start throughout the cpufreq core and clean up the way cpufreq_resume()
deals with policy->cur being out of sync with the real frequency.

[1/3] introduces cpufreq_start_governor() to implement the repeating
      governor start code pattern.
[2/3] moves the frequency update code from cpufreq_update_policy() to
      a separate function, cpufreq_update_current_freq().
[3/3] makes cpufreq_start_governor() call cpufreq_update_current_freq()
      and drops the (super-ugly and racy) piece of code to synchrnonize
      the frequency of the boot CPU with its policy->cur.

Thanks,
Rafael

[PATCH 1/3] cpufreq: Introduce cpufreq_start_governor()

[Rafael J. Wysocki]

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

Starting a governor in cpufreq always follows the same pattern
involving two calls to cpufreq_governor(), one with the event
argument set to CPUFREQ_GOV_START and one with that argument set to
CPUFREQ_GOV_LIMITS.

Introduce cpufreq_start_governor() that will carry out those two
operations and make all places where governors are started use it.

That slightly modifies the behavior of cpufreq_set_policy() which
now also will go back to the old governor if the second call to
cpufreq_governor() (the one with event equal to CPUFREQ_GOV_LIMITS)
fails, but that really is how it should work in the first place.

Also cpufreq_resume() will now pring an error message if the
CPUFREQ_GOV_LIMITS call to cpufreq_governor() fails, but that
makes it follow cpufreq_add_policy_cpu() and cpufreq_offline()
in that respect.

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

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -76,6 +76,7 @@ static inline bool has_target(void)
 /* internal prototypes */
 static int cpufreq_governor(struct cpufreq_policy *policy, unsigned int event);
 static unsigned int __cpufreq_get(struct cpufreq_policy *policy);
+static int cpufreq_start_governor(struct cpufreq_policy *policy);
 
 /**
  * Two notifier lists: the "policy" list is involved in the
@@ -1015,10 +1016,7 @@ static int cpufreq_add_policy_cpu(struct
 	cpumask_set_cpu(cpu, policy->cpus);
 
 	if (has_target()) {
-		ret = cpufreq_governor(policy, CPUFREQ_GOV_START);
-		if (!ret)
-			ret = cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
-
+		ret = cpufreq_start_governor(policy);
 		if (ret)
 			pr_err("%s: Failed to start governor\n", __func__);
 	}
@@ -1376,10 +1374,7 @@ static void cpufreq_offline(unsigned int
 	/* Start governor again for active policy */
 	if (!policy_is_inactive(policy)) {
 		if (has_target()) {
-			ret = cpufreq_governor(policy, CPUFREQ_GOV_START);
-			if (!ret)
-				ret = cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
-
+			ret = cpufreq_start_governor(policy);
 			if (ret)
 				pr_err("%s: Failed to start governor\n", __func__);
 		}
@@ -1659,9 +1654,7 @@ void cpufreq_resume(void)
 				policy);
 		} else {
 			down_write(&policy->rwsem);
-			ret = cpufreq_governor(policy, CPUFREQ_GOV_START);
-			if (!ret)
-				cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
+			ret = cpufreq_start_governor(policy);
 			up_write(&policy->rwsem);
 
 			if (ret)
@@ -2047,6 +2040,14 @@ static int cpufreq_governor(struct cpufr
 	return ret;
 }
 
+static int cpufreq_start_governor(struct cpufreq_policy *policy)
+{
+	int ret;
+
+	ret = cpufreq_governor(policy, CPUFREQ_GOV_START);
+	return ret ? ret : cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
+}
+
 int cpufreq_register_governor(struct cpufreq_governor *governor)
 {
 	int err;
@@ -2183,8 +2184,10 @@ static int cpufreq_set_policy(struct cpu
 		return cpufreq_driver->setpolicy(new_policy);
 	}
 
-	if (new_policy->governor == policy->governor)
-		goto out;
+	if (new_policy->governor == policy->governor) {
+		pr_debug("cpufreq: governor limits update\n");
+		return cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
+	}
 
 	pr_debug("governor switch\n");
 
@@ -2212,10 +2215,11 @@ static int cpufreq_set_policy(struct cpu
 	policy->governor = new_policy->governor;
 	ret = cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT);
 	if (!ret) {
-		ret = cpufreq_governor(policy, CPUFREQ_GOV_START);
-		if (!ret)
-			goto out;
-
+		ret = cpufreq_start_governor(policy);
+		if (!ret) {
+			pr_debug("cpufreq: governor change\n");
+			return 0;
+		}
 		cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
 	}
 
@@ -2226,14 +2230,10 @@ static int cpufreq_set_policy(struct cpu
 		if (cpufreq_governor(policy, CPUFREQ_GOV_POLICY_INIT))
 			policy->governor = NULL;
 		else
-			cpufreq_governor(policy, CPUFREQ_GOV_START);
+			cpufreq_start_governor(policy);
 	}
 
 	return ret;
-
- out:
-	pr_debug("governor: change or update limits\n");
-	return cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 }
 
 /**

Re: [PATCH 1/3] cpufreq: Introduce cpufreq_start_governor()

[Viresh Kumar]

On 21-03-16, 15:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Starting a governor in cpufreq always follows the same pattern
> involving two calls to cpufreq_governor(), one with the event
> argument set to CPUFREQ_GOV_START and one with that argument set to
> CPUFREQ_GOV_LIMITS.
> 
> Introduce cpufreq_start_governor() that will carry out those two
> operations and make all places where governors are started use it.
> 
> That slightly modifies the behavior of cpufreq_set_policy() which
> now also will go back to the old governor if the second call to
> cpufreq_governor() (the one with event equal to CPUFREQ_GOV_LIMITS)
> fails, but that really is how it should work in the first place.
> 
> Also cpufreq_resume() will now pring an error message if the
> CPUFREQ_GOV_LIMITS call to cpufreq_governor() fails, but that
> makes it follow cpufreq_add_policy_cpu() and cpufreq_offline()
> in that respect.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)

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

-- 
viresh

[PATCH 2/3] cpufreq: Introduce cpufreq_update_current_freq()

[Rafael J. Wysocki]

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

Move the part of cpufreq_update_policy() that obtains the current
frequency from the driver and updates policy->cur if necessary to
a separate function, cpufreq_get_current_freq().

That should not introduce functional changes and subsequent change
set will need it.

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

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1555,6 +1555,24 @@ unsigned int cpufreq_get(unsigned int cp
 }
 EXPORT_SYMBOL(cpufreq_get);
 
+static unsigned int cpufreq_update_current_freq(struct cpufreq_policy *policy)
+{
+	unsigned int new_freq;
+
+	new_freq = cpufreq_driver->get(policy->cpu);
+	if (!new_freq)
+		return 0;
+
+	if (!policy->cur) {
+		pr_debug("cpufreq: Driver did not initialize current freq\n");
+		policy->cur = new_freq;
+	} else if (policy->cur != new_freq && has_target()) {
+		cpufreq_out_of_sync(policy, new_freq);
+	}
+
+	return new_freq;
+}
+
 static struct subsys_interface cpufreq_interface = {
 	.name		= "cpufreq",
 	.subsys		= &cpu_subsys,
@@ -2264,19 +2282,11 @@ int cpufreq_update_policy(unsigned int c
 	 * -> ask driver for current freq and notify governors about a change
 	 */
 	if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
-		new_policy.cur = cpufreq_driver->get(cpu);
+		new_policy.cur = cpufreq_update_current_freq(policy);
 		if (WARN_ON(!new_policy.cur)) {
 			ret = -EIO;
 			goto unlock;
 		}
-
-		if (!policy->cur) {
-			pr_debug("Driver did not initialize current freq\n");
-			policy->cur = new_policy.cur;
-		} else {
-			if (policy->cur != new_policy.cur && has_target())
-				cpufreq_out_of_sync(policy, new_policy.cur);
-		}
 	}
 
 	ret = cpufreq_set_policy(policy, &new_policy);

Re: [PATCH 2/3] cpufreq: Introduce cpufreq_update_current_freq()

[Viresh Kumar]

On 21-03-16, 15:46, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Move the part of cpufreq_update_policy() that obtains the current
> frequency from the driver and updates policy->cur if necessary to
> a separate function, cpufreq_get_current_freq().
> 
> That should not introduce functional changes and subsequent change
> set will need it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)

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

-- 
viresh

[PATCH 3/3] cpufreq: Always update current frequency before startig governor

[Rafael J. Wysocki]

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

Make policy->cur match the current frequency returned by the driver's
->get() callback before starting the governor in case they went out of
sync in the meantime and drop the piece of code attempting to
resync policy->cur with the real frequency of the boot CPU from
cpufreq_resume() as it serves no purpose any more (and it's racy and
super-ugly anyway).

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

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1680,17 +1680,6 @@ void cpufreq_resume(void)
 				       __func__, policy);
 		}
 	}
-
-	/*
-	 * schedule call cpufreq_update_policy() for first-online CPU, as that
-	 * wouldn't be hotplugged-out on suspend. It will verify that the
-	 * current freq is in sync with what we believe it to be.
-	 */
-	policy = cpufreq_cpu_get_raw(cpumask_first(cpu_online_mask));
-	if (WARN_ON(!policy))
-		return;
-
-	schedule_work(&policy->update);
 }
 
 /**
@@ -2062,6 +2051,9 @@ static int cpufreq_start_governor(struct
 {
 	int ret;
 
+	if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
+		cpufreq_update_current_freq(policy);
+
 	ret = cpufreq_governor(policy, CPUFREQ_GOV_START);
 	return ret ? ret : cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 }

Re: [PATCH 3/3] cpufreq: Always update current frequency before startig governor

[Viresh Kumar]

On 21-03-16, 15:47, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make policy->cur match the current frequency returned by the driver's
> ->get() callback before starting the governor in case they went out of
> sync in the meantime and drop the piece of code attempting to
> resync policy->cur with the real frequency of the boot CPU from
> cpufreq_resume() as it serves no purpose any more (and it's racy and
> super-ugly anyway).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -1680,17 +1680,6 @@ void cpufreq_resume(void)
>  				       __func__, policy);
>  		}
>  	}
> -
> -	/*
> -	 * schedule call cpufreq_update_policy() for first-online CPU, as that
> -	 * wouldn't be hotplugged-out on suspend. It will verify that the
> -	 * current freq is in sync with what we believe it to be.
> -	 */
> -	policy = cpufreq_cpu_get_raw(cpumask_first(cpu_online_mask));
> -	if (WARN_ON(!policy))
> -		return;
> -
> -	schedule_work(&policy->update);
>  }
>  
>  /**
> @@ -2062,6 +2051,9 @@ static int cpufreq_start_governor(struct
>  {
>  	int ret;
>  
> +	if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
> +		cpufreq_update_current_freq(policy);
> +
>  	ret = cpufreq_governor(policy, CPUFREQ_GOV_START);
>  	return ret ? ret : cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
>  }

This looks fine, but I am searching for answers to few doubts, maybe
you can help..

Why we did the same in process context earlier? And why it wouldn't be
a problem now, when we do it in interrupt context? Will IRQs be
disabled here? If so, then will you hit following ?

static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
		struct cpufreq_freqs *freqs, unsigned int state)
{
	BUG_ON(irqs_disabled());

...
}

And will calling notifiers from interrupt-context just fine ?

-- 
viresh

Re: [PATCH 3/3] cpufreq: Always update current frequency before startig governor

[Rafael J. Wysocki]

On Tuesday, March 22, 2016 09:00:32 AM Viresh Kumar wrote:
> On 21-03-16, 15:47, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Make policy->cur match the current frequency returned by the driver's
> > ->get() callback before starting the governor in case they went out of
> > sync in the meantime and drop the piece of code attempting to
> > resync policy->cur with the real frequency of the boot CPU from
> > cpufreq_resume() as it serves no purpose any more (and it's racy and
> > super-ugly anyway).
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c |   14 +++-----------
> >  1 file changed, 3 insertions(+), 11 deletions(-)
> > 
> > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > @@ -1680,17 +1680,6 @@ void cpufreq_resume(void)
> >  				       __func__, policy);
> >  		}
> >  	}
> > -
> > -	/*
> > -	 * schedule call cpufreq_update_policy() for first-online CPU, as that
> > -	 * wouldn't be hotplugged-out on suspend. It will verify that the
> > -	 * current freq is in sync with what we believe it to be.
> > -	 */
> > -	policy = cpufreq_cpu_get_raw(cpumask_first(cpu_online_mask));
> > -	if (WARN_ON(!policy))
> > -		return;
> > -
> > -	schedule_work(&policy->update);
> >  }
> >  
> >  /**
> > @@ -2062,6 +2051,9 @@ static int cpufreq_start_governor(struct
> >  {
> >  	int ret;
> >  
> > +	if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
> > +		cpufreq_update_current_freq(policy);
> > +
> >  	ret = cpufreq_governor(policy, CPUFREQ_GOV_START);
> >  	return ret ? ret : cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> >  }
> 
> This looks fine, but I am searching for answers to few doubts, maybe
> you can help..
> 
> Why we did the same in process context earlier? And why it wouldn't be
> a problem now, when we do it in interrupt context? Will IRQs be
> disabled here? If so, then will you hit following ?

I'm not sure I'm following.

This is process context too.

Look at the call sites of cpufreq_start_governor() (patch [1/3]):
- cpufreq_offline() - process context
- cpufreq_resume() - process context
- cpufreq_set_policy() - process context
- cpufreq_add_policy_cpu() - process context

Besides, calling cpufreq_governor() from interrupt context wouldn't reall work,
because that acquires mutexes etc, like in cpufreq_governor_init().

> static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
> 		struct cpufreq_freqs *freqs, unsigned int state)
> {
> 	BUG_ON(irqs_disabled());
> 
> ...
> }
> 
> And will calling notifiers from interrupt-context just fine ?

If your question is why the original code doesn't call cpufreq_update_policy()
directly, I think the reason is because cpufreq_resume() used to be one of the
syscore ops and *that* would have been run in interrupt context.

Thanks,
Rafael

Re: [PATCH 3/3] cpufreq: Always update current frequency before startig governor

[Viresh Kumar]

On 22-03-16, 15:33, Rafael J. Wysocki wrote:
> On Tuesday, March 22, 2016 09:00:32 AM Viresh Kumar wrote:

> > Why we did the same in process context earlier? And why it wouldn't be
> > a problem now, when we do it in interrupt context? Will IRQs be
> > disabled here? If so, then will you hit following ?
> 
> I'm not sure I'm following.

Sorry about that.

> This is process context too.
> 
> Look at the call sites of cpufreq_start_governor() (patch [1/3]):
> - cpufreq_offline() - process context
> - cpufreq_resume() - process context

I somehow thought that this is going to happen in interrupt context. :(

> - cpufreq_set_policy() - process context
> - cpufreq_add_policy_cpu() - process context
> 
> Besides, calling cpufreq_governor() from interrupt context wouldn't reall work,
> because that acquires mutexes etc, like in cpufreq_governor_init().
> 
> > static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
> > 		struct cpufreq_freqs *freqs, unsigned int state)
> > {
> > 	BUG_ON(irqs_disabled());
> > 
> > ...
> > }
> > 
> > And will calling notifiers from interrupt-context just fine ?
> 
> If your question is why the original code doesn't call cpufreq_update_policy()
> directly, I think the reason is because cpufreq_resume() used to be one of the
> syscore ops and *that* would have been run in interrupt context.

Yeah.

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

-- 
viresh