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

[Saravana Kannan <skannan@codeaurora.org>]

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