[PATCH 00/13] CPUFreq: Fix {PRE|POST}CHANGE notification sequence

List overview All Threads
Download

newer

older

[PATCH 3/3] smc91x: Fix cast from...

[PATCH] ACPI/fan: Initialize...

Viresh Kumar

19 Jun 2013 19 Jun '13

8:52 a.m.

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

-- 1.7.12.rc2.18.g61b472e

Show replies by date

Viresh Kumar

19 Jun 19 Jun

8:52 a.m.

New subject: [PATCH 01/13] cpufreq: acpi: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Borislav Petkov bp@suse.de Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/acpi-cpufreq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index edc089e..223ddf4 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -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

Viresh Kumar

8:52 a.m.

New subject: [PATCH 02/13] cpufreq: arm-big-little: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/arm_big_little.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index 5d7f53f..3549f07 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -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

Viresh Kumar

8:52 a.m.

New subject: [PATCH 03/13] cpufreq: davinci: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Sekhar Nori nsekhar@ti.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/davinci-cpufreq.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/davinci-cpufreq.c b/drivers/cpufreq/davinci-cpufreq.c index c33c76c..551dd65 100644 --- a/drivers/cpufreq/davinci-cpufreq.c +++ b/drivers/cpufreq/davinci-cpufreq.c @@ -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

Sekhar Nori

8:58 a.m.

New subject: [PATCH 03/13] cpufreq: davinci: call CPUFREQ_POSTCHANGE notfier in error cases

On 6/19/2013 2:22 PM, Viresh Kumar wrote:

...

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.

Cc: Sekhar Nori nsekhar@ti.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Looks good to me.

Acked-by: Sekhar Nori nsekhar@ti.com

Thanks, Sekhar

Viresh Kumar

8:52 a.m.

New subject: [PATCH 04/13] cpufreq: dbx500: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/dbx500-cpufreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/dbx500-cpufreq.c b/drivers/cpufreq/dbx500-cpufreq.c index 6ec6539..1fdb02b 100644 --- a/drivers/cpufreq/dbx500-cpufreq.c +++ b/drivers/cpufreq/dbx500-cpufreq.c @@ -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

Linus Walleij

7:42 p.m.

New subject: [PATCH 04/13] cpufreq: dbx500: call CPUFREQ_POSTCHANGE notfier in error cases

On Wed, Jun 19, 2013 at 10:52 AM, Viresh Kumar viresh.kumar@linaro.org wrote:

...

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.

Cc: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Looks good to me. Acked-by: Linus Walleij linus.walleij@linaro.org

Yours, Linus Walleij

Viresh Kumar

8:52 a.m.

New subject: [PATCH 05/13] cpufreq: e_powersave: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Simon Horman horms@verge.net.au Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/e_powersaver.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c index 37380fb..3d03626 100644 --- a/drivers/cpufreq/e_powersaver.c +++ b/drivers/cpufreq/e_powersaver.c @@ -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

Simon Horman

12:22 p.m.

New subject: [PATCH 05/13] cpufreq: e_powersave: call CPUFREQ_POSTCHANGE notfier in error cases

On Wed, Jun 19, 2013 at 02:22:59PM +0530, Viresh Kumar wrote:

...

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.

Cc: Simon Horman horms@verge.net.au Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

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.

...

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

diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c index 37380fb..3d03626 100644 --- a/drivers/cpufreq/e_powersaver.c +++ b/drivers/cpufreq/e_powersaver.c @@ -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

Viresh Kumar

2:54 p.m.

New subject: [PATCH 05/13] cpufreq: e_powersave: call CPUFREQ_POSTCHANGE notfier in error cases

On 19 June 2013 17:52, Simon Horman horms@verge.net.au wrote:

...

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?

Dave Jones

3:08 p.m.

New subject: [PATCH 05/13] cpufreq: e_powersave: call CPUFREQ_POSTCHANGE notfier in error cases

On Wed, Jun 19, 2013 at 08:24:41PM +0530, Viresh Kumar wrote:

...

On 19 June 2013 17:52, Simon Horman horms@verge.net.au wrote:

...
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

Viresh Kumar

8:53 a.m.

New subject: [PATCH 06/13] cpufreq: exynos: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Kukjin Kim kgene.kim@samsung.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/exynos-cpufreq.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c index 475b4f6..0d32f02 100644 --- a/drivers/cpufreq/exynos-cpufreq.c +++ b/drivers/cpufreq/exynos-cpufreq.c @@ -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

Viresh Kumar

8:53 a.m.

New subject: [PATCH 07/13] cpufreq: imx6q: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Shawn Guo shawn.guo@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/imx6q-cpufreq.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c index b78bc35..e37cdae 100644 --- a/drivers/cpufreq/imx6q-cpufreq.c +++ b/drivers/cpufreq/imx6q-cpufreq.c @@ -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

Shawn Guo

20 Jun 20 Jun

2:52 a.m.

New subject: [PATCH 07/13] cpufreq: imx6q: call CPUFREQ_POSTCHANGE notfier in error cases

On Wed, Jun 19, 2013 at 02:23:01PM +0530, Viresh Kumar wrote:

...

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.

Cc: Shawn Guo shawn.guo@linaro.org

Acked-by: Shawn Guo shawn.guo@linaro.org

...

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Viresh Kumar

19 Jun 19 Jun

8:53 a.m.

New subject: [PATCH 08/13] cpufreq: omap: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Santosh Shilimkar santosh.shilimkar@ti.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/omap-cpufreq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c index 0279d18..29468a5 100644 --- a/drivers/cpufreq/omap-cpufreq.c +++ b/drivers/cpufreq/omap-cpufreq.c @@ -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

Santosh Shilimkar

2:44 p.m.

New subject: [PATCH 08/13] cpufreq: omap: call CPUFREQ_POSTCHANGE notfier in error cases

On Wednesday 19 June 2013 04:53 AM, Viresh Kumar wrote:

...

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.

Cca: Santosh Shilimkar santosh.shilimkar@ti.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Looks safe to me. Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com

Viresh Kumar

8:53 a.m.

New subject: [PATCH 09/13] cpufreq: pcc: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Dave Jones davej@redhat.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/pcc-cpufreq.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c index 0de0008..1581fcc4 100644 --- a/drivers/cpufreq/pcc-cpufreq.c +++ b/drivers/cpufreq/pcc-cpufreq.c @@ -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

Viresh Kumar

8:53 a.m.

New subject: [PATCH 10/13] cpufreq: powernow-k8: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/powernow-k8.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c index b828efe..c916320 100644 --- a/drivers/cpufreq/powernow-k8.c +++ b/drivers/cpufreq/powernow-k8.c @@ -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

Viresh Kumar

8:53 a.m.

New subject: [PATCH 11/13] cpufreq: s3c64xx: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Mark Brown broonie@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/s3c64xx-cpufreq.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c b/drivers/cpufreq/s3c64xx-cpufreq.c index 27cacb5..017ade0 100644 --- a/drivers/cpufreq/s3c64xx-cpufreq.c +++ b/drivers/cpufreq/s3c64xx-cpufreq.c @@ -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

Viresh Kumar

8:53 a.m.

New subject: [PATCH 12/13] cpufreq: tegra: call CPUFREQ_POSTCHANGE notfier in error cases

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.

Cc: Stephen Warren swarren@nvidia.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/tegra-cpufreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c index c74c0e1..e74d777 100644 --- a/drivers/cpufreq/tegra-cpufreq.c +++ b/drivers/cpufreq/tegra-cpufreq.c @@ -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

Stephen Warren

5:11 p.m.

New subject: [PATCH 12/13] cpufreq: tegra: call CPUFREQ_POSTCHANGE notfier in error cases

On 06/19/2013 02:53 AM, Viresh Kumar wrote:

...

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.

Acked-by: Stephen Warren swarren@nvidia.com

Viresh Kumar

8:53 a.m.

New subject: [PATCH 13/13] 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.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..92cb8b3 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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

Rafael J. Wysocki

24 Jun 24 Jun

11:43 a.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On Wednesday, June 19, 2013 02:23:07 PM Viresh Kumar wrote:

...

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.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..92cb8b3 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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--);

Shouldn't we try to avoid going into the negative range here?

...

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)

Rafael

Viresh Kumar

1:08 p.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On 24 June 2013 17:13, Rafael J. Wysocki rjw@sisk.pl wrote:

...

On Wednesday, June 19, 2013 02:23:07 PM Viresh Kumar wrote:

...
  case CPUFREQ_POSTCHANGE:
        WARN_ON(!transition_ongoing--);
Shouldn't we try to avoid going into the negative range here?

What about this patch? Find it attached to apply.

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..6624694 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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)

Rafael J. Wysocki

1:23 p.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On Monday, June 24, 2013 06:38:17 PM Viresh Kumar wrote:

...

On 24 June 2013 17:13, Rafael J. Wysocki rjw@sisk.pl wrote:

...
On Wednesday, June 19, 2013 02:23:07 PM Viresh Kumar wrote:

...
  case CPUFREQ_POSTCHANGE:
        WARN_ON(!transition_ongoing--);
Shouldn't we try to avoid going into the negative range here?
What about this patch? Find it attached to apply.

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..6624694 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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;
          }

You can do

if (WARN(transition_ongoing, "<text>")) return;

and below analogously.

...

```
          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)

Thanks, Rafael

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

Viresh Kumar

1:16 p.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On 24 June 2013 18:53, Rafael J. Wysocki rjw@sisk.pl wrote:

...

You can do

    if (WARN(transition_ongoing, "<text>"))
            return;

and below analogously.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 6624694..6ca7eac 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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--;

Rafael J. Wysocki

1:33 p.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On Monday, June 24, 2013 06:46:39 PM Viresh Kumar wrote:

...

On 24 June 2013 18:53, Rafael J. Wysocki rjw@sisk.pl wrote:

...
You can do
    if (WARN(transition_ongoing, "<text>"))
            return;
and below analogously.
Ahh.. stupid code.. Check if this fixup is fine (attached too)

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

...

    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--;

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

Viresh Kumar

1:31 p.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On 24 June 2013 19:03, Rafael J. Wysocki rjw@sisk.pl wrote:

...

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.

Another fixup:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 6ca7eac..49d942a 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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,

Rafael J. Wysocki

26 Jun 26 Jun

9:57 p.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On Monday, June 24, 2013 07:01:59 PM Viresh Kumar wrote:

...

On 24 June 2013 19:03, Rafael J. Wysocki rjw@sisk.pl wrote:

...
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.

Another fixup:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 6ca7eac..49d942a 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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,

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.

Viresh Kumar

27 Jun 27 Jun

4:56 a.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On 27 June 2013 03:27, Rafael J. Wysocki rjw@sisk.pl wrote:

...

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?

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 viresh.kumar@linaro.org 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.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 14 ++++++++++++++ include/linux/cpufreq.h | 1 + 2 files changed, 15 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..75715f1 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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) diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..8c13a45 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -115,6 +115,7 @@ struct cpufreq_policy {

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

#define CPUFREQ_ADJUST (0)

Rafael J. Wysocki

12:15 p.m.

New subject: [PATCH 13/13] cpufreq: make sure frequency transitions are serialized

On Thursday, June 27, 2013 10:26:27 AM Viresh Kumar wrote:

...

On 27 June 2013 03:27, Rafael J. Wysocki rjw@sisk.pl wrote:

...
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?

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 viresh.kumar@linaro.org 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.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

OK, queued up for 3.11.

Thanks, Rafael

...

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..75715f1 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -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)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..8c13a45 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -115,6 +115,7 @@ struct cpufreq_policy {

struct kobject kobj; struct completion kobj_unregister;

bool transition_ongoing; /* Tracks transition status */

};

#define CPUFREQ_ADJUST (0)

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

Rafael J. Wysocki

24 Jun 24 Jun

11:58 a.m.

On Wednesday, June 19, 2013 02:22:54 PM Viresh Kumar wrote:

...

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

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

3978

days inactive

3986

days old

linaro-kernel@lists.linaro.org

31 comments

participants

tags (0)

participants (9)

Dave Jones
Linus Walleij
Rafael J. Wysocki
Santosh Shilimkar
Sekhar Nori
Shawn Guo
Simon Horman
Stephen Warren
Viresh Kumar