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

[Viresh Kumar]

On 26 September 2013 00:07, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 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;
 };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/