linux-kernel - Re: [PATCH 1/3] cpufreq: move call to __find_governor() to cpufreq_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7507031.JAfx7W3apJ@vostro.rjw.lan>
Date:	Sat, 01 Mar 2014 02:09:20 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	skannan@...eaurora.org, linaro-kernel@...ts.linaro.org,
	cpufreq@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, srivatsa.bhat@...ux.vnet.ibm.com
Subject: Re: [PATCH 1/3] cpufreq: move call to __find_governor() to cpufreq_init_policy()

On Tuesday, February 25, 2014 02:20:09 PM Viresh Kumar wrote:
> We call __find_governor() during addition of first CPU of every policy to find
> the last governor used for this CPU before it was hotplugged-out.
> 
> After that we call cpufreq_parse_governor() in cpufreq_init_policy() either with
> this governor or default governor. And right after that policy->governor is set
> to NULL.
> 
> So, instead of doing this move the relevant parts to cpufreq_init_policy()
> policy only and initialize policy->governor to NULL at the beginning.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
> 
> Hi Saravana,
> 
> I hope only the first two patches would fix things for you but probably you can
> test all three.
> 
> @Rafael: We might need to get these in next rc only as these are more or less
> fixes.
> 
>  drivers/cpufreq/cpufreq.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index c755b5f..cc4f244 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -879,18 +879,27 @@ err_out_kobj_put:
>  
>  static void cpufreq_init_policy(struct cpufreq_policy *policy)
>  {
> +	struct cpufreq_governor *gov = NULL;
>  	struct cpufreq_policy new_policy;
>  	int ret = 0;
>  
>  	memcpy(&new_policy, policy, sizeof(*policy));
>  

And while I'm at it, can we *please* avoid adding new #ifdef blocks into
function bodies?

Please introduce a wrapper around __find_governor() returning NULL for
CONFIG_HOTPLUG_CPU unset.

> +	/* Update governor of new_policy to the governor used before hotplug */
> +#ifdef CONFIG_HOTPLUG_CPU
> +	gov = __find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu));
> +#endif
> +	if (gov)
> +		pr_debug("Restoring governor %s for cpu %d\n",
> +				policy->governor->name, policy->cpu);
> +	else
> +		gov = CPUFREQ_DEFAULT_GOVERNOR;
> +
> +	new_policy.governor = gov;
> +
>  	/* Use the default policy if its valid. */
>  	if (cpufreq_driver->setpolicy)
> -		cpufreq_parse_governor(policy->governor->name,
> -					&new_policy.policy, NULL);
> -
> -	/* assure that the starting sequence is run in cpufreq_set_policy */
> -	policy->governor = NULL;
> +		cpufreq_parse_governor(gov->name, &new_policy.policy, NULL);
>  
>  	/* set default policy */
>  	ret = cpufreq_set_policy(policy, &new_policy);
> @@ -944,11 +953,11 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
>  	unsigned long flags;
>  
>  	read_lock_irqsave(&cpufreq_driver_lock, flags);
> -
>  	policy = per_cpu(cpufreq_cpu_data_fallback, cpu);
> -

Why do these whitespace changes belong to this patch?

>  	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
>  
> +	policy->governor = NULL;
> +
>  	return policy;
>  }
>  
> @@ -1036,7 +1045,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
>  	unsigned long flags;
>  #ifdef CONFIG_HOTPLUG_CPU
>  	struct cpufreq_policy *tpolicy;
> -	struct cpufreq_governor *gov;
>  #endif
>  
>  	if (cpu_is_offline(cpu))
> @@ -1094,7 +1102,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
>  	else
>  		policy->cpu = cpu;
>  
> -	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
>  	cpumask_copy(policy->cpus, cpumask_of(cpu));
>  
>  	init_completion(&policy->kobj_unregister);
> @@ -1179,15 +1186,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
>  	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>  				     CPUFREQ_START, policy);
>  
> -#ifdef CONFIG_HOTPLUG_CPU
> -	gov = __find_governor(per_cpu(cpufreq_cpu_governor, cpu));
> -	if (gov) {
> -		policy->governor = gov;
> -		pr_debug("Restoring governor %s for cpu %d\n",
> -		       policy->governor->name, cpu);
> -	}
> -#endif
> -
>  	if (!frozen) {
>  		ret = cpufreq_add_dev_interface(policy, dev);
>  		if (ret)
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/