linux-kernel - Re: [PATCH] cpufreq: Avoid unnecessary locking in show() and store()

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: <20160212063115.GE16949@vireshk-i7>
Date:	Fri, 12 Feb 2016 12:01:15 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	Linux PM list <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cpufreq: Avoid unnecessary locking in show() and store()

On 11-02-16, 02:25, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> The show() and store() routines in the cpufreq core don't need to
> acquire all of the locks to check if the struct freq_attr they want
> to use really provides the callbacks they need as expected, so change
> them to avoid doing that.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>  drivers/cpufreq/cpufreq.c |   27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -862,13 +862,11 @@ static ssize_t show(struct kobject *kobj
>  	struct freq_attr *fattr = to_attr(attr);
>  	ssize_t ret;
>  
> -	down_read(&policy->rwsem);
> -
> -	if (fattr->show)
> -		ret = fattr->show(policy, buf);
> -	else
> -		ret = -EIO;
> +	if (!fattr->show)
> +		return -EIO;
>  
> +	down_read(&policy->rwsem);
> +	ret = fattr->show(policy, buf);
>  	up_read(&policy->rwsem);
>  
>  	return ret;
> @@ -881,20 +879,17 @@ static ssize_t store(struct kobject *kob
>  	struct freq_attr *fattr = to_attr(attr);
>  	ssize_t ret = -EINVAL;
>  
> -	get_online_cpus();
> -
> -	if (!cpu_online(policy->cpu))
> -		goto unlock;
> +	if (!fattr->store)
> +		return -EIO;
>  
> -	down_write(&policy->rwsem);
> +	get_online_cpus();
>  
> -	if (fattr->store)
> +	if (cpu_online(policy->cpu)) {
> +		down_write(&policy->rwsem);
>  		ret = fattr->store(policy, buf, count);
> -	else
> -		ret = -EIO;
> +		up_write(&policy->rwsem);
> +	}
>  
> -	up_write(&policy->rwsem);
> -unlock:

I have no problems with the patch as is, but how are we going to benefit from it
?

'if (fattr->show/store)' is never ever going to fail, unless we have a bug here.
So, even we may want to add a WARN_ON() for that case instead.

-- 
viresh