[PATCH] cpufreq: Avoid attempts to create duplicate symbolic links

[PATCH] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

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

After commit 87549141d516 (cpufreq: Stop migrating sysfs files on
hotplug) there is a problem with CPUs that share cpufreq policy
objects with other CPUs and are initially offline.

Say CPU1 shares a policy with CPU0 which is online and is registered
first.  As part of the registration process, cpufreq_add_dev() is
called for it.  It creates the policy object and a symbolic link
to it from the CPU1's sysfs directory.  If CPU1 is registered
subsequently and it is offline at that time, cpufreq_add_dev() will
attempt to create a symbolic link to the policy object for it, but
that link is present already, so a warning about that will be
triggered.

To avoid that warning, make cpufreq use an additional CPU mask
containing related CPUs that are actually present for each policy
object.  That mask is initialized when the policy object is populated
after its creation (for the first online CPU using it) and it includes
CPUs from the "policy CPUs" mask returned by the cpufreq driver's
->init() callback that are physically present at that time.  Symbolic
links to the policy are created only for the CPUs in that mask.

If cpufreq_add_dev() is invoked for an offline CPU, it checks the
new mask and only creates the symlink if the CPU was not in it (the
CPU is added to the mask at the same time).

In turn, cpufreq_remove_dev() drops the given CPU from the new mask,
removes its symlink to the policy object and returns, unless it is
the CPU owning the policy object.  In that case, the policy object
is moved to a new CPU's sysfs directory or deleted if the CPU being
removed was the last user of the policy.

While at it, notice that cpufreq_remove_dev() can't fail, because
its return value is ignored, so make it ignore return values from
__cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
and prevent these functions from aborting on errors returned by
__cpufreq_governor().

Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reported-by: Russell King <linux@arm.linux.org.uk>
---

This is supposed to replace the other patches sent so far to address the issue
at hand.

---
 drivers/cpufreq/cpufreq.c |   92 ++++++++++++++++++++++++----------------------
 include/linux/cpufreq.h   |    1 
 2 files changed, 50 insertions(+), 43 deletions(-)

Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -62,6 +62,7 @@ struct cpufreq_policy {
 	/* CPUs sharing clock, require sw coordination */
 	cpumask_var_t		cpus;	/* Online CPUs only */
 	cpumask_var_t		related_cpus; /* Online + Offline CPUs */
+	cpumask_var_t		real_cpus; /* Related and present */
 
 	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
 						should set cpufreq */
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1002,7 +1002,7 @@ static int cpufreq_add_dev_symlink(struc
 	int ret = 0;
 
 	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
+	for_each_cpu(j, policy->real_cpus) {
 		if (j == policy->kobj_cpu)
 			continue;
 
@@ -1019,7 +1019,7 @@ static void cpufreq_remove_dev_symlink(s
 	unsigned int j;
 
 	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
+	for_each_cpu(j, policy->real_cpus) {
 		if (j == policy->kobj_cpu)
 			continue;
 
@@ -1163,11 +1163,14 @@ static struct cpufreq_policy *cpufreq_po
 	if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL))
 		goto err_free_cpumask;
 
+	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
+		goto err_free_rcpumask;
+
 	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;
+		goto err_free_real_cpus;
 	}
 
 	INIT_LIST_HEAD(&policy->policy_list);
@@ -1184,6 +1187,8 @@ static struct cpufreq_policy *cpufreq_po
 
 	return policy;
 
+err_free_real_cpus:
+	free_cpumask_var(policy->real_cpus);
 err_free_rcpumask:
 	free_cpumask_var(policy->related_cpus);
 err_free_cpumask:
@@ -1234,6 +1239,7 @@ static void cpufreq_policy_free(struct c
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	cpufreq_policy_put_kobj(policy, notify);
+	free_cpumask_var(policy->real_cpus);
 	free_cpumask_var(policy->related_cpus);
 	free_cpumask_var(policy->cpus);
 	kfree(policy);
@@ -1258,14 +1264,17 @@ static int cpufreq_add_dev(struct device
 
 	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 (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 logical CPU hotplug then.  For now, just create the
+		 * sysfs link, unless there is no policy.
+		 */
+		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;
+	}
 
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
@@ -1307,6 +1316,9 @@ static int cpufreq_add_dev(struct device
 	/* related cpus should atleast have policy->cpus */
 	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
 
+	cpumask_and(policy->cpus, policy->cpus, cpu_present_mask);
+	cpumask_or(policy->real_cpus, policy->real_cpus, policy->cpus);
+
 	/*
 	 * affected cpus must always be the one, which are online. We aren't
 	 * managing offline cpus here.
@@ -1437,10 +1449,8 @@ static int __cpufreq_remove_dev_prepare(
 
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to stop governor\n", __func__);
-			return ret;
-		}
 	}
 
 	down_write(&policy->rwsem);
@@ -1492,10 +1502,8 @@ static int __cpufreq_remove_dev_finish(s
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to exit governor\n", __func__);
-			return ret;
-		}
 	}
 
 	/*
@@ -1521,42 +1529,40 @@ static int __cpufreq_remove_dev_finish(s
 static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 {
 	unsigned int cpu = dev->id;
-	int ret;
-
-	/*
-	 * 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;
+	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
-		if (!policy)
-			return 0;
+	if (!policy)
+		return 0;
 
-		cpumask_copy(&mask, policy->related_cpus);
-		cpumask_clear_cpu(cpu, &mask);
+	if (cpu_online(cpu)) {
+		__cpufreq_remove_dev_prepare(dev, sif);
+		__cpufreq_remove_dev_finish(dev, sif);
+	}
 
-		/*
-		 * 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;
-		}
+	cpumask_clear_cpu(cpu, policy->real_cpus);
 
+	if (cpumask_empty(policy->real_cpus)) {
 		cpufreq_policy_free(policy, true);
 		return 0;
 	}
 
-	ret = __cpufreq_remove_dev_prepare(dev, sif);
+	if (cpu != policy->kobj_cpu) {
+		remove_cpu_dev_symlink(policy, cpu);
+	} else {
+		/*
+		 * This is the CPU owning the policy object.  Move it to another
+		 * suitable CPU.
+		 */
+		unsigned int new_cpu = cpumask_first(policy->real_cpus);
+		struct device *new_dev = get_cpu_device(new_cpu);
 
-	if (!ret)
-		ret = __cpufreq_remove_dev_finish(dev, sif);
+		dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu);
 
-	return ret;
+		policy->kobj_cpu = new_cpu;
+		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
+	}
+
+	return 0;
 }
 
 static void handle_update(struct work_struct *work)

Re: [PATCH] cpufreq: Avoid attempts to create duplicate symbolic links

[Viresh Kumar]

On 23-07-15, 23:14, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit 87549141d516 (cpufreq: Stop migrating sysfs files on
> hotplug) there is a problem with CPUs that share cpufreq policy
> objects with other CPUs and are initially offline.
> 
> Say CPU1 shares a policy with CPU0 which is online and is registered
> first.  As part of the registration process, cpufreq_add_dev() is
> called for it.  It creates the policy object and a symbolic link
> to it from the CPU1's sysfs directory.  If CPU1 is registered
> subsequently and it is offline at that time, cpufreq_add_dev() will
> attempt to create a symbolic link to the policy object for it, but
> that link is present already, so a warning about that will be
> triggered.
> 
> To avoid that warning, make cpufreq use an additional CPU mask
> containing related CPUs that are actually present for each policy
> object.  That mask is initialized when the policy object is populated
> after its creation (for the first online CPU using it) and it includes
> CPUs from the "policy CPUs" mask returned by the cpufreq driver's
> ->init() callback that are physically present at that time.  Symbolic
> links to the policy are created only for the CPUs in that mask.
> 
> If cpufreq_add_dev() is invoked for an offline CPU, it checks the
> new mask and only creates the symlink if the CPU was not in it (the
> CPU is added to the mask at the same time).
> 
> In turn, cpufreq_remove_dev() drops the given CPU from the new mask,
> removes its symlink to the policy object and returns, unless it is
> the CPU owning the policy object.  In that case, the policy object
> is moved to a new CPU's sysfs directory or deleted if the CPU being
> removed was the last user of the policy.
> 
> While at it, notice that cpufreq_remove_dev() can't fail, because
> its return value is ignored, so make it ignore return values from
> __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
> and prevent these functions from aborting on errors returned by
> __cpufreq_governor().
> 
> Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reported-by: Russell King <linux@arm.linux.org.uk>
> ---
> 
> This is supposed to replace the other patches sent so far to address the issue
> at hand.

Lets take this one and leave my patches. They are generating more
diff and actually doing part of the general improvements Russell
suggested.

> +	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))

I was wondering if we should use cpumask_t type variables, so that we
don't have to allocate these masks. They are always with policies.

> @@ -1307,6 +1316,9 @@ static int cpufreq_add_dev(struct device
>  	/* related cpus should atleast have policy->cpus */
>  	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
>  
> +	cpumask_and(policy->cpus, policy->cpus, cpu_present_mask);
> +	cpumask_or(policy->real_cpus, policy->real_cpus, policy->cpus);
> +

I will do this differently:
        cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);

policy->cpus is anyway going to be anded with online mask.

>  	/*
>  	 * affected cpus must always be the one, which are online. We aren't
>  	 * managing offline cpus here.


>  static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>  {

> -	ret = __cpufreq_remove_dev_prepare(dev, sif);
> +	if (cpu != policy->kobj_cpu) {
> +		remove_cpu_dev_symlink(policy, cpu);
> +	} else {
> +		/*
> +		 * This is the CPU owning the policy object.  Move it to another
> +		 * suitable CPU.
> +		 */
> +		unsigned int new_cpu = cpumask_first(policy->real_cpus);
> +		struct device *new_dev = get_cpu_device(new_cpu);
>  
> -	if (!ret)
> -		ret = __cpufreq_remove_dev_finish(dev, sif);
> +		dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu);
>  
> -	return ret;
> +		policy->kobj_cpu = new_cpu;

You need to remove the link for the target cpu, like what I did in my
patch:

               sysfs_remove_link(&new_dev->kobj, "cpufreq");

> +		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
> +	}
> +
> +	return 0;
>  }
>  
>  static void handle_update(struct work_struct *work)

-- 
viresh

Re: [PATCH] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

On Friday, July 24, 2015 07:39:43 PM Viresh Kumar wrote:
> On 23-07-15, 23:14, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > After commit 87549141d516 (cpufreq: Stop migrating sysfs files on
> > hotplug) there is a problem with CPUs that share cpufreq policy
> > objects with other CPUs and are initially offline.
> > 
> > Say CPU1 shares a policy with CPU0 which is online and is registered
> > first.  As part of the registration process, cpufreq_add_dev() is
> > called for it.  It creates the policy object and a symbolic link
> > to it from the CPU1's sysfs directory.  If CPU1 is registered
> > subsequently and it is offline at that time, cpufreq_add_dev() will
> > attempt to create a symbolic link to the policy object for it, but
> > that link is present already, so a warning about that will be
> > triggered.
> > 
> > To avoid that warning, make cpufreq use an additional CPU mask
> > containing related CPUs that are actually present for each policy
> > object.  That mask is initialized when the policy object is populated
> > after its creation (for the first online CPU using it) and it includes
> > CPUs from the "policy CPUs" mask returned by the cpufreq driver's
> > ->init() callback that are physically present at that time.  Symbolic
> > links to the policy are created only for the CPUs in that mask.
> > 
> > If cpufreq_add_dev() is invoked for an offline CPU, it checks the
> > new mask and only creates the symlink if the CPU was not in it (the
> > CPU is added to the mask at the same time).
> > 
> > In turn, cpufreq_remove_dev() drops the given CPU from the new mask,
> > removes its symlink to the policy object and returns, unless it is
> > the CPU owning the policy object.  In that case, the policy object
> > is moved to a new CPU's sysfs directory or deleted if the CPU being
> > removed was the last user of the policy.
> > 
> > While at it, notice that cpufreq_remove_dev() can't fail, because
> > its return value is ignored, so make it ignore return values from
> > __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
> > and prevent these functions from aborting on errors returned by
> > __cpufreq_governor().
> > 
> > Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug)
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Reported-by: Russell King <linux@arm.linux.org.uk>
> > ---
> > 
> > This is supposed to replace the other patches sent so far to address the issue
> > at hand.
> 
> Lets take this one and leave my patches. They are generating more
> diff and actually doing part of the general improvements Russell
> suggested.

OK, but we can do better than this. :-) (See below)

> > +	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
> 
> I was wondering if we should use cpumask_t type variables, so that we
> don't have to allocate these masks. They are always with policies.

We can do that, but let's do it as a cleanup later.

> > @@ -1307,6 +1316,9 @@ static int cpufreq_add_dev(struct device
> >  	/* related cpus should atleast have policy->cpus */
> >  	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
> >  
> > +	cpumask_and(policy->cpus, policy->cpus, cpu_present_mask);
> > +	cpumask_or(policy->real_cpus, policy->real_cpus, policy->cpus);
> > +
> 
> I will do this differently:
>         cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
> 
> policy->cpus is anyway going to be anded with online mask.

Right.

> >  	/*
> >  	 * affected cpus must always be the one, which are online. We aren't
> >  	 * managing offline cpus here.
> 
> 
> >  static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
> >  {
> 
> > -	ret = __cpufreq_remove_dev_prepare(dev, sif);
> > +	if (cpu != policy->kobj_cpu) {
> > +		remove_cpu_dev_symlink(policy, cpu);
> > +	} else {
> > +		/*
> > +		 * This is the CPU owning the policy object.  Move it to another
> > +		 * suitable CPU.
> > +		 */
> > +		unsigned int new_cpu = cpumask_first(policy->real_cpus);
> > +		struct device *new_dev = get_cpu_device(new_cpu);
> >  
> > -	if (!ret)
> > -		ret = __cpufreq_remove_dev_finish(dev, sif);
> > +		dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu);
> >  
> > -	return ret;
> > +		policy->kobj_cpu = new_cpu;
> 
> You need to remove the link for the target cpu, like what I did in my
> patch:

Right.

Plus I forgot to drop the policy freeing from __cpufreq_remove_dev_finish().

>                sysfs_remove_link(&new_dev->kobj, "cpufreq");
> 
> > +		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  static void handle_update(struct work_struct *work)

Still, as I said before, we can do better here.

The key observation is that if a CPU doesn't go online at all, we don't need to
care about it.  Hence, we can ignore a CPU until it goes online for the first
time and we only need to create policy symlinks for online CPUs (just like we
only create policy objects for online CPUs).

Of course, we need to avoid creating a duplicate policy symlink if the CPU
has been online at least once before, but that's quite straightforward.

I'll send an updated patch shortly.

Thanks,
Rafael

[PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

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

After commit 87549141d516 (cpufreq: Stop migrating sysfs files on
hotplug) there is a problem with CPUs that share cpufreq policy
objects with other CPUs and are initially offline.

Say CPU1 shares a policy with CPU0 which is online and is registered
first.  As part of the registration process, cpufreq_add_dev() is
called for it.  It creates the policy object and a symbolic link
to it from the CPU1's sysfs directory.  If CPU1 is registered
subsequently and it is offline at that time, cpufreq_add_dev() will
attempt to create a symbolic link to the policy object for it, but
that link is present already, so a warning about that will be
triggered.

To avoid that warning, use the observation that cpufreq doesn't
need to care about CPUs that have never been online.  Namely,
make cpufreq use an additional CPU mask for each policy containing
all CPUs using the given policy that have ever been online.  That
mask is initialized when the policy object is populated after its
creation (for the first online CPU using it) and it includes the
policy owner only to start with.

At the same time, cpufreq_add_dev() is modified to ignore offline
CPUs (the only case it can see an offline CPU is when that CPU has
never been online, so cpufreq doesn't need to care about it).  Also,
if it is invoked for a CPU having a policy already, it checks if
that CPU has gone online for the first time and creates a symbolic
link to the policy for it if that's the case.  The CPU is then added
to the new mask of CPUs that have been online at least once.

In turn, cpufreq_remove_dev() drops the given CPU from the new mask.
If the mask turns out to be empty at that point, the policy object is
not needed any more, so it is deleted.  Otherwise, the CPU's symlink
to the policy object is removed, unless that's the CPU owning the
policy object.  In that case, the policy object is moved to the sysfs
directory of another CPU using the same policy.

While at it, notice that cpufreq_remove_dev() can't fail, because
its return value is ignored, so make it ignore return values from
__cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
and prevent these functions from aborting on errors returned by
__cpufreq_governor().  Also, drop the now unused sif argument from
them.

Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reported-by: Russell King <linux@arm.linux.org.uk>
---

Replacement for https://patchwork.kernel.org/patch/6856011/

---
 drivers/cpufreq/cpufreq.c |  195 ++++++++++++++++------------------------------
 include/linux/cpufreq.h   |    1 
 2 files changed, 73 insertions(+), 123 deletions(-)

Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -62,6 +62,7 @@ struct cpufreq_policy {
 	/* CPUs sharing clock, require sw coordination */
 	cpumask_var_t		cpus;	/* Online CPUs only */
 	cpumask_var_t		related_cpus; /* Online + Offline CPUs */
+	cpumask_var_t		managed_cpus; /* CPUs that have ever been online */
 
 	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
 						should set cpufreq */
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -966,67 +966,6 @@ void cpufreq_sysfs_remove_file(const str
 }
 EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
 
-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;
-
-	/* 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;
-
-		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)
 {
@@ -1051,13 +990,10 @@ static int cpufreq_add_dev_interface(str
 	if (ret)
 		return ret;
 
-	if (cpufreq_driver->bios_limit) {
+	if (cpufreq_driver->bios_limit)
 		ret = sysfs_create_file(&policy->kobj, &bios_limit.attr);
-		if (ret)
-			return ret;
-	}
 
-	return cpufreq_add_dev_symlink(policy);
+	return ret;
 }
 
 static void cpufreq_init_policy(struct cpufreq_policy *policy)
@@ -1151,6 +1087,7 @@ static struct cpufreq_policy *cpufreq_po
 static struct cpufreq_policy *cpufreq_policy_alloc(struct device *dev)
 {
 	struct cpufreq_policy *policy;
+	unsigned int cpu = dev->id;
 	int ret;
 
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
@@ -1163,11 +1100,14 @@ static struct cpufreq_policy *cpufreq_po
 	if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL))
 		goto err_free_cpumask;
 
+	if (!zalloc_cpumask_var(&policy->managed_cpus, GFP_KERNEL))
+		goto err_free_rcpumask;
+
 	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;
+		goto err_free_managed_cpus;
 	}
 
 	INIT_LIST_HEAD(&policy->policy_list);
@@ -1177,13 +1117,17 @@ 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;
+	/* Add the "kobject" CPU to the "managed" mask. */
+	cpumask_set_cpu(cpu, policy->managed_cpus);
 
 	return policy;
 
+err_free_managed_cpus:
+	free_cpumask_var(policy->managed_cpus);
 err_free_rcpumask:
 	free_cpumask_var(policy->related_cpus);
 err_free_cpumask:
@@ -1204,7 +1148,6 @@ static void cpufreq_policy_put_kobj(stru
 					     CPUFREQ_REMOVE_POLICY, policy);
 
 	down_write(&policy->rwsem);
-	cpufreq_remove_dev_symlink(policy);
 	kobj = &policy->kobj;
 	cmp = &policy->kobj_unregister;
 	up_write(&policy->rwsem);
@@ -1234,6 +1177,7 @@ static void cpufreq_policy_free(struct c
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	cpufreq_policy_put_kobj(policy, notify);
+	free_cpumask_var(policy->managed_cpus);
 	free_cpumask_var(policy->related_cpus);
 	free_cpumask_var(policy->cpus);
 	kfree(policy);
@@ -1259,24 +1203,34 @@ static int cpufreq_add_dev(struct device
 	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.
+	 * Only possible if we are here from the subsys_interface add callback,
+	 * so if the CPU is offline now, it has never been online.  Ignore it.
 	 */
 	if (cpu_is_offline(cpu))
-		return add_cpu_dev_symlink(per_cpu(cpufreq_cpu_data, cpu), cpu);
+		return 0;
 
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
 
 	/* 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);
-		up_read(&cpufreq_rwsem);
-		return ret;
+		/*
+		 * If this is the first time the CPU is going online, link it to
+		 * the policy and mark it as "managed".
+		 */
+		if (!cpumask_test_cpu(cpu, policy->managed_cpus)) {
+			ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+			if (ret)
+				goto out;
+
+			cpumask_set_cpu(cpu, policy->managed_cpus);
+		}
+		if (!policy_is_inactive(policy)) {
+			ret = cpufreq_add_policy_cpu(policy, cpu, dev);
+			goto out;
+		}
 	}
 
 	/*
@@ -1288,7 +1242,7 @@ static int cpufreq_add_dev(struct device
 		recover_policy = false;
 		policy = cpufreq_policy_alloc(dev);
 		if (!policy)
-			goto nomem_out;
+			goto out;
 	}
 
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
@@ -1414,14 +1368,13 @@ err_get_freq:
 		cpufreq_driver->exit(policy);
 err_set_policy_cpu:
 	cpufreq_policy_free(policy, recover_policy);
-nomem_out:
+out:
 	up_read(&cpufreq_rwsem);
 
 	return ret;
 }
 
-static int __cpufreq_remove_dev_prepare(struct device *dev,
-					struct subsys_interface *sif)
+static int __cpufreq_remove_dev_prepare(struct device *dev)
 {
 	unsigned int cpu = dev->id;
 	int ret = 0;
@@ -1437,10 +1390,8 @@ static int __cpufreq_remove_dev_prepare(
 
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to stop governor\n", __func__);
-			return ret;
-		}
 	}
 
 	down_write(&policy->rwsem);
@@ -1473,8 +1424,7 @@ static int __cpufreq_remove_dev_prepare(
 	return ret;
 }
 
-static int __cpufreq_remove_dev_finish(struct device *dev,
-				       struct subsys_interface *sif)
+static int __cpufreq_remove_dev_finish(struct device *dev)
 {
 	unsigned int cpu = dev->id;
 	int ret;
@@ -1492,10 +1442,8 @@ static int __cpufreq_remove_dev_finish(s
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to exit governor\n", __func__);
-			return ret;
-		}
 	}
 
 	/*
@@ -1506,10 +1454,6 @@ static int __cpufreq_remove_dev_finish(s
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 
-	/* Free the policy only if the driver is getting removed. */
-	if (sif)
-		cpufreq_policy_free(policy, true);
-
 	return 0;
 }
 
@@ -1521,42 +1465,47 @@ static int __cpufreq_remove_dev_finish(s
 static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 {
 	unsigned int cpu = dev->id;
-	int ret;
-
-	/*
-	 * 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;
+	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
-		if (!policy)
-			return 0;
+	if (!policy)
+		return 0;
 
-		cpumask_copy(&mask, policy->related_cpus);
-		cpumask_clear_cpu(cpu, &mask);
+	/* Nothing to do if the CPU has never been online. */
+	if (!cpumask_test_and_clear_cpu(cpu, policy->managed_cpus))
+		return 0;
 
-		/*
-		 * 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;
-		}
+	if (cpu_online(cpu)) {
+		__cpufreq_remove_dev_prepare(dev);
+		__cpufreq_remove_dev_finish(dev);
+	}
 
+	/*
+	 * If all of the CPUs using this policy that have ever been online go
+	 * away, we don't need the policy any more, so delete it.
+	 */
+	if (cpumask_empty(policy->managed_cpus)) {
 		cpufreq_policy_free(policy, true);
 		return 0;
 	}
 
-	ret = __cpufreq_remove_dev_prepare(dev, sif);
+	if (cpu != policy->kobj_cpu) {
+		sysfs_remove_link(&dev->kobj, "cpufreq");
+	} else {
+		/*
+		 * The CPU owning the policy object is going away.  Move it to
+		 * another suitable CPU.
+		 */
+		unsigned int new_cpu = cpumask_first(policy->managed_cpus);
+		struct device *new_dev = get_cpu_device(new_cpu);
+
+		dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu);
 
-	if (!ret)
-		ret = __cpufreq_remove_dev_finish(dev, sif);
+		sysfs_remove_link(&new_dev->kobj, "cpufreq");
+		policy->kobj_cpu = new_cpu;
+		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
+	}
 
-	return ret;
+	return 0;
 }
 
 static void handle_update(struct work_struct *work)
@@ -2395,11 +2344,11 @@ static int cpufreq_cpu_callback(struct n
 			break;
 
 		case CPU_DOWN_PREPARE:
-			__cpufreq_remove_dev_prepare(dev, NULL);
+			__cpufreq_remove_dev_prepare(dev);
 			break;
 
 		case CPU_POST_DEAD:
-			__cpufreq_remove_dev_finish(dev, NULL);
+			__cpufreq_remove_dev_finish(dev);
 			break;
 
 		case CPU_DOWN_FAILED:

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Viresh Kumar]

On 25-07-15, 00:17, Rafael J. Wysocki wrote:
> To avoid that warning, use the observation that cpufreq doesn't
> need to care about CPUs that have never been online.

I have concerns over the very philosophy behind the patch and so
wanted to discuss more on that.

It will be really confusing to have a scenario where:
- we have a four related CPUs: 0-3.
- 0-1 are online and have /sys/devices/system/cpu/cpuX/cpufreq directory
- 2 is offline but was once online and so still has a directory
- 3 never came online after the cpufreq driver is registered (we need
  to think about cpufreq driver being a module here, its possible CPU
  was online earlier) and so it doesn't have a directory.

How will the user distinguish between cpu 3 and 4, both being offline
and user may not know one of them was never online. And the related
CPUs of 0-2 will include CPU 3 as well..

I think, we just moved into the wrong direction. We have a valid
policy for CPU4, with all valid data. Why not show it up in sysfs?

So, what we discussed over IRC earlier was, cpufreq shouldn't care
about CPUs, which are offline and that don't have a policy allocated
for them. So if all the CPUs of a policy never came online after the
driver is registered, we shouldn't care about them.

I think, for know your earlier version of the patch was just fine,
with the improvements I suggested. And we should go ahead with
solution like what I gave, the diff of that was quite big for an rc
fix and so I said your patch looks better.

-- 
viresh

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

Hi Viresh,

On Sat, Jul 25, 2015 at 3:00 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 25-07-15, 00:17, Rafael J. Wysocki wrote:
>> To avoid that warning, use the observation that cpufreq doesn't
>> need to care about CPUs that have never been online.
>
> I have concerns over the very philosophy behind the patch and so
> wanted to discuss more on that.
>
> It will be really confusing to have a scenario where:
> - we have a four related CPUs: 0-3.
> - 0-1 are online and have /sys/devices/system/cpu/cpuX/cpufreq directory
> - 2 is offline but was once online and so still has a directory
> - 3 never came online after the cpufreq driver is registered (we need
>   to think about cpufreq driver being a module here, its possible CPU
>   was online earlier) and so it doesn't have a directory.
>
> How will the user distinguish between cpu 3 and 4, both being offline
> and user may not know one of them was never online. And the related
> CPUs of 0-2 will include CPU 3 as well..

So the problem is that a CPU (which is present) listed by related_cpus
does not have a symbolic link to the policy, right?

That is a valid point, although related_cpus can list CPUs that aren't
present even in theory.  That is a super corner case, however.

> I think, we just moved into the wrong direction. We have a valid
> policy for CPU4, with all valid data. Why not show it up in sysfs?

And why do we care?  The CPU is offline and may never go online before
the cpufreq driver is unloaded.

> So, what we discussed over IRC earlier was, cpufreq shouldn't care
> about CPUs, which are offline and that don't have a policy allocated
> for them. So if all the CPUs of a policy never came online after the
> driver is registered, we shouldn't care about them.

That's an alternative really.  Either we don't care about offline CPUs
and only preserve their sysfs stuff once it's been created (just in
case we need it again when the CPU goes online), or we do care about
offline CPUs that share a policy object wirh at least one online CPU.
The code is slightly simpler in the former case and the information
seen by user space is slightly more consistent in the latter case.

We need to make a choice and you seem to be preferring the second
option.  I'm fine with that, if we choose this one, it really only
makes sense to create all of the links from present CPUs to the policy
object at the time that object is created to avoid havig a (presumably
small) window in which inconsistent information may be seen by user
space.

> I think, for know your earlier version of the patch was just fine,
> with the improvements I suggested. And we should go ahead with
> solution like what I gave, the diff of that was quite big for an rc
> fix and so I said your patch looks better.

OK, I'll prepare a new version of that patch then, but as I said this
choice means that we'll be creating the links to the policy at the
policy creation time going forward.

Thanks,
Rafael

[PATCH v3] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

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

After commit 87549141d516 (cpufreq: Stop migrating sysfs files on
hotplug) there is a problem with CPUs that share cpufreq policy
objects with other CPUs and are initially offline.

Say CPU1 shares a policy with CPU0 which is online and is registered
first.  As part of the registration process, cpufreq_add_dev() is
called for it.  It creates the policy object and a symbolic link
to it from the CPU1's sysfs directory.  If CPU1 is registered
subsequently and it is offline at that time, cpufreq_add_dev() will
attempt to create a symbolic link to the policy object for it, but
that link is present already, so a warning about that will be
triggered.

To avoid that warning, make cpufreq use an additional CPU mask
containing related CPUs that are actually present for each policy
object.  That mask is initialized when the policy object is populated
after its creation (for the first online CPU using it) and it includes
CPUs from the "policy CPUs" mask returned by the cpufreq driver's
->init() callback that are physically present at that time.  Symbolic
links to the policy are created only for the CPUs in that mask.

If cpufreq_add_dev() is invoked for an offline CPU, it checks the
new mask and only creates the symlink if the CPU was not in it (the
CPU is added to the mask at the same time).

In turn, cpufreq_remove_dev() drops the given CPU from the new mask,
removes its symlink to the policy object and returns, unless it is
the CPU owning the policy object.  In that case, the policy object
is moved to a new CPU's sysfs directory or deleted if the CPU being
removed was the last user of the policy.

While at it, notice that cpufreq_remove_dev() can't fail, because
its return value is ignored, so make it ignore return values from
__cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
and prevent these functions from aborting on errors returned by
__cpufreq_governor().  Also drop the now unused sif argument from
them.

Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reported-by: Russell King <linux@arm.linux.org.uk>
---

This goes back to the https://patchwork.kernel.org/patch/6856011/
idea, but has a couple of bugs fixed and goes a it further with
cleaning things up.

---
 drivers/cpufreq/cpufreq.c |  108 +++++++++++++++++++++++-----------------------
 include/linux/cpufreq.h   |    1 
 2 files changed, 56 insertions(+), 53 deletions(-)

Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -62,6 +62,7 @@ struct cpufreq_policy {
 	/* CPUs sharing clock, require sw coordination */
 	cpumask_var_t		cpus;	/* Online CPUs only */
 	cpumask_var_t		related_cpus; /* Online + Offline CPUs */
+	cpumask_var_t		real_cpus; /* Related and present */
 
 	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
 						should set cpufreq */
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1002,7 +1002,7 @@ static int cpufreq_add_dev_symlink(struc
 	int ret = 0;
 
 	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
+	for_each_cpu(j, policy->real_cpus) {
 		if (j == policy->kobj_cpu)
 			continue;
 
@@ -1019,7 +1019,7 @@ static void cpufreq_remove_dev_symlink(s
 	unsigned int j;
 
 	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) {
+	for_each_cpu(j, policy->real_cpus) {
 		if (j == policy->kobj_cpu)
 			continue;
 
@@ -1163,11 +1163,14 @@ static struct cpufreq_policy *cpufreq_po
 	if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL))
 		goto err_free_cpumask;
 
+	if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL))
+		goto err_free_rcpumask;
+
 	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;
+		goto err_free_real_cpus;
 	}
 
 	INIT_LIST_HEAD(&policy->policy_list);
@@ -1184,6 +1187,8 @@ static struct cpufreq_policy *cpufreq_po
 
 	return policy;
 
+err_free_real_cpus:
+	free_cpumask_var(policy->real_cpus);
 err_free_rcpumask:
 	free_cpumask_var(policy->related_cpus);
 err_free_cpumask:
@@ -1234,6 +1239,7 @@ static void cpufreq_policy_free(struct c
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	cpufreq_policy_put_kobj(policy, notify);
+	free_cpumask_var(policy->real_cpus);
 	free_cpumask_var(policy->related_cpus);
 	free_cpumask_var(policy->cpus);
 	kfree(policy);
@@ -1258,14 +1264,17 @@ static int cpufreq_add_dev(struct device
 
 	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 (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;
+	}
 
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
@@ -1307,6 +1316,10 @@ static int cpufreq_add_dev(struct device
 	/* 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)
+		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
+
 	/*
 	 * affected cpus must always be the one, which are online. We aren't
 	 * managing offline cpus here.
@@ -1420,8 +1433,7 @@ nomem_out:
 	return ret;
 }
 
-static int __cpufreq_remove_dev_prepare(struct device *dev,
-					struct subsys_interface *sif)
+static int __cpufreq_remove_dev_prepare(struct device *dev)
 {
 	unsigned int cpu = dev->id;
 	int ret = 0;
@@ -1437,10 +1449,8 @@ static int __cpufreq_remove_dev_prepare(
 
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to stop governor\n", __func__);
-			return ret;
-		}
 	}
 
 	down_write(&policy->rwsem);
@@ -1473,8 +1483,7 @@ static int __cpufreq_remove_dev_prepare(
 	return ret;
 }
 
-static int __cpufreq_remove_dev_finish(struct device *dev,
-				       struct subsys_interface *sif)
+static int __cpufreq_remove_dev_finish(struct device *dev)
 {
 	unsigned int cpu = dev->id;
 	int ret;
@@ -1492,10 +1501,8 @@ static int __cpufreq_remove_dev_finish(s
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to exit governor\n", __func__);
-			return ret;
-		}
 	}
 
 	/*
@@ -1506,10 +1513,6 @@ static int __cpufreq_remove_dev_finish(s
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 
-	/* Free the policy only if the driver is getting removed. */
-	if (sif)
-		cpufreq_policy_free(policy, true);
-
 	return 0;
 }
 
@@ -1521,42 +1524,41 @@ static int __cpufreq_remove_dev_finish(s
 static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 {
 	unsigned int cpu = dev->id;
-	int ret;
-
-	/*
-	 * 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;
+	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
-		if (!policy)
-			return 0;
+	if (!policy)
+		return 0;
 
-		cpumask_copy(&mask, policy->related_cpus);
-		cpumask_clear_cpu(cpu, &mask);
+	if (cpu_online(cpu)) {
+		__cpufreq_remove_dev_prepare(dev);
+		__cpufreq_remove_dev_finish(dev);
+	}
 
-		/*
-		 * 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;
-		}
+	cpumask_clear_cpu(cpu, policy->real_cpus);
 
+	if (cpumask_empty(policy->real_cpus)) {
 		cpufreq_policy_free(policy, true);
 		return 0;
 	}
 
-	ret = __cpufreq_remove_dev_prepare(dev, sif);
+	if (cpu != policy->kobj_cpu) {
+		remove_cpu_dev_symlink(policy, cpu);
+	} else {
+		/*
+		 * The CPU owning the policy object is going away.  Move it to
+		 * another suitable CPU.
+		 */
+		unsigned int new_cpu = cpumask_first(policy->real_cpus);
+		struct device *new_dev = get_cpu_device(new_cpu);
+
+		dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu);
 
-	if (!ret)
-		ret = __cpufreq_remove_dev_finish(dev, sif);
+		sysfs_remove_link(&new_dev->kobj, "cpufreq");
+		policy->kobj_cpu = new_cpu;
+		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
+	}
 
-	return ret;
+	return 0;
 }
 
 static void handle_update(struct work_struct *work)
@@ -2395,11 +2397,11 @@ static int cpufreq_cpu_callback(struct n
 			break;
 
 		case CPU_DOWN_PREPARE:
-			__cpufreq_remove_dev_prepare(dev, NULL);
+			__cpufreq_remove_dev_prepare(dev);
 			break;
 
 		case CPU_POST_DEAD:
-			__cpufreq_remove_dev_finish(dev, NULL);
+			__cpufreq_remove_dev_finish(dev);
 			break;
 
 		case CPU_DOWN_FAILED:

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Viresh Kumar]

On 26-07-15, 00:46, Rafael J. Wysocki wrote:
> OK, I'll prepare a new version of that patch then, but as I said this
> choice means that we'll be creating the links to the policy at the
> policy creation time going forward.

Atleast for the rc fix, we should do exactly this. Right.

But we can rethink about getting both my earlier patches merged for
4.3, which did this:

- Keep adding CPUs to a global mask, which didn't had a existing
  policy and were offline when subsys-callback for called for them.
- And then create the links only when the subsys callback is called
  for CPUs, for which policy already exist, as Russell suggested.

-- 
viresh

Re: [PATCH v3] cpufreq: Avoid attempts to create duplicate symbolic links

[Viresh Kumar]

On 26-07-15, 02:28, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit 87549141d516 (cpufreq: Stop migrating sysfs files on
> hotplug) there is a problem with CPUs that share cpufreq policy
> objects with other CPUs and are initially offline.
> 
> Say CPU1 shares a policy with CPU0 which is online and is registered
> first.  As part of the registration process, cpufreq_add_dev() is
> called for it.  It creates the policy object and a symbolic link
> to it from the CPU1's sysfs directory.  If CPU1 is registered
> subsequently and it is offline at that time, cpufreq_add_dev() will
> attempt to create a symbolic link to the policy object for it, but
> that link is present already, so a warning about that will be
> triggered.
> 
> To avoid that warning, make cpufreq use an additional CPU mask
> containing related CPUs that are actually present for each policy
> object.  That mask is initialized when the policy object is populated
> after its creation (for the first online CPU using it) and it includes
> CPUs from the "policy CPUs" mask returned by the cpufreq driver's
> ->init() callback that are physically present at that time.  Symbolic
> links to the policy are created only for the CPUs in that mask.
> 
> If cpufreq_add_dev() is invoked for an offline CPU, it checks the
> new mask and only creates the symlink if the CPU was not in it (the
> CPU is added to the mask at the same time).
> 
> In turn, cpufreq_remove_dev() drops the given CPU from the new mask,
> removes its symlink to the policy object and returns, unless it is
> the CPU owning the policy object.  In that case, the policy object
> is moved to a new CPU's sysfs directory or deleted if the CPU being
> removed was the last user of the policy.
> 
> While at it, notice that cpufreq_remove_dev() can't fail, because
> its return value is ignored, so make it ignore return values from
> __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
> and prevent these functions from aborting on errors returned by
> __cpufreq_governor().  Also drop the now unused sif argument from
> them.
> 
> Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reported-by: Russell King <linux@arm.linux.org.uk>
> ---
> 
> This goes back to the https://patchwork.kernel.org/patch/6856011/
> idea, but has a couple of bugs fixed and goes a it further with
> cleaning things up.
> 
> ---
>  drivers/cpufreq/cpufreq.c |  108 +++++++++++++++++++++++-----------------------
>  include/linux/cpufreq.h   |    1 
>  2 files changed, 56 insertions(+), 53 deletions(-)

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

-- 
viresh

Re: [PATCH v3] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

On Mon, Jul 27, 2015 at 4:29 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26-07-15, 02:28, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> After commit 87549141d516 (cpufreq: Stop migrating sysfs files on
>> hotplug) there is a problem with CPUs that share cpufreq policy
>> objects with other CPUs and are initially offline.
>>
>> Say CPU1 shares a policy with CPU0 which is online and is registered
>> first.  As part of the registration process, cpufreq_add_dev() is
>> called for it.  It creates the policy object and a symbolic link
>> to it from the CPU1's sysfs directory.  If CPU1 is registered
>> subsequently and it is offline at that time, cpufreq_add_dev() will
>> attempt to create a symbolic link to the policy object for it, but
>> that link is present already, so a warning about that will be
>> triggered.
>>
>> To avoid that warning, make cpufreq use an additional CPU mask
>> containing related CPUs that are actually present for each policy
>> object.  That mask is initialized when the policy object is populated
>> after its creation (for the first online CPU using it) and it includes
>> CPUs from the "policy CPUs" mask returned by the cpufreq driver's
>> ->init() callback that are physically present at that time.  Symbolic
>> links to the policy are created only for the CPUs in that mask.
>>
>> If cpufreq_add_dev() is invoked for an offline CPU, it checks the
>> new mask and only creates the symlink if the CPU was not in it (the
>> CPU is added to the mask at the same time).
>>
>> In turn, cpufreq_remove_dev() drops the given CPU from the new mask,
>> removes its symlink to the policy object and returns, unless it is
>> the CPU owning the policy object.  In that case, the policy object
>> is moved to a new CPU's sysfs directory or deleted if the CPU being
>> removed was the last user of the policy.
>>
>> While at it, notice that cpufreq_remove_dev() can't fail, because
>> its return value is ignored, so make it ignore return values from
>> __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish()
>> and prevent these functions from aborting on errors returned by
>> __cpufreq_governor().  Also drop the now unused sif argument from
>> them.
>>
>> Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug)
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Reported-by: Russell King <linux@arm.linux.org.uk>
>> ---
>>
>> This goes back to the https://patchwork.kernel.org/patch/6856011/
>> idea, but has a couple of bugs fixed and goes a it further with
>> cleaning things up.
>>
>> ---
>>  drivers/cpufreq/cpufreq.c |  108 +++++++++++++++++++++++-----------------------
>>  include/linux/cpufreq.h   |    1
>>  2 files changed, 56 insertions(+), 53 deletions(-)
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks!

Russell, can you test this one, please?

Rafael

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

On Monday, July 27, 2015 07:57:18 AM Viresh Kumar wrote:
> On 26-07-15, 00:46, Rafael J. Wysocki wrote:
> > OK, I'll prepare a new version of that patch then, but as I said this
> > choice means that we'll be creating the links to the policy at the
> > policy creation time going forward.
> 
> Atleast for the rc fix, we should do exactly this. Right.
> 
> But we can rethink about getting both my earlier patches merged for
> 4.3, which did this:
> 
> - Keep adding CPUs to a global mask, which didn't had a existing
>   policy and were offline when subsys-callback for called for them.
> - And then create the links only when the subsys callback is called
>   for CPUs, for which policy already exist, as Russell suggested.

Say the subsys add callback runs for a CPU and it doesn't have a policy.
If it is offline, we ignore it and the add callback won't be executed
for it again.

In turn, if it is online, we create a policy for it and we should (right
away) link the policy to all of the CPUs that were offline when the subsys add
callback was called for them.  That's what we do today.

Is there anything missing in that?

Thanks,
Rafael

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Viresh Kumar]

On 27-07-15, 15:45, Rafael J. Wysocki wrote:
> Say the subsys add callback runs for a CPU and it doesn't have a policy.
> If it is offline, we ignore it and the add callback won't be executed
> for it again.
> 
> In turn, if it is online, we create a policy for it and we should (right
> away) link the policy to all of the CPUs that were offline when the subsys add
> callback was called for them.  That's what we do today.
> 
> Is there anything missing in that?

So the code is working properly after your patch, but I was talking
on the lines of what Russell suggested.

We should play with the links only when we receive add-dev/remove-dev
from subsys callbacks. The exception to that will be the offline CPUs
for which add-dev is called before their policy existed.

-- 
viresh

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

On Monday, July 27, 2015 08:09:35 PM Viresh Kumar wrote:
> On 27-07-15, 15:45, Rafael J. Wysocki wrote:
> > Say the subsys add callback runs for a CPU and it doesn't have a policy.
> > If it is offline, we ignore it and the add callback won't be executed
> > for it again.
> > 
> > In turn, if it is online, we create a policy for it and we should (right
> > away) link the policy to all of the CPUs that were offline when the subsys add
> > callback was called for them.  That's what we do today.
> > 
> > Is there anything missing in that?
> 
> So the code is working properly after your patch, but I was talking
> on the lines of what Russell suggested.
> 
> We should play with the links only when we receive add-dev/remove-dev
> from subsys callbacks. The exception to that will be the offline CPUs
> for which add-dev is called before their policy existed.

The rule is supposed to be "all of the present CPUs which do not own
a policy should point to one, unless it doesn't exist".  The right
approach is then to create links from them to a policy object as soon
as we create one for them.  Waiting for something else to happen is just
pointless and this approach covers both the offline and online CPUs, so
I don't think that changing it would improve things really.

Thanks,
Rafael

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Viresh Kumar]

On 29-07-15, 03:38, Rafael J. Wysocki wrote:
> The rule is supposed to be "all of the present CPUs which do not own
> a policy should point to one, unless it doesn't exist".  The right
> approach is then to create links from them to a policy object as soon
> as we create one for them.  Waiting for something else to happen is just
> pointless and this approach covers both the offline and online CPUs, so
> I don't think that changing it would improve things really.

Ack.

-- 
viresh

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Russell King - ARM Linux]

On Wed, Jul 29, 2015 at 03:38:03AM +0200, Rafael J. Wysocki wrote:
> On Monday, July 27, 2015 08:09:35 PM Viresh Kumar wrote:
> > On 27-07-15, 15:45, Rafael J. Wysocki wrote:
> > > Say the subsys add callback runs for a CPU and it doesn't have a policy.
> > > If it is offline, we ignore it and the add callback won't be executed
> > > for it again.
> > > 
> > > In turn, if it is online, we create a policy for it and we should (right
> > > away) link the policy to all of the CPUs that were offline when the subsys add
> > > callback was called for them.  That's what we do today.
> > > 
> > > Is there anything missing in that?
> > 
> > So the code is working properly after your patch, but I was talking
> > on the lines of what Russell suggested.
> > 
> > We should play with the links only when we receive add-dev/remove-dev
> > from subsys callbacks. The exception to that will be the offline CPUs
> > for which add-dev is called before their policy existed.
> 
> The rule is supposed to be "all of the present CPUs which do not own
> a policy should point to one, unless it doesn't exist".  The right
> approach is then to create links from them to a policy object as soon
> as we create one for them.  Waiting for something else to happen is just
> pointless and this approach covers both the offline and online CPUs, so
> I don't think that changing it would improve things really.

I'm not sure we disagree with that.  It's more about when the symlinks
are created, and when you define that a CPU exists.

If you're attaching to subsystem in sysfs, then the point that the
subsystem interface gets to know about a sysfs node existing is when
it's add_dev method is called - it should not assume that a node exists
prior to that point, otherwise things are racy.

Consider a policy initialisation in parallel with an update of the
CPU present map and adding a CPU to sysfs.  The CPU present map will be
updated first, and then it will be added to sysfs.  If you're initialising
a cpufreq policy in the middle of that and creating symlinks for all
present CPUs, there's a window where the CPU present map indicates that
a CPU is present, but there is no sysfs directory for you to create a
symlink in.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

Hi Russell,

On Wed, Jul 29, 2015 at 11:11 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jul 29, 2015 at 03:38:03AM +0200, Rafael J. Wysocki wrote:
>> On Monday, July 27, 2015 08:09:35 PM Viresh Kumar wrote:
>> > On 27-07-15, 15:45, Rafael J. Wysocki wrote:
>> > > Say the subsys add callback runs for a CPU and it doesn't have a policy.
>> > > If it is offline, we ignore it and the add callback won't be executed
>> > > for it again.
>> > >
>> > > In turn, if it is online, we create a policy for it and we should (right
>> > > away) link the policy to all of the CPUs that were offline when the subsys add
>> > > callback was called for them.  That's what we do today.
>> > >
>> > > Is there anything missing in that?
>> >
>> > So the code is working properly after your patch, but I was talking
>> > on the lines of what Russell suggested.
>> >
>> > We should play with the links only when we receive add-dev/remove-dev
>> > from subsys callbacks. The exception to that will be the offline CPUs
>> > for which add-dev is called before their policy existed.
>>
>> The rule is supposed to be "all of the present CPUs which do not own
>> a policy should point to one, unless it doesn't exist".  The right
>> approach is then to create links from them to a policy object as soon
>> as we create one for them.  Waiting for something else to happen is just
>> pointless and this approach covers both the offline and online CPUs, so
>> I don't think that changing it would improve things really.
>
> I'm not sure we disagree with that.  It's more about when the symlinks
> are created, and when you define that a CPU exists.
>
> If you're attaching to subsystem in sysfs, then the point that the
> subsystem interface gets to know about a sysfs node existing is when
> it's add_dev method is called - it should not assume that a node exists
> prior to that point, otherwise things are racy.
>
> Consider a policy initialisation in parallel with an update of the
> CPU present map and adding a CPU to sysfs.  The CPU present map will be
> updated first, and then it will be added to sysfs.  If you're initialising
> a cpufreq policy in the middle of that and creating symlinks for all
> present CPUs, there's a window where the CPU present map indicates that
> a CPU is present, but there is no sysfs directory for you to create a
> symlink in.

In practice, this means a cpufreq driver registration done in parallel
with CPU hotplug.

Indeed, there is a small race window in that case, but it can be
closed easily by making cpufreq driver registration and CPU hotplug
mutually exclusive (we do that already for cpufreq driver
unregistration in linux-next anyway).

Thanks,
Rafael

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Viresh Kumar]

On 29-07-15, 15:57, Rafael J. Wysocki wrote:
> In practice, this means a cpufreq driver registration done in parallel
> with CPU hotplug.

Not necessarily. Also consider the case where cpufreq driver is already working
for a set of CPUs. And a new set of CPUs (that will share the policy) are
getting physically added to the system.

Anyway, even if there is no problem at all, I do agree with Russell that it will
be better to do operations on behalf of the devices only when we are requested
for those devices.

-- 
viresh

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Rafael J. Wysocki]

Hi Viresh,

On Wed, Jul 29, 2015 at 4:21 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 29-07-15, 15:57, Rafael J. Wysocki wrote:
>> In practice, this means a cpufreq driver registration done in parallel
>> with CPU hotplug.
>
> Not necessarily. Also consider the case where cpufreq driver is already working
> for a set of CPUs. And a new set of CPUs (that will share the policy) are
> getting physically added to the system.

That's what I mean by "hotplug" (as opposed to online/offline).

Adding a CPU involves (a) updating the present map and (b) registering
the CPU device and both are carried out together with CPU
online/offline locked.  At least that's the expectation and I'm not
aware of any architecture doing that differently.

So locking CPU online/offline around the cpufreq driver registration
would close that race and there's one more reason for doing that
(other than we do it for the cpufreq driver unregistration as I said
before): we need the CPU *online* map to be stable too while we're
registering the cpufreq driver.

> Anyway, even if there is no problem at all, I do agree with Russell that it will
> be better to do operations on behalf of the devices only when we are requested
> for those devices.

Problem is, we can't do that for all of them.  If the CPU is ofline
while being registered (the common case for hot-add) and it doesn't
have a policy already, we can't link it to an existing policy anyway,
so that operation has to be carried out later.  That is, we need to
create links for those CPUs after the policy has been created in any
case.

Moreover, the only case when we need to create links for online CPUs
is the registration of a cpufreq driver, because only then we can see
online CPUs that should be linked to a policy, but aren't.  It takes
less code to treat them in the same way as the offline CPUs at that
point (and create the links for them right after the policy has been
created) than to defer it to until sif->add() is called for each of
them, because we know that sif->add() is practically guaraneed to
succeeed for them (this is the code path in which we call
cpufreq_add_policy_cpu() and the governor "stop" only fails if it
hasn't been started for that policy).

Thanks,
Rafael

Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links

[Viresh Kumar]

I am not going to fight hard to prove a point as the code is in good
working conditions, but wanted to finish the discussion ..

On 29-07-15, 22:32, Rafael J. Wysocki wrote:
> On Wed, Jul 29, 2015 at 4:21 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 29-07-15, 15:57, Rafael J. Wysocki wrote:
> >> In practice, this means a cpufreq driver registration done in parallel
> >> with CPU hotplug.
> >
> > Not necessarily. Also consider the case where cpufreq driver is already working
> > for a set of CPUs. And a new set of CPUs (that will share the policy) are
> > getting physically added to the system.
> 
> That's what I mean by "hotplug" (as opposed to online/offline).

You were talking about driver registration done in parallel with
hotplug. But I was pointing at something else.

Suppose a system that has:
- 8 CPUs, 0-3 and 4-7 share policy
- 4-7 physically hotplugged out
- cpufreq driver registered and so policy0 active for cpu 0-3.
- We hotplug 4-7.

So, this is a bit different compared to the case where Russell mentioned
the Racy thing. Not sure if this case also has similary racy situations
though.

> Problem is, we can't do that for all of them.

Right.

> If the CPU is ofline
> while being registered (the common case for hot-add) and it doesn't
> have a policy already, we can't link it to an existing policy anyway,
> so that operation has to be carried out later.  That is, we need to
> create links for those CPUs after the policy has been created in any
> case.

Right, but at least the cpufreq-core is already requested on behalf of
such CPUs. We aren't creating links based on the assumption that a
add_dev() will be called for these devices.

> Moreover, the only case when we need to create links for online CPUs

offline CPUs as well..

> is the registration of a cpufreq driver, because only then we can see
> online CPUs that should be linked to a policy, but aren't.  It takes
> less code to treat them in the same way as the offline CPUs at that
> point (and create the links for them right after the policy has been
> created) than to defer it to until sif->add() is called for each of
> them, because we know that sif->add() is practically guaraneed to
> succeeed for them (this is the code path in which we call
> cpufreq_add_policy_cpu() and the governor "stop" only fails if it
> hasn't been started for that policy).

Choose whatever you feel is right. I have already written below code, so
just pasting it here. Its not to say, that this code looks better :P

---
 drivers/cpufreq/cpufreq.c | 108 +++++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 60 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 24e4ba568e77..87faabce777d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -31,6 +31,12 @@
 #include <linux/tick.h>
 #include <trace/events/power.h>
 
+/*
+ * CPUs that were offline when a request to allocate policy was issued, symlinks
+ * for them should be created once the policy is available for them.
+ */
+cpumask_t real_cpus_pending;
+
 static LIST_HEAD(cpufreq_policy_list);
 
 static inline bool policy_is_inactive(struct cpufreq_policy *policy)
@@ -941,62 +947,46 @@ EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
 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;
+	int ret;
 
 	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);
+	dev_dbg(cpu_dev, "%s: Adding symlink for CPU: %u\n", __func__, cpu);
 
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return;
+	ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+	if (ret)
+		dev_err(cpu_dev, "%s: Failed to create link (%d)\n", __func__,
+			ret);
+	else
+		cpumask_set_cpu(cpu, policy->real_cpus);
 
-	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
+	return ret;
 }
 
-/* Add/remove symlinks for all related CPUs */
-static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
+/*
+ * Create symlinks for CPUs which are already added via subsys callbacks (and
+ * were offline then), before the policy was created.
+ */
+static int cpufreq_add_pending_symlinks(struct cpufreq_policy *policy)
 {
-	unsigned int j;
-	int ret = 0;
+	struct cpumask mask;
+	int cpu, ret;
+
+	cpumask_and(&mask, policy->related_cpus, &real_cpus_pending);
 
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus) {
-		if (j == policy->kobj_cpu)
-			continue;
+	if (cpumask_empty(&mask))
+		return 0;
 
-		ret = add_cpu_dev_symlink(policy, j);
+	for_each_cpu(cpu, &mask) {
+		ret = add_cpu_dev_symlink(policy, cpu);
 		if (ret)
-			break;
+			return ret;
+		cpumask_clear_cpu(cpu, &real_cpus_pending);
 	}
 
-	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(j, policy->real_cpus) {
-		if (j == policy->kobj_cpu)
-			continue;
-
-		remove_cpu_dev_symlink(policy, j);
-	}
+	return 0;
 }
 
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
@@ -1028,7 +1018,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
 			return ret;
 	}
 
-	return cpufreq_add_dev_symlink(policy);
+	return cpufreq_add_pending_symlinks(policy);
 }
 
 static int cpufreq_init_policy(struct cpufreq_policy *policy)
@@ -1155,7 +1145,6 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify)
 					     CPUFREQ_REMOVE_POLICY, policy);
 
 	down_write(&policy->rwsem);
-	cpufreq_remove_dev_symlink(policy);
 	kobj = &policy->kobj;
 	cmp = &policy->kobj_unregister;
 	up_write(&policy->rwsem);
@@ -1235,10 +1224,9 @@ static int cpufreq_online(unsigned int cpu)
 	down_write(&policy->rwsem);
 
 	if (new_policy) {
+		cpumask_copy(policy->real_cpus, cpumask_of(cpu));
 		/* 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);
 	}
 
 	/*
@@ -1363,23 +1351,17 @@ static int cpufreq_online(unsigned int cpu)
 static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	unsigned cpu = dev->id;
-	int ret;
+	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+	int ret = 0;
 
 	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
 
-	if (cpu_online(cpu)) {
+	if (policy)
+		ret = add_cpu_dev_symlink(policy, cpu);
+	else 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;
-	}
+	else
+		cpumask_set_cpu(cpu, &real_cpus_pending);
 
 	return ret;
 }
@@ -1469,8 +1451,10 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	unsigned int cpu = dev->id;
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
-	if (!policy)
+	if (!policy) {
+		cpumask_clear_cpu(cpu, &real_cpus_pending);
 		return 0;
+	}
 
 	if (cpu_online(cpu)) {
 		cpufreq_offline_prepare(cpu);
@@ -1485,7 +1469,8 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	}
 
 	if (cpu != policy->kobj_cpu) {
-		remove_cpu_dev_symlink(policy, cpu);
+		dev_dbg(dev, "%s: Removing symlink\n", __func__);
+		sysfs_remove_link(&dev->kobj, "cpufreq");
 	} else {
 		/*
 		 * The CPU owning the policy object is going away.  Move it to
@@ -2550,6 +2535,9 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
 	if (cpufreq_boost_supported())
 		cpufreq_sysfs_remove_file(&boost.attr);
 
+	if (WARN_ON(!cpumask_empty(&real_cpus_pending)))
+		cpumask_clear(&real_cpus_pending);
+
 	unregister_hotcpu_notifier(&cpufreq_cpu_notifier);
 
 	write_lock_irqsave(&cpufreq_driver_lock, flags);