[PATCH 0/4] CPUFreq Fixes for 3.9

[PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

Hi Rafael,

This is another unplanned patchset for all the platforms that i broke. :)

Okay, there are two important fixes (1 & 4) and two general cleanups (2 & 3). I
hope most of the issues would be resolved by these and we would be able to push
clean cpufreq core into 3.9.

I have pushed them in my for-rafael branch at:

http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/for-rafael

@Artem & Valdis: Please test them and reply with your Tested-by's (in case they
work :) ).

Viresh Kumar (4):
  cpufreq: governors: Fix WARN_ON() for multi-policy platforms
  cpufreq: Remove unused HOTPLUG_CPU code
  cpufreq: Create a macro for unlock_policy_rwsem{read,write}
  cpufreq: Fix locking issues

 drivers/cpufreq/cpufreq.c          | 126 ++++++++++++++++++-------------------
 drivers/cpufreq/cpufreq_governor.c |  32 ++++++----
 2 files changed, 79 insertions(+), 79 deletions(-)

-- 
1.7.12.rc2.18.g61b472e

[PATCH 1/4] cpufreq: governors: Fix WARN_ON() for multi-policy platforms

[Viresh Kumar]

On multi-policy systems there is a single instance of governor for both the
policies (if same governor is chosen for both policies). With the code update
from following patches:

8eeed09 cpufreq: governors: Get rid of dbs_data->enable field
b394058 cpufreq: governors: Reset tunables only for cpufreq_unregister_governor()

We are creating/removing sysfs directory of governor for for every call to
GOV_START and STOP. This would fail for multi-policy system as there is a
per-policy call to START/STOP.

This patch reuses the governor->initialized variable to detect total users of
governor.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c          |  6 ++++--
 drivers/cpufreq/cpufreq_governor.c | 32 +++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ccc598a..3b941a1 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1567,8 +1567,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 						policy->cpu, event);
 	ret = policy->governor->governor(policy, event);
 
-	if (!policy->governor->initialized && (event == CPUFREQ_GOV_START))
-		policy->governor->initialized = 1;
+	if (event == CPUFREQ_GOV_START)
+		policy->governor->initialized++;
+	else if (event == CPUFREQ_GOV_STOP)
+		policy->governor->initialized--;
 
 	/* we keep one module reference alive for
 			each CPU governed by this CPU */
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index e4a306c..5a76086 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -247,11 +247,13 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 					     dbs_data->gov_dbs_timer);
 		}
 
-		rc = sysfs_create_group(cpufreq_global_kobject,
-				dbs_data->attr_group);
-		if (rc) {
-			mutex_unlock(&dbs_data->mutex);
-			return rc;
+		if (!policy->governor->initialized) {
+			rc = sysfs_create_group(cpufreq_global_kobject,
+					dbs_data->attr_group);
+			if (rc) {
+				mutex_unlock(&dbs_data->mutex);
+				return rc;
+			}
 		}
 
 		/*
@@ -262,13 +264,15 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 			cs_dbs_info->down_skip = 0;
 			cs_dbs_info->enable = 1;
 			cs_dbs_info->requested_freq = policy->cur;
-			cpufreq_register_notifier(cs_ops->notifier_block,
-					CPUFREQ_TRANSITION_NOTIFIER);
 
-			if (!policy->governor->initialized)
+			if (!policy->governor->initialized) {
+				cpufreq_register_notifier(cs_ops->notifier_block,
+						CPUFREQ_TRANSITION_NOTIFIER);
+
 				dbs_data->min_sampling_rate =
 					MIN_SAMPLING_RATE_RATIO *
 					jiffies_to_usecs(10);
+			}
 		} else {
 			od_dbs_info->rate_mult = 1;
 			od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
@@ -311,11 +315,13 @@ unlock:
 		mutex_lock(&dbs_data->mutex);
 		mutex_destroy(&cpu_cdbs->timer_mutex);
 
-		sysfs_remove_group(cpufreq_global_kobject,
-				dbs_data->attr_group);
-		if (dbs_data->governor == GOV_CONSERVATIVE)
-			cpufreq_unregister_notifier(cs_ops->notifier_block,
-					CPUFREQ_TRANSITION_NOTIFIER);
+		if (policy->governor->initialized == 1) {
+			sysfs_remove_group(cpufreq_global_kobject,
+					dbs_data->attr_group);
+			if (dbs_data->governor == GOV_CONSERVATIVE)
+				cpufreq_unregister_notifier(cs_ops->notifier_block,
+						CPUFREQ_TRANSITION_NOTIFIER);
+		}
 		mutex_unlock(&dbs_data->mutex);
 
 		break;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 1/4] cpufreq: governors: Fix WARN_ON() for multi-policy platforms

[Viresh Kumar]

On multi-policy systems there is a single instance of governor for both the
policies (if same governor is chosen for both policies). With the code update
from following patches:

8eeed09 cpufreq: governors: Get rid of dbs_data->enable field
b394058 cpufreq: governors: Reset tunables only for cpufreq_unregister_governor()

We are creating/removing sysfs directory of governor for for every call to
GOV_START and STOP. This would fail for multi-policy system as there is a
per-policy call to START/STOP.

This patch reuses the governor->initialized variable to detect total users of
governor.

Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/cpufreq/cpufreq.c          |  6 ++++--
 drivers/cpufreq/cpufreq_governor.c | 32 +++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ccc598a..3b941a1 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1567,8 +1567,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 						policy->cpu, event);
 	ret = policy->governor->governor(policy, event);
 
-	if (!policy->governor->initialized && (event == CPUFREQ_GOV_START))
-		policy->governor->initialized = 1;
+	if (event == CPUFREQ_GOV_START)
+		policy->governor->initialized++;
+	else if (event == CPUFREQ_GOV_STOP)
+		policy->governor->initialized--;
 
 	/* we keep one module reference alive for
 			each CPU governed by this CPU */
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index e4a306c..5a76086 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -247,11 +247,13 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 					     dbs_data->gov_dbs_timer);
 		}
 
-		rc = sysfs_create_group(cpufreq_global_kobject,
-				dbs_data->attr_group);
-		if (rc) {
-			mutex_unlock(&dbs_data->mutex);
-			return rc;
+		if (!policy->governor->initialized) {
+			rc = sysfs_create_group(cpufreq_global_kobject,
+					dbs_data->attr_group);
+			if (rc) {
+				mutex_unlock(&dbs_data->mutex);
+				return rc;
+			}
 		}
 
 		/*
@@ -262,13 +264,15 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 			cs_dbs_info->down_skip = 0;
 			cs_dbs_info->enable = 1;
 			cs_dbs_info->requested_freq = policy->cur;
-			cpufreq_register_notifier(cs_ops->notifier_block,
-					CPUFREQ_TRANSITION_NOTIFIER);
 
-			if (!policy->governor->initialized)
+			if (!policy->governor->initialized) {
+				cpufreq_register_notifier(cs_ops->notifier_block,
+						CPUFREQ_TRANSITION_NOTIFIER);
+
 				dbs_data->min_sampling_rate =
 					MIN_SAMPLING_RATE_RATIO *
 					jiffies_to_usecs(10);
+			}
 		} else {
 			od_dbs_info->rate_mult = 1;
 			od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
@@ -311,11 +315,13 @@ unlock:
 		mutex_lock(&dbs_data->mutex);
 		mutex_destroy(&cpu_cdbs->timer_mutex);
 
-		sysfs_remove_group(cpufreq_global_kobject,
-				dbs_data->attr_group);
-		if (dbs_data->governor == GOV_CONSERVATIVE)
-			cpufreq_unregister_notifier(cs_ops->notifier_block,
-					CPUFREQ_TRANSITION_NOTIFIER);
+		if (policy->governor->initialized == 1) {
+			sysfs_remove_group(cpufreq_global_kobject,
+					dbs_data->attr_group);
+			if (dbs_data->governor == GOV_CONSERVATIVE)
+				cpufreq_unregister_notifier(cs_ops->notifier_block,
+						CPUFREQ_TRANSITION_NOTIFIER);
+		}
 		mutex_unlock(&dbs_data->mutex);
 
 		break;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/4] cpufreq: Remove unused HOTPLUG_CPU code

[Viresh Kumar]

Because the sibling cpu of any online cpu is identified very early in
cpufreq_add_dev(), below code is never executed. And so can be removed.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3b941a1..795b0e8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -858,7 +858,7 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	unsigned int j, cpu = dev->id;
-	int ret = -ENOMEM, found = 0;
+	int ret = -ENOMEM;
 	struct cpufreq_policy *policy;
 	struct cpufreq_driver *driver;
 	unsigned long flags;
@@ -908,6 +908,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 		goto err_free_cpumask;
 
 	policy->cpu = cpu;
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
 	/* Initially set CPU itself as the policy_cpu */
@@ -918,20 +919,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
-	/* Set governor before ->init, so that driver could check it */
-#ifdef CONFIG_HOTPLUG_CPU
-	for_each_online_cpu(sibling) {
-		struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
-		if (cp && cp->governor &&
-		    (cpumask_test_cpu(cpu, cp->related_cpus))) {
-			policy->governor = cp->governor;
-			found = 1;
-			break;
-		}
-	}
-#endif
-	if (!found)
-		policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
 	/* call driver. From then on the cpufreq must be able
 	 * to accept all calls to ->verify and ->setpolicy for this CPU
 	 */
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/4] cpufreq: Remove unused HOTPLUG_CPU code

[Viresh Kumar]

Because the sibling cpu of any online cpu is identified very early in
cpufreq_add_dev(), below code is never executed. And so can be removed.

Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/cpufreq/cpufreq.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3b941a1..795b0e8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -858,7 +858,7 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	unsigned int j, cpu = dev->id;
-	int ret = -ENOMEM, found = 0;
+	int ret = -ENOMEM;
 	struct cpufreq_policy *policy;
 	struct cpufreq_driver *driver;
 	unsigned long flags;
@@ -908,6 +908,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 		goto err_free_cpumask;
 
 	policy->cpu = cpu;
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
 	/* Initially set CPU itself as the policy_cpu */
@@ -918,20 +919,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
-	/* Set governor before ->init, so that driver could check it */
-#ifdef CONFIG_HOTPLUG_CPU
-	for_each_online_cpu(sibling) {
-		struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
-		if (cp && cp->governor &&
-		    (cpumask_test_cpu(cpu, cp->related_cpus))) {
-			policy->governor = cp->governor;
-			found = 1;
-			break;
-		}
-	}
-#endif
-	if (!found)
-		policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
 	/* call driver. From then on the cpufreq must be able
 	 * to accept all calls to ->verify and ->setpolicy for this CPU
 	 */
-- 
1.7.12.rc2.18.g61b472e

[PATCH 3/4] cpufreq: Create a macro for unlock_policy_rwsem{read,write}

[Viresh Kumar]

On the lines of macro: lock_policy_rwsem, we can create another macro for
unlock_policy_rwsem. Lets do it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 795b0e8..c46aed4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -70,8 +70,7 @@ static DEFINE_PER_CPU(int, cpufreq_policy_cpu);
 static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
 
 #define lock_policy_rwsem(mode, cpu)					\
-static int lock_policy_rwsem_##mode					\
-(int cpu)								\
+static int lock_policy_rwsem_##mode(int cpu)				\
 {									\
 	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);		\
 	BUG_ON(policy_cpu == -1);					\
@@ -81,23 +80,18 @@ static int lock_policy_rwsem_##mode					\
 }
 
 lock_policy_rwsem(read, cpu);
-
 lock_policy_rwsem(write, cpu);
 
-static void unlock_policy_rwsem_read(int cpu)
-{
-	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);
-	BUG_ON(policy_cpu == -1);
-	up_read(&per_cpu(cpu_policy_rwsem, policy_cpu));
-}
-
-static void unlock_policy_rwsem_write(int cpu)
-{
-	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);
-	BUG_ON(policy_cpu == -1);
-	up_write(&per_cpu(cpu_policy_rwsem, policy_cpu));
+#define unlock_policy_rwsem(mode, cpu)					\
+static void unlock_policy_rwsem_##mode(int cpu)				\
+{									\
+	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);		\
+	BUG_ON(policy_cpu == -1);					\
+	up_##mode(&per_cpu(cpu_policy_rwsem, policy_cpu));		\
 }
 
+unlock_policy_rwsem(read, cpu);
+unlock_policy_rwsem(write, cpu);
 
 /* internal prototypes */
 static int __cpufreq_governor(struct cpufreq_policy *policy,
-- 
1.7.12.rc2.18.g61b472e

[PATCH 3/4] cpufreq: Create a macro for unlock_policy_rwsem{read, write}

[Viresh Kumar]

On the lines of macro: lock_policy_rwsem, we can create another macro for
unlock_policy_rwsem. Lets do it.

Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/cpufreq/cpufreq.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 795b0e8..c46aed4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -70,8 +70,7 @@ static DEFINE_PER_CPU(int, cpufreq_policy_cpu);
 static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
 
 #define lock_policy_rwsem(mode, cpu)					\
-static int lock_policy_rwsem_##mode					\
-(int cpu)								\
+static int lock_policy_rwsem_##mode(int cpu)				\
 {									\
 	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);		\
 	BUG_ON(policy_cpu == -1);					\
@@ -81,23 +80,18 @@ static int lock_policy_rwsem_##mode					\
 }
 
 lock_policy_rwsem(read, cpu);
-
 lock_policy_rwsem(write, cpu);
 
-static void unlock_policy_rwsem_read(int cpu)
-{
-	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);
-	BUG_ON(policy_cpu == -1);
-	up_read(&per_cpu(cpu_policy_rwsem, policy_cpu));
-}
-
-static void unlock_policy_rwsem_write(int cpu)
-{
-	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);
-	BUG_ON(policy_cpu == -1);
-	up_write(&per_cpu(cpu_policy_rwsem, policy_cpu));
+#define unlock_policy_rwsem(mode, cpu)					\
+static void unlock_policy_rwsem_##mode(int cpu)				\
+{									\
+	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);		\
+	BUG_ON(policy_cpu == -1);					\
+	up_##mode(&per_cpu(cpu_policy_rwsem, policy_cpu));		\
 }
 
+unlock_policy_rwsem(read, cpu);
+unlock_policy_rwsem(write, cpu);
 
 /* internal prototypes */
 static int __cpufreq_governor(struct cpufreq_policy *policy,
-- 
1.7.12.rc2.18.g61b472e

[PATCH 4/4] cpufreq: Fix locking issues

[Viresh Kumar]

cpufreq core uses two locks:
- cpufreq_driver_lock: General lock for driver and cpufreq_cpu_data array.
- cpu_policy_rwsemfix locking: per CPU reader-writer semaphore designed to cure
  all cpufreq/hotplug/workqueue/etc related lock issues.

These locks were not used properly and are placed against their principle
(present before their definition) at various places. This patch is an attempt to
fix their use.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 79 +++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 34 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c46aed4..79511ab 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -59,8 +59,6 @@ static DEFINE_SPINLOCK(cpufreq_driver_lock);
  *   mode before doing so.
  *
  * Additional rules:
- * - All holders of the lock should check to make sure that the CPU they
- *   are concerned with are online after they get the lock.
  * - Governor routines that can be called in cpufreq hotplug path should not
  *   take this sem as top level hotplug notifier handler takes this.
  * - Lock should not be held across
@@ -257,6 +255,7 @@ void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
 {
 	struct cpufreq_policy *policy;
 	struct cpufreq_driver *driver;
+	unsigned long flags;
 
 	BUG_ON(irqs_disabled());
 
@@ -269,7 +268,10 @@ void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
 	pr_debug("notification %u of frequency transition to %u kHz\n",
 		state, freqs->new);
 
+	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	policy = per_cpu(cpufreq_cpu_data, freqs->cpu);
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
@@ -792,8 +794,10 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
 
 	if (ret) {
 		pr_debug("setting policy failed\n");
+		spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		if (driver->exit)
 			driver->exit(policy);
+		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
 	return ret;
 
@@ -814,22 +818,22 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 	policy = cpufreq_cpu_get(sibling);
 	WARN_ON(!policy);
 
-	per_cpu(cpufreq_policy_cpu, cpu) = policy->cpu;
-
-	lock_policy_rwsem_write(cpu);
-
 	__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 
+	lock_policy_rwsem_write(sibling);
+
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
+
 	cpumask_set_cpu(cpu, policy->cpus);
+	per_cpu(cpufreq_policy_cpu, cpu) = policy->cpu;
 	per_cpu(cpufreq_cpu_data, cpu) = policy;
 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	unlock_policy_rwsem_write(sibling);
+
 	__cpufreq_governor(policy, CPUFREQ_GOV_START);
 	__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
-	unlock_policy_rwsem_write(cpu);
-
 	ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 	if (ret) {
 		cpufreq_cpu_put(policy);
@@ -878,11 +882,15 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 
 #ifdef CONFIG_HOTPLUG_CPU
 	/* Check if this cpu was hot-unplugged earlier and has siblings */
+	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	for_each_online_cpu(sibling) {
 		struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
-		if (cp && cpumask_test_cpu(cpu, cp->related_cpus))
+		if (cp && cpumask_test_cpu(cpu, cp->related_cpus)) {
+			spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 			return cpufreq_add_policy_cpu(cpu, sibling, dev);
+		}
 	}
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 #endif
 #endif
 
@@ -907,20 +915,21 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 
 	/* Initially set CPU itself as the policy_cpu */
 	per_cpu(cpufreq_policy_cpu, cpu) = cpu;
-	ret = (lock_policy_rwsem_write(cpu) < 0);
-	WARN_ON(ret);
 
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
+	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	/* call driver. From then on the cpufreq must be able
 	 * to accept all calls to ->verify and ->setpolicy for this CPU
 	 */
 	ret = driver->init(policy);
 	if (ret) {
 		pr_debug("initialization failed\n");
-		goto err_unlock_policy;
+		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		goto err_set_policy_cpu;
 	}
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	/* related cpus should atleast have policy->cpus */
 	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
@@ -950,8 +959,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	if (ret)
 		goto err_out_unregister;
 
-	unlock_policy_rwsem_write(cpu);
-
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
 	module_put(rcu_dereference(cpufreq_driver)->owner);
 	pr_debug("initialization complete\n");
@@ -967,8 +974,8 @@ err_out_unregister:
 	kobject_put(&policy->kobj);
 	wait_for_completion(&policy->kobj_unregister);
 
-err_unlock_policy:
-	unlock_policy_rwsem_write(cpu);
+err_set_policy_cpu:
+	per_cpu(cpufreq_policy_cpu, cpu) = -1;
 	free_cpumask_var(policy->related_cpus);
 err_free_cpumask:
 	free_cpumask_var(policy->cpus);
@@ -1017,12 +1024,14 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
 
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
+
 	data = per_cpu(cpufreq_cpu_data, cpu);
+	per_cpu(cpufreq_cpu_data, cpu) = NULL;
+
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	if (!data) {
 		pr_debug("%s: No cpu_data found\n", __func__);
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		unlock_policy_rwsem_write(cpu);
 		return -EINVAL;
 	}
 
@@ -1034,9 +1043,10 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 			CPUFREQ_NAME_LEN);
 #endif
 
-	per_cpu(cpufreq_cpu_data, cpu) = NULL;
+	WARN_ON(lock_policy_rwsem_write(cpu));
 	cpus = cpumask_weight(data->cpus);
 	cpumask_clear_cpu(cpu, data->cpus);
+	unlock_policy_rwsem_write(cpu);
 
 	if (cpu != data->cpu) {
 		sysfs_remove_link(&dev->kobj, "cpufreq");
@@ -1047,31 +1057,37 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 		ret = kobject_move(&data->kobj, &cpu_dev->kobj);
 		if (ret) {
 			pr_err("%s: Failed to move kobj: %d", __func__, ret);
+
+			WARN_ON(lock_policy_rwsem_write(cpu));
 			cpumask_set_cpu(cpu, data->cpus);
-			ret = sysfs_create_link(&cpu_dev->kobj, &data->kobj,
-					"cpufreq");
+
+			spin_lock_irqsave(&cpufreq_driver_lock, flags);
+			per_cpu(cpufreq_cpu_data, cpu) = data;
 			spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 			unlock_policy_rwsem_write(cpu);
+
+			ret = sysfs_create_link(&cpu_dev->kobj, &data->kobj,
+					"cpufreq");
 			return -EINVAL;
 		}
 
+		WARN_ON(lock_policy_rwsem_write(cpu));
 		update_policy_cpu(data, cpu_dev->id);
+		unlock_policy_rwsem_write(cpu);
 		pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
 				__func__, cpu_dev->id, cpu);
 	}
 
-	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
 	cpufreq_cpu_put(data);
-	unlock_policy_rwsem_write(cpu);
 
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
-		lock_policy_rwsem_write(cpu);
+		lock_policy_rwsem_read(cpu);
 		kobj = &data->kobj;
 		cmp = &data->kobj_unregister;
-		unlock_policy_rwsem_write(cpu);
+		unlock_policy_rwsem_read(cpu);
 		kobject_put(kobj);
 
 		/* we need to make sure that the underlying kobj is actually
@@ -1082,10 +1098,10 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 		wait_for_completion(cmp);
 		pr_debug("wait complete\n");
 
-		lock_policy_rwsem_write(cpu);
+		spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		if (driver->exit)
 			driver->exit(data);
-		unlock_policy_rwsem_write(cpu);
+		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
@@ -1095,6 +1111,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
 	}
 
+	per_cpu(cpufreq_policy_cpu, cpu) = -1;
 	return 0;
 }
 
@@ -1107,9 +1124,6 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	if (cpu_is_offline(cpu))
 		return 0;
 
-	if (unlikely(lock_policy_rwsem_write(cpu)))
-		BUG();
-
 	retval = __cpufreq_remove_dev(dev, sif);
 	return retval;
 }
@@ -1808,9 +1822,6 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
 			break;
 		case CPU_DOWN_PREPARE:
 		case CPU_DOWN_PREPARE_FROZEN:
-			if (unlikely(lock_policy_rwsem_write(cpu)))
-				BUG();
-
 			__cpufreq_remove_dev(dev, NULL);
 			break;
 		case CPU_DOWN_FAILED:
-- 
1.7.12.rc2.18.g61b472e

[PATCH 4/4] cpufreq: Fix locking issues

[Viresh Kumar]

cpufreq core uses two locks:
- cpufreq_driver_lock: General lock for driver and cpufreq_cpu_data array.
- cpu_policy_rwsemfix locking: per CPU reader-writer semaphore designed to cure
  all cpufreq/hotplug/workqueue/etc related lock issues.

These locks were not used properly and are placed against their principle
(present before their definition) at various places. This patch is an attempt to
fix their use.

Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/cpufreq/cpufreq.c | 79 +++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 34 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c46aed4..79511ab 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -59,8 +59,6 @@ static DEFINE_SPINLOCK(cpufreq_driver_lock);
  *   mode before doing so.
  *
  * Additional rules:
- * - All holders of the lock should check to make sure that the CPU they
- *   are concerned with are online after they get the lock.
  * - Governor routines that can be called in cpufreq hotplug path should not
  *   take this sem as top level hotplug notifier handler takes this.
  * - Lock should not be held across
@@ -257,6 +255,7 @@ void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
 {
 	struct cpufreq_policy *policy;
 	struct cpufreq_driver *driver;
+	unsigned long flags;
 
 	BUG_ON(irqs_disabled());
 
@@ -269,7 +268,10 @@ void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
 	pr_debug("notification %u of frequency transition to %u kHz\n",
 		state, freqs->new);
 
+	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	policy = per_cpu(cpufreq_cpu_data, freqs->cpu);
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	switch (state) {
 
 	case CPUFREQ_PRECHANGE:
@@ -792,8 +794,10 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
 
 	if (ret) {
 		pr_debug("setting policy failed\n");
+		spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		if (driver->exit)
 			driver->exit(policy);
+		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
 	return ret;
 
@@ -814,22 +818,22 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 	policy = cpufreq_cpu_get(sibling);
 	WARN_ON(!policy);
 
-	per_cpu(cpufreq_policy_cpu, cpu) = policy->cpu;
-
-	lock_policy_rwsem_write(cpu);
-
 	__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 
+	lock_policy_rwsem_write(sibling);
+
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
+
 	cpumask_set_cpu(cpu, policy->cpus);
+	per_cpu(cpufreq_policy_cpu, cpu) = policy->cpu;
 	per_cpu(cpufreq_cpu_data, cpu) = policy;
 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	unlock_policy_rwsem_write(sibling);
+
 	__cpufreq_governor(policy, CPUFREQ_GOV_START);
 	__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
-	unlock_policy_rwsem_write(cpu);
-
 	ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 	if (ret) {
 		cpufreq_cpu_put(policy);
@@ -878,11 +882,15 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 
 #ifdef CONFIG_HOTPLUG_CPU
 	/* Check if this cpu was hot-unplugged earlier and has siblings */
+	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	for_each_online_cpu(sibling) {
 		struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
-		if (cp && cpumask_test_cpu(cpu, cp->related_cpus))
+		if (cp && cpumask_test_cpu(cpu, cp->related_cpus)) {
+			spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 			return cpufreq_add_policy_cpu(cpu, sibling, dev);
+		}
 	}
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 #endif
 #endif
 
@@ -907,20 +915,21 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 
 	/* Initially set CPU itself as the policy_cpu */
 	per_cpu(cpufreq_policy_cpu, cpu) = cpu;
-	ret = (lock_policy_rwsem_write(cpu) < 0);
-	WARN_ON(ret);
 
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
+	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	/* call driver. From then on the cpufreq must be able
 	 * to accept all calls to ->verify and ->setpolicy for this CPU
 	 */
 	ret = driver->init(policy);
 	if (ret) {
 		pr_debug("initialization failed\n");
-		goto err_unlock_policy;
+		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+		goto err_set_policy_cpu;
 	}
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	/* related cpus should atleast have policy->cpus */
 	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
@@ -950,8 +959,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	if (ret)
 		goto err_out_unregister;
 
-	unlock_policy_rwsem_write(cpu);
-
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
 	module_put(rcu_dereference(cpufreq_driver)->owner);
 	pr_debug("initialization complete\n");
@@ -967,8 +974,8 @@ err_out_unregister:
 	kobject_put(&policy->kobj);
 	wait_for_completion(&policy->kobj_unregister);
 
-err_unlock_policy:
-	unlock_policy_rwsem_write(cpu);
+err_set_policy_cpu:
+	per_cpu(cpufreq_policy_cpu, cpu) = -1;
 	free_cpumask_var(policy->related_cpus);
 err_free_cpumask:
 	free_cpumask_var(policy->cpus);
@@ -1017,12 +1024,14 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
 
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
+
 	data = per_cpu(cpufreq_cpu_data, cpu);
+	per_cpu(cpufreq_cpu_data, cpu) = NULL;
+
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	if (!data) {
 		pr_debug("%s: No cpu_data found\n", __func__);
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
-		unlock_policy_rwsem_write(cpu);
 		return -EINVAL;
 	}
 
@@ -1034,9 +1043,10 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 			CPUFREQ_NAME_LEN);
 #endif
 
-	per_cpu(cpufreq_cpu_data, cpu) = NULL;
+	WARN_ON(lock_policy_rwsem_write(cpu));
 	cpus = cpumask_weight(data->cpus);
 	cpumask_clear_cpu(cpu, data->cpus);
+	unlock_policy_rwsem_write(cpu);
 
 	if (cpu != data->cpu) {
 		sysfs_remove_link(&dev->kobj, "cpufreq");
@@ -1047,31 +1057,37 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 		ret = kobject_move(&data->kobj, &cpu_dev->kobj);
 		if (ret) {
 			pr_err("%s: Failed to move kobj: %d", __func__, ret);
+
+			WARN_ON(lock_policy_rwsem_write(cpu));
 			cpumask_set_cpu(cpu, data->cpus);
-			ret = sysfs_create_link(&cpu_dev->kobj, &data->kobj,
-					"cpufreq");
+
+			spin_lock_irqsave(&cpufreq_driver_lock, flags);
+			per_cpu(cpufreq_cpu_data, cpu) = data;
 			spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 			unlock_policy_rwsem_write(cpu);
+
+			ret = sysfs_create_link(&cpu_dev->kobj, &data->kobj,
+					"cpufreq");
 			return -EINVAL;
 		}
 
+		WARN_ON(lock_policy_rwsem_write(cpu));
 		update_policy_cpu(data, cpu_dev->id);
+		unlock_policy_rwsem_write(cpu);
 		pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
 				__func__, cpu_dev->id, cpu);
 	}
 
-	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
 	cpufreq_cpu_put(data);
-	unlock_policy_rwsem_write(cpu);
 
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
-		lock_policy_rwsem_write(cpu);
+		lock_policy_rwsem_read(cpu);
 		kobj = &data->kobj;
 		cmp = &data->kobj_unregister;
-		unlock_policy_rwsem_write(cpu);
+		unlock_policy_rwsem_read(cpu);
 		kobject_put(kobj);
 
 		/* we need to make sure that the underlying kobj is actually
@@ -1082,10 +1098,10 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 		wait_for_completion(cmp);
 		pr_debug("wait complete\n");
 
-		lock_policy_rwsem_write(cpu);
+		spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		if (driver->exit)
 			driver->exit(data);
-		unlock_policy_rwsem_write(cpu);
+		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
@@ -1095,6 +1111,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
 	}
 
+	per_cpu(cpufreq_policy_cpu, cpu) = -1;
 	return 0;
 }
 
@@ -1107,9 +1124,6 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	if (cpu_is_offline(cpu))
 		return 0;
 
-	if (unlikely(lock_policy_rwsem_write(cpu)))
-		BUG();
-
 	retval = __cpufreq_remove_dev(dev, sif);
 	return retval;
 }
@@ -1808,9 +1822,6 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
 			break;
 		case CPU_DOWN_PREPARE:
 		case CPU_DOWN_PREPARE_FROZEN:
-			if (unlikely(lock_policy_rwsem_write(cpu)))
-				BUG();
-
 			__cpufreq_remove_dev(dev, NULL);
 			break;
 		case CPU_DOWN_FAILED:
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Thursday, February 07, 2013 03:57:42 PM Viresh Kumar wrote:
> Hi Rafael,
> 
> This is another unplanned patchset for all the platforms that i broke. :)
> 
> Okay, there are two important fixes (1 & 4) and two general cleanups (2 & 3). I
> hope most of the issues would be resolved by these and we would be able to push
> clean cpufreq core into 3.9.
> 
> I have pushed them in my for-rafael branch at:
> 
> http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/for-rafael
> 
> @Artem & Valdis: Please test them and reply with your Tested-by's (in case they
> work :) ).
> 
> Viresh Kumar (4):
>   cpufreq: governors: Fix WARN_ON() for multi-policy platforms
>   cpufreq: Remove unused HOTPLUG_CPU code
>   cpufreq: Create a macro for unlock_policy_rwsem{read,write}
>   cpufreq: Fix locking issues
> 
>  drivers/cpufreq/cpufreq.c          | 126 ++++++++++++++++++-------------------
>  drivers/cpufreq/cpufreq_governor.c |  32 ++++++----
>  2 files changed, 79 insertions(+), 79 deletions(-)

I think they all make sense, so applied to linux-next.

I would prefer not to make any more changes to cpufreq before v3.9 from now on,
except for fixes and maybe the Drik's patchset that I kind of scheduled for
merging into bleeding-edge later today.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

On 7 February 2013 18:35, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> I think they all make sense, so applied to linux-next.
>
> I would prefer not to make any more changes to cpufreq before v3.9 from now on,
> except for fixes and maybe the Drik's patchset that I kind of scheduled for

Dirk :)

> merging into bleeding-edge later today.

I probably have few more for you. Some sparse warnings to be fixed for
Dirks work and an dangling exynos patch which is waiting for your reply :)

--
viresh

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Thursday, February 07, 2013 06:52:20 PM Viresh Kumar wrote:
> On 7 February 2013 18:35, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > I think they all make sense, so applied to linux-next.
> >
> > I would prefer not to make any more changes to cpufreq before v3.9 from now on,
> > except for fixes and maybe the Drik's patchset that I kind of scheduled for
> 
> Dirk :)

Yes, sorry Dirk.

> > merging into bleeding-edge later today.
> 
> I probably have few more for you. Some sparse warnings to be fixed for
> Dirks work and an dangling exynos patch which is waiting for your reply :)

Which Exynos patch?

BTW, there still are locking problems in linux-next.  Why do we need
to take cpufreq_driver_lock() around driver->init() in cpufreq_add_dev(),
in particular?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

On 8 February 2013 04:37, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, February 07, 2013 06:52:20 PM Viresh Kumar wrote:
>> On 7 February 2013 18:35, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > I think they all make sense, so applied to linux-next.
>> >
>> > I would prefer not to make any more changes to cpufreq before v3.9 from now on,
>> > except for fixes and maybe the Drik's patchset that I kind of scheduled for
>>
>> Dirk :)
>
> Yes, sorry Dirk.
>
>> > merging into bleeding-edge later today.
>>
>> I probably have few more for you. Some sparse warnings to be fixed for
>> Dirks work and an dangling exynos patch which is waiting for your reply :)
>
> Which Exynos patch?

https://lkml.org/lkml/2013/1/30/592

> BTW, there still are locking problems in linux-next.  Why do we need
> to take cpufreq_driver_lock() around driver->init() in cpufreq_add_dev(),
> in particular?

I thought cpufreq provides atomicity to all drivers callbacks and that's why
i had those around it :(

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

On 8 February 2013 04:37, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> BTW, there still are locking problems in linux-next.  Why do we need
> to take cpufreq_driver_lock() around driver->init() in cpufreq_add_dev(),
> in particular?

I thought a bit more and realized there is no such limitation on
cpufreq_driver->ops about calling routines which can sleep. And thus
we shoudln't
have locks around any of these. I have got a patch for it, that i
would fold-back into
the original patch that introduced locking fixes (attached too for testing):

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Fri, 8 Feb 2013 10:35:31 +0530
Subject: [PATCH] cpufreq: Remove unnecessary locking

I have placed some locks intentionally around calls to driver->ops (init/exit),
which look to be wrong as these calls can call routines that potentially sleep.

Lets remove these locks.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5d8a422..04aab05 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -795,10 +795,8 @@ static int cpufreq_add_dev_interface(unsigned int cpu,

 	if (ret) {
 		pr_debug("setting policy failed\n");
-		spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		if (driver->exit)
 			driver->exit(policy);
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
 	return ret;

@@ -920,17 +918,14 @@ static int cpufreq_add_dev(struct device *dev,
struct subsys_interface *sif)
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);

-	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	/* call driver. From then on the cpufreq must be able
 	 * to accept all calls to ->verify and ->setpolicy for this CPU
 	 */
 	ret = driver->init(policy);
 	if (ret) {
 		pr_debug("initialization failed\n");
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 		goto err_set_policy_cpu;
 	}
-	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);

 	/* related cpus should atleast have policy->cpus */
 	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
@@ -1100,10 +1095,8 @@ static int __cpufreq_remove_dev(struct device
*dev, struct subsys_interface *sif
 		wait_for_completion(cmp);
 		pr_debug("wait complete\n");

-		spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		if (driver->exit)
 			driver->exit(data);
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);

 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);

[-- Attachment #2: 0001-cpufreq-Remove-unnecessary-locking.patch --]
[-- Type: application/octet-stream, Size: 2230 bytes --]

From 564df37b5db51182e5e6e78969f40a2a71159b9c Mon Sep 17 00:00:00 2001
Message-Id: <564df37b5db51182e5e6e78969f40a2a71159b9c.1360300126.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Fri, 8 Feb 2013 10:35:31 +0530
Subject: [PATCH] cpufreq: Remove unnecessary locking

I have placed some locks intentionally around calls to driver->ops (init/exit),
which look to be wrong as these calls can call routines that potentially sleep.

Lets remove these locks.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5d8a422..04aab05 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -795,10 +795,8 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
 
 	if (ret) {
 		pr_debug("setting policy failed\n");
-		spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		if (driver->exit)
 			driver->exit(policy);
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
 	return ret;
 
@@ -920,17 +918,14 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
-	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	/* call driver. From then on the cpufreq must be able
 	 * to accept all calls to ->verify and ->setpolicy for this CPU
 	 */
 	ret = driver->init(policy);
 	if (ret) {
 		pr_debug("initialization failed\n");
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 		goto err_set_policy_cpu;
 	}
-	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	/* related cpus should atleast have policy->cpus */
 	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
@@ -1100,10 +1095,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 		wait_for_completion(cmp);
 		pr_debug("wait complete\n");
 
-		spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		if (driver->exit)
 			driver->exit(data);
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Artem Savkov]

On Fri, Feb 08, 2013 at 10:39:13AM +0530, Viresh Kumar wrote:
> On 8 February 2013 04:37, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > BTW, there still are locking problems in linux-next.  Why do we need
> > to take cpufreq_driver_lock() around driver->init() in cpufreq_add_dev(),
> > in particular?
> I thought a bit more and realized there is no such limitation on
> cpufreq_driver->ops about calling routines which can sleep. And thus
> we shoudln't
> have locks around any of these. I have got a patch for it, that i
> would fold-back into
> the original patch that introduced locking fixes (attached too for testing):
Tested this patch on top of linux-pm.git/bleeding-edge
Now everything seems to be alright.

> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Fri, 8 Feb 2013 10:35:31 +0530
> Subject: [PATCH] cpufreq: Remove unnecessary locking
> 
> I have placed some locks intentionally around calls to driver->ops (init/exit),
> which look to be wrong as these calls can call routines that potentially sleep.
> 
> Lets remove these locks.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5d8a422..04aab05 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -795,10 +795,8 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
> 
>  	if (ret) {
>  		pr_debug("setting policy failed\n");
> -		spin_lock_irqsave(&cpufreq_driver_lock, flags);
>  		if (driver->exit)
>  			driver->exit(policy);
> -		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
>  	}
>  	return ret;
> 
> @@ -920,17 +918,14 @@ static int cpufreq_add_dev(struct device *dev,
> struct subsys_interface *sif)
>  	init_completion(&policy->kobj_unregister);
>  	INIT_WORK(&policy->update, handle_update);
> 
> -	spin_lock_irqsave(&cpufreq_driver_lock, flags);
>  	/* call driver. From then on the cpufreq must be able
>  	 * to accept all calls to ->verify and ->setpolicy for this CPU
>  	 */
>  	ret = driver->init(policy);
>  	if (ret) {
>  		pr_debug("initialization failed\n");
> -		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
>  		goto err_set_policy_cpu;
>  	}
> -	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 
>  	/* related cpus should atleast have policy->cpus */
>  	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
> @@ -1100,10 +1095,8 @@ static int __cpufreq_remove_dev(struct device
> *dev, struct subsys_interface *sif
>  		wait_for_completion(cmp);
>  		pr_debug("wait complete\n");
> 
> -		spin_lock_irqsave(&cpufreq_driver_lock, flags);
>  		if (driver->exit)
>  			driver->exit(data);
> -		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 
>  		free_cpumask_var(data->related_cpus);
>  		free_cpumask_var(data->cpus);

Tested-by: Artem Savkov <artem.savkov@gmail.com>

-- 
Kind regards,
Artem

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Artem Savkov]

On Thu, Feb 07, 2013 at 03:57:42PM +0530, Viresh Kumar wrote:
> Hi Rafael,
> 
> This is another unplanned patchset for all the platforms that i broke. :)
> 
> Okay, there are two important fixes (1 & 4) and two general cleanups (2 & 3). I
> hope most of the issues would be resolved by these and we would be able to push
> clean cpufreq core into 3.9.
> 
> I have pushed them in my for-rafael branch at:
> 
> http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/for-rafael
> 
> @Artem & Valdis: Please test them and reply with your Tested-by's (in case they
> work :) ).
> 
> Viresh Kumar (4):
>   cpufreq: governors: Fix WARN_ON() for multi-policy platforms
>   cpufreq: Remove unused HOTPLUG_CPU code
>   cpufreq: Create a macro for unlock_policy_rwsem{read,write}
>   cpufreq: Fix locking issues
> 
>  drivers/cpufreq/cpufreq.c          | 126 ++++++++++++++++++-------------------
>  drivers/cpufreq/cpufreq_governor.c |  32 ++++++----
>  2 files changed, 79 insertions(+), 79 deletions(-)

Tested out linux-pm.git/linux-next with this patches pulled. It seems
that my systemd-sleep issue is fixed, however there is a new 'sleeping
in invalid context' bug during boot:

[   12.736484] BUG: sleeping function called from invalid context at mm/slub.c:925
[   12.739727] in_atomic(): 1, irqs_disabled(): 1, pid: 1799, name: systemd-modules
[   12.742961] 2 locks held by systemd-modules/1799:
[   12.746153]  #0:  (subsys mutex#3){......}, at: [<c13f4056>] subsys_interface_register+0x36/0xb0
[   12.749499]  #1:  (cpufreq_driver_lock){......}, at: [<c14ba53b>] cpufreq_add_dev+0x22b/0x3d0
[   12.752865] Pid: 1799, comm: systemd-modules Not tainted 3.8.0-rc6+ #1
[   12.756175] Call Trace:
[   12.759538]  [<c1068150>] __might_sleep+0xe0/0x100
[   12.762156]  [<c112a481>] kmem_cache_alloc_trace+0xb1/0x150
[   12.765432]  [<f804e653>] ? acpi_cpufreq_cpu_init+0x73/0x5c0 [acpi_cpufreq]
[   12.768780]  [<f804e653>] acpi_cpufreq_cpu_init+0x73/0x5c0 [acpi_cpufreq]
[   12.772161]  [<c14ba53b>] ? cpufreq_add_dev+0x22b/0x3d0
[   12.775549]  [<c1695af7>] ? _raw_spin_lock_irqsave+0x77/0x90
[   12.778932]  [<c14ba53b>] ? cpufreq_add_dev+0x22b/0x3d0
[   12.782307]  [<c14ba548>] cpufreq_add_dev+0x238/0x3d0
[   12.785652]  [<c13f4095>] subsys_interface_register+0x75/0xb0
[   12.788989]  [<f804ec20>] ? do_drv_write+0x80/0x80 [acpi_cpufreq]
[   12.792325]  [<c14b986b>] cpufreq_register_driver+0x7b/0x150
[   12.795657]  [<f8075000>] ? 0xf8074fff
[   12.798971]  [<f80750ae>] acpi_cpufreq_init+0xae/0x1b3 [acpi_cpufreq]
[   12.802346]  [<c1001222>] do_one_initcall+0x112/0x160
[   12.805723]  [<c106145a>] ? __blocking_notifier_call_chain+0x4a/0x80
[   12.809123]  [<c10a048e>] load_module+0xd7e/0x1460
[   12.812515]  [<c1696d82>] ? error_code+0x5a/0x60
[   12.815891]  [<c10a0be8>] sys_init_module+0x78/0xb0
[   12.819249]  [<c169d67a>] sysenter_do_call+0x12/0x2d
[   12.822924] ------------[ cut here ]------------
[   12.826275] WARNING: at kernel/smp.c:327 smp_call_function_single+0x104/0x130()
[   12.829668] Hardware name: 0578A21
[   12.833020] Modules linked in: acpi_cpufreq(+) mperf thinkpad_acpi
[   12.836456] Pid: 1799, comm: systemd-modules Not tainted 3.8.0-rc6+ #1
[   12.839891] Call Trace:
[   12.843268]  [<c1036a82>] warn_slowpath_common+0x72/0xa0
[   12.846617]  [<c109b154>] ? smp_call_function_single+0x104/0x130
[   12.849921]  [<c109b154>] ? smp_call_function_single+0x104/0x130
[   12.853149]  [<f804eea0>] ? acpi_cpufreq_target+0x280/0x280 [acpi_cpufreq]
[   12.856361]  [<c1036ad2>] warn_slowpath_null+0x22/0x30
[   12.859518]  [<c109b154>] smp_call_function_single+0x104/0x130
[   12.862691]  [<c109b5b4>] smp_call_function_any+0x44/0xb0
[   12.865788]  [<f804eea0>] ? acpi_cpufreq_target+0x280/0x280 [acpi_cpufreq]
[   12.868893]  [<f804e13e>] get_cur_val+0x7e/0x100 [acpi_cpufreq]
[   12.871957]  [<f804e5bb>] get_cur_freq_on_cpu+0x4b/0x70 [acpi_cpufreq]
[   12.874987]  [<f804e9b8>] acpi_cpufreq_cpu_init+0x3d8/0x5c0 [acpi_cpufreq]
[   12.877998]  [<c14ba53b>] ? cpufreq_add_dev+0x22b/0x3d0
[   12.880982]  [<c14ba548>] cpufreq_add_dev+0x238/0x3d0
[   12.883931]  [<c13f4095>] subsys_interface_register+0x75/0xb0
[   12.886840]  [<f804ec20>] ? do_drv_write+0x80/0x80 [acpi_cpufreq]
[   12.889717]  [<c14b986b>] cpufreq_register_driver+0x7b/0x150
[   12.892550]  [<f8075000>] ? 0xf8074fff
[   12.895339]  [<f80750ae>] acpi_cpufreq_init+0xae/0x1b3 [acpi_cpufreq]
[   12.898169]  [<c1001222>] do_one_initcall+0x112/0x160
[   12.900988]  [<c106145a>] ? __blocking_notifier_call_chain+0x4a/0x80
[   12.903820]  [<c10a048e>] load_module+0xd7e/0x1460
[   12.906631]  [<c1696d82>] ? error_code+0x5a/0x60
[   12.909418]  [<c10a0be8>] sys_init_module+0x78/0xb0
[   12.912193]  [<c169d67a>] sysenter_do_call+0x12/0x2d
[   12.914956] ---[ end trace e15032846f0195a0 ]---


-- 
Kind regards,
Artem

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

On 8 February 2013 01:09, Artem Savkov <artem.savkov@gmail.com> wrote:
> Tested out linux-pm.git/linux-next with this patches pulled. It seems
> that my systemd-sleep issue is fixed, however there is a new 'sleeping
> in invalid context' bug during boot:
>
> [   12.736484] BUG: sleeping function called from invalid context at mm/slub.c:925
> [   12.739727] in_atomic(): 1, irqs_disabled(): 1, pid: 1799, name: systemd-modules
> [   12.742961] 2 locks held by systemd-modules/1799:
> [   12.746153]  #0:  (subsys mutex#3){......}, at: [<c13f4056>] subsys_interface_register+0x36/0xb0
> [   12.749499]  #1:  (cpufreq_driver_lock){......}, at: [<c14ba53b>] cpufreq_add_dev+0x22b/0x3d0
> [   12.752865] Pid: 1799, comm: systemd-modules Not tainted 3.8.0-rc6+ #1
> [   12.756175] Call Trace:
> [   12.759538]  [<c1068150>] __might_sleep+0xe0/0x100
> [   12.762156]  [<c112a481>] kmem_cache_alloc_trace+0xb1/0x150
> [   12.765432]  [<f804e653>] ? acpi_cpufreq_cpu_init+0x73/0x5c0 [acpi_cpufreq]
> [   12.768780]  [<f804e653>] acpi_cpufreq_cpu_init+0x73/0x5c0 [acpi_cpufreq]
> [   12.772161]  [<c14ba53b>] ? cpufreq_add_dev+0x22b/0x3d0
> [   12.775549]  [<c1695af7>] ? _raw_spin_lock_irqsave+0x77/0x90
> [   12.778932]  [<c14ba53b>] ? cpufreq_add_dev+0x22b/0x3d0
> [   12.782307]  [<c14ba548>] cpufreq_add_dev+0x238/0x3d0

Can you please try out this:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 819aa33..a77d0bc 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -919,17 +919,14 @@ static int cpufreq_add_dev(struct device *dev,
struct subsys_interface *sif)
        init_completion(&policy->kobj_unregister);
        INIT_WORK(&policy->update, handle_update);

-       spin_lock_irqsave(&cpufreq_driver_lock, flags);
        /* call driver. From then on the cpufreq must be able
         * to accept all calls to ->verify and ->setpolicy for this CPU
         */
        ret = driver->init(policy);
        if (ret) {
                pr_debug("initialization failed\n");
-               spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
                goto err_set_policy_cpu;
        }
-       spin_unlock_irqrestore(&cpufreq_driver_lock, flags);

        /* related cpus should atleast have policy->cpus */
        cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Thursday, February 07, 2013 03:57:42 PM Viresh Kumar wrote:
> Hi Rafael,
> 
> This is another unplanned patchset for all the platforms that i broke. :)
> 
> Okay, there are two important fixes (1 & 4) and two general cleanups (2 & 3). I
> hope most of the issues would be resolved by these and we would be able to push
> clean cpufreq core into 3.9.
> 
> I have pushed them in my for-rafael branch at:
> 
> http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/for-rafael
> 
> @Artem & Valdis: Please test them and reply with your Tested-by's (in case they
> work :) ).
> 
> Viresh Kumar (4):
>   cpufreq: governors: Fix WARN_ON() for multi-policy platforms
>   cpufreq: Remove unused HOTPLUG_CPU code
>   cpufreq: Create a macro for unlock_policy_rwsem{read,write}
>   cpufreq: Fix locking issues
> 
>  drivers/cpufreq/cpufreq.c          | 126 ++++++++++++++++++-------------------
>  drivers/cpufreq/cpufreq_governor.c |  32 ++++++----
>  2 files changed, 79 insertions(+), 79 deletions(-)

I should have done that before, sorry about it.

Can you please rework this series on top of linux-pm.git/pm-cpufreq and
try to avoid introducing new issues this time?

If this works, we'll rebase all of the other new material on top of it,
if possible.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Friday, February 08, 2013 12:33:14 AM Rafael J. Wysocki wrote:
> On Thursday, February 07, 2013 03:57:42 PM Viresh Kumar wrote:
> > Hi Rafael,
> > 
> > This is another unplanned patchset for all the platforms that i broke. :)
> > 
> > Okay, there are two important fixes (1 & 4) and two general cleanups (2 & 3). I
> > hope most of the issues would be resolved by these and we would be able to push
> > clean cpufreq core into 3.9.
> > 
> > I have pushed them in my for-rafael branch at:
> > 
> > http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/for-rafael
> > 
> > @Artem & Valdis: Please test them and reply with your Tested-by's (in case they
> > work :) ).
> > 
> > Viresh Kumar (4):
> >   cpufreq: governors: Fix WARN_ON() for multi-policy platforms
> >   cpufreq: Remove unused HOTPLUG_CPU code
> >   cpufreq: Create a macro for unlock_policy_rwsem{read,write}
> >   cpufreq: Fix locking issues
> > 
> >  drivers/cpufreq/cpufreq.c          | 126 ++++++++++++++++++-------------------
> >  drivers/cpufreq/cpufreq_governor.c |  32 ++++++----
> >  2 files changed, 79 insertions(+), 79 deletions(-)
> 
> I should have done that before, sorry about it.
> 
> Can you please rework this series on top of linux-pm.git/pm-cpufreq and
> try to avoid introducing new issues this time?
> 
> If this works, we'll rebase all of the other new material on top of it,
> if possible.

I've dropped the pm-cpufreq-next branch from linux-next now, BTW.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

On 8 February 2013 05:03, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> I should have done that before, sorry about it.
>
> Can you please rework this series on top of linux-pm.git/pm-cpufreq and
> try to avoid introducing new issues this time?

Even i want to do that, but when i fetch your repo i don't see all applied
patches in this branch.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Friday, February 08, 2013 08:20:55 AM Viresh Kumar wrote:
> On 8 February 2013 05:03, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > I should have done that before, sorry about it.
> >
> > Can you please rework this series on top of linux-pm.git/pm-cpufreq and
> > try to avoid introducing new issues this time?
> 
> Even i want to do that, but when i fetch your repo i don't see all applied
> patches in this branch.

The top-most commit in that branch is

commit 73bf0fc2b03d1f4fdada0ec430dc20bfb089cfd5
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Tue Feb 5 22:21:14 2013 +0100

    cpufreq: Don't remove sysfs link for policy->cpu

because that's when the locking problems were first reported and I stopped
putting new commits into that branch.  And since the locking problems were
introduced by b8eed8a "cpufreq: Simplify __cpufreq_remove_dev()" I want them
to be fixed on top of pm-cpufreq rather than on top of more new stuff that
very well may introduce *more* problems.

So as I said, please rework the fixes on top of linux-pm.git/pm-cpufreq.

Moreover, I'd very much prefer it if you fixed the problems introduced by
b8eed8a "cpufreq: Simplify __cpufreq_remove_dev()" separately and *then* any
other locking problems you're seeing in the code, although people are not
reporting them.

You seem to have a clear picture of how the code should work now, so that
won't be a big trouble I guess.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

On 8 February 2013 18:02, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> So as I said, please rework the fixes on top of linux-pm.git/pm-cpufreq.

I already did. Please check for-rafael branch

> Moreover, I'd very much prefer it if you fixed the problems introduced by
> b8eed8a "cpufreq: Simplify __cpufreq_remove_dev()" separately and *then* any
> other locking problems you're seeing in the code, although people are not
> reporting them.
>
> You seem to have a clear picture of how the code should work now, so that
> won't be a big trouble I guess.

For me all problems are fixed in the current code. :)

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Friday, February 08, 2013 08:06:52 PM Viresh Kumar wrote:
> On 8 February 2013 18:02, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > So as I said, please rework the fixes on top of linux-pm.git/pm-cpufreq.
> 
> I already did. Please check for-rafael branch

Cool.  This is the one I'm supposed to apply, then?

> > Moreover, I'd very much prefer it if you fixed the problems introduced by
> > b8eed8a "cpufreq: Simplify __cpufreq_remove_dev()" separately and *then* any
> > other locking problems you're seeing in the code, although people are not
> > reporting them.
> >
> > You seem to have a clear picture of how the code should work now, so that
> > won't be a big trouble I guess.
> 
> For me all problems are fixed in the current code. :)

Sounds great. :-)

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Friday, February 08, 2013 09:02:37 PM Rafael J. Wysocki wrote:
> On Friday, February 08, 2013 08:06:52 PM Viresh Kumar wrote:
> > On 8 February 2013 18:02, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > So as I said, please rework the fixes on top of linux-pm.git/pm-cpufreq.
> > 
> > I already did. Please check for-rafael branch
> 
> Cool.  This is the one I'm supposed to apply, then?

OK, applied to bleeding-edge.  Hopefully it will be build-tested over the
weekend and I can move it to linux-next.

I dropped the rwlock/RCU patches from Nathan, though, because I had some
doubts about the correctness of the RCU one and the rwlock one alone would
conflict with your further changes.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Dirk Brandewie]

On 02/08/2013 03:56 PM, Rafael J. Wysocki wrote:
> On Friday, February 08, 2013 09:02:37 PM Rafael J. Wysocki wrote:
>> On Friday, February 08, 2013 08:06:52 PM Viresh Kumar wrote:
>>> On 8 February 2013 18:02, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>>> So as I said, please rework the fixes on top of linux-pm.git/pm-cpufreq.
>>>
>>> I already did. Please check for-rafael branch
>>
>> Cool.  This is the one I'm supposed to apply, then?
>
> OK, applied to bleeding-edge.  Hopefully it will be build-tested over the
> weekend and I can move it to linux-next.
>
> I dropped the rwlock/RCU patches from Nathan, though, because I had some
> doubts about the correctness of the RCU one and the rwlock one alone would
> conflict with your further changes.

One piece of fallout from dropping Nathan patches I had rebased mine on top of 
them.

This fixes the breakage do you want me to spin my patches or send this separately?:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 0ebdf8c..a008b8e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1024,7 +1024,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct
                 __cpufreq_governor(data, CPUFREQ_GOV_STOP);

  #ifdef CONFIG_HOTPLUG_CPU
-       if (!driver->setpolicy)
+       if (!cpufreq_driver->setpolicy)
                 strncpy(per_cpu(cpufreq_cpu_governor, cpu),
                         data->governor->name, CPUFREQ_NAME_LEN);
  #endif
@@ -1771,7 +1771,7 @@ int cpufreq_update_policy(unsigned int cpu)
                         pr_debug("Driver did not initialize current freq");
                         data->cur = policy.cur;
                 } else {
-                       if (data->cur != policy.cur && driver->target)
+                       if (data->cur != policy.cur && cpufreq_driver->target)
                                 cpufreq_out_of_sync(cpu, data->cur,
                                                                 policy.cur);
                 }

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Friday, February 08, 2013 04:08:49 PM Dirk Brandewie wrote:
> On 02/08/2013 03:56 PM, Rafael J. Wysocki wrote:
> > On Friday, February 08, 2013 09:02:37 PM Rafael J. Wysocki wrote:
> >> On Friday, February 08, 2013 08:06:52 PM Viresh Kumar wrote:
> >>> On 8 February 2013 18:02, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>>> So as I said, please rework the fixes on top of linux-pm.git/pm-cpufreq.
> >>>
> >>> I already did. Please check for-rafael branch
> >>
> >> Cool.  This is the one I'm supposed to apply, then?
> >
> > OK, applied to bleeding-edge.  Hopefully it will be build-tested over the
> > weekend and I can move it to linux-next.
> >
> > I dropped the rwlock/RCU patches from Nathan, though, because I had some
> > doubts about the correctness of the RCU one and the rwlock one alone would
> > conflict with your further changes.
> 
> One piece of fallout from dropping Nathan patches I had rebased mine on top of 
> them.
> 
> This fixes the breakage do you want me to spin my patches or send this separately?:

No need to, I'll try to fix that in my tree.

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 0ebdf8c..a008b8e 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1024,7 +1024,7 @@ static int __cpufreq_remove_dev(struct device *dev, struct
>                  __cpufreq_governor(data, CPUFREQ_GOV_STOP);
> 
>   #ifdef CONFIG_HOTPLUG_CPU
> -       if (!driver->setpolicy)
> +       if (!cpufreq_driver->setpolicy)
>                  strncpy(per_cpu(cpufreq_cpu_governor, cpu),
>                          data->governor->name, CPUFREQ_NAME_LEN);
>   #endif
> @@ -1771,7 +1771,7 @@ int cpufreq_update_policy(unsigned int cpu)
>                          pr_debug("Driver did not initialize current freq");
>                          data->cur = policy.cur;
>                  } else {
> -                       if (data->cur != policy.cur && driver->target)
> +                       if (data->cur != policy.cur && cpufreq_driver->target)
>                                  cpufreq_out_of_sync(cpu, data->cur,
>                                                                  policy.cur);
>                  }
> 

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

On 9 February 2013 05:38, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> On 02/08/2013 03:56 PM, Rafael J. Wysocki wrote:
>>
>> On Friday, February 08, 2013 09:02:37 PM Rafael J. Wysocki wrote:
>>>
>>> On Friday, February 08, 2013 08:06:52 PM Viresh Kumar wrote:
>>>>
>>>> On 8 February 2013 18:02, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>>>>
>>>>> So as I said, please rework the fixes on top of
>>>>> linux-pm.git/pm-cpufreq.
>>>>
>>>>
>>>> I already did. Please check for-rafael branch
>>>
>>>
>>> Cool.  This is the one I'm supposed to apply, then?
>>
>>
>> OK, applied to bleeding-edge.  Hopefully it will be build-tested over the
>> weekend and I can move it to linux-next.
>>
>> I dropped the rwlock/RCU patches from Nathan, though, because I had some
>> doubts about the correctness of the RCU one and the rwlock one alone would
>> conflict with your further changes.

As soon as i read Rafael's mail, i realized Dirk's patch might be broken
and immediately i saw your mail :)

@Rafael: Sorry for not reviewing Nathan's patch well. I didn't knew much about
RCU then. I am going through its lwn articles now ;)

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Rafael J. Wysocki]

On Saturday, February 09, 2013 07:40:26 AM Viresh Kumar wrote:
> On 9 February 2013 05:38, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> > On 02/08/2013 03:56 PM, Rafael J. Wysocki wrote:
> >>
> >> On Friday, February 08, 2013 09:02:37 PM Rafael J. Wysocki wrote:
> >>>
> >>> On Friday, February 08, 2013 08:06:52 PM Viresh Kumar wrote:
> >>>>
> >>>> On 8 February 2013 18:02, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>>>>
> >>>>> So as I said, please rework the fixes on top of
> >>>>> linux-pm.git/pm-cpufreq.
> >>>>
> >>>>
> >>>> I already did. Please check for-rafael branch
> >>>
> >>>
> >>> Cool.  This is the one I'm supposed to apply, then?
> >>
> >>
> >> OK, applied to bleeding-edge.  Hopefully it will be build-tested over the
> >> weekend and I can move it to linux-next.
> >>
> >> I dropped the rwlock/RCU patches from Nathan, though, because I had some
> >> doubts about the correctness of the RCU one and the rwlock one alone would
> >> conflict with your further changes.
> 
> As soon as i read Rafael's mail, i realized Dirk's patch might be broken
> and immediately i saw your mail :)
> 
> @Rafael: Sorry for not reviewing Nathan's patch well. I didn't knew much about
> RCU then. I am going through its lwn articles now ;)

No biggie, I overlooked that myself first time.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] CPUFreq Fixes for 3.9

[Viresh Kumar]

On 8 February 2013 05:03, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> I should have done that before, sorry about it.

np

> Can you please rework this series on top of linux-pm.git/pm-cpufreq and
> try to avoid introducing new issues this time?

Sorry for this. I didn't got any such issues on my system and i tried to think
as widely as possible. But still just a human with some mistakes :)

> If this works, we'll rebase all of the other new material on top of it,
> if possible.

To make your life a bit easy, i have got all cpufreq patches, that you & me
have got for 3.9, rebased over pm-cpufreq and these are:

f3843e0 cpufreq: exynos: simplify .init() for setting policy->cpus
7ea6658 cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs
6002fd0 cpufreq/x86: Add P-state driver for sandy bridge.
bcfe254 cpufreq_stats: do not remove sysfs files if frequency table is
not present
6c82b96 cpufreq: Do not track governor name for scaling drivers with
internal governors.
2a6df07 cpufreq: Only call cpufreq_out_of_sync() for driver that
implement cpufreq_driver.target()
0893112 cpufreq: Retrieve current frequency from scaling drivers with
internal governors
e034e73 cpufreq: Fix locking issues
003da79 cpufreq: Create a macro for unlock_policy_rwsem{read,write}
0092c75 cpufreq: Remove unused HOTPLUG_CPU code
34d5833 cpufreq: governors: Fix WARN_ON() for multi-policy platforms
e1ee7c8 cpufreq: Convert the cpufreq_driver_lock to use RCU
e076b60 cpufreq: Convert the cpufreq_driver_lock to a rwlock
6d919f9 cpufreq: ondemand: Replace down_differential tuner with adj_up_threshold
80dd878 cpufreq / stats: Get rid of CPUFREQ_STATDEVICE_ATTR
a7e183d cpufreq: Don't check cpu_online(policy->cpu)
9db0116 cpufreq: add imx6q-cpufreq driver


I have pushed them in for-rafael branch in my repo. Look carefully at
the first two patches,
they were not present in your latest repo.

This was the exynos patch i was talking about:

f3843e0 cpufreq: exynos: simplify .init() for setting policy->cpus

I don't know if you dropped this one or what ?

7ea6658 cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs