policy->lock in cpufreq_add_dev

Hello,
I think that it's not necessary to take the policy->lock in cpufreq_add_dev. Am I missing something? What is the lock supposed to protect from?

Michal

diff -Nurp -X linux/Documentation/dontdiff linux/drivers/cpufreq/cpufreq.c linux.mich/drivers/cpufreq/cpufreq.c
@@ -605,7 +605,7 @@ static int cpufreq_add_dev (struct sys_d
 	policy->cpu = cpu;
 	policy->cpus = cpumask_of_cpu(cpu);
 
-	init_MUTEX_LOCKED(&policy->lock);
+	init_MUTEX(&policy->lock);
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update, (void *)(long)cpu);
 

@@ -646,8 +646,7 @@ static int cpufreq_add_dev (struct sys_d
 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	policy->governor = NULL; /* to assure that the starting sequence is
 				  * run in cpufreq_set_policy */
-	up(&policy->lock);
-	
+
 	/* set default policy */
 	
 	ret = cpufreq_set_policy(&new_policy);

_______________________________________________
Cpufreq mailing list
[email protected]
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Well, indeed it is not necessary to take the policy->lock, although it doesn't do any harm AFAICS. I added it to make sure that _all_ accesses to the data is protected by the lock, how serialized they may ever be..

Thanks,
Dominik

PS: Always add Signed-off-by lines to your patches, please, as per Documentation/SubmittingPatches

I thought so. I understand it doesn't do any harm in standard kernel. It is perfectly valid to take the policy->lock there and the function is not performance critical at all, so we can afford that.
There's only an issue in combination with Ingo's realtime-preempt patch,
where every usage of init_MUTEX_LOCKED becomes suspicious. It often indicates that the code uses a mutex where it should use a completion instead. However, this is not the case with cpufreq, where policy->lock really acts as a mutex. So I looked into it and wasn't able to find the reason for the lock being taken in cpufreq_add_dev.

Sorry, I forgot about that. Here it is again, with the sign-off.
I know it's not a bugfix, because the code works perfectly it the normal kernel (only realtime-preempt cares). It's just a minor cleanup. It would be nice if you applied it, but no problem if you don't:

It's unnecessary to take policy->lock in cpufreq_add_dev as the access to the policy object is guaranteed to be serialized there.

diff -Nurp -X linux/Documentation/dontdiff linux/drivers/cpufreq/cpufreq.c linux.mich/drivers/cpufreq/cpufreq.c
@@ -605,7 +605,7 @@ static int cpufreq_add_dev (struct sys_d
 	policy->cpu = cpu;
 	policy->cpus = cpumask_of_cpu(cpu);
 
-	init_MUTEX_LOCKED(&policy->lock);
+	init_MUTEX(&policy->lock);
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update, (void *)(long)cpu);
 

@@ -646,8 +646,7 @@ static int cpufreq_add_dev (struct sys_d
 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	policy->governor = NULL; /* to assure that the starting sequence is
 				  * run in cpufreq_set_policy */
-	up(&policy->lock);
-	
+
 	/* set default policy */
 	
 	ret = cpufreq_set_policy(&new_policy);

_______________________________________________
Cpufreq mailing list
[email protected]
http://lists.linux.org.uk/mailman/listinfo/cpufreq