[PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

Commit 7c30ed ("cpufreq: make sure frequency transitions are serialized")
interacts poorly with systems that have a single core freqency for all
cores.  On such systems we have a single policy for all cores with
several CPUs.  When we do a frequency transition the governor calls the
pre and post change notifiers which causes cpufreq_notify_transition()
per CPU.  Since the policy is the same for all of them all CPUs after
the first and the warnings added are generated by checking a per-policy
flag the warnings will be triggered for all cores after the first.

Fix this by simply reverting commit 7c30ed, though it appears that the
desired behaviour can also be achieved by moving the checks (and
probably some of the other code) out of the per-cpu callback into the
main cpufreq_notify_transition().

Signed-off-by: Mark Brown <broonie@linaro.org>
---

I'll try to have a look at doing the code motion fix tomorrow if that's
more suitable, the check seems like a good idea and looking at the code
it's probably sensible to factor out more of the changes.

 drivers/cpufreq/cpufreq.c | 14 --------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b557503..0b3076c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -312,12 +312,6 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
-		if (WARN(policy->transition_ongoing,
-				"In middle of another frequency transition\n"))
-			return;
-
-		policy->transition_ongoing = true;
-
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
 		 * "old frequency".
@@ -337,12 +331,6 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 		break;
 
 	case CPUFREQ_POSTCHANGE:
-		if (WARN(!policy->transition_ongoing,
-				"No frequency transition in progress\n"))
-			return;
-
-		policy->transition_ongoing = false;
-
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
 			(unsigned long)freqs->cpu);
@@ -1552,8 +1540,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 
 	if (cpufreq_disabled())
 		return -ENODEV;
-	if (policy->transition_ongoing)
-		return -EBUSY;
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d7390b..5fedb6c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -119,7 +119,6 @@ struct cpufreq_policy {
 
 	struct kobject		kobj;
 	struct completion	kobj_unregister;
-	bool			transition_ongoing; /* Tracks transition status */
 };
 
 #define CPUFREQ_ADJUST			(0)
-- 
1.8.3.1

Re: [PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]

On 2 July 2013 05:47, Mark Brown <broonie@kernel.org> wrote:
> I'll try to have a look at doing the code motion fix tomorrow if that's
> more suitable, the check seems like a good idea and looking at the code
> it's probably sensible to factor out more of the changes.

So sorry for this, The time I tried it, was booting it with only one cpu
per cluster for some reason :(

Please see if below fixes it for you. Use attached patch for testing it.

------x------------------x-------------

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 2 Jul 2013 10:41:54 +0530
Subject: [PATCH] cpufreq: Fix serialization of frequency transitions

Commit 7c30ed ("cpufreq: make sure frequency transitions are serialized")
interacts poorly with systems that have a single core freqency for all
cores.  On such systems we have a single policy for all cores with
several CPUs.  When we do a frequency transition the governor calls the
pre and post change notifiers which causes cpufreq_notify_transition()
per CPU.  Since the policy is the same for all of them all CPUs after
the first and the warnings added are generated by checking a per-policy
flag the warnings will be triggered for all cores after the first.

Fix this by allowing notifier to be called for n times. Where n is the number of
cpus in policy->cpus.

Reported-by: Mark Brown <broonie@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 75715f1..438dcee 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -264,11 +264,12 @@ void __cpufreq_notify_transition(struct
cpufreq_policy *policy,
 	switch (state) {

 	case CPUFREQ_PRECHANGE:
-		if (WARN(policy->transition_ongoing,
+		if (WARN(policy->transition_ongoing ==
+					cpumask_weight(policy->cpus),
 				"In middle of another frequency transition\n"))
 			return;

-		policy->transition_ongoing = true;
+		policy->transition_ongoing++;

 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
@@ -293,7 +294,7 @@ void __cpufreq_notify_transition(struct
cpufreq_policy *policy,
 				"No frequency transition in progress\n"))
 			return;

-		policy->transition_ongoing = false;
+		policy->transition_ongoing--;

 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,

[-- Attachment #2: 0001-cpufreq-Fix-serialization-of-frequency-transitions.patch --]
[-- Type: application/octet-stream, Size: 2209 bytes --]

From 4423bc2fade3a7de807a99d8b8894a531cae4b81 Mon Sep 17 00:00:00 2001
Message-Id: <4423bc2fade3a7de807a99d8b8894a531cae4b81.1372742035.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 2 Jul 2013 10:41:54 +0530
Subject: [PATCH] cpufreq: Fix serialization of frequency transitions

Commit 7c30ed ("cpufreq: make sure frequency transitions are serialized")
interacts poorly with systems that have a single core freqency for all
cores.  On such systems we have a single policy for all cores with
several CPUs.  When we do a frequency transition the governor calls the
pre and post change notifiers which causes cpufreq_notify_transition()
per CPU.  Since the policy is the same for all of them all CPUs after
the first and the warnings added are generated by checking a per-policy
flag the warnings will be triggered for all cores after the first.

Fix this by allowing notifier to be called for n times. Where n is the number of
cpus in policy->cpus.

Reported-by: Mark Brown <broonie@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 75715f1..438dcee 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -264,11 +264,12 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
-		if (WARN(policy->transition_ongoing,
+		if (WARN(policy->transition_ongoing ==
+					cpumask_weight(policy->cpus),
 				"In middle of another frequency transition\n"))
 			return;
 
-		policy->transition_ongoing = true;
+		policy->transition_ongoing++;
 
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
@@ -293,7 +294,7 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 				"No frequency transition in progress\n"))
 			return;
 
-		policy->transition_ongoing = false;
+		policy->transition_ongoing--;
 
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

On Tue, Jul 02, 2013 at 10:45:02AM +0530, Viresh Kumar wrote:

> Please see if below fixes it for you. Use attached patch for testing it.

This doesn't apply terribly easily, the patch is corrupt and appears to
be based on something other than what's in -next so neither git am nor
patch can figure out how to apply it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

On 2 July 2013 15:18, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 02, 2013 at 10:45:02AM +0530, Viresh Kumar wrote:
>
>> Please see if below fixes it for you. Use attached patch for testing it.
>
> This doesn't apply terribly easily, the patch is corrupt and appears to
> be based on something other than what's in -next so neither git am nor
> patch can figure out how to apply it.

Two things,

- I rebased my patch now against latest linux-next/master and it applied
  cleanly.
- Somehow one of my changes to include/linux/cpufreq.h didn't get commited

Try attached patch now and it should really work..

I am using gmail for replying to mails from office which corrupts mails. And
I can't use other mail clients from office, firewall issues.

--------x---------------x------------
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 2 Jul 2013 10:41:54 +0530
Subject: [PATCH] cpufreq: Fix serialization of frequency transitions

Commit 7c30ed ("cpufreq: make sure frequency transitions are serialized")
interacts poorly with systems that have a single core freqency for all
cores.  On such systems we have a single policy for all cores with
several CPUs.  When we do a frequency transition the governor calls the
pre and post change notifiers which causes cpufreq_notify_transition()
per CPU.  Since the policy is the same for all of them all CPUs after
the first and the warnings added are generated by checking a per-policy
flag the warnings will be triggered for all cores after the first.

Fix this by allowing notifier to be called for n times. Where n is the number of
cpus in policy->cpus.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 ++++---
 include/linux/cpufreq.h   | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b557503..b7bda8d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -312,11 +312,12 @@ static void __cpufreq_notify_transition(struct
cpufreq_policy *policy,
 	switch (state) {

 	case CPUFREQ_PRECHANGE:
-		if (WARN(policy->transition_ongoing,
+		if (WARN(policy->transition_ongoing ==
+					cpumask_weight(policy->cpus),
 				"In middle of another frequency transition\n"))
 			return;

-		policy->transition_ongoing = true;
+		policy->transition_ongoing++;

 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
@@ -341,7 +342,7 @@ static void __cpufreq_notify_transition(struct
cpufreq_policy *policy,
 				"No frequency transition in progress\n"))
 			return;

-		policy->transition_ongoing = false;
+		policy->transition_ongoing--;

 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d7390b..90d5a15 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -119,7 +119,7 @@ struct cpufreq_policy {

 	struct kobject		kobj;
 	struct completion	kobj_unregister;
-	bool			transition_ongoing; /* Tracks transition status */
+	int			transition_ongoing; /* Tracks transition status */
 };

 #define CPUFREQ_ADJUST			(0)

[-- Attachment #2: 0001-cpufreq-Fix-serialization-of-frequency-transitions.patch --]
[-- Type: application/octet-stream, Size: 2628 bytes --]

From d367fc6fda0f167678a83e1d70e0cfd240a32cbc Mon Sep 17 00:00:00 2001
Message-Id: <d367fc6fda0f167678a83e1d70e0cfd240a32cbc.1372759022.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 2 Jul 2013 10:41:54 +0530
Subject: [PATCH] cpufreq: Fix serialization of frequency transitions

Commit 7c30ed ("cpufreq: make sure frequency transitions are serialized")
interacts poorly with systems that have a single core freqency for all
cores.  On such systems we have a single policy for all cores with
several CPUs.  When we do a frequency transition the governor calls the
pre and post change notifiers which causes cpufreq_notify_transition()
per CPU.  Since the policy is the same for all of them all CPUs after
the first and the warnings added are generated by checking a per-policy
flag the warnings will be triggered for all cores after the first.

Fix this by allowing notifier to be called for n times. Where n is the number of
cpus in policy->cpus.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 ++++---
 include/linux/cpufreq.h   | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b557503..b7bda8d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -312,11 +312,12 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
-		if (WARN(policy->transition_ongoing,
+		if (WARN(policy->transition_ongoing ==
+					cpumask_weight(policy->cpus),
 				"In middle of another frequency transition\n"))
 			return;
 
-		policy->transition_ongoing = true;
+		policy->transition_ongoing++;
 
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
@@ -341,7 +342,7 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 				"No frequency transition in progress\n"))
 			return;
 
-		policy->transition_ongoing = false;
+		policy->transition_ongoing--;
 
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d7390b..90d5a15 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -119,7 +119,7 @@ struct cpufreq_policy {
 
 	struct kobject		kobj;
 	struct completion	kobj_unregister;
-	bool			transition_ongoing; /* Tracks transition status */
+	int			transition_ongoing; /* Tracks transition status */
 };
 
 #define CPUFREQ_ADJUST			(0)
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

On Tue, Jul 02, 2013 at 03:32:34PM +0530, Viresh Kumar wrote:
> On 2 July 2013 15:18, Mark Brown <broonie@kernel.org> wrote:

> > This doesn't apply terribly easily, the patch is corrupt and appears to
> > be based on something other than what's in -next so neither git am nor
> > patch can figure out how to apply it.

> Two things,

> - I rebased my patch now against latest linux-next/master and it applied
>   cleanly.
> - Somehow one of my changes to include/linux/cpufreq.h didn't get commited

> Try attached patch now and it should really work..

That seems to work, yes.

Tested-by: Mark Brown <broonie@linaro.org>

> I am using gmail for replying to mails from office which corrupts mails. And
> I can't use other mail clients from office, firewall issues.

If you're forced to do stuff like that then don't include the patch in
the body of the e-mail, it just makes everything worse.  Not only is the
copy in the body of the e-mail broken but there's now two copies so
tools like git am get even more upset as they may potentially see some
of the hunks twice.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[Viresh Kumar]

On 2 July 2013 15:59, Mark Brown <broonie@kernel.org> wrote:
> That seems to work, yes.
>
> Tested-by: Mark Brown <broonie@linaro.org>

Thanks.

@Rafael: I will send it separately to you.

> If you're forced to do stuff like that then don't include the patch in
> the body of the e-mail, it just makes everything worse.  Not only is the
> copy in the body of the e-mail broken but there's now two copies so
> tools like git am get even more upset as they may potentially see some
> of the hunks twice.

I do it so that people can review it. I still believe its important.

Re: [PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On Tue, Jul 02, 2013 at 04:02:32PM +0530, Viresh Kumar wrote:
> On 2 July 2013 15:59, Mark Brown <broonie@kernel.org> wrote:

> > If you're forced to do stuff like that then don't include the patch in
> > the body of the e-mail, it just makes everything worse.  Not only is the
> > copy in the body of the e-mail broken but there's now two copies so
> > tools like git am get even more upset as they may potentially see some
> > of the hunks twice.

> I do it so that people can review it. I still believe its important.

If your patch is sent as a text/plain attachment most mail clients will
tend to do the right thing with replies, at least the good ones.  Or
just figure out a VPN or something to get round the firewall issues,
mobile broadband is often a good technique if there's no carry
restrictions (usually phones are allowed...).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] cpufreq: Fix runtime warnings on systems with shared core clocks

[Viresh Kumar]

On 2 July 2013 16:45, Mark Brown <broonie@kernel.org> wrote:
> If your patch is sent as a text/plain attachment most mail clients will
> tend to do the right thing with replies, at least the good ones.  Or
> just figure out a VPN or something to get round the firewall issues,
> mobile broadband is often a good technique if there's no carry
> restrictions (usually phones are allowed...).

I used mobile only for sending the last patch to Rafael.. But running
thunderbird over it is a bit heavy :)