linux-kernel - Re: [PATCH 09/12] PM / OPP: Move away from RCU locking

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]

Date:   Mon, 9 Jan 2017 15:57:27 -0800
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Rafael Wysocki <rjw@...ysocki.net>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        linaro-kernel@...ts.linaro.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH 09/12] PM / OPP: Move away from RCU locking

On 12/07, Viresh Kumar wrote:
> The RCU locking isn't well suited for the OPP core. The RCU locking fits
> better for reader heavy stuff, while the OPP core have at max one or two
> readers only at a time.
> 
> Over that, it was getting very confusing the way RCU locking was used
> with the OPP core. The individual OPPs are mostly well handled, i.e. for
> an update a new structure was created and then that replaced the older
> one. But the OPP tables were updated directly all the time from various
> parts of the core. Though they were mostly used from within RCU locked
> region, they didn't had much to do with RCU and were governed by the
> mutex instead.
> 
> And that mixed with the 'opp_table_lock' has made the core even more
> confusing.
> 
> Now that we are already managing the OPPs and the OPP tables with kernel
> reference infrastructure, we can get rid of RCU locking completely and
> simplify the code a lot.
> 
> Remove all RCU references from code and comments.
> 
> Acquire opp_table->lock while parsing the list of OPPs though.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>

Reviewed-by: Stephen Boyd <sboyd@...eaurora.org>

> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 2b689fc73596..b5e9600058c2 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -133,19 +117,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_voltage);
>   */
>  unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
>  {
> -	struct dev_pm_opp *tmp_opp;
> -	unsigned long f = 0;
> -
> -	rcu_read_lock();
> -
> -	tmp_opp = rcu_dereference(opp);
> -	if (IS_ERR_OR_NULL(tmp_opp) || !tmp_opp->available)
> +	if (IS_ERR_OR_NULL(opp) || !opp->available) {

I suppose this is one thing RCU was being used for, marking OPPs
available and then having these "getter" APIs fail if the OPPs go
away. But that was never right because the OPP could have been
made unavailable after this function returned and things still
wouldn't work.

>  		pr_err("%s: Invalid parameters\n", __func__);
> -	else
> -		f = tmp_opp->rate;
> +		return 0;
> +	}
>  
> -	rcu_read_unlock();
> -	return f;
> +	return opp->rate;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
>  

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