Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1190359 > unrolled thread

[PATCH 2/2] cpufreq: Separate CPU device removal from CPU online

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] cpufreq: Separate CPU device removal from CPU online "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-07-23 01:40 +0200
    Re: [PATCH 2/2] cpufreq: Separate CPU device removal from CPU online Viresh Kumar <viresh.kumar@linaro.org> - 2015-07-23 08:40 +0200
      Re: [PATCH 2/2] cpufreq: Separate CPU device removal from CPU online Viresh Kumar <viresh.kumar@linaro.org> - 2015-07-24 04:20 +0200

#1190359 — [PATCH 2/2] cpufreq: Separate CPU device removal from CPU online

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

To separate the CPU online interface from the CPU device removal
one, split cpufreq_dev_online() out of cpufreq_add_dev() and make
cpufreq_cpu_callback() call the former, while the latter will only
be used as the CPU device removal subsystem interface callback.

Suggested-by: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |  109 +++++++++++++++++++++++-----------------------
 1 file changed, 56 insertions(+), 53 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1177,45 +1177,14 @@ static void cpufreq_policy_free(struct c
 	kfree(policy);
 }
 
-/**
- * cpufreq_add_dev - add a CPU device
- *
- * Adds the cpufreq interface for a CPU device.
- *
- * The Oracle says: try running cpufreq registration/unregistration concurrently
- * with with cpu hotplugging and all hell will break loose. Tried to clean this
- * mess up, but more thorough testing is needed. - Mathieu
- */
-static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
+static int cpufreq_dev_online(struct device *dev, bool recover_policy)
 {
 	unsigned int j, cpu = dev->id;
 	int ret = -ENOMEM;
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 	unsigned long flags;
-	bool recover_policy = !sif;
 
-	pr_debug("adding CPU %u\n", cpu);
-
-	/* sysfs links are only created on subsys callback */
-	if (sif && policy) {
-		pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
-		ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
-		if (ret) {
-			dev_dbg(dev, "%s: Failed to create link (%d)\n",
-				__func__, ret);
-			return ret;
-		}
-
-		/* Track CPUs for which sysfs links are created */
-		cpumask_set_cpu(cpu, policy->linked_cpus);
-	}
-
-	/*
-	 * A hotplug notifier will follow and we will take care of rest
-	 * of the initialization then.
-	 */
-	if (cpu_is_offline(cpu))
-		return 0;
+	pr_debug("%s: bringing CPU%u online\n", __func__, cpu);
 
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
@@ -1376,6 +1345,36 @@ out_release_rwsem:
 	return ret;
 }
 
+/**
+ * cpufreq_add_dev - add a cpufreq interface to a CPU device.
+ * @dev: CPU device to add the interface to.
+ * @sif: Subsystem interface pointer.
+ */
+static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
+{
+	unsigned int cpu = dev->id;
+	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+
+	pr_debug("%s: adding CPU %u\n", __func__, cpu);
+
+	if (policy && policy->kobj_cpu != cpu) {
+		int ret;
+
+		pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
+		ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+		if (ret) {
+			dev_dbg(dev, "%s: Failed to create link (%d)\n",
+				__func__, ret);
+			return ret;
+		}
+
+		/* Track CPUs for which sysfs links are created */
+		cpumask_set_cpu(cpu, policy->linked_cpus);
+	}
+
+	return cpu_online(cpu) ? cpufreq_dev_online(dev, false) : 0;
+}
+
 static void cpufreq_offline_prepare(unsigned int cpu)
 {
 	struct cpufreq_policy *policy;
@@ -2344,31 +2343,35 @@ unlock:
 }
 EXPORT_SYMBOL(cpufreq_update_policy);
 
+static void cpufreq_cpu_online(unsigned int cpu)
+{
+	struct device *dev = get_cpu_device(cpu);
+
+	if (dev)
+		cpufreq_dev_online(dev, true);
+}
+
 static int cpufreq_cpu_callback(struct notifier_block *nfb,
 					unsigned long action, void *hcpu)
 {
 	unsigned int cpu = (unsigned long)hcpu;
-	struct device *dev;
 
-	dev = get_cpu_device(cpu);
-	if (dev) {
-		switch (action & ~CPU_TASKS_FROZEN) {
-		case CPU_ONLINE:
-			cpufreq_add_dev(dev, NULL);
-			break;
-
-		case CPU_DOWN_PREPARE:
-			cpufreq_offline_prepare(cpu);
-			break;
-
-		case CPU_POST_DEAD:
-			cpufreq_offline_finish(cpu);
-			break;
-
-		case CPU_DOWN_FAILED:
-			cpufreq_add_dev(dev, NULL);
-			break;
-		}
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_ONLINE:
+		cpufreq_cpu_online(cpu);
+		break;
+
+	case CPU_DOWN_PREPARE:
+		cpufreq_offline_prepare(cpu);
+		break;
+
+	case CPU_POST_DEAD:
+		cpufreq_offline_finish(cpu);
+		break;
+
+	case CPU_DOWN_FAILED:
+		cpufreq_cpu_online(cpu);
+		break;
 	}
 	return NOTIFY_OK;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1190654

On 23-07-15, 02:04, Rafael J. Wysocki wrote:
> +static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> +{
> +	unsigned int cpu = dev->id;
> +	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> +
> +	pr_debug("%s: adding CPU %u\n", __func__, cpu);
> +
> +	if (policy && policy->kobj_cpu != cpu) {

Why are you comparing cpu against kobj_cpu ? I don't think it can ever
be false.

> +		int ret;
> +
> +		pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);

dev_dbg

> +		ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> +		if (ret) {
> +			dev_dbg(dev, "%s: Failed to create link (%d)\n",

dev_err

> +				__func__, ret);
> +			return ret;
> +		}
> +
> +		/* Track CPUs for which sysfs links are created */
> +		cpumask_set_cpu(cpu, policy->linked_cpus);
> +	}
> +
> +	return cpu_online(cpu) ? cpufreq_dev_online(dev, false) : 0;
> +}

Looks fine otherwise. Thanks for getting your hands dirty :)

>  static void cpufreq_offline_prepare(unsigned int cpu)
>  {
>  	struct cpufreq_policy *policy;
> @@ -2344,31 +2343,35 @@ unlock:
>  }
>  EXPORT_SYMBOL(cpufreq_update_policy);
>  
> +static void cpufreq_cpu_online(unsigned int cpu)
> +{
> +	struct device *dev = get_cpu_device(cpu);
> +
> +	if (dev)
> +		cpufreq_dev_online(dev, true);
> +}

What about dropping this wrapper function and ...

>  static int cpufreq_cpu_callback(struct notifier_block *nfb,
>  					unsigned long action, void *hcpu)
>  {
>  	unsigned int cpu = (unsigned long)hcpu;
> -	struct device *dev;
>  
> -	dev = get_cpu_device(cpu);

... keeping this as is? And then we can do
s/cpufreq_dev_online/cpufreq_cpu_online which suits better.

> -	if (dev) {
> -		switch (action & ~CPU_TASKS_FROZEN) {
> -		case CPU_ONLINE:
> -			cpufreq_add_dev(dev, NULL);
> -			break;
> -
> -		case CPU_DOWN_PREPARE:
> -			cpufreq_offline_prepare(cpu);
> -			break;
> -
> -		case CPU_POST_DEAD:
> -			cpufreq_offline_finish(cpu);
> -			break;
> -
> -		case CPU_DOWN_FAILED:
> -			cpufreq_add_dev(dev, NULL);
> -			break;
> -		}
> +	switch (action & ~CPU_TASKS_FROZEN) {
> +	case CPU_ONLINE:
> +		cpufreq_cpu_online(cpu);
> +		break;
> +
> +	case CPU_DOWN_PREPARE:
> +		cpufreq_offline_prepare(cpu);
> +		break;
> +
> +	case CPU_POST_DEAD:
> +		cpufreq_offline_finish(cpu);
> +		break;
> +
> +	case CPU_DOWN_FAILED:
> +		cpufreq_cpu_online(cpu);
> +		break;
>  	}
>  	return NOTIFY_OK;
>  }

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191453

On 23-07-15, 22:56, Rafael J. Wysocki wrote:
> > > +	if (policy && policy->kobj_cpu != cpu) {
> > 
> > Why are you comparing cpu against kobj_cpu ? I don't think it can ever
> > be false.

So what I meant was that the expression 'policy->kobj_cpu != cpu' will
never return 'false'. Because policy->kobj_cpu is going to get set to
the cpu for which we allocated the policy. And so it wouldn't match
for any other CPU.

> It can.  When we're adding a CPU that has a policy already, because it is
> "related" to a previously registered CPU.

In this case also the expression will return true.

> > > +		ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> > > +		if (ret) {
> > > +			dev_dbg(dev, "%s: Failed to create link (%d)\n",
> > 
> > dev_err
> 
> Well, I'm wondering about this.  Why does this have to be dev_err()?

Isn't this an error? We need to create a symlink, we failed and
atleast the user should know about it. Why hide such failures ?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web