Re: [RFC v3] cpufreq: Make sure frequency transitions are serialized

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 03/20/2014 10:09 AM, Viresh Kumar wrote:
> On 19 March 2014 17:45, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 199b52b..e90388f 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -349,6 +349,38 @@ void cpufreq_notify_post_transition(struct cpufreq_policy *policy,
>>  EXPORT_SYMBOL_GPL(cpufreq_notify_post_transition);
>>
>>
>> +void cpufreq_freq_transition_begin(struct cpufreq_policy *policy,
>> +               struct cpufreq_freqs *freqs, unsigned int state)
>> +{
>> +wait:
>> +       wait_event(&policy->transition_wait, !policy->transition_ongoing);
> 
> I think its broken here. At this point another thread can come take lock,
> update transition_ongoing, send notification and finally unlock..
> 
> And after that we can take the lock and send another notification..
> 
> Correct?
> 

Good catch! I missed that yesterday. Please find the updated patch below,
with all your suggestions incorporated. Does this version look any better?

------------------------------------------------------------------------

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 199b52b..5283f10 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -349,6 +349,39 @@ void cpufreq_notify_post_transition(struct cpufreq_policy *policy,
 EXPORT_SYMBOL_GPL(cpufreq_notify_post_transition);
 
 
+void cpufreq_freq_transition_begin(struct cpufreq_policy *policy,
+		struct cpufreq_freqs *freqs, unsigned int state)
+{
+wait:
+	wait_event(&policy->transition_wait, !policy->transition_ongoing);
+
+	mutex_lock(&policy->transition_lock);
+
+	if (policy->transition_ongoing) {
+		mutex_unlock(&policy->transition_lock);
+		goto wait;
+	}
+
+	policy->transition_ongoing = true;
+
+	mutex_unlock(&policy->transition_lock);
+
+	cpufreq_notify_transition(policy, freqs, CPUFREQ_PRECHANGE);
+}
+
+void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
+		struct cpufreq_freqs *freqs, unsigned int state)
+{
+	cpufreq_notify_transition(policy, freqs, CPUFREQ_POSTCHANGE);
+
+	mutex_lock(&policy->transition_lock);
+	policy->transition_ongoing = false;
+	mutex_unlock(&policy->transition_lock);
+
+	wake_up(&policy->transition_wait);
+}
+
+
 /*********************************************************************
  *                          SYSFS INTERFACE                          *
  *********************************************************************/
@@ -968,6 +1001,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
 
 	INIT_LIST_HEAD(&policy->policy_list);
 	init_rwsem(&policy->rwsem);
+	mutex_init(&policy->transition_lock);
+	init_waitqueue_head(&policy->transition_wait);
 
 	return policy;
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d89e0e..8bded24 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -101,6 +101,11 @@ struct cpufreq_policy {
 	 *     __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 	 */
 	struct rw_semaphore	rwsem;
+
+	/* Synchronization for frequency transitions */
+	bool			transition_ongoing; /* Tracks transition status */
+	struct mutex		transition_lock;
+	wait_queue_head_t	transition_wait;
 };
 
 /* Only for ACPI */