[PATCH 1/5] cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask

[PATCH 1/5] cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask

[Viresh Kumar]

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 25c4c15103a0..b32521432db4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1221,7 +1221,7 @@ static int cpufreq_online(unsigned int cpu)
 
 	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
-		cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
+		cpumask_copy(policy->related_cpus, policy->cpus);
 		/* Remember CPUs present at the policy creation time. */
 		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
 	}
-- 
2.4.0

Re: [PATCH 1/5] cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask

[Saravana Kannan]

On 10/11/2015 10:21 AM, Viresh Kumar wrote:
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

The commit text should explain the why you are doing this.

> ---
>   drivers/cpufreq/cpufreq.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 25c4c15103a0..b32521432db4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1221,7 +1221,7 @@ static int cpufreq_online(unsigned int cpu)
>
>   	if (new_policy) {
>   		/* related_cpus should at least include policy->cpus. */
> -		cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
> +		cpumask_copy(policy->related_cpus, policy->cpus);

Again, why? It actually seems wrong. A 4 core cluster could come up with 
just 2 cores when the policy is added. But the related CPUs would be 4 CPUs.

>   		/* Remember CPUs present at the policy creation time. */
>   		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
>   	}
>

-Saravana

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

Re: [PATCH 1/5] cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask

[Viresh Kumar]

On 12-10-15, 12:12, Saravana Kannan wrote:
> >  	if (new_policy) {
> >  		/* related_cpus should at least include policy->cpus. */
> >-		cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
> >+		cpumask_copy(policy->related_cpus, policy->cpus);
> 
> Again, why? It actually seems wrong. A 4 core cluster could come up
> with just 2 cores when the policy is added. But the related CPUs
> would be 4 CPUs.

Firstly, the patch hasn't changed anything at all. related_cpus was
empty until this point, and orring or setting it with ->cpus will
result in the same output.

Secondly, this is what we always wanted. related_cpus should contain
the mask of all possible CPUs for that cluster.

-- 
viresh

Re: [PATCH 1/5] cpufreq: Use cpumask_copy instead of cpumask_or to copy a mask

[Saravana Kannan]

On 10/12/2015 08:23 PM, Viresh Kumar wrote:
> On 12-10-15, 12:12, Saravana Kannan wrote:
>>>   	if (new_policy) {
>>>   		/* related_cpus should at least include policy->cpus. */
>>> -		cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
>>> +		cpumask_copy(policy->related_cpus, policy->cpus);
>>
>> Again, why? It actually seems wrong. A 4 core cluster could come up
>> with just 2 cores when the policy is added. But the related CPUs
>> would be 4 CPUs.
>
> Firstly, the patch hasn't changed anything at all. related_cpus was
> empty until this point, and orring or setting it with ->cpus will
> result in the same output.

I was under the impression that the CPUfreq drivers were expected to 
fill in related_cpus and the or-ing was a safety net. If that's not the 
case, then this change is fine.

> Secondly, this is what we always wanted. related_cpus should contain
> the mask of all possible CPUs for that cluster.

I think the confusion was that I thought the drivers are supposed to do 
this. If this doesn't mess up other CPUfreq drivers that I'm not 
familiar with, then I don't have concerns.

Can you still explain the why in the commit text though? If it's just 
that related_cpus is always empty and copying is more efficient than 
or-ing, then mention that?

Thanks,
Saravana

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

[PATCH 2/5] cpufreq: create cpu/cpufreq at boot time

[Viresh Kumar]

Later patches will need to create policy specific directories in
/sys/devices/system/cpu/cpufreq/ directory and so the cpufreq directory
wouldn't be ever empty.

And so no fun creating/destroying it on need basis anymore. Create it
once on system boot.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c          | 32 ++------------------------------
 drivers/cpufreq/cpufreq_governor.c | 20 +++++---------------
 include/linux/cpufreq.h            |  2 --
 3 files changed, 7 insertions(+), 47 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b32521432db4..db688d18a189 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -883,43 +883,15 @@ static struct kobj_type ktype_cpufreq = {
 struct kobject *cpufreq_global_kobject;
 EXPORT_SYMBOL(cpufreq_global_kobject);
 
-static int cpufreq_global_kobject_usage;
-
-int cpufreq_get_global_kobject(void)
-{
-	if (!cpufreq_global_kobject_usage++)
-		return kobject_add(cpufreq_global_kobject,
-				&cpu_subsys.dev_root->kobj, "%s", "cpufreq");
-
-	return 0;
-}
-EXPORT_SYMBOL(cpufreq_get_global_kobject);
-
-void cpufreq_put_global_kobject(void)
-{
-	if (!--cpufreq_global_kobject_usage)
-		kobject_del(cpufreq_global_kobject);
-}
-EXPORT_SYMBOL(cpufreq_put_global_kobject);
-
 int cpufreq_sysfs_create_file(const struct attribute *attr)
 {
-	int ret = cpufreq_get_global_kobject();
-
-	if (!ret) {
-		ret = sysfs_create_file(cpufreq_global_kobject, attr);
-		if (ret)
-			cpufreq_put_global_kobject();
-	}
-
-	return ret;
+	return sysfs_create_file(cpufreq_global_kobject, attr);
 }
 EXPORT_SYMBOL(cpufreq_sysfs_create_file);
 
 void cpufreq_sysfs_remove_file(const struct attribute *attr)
 {
 	sysfs_remove_file(cpufreq_global_kobject, attr);
-	cpufreq_put_global_kobject();
 }
 EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
 
@@ -2589,7 +2561,7 @@ static int __init cpufreq_core_init(void)
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	cpufreq_global_kobject = kobject_create();
+	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
 	BUG_ON(!cpufreq_global_kobject);
 
 	register_syscore_ops(&cpufreq_syscore_ops);
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 750626d8fb03..11258c4c1b17 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -348,29 +348,21 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
 	set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
 					latency * LATENCY_MULTIPLIER));
 
-	if (!have_governor_per_policy()) {
-		if (WARN_ON(cpufreq_get_global_kobject())) {
-			ret = -EINVAL;
-			goto cdata_exit;
-		}
+	if (!have_governor_per_policy())
 		cdata->gdbs_data = dbs_data;
-	}
 
 	ret = sysfs_create_group(get_governor_parent_kobj(policy),
 				 get_sysfs_attr(dbs_data));
 	if (ret)
-		goto put_kobj;
+		goto reset_gdbs_data;
 
 	policy->governor_data = dbs_data;
 
 	return 0;
 
-put_kobj:
-	if (!have_governor_per_policy()) {
+reset_gdbs_data:
+	if (!have_governor_per_policy())
 		cdata->gdbs_data = NULL;
-		cpufreq_put_global_kobject();
-	}
-cdata_exit:
 	cdata->exit(dbs_data, !policy->governor->initialized);
 free_common_dbs_info:
 	free_common_dbs_info(policy, cdata);
@@ -394,10 +386,8 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
 		sysfs_remove_group(get_governor_parent_kobj(policy),
 				   get_sysfs_attr(dbs_data));
 
-		if (!have_governor_per_policy()) {
+		if (!have_governor_per_policy())
 			cdata->gdbs_data = NULL;
-			cpufreq_put_global_kobject();
-		}
 
 		cdata->exit(dbs_data, policy->governor->initialized == 1);
 		kfree(dbs_data);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index dca22de98d94..338bf0e59bb8 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -149,8 +149,6 @@ static inline bool policy_is_shared(struct cpufreq_policy *policy)
 
 /* /sys/devices/system/cpu/cpufreq: entry point for global variables */
 extern struct kobject *cpufreq_global_kobject;
-int cpufreq_get_global_kobject(void);
-void cpufreq_put_global_kobject(void);
 int cpufreq_sysfs_create_file(const struct attribute *attr);
 void cpufreq_sysfs_remove_file(const struct attribute *attr);
 
-- 
2.4.0

[PATCH 3/5] cpufreq: remove cpufreq_sysfs_{create|remove}_file()

[Viresh Kumar]

They don't do anything special now, remove the unnecessary wrapper.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 22 +++++-----------------
 include/linux/cpufreq.h   |  2 --
 2 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index db688d18a189..2cb0e3b8170e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -880,21 +880,6 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };
 
-struct kobject *cpufreq_global_kobject;
-EXPORT_SYMBOL(cpufreq_global_kobject);
-
-int cpufreq_sysfs_create_file(const struct attribute *attr)
-{
-	return sysfs_create_file(cpufreq_global_kobject, attr);
-}
-EXPORT_SYMBOL(cpufreq_sysfs_create_file);
-
-void cpufreq_sysfs_remove_file(const struct attribute *attr)
-{
-	sysfs_remove_file(cpufreq_global_kobject, attr);
-}
-EXPORT_SYMBOL(cpufreq_sysfs_remove_file);
-
 static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
 {
 	struct device *cpu_dev;
@@ -2397,7 +2382,7 @@ static int create_boost_sysfs_file(void)
 	if (!cpufreq_driver->set_boost)
 		cpufreq_driver->set_boost = cpufreq_boost_set_sw;
 
-	ret = cpufreq_sysfs_create_file(&boost.attr);
+	ret = sysfs_create_file(cpufreq_global_kobject, &boost.attr);
 	if (ret)
 		pr_err("%s: cannot register global BOOST sysfs file\n",
 		       __func__);
@@ -2408,7 +2393,7 @@ static int create_boost_sysfs_file(void)
 static void remove_boost_sysfs_file(void)
 {
 	if (cpufreq_boost_supported())
-		cpufreq_sysfs_remove_file(&boost.attr);
+		sysfs_remove_file(cpufreq_global_kobject, &boost.attr);
 }
 
 int cpufreq_enable_boost_support(void)
@@ -2556,6 +2541,9 @@ static struct syscore_ops cpufreq_syscore_ops = {
 	.shutdown = cpufreq_suspend,
 };
 
+struct kobject *cpufreq_global_kobject;
+EXPORT_SYMBOL(cpufreq_global_kobject);
+
 static int __init cpufreq_core_init(void)
 {
 	if (cpufreq_disabled())
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 338bf0e59bb8..9623218d996a 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -149,8 +149,6 @@ static inline bool policy_is_shared(struct cpufreq_policy *policy)
 
 /* /sys/devices/system/cpu/cpufreq: entry point for global variables */
 extern struct kobject *cpufreq_global_kobject;
-int cpufreq_sysfs_create_file(const struct attribute *attr);
-void cpufreq_sysfs_remove_file(const struct attribute *attr);
 
 #ifdef CONFIG_CPU_FREQ
 unsigned int cpufreq_get(unsigned int cpu);
-- 
2.4.0

[PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Viresh Kumar]

The cpufreq sysfs interface had been a bit inconsistent as one of the
CPUs for a policy had a real directory within its sysfs 'cpuX' directory
and all other CPUs had links to it. That also made the code a bit
complex as we need to take care of moving the sysfs directory if the CPU
containing the real directory is getting physically hot-unplugged.

Solve this by creating 'policyX' directories (per-policy) in
/sys/devices/system/cpu/cpufreq/ directory, where X is the CPU for which
the policy was first created.

This also removes the need of keeping kobj_cpu and we can remove it now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 34 ++++------------------------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2cb0e3b8170e..58aabe0f2d2c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -917,9 +917,6 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
 
 	/* Some related CPUs might not be present (physically hotplugged) */
 	for_each_cpu(j, policy->real_cpus) {
-		if (j == policy->kobj_cpu)
-			continue;
-
 		ret = add_cpu_dev_symlink(policy, j);
 		if (ret)
 			break;
@@ -933,12 +930,8 @@ 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;
-
+	for_each_cpu(j, policy->real_cpus)
 		remove_cpu_dev_symlink(policy, j);
-	}
 }
 
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
@@ -1054,8 +1047,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	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");
+	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;
@@ -1069,10 +1062,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	INIT_WORK(&policy->update, handle_update);
 
 	policy->cpu = cpu;
-
-	/* Set this once on allocation */
-	policy->kobj_cpu = cpu;
-
 	return policy;
 
 err_free_real_cpus:
@@ -1424,22 +1413,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 		return;
 	}
 
-	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);
-
-		sysfs_remove_link(&new_dev->kobj, "cpufreq");
-		policy->kobj_cpu = new_cpu;
-		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
-	}
+	remove_cpu_dev_symlink(policy, cpu);
 }
 
 static void handle_update(struct work_struct *work)
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 9623218d996a..ef4c5b1a860f 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -65,7 +65,6 @@ struct cpufreq_policy {
 	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
 						should set cpufreq */
 	unsigned int		cpu;    /* cpu managing this policy, must be online */
-	unsigned int		kobj_cpu; /* cpu managing sysfs files, can be offline */
 
 	struct clk		*clk;
 	struct cpufreq_cpuinfo	cpuinfo;/* see above */
-- 
2.4.0

Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Saravana Kannan]

On 10/11/2015 10:21 AM, Viresh Kumar wrote:
> The cpufreq sysfs interface had been a bit inconsistent as one of the
> CPUs for a policy had a real directory within its sysfs 'cpuX' directory
> and all other CPUs had links to it. That also made the code a bit
> complex as we need to take care of moving the sysfs directory if the CPU
> containing the real directory is getting physically hot-unplugged.

This should actually make hotplug a bit faster too.

> Solve this by creating 'policyX' directories (per-policy) in
> /sys/devices/system/cpu/cpufreq/ directory, where X is the CPU for which
> the policy was first created.
Can we use the first CPU in the related CPUs mask? Instead of the first 
CPU that the policy got created on? The policyX numbering would be a bit 
more consistent that way.

>
> This also removes the need of keeping kobj_cpu and we can remove it now.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Suggested-by: ?

> ---
>   drivers/cpufreq/cpufreq.c | 34 ++++------------------------------
>   include/linux/cpufreq.h   |  1 -
>   2 files changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2cb0e3b8170e..58aabe0f2d2c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -917,9 +917,6 @@ static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
>
>   	/* Some related CPUs might not be present (physically hotplugged) */
>   	for_each_cpu(j, policy->real_cpus) {
Didn't notice when this got added. Do we really need this anymore if we 
don't care about moving the directory and creating symlinks? I don't 
think we need it anymore. And if we really need to know related - 
offline, we can use for_each_cpu_and(related, online/present mask)

> -		if (j == policy->kobj_cpu)
> -			continue;
> -
>   		ret = add_cpu_dev_symlink(policy, j);
>   		if (ret)
>   			break;
> @@ -933,12 +930,8 @@ 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;
> -
> +	for_each_cpu(j, policy->real_cpus)
>   		remove_cpu_dev_symlink(policy, j);
> -	}
>   }
>
>   static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
> @@ -1054,8 +1047,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   	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");
> +	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;
> @@ -1069,10 +1062,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>   	INIT_WORK(&policy->update, handle_update);
>
>   	policy->cpu = cpu;
> -
> -	/* Set this once on allocation */
> -	policy->kobj_cpu = cpu;
> -
>   	return policy;
>
>   err_free_real_cpus:
> @@ -1424,22 +1413,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>   		return;
>   	}
>
> -	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);
> -
> -		sysfs_remove_link(&new_dev->kobj, "cpufreq");
> -		policy->kobj_cpu = new_cpu;
> -		WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj));
> -	}
> +	remove_cpu_dev_symlink(policy, cpu);
>   }
>
>   static void handle_update(struct work_struct *work)
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 9623218d996a..ef4c5b1a860f 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -65,7 +65,6 @@ struct cpufreq_policy {
>   	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
>   						should set cpufreq */
>   	unsigned int		cpu;    /* cpu managing this policy, must be online */
> -	unsigned int		kobj_cpu; /* cpu managing sysfs files, can be offline */
>
>   	struct clk		*clk;
>   	struct cpufreq_cpuinfo	cpuinfo;/* see above */
>

-Saravana

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

Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Viresh Kumar]

On 12-10-15, 12:31, Saravana Kannan wrote:
> Can we use the first CPU in the related CPUs mask? Instead of the
> first CPU that the policy got created on? The policyX numbering
> would be a bit more consistent that way.

Okay..

> Suggested-by: ?

Will add. Though me/Rafael thought about it long back, but then
dropped the idea :)

> Didn't notice when this got added. Do we really need this anymore if
> we don't care about moving the directory and creating symlinks? I
> don't think we need it anymore. And if we really need to know
> related - offline, we can use for_each_cpu_and(related,
> online/present mask)

Its about tracking present-cpus, for which the link is present. So, it
is still required.

-- 
viresh

Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Saravana Kannan]

On 10/12/2015 08:39 PM, Viresh Kumar wrote:
> On 12-10-15, 12:31, Saravana Kannan wrote:
>> Can we use the first CPU in the related CPUs mask? Instead of the
>> first CPU that the policy got created on? The policyX numbering
>> would be a bit more consistent that way.
>
> Okay..
>
>> Suggested-by: ?
>
> Will add. Though me/Rafael thought about it long back, but then
> dropped the idea :)
>
>> Didn't notice when this got added. Do we really need this anymore if
>> we don't care about moving the directory and creating symlinks? I
>> don't think we need it anymore. And if we really need to know
>> related - offline, we can use for_each_cpu_and(related,
>> online/present mask)
>
> Its about tracking present-cpus, for which the link is present. So, it
> is still required.

But we don't need to track track of "present-cpus" separately though. We 
could do the for_each_cpu_and() when we create the symlinks for the 
first time. And after that, we can just use the subsystem interface 
callbacks (cpufreq_add_dev() and cpufreq_remove_dev()) to keep the 
symlinks updated.

I don't see any place where keeping track of this separately is more 
efficient. This would save some memory savings when the number of CPUs 
is large and also simplify the code because we won't have to keep 
another field up to date.

-Saravana

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

Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Viresh Kumar]

On 13-10-15, 12:29, Saravana Kannan wrote:
> But we don't need to track track of "present-cpus" separately
> though. We could do the for_each_cpu_and() when we create the
> symlinks for the first time. And after that, we can just use the
> subsystem interface callbacks (cpufreq_add_dev() and
> cpufreq_remove_dev()) to keep the symlinks updated.
> 
> I don't see any place where keeping track of this separately is more
> efficient. This would save some memory savings when the number of
> CPUs is large and also simplify the code because we won't have to
> keep another field up to date.

It is still required to track when can we free the policy.

-- 
viresh

Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Saravana Kannan]

On 10/14/2015 11:55 PM, Viresh Kumar wrote:
> On 13-10-15, 12:29, Saravana Kannan wrote:
>> But we don't need to track track of "present-cpus" separately
>> though. We could do the for_each_cpu_and() when we create the
>> symlinks for the first time. And after that, we can just use the
>> subsystem interface callbacks (cpufreq_add_dev() and
>> cpufreq_remove_dev()) to keep the symlinks updated.
>>
>> I don't see any place where keeping track of this separately is more
>> efficient. This would save some memory savings when the number of
>> CPUs is large and also simplify the code because we won't have to
>> keep another field up to date.
>
> It is still required to track when can we free the policy.
>

Ok, I'm not very familiar with this new field's uses. I'll take a closer 
look later and respond if I have other thoughts.

Thanks,
Saravana

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

Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Viresh Kumar]

On 12-10-15, 12:31, Saravana Kannan wrote:
> Can we use the first CPU in the related CPUs mask? Instead of the
> first CPU that the policy got created on? The policyX numbering
> would be a bit more consistent that way.

Okay, checked this again. The problem is that ->init() isn't called
yet and we are very early in the initialization sequence. So, we can't
really know related_cpus yet. So I will keep it unchanged for now.

-- 
viresh

Re: [PATCH 4/5] cpufreq: create cpu/cpufreq/policyX directories

[Saravana Kannan]

On 10/12/2015 11:15 PM, Viresh Kumar wrote:
> On 12-10-15, 12:31, Saravana Kannan wrote:
>> Can we use the first CPU in the related CPUs mask? Instead of the
>> first CPU that the policy got created on? The policyX numbering
>> would be a bit more consistent that way.
>
> Okay, checked this again. The problem is that ->init() isn't called
> yet and we are very early in the initialization sequence. So, we can't
> really know related_cpus yet. So I will keep it unchanged for now.
>

Can we move the sysfs add to the end so that by the time we add sysfs, 
we'll have all the details?

-Saravana

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

[PATCH 5/5] cpufreq: Drop redundant check for inactive policies

[Viresh Kumar]

We just made sure policy->cpu is online and this check will always fail
as the policy is active. Drop it.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 58aabe0f2d2c..4fa2215cc6ec 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -843,18 +843,11 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 
 	down_write(&policy->rwsem);
 
-	/* Updating inactive policies is invalid, so avoid doing that. */
-	if (unlikely(policy_is_inactive(policy))) {
-		ret = -EBUSY;
-		goto unlock_policy_rwsem;
-	}
-
 	if (fattr->store)
 		ret = fattr->store(policy, buf, count);
 	else
 		ret = -EIO;
 
-unlock_policy_rwsem:
 	up_write(&policy->rwsem);
 unlock:
 	put_online_cpus();
-- 
2.4.0

Re: [PATCH 5/5] cpufreq: Drop redundant check for inactive policies

[Saravana Kannan]

On 10/11/2015 10:21 AM, Viresh Kumar wrote:
> We just made sure policy->cpu is online and this check will always fail
> as the policy is active. Drop it.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/cpufreq.c | 7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 58aabe0f2d2c..4fa2215cc6ec 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -843,18 +843,11 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>
>   	down_write(&policy->rwsem);
>
> -	/* Updating inactive policies is invalid, so avoid doing that. */
> -	if (unlikely(policy_is_inactive(policy))) {
> -		ret = -EBUSY;
> -		goto unlock_policy_rwsem;
> -	}
> -
>   	if (fattr->store)
>   		ret = fattr->store(policy, buf, count);
>   	else
>   		ret = -EIO;
>
> -unlock_policy_rwsem:
>   	up_write(&policy->rwsem);
>   unlock:
>   	put_online_cpus();
>

Doesn't really seem related to the sysfs reorg/clean up. Should it be a 
separate patch outside of this series?

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

-Saravana

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

Re: [PATCH 5/5] cpufreq: Drop redundant check for inactive policies

[Viresh Kumar]

On 12-10-15, 12:35, Saravana Kannan wrote:
> Doesn't really seem related to the sysfs reorg/clean up. Should it
> be a separate patch outside of this series?

Sent it separately now ..

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

A reviewed-by would have been more appropriate here though.

-- 
viresh