[PATCH V7 0/6] cpufreq: Don't loose cpufreq history on CPU hotplug

[PATCH V7 0/6] cpufreq: Don't loose cpufreq history on CPU hotplug

[Viresh Kumar]

Hi Rafael,

The aim of this series is to stop managing cpufreq sysfs directories on CPU
hotplug.

Currently on removal of a 'cpu != policy->cpu', we remove its sysfs directories
by removing the soft-link. And on removal of policy->cpu, we migrate the sysfs
directories to the next cpu in policy. But if policy->cpu was the last CPU, we
remove the policy completely and allocate it again once the CPUs come back.

This has shortcomings:

- Code Complexity
- Slower hotplug
- sysfs file permissions are reset after all policy->cpus are offlined
- CPUFreq stats history lost after all policy->cpus are offlined
- Special management of sysfs stuff during suspend/resume


To make things simple we can stop playing with sysfs files unless the driver is
getting removed. Also the policy can be kept intact to be used later.

First few patches provide a clean base for others *more important* patches.

V6->V7:
- Change the order of patches, "cpufreq: Remove cpufreq_update_policy"
  is moved to the end of the series and updated its changelog.

V5-V6:
- Merged v4 9/14 into 10/14 after some updates.
- Keep separate routines to add/remove symlinks.
- Only updated one patch in this version

V4-V5:
- Sending all patches again
- Fixed comments in one of the patches as suggested by you
- Merged the last commit about "physical hotplug of CPUs" with "cpufreq: Stop
  migrating sysfs files on hotplug".
- A new patch (content is old, just separated out into its own patch) "cpufreq:
  add/remove sysfs links via cpufreq_add_remove_dev_symlink()"

V3-V4:
- Only four patches sent this time:
  - [PATCH V4 01/14] cpufreq: Create for_each_{in}active_policy()
  - [PATCH V4 04/14] cpufreq: Don't traverse all active policies to find
  - [PATCH V4 05/14] cpufreq: Manage governor usage history with
  - [PATCH V4 06/14] cpufreq: Mark policy->governor = NULL for inactive
- Remove __temp from the arguments of for_each_[in]active_policies.
- Simplified macros/next_policy, etc.

V2->V3:
- First five are already applied by you and the 7th one was sent separately as a
  fix earlier and got applied. So, V2 had 20 patches and V3 has 14.
- Dropped while(1) and used do/while.
- policy->governor marked as NULL only while removing the governor and not when
  policy becomes inactive.
- Commit logs/comments updated
- Applied Acks from Saravan

v1->V2:
- Dropped the idea of using policy-lists for getting policy for any cpu
- Also dropped fallback list and its per-cpu variable
- Stopped cleaning cpufreq_cpu_data and doing list_del(policy) on logical
  hotplug.
- Added support for physical hotplug of CPUs (Untested).

Viresh Kumar (6):
  cpufreq: Don't allow updating inactive policies from sysfs
  cpufreq: Stop migrating sysfs files on hotplug
  cpufreq: Initialize policy->kobj while allocating policy
  cpufreq: Call cpufreq_policy_put_kobj() from cpufreq_policy_free()
  cpufreq: Restart governor as soon as possible
  cpufreq: Remove cpufreq_update_policy()

 drivers/cpufreq/cpufreq.c | 303 +++++++++++++++++++++++++---------------------
 1 file changed, 168 insertions(+), 135 deletions(-)

-- 
2.4.0

[PATCH V7 1/6] cpufreq: Don't allow updating inactive policies from sysfs

[Viresh Kumar]

Later commits would change the way policies are managed today. Policies
wouldn't be freed on cpu hotplug (currently they aren't freed only for
suspend), and while the CPU is offline, the sysfs cpufreq files would
still be present.

User may accidentally try to update the sysfs files in following
directory: '/sys/devices/system/cpu/cpuX/cpufreq/'. And that would
result in undefined behavior as policy wouldn't be active then.

Apart from updating the store() routine, we also update __cpufreq_get()
which can call cpufreq_out_of_sync(). The later routine tries to update
policy->cur and starts notifying kernel about it.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5d780ff5a10a..7eeff892c0a6 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -875,11 +875,18 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 
 	down_write(&policy->rwsem);
 
+	/* Updating inactive policies is invalid, so avoid doing that. */
+	if (unlikely(policy_is_inactive(policy))) {
+		ret = -EPERM;
+		goto unlock_policy_rwsem;
+	}
+
 	if (fattr->store)
 		ret = fattr->store(policy, buf, count);
 	else
 		ret = -EIO;
 
+unlock_policy_rwsem:
 	up_write(&policy->rwsem);
 
 	up_read(&cpufreq_rwsem);
@@ -1610,6 +1617,10 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
 
 	ret_freq = cpufreq_driver->get(policy->cpu);
 
+	/* Updating inactive policies is invalid, so avoid doing that. */
+	if (unlikely(policy_is_inactive(policy)))
+		return ret_freq;
+
 	if (ret_freq && policy->cur &&
 		!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
 		/* verify no discrepancy between actual and
-- 
2.4.0

Re: [PATCH V7 1/6] cpufreq: Don't allow updating inactive policies from sysfs

[Rafael J. Wysocki]

On Monday, June 08, 2015 06:25:27 PM Viresh Kumar wrote:
> Later commits would change the way policies are managed today. Policies
> wouldn't be freed on cpu hotplug (currently they aren't freed only for
> suspend), and while the CPU is offline, the sysfs cpufreq files would
> still be present.
> 
> User may accidentally try to update the sysfs files in following
> directory: '/sys/devices/system/cpu/cpuX/cpufreq/'. And that would
> result in undefined behavior as policy wouldn't be active then.
> 
> Apart from updating the store() routine, we also update __cpufreq_get()
> which can call cpufreq_out_of_sync(). The later routine tries to update
> policy->cur and starts notifying kernel about it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5d780ff5a10a..7eeff892c0a6 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -875,11 +875,18 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>  
>  	down_write(&policy->rwsem);
>  
> +	/* Updating inactive policies is invalid, so avoid doing that. */
> +	if (unlikely(policy_is_inactive(policy))) {
> +		ret = -EPERM;

I'm not sure if -EPERM is the best error code for this case.  It doesn't depend
on the actual permissions, but rather on the policy status that may change
going forward.

It might be better to use -EBUSY even.

> +		goto unlock_policy_rwsem;
> +	}
> +
>  	if (fattr->store)
>  		ret = fattr->store(policy, buf, count);
>  	else
>  		ret = -EIO;
>  
> +unlock_policy_rwsem:
>  	up_write(&policy->rwsem);
>  
>  	up_read(&cpufreq_rwsem);
> @@ -1610,6 +1617,10 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
>  
>  	ret_freq = cpufreq_driver->get(policy->cpu);
>  
> +	/* Updating inactive policies is invalid, so avoid doing that. */
> +	if (unlikely(policy_is_inactive(policy)))
> +		return ret_freq;
> +
>  	if (ret_freq && policy->cur &&
>  		!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
>  		/* verify no discrepancy between actual and
> 

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

Re: [PATCH V7 1/6] cpufreq: Don't allow updating inactive policies from sysfs

[Viresh Kumar]

On 09-06-15, 01:19, Rafael J. Wysocki wrote:
> I'm not sure if -EPERM is the best error code for this case.  It doesn't depend
> on the actual permissions, but rather on the policy status that may change
> going forward.
> 
> It might be better to use -EBUSY even.

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 27 Jan 2015 13:22:23 +0530
Subject: [PATCH] cpufreq: Don't allow updating inactive policies from sysfs

Later commits would change the way policies are managed today. Policies
wouldn't be freed on cpu hotplug (currently they aren't freed only for
suspend), and while the CPU is offline, the sysfs cpufreq files would
still be present.

User may accidentally try to update the sysfs files in following
directory: '/sys/devices/system/cpu/cpuX/cpufreq/'. And that would
result in undefined behavior as policy wouldn't be active then.

Apart from updating the store() routine, we also update __cpufreq_get()
which can call cpufreq_out_of_sync(). The later routine tries to update
policy->cur and starts notifying kernel about it.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5d780ff5a10a..16214dfe78c5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -875,11 +875,18 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 
 	down_write(&policy->rwsem);
 
+	/* Updating inactive policies is invalid, so avoid doing that. */
+	if (unlikely(policy_is_inactive(policy))) {
+		ret = -EBUSY;
+		goto unlock_policy_rwsem;
+	}
+
 	if (fattr->store)
 		ret = fattr->store(policy, buf, count);
 	else
 		ret = -EIO;
 
+unlock_policy_rwsem:
 	up_write(&policy->rwsem);
 
 	up_read(&cpufreq_rwsem);
@@ -1610,6 +1617,10 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
 
 	ret_freq = cpufreq_driver->get(policy->cpu);
 
+	/* Updating inactive policies is invalid, so avoid doing that. */
+	if (unlikely(policy_is_inactive(policy)))
+		return ret_freq;
+
 	if (ret_freq && policy->cur &&
 		!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
 		/* verify no discrepancy between actual and


-- 
viresh

Re: [PATCH V7 1/6] cpufreq: Don't allow updating inactive policies from sysfs

[Saravana Kannan]

On 06/08/2015 05:55 AM, Viresh Kumar wrote:
> Later commits would change the way policies are managed today. Policies
> wouldn't be freed on cpu hotplug (currently they aren't freed only for
> suspend), and while the CPU is offline, the sysfs cpufreq files would
> still be present.
>
> User may accidentally try to update the sysfs files in following
> directory: '/sys/devices/system/cpu/cpuX/cpufreq/'. And that would
> result in undefined behavior as policy wouldn't be active then.
>
> Apart from updating the store() routine, we also update __cpufreq_get()
> which can call cpufreq_out_of_sync(). The later routine tries to update
> policy->cur and starts notifying kernel about it.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/cpufreq.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5d780ff5a10a..7eeff892c0a6 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -875,11 +875,18 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>
>   	down_write(&policy->rwsem);
>
> +	/* Updating inactive policies is invalid, so avoid doing that. */
> +	if (unlikely(policy_is_inactive(policy))) {
> +		ret = -EPERM;
> +		goto unlock_policy_rwsem;
> +	}
> +
>   	if (fattr->store)
>   		ret = fattr->store(policy, buf, count);
>   	else
>   		ret = -EIO;
>
> +unlock_policy_rwsem:
>   	up_write(&policy->rwsem);
>
>   	up_read(&cpufreq_rwsem);
> @@ -1610,6 +1617,10 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
>
>   	ret_freq = cpufreq_driver->get(policy->cpu);
>
> +	/* Updating inactive policies is invalid, so avoid doing that. */
> +	if (unlikely(policy_is_inactive(policy)))
> +		return ret_freq;
> +
>   	if (ret_freq && policy->cur &&
>   		!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
>   		/* verify no discrepancy between actual and
>

Acked-by: Saravana Kannan <skannan@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH V7 2/6] cpufreq: Stop migrating sysfs files on hotplug

[Viresh Kumar]

When we hot-unplug a cpu, we remove its sysfs cpufreq directory and if
the outgoing cpu was the owner of policy->kobj earlier then we migrate
the sysfs directory to under another online cpu.

There are few disadvantages this brings:
- Code Complexity
- Slower hotplug/suspend/resume
- sysfs file permissions are reset after all policy->cpus are offlined
- CPUFreq stats history lost after all policy->cpus are offlined
- Special management of sysfs stuff during suspend/resume

To overcome these, this patch modifies the way sysfs directories are
managed:
- Select sysfs kobjects owner while initializing policy and don't change
  it during hotplugs. Track it with kobj_cpu created earlier.

- Create symlinks for all related CPUs (can be offline) instead of
  affected CPUs on policy initialization and remove them only when the
  policy is freed.

- Free policy structure only on the removal of cpufreq-driver and not
  during hotplug/suspend/resume, detected by checking 'struct
  subsys_interface *' (Valid only when called from
  subsys_interface_unregister() while unregistering driver).

Apart from this, special care is taken to handle physical hoplug of CPUs
as we wouldn't remove sysfs links or remove policies on logical
hotplugs. Physical hotplug happens in the following sequence.

Hot removal:
- CPU is offlined first, ~ 'echo 0 >
  /sys/devices/system/cpu/cpuX/online'
- Then its device is removed along with all sysfs files, cpufreq core
  notified with cpufreq_remove_dev() callback from subsys-interface..

Hot addition:
- First the device along with its sysfs files is added, cpufreq core
  notified with cpufreq_add_dev() callback from subsys-interface..
- CPU is onlined, ~ 'echo 1 > /sys/devices/system/cpu/cpuX/online'

We call the same routines with both hotplug and subsys callbacks, and we
sense physical hotplug with cpu_offline() check in subsys callback. We
can handle most of the stuff with regular hotplug callback paths and
add/remove cpufreq sysfs links or free policy from subsys callbacks.

[ Something similar attempted by Saravana earlier ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 223 ++++++++++++++++++++++++++++------------------
 1 file changed, 138 insertions(+), 85 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7eeff892c0a6..663a934259a4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -957,28 +957,64 @@ void cpufreq_sysfs_remove_file(const struct attribute *attr)
 }
 EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
 
-/* symlink affected CPUs */
+static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+{
+	struct device *cpu_dev;
+
+	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
+
+	cpu_dev = get_cpu_device(cpu);
+	if (WARN_ON(!cpu_dev))
+		return 0;
+
+	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+}
+
+static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+{
+	struct device *cpu_dev;
+
+	pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu);
+
+	cpu_dev = get_cpu_device(cpu);
+	if (WARN_ON(!cpu_dev))
+		return;
+
+	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
+}
+
+/* Add/remove symlinks for all related CPUs */
 static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
 {
 	unsigned int j;
 	int ret = 0;
 
-	for_each_cpu(j, policy->cpus) {
-		struct device *cpu_dev;
-
+	/* Some related CPUs might not be present (physically hotplugged) */
+	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
 		if (j == policy->kobj_cpu)
 			continue;
 
-		pr_debug("Adding link for CPU: %u\n", j);
-		cpu_dev = get_cpu_device(j);
-		ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
-					"cpufreq");
+		ret = add_cpu_dev_symlink(policy, j);
 		if (ret)
 			break;
 	}
+
 	return ret;
 }
 
+static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
+{
+	unsigned int j;
+
+	/* Some related CPUs might not be present (physically hotplugged) */
+	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
+		if (j == policy->kobj_cpu)
+			continue;
+
+		remove_cpu_dev_symlink(policy, j);
+	}
+}
+
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
 				     struct device *dev)
 {
@@ -1075,7 +1111,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
 		}
 	}
 
-	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+	return 0;
 }
 
 static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
@@ -1095,7 +1131,7 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
 	return policy;
 }
 
-static struct cpufreq_policy *cpufreq_policy_alloc(void)
+static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)
 {
 	struct cpufreq_policy *policy;
 
@@ -1116,6 +1152,11 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
+	policy->cpu = cpu;
+
+	/* Set this once on allocation */
+	policy->kobj_cpu = cpu;
+
 	return policy;
 
 err_free_cpumask:
@@ -1134,10 +1175,11 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
 	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
 			CPUFREQ_REMOVE_POLICY, policy);
 
-	down_read(&policy->rwsem);
+	down_write(&policy->rwsem);
+	cpufreq_remove_dev_symlink(policy);
 	kobj = &policy->kobj;
 	cmp = &policy->kobj_unregister;
-	up_read(&policy->rwsem);
+	up_write(&policy->rwsem);
 	kobject_put(kobj);
 
 	/*
@@ -1168,27 +1210,14 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 	kfree(policy);
 }
 
-static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
-			     struct device *cpu_dev)
+static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
-	int ret;
-
 	if (WARN_ON(cpu == policy->cpu))
-		return 0;
-
-	/* Move kobject to the new policy->cpu */
-	ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
-	if (ret) {
-		pr_err("%s: Failed to move kobj: %d\n", __func__, ret);
-		return ret;
-	}
+		return;
 
 	down_write(&policy->rwsem);
 	policy->cpu = cpu;
-	policy->kobj_cpu = cpu;
 	up_write(&policy->rwsem);
-
-	return 0;
 }
 
 /**
@@ -1206,13 +1235,25 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	int ret = -ENOMEM;
 	struct cpufreq_policy *policy;
 	unsigned long flags;
-	bool recover_policy = cpufreq_suspended;
-
-	if (cpu_is_offline(cpu))
-		return 0;
+	bool recover_policy = !sif;
 
 	pr_debug("adding CPU %u\n", cpu);
 
+	/*
+	 * Only possible if 'cpu' wasn't physically present earlier and we are
+	 * here from subsys_interface add callback. A hotplug notifier will
+	 * follow and we will handle it like logical CPU hotplug then. For now,
+	 * just create the sysfs link.
+	 */
+	if (cpu_is_offline(cpu)) {
+		policy = per_cpu(cpufreq_cpu_data, cpu);
+		/* No need to create link of the first cpu of a policy */
+		if (!policy)
+			return 0;
+
+		return add_cpu_dev_symlink(policy, cpu);
+	}
+
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
 
@@ -1232,7 +1273,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	policy = recover_policy ? cpufreq_policy_restore(cpu) : NULL;
 	if (!policy) {
 		recover_policy = false;
-		policy = cpufreq_policy_alloc();
+		policy = cpufreq_policy_alloc(cpu);
 		if (!policy)
 			goto nomem_out;
 	}
@@ -1243,12 +1284,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	 * the creation of a brand new one. So we need to perform this update
 	 * by invoking update_policy_cpu().
 	 */
-	if (recover_policy && cpu != policy->cpu) {
-		WARN_ON(update_policy_cpu(policy, cpu, dev));
-	} else {
-		policy->cpu = cpu;
-		policy->kobj_cpu = cpu;
-	}
+	if (recover_policy && cpu != policy->cpu)
+		update_policy_cpu(policy, cpu);
 
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
@@ -1427,29 +1464,14 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 			CPUFREQ_NAME_LEN);
 	up_write(&policy->rwsem);
 
-	if (cpu != policy->kobj_cpu) {
-		sysfs_remove_link(&dev->kobj, "cpufreq");
-	} else if (cpus > 1) {
-		/* Nominate new CPU */
-		int new_cpu = cpumask_any_but(policy->cpus, cpu);
-		struct device *cpu_dev = get_cpu_device(new_cpu);
-
-		sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
-		ret = update_policy_cpu(policy, new_cpu, cpu_dev);
-		if (ret) {
-			if (sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
-					      "cpufreq"))
-				pr_err("%s: Failed to restore kobj link to cpu:%d\n",
-				       __func__, cpu_dev->id);
-			return ret;
-		}
+	if (cpu != policy->cpu)
+		return 0;
 
-		if (!cpufreq_suspended)
-			pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
-				 __func__, new_cpu, cpu);
-	} else if (cpufreq_driver->stop_cpu) {
+	if (cpus > 1)
+		/* Nominate new CPU */
+		update_policy_cpu(policy, cpumask_any_but(policy->cpus, cpu));
+	else if (cpufreq_driver->stop_cpu)
 		cpufreq_driver->stop_cpu(policy);
-	}
 
 	return 0;
 }
@@ -1470,32 +1492,11 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 	cpumask_clear_cpu(cpu, policy->cpus);
 	up_write(&policy->rwsem);
 
-	/* If cpu is last user of policy, free policy */
-	if (policy_is_inactive(policy)) {
-		if (has_target()) {
-			ret = __cpufreq_governor(policy,
-					CPUFREQ_GOV_POLICY_EXIT);
-			if (ret) {
-				pr_err("%s: Failed to exit governor\n",
-				       __func__);
-				return ret;
-			}
-		}
-
-		if (!cpufreq_suspended)
-			cpufreq_policy_put_kobj(policy);
+	/* Not the last cpu of policy, start governor again ? */
+	if (!policy_is_inactive(policy)) {
+		if (!has_target())
+			return 0;
 
-		/*
-		 * Perform the ->exit() even during light-weight tear-down,
-		 * since this is a core component, and is essential for the
-		 * subsequent light-weight ->init() to succeed.
-		 */
-		if (cpufreq_driver->exit)
-			cpufreq_driver->exit(policy);
-
-		if (!cpufreq_suspended)
-			cpufreq_policy_free(policy);
-	} else if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
 		if (!ret)
 			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
@@ -1504,8 +1505,34 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 			pr_err("%s: Failed to start governor\n", __func__);
 			return ret;
 		}
+
+		return 0;
 	}
 
+	/* If cpu is last user of policy, free policy */
+	if (has_target()) {
+		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+		if (ret) {
+			pr_err("%s: Failed to exit governor\n", __func__);
+			return ret;
+		}
+	}
+
+	/* Free the policy kobjects only if the driver is getting removed. */
+	if (sif)
+		cpufreq_policy_put_kobj(policy);
+
+	/*
+	 * Perform the ->exit() even during light-weight tear-down,
+	 * since this is a core component, and is essential for the
+	 * subsequent light-weight ->init() to succeed.
+	 */
+	if (cpufreq_driver->exit)
+		cpufreq_driver->exit(policy);
+
+	if (sif)
+		cpufreq_policy_free(policy);
+
 	return 0;
 }
 
@@ -1519,8 +1546,34 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	unsigned int cpu = dev->id;
 	int ret;
 
-	if (cpu_is_offline(cpu))
+	/*
+	 * Only possible if 'cpu' is getting physically removed now. A hotplug
+	 * notifier should have already been called and we just need to remove
+	 * link or free policy here.
+	 */
+	if (cpu_is_offline(cpu)) {
+		struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+		struct cpumask mask;
+
+		if (!policy)
+			return 0;
+
+		cpumask_copy(&mask, policy->related_cpus);
+		cpumask_clear_cpu(cpu, &mask);
+
+		/*
+		 * Free policy only if all policy->related_cpus are removed
+		 * physically.
+		 */
+		if (cpumask_intersects(&mask, cpu_present_mask)) {
+			remove_cpu_dev_symlink(policy, cpu);
+			return 0;
+		}
+
+		cpufreq_policy_put_kobj(policy);
+		cpufreq_policy_free(policy);
 		return 0;
+	}
 
 	ret = __cpufreq_remove_dev_prepare(dev, sif);
 
-- 
2.4.0

Re: [PATCH V7 2/6] cpufreq: Stop migrating sysfs files on hotplug

[Rafael J. Wysocki]

On Monday, June 08, 2015 06:25:28 PM Viresh Kumar wrote:
> When we hot-unplug a cpu, we remove its sysfs cpufreq directory and if
> the outgoing cpu was the owner of policy->kobj earlier then we migrate
> the sysfs directory to under another online cpu.
> 
> There are few disadvantages this brings:
> - Code Complexity
> - Slower hotplug/suspend/resume
> - sysfs file permissions are reset after all policy->cpus are offlined
> - CPUFreq stats history lost after all policy->cpus are offlined
> - Special management of sysfs stuff during suspend/resume
> 
> To overcome these, this patch modifies the way sysfs directories are
> managed:
> - Select sysfs kobjects owner while initializing policy and don't change
>   it during hotplugs. Track it with kobj_cpu created earlier.
> 
> - Create symlinks for all related CPUs (can be offline) instead of
>   affected CPUs on policy initialization and remove them only when the
>   policy is freed.
> 
> - Free policy structure only on the removal of cpufreq-driver and not
>   during hotplug/suspend/resume, detected by checking 'struct
>   subsys_interface *' (Valid only when called from
>   subsys_interface_unregister() while unregistering driver).
> 
> Apart from this, special care is taken to handle physical hoplug of CPUs
> as we wouldn't remove sysfs links or remove policies on logical
> hotplugs. Physical hotplug happens in the following sequence.
> 
> Hot removal:
> - CPU is offlined first, ~ 'echo 0 >
>   /sys/devices/system/cpu/cpuX/online'
> - Then its device is removed along with all sysfs files, cpufreq core
>   notified with cpufreq_remove_dev() callback from subsys-interface..
> 
> Hot addition:
> - First the device along with its sysfs files is added, cpufreq core
>   notified with cpufreq_add_dev() callback from subsys-interface..
> - CPU is onlined, ~ 'echo 1 > /sys/devices/system/cpu/cpuX/online'
> 
> We call the same routines with both hotplug and subsys callbacks, and we
> sense physical hotplug with cpu_offline() check in subsys callback. We
> can handle most of the stuff with regular hotplug callback paths and
> add/remove cpufreq sysfs links or free policy from subsys callbacks.
> 
> [ Something similar attempted by Saravana earlier ]
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 223 ++++++++++++++++++++++++++++------------------
>  1 file changed, 138 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7eeff892c0a6..663a934259a4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -957,28 +957,64 @@ void cpufreq_sysfs_remove_file(const struct attribute *attr)
>  }
>  EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
>  
> -/* symlink affected CPUs */
> +static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
> +{
> +	struct device *cpu_dev;
> +

If you added

	if (!policy)
		return 0;

here, you could save some lines of code below. ->


> +	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
> +
> +	cpu_dev = get_cpu_device(cpu);
> +	if (WARN_ON(!cpu_dev))
> +		return 0;
> +
> +	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
> +}
> +
> +static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
> +{
> +	struct device *cpu_dev;
> +
> +	pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu);
> +
> +	cpu_dev = get_cpu_device(cpu);
> +	if (WARN_ON(!cpu_dev))
> +		return;
> +
> +	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
> +}
> +
> +/* Add/remove symlinks for all related CPUs */
>  static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
>  {
>  	unsigned int j;
>  	int ret = 0;
>  
> -	for_each_cpu(j, policy->cpus) {
> -		struct device *cpu_dev;
> -
> +	/* Some related CPUs might not be present (physically hotplugged) */
> +	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
>  		if (j == policy->kobj_cpu)
>  			continue;
>  
> -		pr_debug("Adding link for CPU: %u\n", j);
> -		cpu_dev = get_cpu_device(j);
> -		ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
> -					"cpufreq");
> +		ret = add_cpu_dev_symlink(policy, j);
>  		if (ret)
>  			break;
>  	}
> +
>  	return ret;
>  }
>  
> +static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
> +{
> +	unsigned int j;
> +
> +	/* Some related CPUs might not be present (physically hotplugged) */
> +	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
> +		if (j == policy->kobj_cpu)
> +			continue;
> +
> +		remove_cpu_dev_symlink(policy, j);
> +	}
> +}
> +
>  static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
>  				     struct device *dev)
>  {
> @@ -1075,7 +1111,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
>  		}
>  	}
>  
> -	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> +	return 0;
>  }
>  
>  static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
> @@ -1095,7 +1131,7 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
>  	return policy;
>  }
>  
> -static struct cpufreq_policy *cpufreq_policy_alloc(void)
> +static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)
>  {
>  	struct cpufreq_policy *policy;
>  
> @@ -1116,6 +1152,11 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
>  	init_completion(&policy->kobj_unregister);
>  	INIT_WORK(&policy->update, handle_update);
>  
> +	policy->cpu = cpu;
> +
> +	/* Set this once on allocation */
> +	policy->kobj_cpu = cpu;
> +
>  	return policy;
>  
>  err_free_cpumask:
> @@ -1134,10 +1175,11 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
>  	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>  			CPUFREQ_REMOVE_POLICY, policy);
>  
> -	down_read(&policy->rwsem);
> +	down_write(&policy->rwsem);
> +	cpufreq_remove_dev_symlink(policy);
>  	kobj = &policy->kobj;
>  	cmp = &policy->kobj_unregister;
> -	up_read(&policy->rwsem);
> +	up_write(&policy->rwsem);
>  	kobject_put(kobj);
>  
>  	/*
> @@ -1168,27 +1210,14 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>  	kfree(policy);
>  }
>  
> -static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
> -			     struct device *cpu_dev)
> +static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
>  {
> -	int ret;
> -
>  	if (WARN_ON(cpu == policy->cpu))
> -		return 0;
> -
> -	/* Move kobject to the new policy->cpu */
> -	ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
> -	if (ret) {
> -		pr_err("%s: Failed to move kobj: %d\n", __func__, ret);
> -		return ret;
> -	}
> +		return;
>  
>  	down_write(&policy->rwsem);
>  	policy->cpu = cpu;
> -	policy->kobj_cpu = cpu;
>  	up_write(&policy->rwsem);
> -
> -	return 0;
>  }
>  
>  /**
> @@ -1206,13 +1235,25 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>  	int ret = -ENOMEM;
>  	struct cpufreq_policy *policy;
>  	unsigned long flags;
> -	bool recover_policy = cpufreq_suspended;
> -
> -	if (cpu_is_offline(cpu))
> -		return 0;
> +	bool recover_policy = !sif;
>  
>  	pr_debug("adding CPU %u\n", cpu);
>  
> +	/*
> +	 * Only possible if 'cpu' wasn't physically present earlier and we are
> +	 * here from subsys_interface add callback. A hotplug notifier will
> +	 * follow and we will handle it like logical CPU hotplug then. For now,
> +	 * just create the sysfs link.
> +	 */
> +	if (cpu_is_offline(cpu)) {
> +		policy = per_cpu(cpufreq_cpu_data, cpu);
> +		/* No need to create link of the first cpu of a policy */
> +		if (!policy)
> +			return 0;
> +
> +		return add_cpu_dev_symlink(policy, cpu);

-> If add_cpu_dev_symlink() did the !policy check, the code above could be
rewritten as

	if (cpu_is_offline(cpu)) 
		return add_cpu_dev_symlink(per_cpu(cpufreq_cpu_data, cpu), cpu);

> +	}
> +
>  	if (!down_read_trylock(&cpufreq_rwsem))
>  		return 0;
>  
> @@ -1232,7 +1273,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>  	policy = recover_policy ? cpufreq_policy_restore(cpu) : NULL;
>  	if (!policy) {
>  		recover_policy = false;
> -		policy = cpufreq_policy_alloc();
> +		policy = cpufreq_policy_alloc(cpu);
>  		if (!policy)
>  			goto nomem_out;
>  	}
> @@ -1243,12 +1284,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>  	 * the creation of a brand new one. So we need to perform this update
>  	 * by invoking update_policy_cpu().
>  	 */
> -	if (recover_policy && cpu != policy->cpu) {
> -		WARN_ON(update_policy_cpu(policy, cpu, dev));
> -	} else {
> -		policy->cpu = cpu;
> -		policy->kobj_cpu = cpu;
> -	}
> +	if (recover_policy && cpu != policy->cpu)
> +		update_policy_cpu(policy, cpu);
>  
>  	cpumask_copy(policy->cpus, cpumask_of(cpu));
>  
> @@ -1427,29 +1464,14 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>  			CPUFREQ_NAME_LEN);
>  	up_write(&policy->rwsem);
>  
> -	if (cpu != policy->kobj_cpu) {
> -		sysfs_remove_link(&dev->kobj, "cpufreq");
> -	} else if (cpus > 1) {
> -		/* Nominate new CPU */
> -		int new_cpu = cpumask_any_but(policy->cpus, cpu);
> -		struct device *cpu_dev = get_cpu_device(new_cpu);
> -
> -		sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
> -		ret = update_policy_cpu(policy, new_cpu, cpu_dev);
> -		if (ret) {
> -			if (sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
> -					      "cpufreq"))
> -				pr_err("%s: Failed to restore kobj link to cpu:%d\n",
> -				       __func__, cpu_dev->id);
> -			return ret;
> -		}
> +	if (cpu != policy->cpu)
> +		return 0;
>  
> -		if (!cpufreq_suspended)
> -			pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
> -				 __func__, new_cpu, cpu);
> -	} else if (cpufreq_driver->stop_cpu) {
> +	if (cpus > 1)
> +		/* Nominate new CPU */
> +		update_policy_cpu(policy, cpumask_any_but(policy->cpus, cpu));
> +	else if (cpufreq_driver->stop_cpu)
>  		cpufreq_driver->stop_cpu(policy);
> -	}
>  
>  	return 0;
>  }
> @@ -1470,32 +1492,11 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>  	cpumask_clear_cpu(cpu, policy->cpus);
>  	up_write(&policy->rwsem);
>  
> -	/* If cpu is last user of policy, free policy */
> -	if (policy_is_inactive(policy)) {
> -		if (has_target()) {
> -			ret = __cpufreq_governor(policy,
> -					CPUFREQ_GOV_POLICY_EXIT);
> -			if (ret) {
> -				pr_err("%s: Failed to exit governor\n",
> -				       __func__);
> -				return ret;
> -			}
> -		}
> -
> -		if (!cpufreq_suspended)
> -			cpufreq_policy_put_kobj(policy);
> +	/* Not the last cpu of policy, start governor again ? */
> +	if (!policy_is_inactive(policy)) {
> +		if (!has_target())
> +			return 0;
>  
> -		/*
> -		 * Perform the ->exit() even during light-weight tear-down,
> -		 * since this is a core component, and is essential for the
> -		 * subsequent light-weight ->init() to succeed.
> -		 */
> -		if (cpufreq_driver->exit)
> -			cpufreq_driver->exit(policy);
> -
> -		if (!cpufreq_suspended)
> -			cpufreq_policy_free(policy);
> -	} else if (has_target()) {
>  		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
>  		if (!ret)
>  			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> @@ -1504,8 +1505,34 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>  			pr_err("%s: Failed to start governor\n", __func__);
>  			return ret;
>  		}
> +
> +		return 0;
>  	}
>  
> +	/* If cpu is last user of policy, free policy */
> +	if (has_target()) {
> +		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
> +		if (ret) {
> +			pr_err("%s: Failed to exit governor\n", __func__);
> +			return ret;
> +		}
> +	}
> +
> +	/* Free the policy kobjects only if the driver is getting removed. */
> +	if (sif)
> +		cpufreq_policy_put_kobj(policy);
> +
> +	/*
> +	 * Perform the ->exit() even during light-weight tear-down,
> +	 * since this is a core component, and is essential for the
> +	 * subsequent light-weight ->init() to succeed.
> +	 */
> +	if (cpufreq_driver->exit)
> +		cpufreq_driver->exit(policy);
> +
> +	if (sif)
> +		cpufreq_policy_free(policy);
> +
>  	return 0;
>  }
>  
> @@ -1519,8 +1546,34 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>  	unsigned int cpu = dev->id;
>  	int ret;
>  
> -	if (cpu_is_offline(cpu))
> +	/*
> +	 * Only possible if 'cpu' is getting physically removed now. A hotplug
> +	 * notifier should have already been called and we just need to remove
> +	 * link or free policy here.
> +	 */
> +	if (cpu_is_offline(cpu)) {
> +		struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> +		struct cpumask mask;
> +
> +		if (!policy)
> +			return 0;
> +
> +		cpumask_copy(&mask, policy->related_cpus);
> +		cpumask_clear_cpu(cpu, &mask);
> +
> +		/*
> +		 * Free policy only if all policy->related_cpus are removed
> +		 * physically.
> +		 */
> +		if (cpumask_intersects(&mask, cpu_present_mask)) {
> +			remove_cpu_dev_symlink(policy, cpu);
> +			return 0;
> +		}
> +
> +		cpufreq_policy_put_kobj(policy);
> +		cpufreq_policy_free(policy);
>  		return 0;
> +	}
>  
>  	ret = __cpufreq_remove_dev_prepare(dev, sif);
>  
> 

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

Re: [PATCH V7 2/6] cpufreq: Stop migrating sysfs files on hotplug

[Viresh Kumar]

On 09-06-15, 01:23, Rafael J. Wysocki wrote:
> > -/* symlink affected CPUs */
> > +static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
> > +{
> > +	struct device *cpu_dev;
> > +
> 
> If you added
> 
> 	if (!policy)
> 		return 0;
> 
> here, you could save some lines of code below. ->

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Mon, 18 May 2015 08:23:50 +0530
Subject: [PATCH] cpufreq: Stop migrating sysfs files on hotplug

When we hot-unplug a cpu, we remove its sysfs cpufreq directory and if
the outgoing cpu was the owner of policy->kobj earlier then we migrate
the sysfs directory to under another online cpu.

There are few disadvantages this brings:
- Code Complexity
- Slower hotplug/suspend/resume
- sysfs file permissions are reset after all policy->cpus are offlined
- CPUFreq stats history lost after all policy->cpus are offlined
- Special management of sysfs stuff during suspend/resume

To overcome these, this patch modifies the way sysfs directories are
managed:
- Select sysfs kobjects owner while initializing policy and don't change
  it during hotplugs. Track it with kobj_cpu created earlier.

- Create symlinks for all related CPUs (can be offline) instead of
  affected CPUs on policy initialization and remove them only when the
  policy is freed.

- Free policy structure only on the removal of cpufreq-driver and not
  during hotplug/suspend/resume, detected by checking 'struct
  subsys_interface *' (Valid only when called from
  subsys_interface_unregister() while unregistering driver).

Apart from this, special care is taken to handle physical hoplug of CPUs
as we wouldn't remove sysfs links or remove policies on logical
hotplugs. Physical hotplug happens in the following sequence.

Hot removal:
- CPU is offlined first, ~ 'echo 0 >
  /sys/devices/system/cpu/cpuX/online'
- Then its device is removed along with all sysfs files, cpufreq core
  notified with cpufreq_remove_dev() callback from subsys-interface..

Hot addition:
- First the device along with its sysfs files is added, cpufreq core
  notified with cpufreq_add_dev() callback from subsys-interface..
- CPU is onlined, ~ 'echo 1 > /sys/devices/system/cpu/cpuX/online'

We call the same routines with both hotplug and subsys callbacks, and we
sense physical hotplug with cpu_offline() check in subsys callback. We
can handle most of the stuff with regular hotplug callback paths and
add/remove cpufreq sysfs links or free policy from subsys callbacks.

[ Something similar attempted by Saravana earlier ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 220 ++++++++++++++++++++++++++++------------------
 1 file changed, 135 insertions(+), 85 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 16214dfe78c5..4bfe0a53502f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -957,28 +957,67 @@ void cpufreq_sysfs_remove_file(const struct attribute *attr)
 }
 EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
 
-/* symlink affected CPUs */
+static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+{
+	struct device *cpu_dev;
+
+	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
+
+	if (!policy)
+		return 0;
+
+	cpu_dev = get_cpu_device(cpu);
+	if (WARN_ON(!cpu_dev))
+		return 0;
+
+	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+}
+
+static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+{
+	struct device *cpu_dev;
+
+	pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu);
+
+	cpu_dev = get_cpu_device(cpu);
+	if (WARN_ON(!cpu_dev))
+		return;
+
+	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
+}
+
+/* Add/remove symlinks for all related CPUs */
 static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
 {
 	unsigned int j;
 	int ret = 0;
 
-	for_each_cpu(j, policy->cpus) {
-		struct device *cpu_dev;
-
+	/* Some related CPUs might not be present (physically hotplugged) */
+	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
 		if (j == policy->kobj_cpu)
 			continue;
 
-		pr_debug("Adding link for CPU: %u\n", j);
-		cpu_dev = get_cpu_device(j);
-		ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
-					"cpufreq");
+		ret = add_cpu_dev_symlink(policy, j);
 		if (ret)
 			break;
 	}
+
 	return ret;
 }
 
+static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
+{
+	unsigned int j;
+
+	/* Some related CPUs might not be present (physically hotplugged) */
+	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
+		if (j == policy->kobj_cpu)
+			continue;
+
+		remove_cpu_dev_symlink(policy, j);
+	}
+}
+
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
 				     struct device *dev)
 {
@@ -1075,7 +1114,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
 		}
 	}
 
-	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+	return 0;
 }
 
 static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
@@ -1095,7 +1134,7 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
 	return policy;
 }
 
-static struct cpufreq_policy *cpufreq_policy_alloc(void)
+static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)
 {
 	struct cpufreq_policy *policy;
 
@@ -1116,6 +1155,11 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
+	policy->cpu = cpu;
+
+	/* Set this once on allocation */
+	policy->kobj_cpu = cpu;
+
 	return policy;
 
 err_free_cpumask:
@@ -1134,10 +1178,11 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
 	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
 			CPUFREQ_REMOVE_POLICY, policy);
 
-	down_read(&policy->rwsem);
+	down_write(&policy->rwsem);
+	cpufreq_remove_dev_symlink(policy);
 	kobj = &policy->kobj;
 	cmp = &policy->kobj_unregister;
-	up_read(&policy->rwsem);
+	up_write(&policy->rwsem);
 	kobject_put(kobj);
 
 	/*
@@ -1168,27 +1213,14 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 	kfree(policy);
 }
 
-static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
-			     struct device *cpu_dev)
+static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
-	int ret;
-
 	if (WARN_ON(cpu == policy->cpu))
-		return 0;
-
-	/* Move kobject to the new policy->cpu */
-	ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
-	if (ret) {
-		pr_err("%s: Failed to move kobj: %d\n", __func__, ret);
-		return ret;
-	}
+		return;
 
 	down_write(&policy->rwsem);
 	policy->cpu = cpu;
-	policy->kobj_cpu = cpu;
 	up_write(&policy->rwsem);
-
-	return 0;
 }
 
 /**
@@ -1206,13 +1238,19 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	int ret = -ENOMEM;
 	struct cpufreq_policy *policy;
 	unsigned long flags;
-	bool recover_policy = cpufreq_suspended;
-
-	if (cpu_is_offline(cpu))
-		return 0;
+	bool recover_policy = !sif;
 
 	pr_debug("adding CPU %u\n", cpu);
 
+	/*
+	 * Only possible if 'cpu' wasn't physically present earlier and we are
+	 * here from subsys_interface add callback. A hotplug notifier will
+	 * follow and we will handle it like logical CPU hotplug then. For now,
+	 * just create the sysfs link.
+	 */
+	if (cpu_is_offline(cpu))
+		return add_cpu_dev_symlink(per_cpu(cpufreq_cpu_data, cpu), cpu);
+
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
 
@@ -1232,7 +1270,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	policy = recover_policy ? cpufreq_policy_restore(cpu) : NULL;
 	if (!policy) {
 		recover_policy = false;
-		policy = cpufreq_policy_alloc();
+		policy = cpufreq_policy_alloc(cpu);
 		if (!policy)
 			goto nomem_out;
 	}
@@ -1243,12 +1281,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	 * the creation of a brand new one. So we need to perform this update
 	 * by invoking update_policy_cpu().
 	 */
-	if (recover_policy && cpu != policy->cpu) {
-		WARN_ON(update_policy_cpu(policy, cpu, dev));
-	} else {
-		policy->cpu = cpu;
-		policy->kobj_cpu = cpu;
-	}
+	if (recover_policy && cpu != policy->cpu)
+		update_policy_cpu(policy, cpu);
 
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
@@ -1427,29 +1461,14 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 			CPUFREQ_NAME_LEN);
 	up_write(&policy->rwsem);
 
-	if (cpu != policy->kobj_cpu) {
-		sysfs_remove_link(&dev->kobj, "cpufreq");
-	} else if (cpus > 1) {
-		/* Nominate new CPU */
-		int new_cpu = cpumask_any_but(policy->cpus, cpu);
-		struct device *cpu_dev = get_cpu_device(new_cpu);
-
-		sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
-		ret = update_policy_cpu(policy, new_cpu, cpu_dev);
-		if (ret) {
-			if (sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
-					      "cpufreq"))
-				pr_err("%s: Failed to restore kobj link to cpu:%d\n",
-				       __func__, cpu_dev->id);
-			return ret;
-		}
+	if (cpu != policy->cpu)
+		return 0;
 
-		if (!cpufreq_suspended)
-			pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
-				 __func__, new_cpu, cpu);
-	} else if (cpufreq_driver->stop_cpu) {
+	if (cpus > 1)
+		/* Nominate new CPU */
+		update_policy_cpu(policy, cpumask_any_but(policy->cpus, cpu));
+	else if (cpufreq_driver->stop_cpu)
 		cpufreq_driver->stop_cpu(policy);
-	}
 
 	return 0;
 }
@@ -1470,32 +1489,11 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 	cpumask_clear_cpu(cpu, policy->cpus);
 	up_write(&policy->rwsem);
 
-	/* If cpu is last user of policy, free policy */
-	if (policy_is_inactive(policy)) {
-		if (has_target()) {
-			ret = __cpufreq_governor(policy,
-					CPUFREQ_GOV_POLICY_EXIT);
-			if (ret) {
-				pr_err("%s: Failed to exit governor\n",
-				       __func__);
-				return ret;
-			}
-		}
-
-		if (!cpufreq_suspended)
-			cpufreq_policy_put_kobj(policy);
+	/* Not the last cpu of policy, start governor again ? */
+	if (!policy_is_inactive(policy)) {
+		if (!has_target())
+			return 0;
 
-		/*
-		 * Perform the ->exit() even during light-weight tear-down,
-		 * since this is a core component, and is essential for the
-		 * subsequent light-weight ->init() to succeed.
-		 */
-		if (cpufreq_driver->exit)
-			cpufreq_driver->exit(policy);
-
-		if (!cpufreq_suspended)
-			cpufreq_policy_free(policy);
-	} else if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
 		if (!ret)
 			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
@@ -1504,8 +1502,34 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 			pr_err("%s: Failed to start governor\n", __func__);
 			return ret;
 		}
+
+		return 0;
 	}
 
+	/* If cpu is last user of policy, free policy */
+	if (has_target()) {
+		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+		if (ret) {
+			pr_err("%s: Failed to exit governor\n", __func__);
+			return ret;
+		}
+	}
+
+	/* Free the policy kobjects only if the driver is getting removed. */
+	if (sif)
+		cpufreq_policy_put_kobj(policy);
+
+	/*
+	 * Perform the ->exit() even during light-weight tear-down,
+	 * since this is a core component, and is essential for the
+	 * subsequent light-weight ->init() to succeed.
+	 */
+	if (cpufreq_driver->exit)
+		cpufreq_driver->exit(policy);
+
+	if (sif)
+		cpufreq_policy_free(policy);
+
 	return 0;
 }
 
@@ -1519,8 +1543,34 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	unsigned int cpu = dev->id;
 	int ret;
 
-	if (cpu_is_offline(cpu))
+	/*
+	 * Only possible if 'cpu' is getting physically removed now. A hotplug
+	 * notifier should have already been called and we just need to remove
+	 * link or free policy here.
+	 */
+	if (cpu_is_offline(cpu)) {
+		struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+		struct cpumask mask;
+
+		if (!policy)
+			return 0;
+
+		cpumask_copy(&mask, policy->related_cpus);
+		cpumask_clear_cpu(cpu, &mask);
+
+		/*
+		 * Free policy only if all policy->related_cpus are removed
+		 * physically.
+		 */
+		if (cpumask_intersects(&mask, cpu_present_mask)) {
+			remove_cpu_dev_symlink(policy, cpu);
+			return 0;
+		}
+
+		cpufreq_policy_put_kobj(policy);
+		cpufreq_policy_free(policy);
 		return 0;
+	}
 
 	ret = __cpufreq_remove_dev_prepare(dev, sif);
 
-- 
viresh

Re: [PATCH V7 2/6] cpufreq: Stop migrating sysfs files on hotplug

[Saravana Kannan]

On 06/08/2015 05:55 AM, Viresh Kumar wrote:
> When we hot-unplug a cpu, we remove its sysfs cpufreq directory and if
> the outgoing cpu was the owner of policy->kobj earlier then we migrate
> the sysfs directory to under another online cpu.
>
> There are few disadvantages this brings:
> - Code Complexity
> - Slower hotplug/suspend/resume
> - sysfs file permissions are reset after all policy->cpus are offlined
> - CPUFreq stats history lost after all policy->cpus are offlined
> - Special management of sysfs stuff during suspend/resume
>
> To overcome these, this patch modifies the way sysfs directories are
> managed:
> - Select sysfs kobjects owner while initializing policy and don't change
>    it during hotplugs. Track it with kobj_cpu created earlier.
>
> - Create symlinks for all related CPUs (can be offline) instead of
>    affected CPUs on policy initialization and remove them only when the
>    policy is freed.
>
> - Free policy structure only on the removal of cpufreq-driver and not
>    during hotplug/suspend/resume, detected by checking 'struct
>    subsys_interface *' (Valid only when called from
>    subsys_interface_unregister() while unregistering driver).
>
> Apart from this, special care is taken to handle physical hoplug of CPUs
> as we wouldn't remove sysfs links or remove policies on logical
> hotplugs. Physical hotplug happens in the following sequence.
>
> Hot removal:
> - CPU is offlined first, ~ 'echo 0 >
>    /sys/devices/system/cpu/cpuX/online'
> - Then its device is removed along with all sysfs files, cpufreq core
>    notified with cpufreq_remove_dev() callback from subsys-interface..
>
> Hot addition:
> - First the device along with its sysfs files is added, cpufreq core
>    notified with cpufreq_add_dev() callback from subsys-interface..
> - CPU is onlined, ~ 'echo 1 > /sys/devices/system/cpu/cpuX/online'
>
> We call the same routines with both hotplug and subsys callbacks, and we
> sense physical hotplug with cpu_offline() check in subsys callback. We
> can handle most of the stuff with regular hotplug callback paths and
> add/remove cpufreq sysfs links or free policy from subsys callbacks.
>
> [ Something similar attempted by Saravana earlier ]
Full name and email would be nice.

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/cpufreq.c | 223 ++++++++++++++++++++++++++++------------------
>   1 file changed, 138 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7eeff892c0a6..663a934259a4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -957,28 +957,64 @@ void cpufreq_sysfs_remove_file(const struct attribute *attr)
>   }
>   EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
>
> -/* symlink affected CPUs */
> +static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
> +{
> +	struct device *cpu_dev;
> +
> +	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
> +
> +	cpu_dev = get_cpu_device(cpu);
> +	if (WARN_ON(!cpu_dev))
> +		return 0;
> +
> +	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
> +}
> +
> +static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
> +{
> +	struct device *cpu_dev;
> +
> +	pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu);
> +
> +	cpu_dev = get_cpu_device(cpu);
> +	if (WARN_ON(!cpu_dev))
> +		return;
> +
> +	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
> +}
> +
> +/* Add/remove symlinks for all related CPUs */
>   static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
>   {
>   	unsigned int j;
>   	int ret = 0;
>
> -	for_each_cpu(j, policy->cpus) {
> -		struct device *cpu_dev;
> -
> +	/* Some related CPUs might not be present (physically hotplugged) */
> +	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
>   		if (j == policy->kobj_cpu)
>   			continue;
>
> -		pr_debug("Adding link for CPU: %u\n", j);
> -		cpu_dev = get_cpu_device(j);
> -		ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
> -					"cpufreq");
> +		ret = add_cpu_dev_symlink(policy, j);
>   		if (ret)
>   			break;
>   	}
> +
>   	return ret;
>   }
>
> +static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
> +{
> +	unsigned int j;
> +
> +	/* Some related CPUs might not be present (physically hotplugged) */
> +	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
> +		if (j == policy->kobj_cpu)
> +			continue;
> +
> +		remove_cpu_dev_symlink(policy, j);
> +	}
> +}
> +
>   static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
>   				     struct device *dev)
>   {
> @@ -1075,7 +1111,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
>   		}
>   	}
>
> -	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> +	return 0;
>   }
>
>   static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
> @@ -1095,7 +1131,7 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
>   	return policy;
>   }
>
> -static struct cpufreq_policy *cpufreq_policy_alloc(void)
> +static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)
>   {
>   	struct cpufreq_policy *policy;
>
> @@ -1116,6 +1152,11 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
>   	init_completion(&policy->kobj_unregister);
>   	INIT_WORK(&policy->update, handle_update);
>
> +	policy->cpu = cpu;
> +
> +	/* Set this once on allocation */
> +	policy->kobj_cpu = cpu;
> +
>   	return policy;
>
>   err_free_cpumask:
> @@ -1134,10 +1175,11 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
>   	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>   			CPUFREQ_REMOVE_POLICY, policy);
>
> -	down_read(&policy->rwsem);
> +	down_write(&policy->rwsem);
> +	cpufreq_remove_dev_symlink(policy);
>   	kobj = &policy->kobj;
>   	cmp = &policy->kobj_unregister;
> -	up_read(&policy->rwsem);
> +	up_write(&policy->rwsem);
>   	kobject_put(kobj);
>
>   	/*
> @@ -1168,27 +1210,14 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>   	kfree(policy);
>   }
>
> -static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
> -			     struct device *cpu_dev)
> +static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
>   {
> -	int ret;
> -
>   	if (WARN_ON(cpu == policy->cpu))
Can you remind me again why this is a warning? Would we still need this 
check?

> -		return 0;
> -
> -	/* Move kobject to the new policy->cpu */
> -	ret = kobject_move(&policy->kobj, &cpu_dev->kobj);
> -	if (ret) {
> -		pr_err("%s: Failed to move kobj: %d\n", __func__, ret);
> -		return ret;
> -	}
> +		return;
>
>   	down_write(&policy->rwsem);
>   	policy->cpu = cpu;
> -	policy->kobj_cpu = cpu;
>   	up_write(&policy->rwsem);
> -
> -	return 0;
>   }
>
>   /**
> @@ -1206,13 +1235,25 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>   	int ret = -ENOMEM;
>   	struct cpufreq_policy *policy;
>   	unsigned long flags;
> -	bool recover_policy = cpufreq_suspended;
> -
> -	if (cpu_is_offline(cpu))
> -		return 0;
> +	bool recover_policy = !sif;
The policy is always going to be there, so calling it "recover_policy" 
is kinda confusing. But I can't suggest a better name now.

>
>   	pr_debug("adding CPU %u\n", cpu);
>
> +	/*
> +	 * Only possible if 'cpu' wasn't physically present earlier and we are
> +	 * here from subsys_interface add callback. A hotplug notifier will
> +	 * follow and we will handle it like logical CPU hotplug then. For now,
> +	 * just create the sysfs link.
> +	 */
> +	if (cpu_is_offline(cpu)) {
My changes were on an older code base, so things might have changed by 
now. But at this location, there was definitely a case where I had to 
check for "sif" before creating symlinks. I need to think a bit more to 
remember what that reason was and see if you have to do it too.

I'll try to respond more later. But I just wanted to send out what I 
could when I have little time to review this.

-Saravana

> +		policy = per_cpu(cpufreq_cpu_data, cpu);
> +		/* No need to create link of the first cpu of a policy */
> +		if (!policy)
> +			return 0;
> +
> +		return add_cpu_dev_symlink(policy, cpu);
> +	}
> +
>   	if (!down_read_trylock(&cpufreq_rwsem))
>   		return 0;
>
> @@ -1232,7 +1273,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>   	policy = recover_policy ? cpufreq_policy_restore(cpu) : NULL;
>   	if (!policy) {
>   		recover_policy = false;
> -		policy = cpufreq_policy_alloc();
> +		policy = cpufreq_policy_alloc(cpu);
>   		if (!policy)
>   			goto nomem_out;
>   	}
> @@ -1243,12 +1284,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>   	 * the creation of a brand new one. So we need to perform this update
>   	 * by invoking update_policy_cpu().
>   	 */
> -	if (recover_policy && cpu != policy->cpu) {
> -		WARN_ON(update_policy_cpu(policy, cpu, dev));
> -	} else {
> -		policy->cpu = cpu;
> -		policy->kobj_cpu = cpu;
> -	}
> +	if (recover_policy && cpu != policy->cpu)
> +		update_policy_cpu(policy, cpu);
>
>   	cpumask_copy(policy->cpus, cpumask_of(cpu));
>
> @@ -1427,29 +1464,14 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>   			CPUFREQ_NAME_LEN);
>   	up_write(&policy->rwsem);
>
> -	if (cpu != policy->kobj_cpu) {
> -		sysfs_remove_link(&dev->kobj, "cpufreq");
> -	} else if (cpus > 1) {
> -		/* Nominate new CPU */
> -		int new_cpu = cpumask_any_but(policy->cpus, cpu);
> -		struct device *cpu_dev = get_cpu_device(new_cpu);
> -
> -		sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
> -		ret = update_policy_cpu(policy, new_cpu, cpu_dev);
> -		if (ret) {
> -			if (sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
> -					      "cpufreq"))
> -				pr_err("%s: Failed to restore kobj link to cpu:%d\n",
> -				       __func__, cpu_dev->id);
> -			return ret;
> -		}
> +	if (cpu != policy->cpu)
> +		return 0;
>
> -		if (!cpufreq_suspended)
> -			pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n",
> -				 __func__, new_cpu, cpu);
> -	} else if (cpufreq_driver->stop_cpu) {
> +	if (cpus > 1)
> +		/* Nominate new CPU */
> +		update_policy_cpu(policy, cpumask_any_but(policy->cpus, cpu));
> +	else if (cpufreq_driver->stop_cpu)
>   		cpufreq_driver->stop_cpu(policy);
> -	}
>
>   	return 0;
>   }
> @@ -1470,32 +1492,11 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>   	cpumask_clear_cpu(cpu, policy->cpus);
>   	up_write(&policy->rwsem);
>
> -	/* If cpu is last user of policy, free policy */
> -	if (policy_is_inactive(policy)) {
> -		if (has_target()) {
> -			ret = __cpufreq_governor(policy,
> -					CPUFREQ_GOV_POLICY_EXIT);
> -			if (ret) {
> -				pr_err("%s: Failed to exit governor\n",
> -				       __func__);
> -				return ret;
> -			}
> -		}
> -
> -		if (!cpufreq_suspended)
> -			cpufreq_policy_put_kobj(policy);
> +	/* Not the last cpu of policy, start governor again ? */
> +	if (!policy_is_inactive(policy)) {
> +		if (!has_target())
> +			return 0;
>
> -		/*
> -		 * Perform the ->exit() even during light-weight tear-down,
> -		 * since this is a core component, and is essential for the
> -		 * subsequent light-weight ->init() to succeed.
> -		 */
> -		if (cpufreq_driver->exit)
> -			cpufreq_driver->exit(policy);
> -
> -		if (!cpufreq_suspended)
> -			cpufreq_policy_free(policy);
> -	} else if (has_target()) {
>   		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
>   		if (!ret)
>   			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
> @@ -1504,8 +1505,34 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>   			pr_err("%s: Failed to start governor\n", __func__);
>   			return ret;
>   		}
> +
> +		return 0;
>   	}
>
> +	/* If cpu is last user of policy, free policy */
> +	if (has_target()) {
> +		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
> +		if (ret) {
> +			pr_err("%s: Failed to exit governor\n", __func__);
> +			return ret;
> +		}
> +	}
> +
> +	/* Free the policy kobjects only if the driver is getting removed. */
> +	if (sif)
> +		cpufreq_policy_put_kobj(policy);
> +
> +	/*
> +	 * Perform the ->exit() even during light-weight tear-down,
> +	 * since this is a core component, and is essential for the
> +	 * subsequent light-weight ->init() to succeed.
> +	 */
> +	if (cpufreq_driver->exit)
> +		cpufreq_driver->exit(policy);
> +
> +	if (sif)
> +		cpufreq_policy_free(policy);
> +
>   	return 0;
>   }
>
> @@ -1519,8 +1546,34 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>   	unsigned int cpu = dev->id;
>   	int ret;
>
> -	if (cpu_is_offline(cpu))
> +	/*
> +	 * Only possible if 'cpu' is getting physically removed now. A hotplug
> +	 * notifier should have already been called and we just need to remove
> +	 * link or free policy here.
> +	 */
> +	if (cpu_is_offline(cpu)) {
> +		struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> +		struct cpumask mask;
> +
> +		if (!policy)
> +			return 0;
> +
> +		cpumask_copy(&mask, policy->related_cpus);
> +		cpumask_clear_cpu(cpu, &mask);
> +
> +		/*
> +		 * Free policy only if all policy->related_cpus are removed
> +		 * physically.
> +		 */
> +		if (cpumask_intersects(&mask, cpu_present_mask)) {
> +			remove_cpu_dev_symlink(policy, cpu);
> +			return 0;
> +		}
> +
> +		cpufreq_policy_put_kobj(policy);
> +		cpufreq_policy_free(policy);
>   		return 0;
> +	}
>
>   	ret = __cpufreq_remove_dev_prepare(dev, sif);
>
>


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V7 2/6] cpufreq: Stop migrating sysfs files on hotplug

[Viresh Kumar]

On 09-06-15, 17:20, Saravana Kannan wrote:
> >[ Something similar attempted by Saravana earlier ]
> Full name and email would be nice.

Sure. @Rafael can you please hand edit this in case I am not required
to resend the patch ?

[ Something similar attempted by Saravana Kannan <skannan@codeaurora.org> earlier ]

> >+static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
> >  {
> >-	int ret;
> >-
> >  	if (WARN_ON(cpu == policy->cpu))
> Can you remind me again why this is a warning? Would we still need
> this check?

Its removed in a later commit. Its a warning because we are updating
policy->cpu here and that must have been done only if policy->cpu !=
cpu.

> >+	bool recover_policy = !sif;
> The policy is always going to be there, so calling it
> "recover_policy" is kinda confusing. But I can't suggest a better
> name now.

Not always. Its created as well sometimes :)

> >
> >  	pr_debug("adding CPU %u\n", cpu);
> >
> >+	/*
> >+	 * Only possible if 'cpu' wasn't physically present earlier and we are
> >+	 * here from subsys_interface add callback. A hotplug notifier will
> >+	 * follow and we will handle it like logical CPU hotplug then. For now,
> >+	 * just create the sysfs link.
> >+	 */
> >+	if (cpu_is_offline(cpu)) {
> My changes were on an older code base, so things might have changed
> by now. But at this location, there was definitely a case where I
> had to check for "sif" before creating symlinks. I need to think a

cpu will be offline here only for sif==true.

> bit more to remember what that reason was and see if you have to do
> it too.
> 
> I'll try to respond more later. But I just wanted to send out what I
> could when I have little time to review this.

Okay. Good to see you again.

-- 
viresh

Re: [PATCH V7 2/6] cpufreq: Stop migrating sysfs files on hotplug

[Rafael J. Wysocki]

On Wednesday, June 10, 2015 07:49:12 AM Viresh Kumar wrote:
> On 09-06-15, 17:20, Saravana Kannan wrote:
> > >[ Something similar attempted by Saravana earlier ]
> > Full name and email would be nice.
> 
> Sure. @Rafael can you please hand edit this in case I am not required
> to resend the patch ?
> 
> [ Something similar attempted by Saravana Kannan <skannan@codeaurora.org> earlier ]

OK, I've added Original-by tags to this patch and the [5/6].


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

[PATCH V7 3/6] cpufreq: Initialize policy->kobj while allocating policy

[Viresh Kumar]

policy->kobj is required to be initialized once in the lifetime of a
policy.  Currently we are initializing it from __cpufreq_add_dev() and
that doesn't look to be the best place for doing so as we have to do
this on special cases (like: !recover_policy).

We can initialize it from a more obvious place cpufreq_policy_alloc()
and that will make code look cleaner, specially the error handling part.

The error handling part of __cpufreq_add_dev() was doing almost the same
thing while recover_policy is true or false. Fix that as well by always
calling cpufreq_policy_put_kobj() with an additional parameter to skip
notification part of it.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 663a934259a4..d890804d561f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1131,9 +1131,10 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
 	return policy;
 }
 
-static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)
+static struct cpufreq_policy *cpufreq_policy_alloc(struct device *dev)
 {
 	struct cpufreq_policy *policy;
+	int ret;
 
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
 	if (!policy)
@@ -1145,6 +1146,13 @@ static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)
 	if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL))
 		goto err_free_cpumask;
 
+	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq, &dev->kobj,
+				   "cpufreq");
+	if (ret) {
+		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
+		goto err_free_rcpumask;
+	}
+
 	INIT_LIST_HEAD(&policy->policy_list);
 	init_rwsem(&policy->rwsem);
 	spin_lock_init(&policy->transition_lock);
@@ -1152,13 +1160,15 @@ static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
-	policy->cpu = cpu;
+	policy->cpu = dev->id;
 
 	/* Set this once on allocation */
-	policy->kobj_cpu = cpu;
+	policy->kobj_cpu = dev->id;
 
 	return policy;
 
+err_free_rcpumask:
+	free_cpumask_var(policy->related_cpus);
 err_free_cpumask:
 	free_cpumask_var(policy->cpus);
 err_free_policy:
@@ -1167,13 +1177,14 @@ static struct cpufreq_policy *cpufreq_policy_alloc(int cpu)
 	return NULL;
 }
 
-static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
+static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify)
 {
 	struct kobject *kobj;
 	struct completion *cmp;
 
-	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
-			CPUFREQ_REMOVE_POLICY, policy);
+	if (notify)
+		blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
+					     CPUFREQ_REMOVE_POLICY, policy);
 
 	down_write(&policy->rwsem);
 	cpufreq_remove_dev_symlink(policy);
@@ -1273,7 +1284,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	policy = recover_policy ? cpufreq_policy_restore(cpu) : NULL;
 	if (!policy) {
 		recover_policy = false;
-		policy = cpufreq_policy_alloc(cpu);
+		policy = cpufreq_policy_alloc(dev);
 		if (!policy)
 			goto nomem_out;
 	}
@@ -1313,15 +1324,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 		policy->user_policy.min = policy->min;
 		policy->user_policy.max = policy->max;
 
-		/* prepare interface data */
-		ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
-					   &dev->kobj, "cpufreq");
-		if (ret) {
-			pr_err("%s: failed to init policy->kobj: %d\n",
-			       __func__, ret);
-			goto err_init_policy_kobj;
-		}
-
 		write_lock_irqsave(&cpufreq_driver_lock, flags);
 		for_each_cpu(j, policy->related_cpus)
 			per_cpu(cpufreq_cpu_data, j) = policy;
@@ -1413,18 +1415,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 
 err_out_unregister:
 err_get_freq:
-	if (!recover_policy) {
-		kobject_put(&policy->kobj);
-		wait_for_completion(&policy->kobj_unregister);
-	}
-err_init_policy_kobj:
 	up_write(&policy->rwsem);
 
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 err_set_policy_cpu:
-	if (recover_policy)
-		cpufreq_policy_put_kobj(policy);
+	cpufreq_policy_put_kobj(policy, recover_policy);
 	cpufreq_policy_free(policy);
 
 nomem_out:
@@ -1520,7 +1516,7 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 
 	/* Free the policy kobjects only if the driver is getting removed. */
 	if (sif)
-		cpufreq_policy_put_kobj(policy);
+		cpufreq_policy_put_kobj(policy, true);
 
 	/*
 	 * Perform the ->exit() even during light-weight tear-down,
@@ -1570,7 +1566,7 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 			return 0;
 		}
 
-		cpufreq_policy_put_kobj(policy);
+		cpufreq_policy_put_kobj(policy, true);
 		cpufreq_policy_free(policy);
 		return 0;
 	}
-- 
2.4.0

[PATCH V7 4/6] cpufreq: Call cpufreq_policy_put_kobj() from cpufreq_policy_free()

[Viresh Kumar]

cpufreq_policy_put_kobj() is actually part of freeing the policy and can
be called from cpufreq_policy_free() directly instead of a separate
call.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index d890804d561f..d3bd85263643 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1203,7 +1203,7 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify)
 	pr_debug("wait complete\n");
 }
 
-static void cpufreq_policy_free(struct cpufreq_policy *policy)
+static void cpufreq_policy_free(struct cpufreq_policy *policy, bool notify)
 {
 	unsigned long flags;
 	int cpu;
@@ -1216,6 +1216,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 		per_cpu(cpufreq_cpu_data, cpu) = NULL;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	cpufreq_policy_put_kobj(policy, notify);
 	free_cpumask_var(policy->related_cpus);
 	free_cpumask_var(policy->cpus);
 	kfree(policy);
@@ -1420,9 +1421,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 err_set_policy_cpu:
-	cpufreq_policy_put_kobj(policy, recover_policy);
-	cpufreq_policy_free(policy);
-
+	cpufreq_policy_free(policy, recover_policy);
 nomem_out:
 	up_read(&cpufreq_rwsem);
 
@@ -1514,10 +1513,6 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 		}
 	}
 
-	/* Free the policy kobjects only if the driver is getting removed. */
-	if (sif)
-		cpufreq_policy_put_kobj(policy, true);
-
 	/*
 	 * Perform the ->exit() even during light-weight tear-down,
 	 * since this is a core component, and is essential for the
@@ -1526,8 +1521,9 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 
+	/* Free the policy only if the driver is getting removed. */
 	if (sif)
-		cpufreq_policy_free(policy);
+		cpufreq_policy_free(policy, true);
 
 	return 0;
 }
@@ -1566,8 +1562,7 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 			return 0;
 		}
 
-		cpufreq_policy_put_kobj(policy, true);
-		cpufreq_policy_free(policy);
+		cpufreq_policy_free(policy, true);
 		return 0;
 	}
 
-- 
2.4.0

[PATCH V7 5/6] cpufreq: Restart governor as soon as possible

[Viresh Kumar]

On cpu hot-unplug, we don't need to wait for POST_DEAD notification to
restart the governor if the policy has atleast one online cpu left. We
can restart the governor right from the DOWN_PREPARE notification
instead.

[ Something similar attempted by Saravana earlier ]

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index d3bd85263643..5f1c3366fb8e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1431,8 +1431,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 static int __cpufreq_remove_dev_prepare(struct device *dev,
 					struct subsys_interface *sif)
 {
-	unsigned int cpu = dev->id, cpus;
-	int ret;
+	unsigned int cpu = dev->id;
+	int ret = 0;
 	struct cpufreq_policy *policy;
 
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
@@ -1452,23 +1452,33 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 	}
 
 	down_write(&policy->rwsem);
-	cpus = cpumask_weight(policy->cpus);
+	cpumask_clear_cpu(cpu, policy->cpus);
 
-	if (has_target() && cpus == 1)
-		strncpy(policy->last_governor, policy->governor->name,
-			CPUFREQ_NAME_LEN);
+	if (policy_is_inactive(policy)) {
+		if (has_target())
+			strncpy(policy->last_governor, policy->governor->name,
+				CPUFREQ_NAME_LEN);
+	} else if (cpu == policy->cpu) {
+		/* Nominate new CPU */
+		policy->cpu = cpumask_any(policy->cpus);
+	}
 	up_write(&policy->rwsem);
 
-	if (cpu != policy->cpu)
-		return 0;
+	/* Start governor again for active policy */
+	if (!policy_is_inactive(policy)) {
+		if (has_target()) {
+			ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
+			if (!ret)
+				ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
-	if (cpus > 1)
-		/* Nominate new CPU */
-		update_policy_cpu(policy, cpumask_any_but(policy->cpus, cpu));
-	else if (cpufreq_driver->stop_cpu)
+			if (ret)
+				pr_err("%s: Failed to start governor\n", __func__);
+		}
+	} else if (cpufreq_driver->stop_cpu) {
 		cpufreq_driver->stop_cpu(policy);
+	}
 
-	return 0;
+	return ret;
 }
 
 static int __cpufreq_remove_dev_finish(struct device *dev,
@@ -1476,33 +1486,16 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 {
 	unsigned int cpu = dev->id;
 	int ret;
-	struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
+	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
 	if (!policy) {
 		pr_debug("%s: No cpu_data found\n", __func__);
 		return -EINVAL;
 	}
 
-	down_write(&policy->rwsem);
-	cpumask_clear_cpu(cpu, policy->cpus);
-	up_write(&policy->rwsem);
-
-	/* Not the last cpu of policy, start governor again ? */
-	if (!policy_is_inactive(policy)) {
-		if (!has_target())
-			return 0;
-
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
-		if (!ret)
-			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
-
-		if (ret) {
-			pr_err("%s: Failed to start governor\n", __func__);
-			return ret;
-		}
-
+	/* Only proceed for inactive policies */
+	if (!policy_is_inactive(policy))
 		return 0;
-	}
 
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {
-- 
2.4.0

Re: [PATCH V7 5/6] cpufreq: Restart governor as soon as possible

[Rafael J. Wysocki]

On Monday, June 08, 2015 06:25:31 PM Viresh Kumar wrote:
> On cpu hot-unplug, we don't need to wait for POST_DEAD notification to
> restart the governor if the policy has atleast one online cpu left. We
> can restart the governor right from the DOWN_PREPARE notification
> instead.

Any explanation why this is correct here?


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

Re: [PATCH V7 5/6] cpufreq: Restart governor as soon as possible

[Viresh Kumar]

On 09-06-15, 01:27, Rafael J. Wysocki wrote:
> Any explanation why this is correct here?

Does this look better ? (Code isn't updated)

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Thu, 19 Feb 2015 11:53:02 +0530
Subject: [PATCH] cpufreq: Restart governor as soon as possible

__cpufreq_remove_dev_finish() is doing two things today:
- Restarts the governor if some CPUs from concerned policy are still
  online.
- Frees the policy if all CPUs are offline.

The first task of restarting the governor can be moved to
__cpufreq_remove_dev_prepare() to restart the governor early. There is
no race between _prepare() and _finish() as they would be handling
completely different cases. _finish() will only be required if we are
going to free the policy and that has nothing to do with restarting the
governor.

[ Something similar attempted by Saravana earlier ]

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8b810071ddd2..c355ab656dfb 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1428,8 +1428,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 static int __cpufreq_remove_dev_prepare(struct device *dev,
 					struct subsys_interface *sif)
 {
-	unsigned int cpu = dev->id, cpus;
-	int ret;
+	unsigned int cpu = dev->id;
+	int ret = 0;
 	struct cpufreq_policy *policy;
 
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
@@ -1449,23 +1449,33 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 	}
 
 	down_write(&policy->rwsem);
-	cpus = cpumask_weight(policy->cpus);
+	cpumask_clear_cpu(cpu, policy->cpus);
 
-	if (has_target() && cpus == 1)
-		strncpy(policy->last_governor, policy->governor->name,
-			CPUFREQ_NAME_LEN);
+	if (policy_is_inactive(policy)) {
+		if (has_target())
+			strncpy(policy->last_governor, policy->governor->name,
+				CPUFREQ_NAME_LEN);
+	} else if (cpu == policy->cpu) {
+		/* Nominate new CPU */
+		policy->cpu = cpumask_any(policy->cpus);
+	}
 	up_write(&policy->rwsem);
 
-	if (cpu != policy->cpu)
-		return 0;
+	/* Start governor again for active policy */
+	if (!policy_is_inactive(policy)) {
+		if (has_target()) {
+			ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
+			if (!ret)
+				ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
-	if (cpus > 1)
-		/* Nominate new CPU */
-		update_policy_cpu(policy, cpumask_any_but(policy->cpus, cpu));
-	else if (cpufreq_driver->stop_cpu)
+			if (ret)
+				pr_err("%s: Failed to start governor\n", __func__);
+		}
+	} else if (cpufreq_driver->stop_cpu) {
 		cpufreq_driver->stop_cpu(policy);
+	}
 
-	return 0;
+	return ret;
 }
 
 static int __cpufreq_remove_dev_finish(struct device *dev,
@@ -1473,33 +1483,16 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 {
 	unsigned int cpu = dev->id;
 	int ret;
-	struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
+	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
 	if (!policy) {
 		pr_debug("%s: No cpu_data found\n", __func__);
 		return -EINVAL;
 	}
 
-	down_write(&policy->rwsem);
-	cpumask_clear_cpu(cpu, policy->cpus);
-	up_write(&policy->rwsem);
-
-	/* Not the last cpu of policy, start governor again ? */
-	if (!policy_is_inactive(policy)) {
-		if (!has_target())
-			return 0;
-
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
-		if (!ret)
-			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
-
-		if (ret) {
-			pr_err("%s: Failed to start governor\n", __func__);
-			return ret;
-		}
-
+	/* Only proceed for inactive policies */
+	if (!policy_is_inactive(policy))
 		return 0;
-	}
 
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {


-- 
viresh

[PATCH V7 6/6] cpufreq: Remove cpufreq_update_policy()

[Viresh Kumar]

cpufreq_update_policy() was kept as a separate routine earlier as it was
handling migration of sysfs directories, which isn't the case anymore.
It is only updating policy->cpu now and is called by a single caller.

The WARN_ON() isn't really required anymore, as we are just updating the
cpu now, not moving the sysfs directories.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5f1c3366fb8e..d0497f379e3a 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1126,6 +1126,10 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
 	if (likely(policy)) {
 		/* Policy should be inactive here */
 		WARN_ON(!policy_is_inactive(policy));
+
+		down_write(&policy->rwsem);
+		policy->cpu = cpu;
+		up_write(&policy->rwsem);
 	}
 
 	return policy;
@@ -1222,16 +1226,6 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy, bool notify)
 	kfree(policy);
 }
 
-static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
-{
-	if (WARN_ON(cpu == policy->cpu))
-		return;
-
-	down_write(&policy->rwsem);
-	policy->cpu = cpu;
-	up_write(&policy->rwsem);
-}
-
 /**
  * cpufreq_add_dev - add a CPU device
  *
@@ -1290,15 +1284,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 			goto nomem_out;
 	}
 
-	/*
-	 * In the resume path, since we restore a saved policy, the assignment
-	 * to policy->cpu is like an update of the existing policy, rather than
-	 * the creation of a brand new one. So we need to perform this update
-	 * by invoking update_policy_cpu().
-	 */
-	if (recover_policy && cpu != policy->cpu)
-		update_policy_cpu(policy, cpu);
-
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
 	/* call driver. From then on the cpufreq must be able
-- 
2.4.0

Re: [PATCH V7 0/6] cpufreq: Don't loose cpufreq history on CPU hotplug

[Rafael J. Wysocki]

On Monday, June 08, 2015 06:25:26 PM Viresh Kumar wrote:
> Hi Rafael,
> 
> The aim of this series is to stop managing cpufreq sysfs directories on CPU
> hotplug.
> 
> Currently on removal of a 'cpu != policy->cpu', we remove its sysfs directories
> by removing the soft-link. And on removal of policy->cpu, we migrate the sysfs
> directories to the next cpu in policy. But if policy->cpu was the last CPU, we
> remove the policy completely and allocate it again once the CPUs come back.
> 
> This has shortcomings:
> 
> - Code Complexity
> - Slower hotplug
> - sysfs file permissions are reset after all policy->cpus are offlined
> - CPUFreq stats history lost after all policy->cpus are offlined
> - Special management of sysfs stuff during suspend/resume
> 
> 
> To make things simple we can stop playing with sysfs files unless the driver is
> getting removed. Also the policy can be kept intact to be used later.
> 
> First few patches provide a clean base for others *more important* patches.
> 
> V6->V7:
> - Change the order of patches, "cpufreq: Remove cpufreq_update_policy"
>   is moved to the end of the series and updated its changelog.
> 
> V5-V6:
> - Merged v4 9/14 into 10/14 after some updates.
> - Keep separate routines to add/remove symlinks.
> - Only updated one patch in this version
> 
> V4-V5:
> - Sending all patches again
> - Fixed comments in one of the patches as suggested by you
> - Merged the last commit about "physical hotplug of CPUs" with "cpufreq: Stop
>   migrating sysfs files on hotplug".
> - A new patch (content is old, just separated out into its own patch) "cpufreq:
>   add/remove sysfs links via cpufreq_add_remove_dev_symlink()"
> 
> V3-V4:
> - Only four patches sent this time:
>   - [PATCH V4 01/14] cpufreq: Create for_each_{in}active_policy()
>   - [PATCH V4 04/14] cpufreq: Don't traverse all active policies to find
>   - [PATCH V4 05/14] cpufreq: Manage governor usage history with
>   - [PATCH V4 06/14] cpufreq: Mark policy->governor = NULL for inactive
> - Remove __temp from the arguments of for_each_[in]active_policies.
> - Simplified macros/next_policy, etc.
> 
> V2->V3:
> - First five are already applied by you and the 7th one was sent separately as a
>   fix earlier and got applied. So, V2 had 20 patches and V3 has 14.
> - Dropped while(1) and used do/while.
> - policy->governor marked as NULL only while removing the governor and not when
>   policy becomes inactive.
> - Commit logs/comments updated
> - Applied Acks from Saravan
> 
> v1->V2:
> - Dropped the idea of using policy-lists for getting policy for any cpu
> - Also dropped fallback list and its per-cpu variable
> - Stopped cleaning cpufreq_cpu_data and doing list_del(policy) on logical
>   hotplug.
> - Added support for physical hotplug of CPUs (Untested).
> 
> Viresh Kumar (6):
>   cpufreq: Don't allow updating inactive policies from sysfs
>   cpufreq: Stop migrating sysfs files on hotplug
>   cpufreq: Initialize policy->kobj while allocating policy
>   cpufreq: Call cpufreq_policy_put_kobj() from cpufreq_policy_free()
>   cpufreq: Restart governor as soon as possible
>   cpufreq: Remove cpufreq_update_policy()
> 
>  drivers/cpufreq/cpufreq.c | 303 +++++++++++++++++++++++++---------------------
>  1 file changed, 168 insertions(+), 135 deletions(-)

Overall, this looks good to me.  There are some nitpicks here and there, though
(please see replies to individual patches for details).


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