Skip to contentSkip to conversation

cpufreq: create sysfs symlink for cpus onlined after boot

PATCH In review Prashanth Prakash 4 participants 10 messages Mar 23, 2017 – Apr 10, 2017 CPU frequency scaling framework

lore
Share & tools
lore

Prashanth Prakash

author
#0 lore raw

PATCH 1/1 cpufreq: create sysfs symlink for cpus onlined after boot

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Signed-off-by Prashanth Prakash <[email protected]>
---
 drivers/cpufreq/cpufreq.c | 63 +++++++++++++++++++++++++++++++++++------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 47 insertions(+), 17 deletions(-)
drivers/cpufreq/cpufreq.c +47 -16
index 5dbdd26..e7d8ad5 100644
@@ -62,6 +62,7 @@ static inline bool policy_is_inactive(struct cpufreq_policy *policy)
  * also protects the cpufreq_cpu_data array.
  */
 static struct cpufreq_driver *cpufreq_driver;
+static cpumask_var_t cpufreq_real_cpus; /* mask of real/registered CPUs  */
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
 

@@ -1048,14 +1049,11 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	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,
 				   cpufreq_global_kobject, "policy%u", cpu);
 	if (ret) {
 		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
-		goto err_free_real_cpus;
+		goto err_free_rcpumask;
 	}
 
 	INIT_LIST_HEAD(&policy->policy_list);

@@ -1068,8 +1066,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	policy->cpu = cpu;
 	return policy;
 
-err_free_real_cpus:
-	free_cpumask_var(policy->real_cpus);
 err_free_rcpumask:
 	free_cpumask_var(policy->related_cpus);
 err_free_cpumask:

@@ -1116,7 +1112,6 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	cpufreq_policy_put_kobj(policy);
-	free_cpumask_var(policy->real_cpus);
 	free_cpumask_var(policy->related_cpus);
 	free_cpumask_var(policy->cpus);
 	kfree(policy);

@@ -1181,8 +1176,16 @@ static int cpufreq_online(unsigned int cpu)
 		policy->user_policy.max = policy->max;
 
 		write_lock_irqsave(&cpufreq_driver_lock, flags);
-		for_each_cpu(j, policy->related_cpus)
+		for_each_cpu(j, policy->related_cpus) {
+			/* Setup sysfs links only for real CPUs */
+			if (cpumask_test_cpu(j, cpufreq_real_cpus)) {
+				ret = add_cpu_dev_symlink(policy,
+							get_cpu_device(j));
+				if (ret)
+					goto out_delete_symlinks;
+			}
 			per_cpu(cpufreq_cpu_data, j) = policy;
+		}
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	} else {
 		policy->min = policy->user_policy.min;

@@ -1270,6 +1273,13 @@ static int cpufreq_online(unsigned int cpu)
 
 	return 0;
 
+out_delete_symlinks:
+	for_each_cpu(j, policy->related_cpus) {
+		if (per_cpu(cpufreq_cpu_data, j) &&
+			cpumask_test_cpu(j, cpufreq_real_cpus))
+			remove_cpu_dev_symlink(policy, get_cpu_device(j));
+		per_cpu(cpufreq_cpu_data, j) = NULL;
+	}
 out_exit_policy:
 	up_write(&policy->rwsem);
 

@@ -1301,14 +1311,22 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 			return ret;
 	}
 
-	/* Create sysfs link on CPU registration */
+	if (cpumask_test_and_set_cpu(cpu, cpufreq_real_cpus))
+		return 0;
+
+	/*
+	 * sysfs links will be created on CPU registration <OR>
+	 * if a CPU was offline during registration (when maxcpus=N is used)
+	 * then sysfs links will be created when a new policy is created in
+	 * cpufreq_online
+	 */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
-	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
+	if (!policy)
 		return 0;
 
 	ret = add_cpu_dev_symlink(policy, dev);
 	if (ret) {
-		cpumask_clear_cpu(cpu, policy->real_cpus);
+		cpumask_clear_cpu(cpu, cpufreq_real_cpus);
 		cpufreq_offline(cpu);
 	}
 

@@ -1384,7 +1402,7 @@ static int cpufreq_offline(unsigned int cpu)
  */
 static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 {
-	unsigned int cpu = dev->id;
+	unsigned int cpu = dev->id, i;
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
 	if (!policy)

@@ -1393,11 +1411,14 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	if (cpu_online(cpu))
 		cpufreq_offline(cpu);
 
-	cpumask_clear_cpu(cpu, policy->real_cpus);
-	remove_cpu_dev_symlink(policy, dev);
+	if (cpumask_test_and_clear_cpu(cpu, cpufreq_real_cpus))
+		remove_cpu_dev_symlink(policy, dev);
 
-	if (cpumask_empty(policy->real_cpus))
-		cpufreq_policy_free(policy);
+	for_each_cpu(i, policy->related_cpus) {
+		if (cpumask_test_cpu(i, cpufreq_real_cpus))
+			return;
+	}
+	cpufreq_policy_free(policy);
 }
 
 /**

@@ -2530,6 +2551,9 @@ static int __init cpufreq_core_init(void)
 	if (cpufreq_disabled())
 		return -ENODEV;
 
+	if (!zalloc_cpumask_var(&cpufreq_real_cpus, GFP_KERNEL))
+		return -ENOMEM;
+
 	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
 	BUG_ON(!cpufreq_global_kobject);
 

@@ -2537,5 +2561,12 @@ static int __init cpufreq_core_init(void)
 
 	return 0;
 }
+
+static void __exit cpufreq_core_exit(void)
+{
+	free_cpumask_var(cpufreq_real_cpus);
+}
+
 module_param(off, int, 0444);
 core_initcall(cpufreq_core_init);
+module_exit(cpufreq_core_exit);
include/linux/cpufreq.h +0 -1
index 87165f0..86e0bd5 100644
@@ -66,7 +66,6 @@ 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 */
Signature · 3 lines
Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
maintainer
#1 lore raw
Quoted 10 lines · Prashanth Prakash

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Signed-off-by Prashanth Prakash <[email protected]>

This looks way overly complicated to me. Let me look at it in the next couple of days.

Thanks,
Rafael

author
#2 lore raw
Quoted 6 lines · Rafael J. Wysocki
Quoted 4 lines · Prashanth Prakash

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

Quoted 8 lines
Quoted 6 lines

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Signed-off-by Prashanth Prakash <[email protected]>

This looks way overly complicated to me. Let me look at it in the next couple of days.

Sure. Thanks!

#3 lore raw
Quoted 12 lines · Prashanth
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Prashanth Prakash

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Prashanth Prakash <[email protected]>

This looks way overly complicated to me. Let me look at it in the next couple of days.

Sure. Thanks!

So the patch below works for me. Please test.

The problem is that we only try to create links once, when CPUs are registered or when the cpufreq driver is registered (depending on which happens first), but if all CPUs that would share a policy are offline at that time, the policy is not there and we don't create the links at all.

This means we also need to try to create the links when creating a new policy (because the CPUs may have been added without the links at this point already).

A slight side effect of this is that failures to create links are not regarded as hard errors now and only cause messages to be printed, but I don't see how that could be a big issue.

---
 drivers/cpufreq/cpufreq.c |   30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)
drivers/cpufreq/cpufreq.c +18 -12
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
@@ -918,11 +918,20 @@ static struct kobj_type ktype_cpufreq =
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
-			       struct device *dev)
+static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu)
 {
+	struct device *dev;
+
+	if (cpumask_test_and_set_cpu(cpu, policy->real_cpus))
+		return;
+
+	dev = get_cpu_device(cpu);
+	if (!dev)
+		return;
+
 	dev_dbg(dev, "%s: Adding symlink\n", __func__);
-	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+	if (sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"))
+		dev_err(dev, "cpufreq symlink creation failed\n");
 }
 
 static void remove_cpu_dev_symlink(struct cpufreq_policy *policy,

@@ -1184,6 +1193,9 @@ static int cpufreq_online(unsigned int c
 		for_each_cpu(j, policy->related_cpus)
 			per_cpu(cpufreq_cpu_data, j) = policy;
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+		for_each_cpu(j, policy->related_cpus)
+			add_cpu_dev_symlink(policy, j);
 	} else {
 		policy->min = policy->user_policy.min;
 		policy->max = policy->user_policy.max;

@@ -1303,16 +1315,10 @@ static int cpufreq_add_dev(struct device
 
 	/* Create sysfs link on CPU registration */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
-	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
-		return 0;
+	if (policy)
+		add_cpu_dev_symlink(policy, cpu);
 
-	ret = add_cpu_dev_symlink(policy, dev);
-	if (ret) {
-		cpumask_clear_cpu(cpu, policy->real_cpus);
-		cpufreq_offline(cpu);
-	}
-
-	return ret;
+	return 0;
 }
 
 static int cpufreq_offline(unsigned int cpu)
#4 lore raw
Quoted 25 lines · Rafael J. Wysocki
Quoted 12 lines · Prashanth
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Prashanth Prakash

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Prashanth Prakash <[email protected]>

This looks way overly complicated to me. Let me look at it in the next couple of days.

Sure. Thanks!

So the patch below works for me. Please test.

The problem is that we only try to create links once, when CPUs are registered or when the cpufreq driver is registered (depending on which happens first), but if all CPUs that would share a policy are offline at that time, the policy is not there and we don't create the links at all.

Quoted 6 lines

This means we also need to try to create the links when creating a new policy (because the CPUs may have been added without the links at this point already).

A slight side effect of this is that failures to create links are not regarded as hard errors now and only cause messages to be printed, but I don't see how that could be a big issue.

Actually, the rwlock around the for_each_cpu() loop in cpufreq_online() doesn't matter AFAICS, so the code can be slightly simplified by dropping it.

---
 drivers/cpufreq/cpufreq.c |   33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)
drivers/cpufreq/cpufreq.c +18 -15
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
@@ -918,11 +918,20 @@ static struct kobj_type ktype_cpufreq =
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
-			       struct device *dev)
+static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu)
 {
+	struct device *dev;
+
+	if (cpumask_test_and_set_cpu(cpu, policy->real_cpus))
+		return;
+
+	dev = get_cpu_device(cpu);
+	if (!dev)
+		return;
+
 	dev_dbg(dev, "%s: Adding symlink\n", __func__);
-	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+	if (sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"))
+		dev_err(dev, "cpufreq symlink creation failed\n");
 }
 
 static void remove_cpu_dev_symlink(struct cpufreq_policy *policy,

@@ -1180,10 +1189,10 @@ static int cpufreq_online(unsigned int c
 		policy->user_policy.min = policy->min;
 		policy->user_policy.max = policy->max;
 
-		write_lock_irqsave(&cpufreq_driver_lock, flags);
-		for_each_cpu(j, policy->related_cpus)
+		for_each_cpu(j, policy->related_cpus) {
 			per_cpu(cpufreq_cpu_data, j) = policy;
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+			add_cpu_dev_symlink(policy, j);
+		}
 	} else {
 		policy->min = policy->user_policy.min;
 		policy->max = policy->user_policy.max;

@@ -1303,16 +1312,10 @@ static int cpufreq_add_dev(struct device
 
 	/* Create sysfs link on CPU registration */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
-	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
-		return 0;
-
-	ret = add_cpu_dev_symlink(policy, dev);
-	if (ret) {
-		cpumask_clear_cpu(cpu, policy->real_cpus);
-		cpufreq_offline(cpu);
-	}
+	if (policy)
+		add_cpu_dev_symlink(policy, cpu);
 
-	return ret;
+	return 0;
 }
 
 static int cpufreq_offline(unsigned int cpu)
#5 lore raw
Quoted 29 lines · Rafael J. Wysocki
Quoted 28 lines · Rafael J. Wysocki
Quoted 12 lines · Prashanth
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Prashanth Prakash

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Prashanth Prakash <[email protected]>

This looks way overly complicated to me. Let me look at it in the next couple of days.

Sure. Thanks!

So the patch below works for me. Please test.

The problem is that we only try to create links once, when CPUs are registered or when the cpufreq driver is registered (depending on which happens first), but if all CPUs that would share a policy are offline at that time, the policy is not there and we don't create the links at all.

This means we also need to try to create the links when creating a new policy (because the CPUs may have been added without the links at this point already).

Quoted 6 lines
Quoted 3 lines

A slight side effect of this is that failures to create links are not regarded as hard errors now and only cause messages to be printed, but I don't see how that could be a big issue.

Actually, the rwlock around the for_each_cpu() loop in cpufreq_online() doesn't matter AFAICS, so the code can be slightly simplified by dropping it.

And I forgot about the cleanup on errors in cpufreq_online(), so one more update follows (with a changelog and all).

---
From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] cpufreq: Fix creation of symbolic links to policy directories

The cpufreq core only tries to create symbolic links from CPU directories in sysfs to policy directories in cpufreq_add_dev(), either when a given CPU is registered or when the cpufreq driver is registered, whichever happens first. That is not sufficient, however, because cpufreq_add_dev() may be called for an offline CPU whose policy object has not been created yet and, quite obviously, the symbolic cannot be added in that case.

Fix that by making cpufreq_online() attempt to add symbolic links to policy objects for the CPUs in the related_cpus mask of every new policy object created by it.

The cpufreq_driver_lock locking around the for_each_cpu() loop in cpufreq_online() is dropped, because it is not necessary and the code is somewhat simpler without it. Moreover, failures to create a symbolic link will not be regarded as hard errors any more and the CPUs without those links will not be taken offline automatically, but that should not be problematic in practice.

Reported-by Prashanth Prakash <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/cpufreq/cpufreq.c |   35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)
drivers/cpufreq/cpufreq.c +20 -15
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
@@ -918,11 +918,19 @@ static struct kobj_type ktype_cpufreq =
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
-			       struct device *dev)
+static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu)
 {
+	struct device *dev = get_cpu_device(cpu);
+
+	if (!dev)
+		return;
+
+	if (cpumask_test_and_set_cpu(cpu, policy->real_cpus))
+		return;
+
 	dev_dbg(dev, "%s: Adding symlink\n", __func__);
-	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+	if (sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"))
+		dev_err(dev, "cpufreq symlink creation failed\n");
 }
 
 static void remove_cpu_dev_symlink(struct cpufreq_policy *policy,

@@ -1180,10 +1188,10 @@ static int cpufreq_online(unsigned int c
 		policy->user_policy.min = policy->min;
 		policy->user_policy.max = policy->max;
 
-		write_lock_irqsave(&cpufreq_driver_lock, flags);
-		for_each_cpu(j, policy->related_cpus)
+		for_each_cpu(j, policy->related_cpus) {
 			per_cpu(cpufreq_cpu_data, j) = policy;
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+			add_cpu_dev_symlink(policy, j);
+		}
 	} else {
 		policy->min = policy->user_policy.min;
 		policy->max = policy->user_policy.max;

@@ -1271,6 +1279,9 @@ static int cpufreq_online(unsigned int c
 	return 0;
 
 out_exit_policy:
+	for_each_cpu(j, policy->real_cpus)
+		remove_cpu_dev_symlink(policy, j);
+
 	up_write(&policy->rwsem);
 
 	if (cpufreq_driver->exit)

@@ -1303,16 +1314,10 @@ static int cpufreq_add_dev(struct device
 
 	/* Create sysfs link on CPU registration */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
-	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
-		return 0;
-
-	ret = add_cpu_dev_symlink(policy, dev);
-	if (ret) {
-		cpumask_clear_cpu(cpu, policy->real_cpus);
-		cpufreq_offline(cpu);
-	}
+	if (policy)
+		add_cpu_dev_symlink(policy, cpu);
 
-	return ret;
+	return 0;
 }
 
 static int cpufreq_offline(unsigned int cpu)
#6 lore raw
Quoted 34 lines · Rafael J. Wysocki
Quoted 33 lines · Rafael J. Wysocki
Quoted 31 lines · Rafael J. Wysocki
Quoted 12 lines · Prashanth
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Prashanth Prakash

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Prashanth Prakash <[email protected]>

This looks way overly complicated to me. Let me look at it in the next couple of days.

Sure. Thanks!

So the patch below works for me. Please test.

The problem is that we only try to create links once, when CPUs are registered or when the cpufreq driver is registered (depending on which happens first), but if all CPUs that would share a policy are offline at that time, the policy is not there and we don't create the links at all.

This means we also need to try to create the links when creating a new policy (because the CPUs may have been added without the links at this point already).

A slight side effect of this is that failures to create links are not regarded as hard errors now and only cause messages to be printed, but I don't see how that could be a big issue.

Quoted 5 lines
Quoted 2 lines

Actually, the rwlock around the for_each_cpu() loop in cpufreq_online() doesn't matter AFAICS, so the code can be slightly simplified by dropping it.

And I forgot about the cleanup on errors in cpufreq_online(), so one more update follows (with a changelog and all).

One more update, sorry.

remove_cpu_dev_symlink() needs a device pointer, not a CPU number. Also the links cleanup need not be done under policy->rwsem.

---
From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] cpufreq: Fix creation of symbolic links to policy directories

The cpufreq core only tries to create symbolic links from CPU directories in sysfs to policy directories in cpufreq_add_dev(), either when a given CPU is registered or when the cpufreq driver is registered, whichever happens first. That is not sufficient, however, because cpufreq_add_dev() may be called for an offline CPU whose policy object has not been created yet and, quite obviously, the symbolic cannot be added in that case.

Fix that by making cpufreq_online() attempt to add symbolic links to policy objects for the CPUs in the related_cpus mask of every new policy object created by it.

The cpufreq_driver_lock locking around the for_each_cpu() loop in cpufreq_online() is dropped, because it is not necessary and the code is somewhat simpler without it. Moreover, failures to create a symbolic link will not be regarded as hard errors any more and the CPUs without those links will not be taken offline automatically, but that should not be problematic in practice.

Reported-by Prashanth Prakash <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/cpufreq/cpufreq.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)
drivers/cpufreq/cpufreq.c +21 -17
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
@@ -918,11 +918,19 @@ static struct kobj_type ktype_cpufreq =
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
-			       struct device *dev)
+static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu)
 {
+	struct device *dev = get_cpu_device(cpu);
+
+	if (!dev)
+		return;
+
+	if (cpumask_test_and_set_cpu(cpu, policy->real_cpus))
+		return;
+
 	dev_dbg(dev, "%s: Adding symlink\n", __func__);
-	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+	if (sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"))
+		dev_err(dev, "cpufreq symlink creation failed\n");
 }
 
 static void remove_cpu_dev_symlink(struct cpufreq_policy *policy,

@@ -1180,10 +1188,10 @@ static int cpufreq_online(unsigned int c
 		policy->user_policy.min = policy->min;
 		policy->user_policy.max = policy->max;
 
-		write_lock_irqsave(&cpufreq_driver_lock, flags);
-		for_each_cpu(j, policy->related_cpus)
+		for_each_cpu(j, policy->related_cpus) {
 			per_cpu(cpufreq_cpu_data, j) = policy;
-		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+			add_cpu_dev_symlink(policy, j);
+		}
 	} else {
 		policy->min = policy->user_policy.min;
 		policy->max = policy->user_policy.max;

@@ -1275,13 +1283,15 @@ out_exit_policy:
 
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
+
+	for_each_cpu(j, policy->real_cpus)
+		remove_cpu_dev_symlink(policy, get_cpu_device(j));
+
 out_free_policy:
 	cpufreq_policy_free(policy);
 	return ret;
 }
 
-static int cpufreq_offline(unsigned int cpu);
-
 /**
  * cpufreq_add_dev - the cpufreq interface for a CPU device.
  * @dev: CPU device.

@@ -1303,16 +1313,10 @@ static int cpufreq_add_dev(struct device
 
 	/* Create sysfs link on CPU registration */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
-	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
-		return 0;
+	if (policy)
+		add_cpu_dev_symlink(policy, cpu);
 
-	ret = add_cpu_dev_symlink(policy, dev);
-	if (ret) {
-		cpumask_clear_cpu(cpu, policy->real_cpus);
-		cpufreq_offline(cpu);
-	}
-
-	return ret;
+	return 0;
 }
 
 static int cpufreq_offline(unsigned int cpu)
author
#7 lore raw

Hi Rafael,

Quoted 65 lines · Rafael J. Wysocki
Quoted 29 lines · Rafael J. Wysocki
Quoted 28 lines · Rafael J. Wysocki
Quoted 27 lines · Rafael J. Wysocki
Quoted 12 lines · Prashanth
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Prashanth Prakash

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Prashanth Prakash <[email protected]>

This looks way overly complicated to me. Let me look at it in the next couple of days.

Sure. Thanks!

So the patch below works for me. Please test.

The problem is that we only try to create links once, when CPUs are registered or when the cpufreq driver is registered (depending on which happens first), but if all CPUs that would share a policy are offline at that time, the policy is not there and we don't create the links at all.

This means we also need to try to create the links when creating a new policy (because the CPUs may have been added without the links at this point already).

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

A slight side effect of this is that failures to create links are not regarded as hard errors now and only cause messages to be printed, but I don't see how that could be a big issue.

Actually, the rwlock around the for_each_cpu() loop in cpufreq_online() doesn't matter AFAICS, so the code can be slightly simplified by dropping it.

And I forgot about the cleanup on errors in cpufreq_online(), so one more update follows (with a changelog and all).

One more update, sorry.

remove_cpu_dev_symlink() needs a device pointer, not a CPU number. Also the links cleanup need not be done under policy->rwsem.

---
From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] cpufreq: Fix creation of symbolic links to policy directories

The cpufreq core only tries to create symbolic links from CPU directories in sysfs to policy directories in cpufreq_add_dev(), either when a given CPU is registered or when the cpufreq driver is registered, whichever happens first. That is not sufficient, however, because cpufreq_add_dev() may be called for an offline CPU whose policy object has not been created yet and, quite obviously, the symbolic cannot be added in that case.

Fix that by making cpufreq_online() attempt to add symbolic links to policy objects for the CPUs in the related_cpus mask of every new policy object created by it.

The cpufreq_driver_lock locking around the for_each_cpu() loop in cpufreq_online() is dropped, because it is not necessary and the code is somewhat simpler without it. Moreover, failures to create a symbolic link will not be regarded as hard errors any more and the CPUs without those links will not be taken offline automatically, but that should not be problematic in practice.

Quoted 2 lines
Reported-by Prashanth Prakash <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>

Tested on a Qualcomm Centriq board and it works as expected. Thanks!

-Prashanth

#8 lore raw
Quoted 68 lines · Prashanth

Hi Rafael,

Quoted 65 lines · Rafael J. Wysocki
Quoted 29 lines · Rafael J. Wysocki
Quoted 28 lines · Rafael J. Wysocki
Quoted 27 lines · Rafael J. Wysocki
Quoted 12 lines · Prashanth
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Prashanth Prakash

Adds an additional code path within cpufreq_online to create sysfs symlinks for cpus that are bought onilne for the first time after completion of boot.

With maxcpus=N kernel parameter, it is possible to bring additional cpus online after boot. In these cases per-cpu "cpufreq" symlinks are not being created during registration as policy may not exist for the cpus that were offline during boot.

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Prashanth Prakash <[email protected]>

This looks way overly complicated to me. Let me look at it in the next couple of days.

Sure. Thanks!

So the patch below works for me. Please test.

The problem is that we only try to create links once, when CPUs are registered or when the cpufreq driver is registered (depending on which happens first), but if all CPUs that would share a policy are offline at that time, the policy is not there and we don't create the links at all.

This means we also need to try to create the links when creating a new policy (because the CPUs may have been added without the links at this point already).

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

A slight side effect of this is that failures to create links are not regarded as hard errors now and only cause messages to be printed, but I don't see how that could be a big issue.

Actually, the rwlock around the for_each_cpu() loop in cpufreq_online() doesn't matter AFAICS, so the code can be slightly simplified by dropping it.

And I forgot about the cleanup on errors in cpufreq_online(), so one more update follows (with a changelog and all).

One more update, sorry.

remove_cpu_dev_symlink() needs a device pointer, not a CPU number. Also the links cleanup need not be done under policy->rwsem.

---
From: Rafael J. Wysocki <[email protected]>
Subject: [PATCH] cpufreq: Fix creation of symbolic links to policy directories

The cpufreq core only tries to create symbolic links from CPU directories in sysfs to policy directories in cpufreq_add_dev(), either when a given CPU is registered or when the cpufreq driver is registered, whichever happens first. That is not sufficient, however, because cpufreq_add_dev() may be called for an offline CPU whose policy object has not been created yet and, quite obviously, the symbolic cannot be added in that case.

Fix that by making cpufreq_online() attempt to add symbolic links to policy objects for the CPUs in the related_cpus mask of every new policy object created by it.

The cpufreq_driver_lock locking around the for_each_cpu() loop in cpufreq_online() is dropped, because it is not necessary and the code is somewhat simpler without it. Moreover, failures to create a symbolic link will not be regarded as hard errors any more and the CPUs without those links will not be taken offline automatically, but that should not be problematic in practice.

Quoted 4 lines
Quoted 2 lines
Reported-by Prashanth Prakash <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>

Tested on a Qualcomm Centriq board and it works as expected. Thanks!

OK, queued up, thanks!

I'll push it for merging by the end of the week unless someone (Viresh in particular) finds any problems with it.

Thanks,
Rafael

maintainer
#9 lore raw
Quoted 4 lines · Rafael J. Wysocki

OK, queued up, thanks!

I'll push it for merging by the end of the week unless someone (Viresh in particular) finds any problems with it.

Sorry for being late to the party. I don't see any cases where this would break, but maybe we can simplify things by always creating links while onlining a CPU.

drivers/cpufreq/cpufreq.c +4 -11
index bc96d423781a..f700e06d4bad 100644
@@ -922,9 +922,6 @@ static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu)
 {
        struct device *dev = get_cpu_device(cpu);
 
-       if (!dev)
-               return;
-
        if (cpumask_test_and_set_cpu(cpu, policy->real_cpus))
                return;
 

@@ -1013,6 +1010,8 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp
 {
        int ret = 0;
 
+       add_cpu_dev_symlink(policy, cpu);
+
        /* Has this CPU been taken care of already? */
        if (cpumask_test_cpu(cpu, policy->cpus))
                return 0;

@@ -1188,10 +1187,8 @@ static int cpufreq_online(unsigned int cpu)
                policy->user_policy.min = policy->min;
                policy->user_policy.max = policy->max;
 
-               for_each_cpu(j, policy->related_cpus) {
+               for_each_cpu(j, policy->related_cpus)
                        per_cpu(cpufreq_cpu_data, j) = policy;
-                       add_cpu_dev_symlink(policy, j);
-               }
        } else {
                policy->min = policy->user_policy.min;
                policy->max = policy->user_policy.max;

@@ -1266,6 +1263,7 @@ static int cpufreq_online(unsigned int cpu)
                goto out_exit_policy;
        }
 
+       add_cpu_dev_symlink(policy, cpu);
        up_write(&policy->rwsem);
 
        kobject_uevent(&policy->kobj, KOBJ_ADD);

@@ -1311,11 +1309,6 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
                        return ret;
        }
 
-       /* Create sysfs link on CPU registration */
-       policy = per_cpu(cpufreq_cpu_data, cpu);
-       if (policy)
-               add_cpu_dev_symlink(policy, cpu);
-
        return 0;
 }
Signature · 1 line
viresh