[PATCH] Reorganize the cpufreq cpu hotplug locking to not be totally bizare

The patch below moves the cpu hotplugging higher up in the cpufreq
layering; this is needed to avoid recursive taking of the cpu hotplug
lock and to otherwise detangle the mess.

The new rules are:
1. you must do lock_cpu_hotplug() around the following functions:
   __cpufreq_driver_target
   __cpufreq_governor (for CPUFREQ_GOV_LIMITS operation only)
   __cpufreq_set_policy
2. governer methods (.governer) must NOT take the lock_cpu_hotplug()
   lock in any way; they are called with the lock taken already
3. if your governer spawns a thread that does things, like calling
   __cpufreq_driver_target, your thread must honor rule #1.
4. the policy lock and other cpufreq internal locks nest within
   the lock_cpu_hotplug() lock.

I'm not entirely happy about how the __cpufreq_governor rule ended up
(conditional locking rule depending on the argument) but basically all
callers pass this as a constant so it's not too horrible.

The patch also removes the cpufreq_governor() function since during the
locking audit it turned out to be entirely unused (so no need to fix it)

The patch works on my testbox, but it could use more testing
(otoh... it can't be much worse than the current code)

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8d328186f774b81cfdd1eabf5c01d244f50115be..bc1088d9b379a97843c58b050f046b8b0df023f5 100644 (file)
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -364,10 +364,12 @@ static ssize_t store_##file_name                                  \
        if (ret != 1)                                                   \
                return -EINVAL;                                         \
                                                                        \
+       lock_cpu_hotplug();                                             \
        mutex_lock(&policy->lock);                                      \
        ret = __cpufreq_set_policy(policy, &new_policy);                \
        policy->user_policy.object = policy->object;                    \
        mutex_unlock(&policy->lock);                                    \
+       unlock_cpu_hotplug();                                           \
                                                                        \
        return ret ? ret : count;                                       \
 }
@@ -1197,20 +1199,18 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
  *********************************************************************/
 
 
+/* Must be called with lock_cpu_hotplug held */
 int __cpufreq_driver_target(struct cpufreq_policy *policy,
                            unsigned int target_freq,
                            unsigned int relation)
 {
        int retval = -EINVAL;
 
-       lock_cpu_hotplug();
        dprintk("target for CPU %u: %u kHz, relation %u\n", policy->cpu,
                target_freq, relation);
        if (cpu_online(policy->cpu) && cpufreq_driver->target)
                retval = cpufreq_driver->target(policy, target_freq, relation);
 
-       unlock_cpu_hotplug();
-
        return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
@@ -1225,17 +1225,23 @@ int cpufreq_driver_target(struct cpufreq_policy *policy,
        if (!policy)
                return -EINVAL;
 
+       lock_cpu_hotplug();
        mutex_lock(&policy->lock);
 
        ret = __cpufreq_driver_target(policy, target_freq, relation);
 
        mutex_unlock(&policy->lock);
+       unlock_cpu_hotplug();
 
        cpufreq_cpu_put(policy);
        return ret;
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_target);
 
+/*
+ * Locking: Must be called with the lock_cpu_hotplug() lock held
+ * when "event" is CPUFREQ_GOV_LIMITS
+ */
 
 static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event)
 {
@@ -1257,24 +1263,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event)
 }
 
 
-int cpufreq_governor(unsigned int cpu, unsigned int event)
-{
-       int ret = 0;
-       struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-
-       if (!policy)
-               return -EINVAL;
-
-       mutex_lock(&policy->lock);
-       ret = __cpufreq_governor(policy, event);
-       mutex_unlock(&policy->lock);
-
-       cpufreq_cpu_put(policy);
-       return ret;
-}
-EXPORT_SYMBOL_GPL(cpufreq_governor);
-
-
 int cpufreq_register_governor(struct cpufreq_governor *governor)
 {
        struct cpufreq_governor *t;
@@ -1342,6 +1330,9 @@ int cpufreq_get_policy(struct cpufreq_policy *policy, unsigned int cpu)
 EXPORT_SYMBOL(cpufreq_get_policy);
 
 
+/*
+ * Locking: Must be called with the lock_cpu_hotplug() lock held
+ */
 static int __cpufreq_set_policy(struct cpufreq_policy *data, struct cpufreq_policy *policy)
 {
        int ret = 0;
@@ -1436,6 +1427,8 @@ int cpufreq_set_policy(struct cpufreq_policy *policy)
        if (!data)
                return -EINVAL;
 
+       lock_cpu_hotplug();
+
        /* lock this CPU */
        mutex_lock(&data->lock);
 
@@ -1446,6 +1439,8 @@ int cpufreq_set_policy(struct cpufreq_policy *policy)
        data->user_policy.governor = data->governor;
 
        mutex_unlock(&data->lock);
+
+       unlock_cpu_hotplug();
        cpufreq_cpu_put(data);
 
        return ret;
@@ -1469,6 +1464,7 @@ int cpufreq_update_policy(unsigned int cpu)
        if (!data)
                return -ENODEV;
 
+       lock_cpu_hotplug();
        mutex_lock(&data->lock);
 
        dprintk("updating policy for CPU %u\n", cpu);
@@ -1494,7 +1490,7 @@ int cpufreq_update_policy(unsigned int cpu)
        ret = __cpufreq_set_policy(data, &policy);
 
        mutex_unlock(&data->lock);
-
+       unlock_cpu_hotplug();
        cpufreq_cpu_put(data);
        return ret;
 }

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index b3ebc8f019753db87d072914b1d699e9f36c85eb..c4c578defabfa0e5df0251d430fde58fee7d45ab 100644 (file)
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -525,7 +525,6 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
                break;
 
        case CPUFREQ_GOV_LIMITS:
-               lock_cpu_hotplug();
                mutex_lock(&dbs_mutex);
                if (policy->max < this_dbs_info->cur_policy->cur)
                        __cpufreq_driver_target(
@@ -536,7 +535,6 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
                                        this_dbs_info->cur_policy,
                                        policy->min, CPUFREQ_RELATION_L);
                mutex_unlock(&dbs_mutex);
-               unlock_cpu_hotplug();
                break;
        }
        return 0;

diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 178f0c547eb7ac928fcdce0d0f0edd3da73c4c70..52cf1f02182591f64f527144e1e3a1394ee9a050 100644 (file)
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -309,7 +309,9 @@ static void do_dbs_timer(void *data)
        if (!dbs_info->enable)
                return;
 
+       lock_cpu_hotplug();
        dbs_check_cpu(dbs_info);
+       unlock_cpu_hotplug();
        queue_delayed_work_on(cpu, kondemand_wq, &dbs_info->work,
                        usecs_to_jiffies(dbs_tuners_ins.sampling_rate));
 }
@@ -412,7 +414,6 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
                break;
 
        case CPUFREQ_GOV_LIMITS:
-               lock_cpu_hotplug();
                mutex_lock(&dbs_mutex);
                if (policy->max < this_dbs_info->cur_policy->cur)
                        __cpufreq_driver_target(this_dbs_info->cur_policy,
@@ -423,7 +424,6 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
                                                policy->min,
                                                CPUFREQ_RELATION_L);
                mutex_unlock(&dbs_mutex);
-               unlock_cpu_hotplug();
                break;
        }
        return 0;

diff --git a/drivers/cpufreq/cpufreq_userspace.c b/drivers/cpufreq/cpufreq_userspace.c
index 44ae5e5b94cf4a30f4d1ab18287f72ec28caadb1..a06c204589cdf7c9f8e0e68e38c337f1535ace66 100644 (file)
--- a/drivers/cpufreq/cpufreq_userspace.c
+++ b/drivers/cpufreq/cpufreq_userspace.c
@@ -18,6 +18,7 @@
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/cpufreq.h>
+#include <linux/cpu.h>
 #include <linux/types.h>
 #include <linux/fs.h>
 #include <linux/sysfs.h>
@@ -70,6 +71,7 @@ static int cpufreq_set(unsigned int freq, struct cpufreq_policy *policy)
 
        dprintk("cpufreq_set for cpu %u, freq %u kHz\n", policy->cpu, freq);
 
+       lock_cpu_hotplug();
        mutex_lock(&userspace_mutex);
        if (!cpu_is_managed[policy->cpu])
                goto err;
@@ -92,6 +94,7 @@ static int cpufreq_set(unsigned int freq, struct cpufreq_policy *policy)
 
  err:
        mutex_unlock(&userspace_mutex);
+       unlock_cpu_hotplug();
        return ret;
 }
 

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 35e137636b0b4c6dc44cdfc04ef53ec787ffadfd..4ea39fee99c7938e4bd9dfec951773b025ada1a7 100644 (file)
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -172,9 +172,6 @@ extern int __cpufreq_driver_target(struct cpufreq_policy *policy,
                                   unsigned int relation);
 
 
-/* pass an event to the cpufreq governor */
-int cpufreq_governor(unsigned int cpu, unsigned int event);
-
 int cpufreq_register_governor(struct cpufreq_governor *governor);
 void cpufreq_unregister_governor(struct cpufreq_governor *governor);