[PATCH 1/2] cpufreq: distinguish drivers that do asynchronous notifications

[PATCH 1/2] cpufreq: distinguish drivers that do asynchronous notifications

[Viresh Kumar]

There are few special cases like exynos5440 which doesn't send POSTCHANGE
notification from their ->target() routine and call some kind of bottom halves
for doing this work, work/tasklet/etc.. From which they finally send POSTCHANGE
notification.

Its better if we distinguish them from other cpufreq drivers in some way so that
core can handle them specially. So this patch introduces another flag:
CPUFREQ_ASYNC_NOTIFICATION, which will be set by such drivers.

This also changes exynos5440-cpufreq.c in order to set this flag.

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

diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index d514c15..f44664a 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -342,7 +342,7 @@ static int exynos_cpufreq_cpu_init(struct cpufreq_policy *policy)
 }
 
 static struct cpufreq_driver exynos_driver = {
-	.flags		= CPUFREQ_STICKY,
+	.flags		= CPUFREQ_STICKY | CPUFREQ_ASYNC_NOTIFICATION,
 	.verify		= exynos_verify_speed,
 	.target		= exynos_target,
 	.get		= exynos_getspeed,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index fcabc42..3cefb7b 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -218,6 +218,12 @@ struct cpufreq_driver {
 					 * frequency transitions */
 #define CPUFREQ_PM_NO_WARN	0x04	/* don't warn on suspend/resume speed
 					 * mismatches */
+/*
+ * Driver will do POSTCHANGE notifications from outside of their ->target()
+ * routine and so must set cpufreq_driver->flags with this flag, so that core
+ * can handle them specially.
+ */
+#define CPUFREQ_ASYNC_NOTIFICATION 0x08
 
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/2] cpufreq: make sure frequency transitions are serialized

[Viresh Kumar]

Some part of this patch was pushed in mainline earlier but was then removed due
to loopholes in the patch. Those are now fixed and this patch is tested by the
people who reported these problems.

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. Also, calls to cpufreq_driver_target()
or cpufreq_driver->target() must also be serialized. Following examples show why
this is important:

Scenario 1:
-----------
One thread reading value of cpuinfo_cur_freq, which will call
__cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..

And ondemand governor trying to change freq of cpu at the same time and so
sending notification via ->target()..

Notifiers are not serialized and suppose this is what happened
- PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
- PRECHANGE Notification for freq B (from target())
- Freq changed by target() to B
- POSTCHANGE Notification for freq B
- POSTCHANGE Notification for freq A

Now the last POSTCHANGE Notification happened for freq A and hardware is
currently running at freq B :)

Where would we break then?: adjust_jiffies() in cpufreq.c,
cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
jiffies).. All loops_per_jiffy based stuff is broken..

Scenario 2:
-----------
Governor is changing freq and has called __cpufreq_driver_target(). At the same
time we are changing scaling_{min|max}_freq from sysfs, which would eventually
end up calling governor's: CPUFREQ_GOV_LIMITS notification, that will also call:
__cpufreq_driver_target().. And hence concurrent calls to ->target()

And Platform have something like this in their ->target()
(Like: cpufreq-cpu0, omap, exynos, etc)

A. If new freq is more than old: Increase voltage
B. Change freq
C. If new freq is less than old: decrease voltage

Now, two concurrent calls to target are X and Y, where X is trying to increase
freq and Y is trying to decrease it..

And this is the sequence that followed due to races..

X.A: voltage increased for larger freq
Y.A: nothing happened here
Y.B: freq decreased
Y.C: voltage decreased
X.B: freq increased
X.C: nothing happened..

We ended up setting a freq which is not supported by the voltage we have
set.. That will probably make clock to CPU unstable and system wouldn't
be workable anymore...

This patch adds some protection against to make transitions serialized.

Tested-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c            | 83 ++++++++++++++++++++++++++++++++++++
 drivers/cpufreq/exynos5440-cpufreq.c |  3 ++
 include/linux/cpufreq.h              |  1 +
 3 files changed, 87 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 43c24aa..f8b0889 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -268,6 +268,8 @@ static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
 static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 		struct cpufreq_freqs *freqs, unsigned int state)
 {
+	unsigned long flags;
+
 	BUG_ON(irqs_disabled());
 
 	if (cpufreq_disabled())
@@ -280,6 +282,17 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
+		write_lock_irqsave(&cpufreq_driver_lock, flags);
+		if (WARN(policy->transition_ongoing >
+					cpumask_weight(policy->cpus),
+				"In middle of another frequency transition\n")) {
+			write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+			return;
+		}
+
+		policy->transition_ongoing++;
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 		/* detect if the driver reported a value as "old frequency"
 		 * which is not equal to what the cpufreq core thinks is
 		 * "old frequency".
@@ -299,6 +312,16 @@ static void __cpufreq_notify_transition(struct cpufreq_policy *policy,
 		break;
 
 	case CPUFREQ_POSTCHANGE:
+		write_lock_irqsave(&cpufreq_driver_lock, flags);
+		if (WARN(policy->transition_ongoing < 2,
+				"No frequency transition in progress\n")) {
+			write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+			return;
+		}
+
+		policy->transition_ongoing--;
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
 		pr_debug("FREQ: %lu - CPU: %lu", (unsigned long)freqs->new,
 			(unsigned long)freqs->cpu);
@@ -324,6 +347,20 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
 {
 	for_each_cpu(freqs->cpu, policy->cpus)
 		__cpufreq_notify_transition(policy, freqs, state);
+
+	if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+			&& (state == CPUFREQ_POSTCHANGE)) {
+		unsigned long flags;
+
+		/*
+		 * Some drivers don't send POSTCHANGE notification from their
+		 * ->target() but from some kind of bottom half and so we are
+		 * ending transaction here..
+		 */
+		write_lock_irqsave(&cpufreq_driver_lock, flags);
+		policy->transition_ongoing--;
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	}
 }
 EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
 
@@ -1369,8 +1406,33 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq,
 	policy = per_cpu(cpufreq_cpu_data, cpu);
 	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	/*
+	 * The role of this function is to make sure that the CPU frequency we
+	 * use is the same as the CPU is actually running at. Therefore, if a
+	 * CPU frequency change notification is in progress, it will do the
+	 * update anyway, so we have nothing to do here in that case.
+	 */
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (policy->transition_ongoing) {
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		return;
+	}
+	policy->transition_ongoing++;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
 	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
+
+	/*
+	 * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement
+	 * transition_ongoing from POSTCHANGE notifiers.
+	 */
+	if (cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+		return;
+
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	policy->transition_ongoing--;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 }
 
 /**
@@ -1656,6 +1718,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 {
 	int retval = -EINVAL;
 	unsigned int old_target_freq = target_freq;
+	unsigned long flags;
 
 	if (cpufreq_disabled())
 		return -ENODEV;
@@ -1672,9 +1735,29 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (policy->transition_ongoing) {
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		return -EBUSY;
+	}
+	policy->transition_ongoing++;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
+	/*
+	 * For drivers with CPUFREQ_ASYNC_NOTIFICATION flag set, we decrement
+	 * transition_ongoing from POSTCHANGE notifiers.
+	 */
+	if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
+			&& (retval == -EINPROGRESS))
+		return retval;
+
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	policy->transition_ongoing--;
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index f44664a..1e391ac 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -251,6 +251,9 @@ static int exynos_target(struct cpufreq_policy *policy,
 
 		__raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
 	}
+
+	/* Mark transition as In-progress */
+	ret = -EINPROGRESS;
 out:
 	mutex_unlock(&cpufreq_lock);
 	return ret;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 3cefb7b..c770bc0 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -85,6 +85,7 @@ struct cpufreq_policy {
 	struct list_head        policy_list;
 	struct kobject		kobj;
 	struct completion	kobj_unregister;
+	int			transition_ongoing; /* Tracks transition status */
 };
 
 /* Only for ACPI */
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 2/2] cpufreq: make sure frequency transitions are serialized

[Viresh Kumar]

On 12 September 2013 15:40, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Some part of this patch was pushed in mainline earlier but was then removed due
> to loopholes in the patch. Those are now fixed and this patch is tested by the
> people who reported these problems.
>
> 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. Also, calls to cpufreq_driver_target()
> or cpufreq_driver->target() must also be serialized. Following examples show why
> this is important:

Adding:

Tested-by: Stephen Warren <swarren@nvidia.com>

Picked from the other thread..

Re: [PATCH 2/2] cpufreq: make sure frequency transitions are serialized

[Rafael J. Wysocki]

On Thursday, September 12, 2013 03:40:46 PM Viresh Kumar wrote:
> Some part of this patch was pushed in mainline earlier but was then removed due
> to loopholes in the patch. Those are now fixed and this patch is tested by the
> people who reported these problems.
> 
> 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. Also, calls to cpufreq_driver_target()
> or cpufreq_driver->target() must also be serialized. Following examples show why
> this is important:
> 
> Scenario 1:
> -----------
> One thread reading value of cpuinfo_cur_freq, which will call
> __cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..
> 
> And ondemand governor trying to change freq of cpu at the same time and so
> sending notification via ->target()..
> 
> Notifiers are not serialized and suppose this is what happened
> - PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
> - PRECHANGE Notification for freq B (from target())
> - Freq changed by target() to B
> - POSTCHANGE Notification for freq B
> - POSTCHANGE Notification for freq A
> 
> Now the last POSTCHANGE Notification happened for freq A and hardware is
> currently running at freq B :)
> 
> Where would we break then?: adjust_jiffies() in cpufreq.c,
> cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
> jiffies).. All loops_per_jiffy based stuff is broken..
> 
> Scenario 2:
> -----------
> Governor is changing freq and has called __cpufreq_driver_target(). At the same
> time we are changing scaling_{min|max}_freq from sysfs, which would eventually
> end up calling governor's: CPUFREQ_GOV_LIMITS notification, that will also call:
> __cpufreq_driver_target().. And hence concurrent calls to ->target()
> 
> And Platform have something like this in their ->target()
> (Like: cpufreq-cpu0, omap, exynos, etc)
> 
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
> 
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
> 
> And this is the sequence that followed due to races..
> 
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
> 
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
> 
> This patch adds some protection against to make transitions serialized.
> 
> Tested-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

So the problem is real, but the fix seems to be of a "quick and dirty" kind.

First of all, it looks like we need a clear "begin transition" call that
I suppose drivers should execute from their .target() methods once they have
decided to do a transition.  That would increment the "ongoing" counter etc.

Second, we need a corresponding "end transition" call that would be executed
whenever appropriate from the driver's perspective.

Clearly, these two things should be independent of the notifiers and the
notifications should only be done between "begin transition" and "end
transition" and only by whoever called the "begin transition" to start with.

Now, question is what should happen if "begin transition" is called when
the previous transition hasn't been completed yet, should it block or should
it fail?  There seem to be arguments for both, but I suppose blocking would be
easier to implement.

Thanks,
Rafael

Re: [PATCH 2/2] cpufreq: make sure frequency transitions are serialized

[Srivatsa S. Bhat]

On 09/26/2013 12:07 AM, Rafael J. Wysocki wrote:
> On Thursday, September 12, 2013 03:40:46 PM Viresh Kumar wrote:
>> Some part of this patch was pushed in mainline earlier but was then removed due
>> to loopholes in the patch. Those are now fixed and this patch is tested by the
>> people who reported these problems.
>>
>> 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. Also, calls to cpufreq_driver_target()
>> or cpufreq_driver->target() must also be serialized. Following examples show why
>> this is important:
>>
>> Scenario 1:
>> -----------
>> One thread reading value of cpuinfo_cur_freq, which will call
>> __cpufreq_cpu_get()->cpufreq_out_of_sync()->cpufreq_notify_transition()..
>>
>> And ondemand governor trying to change freq of cpu at the same time and so
>> sending notification via ->target()..
>>
>> Notifiers are not serialized and suppose this is what happened
>> - PRECHANGE Notification for freq A (from cpuinfo_cur_freq)
>> - PRECHANGE Notification for freq B (from target())
>> - Freq changed by target() to B
>> - POSTCHANGE Notification for freq B
>> - POSTCHANGE Notification for freq A
>>
>> Now the last POSTCHANGE Notification happened for freq A and hardware is
>> currently running at freq B :)
>>
>> Where would we break then?: adjust_jiffies() in cpufreq.c,
>> cpufreq_callback() in arch/arm/kernel/smp.c (which is also adjusting
>> jiffies).. All loops_per_jiffy based stuff is broken..
>>
>> Scenario 2:
>> -----------
>> Governor is changing freq and has called __cpufreq_driver_target(). At the same
>> time we are changing scaling_{min|max}_freq from sysfs, which would eventually
>> end up calling governor's: CPUFREQ_GOV_LIMITS notification, that will also call:
>> __cpufreq_driver_target().. And hence concurrent calls to ->target()
>>
>> And Platform have something like this in their ->target()
>> (Like: cpufreq-cpu0, omap, exynos, etc)
>>
>> A. If new freq is more than old: Increase voltage
>> B. Change freq
>> C. If new freq is less than old: decrease voltage
>>
>> Now, two concurrent calls to target are X and Y, where X is trying to increase
>> freq and Y is trying to decrease it..
>>
>> And this is the sequence that followed due to races..
>>
>> X.A: voltage increased for larger freq
>> Y.A: nothing happened here
>> Y.B: freq decreased
>> Y.C: voltage decreased
>> X.B: freq increased
>> X.C: nothing happened..
>>
>> We ended up setting a freq which is not supported by the voltage we have
>> set.. That will probably make clock to CPU unstable and system wouldn't
>> be workable anymore...
>>
>> This patch adds some protection against to make transitions serialized.
>>
>> Tested-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> So the problem is real, but the fix seems to be of a "quick and dirty" kind.
> 
> First of all, it looks like we need a clear "begin transition" call that
> I suppose drivers should execute from their .target() methods once they have
> decided to do a transition.  That would increment the "ongoing" counter etc.
> 
> Second, we need a corresponding "end transition" call that would be executed
> whenever appropriate from the driver's perspective.
> 
> Clearly, these two things should be independent of the notifiers and the
> notifications should only be done between "begin transition" and "end
> transition" and only by whoever called the "begin transition" to start with.
> 
> Now, question is what should happen if "begin transition" is called when
> the previous transition hasn't been completed yet, should it block or should
> it fail?  There seem to be arguments for both, but I suppose blocking would be
> easier to implement.
>

I agree with Rafael. We really need an elegant solution for this problem,
something whose very semantics makes it easy and almost intuitive to get the
synchronization right. Otherwise it might become too easy to get things wrong
in these cases.

Regards,
Srivatsa S. Bhat

Re: [PATCH 2/2] cpufreq: make sure frequency transitions are serialized

[Viresh Kumar]

On 26 September 2013 00:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> So the problem is real, but the fix seems to be of a "quick and dirty" kind.

Hmm..

> First of all, it looks like we need a clear "begin transition" call that
> I suppose drivers should execute from their .target() methods once they have
> decided to do a transition.  That would increment the "ongoing" counter etc.
>
> Second, we need a corresponding "end transition" call that would be executed
> whenever appropriate from the driver's perspective.

Hmm..

> Clearly, these two things should be independent of the notifiers and the
> notifications should only be done between "begin transition" and "end
> transition" and only by whoever called the "begin transition" to start with.

So, we need to have begin/end calls in cpufreq_out_of_sync() as well?
As that is sending notifications..

> Now, question is what should happen if "begin transition" is called when
> the previous transition hasn't been completed yet, should it block or should
> it fail?  There seem to be arguments for both, but I suppose blocking would be
> easier to implement.

Hmm.. I will repost this once my other patches are in now.. as that will
change ->target() routine for multiple drivers and things would be simple
to fix then..

Re: [PATCH 2/2] cpufreq: make sure frequency transitions are serialized

[Viresh Kumar]

On 26 September 2013 00:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> So the problem is real, but the fix seems to be of a "quick and dirty" kind.
>
> First of all, it looks like we need a clear "begin transition" call that
> I suppose drivers should execute from their .target() methods once they have
> decided to do a transition.  That would increment the "ongoing" counter etc.
>
> Second, we need a corresponding "end transition" call that would be executed
> whenever appropriate from the driver's perspective.
>
> Clearly, these two things should be independent of the notifiers and the
> notifications should only be done between "begin transition" and "end
> transition" and only by whoever called the "begin transition" to start with.
>
> Now, question is what should happen if "begin transition" is called when
> the previous transition hasn't been completed yet, should it block or should
> it fail?  There seem to be arguments for both, but I suppose blocking would be
> easier to implement.

I got another solution which is much simpler, and I really can't believe
(psychologically) that it will solve all the problems we were talking about..
Please see if there is any loophole in there..

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ec391d7..e4ed89a 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -340,6 +340,13 @@ static void __cpufreq_notify_transition(struct
cpufreq_policy *policy,
        }
 }

+void cpufreq_notify_transition_each_cpu(struct cpufreq_policy *policy,
+               struct cpufreq_freqs *freqs, unsigned int state)
+{
+       for_each_cpu(freqs->cpu, policy->cpus)
+               __cpufreq_notify_transition(policy, freqs, state);
+}
+
 /**
  * cpufreq_notify_transition - call notifier chain and adjust_jiffies
  * on frequency transition.
@@ -351,8 +358,34 @@ static void __cpufreq_notify_transition(struct
cpufreq_policy *policy,
 void cpufreq_notify_transition(struct cpufreq_policy *policy,
                struct cpufreq_freqs *freqs, unsigned int state)
 {
-       for_each_cpu(freqs->cpu, policy->cpus)
-               __cpufreq_notify_transition(policy, freqs, state);
+       if ((state != CPUFREQ_PRECHANGE) && (state != CPUFREQ_POSTCHANGE))
+               return cpufreq_notify_transition_each_cpu(policy, freqs, state);
+
+       /* Serialize pre-post notifications */
+       mutex_lock(&policy->transition_lock);
+       if (unlikely(WARN_ON(!policy->transition_ongoing &&
+                               (state == CPUFREQ_POSTCHANGE)))) {
+               mutex_unlock(&policy->transition_lock);
+               return;
+       }
+
+       if (state == CPUFREQ_PRECHANGE) {
+               while (policy->transition_ongoing) {
+                       mutex_unlock(&policy->transition_lock);
+                       cpu_relax();
+                       mutex_lock(&policy->transition_lock);
+               }
+
+               policy->transition_ongoing = true;
+               mutex_unlock(&policy->transition_lock);
+       }
+
+       cpufreq_notify_transition_each_cpu(policy, freqs, state);
+
+       if (state == CPUFREQ_POSTCHANGE) {
+               policy->transition_ongoing = false;
+               mutex_unlock(&policy->transition_lock);
+       }
 }
 EXPORT_SYMBOL_GPL(cpufreq_notify_transition);

@@ -950,6 +983,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
                goto err_free_cpumask;

        INIT_LIST_HEAD(&policy->policy_list);
+       mutex_init(&policy->transition_lock);
+
        return policy;

 err_free_cpumask:
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 0aba2a6c..bb76909 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -85,6 +85,8 @@ struct cpufreq_policy {
        struct list_head        policy_list;
        struct kobject          kobj;
        struct completion       kobj_unregister;
+       bool                    transition_ongoing; /* Tracks
transition status */
+       struct mutex            transition_lock;
 };

Re: [PATCH 2/2] cpufreq: make sure frequency transitions are serialized

[Viresh Kumar]

On 23 October 2013 14:43, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> I got another solution which is much simpler, and I really can't believe
> (psychologically) that it will solve all the problems we were talking about..
> Please see if there is any loophole in there..

Ping!!

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index ec391d7..e4ed89a 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -340,6 +340,13 @@ static void __cpufreq_notify_transition(struct
> cpufreq_policy *policy,
>         }
>  }
>
> +void cpufreq_notify_transition_each_cpu(struct cpufreq_policy *policy,
> +               struct cpufreq_freqs *freqs, unsigned int state)
> +{
> +       for_each_cpu(freqs->cpu, policy->cpus)
> +               __cpufreq_notify_transition(policy, freqs, state);
> +}
> +
>  /**
>   * cpufreq_notify_transition - call notifier chain and adjust_jiffies
>   * on frequency transition.
> @@ -351,8 +358,34 @@ static void __cpufreq_notify_transition(struct
> cpufreq_policy *policy,
>  void cpufreq_notify_transition(struct cpufreq_policy *policy,
>                 struct cpufreq_freqs *freqs, unsigned int state)
>  {
> -       for_each_cpu(freqs->cpu, policy->cpus)
> -               __cpufreq_notify_transition(policy, freqs, state);
> +       if ((state != CPUFREQ_PRECHANGE) && (state != CPUFREQ_POSTCHANGE))
> +               return cpufreq_notify_transition_each_cpu(policy, freqs, state);
> +
> +       /* Serialize pre-post notifications */
> +       mutex_lock(&policy->transition_lock);
> +       if (unlikely(WARN_ON(!policy->transition_ongoing &&
> +                               (state == CPUFREQ_POSTCHANGE)))) {
> +               mutex_unlock(&policy->transition_lock);
> +               return;
> +       }
> +
> +       if (state == CPUFREQ_PRECHANGE) {
> +               while (policy->transition_ongoing) {
> +                       mutex_unlock(&policy->transition_lock);
> +                       cpu_relax();
> +                       mutex_lock(&policy->transition_lock);
> +               }
> +
> +               policy->transition_ongoing = true;
> +               mutex_unlock(&policy->transition_lock);
> +       }
> +
> +       cpufreq_notify_transition_each_cpu(policy, freqs, state);
> +
> +       if (state == CPUFREQ_POSTCHANGE) {
> +               policy->transition_ongoing = false;
> +               mutex_unlock(&policy->transition_lock);
> +       }
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
>
> @@ -950,6 +983,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
>                 goto err_free_cpumask;
>
>         INIT_LIST_HEAD(&policy->policy_list);
> +       mutex_init(&policy->transition_lock);
> +
>         return policy;
>
>  err_free_cpumask:
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 0aba2a6c..bb76909 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -85,6 +85,8 @@ struct cpufreq_policy {
>         struct list_head        policy_list;
>         struct kobject          kobj;
>         struct completion       kobj_unregister;
> +       bool                    transition_ongoing; /* Tracks
> transition status */
> +       struct mutex            transition_lock;
>  };