CPUFreq: Fix {PRE|POST}CHANGE notification sequence

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This isn't obeyed by many of the drivers and core wasn't forcing it.

This patchset first fixes all the driver to follow it strictly and then adds some protection against this. Now, we keep track of the last transaction and see if something went wrong.

Last patch is intentionally kept towards the end, so that git bisect still works well for all the faulty drivers.

This is pushed here and a pull request for 3.11 would be sent after few days.

git://git.linaro.org/people/vireshk/linux.git cpufreq-fix-notification

Viresh Kumar (13):
cpufreq: acpi: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: arm-big-little: call CPUFREQ_POSTCHANGE notfier in error
cases
cpufreq: davinci: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: dbx500: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: e_powersave: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: exynos: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: imx6q: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: omap: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: pcc: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: powernow-k8: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: s3c64xx: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: tegra: call CPUFREQ_POSTCHANGE notfier in error cases
cpufreq: make sure frequency transitions are serialized

 drivers/cpufreq/acpi-cpufreq.c    |  6 ++++--
 drivers/cpufreq/arm_big_little.c  |  4 +---
 drivers/cpufreq/cpufreq.c         |  9 +++++++++
 drivers/cpufreq/davinci-cpufreq.c |  3 +++
 drivers/cpufreq/dbx500-cpufreq.c  |  4 ++--
 drivers/cpufreq/e_powersaver.c    |  3 +++
 drivers/cpufreq/exynos-cpufreq.c  | 10 ++++++++--
 drivers/cpufreq/imx6q-cpufreq.c   | 17 +++++++++++------
 drivers/cpufreq/omap-cpufreq.c    |  6 +++---
 drivers/cpufreq/pcc-cpufreq.c     |  2 ++
 drivers/cpufreq/powernow-k8.c     |  6 +++---
 drivers/cpufreq/s3c64xx-cpufreq.c |  8 ++++++--
 drivers/cpufreq/tegra-cpufreq.c   |  4 ++--
 13 files changed, 57 insertions(+), 25 deletions(-)

1.7.12.rc2.18.g61b472e

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

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

index edc089e..223ddf4 100644
@@ -494,12 +494,14 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy,
 			pr_debug("acpi_cpufreq_target failed (%d)\n",
 				policy->cpu);
 			result = -EAGAIN;
-			goto out;
+			freqs.new = freqs.old;
 		}
 	}
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
-	perf->state = next_perf_state;
+
+	if (!result)
+		perf->state = next_perf_state;
 
 out:
 	return result;

1.7.12.rc2.18.g61b472e

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

This also removes code setting policy->cur as this is also done by POSTCHANGE notifier.

---
 drivers/cpufreq/arm_big_little.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 5d7f53f..3549f07 100644
@@ -84,11 +84,9 @@ static int bL_cpufreq_set_target(struct cpufreq_policy *policy,
 	ret = clk_set_rate(clk[cur_cluster], freqs.new * 1000);
 	if (ret) {
 		pr_err("clk_set_rate failed: %d\n", ret);
-		return ret;
+		freqs.new = freqs.old;
 	}
 
-	policy->cur = freqs.new;
-
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 
 	return ret;

1.7.12.rc2.18.g61b472e

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

Davinci driver was taking care of it but frequency isn't restored to freqs.old.

This patch fixes it.

---
 drivers/cpufreq/davinci-cpufreq.c | 3 +++
 1 file changed, 3 insertions(+)

index c33c76c..551dd65 100644
@@ -114,6 +114,9 @@ static int davinci_target(struct cpufreq_policy *policy,
 		pdata->set_voltage(idx);
 
 out:
+	if (ret)
+		freqs.new = freqs.old;
+
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 
 	return ret;

1.7.12.rc2.18.g61b472e

Looks good to me.

Thanks,
Sekhar

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

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

index 6ec6539..1fdb02b 100644
@@ -57,13 +57,13 @@ static int dbx500_cpufreq_target(struct cpufreq_policy *policy,
 	if (ret) {
 		pr_err("dbx500-cpufreq: Failed to set armss_clk to %d Hz: error %d\n",
 		       freqs.new * 1000, ret);
-		return ret;
+		freqs.new = freqs.old;
 	}
 
 	/* post change notification */
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 
-	return 0;
+	return ret;
 }
 
 static unsigned int dbx500_cpufreq_getspeed(unsigned int cpu)

1.7.12.rc2.18.g61b472e

Looks good to me.

Yours,
Linus Walleij

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This driver was taking care of it but frequency isn't restored to freqs.old.

This patch fixes it.

---
 drivers/cpufreq/e_powersaver.c | 3 +++
 1 file changed, 3 insertions(+)

index 37380fb..3d03626 100644
@@ -161,6 +161,9 @@ postchange:
 		current_multiplier);
 	}
 #endif
+	if (err)
+		freqs.new = freqs.old;
+
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 	return err;
 }

1.7.12.rc2.18.g61b472e

I have no objections to this change but at the same time I don't feel that I know the code well enough to review it.

Probably I made a mistake adding your name. Don't know how I made up my mind that you are the main contributor for this.

Probably Dave is?

e_powersaver ? Rafael.

Dave

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

---
 drivers/cpufreq/exynos-cpufreq.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 475b4f6..0d32f02 100644
@@ -113,7 +113,8 @@ static int exynos_cpufreq_scale(unsigned int target_freq)
 		if (ret) {
 			pr_err("%s: failed to set cpu voltage to %d\n",
 				__func__, arm_volt);
-			goto out;
+			freqs.new = freqs.old;
+			goto post_notify;
 		}
 	}
 

@@ -123,14 +124,19 @@ static int exynos_cpufreq_scale(unsigned int target_freq)
 		if (ret) {
 			pr_err("%s: failed to set cpu voltage to %d\n",
 				__func__, safe_arm_volt);
-			goto out;
+			freqs.new = freqs.old;
+			goto post_notify;
 		}
 	}
 
 	exynos_info->set_freq(old_index, index);
 
+post_notify:
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 
+	if (ret)
+		goto out;
+
 	/* When the new frequency is lower than current frequency */
 	if ((freqs.new < freqs.old) ||
 	   ((freqs.new > freqs.old) && safe_arm_volt)) {

1.7.12.rc2.18.g61b472e

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

This also moves PRECHANGE notifier down so that we call it just before starting frequency transition.

---
 drivers/cpufreq/imx6q-cpufreq.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

index b78bc35..e37cdae 100644
@@ -68,8 +68,6 @@ static int imx6q_set_target(struct cpufreq_policy *policy,
 	if (freqs.old == freqs.new)
 		return 0;
 
-	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
-
 	rcu_read_lock();
 	opp = opp_find_freq_ceil(cpu_dev, &freq_hz);
 	if (IS_ERR(opp)) {

@@ -86,13 +84,16 @@ static int imx6q_set_target(struct cpufreq_policy *policy,
 		freqs.old / 1000, volt_old / 1000,
 		freqs.new / 1000, volt / 1000);
 
+	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
+
 	/* scaling up?  scale voltage before frequency */
 	if (freqs.new > freqs.old) {
 		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
 		if (ret) {
 			dev_err(cpu_dev,
 				"failed to scale vddarm up: %d\n", ret);
-			return ret;
+			freqs.new = freqs.old;
+			goto post_notify;
 		}
 
 		/*

@@ -145,15 +146,18 @@ static int imx6q_set_target(struct cpufreq_policy *policy,
 	if (ret) {
 		dev_err(cpu_dev, "failed to set clock rate: %d\n", ret);
 		regulator_set_voltage_tol(arm_reg, volt_old, 0);
-		return ret;
+		freqs.new = freqs.old;
+		goto post_notify;
 	}
 
 	/* scaling down?  scale voltage after frequency */
 	if (freqs.new < freqs.old) {
 		ret = regulator_set_voltage_tol(arm_reg, volt, 0);
-		if (ret)
+		if (ret) {
 			dev_warn(cpu_dev,
 				 "failed to scale vddarm down: %d\n", ret);
+			ret = 0;
+		}
 
 		if (freqs.old == FREQ_1P2_GHZ / 1000) {
 			regulator_set_voltage_tol(pu_reg,

@@ -163,9 +167,10 @@ static int imx6q_set_target(struct cpufreq_policy *policy,
 		}
 	}
 
+post_notify:
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 
-	return 0;
+	return ret;
 }
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)

1.7.12.rc2.18.g61b472e

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

Omap driver was taking care of it well, but wasn't restoring freqs.new to freqs.old in some cases. I wasn't required to add code for it as moving PRECHANGE notifier down was a better option, so that we call it just before starting frequency transition.

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

index 0279d18..29468a5 100644
@@ -93,9 +93,6 @@ static int omap_target(struct cpufreq_policy *policy,
 	if (freqs.old == freqs.new && policy->cur == freqs.new)
 		return ret;
 
-	/* notifiers */
-	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
-
 	freq = freqs.new * 1000;
 	ret = clk_round_rate(mpu_clk, freq);
 	if (IS_ERR_VALUE(ret)) {

@@ -125,6 +122,9 @@ static int omap_target(struct cpufreq_policy *policy,
 		freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
 		freqs.new / 1000, volt ? volt / 1000 : -1);
 
+	/* notifiers */
+	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
+
 	/* scaling up?  scale voltage before frequency */
 	if (mpu_reg && (freqs.new > freqs.old)) {
 		r = regulator_set_voltage(mpu_reg, volt - tol, volt + tol);

1.7.12.rc2.18.g61b472e

Looks safe to me.

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

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

index 0de0008..1581fcc4 100644
@@ -243,6 +243,8 @@ static int pcc_cpufreq_target(struct cpufreq_policy *policy,
 	return 0;
 
 cmd_incomplete:
+	freqs.new = freqs.old;
+	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 	iowrite16(0, &pcch_hdr->status);
 	spin_unlock(&pcc_lock);
 	return -EINVAL;

1.7.12.rc2.18.g61b472e

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

---
 drivers/cpufreq/powernow-k8.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index b828efe..c916320 100644
@@ -967,9 +967,9 @@ static int transition_frequency_fidvid(struct powernow_k8_data *data,
 
 	res = transition_fid_vid(data, fid, vid);
 	if (res)
-		return res;
-
-	freqs.new = find_khz_freq_from_fid(data->currfid);
+		freqs.new = freqs.old;
+	else
+		freqs.new = find_khz_freq_from_fid(data->currfid);
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 	return res;

1.7.12.rc2.18.g61b472e

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

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

index 27cacb5..017ade0 100644
@@ -104,7 +104,8 @@ static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 		if (ret != 0) {
 			pr_err("Failed to set VDDARM for %dkHz: %d\n",
 			       freqs.new, ret);
-			goto err;
+			freqs.new = freqs.old;
+			goto post_notify;
 		}
 	}
 #endif

@@ -113,10 +114,13 @@ static int s3c64xx_cpufreq_set_target(struct cpufreq_policy *policy,
 	if (ret < 0) {
 		pr_err("Failed to set rate %dkHz: %d\n",
 		       freqs.new, ret);
-		goto err;
+		freqs.new = freqs.old;
 	}
 
+post_notify:
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
+	if (ret)
+		goto err;
 
 #ifdef CONFIG_REGULATOR
 	if (vddarm && freqs.new < freqs.old) {

1.7.12.rc2.18.g61b472e

PRECHANGE and POSTCHANGE notifiers must be called in groups, i.e either both should be called or both shouldn't be.

In case we have started PRECHANGE notifier and found an error, we must call POSTCHANGE notifier with freqs.new = freqs.old to guarantee that sequence of calling notifiers is complete.

This patch fixes it.

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

index c74c0e1..e74d777 100644
@@ -138,12 +138,12 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
 	if (ret) {
 		pr_err("cpu-tegra: Failed to set cpu frequency to %d kHz\n",
 			freqs.new);
-		return ret;
+		freqs.new = freqs.old;
 	}
 
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
 
-	return 0;
+	return ret;
 }
 
 static unsigned long tegra_cpu_highest_speed(void)

1.7.12.rc2.18.g61b472e

Whenever we are changing frequency of a cpu, we are calling PRECHANGE and POSTCHANGE notifiers. They must be serialized. i.e. PRECHANGE or POSTCHANGE shouldn't be called twice contiguously.

This can happen due to bugs in users of __cpufreq_driver_target() or actual cpufreq drivers who are sending these notifiers.

This patch adds some protection against this. Now, we keep track of the last transaction and see if something went wrong.

---
 drivers/cpufreq/cpufreq.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index 2d53f47..92cb8b3 100644
@@ -107,6 +107,9 @@ static void handle_update(struct work_struct *work);
 static BLOCKING_NOTIFIER_HEAD(cpufreq_policy_notifier_list);
 static struct srcu_notifier_head cpufreq_transition_notifier_list;
 
+/* Tracks status of transition */
+static int transition_ongoing;
+
 static bool init_cpufreq_transition_notifier_list_called;
 static int __init init_cpufreq_transition_notifier_list(void)
 {

@@ -264,6 +267,8 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
+		WARN_ON(transition_ongoing++);
+
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
 		 * "old frequency".

@@ -283,6 +288,8 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 		break;
 
 	case CPUFREQ_POSTCHANGE:
+		WARN_ON(!transition_ongoing--);
+
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
 			(unsigned long)freqs->cpu);

@@ -1458,6 +1465,8 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 
 	if (cpufreq_disabled())
 		return -ENODEV;
+	if (transition_ongoing)
+		return -EBUSY;
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)

1.7.12.rc2.18.g61b472e

What about this patch? Find it attached to apply.

index 2d53f47..6624694 100644
@@ -107,6 +107,9 @@ static void handle_update(struct work_struct *work);
 static BLOCKING_NOTIFIER_HEAD(cpufreq_policy_notifier_list);
 static struct srcu_notifier_head cpufreq_transition_notifier_list;
 
+/* Tracks status of transition */
+static int transition_ongoing;
+
 static bool init_cpufreq_transition_notifier_list_called;
 static int __init init_cpufreq_transition_notifier_list(void)
 {

@@ -264,6 +267,13 @@ void __cpufreq_notify_transition(struct

cpufreq_policy *policy,

        switch (state) {

        case CPUFREQ_PRECHANGE:

+ if (transition_ongoing) {
+ WARN(1, "In middle of another frequency transition\n");
+ return;
+ }
+
+ transition_ongoing++;
+
/* detect if the driver reported a value as "old frequency"
* which is not equal to what the cpufreq core thinks is
* "old frequency".
@@ -283,6 +293,13 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,

                break;

        case CPUFREQ_POSTCHANGE:

+ if (!transition_ongoing) {
+ WARN(1, "No frequency transition in progress\n");
+ return;
+ }
+
+ transition_ongoing--;
+

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

@@ -1458,6 +1475,8 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,

        if (cpufreq_disabled())
                return -ENODEV;

+ if (transition_ongoing)
+ return -EBUSY;

        /* Make sure that target_freq is within supported range */
        if (target_freq > policy->max)

From 25fb6e6164b107f1674ec32f0d0e47328c2c11a6 Mon Sep 17 00:00:00 2001
Message-Id: <25fb6e6164b107f1674ec32f0d0e47328c2c11a6.1372079234.git.viresh.kumar@linaro.org>
From: Viresh Kumar <[email protected]>
Date: Wed, 19 Jun 2013 10:16:55 +0530
Subject: [PATCH] cpufreq: make sure frequency transitions are serialized

Whenever we are changing frequency of a cpu, we are calling PRECHANGE and POSTCHANGE notifiers. They must be serialized. i.e. PRECHANGE or POSTCHANGE shouldn't be called twice contiguously.

This can happen due to bugs in users of __cpufreq_driver_target() or actual cpufreq drivers who are sending these notifiers.

This patch adds some protection against this. Now, we keep track of the last transaction and see if something went wrong.

---
 drivers/cpufreq/cpufreq.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

index 2d53f47..6624694 100644
@@ -107,6 +107,9 @@ static void handle_update(struct work_struct *work);
 static BLOCKING_NOTIFIER_HEAD(cpufreq_policy_notifier_list);
 static struct srcu_notifier_head cpufreq_transition_notifier_list;
 
+/* Tracks status of transition */
+static int transition_ongoing;
+
 static bool init_cpufreq_transition_notifier_list_called;
 static int __init init_cpufreq_transition_notifier_list(void)
 {

@@ -264,6 +267,13 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
+		if (transition_ongoing) {
+			WARN(1, "In middle of another frequency transition\n");
+			return;
+		}
+
+		transition_ongoing++;
+
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
 		 * "old frequency".

@@ -283,6 +293,13 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 		break;
 
 	case CPUFREQ_POSTCHANGE:
+		if (!transition_ongoing) {
+			WARN(1, "No frequency transition in progress\n");
+			return;
+		}
+
+		transition_ongoing--;
+
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
 			(unsigned long)freqs->cpu);

@@ -1458,6 +1475,8 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 
 	if (cpufreq_disabled())
 		return -ENODEV;
+	if (transition_ongoing)
+		return -EBUSY;
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)

1.7.12.rc2.18.g61b472e

Ahh.. stupid code.. Check if this fixup is fine (attached too)

index 6624694..6ca7eac 100644
@@ -267,10 +267,9 @@ void __cpufreq_notify_transition(struct

cpufreq_policy *policy,

        switch (state) {

        case CPUFREQ_PRECHANGE:

- if (transition_ongoing) {
- WARN(1, "In middle of another frequency transition\n");
+ if (WARN(transition_ongoing,
+ "In middle of another frequency transition\n"))

                        return;

- }

                transition_ongoing++;

@@ -293,10 +292,9 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,

                break;

        case CPUFREQ_POSTCHANGE:

- if (!transition_ongoing) {
- WARN(1, "No frequency transition in progress\n");
+ if (WARN(!transition_ongoing,
+ "No frequency transition in progress\n"))

                        return;

- }

                transition_ongoing--;

From 01a85c5e860f4b52a09418559fd27f480357272f Mon Sep 17 00:00:00 2001
Message-Id: <01a85c5e860f4b52a09418559fd27f480357272f.1372079781.git.viresh.kumar@linaro.org>
From: Viresh Kumar <[email protected]>
Date: Wed, 19 Jun 2013 10:16:55 +0530
Subject: [PATCH] cpufreq: make sure frequency transitions are serialized

Whenever we are changing frequency of a cpu, we are calling PRECHANGE and POSTCHANGE notifiers. They must be serialized. i.e. PRECHANGE or POSTCHANGE shouldn't be called twice contiguously.

This can happen due to bugs in users of __cpufreq_driver_target() or actual cpufreq drivers who are sending these notifiers.

This patch adds some protection against this. Now, we keep track of the last transaction and see if something went wrong.

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

index 2d53f47..6ca7eac 100644
@@ -107,6 +107,9 @@ static void handle_update(struct work_struct *work);
 static BLOCKING_NOTIFIER_HEAD(cpufreq_policy_notifier_list);
 static struct srcu_notifier_head cpufreq_transition_notifier_list;
 
+/* Tracks status of transition */
+static int transition_ongoing;
+
 static bool init_cpufreq_transition_notifier_list_called;
 static int __init init_cpufreq_transition_notifier_list(void)
 {

@@ -264,6 +267,12 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
+		if (WARN(transition_ongoing,
+				"In middle of another frequency transition\n"))
+			return;
+
+		transition_ongoing++;
+
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
 		 * "old frequency".

@@ -283,6 +292,12 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 		break;
 
 	case CPUFREQ_POSTCHANGE:
+		if (WARN(!transition_ongoing,
+				"No frequency transition in progress\n"))
+			return;
+
+		transition_ongoing--;
+
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
 			(unsigned long)freqs->cpu);

@@ -1458,6 +1473,8 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 
 	if (cpufreq_disabled())
 		return -ENODEV;
+	if (transition_ongoing)
+		return -EBUSY;
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)

1.7.12.rc2.18.g61b472e

Looks OK, but since transition_ongoing is either 0 or 1 now, as far as I can say, it would be better to make it a bool and use = true/false instead of ++/-- I suppose.

Thanks,
Rafael

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Another fixup:

index 6ca7eac..49d942a 100644
@@ -108,7 +108,7 @@ static BLOCKING_NOTIFIER_HEAD(cpufreq_policy_notifier_list);
 static struct srcu_notifier_head cpufreq_transition_notifier_list;
 
 /* Tracks status of transition */
-static int transition_ongoing;
+static bool transition_ongoing;
 
 static bool init_cpufreq_transition_notifier_list_called;
 static int __init init_cpufreq_transition_notifier_list(void)

@@ -271,7 +271,7 @@ void __cpufreq_notify_transition(struct

cpufreq_policy *policy,
"In middle of another frequency transition\n"))
return;

- transition_ongoing++;
+ transition_ongoing = true;

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

- transition_ongoing--;
+ transition_ongoing = false;

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

From b8c3bd9fcb521f1b66d52f157fc26eccf6cc4f29 Mon Sep 17 00:00:00 2001
Message-Id: <b8c3bd9fcb521f1b66d52f157fc26eccf6cc4f29.1372080656.git.viresh.kumar@linaro.org>
From: Viresh Kumar <[email protected]>
Date: Wed, 19 Jun 2013 10:16:55 +0530
Subject: [PATCH] cpufreq: make sure frequency transitions are serialized

Whenever we are changing frequency of a cpu, we are calling PRECHANGE and POSTCHANGE notifiers. They must be serialized. i.e. PRECHANGE or POSTCHANGE shouldn't be called twice contiguously.

This can happen due to bugs in users of __cpufreq_driver_target() or actual cpufreq drivers who are sending these notifiers.

This patch adds some protection against this. Now, we keep track of the last transaction and see if something went wrong.

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

index 2d53f47..49d942a 100644
@@ -107,6 +107,9 @@ static void handle_update(struct work_struct *work);
 static BLOCKING_NOTIFIER_HEAD(cpufreq_policy_notifier_list);
 static struct srcu_notifier_head cpufreq_transition_notifier_list;
 
+/* Tracks status of transition */
+static bool transition_ongoing;
+
 static bool init_cpufreq_transition_notifier_list_called;
 static int __init init_cpufreq_transition_notifier_list(void)
 {

@@ -264,6 +267,12 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
+		if (WARN(transition_ongoing,
+				"In middle of another frequency transition\n"))
+			return;
+
+		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".

@@ -283,6 +292,12 @@ void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 		break;
 
 	case CPUFREQ_POSTCHANGE:
+		if (WARN(!transition_ongoing,
+				"No frequency transition in progress\n"))
+			return;
+
+		transition_ongoing = false;
+
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
 			(unsigned long)freqs->cpu);

@@ -1458,6 +1473,8 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 
 	if (cpufreq_disabled())
 		return -ENODEV;
+	if (transition_ongoing)
+		return -EBUSY;
 
 	/* Make sure that target_freq is within supported range */
 	if (target_freq > policy->max)

1.7.12.rc2.18.g61b472e

Well, now, seeing that the locking around this seems to be kind of haphazard, I'm wondering what prevents two different threads from doing CPUFREQ_PRECHANGE concurrently in such a way that thread A will check transition_ongoing and thread B will check transition_ongoing and then both will set it if it was 'false' before. And then one of them will trigger the WARN() in CPUFREQ_POSTCHANGE.

Is there any protection in place and if so then how does it work?

Rafael

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

cpufreq_notify_transition() is called from driver->target() which is called from __cpufreq_driver_target(). __cpufreq_driver_target() is called directly by governors and cpufreq_driver_target() otherwise.

cpufreq_driver_target() implements proper locking and so it is fine. __cpufreq_driver_target() is called from governors. From governors it is is serialized in the sense two threads wouldn't call it at the same time.

And so I thought this will work. But I just found a mistake in my code. For multi-socket platforms with clock domains for sockets/clusters, a single instance of transition_ongoing isn't enough and so this must be embedded in struct cpufreq_policy.

Below patch must get this fixed (Attached).

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

From: Viresh Kumar <[email protected]>
Date: Wed, 19 Jun 2013 10:16:55 +0530
Subject: [PATCH] cpufreq: make sure frequency transitions are serialized

Whenever we are changing frequency of a cpu, we are calling PRECHANGE and POSTCHANGE notifiers. They must be serialized. i.e. PRECHANGE or POSTCHANGE shouldn't be called twice contiguously.

This can happen due to bugs in users of __cpufreq_driver_target() or actual cpufreq drivers who are sending these notifiers.

This patch adds some protection against this. Now, we keep track of the last transaction and see if something went wrong.

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

index 2d53f47..75715f1 100644
@@ -264,6 +264,12 @@ 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".
@@ -283,6 +289,12 @@ 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);
@@ -1458,6 +1470,8 @@ 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)

index 037d36a..8c13a45 100644
@@ -115,6 +115,7 @@ struct cpufreq_policy {
 
 	struct kobject		kobj;
 	struct completion	kobj_unregister;
+	bool			transition_ongoing; /* Tracks transition status */
 };
 
 #define CPUFREQ_ADJUST			(0)

From f0573570c85b0fd14db1130ee4885a50ed7bdb94 Mon Sep 17 00:00:00 2001
Message-Id: <f0573570c85b0fd14db1130ee4885a50ed7bdb94.1372308892.git.viresh.kumar@linaro.org>
From: Viresh Kumar <[email protected]>
Date: Wed, 19 Jun 2013 10:16:55 +0530
Subject: [PATCH] cpufreq: make sure frequency transitions are serialized

Whenever we are changing frequency of a cpu, we are calling PRECHANGE and POSTCHANGE notifiers. They must be serialized. i.e. PRECHANGE or POSTCHANGE shouldn't be called twice contiguously.

This can happen due to bugs in users of __cpufreq_driver_target() or actual cpufreq drivers who are sending these notifiers.

This patch adds some protection against this. Now, we keep track of the last transaction and see if something went wrong.

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

index 2d53f47..75715f1 100644
@@ -264,6 +264,12 @@ 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".

@@ -283,6 +289,12 @@ 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);

@@ -1458,6 +1470,8 @@ 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)

index 037d36a..8c13a45 100644
@@ -115,6 +115,7 @@ struct cpufreq_policy {
 
 	struct kobject		kobj;
 	struct completion	kobj_unregister;
+	bool			transition_ongoing; /* Tracks transition status */
 };
 
 #define CPUFREQ_ADJUST			(0)

1.7.12.rc2.18.g61b472e

OK, queued up for 3.11.

Thanks,
Rafael

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Patches [1,5,9-10/13] applied to bleeding-edge, the ARM ones I'm expecting to get from you and [13/13] can wait.

Thanks,
Rafael