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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 23 Oct 2013 14:43:12 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc:	"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] cpufreq: make sure frequency transitions are serialized

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