[PATCH 0/2] cpufreq: Better separation of device addition/removal and online/offline paths

[PATCH 0/2] cpufreq: Better separation of device addition/removal and online/offline paths

[Rafael J. Wysocki]

Hi,

As per the recent discussion (http://marc.info/?l=linux-pm&m=143758336123670&w=4),
separete CPU device addition/removal code paths from the CPU online/offline code
paths in the cpufreq core a bit better.

Both on top of http://marc.info/?l=linux-pm&m=143760250929586&w=4.

Thanks,
Rafael

[PATCH 1/2] cpufreq: Rename two functions related to CPU offline

[Rafael J. Wysocki]

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

Since __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
are about CPU offline rather than about CPU removal, rename them to
cpufreq_offline_prepare() and cpufreq_offline_finish(), respectively.

Also change their argument from a struct device pointer to a CPU
number, because they use the CPU number only internally anyway.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1376,9 +1376,8 @@ out_release_rwsem:
 	return ret;
 }
 
-static void __cpufreq_remove_dev_prepare(struct device *dev)
+static void cpufreq_offline_prepare(unsigned int cpu)
 {
-	unsigned int cpu = dev->id;
 	struct cpufreq_policy *policy;
 
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
@@ -1423,9 +1422,8 @@ static void __cpufreq_remove_dev_prepare
 	}
 }
 
-static void __cpufreq_remove_dev_finish(struct device *dev)
+static void cpufreq_offline_finish(unsigned int cpu)
 {
-	unsigned int cpu = dev->id;
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
 	if (!policy) {
@@ -1467,8 +1465,8 @@ static int cpufreq_remove_dev(struct dev
 		return 0;
 
 	if (cpu_online(cpu)) {
-		__cpufreq_remove_dev_prepare(dev);
-		__cpufreq_remove_dev_finish(dev);
+		cpufreq_offline_prepare(cpu);
+		cpufreq_offline_finish(cpu);
 	}
 
 	/* sysfs links are removed only on subsys callback */
@@ -2360,11 +2358,11 @@ static int cpufreq_cpu_callback(struct n
 			break;
 
 		case CPU_DOWN_PREPARE:
-			__cpufreq_remove_dev_prepare(dev);
+			cpufreq_offline_prepare(cpu);
 			break;
 
 		case CPU_POST_DEAD:
-			__cpufreq_remove_dev_finish(dev);
+			cpufreq_offline_finish(cpu);
 			break;
 
 		case CPU_DOWN_FAILED:

Re: [PATCH 1/2] cpufreq: Rename two functions related to CPU offline

[Viresh Kumar]

On 23-07-15, 02:01, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
> are about CPU offline rather than about CPU removal, rename them to
> cpufreq_offline_prepare() and cpufreq_offline_finish(), respectively.
> 
> Also change their argument from a struct device pointer to a CPU
> number, because they use the CPU number only internally anyway.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

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

-- 
viresh

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

[Rafael J. Wysocki]

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;
 }

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

[Viresh Kumar]

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

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

[Rafael J. Wysocki]

On Thursday, July 23, 2015 12:09:42 PM Viresh Kumar wrote:
> 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.

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


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

OK

> > +		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()?


> > +				__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.

Well, we don't need the dev things for DOWN_PREPARE and POST_DEAD.

We actually only need it in a few places in cpufreq_dev_online(), or
maybe simply cpufreq_online(), so it can take the cpu argument too.

> > -	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;
> >  }
> 
> 

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

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

[Viresh Kumar]

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

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

[Rafael J. Wysocki]

On Friday, July 24, 2015 07:49:16 AM Viresh Kumar wrote:
> 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.

Unless kobj_cpu is removed before another one is registered (not a very
realistic scenario, but there's no guarantee that it will never happen).

But in that case we won't do the right thing anyway ...

> > 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 ?

dev_err() doesn't mean "device error message".  It means "high priority device
message".

The user will know that the link is not there anyway and the question here is
whether or not the associated kernel message has to be high-priority.


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

[PATCH 0/7] cpufreq: Better separation of device addition/removal and online/offline paths

[Rafael J. Wysocki]

Hi,

On Thursday, July 23, 2015 02:00:23 AM Rafael J. Wysocki wrote:
> Hi,
> 
> As per the recent discussion (http://marc.info/?l=linux-pm&m=143758336123670&w=4),
> separete CPU device addition/removal code paths from the CPU online/offline code
> paths in the cpufreq core a bit better.

A new version of this, with more cleanups included, on top of the linux-next
branch of the linux-pm.git tree (from today).

Thanks,
Rafael

[PATCH 1/7] cpufreq: Rework two functions related to CPU offline

[Rafael J. Wysocki]

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

Since __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
are about CPU offline rather than about CPU removal, rename them to
cpufreq_offline_prepare() and cpufreq_offline_finish(), respectively.

Also change their argument from a struct device pointer to a CPU
number, because they use the CPU number only internally anyway
and make them void as their return values are ignored.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1398,10 +1398,8 @@ out_release_rwsem:
 	return ret;
 }
 
-static int __cpufreq_remove_dev_prepare(struct device *dev)
+static void cpufreq_offline_prepare(unsigned int cpu)
 {
-	unsigned int cpu = dev->id;
-	int ret = 0;
 	struct cpufreq_policy *policy;
 
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
@@ -1409,11 +1407,11 @@ static int __cpufreq_remove_dev_prepare(
 	policy = cpufreq_cpu_get_raw(cpu);
 	if (!policy) {
 		pr_debug("%s: No cpu_data found\n", __func__);
-		return -EINVAL;
+		return;
 	}
 
 	if (has_target()) {
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
+		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 		if (ret)
 			pr_err("%s: Failed to stop governor\n", __func__);
 	}
@@ -1434,7 +1432,7 @@ static int __cpufreq_remove_dev_prepare(
 	/* Start governor again for active policy */
 	if (!policy_is_inactive(policy)) {
 		if (has_target()) {
-			ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
+			int ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
 			if (!ret)
 				ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
@@ -1444,28 +1442,24 @@ static int __cpufreq_remove_dev_prepare(
 	} else if (cpufreq_driver->stop_cpu) {
 		cpufreq_driver->stop_cpu(policy);
 	}
-
-	return ret;
 }
 
-static int __cpufreq_remove_dev_finish(struct device *dev)
+static void cpufreq_offline_finish(unsigned int cpu)
 {
-	unsigned int cpu = dev->id;
-	int ret;
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
 	if (!policy) {
 		pr_debug("%s: No cpu_data found\n", __func__);
-		return -EINVAL;
+		return;
 	}
 
 	/* Only proceed for inactive policies */
 	if (!policy_is_inactive(policy))
-		return 0;
+		return;
 
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
 		if (ret)
 			pr_err("%s: Failed to exit governor\n", __func__);
 	}
@@ -1477,8 +1471,6 @@ static int __cpufreq_remove_dev_finish(s
 	 */
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
-
-	return 0;
 }
 
 /**
@@ -1495,8 +1487,8 @@ static int cpufreq_remove_dev(struct dev
 		return 0;
 
 	if (cpu_online(cpu)) {
-		__cpufreq_remove_dev_prepare(dev);
-		__cpufreq_remove_dev_finish(dev);
+		cpufreq_offline_prepare(cpu);
+		cpufreq_offline_finish(cpu);
 	}
 
 	cpumask_clear_cpu(cpu, policy->real_cpus);
@@ -2379,11 +2371,11 @@ static int cpufreq_cpu_callback(struct n
 			break;
 
 		case CPU_DOWN_PREPARE:
-			__cpufreq_remove_dev_prepare(dev);
+			cpufreq_offline_prepare(cpu);
 			break;
 
 		case CPU_POST_DEAD:
-			__cpufreq_remove_dev_finish(dev);
+			cpufreq_offline_finish(cpu);
 			break;
 
 		case CPU_DOWN_FAILED:

Re: [PATCH 1/7] cpufreq: Rework two functions related to CPU offline

[Viresh Kumar]

On 27-07-15, 16:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
> are about CPU offline rather than about CPU removal, rename them to
> cpufreq_offline_prepare() and cpufreq_offline_finish(), respectively.
> 
> Also change their argument from a struct device pointer to a CPU
> number, because they use the CPU number only internally anyway
> and make them void as their return values are ignored.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)

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

-- 
viresh

[PATCH 2/7] cpufreq: Drop cpufreq_policy_restore()

[Rafael J. Wysocki]

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

Notice that when cpufreq_policy_restore() is called, its per-CPU
cpufreq_cpu_data variable has been already dereferenced and if that
variable is not NULL, the policy local pointer in cpufreq_add_dev()
contains its value.

Therefore it is not necessary to dereference it again and the
policy pointer can be used directly.  Moreover, if that pointer
is not NULL, the policy is inactive (or the previous check would
have made us return from cpufreq_add_dev()) so the restoration
code from cpufreq_policy_restore() can be moved to that point
in cpufreq_add_dev().

Do that and drop cpufreq_policy_restore().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   44 +++++++++++---------------------------------
 1 file changed, 11 insertions(+), 33 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1092,28 +1092,6 @@ static int cpufreq_add_policy_cpu(struct
 	return 0;
 }
 
-static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
-{
-	struct cpufreq_policy *policy;
-	unsigned long flags;
-
-	read_lock_irqsave(&cpufreq_driver_lock, flags);
-	policy = per_cpu(cpufreq_cpu_data, cpu);
-	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
-	if (likely(policy)) {
-		/* Policy should be inactive here */
-		WARN_ON(!policy_is_inactive(policy));
-
-		down_write(&policy->rwsem);
-		policy->cpu = cpu;
-		policy->governor = NULL;
-		up_write(&policy->rwsem);
-	}
-
-	return policy;
-}
-
 static struct cpufreq_policy *cpufreq_policy_alloc(struct device *dev)
 {
 	struct cpufreq_policy *policy;
@@ -1226,7 +1204,7 @@ static int cpufreq_add_dev(struct device
 	int ret = -ENOMEM;
 	struct cpufreq_policy *policy;
 	unsigned long flags;
-	bool recover_policy = !sif;
+	bool recover_policy;
 
 	pr_debug("adding CPU %u\n", cpu);
 
@@ -1244,18 +1222,18 @@ static int cpufreq_add_dev(struct device
 
 	/* Check if this CPU already has a policy to manage it */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
-	if (policy && !policy_is_inactive(policy)) {
+	if (policy) {
 		WARN_ON(!cpumask_test_cpu(cpu, policy->related_cpus));
-		ret = cpufreq_add_policy_cpu(policy, cpu, dev);
-		return ret;
-	}
+		if (!policy_is_inactive(policy))
+			return cpufreq_add_policy_cpu(policy, cpu, dev);
 
-	/*
-	 * Restore the saved policy when doing light-weight init and fall back
-	 * to the full init if that fails.
-	 */
-	policy = recover_policy ? cpufreq_policy_restore(cpu) : NULL;
-	if (!policy) {
+		/* This is the only online CPU for the policy.  Start over. */
+		recover_policy = true;
+		down_write(&policy->rwsem);
+		policy->cpu = cpu;
+		policy->governor = NULL;
+		up_write(&policy->rwsem);
+	} else {
 		recover_policy = false;
 		policy = cpufreq_policy_alloc(dev);
 		if (!policy)

Re: [PATCH 2/7] cpufreq: Drop cpufreq_policy_restore()

[Viresh Kumar]

On 27-07-15, 16:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notice that when cpufreq_policy_restore() is called, its per-CPU
> cpufreq_cpu_data variable has been already dereferenced and if that
> variable is not NULL, the policy local pointer in cpufreq_add_dev()
> contains its value.
> 
> Therefore it is not necessary to dereference it again and the
> policy pointer can be used directly.  Moreover, if that pointer
> is not NULL, the policy is inactive (or the previous check would
> have made us return from cpufreq_add_dev()) so the restoration
> code from cpufreq_policy_restore() can be moved to that point
> in cpufreq_add_dev().
> 
> Do that and drop cpufreq_policy_restore().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   44 +++++++++++---------------------------------
>  1 file changed, 11 insertions(+), 33 deletions(-)

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

-- 
viresh

[PATCH 3/7] cpufreq: Drop unnecessary label from cpufreq_add_dev()

[Rafael J. Wysocki]

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

The leftover out_release_rwsem label in cpufreq_add_dev() is not
necessary any more and confusing, so drop it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1237,7 +1237,7 @@ static int cpufreq_add_dev(struct device
 		recover_policy = false;
 		policy = cpufreq_policy_alloc(dev);
 		if (!policy)
-			goto out_release_rwsem;
+			return -ENOMEM;
 	}
 
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
@@ -1372,7 +1372,6 @@ out_exit_policy:
 		cpufreq_driver->exit(policy);
 out_free_policy:
 	cpufreq_policy_free(policy, recover_policy);
-out_release_rwsem:
 	return ret;
 }
 

Re: [PATCH 3/7] cpufreq: Drop unnecessary label from cpufreq_add_dev()

[Viresh Kumar]

On 27-07-15, 16:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The leftover out_release_rwsem label in cpufreq_add_dev() is not
> necessary any more and confusing, so drop it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -1237,7 +1237,7 @@ static int cpufreq_add_dev(struct device
>  		recover_policy = false;
>  		policy = cpufreq_policy_alloc(dev);
>  		if (!policy)
> -			goto out_release_rwsem;
> +			return -ENOMEM;
>  	}
>  
>  	cpumask_copy(policy->cpus, cpumask_of(cpu));
> @@ -1372,7 +1372,6 @@ out_exit_policy:
>  		cpufreq_driver->exit(policy);
>  out_free_policy:
>  	cpufreq_policy_free(policy, recover_policy);
> -out_release_rwsem:
>  	return ret;

There is no need to initialize ret to -ENOMEM now, please get rid of
that as well and add my

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

-- 
viresh

[PATCH 4/7] cpufreq: Drop unused dev argument from two functions

[Rafael J. Wysocki]

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

The dev argument of cpufreq_add_policy_cpu() and
cpufreq_add_dev_interface() is not used by any of them,
so drop it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -999,8 +999,7 @@ static void cpufreq_remove_dev_symlink(s
 	}
 }
 
-static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
-				     struct device *dev)
+static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
 {
 	struct freq_attr **drv_attr;
 	int ret = 0;
@@ -1057,8 +1056,7 @@ static int cpufreq_init_policy(struct cp
 	return cpufreq_set_policy(policy, &new_policy);
 }
 
-static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
-				  unsigned int cpu, struct device *dev)
+static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
 	int ret = 0;
 
@@ -1225,7 +1223,7 @@ static int cpufreq_add_dev(struct device
 	if (policy) {
 		WARN_ON(!cpumask_test_cpu(cpu, policy->related_cpus));
 		if (!policy_is_inactive(policy))
-			return cpufreq_add_policy_cpu(policy, cpu, dev);
+			return cpufreq_add_policy_cpu(policy, cpu);
 
 		/* This is the only online CPU for the policy.  Start over. */
 		recover_policy = true;
@@ -1328,7 +1326,7 @@ static int cpufreq_add_dev(struct device
 				     CPUFREQ_START, policy);
 
 	if (!recover_policy) {
-		ret = cpufreq_add_dev_interface(policy, dev);
+		ret = cpufreq_add_dev_interface(policy);
 		if (ret)
 			goto out_exit_policy;
 		blocking_notifier_call_chain(&cpufreq_policy_notifier_list,

Re: [PATCH 4/7] cpufreq: Drop unused dev argument from two functions

[Viresh Kumar]

On 27-07-15, 16:05, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The dev argument of cpufreq_add_policy_cpu() and
> cpufreq_add_dev_interface() is not used by any of them,
> so drop it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

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

-- 
viresh

[PATCH 5/7] cpufreq: Do not update related_cpus on every policy activation

[Rafael J. Wysocki]

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

The related_cpus mask includes CPUs whose cpufreq_cpu_data per-CPU
pointers have been set to the given policy.  Since those pointers
are only set at the policy creation time and unset when the policy
is deleted, the related_cpus should not be updated between those
two operations.

For this reason, avoid updating it whenever the first of the
"related" CPUs goes online.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1251,12 +1251,12 @@ static int cpufreq_add_dev(struct device
 
 	down_write(&policy->rwsem);
 
-	/* related cpus should atleast have policy->cpus */
-	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
-
-	/* Remember which CPUs have been present at the policy creation time. */
-	if (!recover_policy)
+	if (!recover_policy) {
+		/* related_cpus should at least include policy->cpus. */
+		cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
+		/* Remember CPUs present at the policy creation time. */
 		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
+	}
 
 	/*
 	 * affected cpus must always be the one, which are online. We aren't

Re: [PATCH 5/7] cpufreq: Do not update related_cpus on every policy activation

[Viresh Kumar]

On 27-07-15, 16:06, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The related_cpus mask includes CPUs whose cpufreq_cpu_data per-CPU
> pointers have been set to the given policy.  Since those pointers
> are only set at the policy creation time and unset when the policy
> is deleted, the related_cpus should not be updated between those
> two operations.
> 
> For this reason, avoid updating it whenever the first of the
> "related" CPUs goes online.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

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

-- 
viresh

[PATCH 6/7] cpufreq: Pass CPU number to cpufreq_policy_alloc()

[Rafael J. Wysocki]

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

Change cpufreq_policy_alloc() to take a CPU number instead of a CPU
device pointer as its argument, as it is the only function called by
cpufreq_add_dev() taking a device pointer argument at this point.

That will allow us to split the CPU online part from cpufreq_add_dev()
more cleanly going forward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1090,11 +1090,15 @@ static int cpufreq_add_policy_cpu(struct
 	return 0;
 }
 
-static struct cpufreq_policy *cpufreq_policy_alloc(struct device *dev)
+static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
+	struct device *dev = get_cpu_device(cpu);
 	struct cpufreq_policy *policy;
 	int ret;
 
+	if (WARN_ON(!dev))
+		return NULL;
+
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
 	if (!policy)
 		return NULL;
@@ -1122,10 +1126,10 @@ static struct cpufreq_policy *cpufreq_po
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
-	policy->cpu = dev->id;
+	policy->cpu = cpu;
 
 	/* Set this once on allocation */
-	policy->kobj_cpu = dev->id;
+	policy->kobj_cpu = cpu;
 
 	return policy;
 
@@ -1233,7 +1237,7 @@ static int cpufreq_add_dev(struct device
 		up_write(&policy->rwsem);
 	} else {
 		recover_policy = false;
-		policy = cpufreq_policy_alloc(dev);
+		policy = cpufreq_policy_alloc(cpu);
 		if (!policy)
 			return -ENOMEM;
 	}

Re: [PATCH 6/7] cpufreq: Pass CPU number to cpufreq_policy_alloc()

[Viresh Kumar]

On 27-07-15, 16:07, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Change cpufreq_policy_alloc() to take a CPU number instead of a CPU
> device pointer as its argument, as it is the only function called by
> cpufreq_add_dev() taking a device pointer argument at this point.
> 
> That will allow us to split the CPU online part from cpufreq_add_dev()
> more cleanly going forward.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

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

-- 
viresh

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

[Rafael J. Wysocki]

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

To separate the CPU online interface from the CPU device removal
one, split cpufreq_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.

While at it, notice that the return value of sif->add_dev() is
ignored in bus_probe_device(), so (the new) cpufreq_add_dev()
doesn't need to bother with returning anything different from 0
and cpufreq_online() may be a void function.

Moreover, since the return value of cpufreq_add_policy_cpu() is
going to be ignored now too, make a void function of it as well.

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

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1056,19 +1056,17 @@ static int cpufreq_init_policy(struct cp
 	return cpufreq_set_policy(policy, &new_policy);
 }
 
-static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
+static void cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
-	int ret = 0;
-
 	/* Has this CPU been taken care of already? */
 	if (cpumask_test_cpu(cpu, policy->cpus))
-		return 0;
+		return;
 
 	if (has_target()) {
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
+		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 		if (ret) {
 			pr_err("%s: Failed to stop governor\n", __func__);
-			return ret;
+			return;
 		}
 	}
 
@@ -1077,17 +1075,13 @@ static int cpufreq_add_policy_cpu(struct
 	up_write(&policy->rwsem);
 
 	if (has_target()) {
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
+		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
 		if (!ret)
 			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to start governor\n", __func__);
-			return ret;
-		}
 	}
-
-	return 0;
 }
 
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
@@ -1191,44 +1185,24 @@ 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 void cpufreq_online(unsigned int cpu)
 {
-	unsigned int j, cpu = dev->id;
-	int ret = -ENOMEM;
 	struct cpufreq_policy *policy;
-	unsigned long flags;
 	bool recover_policy;
+	unsigned long flags;
+	unsigned int j;
+	int ret;
 
-	pr_debug("adding CPU %u\n", cpu);
-
-	if (cpu_is_offline(cpu)) {
-		/*
-		 * Only possible if we are here from the subsys_interface add
-		 * callback.  A hotplug notifier will follow and we will handle
-		 * it as CPU online then.  For now, just create the sysfs link,
-		 * unless there is no policy or the link is already present.
-		 */
-		policy = per_cpu(cpufreq_cpu_data, cpu);
-		return policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus)
-			? add_cpu_dev_symlink(policy, cpu) : 0;
-	}
+	pr_debug("%s: bringing CPU%u online\n", __func__, cpu);
 
 	/* Check if this CPU already has a policy to manage it */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
 	if (policy) {
 		WARN_ON(!cpumask_test_cpu(cpu, policy->related_cpus));
-		if (!policy_is_inactive(policy))
-			return cpufreq_add_policy_cpu(policy, cpu);
-
+		if (!policy_is_inactive(policy)) {
+			cpufreq_add_policy_cpu(policy, cpu);
+			return;
+		}
 		/* This is the only online CPU for the policy.  Start over. */
 		recover_policy = true;
 		down_write(&policy->rwsem);
@@ -1239,7 +1213,7 @@ static int cpufreq_add_dev(struct device
 		recover_policy = false;
 		policy = cpufreq_policy_alloc(cpu);
 		if (!policy)
-			return -ENOMEM;
+			return;
 	}
 
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
@@ -1362,7 +1336,7 @@ static int cpufreq_add_dev(struct device
 
 	pr_debug("initialization complete\n");
 
-	return 0;
+	return;
 
 out_remove_policy_notify:
 	/* cpufreq_policy_free() will notify based on this */
@@ -1374,7 +1348,34 @@ out_exit_policy:
 		cpufreq_driver->exit(policy);
 out_free_policy:
 	cpufreq_policy_free(policy, recover_policy);
-	return ret;
+}
+
+/**
+ * cpufreq_add_dev - the cpufreq interface for a CPU device.
+ * @dev: CPU device.
+ * @sif: Subsystem interface structure pointer (not used)
+ */
+static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
+{
+	unsigned cpu = dev->id;
+
+	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
+
+	if (cpu_online(cpu)) {
+		cpufreq_online(cpu);
+	} else {
+		/*
+		 * A hotplug notifier will follow and we will handle it as CPU
+		 * online then.  For now, just create the sysfs link, unless
+		 * there is no policy or the link is already present.
+		 */
+		struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+
+		if (policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus))
+			WARN_ON(add_cpu_dev_symlink(policy, cpu));
+	}
+
+	return 0;
 }
 
 static void cpufreq_offline_prepare(unsigned int cpu)
@@ -2340,27 +2341,23 @@ static int cpufreq_cpu_callback(struct n
 					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_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_online(cpu);
+		break;
 	}
 	return NOTIFY_OK;
 }

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

[Viresh Kumar]

On 27-07-15, 16:09, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> To separate the CPU online interface from the CPU device removal
> one,

Why do you call this cpu device removal code?

> split cpufreq_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.
> 
> While at it, notice that the return value of sif->add_dev() is
> ignored in bus_probe_device(), so (the new) cpufreq_add_dev()
> doesn't need to bother with returning anything different from 0
> and cpufreq_online() may be a void function.

That is going to change in 4.3:

https://lkml.org/lkml/2015/6/26/132

> 
> Moreover, since the return value of cpufreq_add_policy_cpu() is
> going to be ignored now too, make a void function of it as well.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Suggested-by: Russell King <linux@arm.linux.org.uk>
> ---
>  drivers/cpufreq/cpufreq.c |  125 ++++++++++++++++++++++------------------------
>  1 file changed, 61 insertions(+), 64 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -1056,19 +1056,17 @@ static int cpufreq_init_policy(struct cp
>  	return cpufreq_set_policy(policy, &new_policy);
>  }
>  
> -static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
> +static void cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
>  {
> -	int ret = 0;
> -
>  	/* Has this CPU been taken care of already? */
>  	if (cpumask_test_cpu(cpu, policy->cpus))
> -		return 0;
> +		return;
>  
>  	if (has_target()) {
> -		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> +		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);

Why should we move the definition of ret here and ...

>  		if (ret) {
>  			pr_err("%s: Failed to stop governor\n", __func__);
> -			return ret;
> +			return;
>  		}
>  	}

-- 
viresh

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

[Rafael J. Wysocki]

On Mon, Jul 27, 2015 at 5:06 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27-07-15, 16:09, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> To separate the CPU online interface from the CPU device removal
>> one,
>
> Why do you call this cpu device removal code?

By mistake.

Of course, that should be addition/registration.

>> split cpufreq_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.
>>
>> While at it, notice that the return value of sif->add_dev() is
>> ignored in bus_probe_device(), so (the new) cpufreq_add_dev()
>> doesn't need to bother with returning anything different from 0
>> and cpufreq_online() may be a void function.
>
> That is going to change in 4.3:
>
> https://lkml.org/lkml/2015/6/26/132

There are some problems with access to klml.org today and I'm not sure
what you mean.

Can you explain your points in addition to sending links to stuff, please?

>> Moreover, since the return value of cpufreq_add_policy_cpu() is
>> going to be ignored now too, make a void function of it as well.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Suggested-by: Russell King <linux@arm.linux.org.uk>
>> ---
>>  drivers/cpufreq/cpufreq.c |  125 ++++++++++++++++++++++------------------------
>>  1 file changed, 61 insertions(+), 64 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
>> +++ linux-pm/drivers/cpufreq/cpufreq.c
>> @@ -1056,19 +1056,17 @@ static int cpufreq_init_policy(struct cp
>>       return cpufreq_set_policy(policy, &new_policy);
>>  }
>>
>> -static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
>> +static void cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
>>  {
>> -     int ret = 0;
>> -
>>       /* Has this CPU been taken care of already? */
>>       if (cpumask_test_cpu(cpu, policy->cpus))
>> -             return 0;
>> +             return;
>>
>>       if (has_target()) {
>> -             ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
>> +             int ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
>
> Why should we move the definition of ret here and ...

We don't have to, but then we don't need the variable outside of the
blocks it is used in.

>>               if (ret) {
>>                       pr_err("%s: Failed to stop governor\n", __func__);
>> -                     return ret;
>> +                     return;
>>               }
>>       }
>

I'll send a new version of this patch.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Mon, Jul 27, 2015 at 10:56 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Jul 27, 2015 at 5:06 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 27-07-15, 16:09, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> To separate the CPU online interface from the CPU device removal
>>> one,
>>
>> Why do you call this cpu device removal code?
>
> By mistake.
>
> Of course, that should be addition/registration.
>
>>> split cpufreq_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.
>>>
>>> While at it, notice that the return value of sif->add_dev() is
>>> ignored in bus_probe_device(), so (the new) cpufreq_add_dev()
>>> doesn't need to bother with returning anything different from 0
>>> and cpufreq_online() may be a void function.
>>
>> That is going to change in 4.3:
>>
>> https://lkml.org/lkml/2015/6/26/132
>
> There are some problems with access to klml.org today and I'm not sure
> what you mean.
>
> Can you explain your points in addition to sending links to stuff, please?

OK, I've just seen that patch, but it doesn't modify bus_probe_device() AFAICS.

Plus we also ignore the return value of cpufreq_add_dev() in the
hotplug notifier callback.

Thanks,
Rafael

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

[Viresh Kumar]

On 27-07-15, 23:56, Rafael J. Wysocki wrote:
> OK, I've just seen that patch, but it doesn't modify bus_probe_device() AFAICS.

Why is bus_probe_device() required to be modified?

I wrote this patch to propagate -EPROBE_DEFER from the ->init()
callback, which is called from cpufreq_register_driver().

And that worked after my patch..

> Plus we also ignore the return value of cpufreq_add_dev() in the
> hotplug notifier callback.

My use case needed it for the subsys callback.

-- 
viresh

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

[Rafael J. Wysocki]

Hi Viresh,

On Tue, Jul 28, 2015 at 4:06 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27-07-15, 23:56, Rafael J. Wysocki wrote:
>> OK, I've just seen that patch, but it doesn't modify bus_probe_device() AFAICS.
>
> Why is bus_probe_device() required to be modified?

Because that's a different place where sif->add() is called and if it
can fail, we need to fail the probing there too.

In other words, failing things in one place and not doing that in
another analogous place is inconsistent and incorrect.

> I wrote this patch to propagate -EPROBE_DEFER from the ->init()
> callback, which is called from cpufreq_register_driver().
>
> And that worked after my patch..
>
>> Plus we also ignore the return value of cpufreq_add_dev() in the
>> hotplug notifier callback.
>
> My use case needed it for the subsys callback.

That case is a failing cpufreq driver registration, right?

What if a CPU is registered after the cpufreq driver has been
registered and *that* fails?  Shouldn't it fail in the same way in
both cases?

Thanks,
Rafael

[Update][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online

[Rafael J. Wysocki]

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

To separate the CPU online interface from the CPU device
registration, split cpufreq_online() out of cpufreq_add_dev()
and make cpufreq_cpu_callback() call the former, while
cpufreq_add_dev() itself will only be used as the CPU device
addition subsystem interface callback.

While at it, notice that the return value of sif->add_dev() is
ignored in bus_probe_device(), so (the new) cpufreq_add_dev()
doesn't need to bother with returning anything different from 0
and cpufreq_online() may be a void function.

Moreover, since the return value of cpufreq_add_policy_cpu() is
going to be ignored now too, make a void function of it.

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

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1056,19 +1056,19 @@ static int cpufreq_init_policy(struct cp
 	return cpufreq_set_policy(policy, &new_policy);
 }
 
-static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
+static void cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
-	int ret = 0;
+	int ret;
 
 	/* Has this CPU been taken care of already? */
 	if (cpumask_test_cpu(cpu, policy->cpus))
-		return 0;
+		return;
 
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 		if (ret) {
 			pr_err("%s: Failed to stop governor\n", __func__);
-			return ret;
+			return;
 		}
 	}
 
@@ -1081,13 +1081,9 @@ static int cpufreq_add_policy_cpu(struct
 		if (!ret)
 			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to start governor\n", __func__);
-			return ret;
-		}
 	}
-
-	return 0;
 }
 
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
@@ -1191,44 +1187,24 @@ 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 void cpufreq_online(unsigned int cpu)
 {
-	unsigned int j, cpu = dev->id;
-	int ret;
 	struct cpufreq_policy *policy;
-	unsigned long flags;
 	bool recover_policy;
+	unsigned long flags;
+	unsigned int j;
+	int ret;
 
-	pr_debug("adding CPU %u\n", cpu);
-
-	if (cpu_is_offline(cpu)) {
-		/*
-		 * Only possible if we are here from the subsys_interface add
-		 * callback.  A hotplug notifier will follow and we will handle
-		 * it as CPU online then.  For now, just create the sysfs link,
-		 * unless there is no policy or the link is already present.
-		 */
-		policy = per_cpu(cpufreq_cpu_data, cpu);
-		return policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus)
-			? add_cpu_dev_symlink(policy, cpu) : 0;
-	}
+	pr_debug("%s: bringing CPU%u online\n", __func__, cpu);
 
 	/* Check if this CPU already has a policy to manage it */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
 	if (policy) {
 		WARN_ON(!cpumask_test_cpu(cpu, policy->related_cpus));
-		if (!policy_is_inactive(policy))
-			return cpufreq_add_policy_cpu(policy, cpu);
-
+		if (!policy_is_inactive(policy)) {
+			cpufreq_add_policy_cpu(policy, cpu);
+			return;
+		}
 		/* This is the only online CPU for the policy.  Start over. */
 		recover_policy = true;
 		down_write(&policy->rwsem);
@@ -1239,7 +1215,7 @@ static int cpufreq_add_dev(struct device
 		recover_policy = false;
 		policy = cpufreq_policy_alloc(cpu);
 		if (!policy)
-			return -ENOMEM;
+			return;
 	}
 
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
@@ -1362,7 +1338,7 @@ static int cpufreq_add_dev(struct device
 
 	pr_debug("initialization complete\n");
 
-	return 0;
+	return;
 
 out_remove_policy_notify:
 	/* cpufreq_policy_free() will notify based on this */
@@ -1374,7 +1350,34 @@ out_exit_policy:
 		cpufreq_driver->exit(policy);
 out_free_policy:
 	cpufreq_policy_free(policy, recover_policy);
-	return ret;
+}
+
+/**
+ * cpufreq_add_dev - the cpufreq interface for a CPU device.
+ * @dev: CPU device.
+ * @sif: Subsystem interface structure pointer (not used)
+ */
+static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
+{
+	unsigned cpu = dev->id;
+
+	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
+
+	if (cpu_online(cpu)) {
+		cpufreq_online(cpu);
+	} else {
+		/*
+		 * A hotplug notifier will follow and we will handle it as CPU
+		 * online then.  For now, just create the sysfs link, unless
+		 * there is no policy or the link is already present.
+		 */
+		struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+
+		if (policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus))
+			WARN_ON(add_cpu_dev_symlink(policy, cpu));
+	}
+
+	return 0;
 }
 
 static void cpufreq_offline_prepare(unsigned int cpu)
@@ -2340,27 +2343,23 @@ static int cpufreq_cpu_callback(struct n
 					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_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_online(cpu);
+		break;
 	}
 	return NOTIFY_OK;
 }

Re: [Update][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online

[Viresh Kumar]

On 27-07-15, 23:55, Rafael J. Wysocki wrote:
> +static void cpufreq_online(unsigned int cpu)

As I said in the earlier message, I need the return value of this to
be used for add_dev() callback. Which is required to retry probing the
device if it wasn't ready on a earlier call, i.e. it returns
-EPROBE_DEFER. My other patch already fixes the subsys layer for this.

-- 
viresh

Re: [Update][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online

[Rafael J. Wysocki]

Hi Viresh,

On Tue, Jul 28, 2015 at 4:20 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27-07-15, 23:55, Rafael J. Wysocki wrote:
>> +static void cpufreq_online(unsigned int cpu)
>
> As I said in the earlier message, I need the return value of this to
> be used for add_dev() callback. Which is required to retry probing the
> device if it wasn't ready on a earlier call, i.e. it returns
> -EPROBE_DEFER. My other patch already fixes the subsys layer for this.

If this is the patch I've looked at, it doesn't fix this.  It only
fixes one case.

It also doesn't fix the hotplug notifier failure case.

I can retain the return value, but we need to be consistent about using it.

Thanks,
Rafael

[Update 2x][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online

[Rafael J. Wysocki]

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

To separate the CPU online interface from the CPU device
registration, split cpufreq_online() out of cpufreq_add_dev()
and make cpufreq_cpu_callback() call the former, while
cpufreq_add_dev() itself will only be used as the CPU device
addition subsystem interface callback.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Suggested-by: Russell King <linux@arm.linux.org.uk>
---

cpufreq_online() returns an int and cpufreq_add_dev() propagates that to the
caller in this version.

That said I don't agree with using that to defer the platform device probing
in cpufreq-dt.  That's just over the top to me.

---
 drivers/cpufreq/cpufreq.c |   96 +++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 46 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1191,36 +1191,15 @@ 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_online(unsigned int cpu)
 {
-	unsigned int j, cpu = dev->id;
-	int ret;
 	struct cpufreq_policy *policy;
-	unsigned long flags;
 	bool recover_policy;
+	unsigned long flags;
+	unsigned int j;
+	int ret;
 
-	pr_debug("adding CPU %u\n", cpu);
-
-	if (cpu_is_offline(cpu)) {
-		/*
-		 * Only possible if we are here from the subsys_interface add
-		 * callback.  A hotplug notifier will follow and we will handle
-		 * it as CPU online then.  For now, just create the sysfs link,
-		 * unless there is no policy or the link is already present.
-		 */
-		policy = per_cpu(cpufreq_cpu_data, cpu);
-		return policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus)
-			? add_cpu_dev_symlink(policy, cpu) : 0;
-	}
+	pr_debug("%s: bringing CPU%u online\n", __func__, cpu);
 
 	/* Check if this CPU already has a policy to manage it */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
@@ -1377,6 +1356,35 @@ out_free_policy:
 	return ret;
 }
 
+/**
+ * cpufreq_add_dev - the cpufreq interface for a CPU device.
+ * @dev: CPU device.
+ * @sif: Subsystem interface structure pointer (not used)
+ */
+static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
+{
+	unsigned cpu = dev->id;
+	int ret;
+
+	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
+
+	if (cpu_online(cpu)) {
+		ret = cpufreq_online(cpu);
+	} else {
+		/*
+		 * A hotplug notifier will follow and we will handle it as CPU
+		 * online then.  For now, just create the sysfs link, unless
+		 * there is no policy or the link is already present.
+		 */
+		struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+
+		ret = policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus)
+			? add_cpu_dev_symlink(policy, cpu) : 0;
+	}
+
+	return ret;
+}
+
 static void cpufreq_offline_prepare(unsigned int cpu)
 {
 	struct cpufreq_policy *policy;
@@ -2340,27 +2348,23 @@ static int cpufreq_cpu_callback(struct n
 					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_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_online(cpu);
+		break;
 	}
 	return NOTIFY_OK;
 }

[PATCH] cpufreq: Replace recover_policy with new_policy in cpufreq_online()

[Rafael J. Wysocki]

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

The recover_policy is unsed in cpufreq_online() to indicate whether
a new policy object is created or an existing one is reinitialized.

The "recover" part of the name is slightly confusing (it should be
"reinitialization" rather than "recovery") and the logical not (!)
operator is applied to it in almost all of the checks it is used in,
so replace that variable with a new one called "new_policy" that
will be true in the case of a new policy creation.

While at it, drop one of the labels that is jumped to from only
one spot.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

One extra cleanup on top of https://patchwork.kernel.org/patch/6888751/

---
 drivers/cpufreq/cpufreq.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1194,7 +1194,7 @@ static void cpufreq_policy_free(struct c
 static int cpufreq_online(unsigned int cpu)
 {
 	struct cpufreq_policy *policy;
-	bool recover_policy;
+	bool new_policy;
 	unsigned long flags;
 	unsigned int j;
 	int ret;
@@ -1209,13 +1209,13 @@ static int cpufreq_online(unsigned int c
 			return cpufreq_add_policy_cpu(policy, cpu);
 
 		/* This is the only online CPU for the policy.  Start over. */
-		recover_policy = true;
+		new_policy = false;
 		down_write(&policy->rwsem);
 		policy->cpu = cpu;
 		policy->governor = NULL;
 		up_write(&policy->rwsem);
 	} else {
-		recover_policy = false;
+		new_policy = true;
 		policy = cpufreq_policy_alloc(cpu);
 		if (!policy)
 			return -ENOMEM;
@@ -1234,7 +1234,7 @@ static int cpufreq_online(unsigned int c
 
 	down_write(&policy->rwsem);
 
-	if (!recover_policy) {
+	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
 		/* Remember CPUs present at the policy creation time. */
@@ -1247,7 +1247,7 @@ static int cpufreq_online(unsigned int c
 	 */
 	cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
 
-	if (!recover_policy) {
+	if (new_policy) {
 		policy->user_policy.min = policy->min;
 		policy->user_policy.max = policy->max;
 
@@ -1308,7 +1308,7 @@ static int cpufreq_online(unsigned int c
 	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
 				     CPUFREQ_START, policy);
 
-	if (!recover_policy) {
+	if (new_policy) {
 		ret = cpufreq_add_dev_interface(policy);
 		if (ret)
 			goto out_exit_policy;
@@ -1324,10 +1324,12 @@ static int cpufreq_online(unsigned int c
 	if (ret) {
 		pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n",
 		       __func__, cpu, ret);
-		goto out_remove_policy_notify;
+		/* cpufreq_policy_free() will notify based on this */
+		new_policy = false;
+		goto out_exit_policy;
 	}
 
-	if (!recover_policy) {
+	if (new_policy) {
 		policy->user_policy.policy = policy->policy;
 		policy->user_policy.governor = policy->governor;
 	}
@@ -1343,16 +1345,13 @@ static int cpufreq_online(unsigned int c
 
 	return 0;
 
-out_remove_policy_notify:
-	/* cpufreq_policy_free() will notify based on this */
-	recover_policy = true;
 out_exit_policy:
 	up_write(&policy->rwsem);
 
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 out_free_policy:
-	cpufreq_policy_free(policy, recover_policy);
+	cpufreq_policy_free(policy, !new_policy);
 	return ret;
 }
 

Re: [PATCH] cpufreq: Replace recover_policy with new_policy in cpufreq_online()

[Viresh Kumar]

On 29-07-15, 03:08, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The recover_policy is unsed in cpufreq_online() to indicate whether
> a new policy object is created or an existing one is reinitialized.
> 
> The "recover" part of the name is slightly confusing (it should be
> "reinitialization" rather than "recovery") and the logical not (!)
> operator is applied to it in almost all of the checks it is used in,
> so replace that variable with a new one called "new_policy" that
> will be true in the case of a new policy creation.
> 
> While at it, drop one of the labels that is jumped to from only
> one spot.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> One extra cleanup on top of https://patchwork.kernel.org/patch/6888751/
> 
> ---
>  drivers/cpufreq/cpufreq.c |   23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)

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

-- 
viresh

Re: [Update 2x][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online

[Viresh Kumar]

On 29-07-15, 03:03, Rafael J. Wysocki wrote:
> +static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> +{
> +	unsigned cpu = dev->id;
> +	int ret;
> +
> +	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
> +
> +	if (cpu_online(cpu)) {
> +		ret = cpufreq_online(cpu);

I will do return right here ...

> +	} else {

... and this else will not be required anymore.

> +		/*
> +		 * A hotplug notifier will follow and we will handle it as CPU
> +		 * online then.  For now, just create the sysfs link, unless
> +		 * there is no policy or the link is already present.
> +		 */
> +		struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> +
> +		ret = policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus)
> +			? add_cpu_dev_symlink(policy, cpu) : 0;
> +	}
> +
> +	return ret;
> +}

Looks good otherwise.

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

-- 
viresh

Re: [Update 2x][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online

[Rafael J. Wysocki]

Hi Viresh,

On Wed, Jul 29, 2015 at 7:32 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 29-07-15, 03:03, Rafael J. Wysocki wrote:
>> +static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>> +{
>> +     unsigned cpu = dev->id;
>> +     int ret;
>> +
>> +     dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
>> +
>> +     if (cpu_online(cpu)) {
>> +             ret = cpufreq_online(cpu);
>
> I will do return right here ...
>
>> +     } else {
>
> ... and this else will not be required anymore.

No.

You'd still need policy, so its definition and initialization would stay there.

The only thing you can save by doing that change is the ret variable,
but I like the code more the way it is.

>> +             /*
>> +              * A hotplug notifier will follow and we will handle it as CPU
>> +              * online then.  For now, just create the sysfs link, unless
>> +              * there is no policy or the link is already present.
>> +              */
>> +             struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
>> +
>> +             ret = policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus)
>> +                     ? add_cpu_dev_symlink(policy, cpu) : 0;
>> +     }
>> +
>> +     return ret;
>> +}
>
> Looks good otherwise.
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks,
Rafael

Re: [Update 2x][PATCH 7/7] cpufreq: Separate CPU device registration from CPU online

[Viresh Kumar]

On 29-07-15, 16:02, Rafael J. Wysocki wrote:
> You'd still need policy, so its definition and initialization would stay there.
> 
> The only thing you can save by doing that change is the ret variable,
> but I like the code more the way it is.

Okay, that's fine.

-- 
viresh