[PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Viresh Kumar]

__cpufreq_remove_dev() is called on multiple occasions: cpufreq_driver
unregister and cpu removals.

Current implementation of this routine is overly complex without much need. If
the cpu to be removed is the policy->cpu, we remove the policy first and add all
other cpus again from policy->cpus and then finally call __cpufreq_remove_dev()
again to remove the cpu to be deleted. Haahhhh..

There exist a simple solution to removal of a cpu:
- Simply use the old policy structure
- update its fields like: policy->cpu, etc.
- notify any users of cpufreq, which depend on changing policy->cpu

Hence this patch, which tries to implement the above theory. It is tested well
by myself on ARM big.LITTLE TC2 SoC, which has 5 cores (2 A15 and 3 A7). Both
A15's share same struct policy and all A7's share same policy structure.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Hi Guys,

I am just an beginner in cpufreq stuff, please ignore any foolish mistakes. :)

Yesterday, i reviewed a cpufreq driver that had some ugly code in init()
routine:
http://www.spinics.net/lists/arm-kernel/msg215348.html

It wasn't ugly due to the author of the patch, but the way
__cpufreq_remove_dev() is implemented. And then i thought to simplify it.

[Probably need to simplify cpufreq_add_dev() too, but that can be done as next
step.]

I have rebased this patch over some other cpufreq core fixes i had posted
earlier:

https://lkml.org/lkml/2012/12/16/5

ARM mail servers are broken and hence this patch can't be applied as is. :(
I have pushed this and the dependency patch here:

http://git.linaro.org/gitweb?p=arm/big.LITTLE/mp.git;a=shortlog;h=refs/heads/cpufreq-fixes-v2

@Shawn: I believe your driver don't require that ugly code anymore (Though i
know there is a situation for that to happen, if we have two cpus, you remove
second one and then add it back. With this cpufreq_add_dev() would call init()
first and then try to match if there are any managed_policies present. But the
issue you pointed out about unregistering the driver would be solved by this
patch.)

 drivers/cpufreq/cpufreq.c       | 158 +++++++++++++++++-----------------------
 drivers/cpufreq/cpufreq_stats.c |  21 +++++-
 drivers/cpufreq/freq_table.c    |  10 +++
 include/linux/cpufreq.h         |   5 ++
 4 files changed, 101 insertions(+), 93 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 271d3be..8df41ad 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1036,6 +1036,22 @@ module_out:
 	return ret;
 }
 
+static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
+{
+	unsigned int old_cpu = policy->cpu;
+	int j;
+
+	policy->cpu = cpu;
+
+	for_each_cpu(j, policy->cpus) {
+		if (!cpu_online(j))
+			continue;
+		per_cpu(cpufreq_policy_cpu, j) = cpu;
+	}
+
+	cpufreq_frequency_table_update_policy_cpu(old_cpu, cpu);
+	cpufreq_stats_update_policy_cpu(old_cpu, cpu);
+}
 
 /**
  * __cpufreq_remove_dev - remove a CPU device
@@ -1046,132 +1062,92 @@ module_out:
  */
 static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 {
-	unsigned int cpu = dev->id;
+	unsigned int cpu = dev->id, ret, cpus;
 	unsigned long flags;
 	struct cpufreq_policy *data;
 	struct kobject *kobj;
 	struct completion *cmp;
-#ifdef CONFIG_SMP
 	struct device *cpu_dev;
-	unsigned int j;
-#endif
 
-	pr_debug("unregistering CPU %u\n", cpu);
+	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
 
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	data = per_cpu(cpufreq_cpu_data, cpu);
 
 	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;
 	}
-	per_cpu(cpufreq_cpu_data, cpu) = NULL;
 
-#ifdef CONFIG_SMP
-	/* if this isn't the CPU which is the parent of the kobj, we
-	 * only need to unlink, put and exit
-	 */
-	if (unlikely(cpu != data->cpu)) {
-		pr_debug("removing link\n");
+	if (cpufreq_driver->target)
 		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
-		cpumask_clear_cpu(cpu, data->cpus);
-		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
-		__cpufreq_governor(data, CPUFREQ_GOV_START);
-		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
-
-		kobj = &dev->kobj;
-		cpufreq_cpu_put(data);
-		unlock_policy_rwsem_write(cpu);
-		sysfs_remove_link(kobj, "cpufreq");
-		return 0;
-	}
-#endif
-
-#ifdef CONFIG_SMP
 
 #ifdef CONFIG_HOTPLUG_CPU
 	strncpy(per_cpu(cpufreq_cpu_governor, cpu), data->governor->name,
 			CPUFREQ_NAME_LEN);
 #endif
 
-	/* if we have other CPUs still registered, we need to unlink them,
-	 * or else wait_for_completion below will lock up. Clean the
-	 * per_cpu(cpufreq_cpu_data) while holding the lock, and remove
-	 * the sysfs links afterwards.
-	 */
-	if (unlikely(cpumask_weight(data->cpus) > 1)) {
-		for_each_cpu(j, data->cpus) {
-			if (j == cpu)
-				continue;
-			per_cpu(cpufreq_cpu_data, j) = NULL;
-		}
-	}
-
-	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+	per_cpu(cpufreq_cpu_data, cpu) = NULL;
+	cpus = cpumask_weight(data->cpus);
+	cpumask_clear_cpu(cpu, data->cpus);
 
-	if (unlikely(cpumask_weight(data->cpus) > 1)) {
-		for_each_cpu(j, data->cpus) {
-			if (j == cpu)
-				continue;
-			pr_debug("removing link for cpu %u\n", j);
-#ifdef CONFIG_HOTPLUG_CPU
-			strncpy(per_cpu(cpufreq_cpu_governor, j),
-				data->governor->name, CPUFREQ_NAME_LEN);
-#endif
-			cpu_dev = get_cpu_device(j);
-			kobj = &cpu_dev->kobj;
+	if (unlikely((cpu == data->cpu) && (cpus > 1))) {
+		/* first sibling now owns the new sysfs dir */
+		cpu_dev = get_cpu_device(cpumask_first(data->cpus));
+		sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
+		ret = kobject_move(&data->kobj, &cpu_dev->kobj);
+		if (ret) {
+			pr_err("%s: Failed to move kobj: %d", __func__, ret);
+			cpumask_set_cpu(cpu, data->cpus);
+			ret = sysfs_create_link(&cpu_dev->kobj, &data->kobj,
+					"cpufreq");
+			spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 			unlock_policy_rwsem_write(cpu);
-			sysfs_remove_link(kobj, "cpufreq");
-			lock_policy_rwsem_write(cpu);
-			cpufreq_cpu_put(data);
+			return -EINVAL;
 		}
+
+		update_policy_cpu(data, cpu_dev->id);
+		pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
+				__func__, cpu_dev->id, cpu);
 	}
-#else
-	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
-#endif
 
-	if (cpufreq_driver->target)
-		__cpufreq_governor(data, CPUFREQ_GOV_STOP);
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
-	kobj = &data->kobj;
-	cmp = &data->kobj_unregister;
+	pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
+	cpufreq_cpu_put(data);
 	unlock_policy_rwsem_write(cpu);
-	kobject_put(kobj);
+	sysfs_remove_link(&dev->kobj, "cpufreq");
 
-	/* we need to make sure that the underlying kobj is actually
-	 * not referenced anymore by anybody before we proceed with
-	 * unloading.
-	 */
-	pr_debug("waiting for dropping of refcount\n");
-	wait_for_completion(cmp);
-	pr_debug("wait complete\n");
-
-	lock_policy_rwsem_write(cpu);
-	if (cpufreq_driver->exit)
-		cpufreq_driver->exit(data);
-	unlock_policy_rwsem_write(cpu);
+	/* If cpu is last user of policy, free policy */
+	if (cpus == 1) {
+		lock_policy_rwsem_write(cpu);
+		kobj = &data->kobj;
+		cmp = &data->kobj_unregister;
+		unlock_policy_rwsem_write(cpu);
+		kobject_put(kobj);
 
-#ifdef CONFIG_HOTPLUG_CPU
-	/* when the CPU which is the parent of the kobj is hotplugged
-	 * offline, check for siblings, and create cpufreq sysfs interface
-	 * and symlinks
-	 */
-	if (unlikely(cpumask_weight(data->cpus) > 1)) {
-		/* first sibling now owns the new sysfs dir */
-		cpumask_clear_cpu(cpu, data->cpus);
-		cpufreq_add_dev(get_cpu_device(cpumask_first(data->cpus)), NULL);
+		/* we need to make sure that the underlying kobj is actually
+		 * not referenced anymore by anybody before we proceed with
+		 * unloading.
+		 */
+		pr_debug("waiting for dropping of refcount\n");
+		wait_for_completion(cmp);
+		pr_debug("wait complete\n");
 
-		/* finally remove our own symlink */
 		lock_policy_rwsem_write(cpu);
-		__cpufreq_remove_dev(dev, sif);
-	}
-#endif
+		if (cpufreq_driver->exit)
+			cpufreq_driver->exit(data);
+		unlock_policy_rwsem_write(cpu);
 
-	free_cpumask_var(data->related_cpus);
-	free_cpumask_var(data->cpus);
-	kfree(data);
+		free_cpumask_var(data->related_cpus);
+		free_cpumask_var(data->cpus);
+		kfree(data);
+	} else if (cpufreq_driver->target) {
+		__cpufreq_governor(data, CPUFREQ_GOV_START);
+		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
+	}
 
 	return 0;
 }
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index e40e508..0afface 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -170,11 +170,13 @@ static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
 static void cpufreq_stats_free_table(unsigned int cpu)
 {
 	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, cpu);
+
 	if (stat) {
+		pr_debug("%s: Free stat table\n", __func__);
 		kfree(stat->time_in_state);
 		kfree(stat);
+		per_cpu(cpufreq_stats_table, cpu) = NULL;
 	}
-	per_cpu(cpufreq_stats_table, cpu) = NULL;
 }
 
 /* must be called early in the CPU removal sequence (before
@@ -183,8 +185,10 @@ static void cpufreq_stats_free_table(unsigned int cpu)
 static void cpufreq_stats_free_sysfs(unsigned int cpu)
 {
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-	if (policy && policy->cpu == cpu)
+	if (policy && (cpumask_weight(policy->cpus) == 1)) {
+		pr_debug("%s: Free sysfs stat\n", __func__);
 		sysfs_remove_group(&policy->kobj, &stats_attr_group);
+	}
 	if (policy)
 		cpufreq_cpu_put(policy);
 }
@@ -262,6 +266,19 @@ error_get_fail:
 	return ret;
 }
 
+void cpufreq_stats_update_policy_cpu(unsigned int old_cpu,
+		unsigned int new_cpu)
+{
+	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, old_cpu);
+
+	pr_debug("Updating stats_table for new_cpu %u from old_cpu %u\n",
+			new_cpu, old_cpu);
+	per_cpu(cpufreq_stats_table, new_cpu) = per_cpu(cpufreq_stats_table,
+			old_cpu);
+	per_cpu(cpufreq_stats_table, old_cpu) = NULL;
+	stat->cpu = new_cpu;
+}
+
 static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
 		unsigned long val, void *data)
 {
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 49cda25..a4b384f 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -227,6 +227,16 @@ void cpufreq_frequency_table_put_attr(unsigned int cpu)
 }
 EXPORT_SYMBOL_GPL(cpufreq_frequency_table_put_attr);
 
+void cpufreq_frequency_table_update_policy_cpu(unsigned int old_cpu,
+		unsigned int new_cpu)
+{
+	pr_debug("Updating show_table for new_cpu %u from old_cpu %u\n",
+			new_cpu, old_cpu);
+	per_cpu(cpufreq_show_table, new_cpu) = per_cpu(cpufreq_show_table,
+			old_cpu);
+	per_cpu(cpufreq_show_table, old_cpu) = NULL;
+}
+
 struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
 {
 	return per_cpu(cpufreq_show_table, cpu);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index a55b88e..a42aa05 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -405,6 +405,11 @@ extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs;
 
 void cpufreq_frequency_table_get_attr(struct cpufreq_frequency_table *table,
 				      unsigned int cpu);
+void cpufreq_frequency_table_update_policy_cpu(unsigned int old_cpu,
+		unsigned int new_cpu);
 
 void cpufreq_frequency_table_put_attr(unsigned int cpu);
+
+void cpufreq_stats_update_policy_cpu(unsigned int old_cpu,
+		unsigned int new_cpu);
 #endif /* _LINUX_CPUFREQ_H */
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Viresh Kumar]

On 9 January 2013 16:50, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> [Probably need to simplify cpufreq_add_dev() too, but that can be done as next
> step.]

I have tried that too, it is also pushed at:

https://lkml.org/lkml/2012/12/16/5

[Untested for now, will be doing it tomorrow]

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 9 Jan 2013 21:02:50 +0530
Subject: [PATCH] cpufreq: Simplify cpufreq_add_dev()

Currently cpufreq_add_dev() firsts allocated policy, calls ->init() and then
checks if this cpu should be already managed or not. And if it already managed,
free its policy.

We can save all this if we somehow know if this cpu is managed or not in
advance. policy->related_cpus contains list of all valid sibling cpus of
policy->cpu. We can check this to know if current cpu is already managed.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b8709899..e3f7c7b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -701,92 +701,6 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };

-/*
- * Returns:
- *   Negative: Failure
- *   0:        Success
- *   Positive: When we have a managed CPU and the sysfs got symlinked
- */
-static int cpufreq_add_dev_policy(unsigned int cpu,
-				  struct cpufreq_policy *policy,
-				  struct device *dev)
-{
-	int ret = 0;
-#ifdef CONFIG_SMP
-	unsigned long flags;
-	unsigned int j;
-#ifdef CONFIG_HOTPLUG_CPU
-	struct cpufreq_governor *gov;
-
-	gov = __find_governor(per_cpu(cpufreq_cpu_governor, cpu));
-	if (gov) {
-		policy->governor = gov;
-		pr_debug("Restoring governor %s for cpu %d\n",
-		       policy->governor->name, cpu);
-	}
-#endif
-
-	for_each_cpu(j, policy->cpus) {
-		struct cpufreq_policy *managed_policy;
-
-		if (cpu == j)
-			continue;
-
-		/* Check for existing affected CPUs.
-		 * They may not be aware of it due to CPU Hotplug.
-		 * cpufreq_cpu_put is called when the device is removed
-		 * in __cpufreq_remove_dev()
-		 */
-		managed_policy = cpufreq_cpu_get(j);
-		if (unlikely(managed_policy)) {
-
-			/* Set proper policy_cpu */
-			unlock_policy_rwsem_write(cpu);
-			per_cpu(cpufreq_policy_cpu, cpu) = managed_policy->cpu;
-
-			if (lock_policy_rwsem_write(cpu) < 0) {
-				/* Should not go through policy unlock path */
-				if (cpufreq_driver->exit)
-					cpufreq_driver->exit(policy);
-				cpufreq_cpu_put(managed_policy);
-				return -EBUSY;
-			}
-
-			__cpufreq_governor(managed_policy, CPUFREQ_GOV_STOP);
-
-			spin_lock_irqsave(&cpufreq_driver_lock, flags);
-			cpumask_copy(managed_policy->cpus, policy->cpus);
-			per_cpu(cpufreq_cpu_data, cpu) = managed_policy;
-			spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
-			__cpufreq_governor(managed_policy, CPUFREQ_GOV_START);
-			__cpufreq_governor(managed_policy, CPUFREQ_GOV_LIMITS);
-
-			pr_debug("CPU already managed, adding link\n");
-			ret = sysfs_create_link(&dev->kobj,
-						&managed_policy->kobj,
-						"cpufreq");
-			if (ret)
-				cpufreq_cpu_put(managed_policy);
-			/*
-			 * Success. We only needed to be added to the mask.
-			 * Call driver->exit() because only the cpu parent of
-			 * the kobj needed to call init().
-			 */
-			if (cpufreq_driver->exit)
-				cpufreq_driver->exit(policy);
-
-			if (!ret)
-				return 1;
-			else
-				return ret;
-		}
-	}
-#endif
-	return ret;
-}
-
-
 /* symlink affected CPUs */
 static int cpufreq_add_dev_symlink(unsigned int cpu,
 				   struct cpufreq_policy *policy)
@@ -891,6 +805,41 @@ err_out_kobj_put:
 	return ret;
 }

+static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
+				  struct device *dev)
+{
+	struct cpufreq_policy *policy;
+	int ret = 0;
+	unsigned long flags;
+
+	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);
+
+	spin_lock_irqsave(&cpufreq_driver_lock, flags);
+	cpumask_set_cpu(cpu, policy->cpus);
+	per_cpu(cpufreq_cpu_data, cpu) = policy;
+	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+	__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);
+		return ret;
+	}
+
+	return 0;
+}
+

 /**
  * cpufreq_add_dev - add a CPU device
@@ -903,12 +852,12 @@ err_out_kobj_put:
  */
 static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
-	unsigned int cpu = dev->id;
-	int ret = 0, found = 0;
+	unsigned int j, cpu = dev->id;
+	int ret = -ENOMEM, found = 0;
 	struct cpufreq_policy *policy;
 	unsigned long flags;
-	unsigned int j;
 #ifdef CONFIG_HOTPLUG_CPU
+	struct cpufreq_governor *gov;
 	int sibling;
 #endif

@@ -925,6 +874,13 @@ static int cpufreq_add_dev(struct device *dev,
struct subsys_interface *sif)
 		cpufreq_cpu_put(policy);
 		return 0;
 	}
+
+	/* Check if this cpu was hot-unplugged earlier and has siblings */
+	for_each_online_cpu(sibling) {
+		struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
+		if (cp && cpumask_test_cpu(cpu, cp->related_cpus))
+			return cpufreq_add_policy_cpu(cpu, sibling, dev);
+	}
 #endif

 	if (!try_module_get(cpufreq_driver->owner)) {
@@ -932,7 +888,6 @@ static int cpufreq_add_dev(struct device *dev,
struct subsys_interface *sif)
 		goto module_out;
 	}

-	ret = -ENOMEM;
 	policy = kzalloc(sizeof(struct cpufreq_policy), GFP_KERNEL);
 	if (!policy)
 		goto nomem_out;
@@ -990,14 +945,14 @@ static int cpufreq_add_dev(struct device *dev,
struct subsys_interface *sif)
 	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
 				     CPUFREQ_START, policy);

-	ret = cpufreq_add_dev_policy(cpu, policy, dev);
-	if (ret) {
-		if (ret > 0)
-			/* This is a managed cpu, symlink created,
-			   exit with 0 */
-			ret = 0;
-		goto err_unlock_policy;
+#ifdef CONFIG_HOTPLUG_CPU
+	gov = __find_governor(per_cpu(cpufreq_cpu_governor, cpu));
+	if (gov) {
+		policy->governor = gov;
+		pr_debug("Restoring governor %s for cpu %d\n",
+		       policy->governor->name, cpu);
 	}
+#endif

 	ret = cpufreq_add_dev_interface(cpu, policy, dev);
 	if (ret)
@@ -1011,7 +966,6 @@ static int cpufreq_add_dev(struct device *dev,
struct subsys_interface *sif)

 	return 0;

-
 err_out_unregister:
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	for_each_cpu(j, policy->cpus)

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Viresh Kumar]

On 9 January 2013 21:09, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 9 January 2013 16:50, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> [Probably need to simplify cpufreq_add_dev() too, but that can be done as next
>> step.]
>
> I have tried that too, it is also pushed at:
>
> https://lkml.org/lkml/2012/12/16/5
>
> [Untested for now, will be doing it tomorrow]
>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Wed, 9 Jan 2013 21:02:50 +0530
> Subject: [PATCH] cpufreq: Simplify cpufreq_add_dev()
>
> Currently cpufreq_add_dev() firsts allocated policy, calls ->init() and then
> checks if this cpu should be already managed or not. And if it already managed,
> free its policy.
>
> We can save all this if we somehow know if this cpu is managed or not in
> advance. policy->related_cpus contains list of all valid sibling cpus of
> policy->cpu. We can check this to know if current cpu is already managed.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Tested-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Viresh Kumar]

On 9 January 2013 21:09, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> I have tried that too, it is also pushed at:
>
> https://lkml.org/lkml/2012/12/16/5

Bad link :(

http://git.linaro.org/gitweb?p=arm/big.LITTLE/mp.git;a=shortlog;h=refs/heads/cpufreq-fixes-v2

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Shawn Guo]

On Wed, Jan 09, 2013 at 04:50:44PM +0530, Viresh Kumar wrote:
> @Shawn: I believe your driver don't require that ugly code anymore (Though i
> know there is a situation for that to happen, if we have two cpus, you remove
> second one and then add it back. With this cpufreq_add_dev() would call init()
> first and then try to match if there are any managed_policies present. But the
> issue you pointed out about unregistering the driver would be solved by this
> patch.)

Yes, just played it and it works for me.  However, I would have to keep
that little ugly code in my patch to save the dependency on your patch.
Will send a follow-up to clean that up once your patch hits mainline.

Shawn

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Viresh Kumar]

On 10 January 2013 13:24, Shawn Guo <shawn.guo@linaro.org> wrote:
> Yes, just played it and it works for me.  However, I would have to keep
> that little ugly code in my patch to save the dependency on your patch.
> Will send a follow-up to clean that up once your patch hits mainline.

Good. Hopefully, patches from both of us would hit 3.9 and Rafael can
apply yours after mine, with this check removed :)

--
viresh

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Viresh Kumar]

On 10 January 2013 13:24, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Jan 09, 2013 at 04:50:44PM +0530, Viresh Kumar wrote:
>> @Shawn: I believe your driver don't require that ugly code anymore (Though i
>> know there is a situation for that to happen, if we have two cpus, you remove
>> second one and then add it back. With this cpufreq_add_dev() would call init()
>> first and then try to match if there are any managed_policies present. But the
>> issue you pointed out about unregistering the driver would be solved by this
>> patch.)
>
> Yes, just played it and it works for me.  However, I would have to keep
> that little ugly code in my patch to save the dependency on your patch.
> Will send a follow-up to clean that up once your patch hits mainline.

Another thing, can i have a tested-by from you for both my patches ? remove and
add dev?

That would help it go quicker.

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Shawn Guo]

On 10 January 2013 16:05, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Another thing, can i have a tested-by from you for both my patches ? remove and
> add dev?
>
For both:

Tested-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Viresh Kumar]

On 9 January 2013 16:50, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> +static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
> +{

> +       cpufreq_frequency_table_update_policy_cpu(old_cpu, cpu);
> +       cpufreq_stats_update_policy_cpu(old_cpu, cpu);

This looked a bit ugly to me, as there may be other users too who want
to get this notification and so have below patch for this: pushed in my repo.

Tested on TC2. I have tested my patches well now and the activity is over from
my end now, unless somebody finds something objectionable :)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e3f7c7b..8a0b65e0 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -990,9 +990,9 @@ module_out:

 static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
-	unsigned int old_cpu = policy->cpu;
 	int j;

+	policy->last_cpu = policy->cpu;
 	policy->cpu = cpu;

 	for_each_cpu(j, policy->cpus) {
@@ -1001,8 +1001,9 @@ static void update_policy_cpu(struct
cpufreq_policy *policy, unsigned int cpu)
 		per_cpu(cpufreq_policy_cpu, j) = cpu;
 	}

-	cpufreq_frequency_table_update_policy_cpu(old_cpu, cpu);
-	cpufreq_stats_update_policy_cpu(old_cpu, cpu);
+	cpufreq_frequency_table_update_policy_cpu(policy);
+	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+			CPUFREQ_UPDATE_POLICY_CPU, policy);
 }

 /**
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index b0df77a..2665f0a 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -267,17 +267,17 @@ error_get_fail:
 	return ret;
 }

-void cpufreq_stats_update_policy_cpu(unsigned int old_cpu,
-		unsigned int new_cpu)
+static void cpufreq_stats_update_policy_cpu(struct cpufreq_policy *policy)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, old_cpu);
-
-	pr_debug("Updating stats_table for new_cpu %u from old_cpu %u\n",
-			new_cpu, old_cpu);
-	per_cpu(cpufreq_stats_table, new_cpu) = per_cpu(cpufreq_stats_table,
-			old_cpu);
-	per_cpu(cpufreq_stats_table, old_cpu) = NULL;
-	stat->cpu = new_cpu;
+	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table,
+			policy->last_cpu);
+
+	pr_debug("Updating stats_table for new_cpu %u from last_cpu %u\n",
+			policy->cpu, policy->last_cpu);
+	per_cpu(cpufreq_stats_table, policy->cpu) = per_cpu(cpufreq_stats_table,
+			policy->last_cpu);
+	per_cpu(cpufreq_stats_table, policy->last_cpu) = NULL;
+	stat->cpu = policy->cpu;
 }

 static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
@@ -287,6 +287,12 @@ static int cpufreq_stat_notifier_policy(struct
notifier_block *nb,
 	struct cpufreq_policy *policy = data;
 	struct cpufreq_frequency_table *table;
 	unsigned int cpu = policy->cpu;
+
+	if (val == CPUFREQ_UPDATE_POLICY_CPU) {
+		cpufreq_stats_update_policy_cpu(policy);
+		return 0;
+	}
+
 	if (val != CPUFREQ_NOTIFY)
 		return 0;
 	table = cpufreq_frequency_get_table(cpu);
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 2c3b07b..281e3b4 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -225,14 +225,13 @@ void cpufreq_frequency_table_put_attr(unsigned int cpu)
 }
 EXPORT_SYMBOL_GPL(cpufreq_frequency_table_put_attr);

-void cpufreq_frequency_table_update_policy_cpu(unsigned int old_cpu,
-		unsigned int new_cpu)
+void cpufreq_frequency_table_update_policy_cpu(struct cpufreq_policy *policy)
 {
-	pr_debug("Updating show_table for new_cpu %u from old_cpu %u\n",
-			new_cpu, old_cpu);
-	per_cpu(cpufreq_show_table, new_cpu) = per_cpu(cpufreq_show_table,
-			old_cpu);
-	per_cpu(cpufreq_show_table, old_cpu) = NULL;
+	pr_debug("Updating show_table for new_cpu %u from last_cpu %u\n",
+			policy->cpu, policy->last_cpu);
+	per_cpu(cpufreq_show_table, policy->cpu) = per_cpu(cpufreq_show_table,
+			policy->last_cpu);
+	per_cpu(cpufreq_show_table, policy->last_cpu) = NULL;
 }

 struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index af50dbd..7a3192a 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -90,7 +90,9 @@ struct cpufreq_policy {
 	cpumask_var_t		related_cpus; /* CPUs with any coordination */
 	unsigned int		shared_type; /* ANY or ALL affected CPUs
 						should set cpufreq */
-	unsigned int		cpu;    /* cpu nr of registered CPU */
+	unsigned int		cpu;    /* cpu nr of CPU managing this policy */
+	unsigned int		last_cpu; /* cpu nr of previous CPU that managed
+					   * this policy */
 	struct cpufreq_cpuinfo	cpuinfo;/* see above */

 	unsigned int		min;    /* in kHz */
@@ -109,10 +111,11 @@ struct cpufreq_policy {
 	struct completion	kobj_unregister;
 };

-#define CPUFREQ_ADJUST		(0)
-#define CPUFREQ_INCOMPATIBLE	(1)
-#define CPUFREQ_NOTIFY		(2)
-#define CPUFREQ_START		(3)
+#define CPUFREQ_ADJUST			(0)
+#define CPUFREQ_INCOMPATIBLE		(1)
+#define CPUFREQ_NOTIFY			(2)
+#define CPUFREQ_START			(3)
+#define CPUFREQ_UPDATE_POLICY_CPU	(4)

 #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
 #define CPUFREQ_SHARED_TYPE_HW	 (1) /* HW does needed coordination */
@@ -405,12 +408,8 @@ extern struct freq_attr
cpufreq_freq_attr_scaling_available_freqs;

 void cpufreq_frequency_table_get_attr(struct cpufreq_frequency_table *table,
 				      unsigned int cpu);
-void cpufreq_frequency_table_update_policy_cpu(unsigned int old_cpu,
-		unsigned int new_cpu);
+void cpufreq_frequency_table_update_policy_cpu(struct cpufreq_policy *policy);

 void cpufreq_frequency_table_put_attr(unsigned int cpu);

-void cpufreq_stats_update_policy_cpu(unsigned int old_cpu,
-		unsigned int new_cpu);
-

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Rafael J. Wysocki]

On Thursday, January 10, 2013 04:10:55 PM Shawn Guo wrote:
> On 10 January 2013 16:05, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Another thing, can i have a tested-by from you for both my patches ? remove and
> > add dev?
> >
> For both:
> 
> Tested-by: Shawn Guo <shawn.guo@linaro.org>

OK

Can any one of you two create a series of patches for me to take for v3.9
with all of the correct Tested-by and Reviewed-by etc. tags, because I've
already lost track of your multiple versions and threads?

Rafael


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

Re: [PATCH] cpufreq: Simplify __cpufreq_remove_dev()

[Viresh Kumar]

On 12 January 2013 03:41, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Can any one of you two create a series of patches for me to take for v3.9
> with all of the correct Tested-by and Reviewed-by etc. tags, because I've
> already lost track of your multiple versions and threads?

:)
I will.