diff mbox

PM / OPP: optimize dev_pm_opp_set_rate() a bit

Message ID 1469191370-1285-1-git-send-email-jszhang@marvell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jisheng Zhang July 22, 2016, 12:42 p.m. UTC 
In dev_pm_opp_set_rate(), _find_opp_table() is called three times: once
by _get_opp_clk(), twice by dev_pm_opp_find_freq_ceil(). If there are
several opp_tables in the system, three times of opp table finding is a
big waste. This patch reduced the call of _find_opp_table() to only
once.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/base/power/opp/core.c | 85 ++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 49 deletions(-)

Comments

kernel test robot July 22, 2016, 2:30 p.m. UTC | #1 
Hi,

[auto build test WARNING on pm/linux-next]
[also build test WARNING on v4.7-rc7 next-20160722]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jisheng-Zhang/PM-OPP-optimize-dev_pm_opp_set_rate-a-bit/20160722-205339
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/base/power/opp/core.c: In function 'dev_pm_opp_set_rate':
>> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt_max' may be used uninitialized in this function [-Wmaybe-uninitialized]
      _set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt_min' may be used uninitialized in this function [-Wmaybe-uninitialized]
>> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt' may be used uninitialized in this function [-Wmaybe-uninitialized]

vim +/ou_volt_max +666 drivers/base/power/opp/core.c

6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  650  	if (freq < old_freq) {
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  651  		ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  652  				       u_volt_max);
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  653  		if (ret)
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  654  			goto restore_freq;
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  655  	}
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  656  
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  657  	return 0;
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  658  
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  659  restore_freq:
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  660  	if (clk_set_rate(clk, old_freq))
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  661  		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  662  			__func__, old_freq);
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  663  restore_voltage:
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  664  	/* This shouldn't harm even if the voltages weren't updated earlier */
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  665  	if (!IS_ERR(old_opp))
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09 @666  		_set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  667  
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  668  	return ret;
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  669  }
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  670  EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  671  
2c2709dc drivers/base/power/opp/core.c Viresh Kumar 2016-02-16  672  /* OPP-dev Helpers */
2c2709dc drivers/base/power/opp/core.c Viresh Kumar 2016-02-16  673  static void _kfree_opp_dev_rcu(struct rcu_head *head)
06441658 drivers/base/power/opp.c      Viresh Kumar 2015-07-29  674  {

:::::: The code at line 666 was first introduced by commit
:::::: 6a0712f6f199e737aa5913d28ec4bd3a25de9660 PM / OPP: Add dev_pm_opp_set_rate()

:::::: TO: Viresh Kumar <viresh.kumar@linaro.org>
:::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Viresh Kumar July 22, 2016, 4:21 p.m. UTC | #2 
On 22-07-16, 20:42, Jisheng Zhang wrote:
>  static int _set_opp_voltage(struct device *dev, struct regulator *reg,
>  			    unsigned long u_volt, unsigned long u_volt_min,
>  			    unsigned long u_volt_max)
> @@ -586,9 +565,24 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  		return -EINVAL;
>  	}
>  
> -	clk = _get_opp_clk(dev);
> -	if (IS_ERR(clk))
> +	rcu_read_lock();
> +
> +	opp_table = _find_opp_table(dev);
> +	if (IS_ERR(opp_table)) {
> +		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
> +		rcu_read_unlock();
> +		return PTR_ERR(opp_table);
> +	}
> +
> +	clk = opp_table->clk;
> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "%s: No clock available for the device\n",
> +			__func__);
> +		rcu_read_unlock();
>  		return PTR_ERR(clk);
> +	}
> +
> +	rcu_read_unlock();

It is not _safe_ to use opp_table pointer after the rcu_read_unlock()
here.
Jisheng Zhang July 25, 2016, 5:12 a.m. UTC | #3 
Dear Viresh,

On Fri, 22 Jul 2016 09:21:51 -0700 Viresh Kumar  wrote:

> On 22-07-16, 20:42, Jisheng Zhang wrote:
> >  static int _set_opp_voltage(struct device *dev, struct regulator *reg,
> >  			    unsigned long u_volt, unsigned long u_volt_min,
> >  			    unsigned long u_volt_max)
> > @@ -586,9 +565,24 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> >  		return -EINVAL;
> >  	}
> >  
> > -	clk = _get_opp_clk(dev);
> > -	if (IS_ERR(clk))
> > +	rcu_read_lock();
> > +
> > +	opp_table = _find_opp_table(dev);
> > +	if (IS_ERR(opp_table)) {
> > +		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
> > +		rcu_read_unlock();
> > +		return PTR_ERR(opp_table);
> > +	}
> > +
> > +	clk = opp_table->clk;
> > +	if (IS_ERR(clk)) {
> > +		dev_err(dev, "%s: No clock available for the device\n",
> > +			__func__);
> > +		rcu_read_unlock();
> >  		return PTR_ERR(clk);
> > +	}
> > +
> > +	rcu_read_unlock();  
> 
> It is not _safe_ to use opp_table pointer after the rcu_read_unlock()

Oops, indeed. Thanks very much for pointing it out! Will fix it in v2, so
it seems we can only reduce the call of _find_opp_table to twice.

Thanks,
Jisheng
Jisheng Zhang July 25, 2016, 5:19 a.m. UTC | #4 
Dear all,

On Fri, 22 Jul 2016 22:30:53 +0800 kbuild test robot wrote:

> Hi,
> 
> [auto build test WARNING on pm/linux-next]
> [also build test WARNING on v4.7-rc7 next-20160722]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Jisheng-Zhang/PM-OPP-optimize-dev_pm_opp_set_rate-a-bit/20160722-205339
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> config: x86_64-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/base/power/opp/core.c: In function 'dev_pm_opp_set_rate':
> >> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt_max' may be used uninitialized in this function [-Wmaybe-uninitialized]  
>       _set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt_min' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt' may be used uninitialized in this function [-Wmaybe-uninitialized]  

These warnings seem weired. We only use them when !IS_ERR(old_opp), and we
should already set them if !IS_ERR(old_opp). Another weired thing is if
we add something, printk e.g in _find_freq_ceil(), then these warnings disappear

Could you please kindly give some suggestions about how to fix these warnings?

Thanks,
Jisheng

> 
> vim +/ou_volt_max +666 drivers/base/power/opp/core.c
> 
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  650  	if (freq < old_freq) {
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  651  		ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  652  				       u_volt_max);
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  653  		if (ret)
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  654  			goto restore_freq;
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  655  	}
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  656  
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  657  	return 0;
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  658  
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  659  restore_freq:
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  660  	if (clk_set_rate(clk, old_freq))
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  661  		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  662  			__func__, old_freq);
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  663  restore_voltage:
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  664  	/* This shouldn't harm even if the voltages weren't updated earlier */
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  665  	if (!IS_ERR(old_opp))
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09 @666  		_set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  667  
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  668  	return ret;
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  669  }
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  670  EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
> 6a0712f6 drivers/base/power/opp/core.c Viresh Kumar 2016-02-09  671  
> 2c2709dc drivers/base/power/opp/core.c Viresh Kumar 2016-02-16  672  /* OPP-dev Helpers */
> 2c2709dc drivers/base/power/opp/core.c Viresh Kumar 2016-02-16  673  static void _kfree_opp_dev_rcu(struct rcu_head *head)
> 06441658 drivers/base/power/opp.c      Viresh Kumar 2015-07-29  674  {
> 
> :::::: The code at line 666 was first introduced by commit
> :::::: 6a0712f6f199e737aa5913d28ec4bd3a25de9660 PM / OPP: Add dev_pm_opp_set_rate()
> 
> :::::: TO: Viresh Kumar <viresh.kumar@linaro.org>
> :::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Jisheng Zhang July 25, 2016, 6:05 a.m. UTC | #5 
On Mon, 25 Jul 2016 13:19:47 +0800 Jisheng Zhang wrote:

> Dear all,
> 
> On Fri, 22 Jul 2016 22:30:53 +0800 kbuild test robot wrote:
> 
> > Hi,
> > 
> > [auto build test WARNING on pm/linux-next]
> > [also build test WARNING on v4.7-rc7 next-20160722]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Jisheng-Zhang/PM-OPP-optimize-dev_pm_opp_set_rate-a-bit/20160722-205339
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> > config: x86_64-allmodconfig (attached as .config)
> > compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
> > reproduce:
> >         # save the attached .config to linux build tree
> >         make ARCH=x86_64 
> > 
> > Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> > http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
> > 
> > All warnings (new ones prefixed by >>):
> > 
> >    drivers/base/power/opp/core.c: In function 'dev_pm_opp_set_rate':  
> > >> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt_max' may be used uninitialized in this function [-Wmaybe-uninitialized]    
> >       _set_opp_voltage(dev, reg, ou_volt, ou_volt_min, ou_volt_max);
> >       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  
> > >> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt_min' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > >> drivers/base/power/opp/core.c:666:3: warning: 'ou_volt' may be used uninitialized in this function [-Wmaybe-uninitialized]    
> 
> These warnings seem weired. We only use them when !IS_ERR(old_opp), and we
> should already set them if !IS_ERR(old_opp). Another weired thing is if
> we add something, printk e.g in _find_freq_ceil(), then these warnings disappear

Hmm, it looks that gcc will inline _find_freq_ceil(), then gcc can't
detect that ou_volt* are already set. Mark _find_freq_ceil() noinline would fix
the warnings

Thanks,
Jisheng

> 
> Could you please kindly give some suggestions about how to fix these warnings?
>
diff mbox

Patch

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 7c04c87..96043ee 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -402,6 +402,22 @@  struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);
 
+static struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
+					  unsigned long *freq)
+{
+	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+
+	list_for_each_entry_rcu(temp_opp, &opp_table->opp_list, node) {
+		if (temp_opp->available && temp_opp->rate >= *freq) {
+			opp = temp_opp;
+			*freq = opp->rate;
+			break;
+		}
+	}
+
+	return opp;
+}
+
 /**
  * dev_pm_opp_find_freq_ceil() - Search for an rounded ceil freq
  * @dev:	device for which we do this operation
@@ -427,7 +443,6 @@  struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq)
 {
 	struct opp_table *opp_table;
-	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
 
 	opp_rcu_lockdep_assert();
 
@@ -440,15 +455,7 @@  struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 	if (IS_ERR(opp_table))
 		return ERR_CAST(opp_table);
 
-	list_for_each_entry_rcu(temp_opp, &opp_table->opp_list, node) {
-		if (temp_opp->available && temp_opp->rate >= *freq) {
-			opp = temp_opp;
-			*freq = opp->rate;
-			break;
-		}
-	}
-
-	return opp;
+	return _find_freq_ceil(opp_table, freq);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
 
@@ -506,34 +513,6 @@  struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
-/*
- * The caller needs to ensure that opp_table (and hence the clk) isn't freed,
- * while clk returned here is used.
- */
-static struct clk *_get_opp_clk(struct device *dev)
-{
-	struct opp_table *opp_table;
-	struct clk *clk;
-
-	rcu_read_lock();
-
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
-		clk = ERR_CAST(opp_table);
-		goto unlock;
-	}
-
-	clk = opp_table->clk;
-	if (IS_ERR(clk))
-		dev_err(dev, "%s: No clock available for the device\n",
-			__func__);
-
-unlock:
-	rcu_read_unlock();
-	return clk;
-}
-
 static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 			    unsigned long u_volt, unsigned long u_volt_min,
 			    unsigned long u_volt_max)
@@ -586,9 +565,24 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return -EINVAL;
 	}
 
-	clk = _get_opp_clk(dev);
-	if (IS_ERR(clk))
+	rcu_read_lock();
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
+		rcu_read_unlock();
+		return PTR_ERR(opp_table);
+	}
+
+	clk = opp_table->clk;
+	if (IS_ERR(clk)) {
+		dev_err(dev, "%s: No clock available for the device\n",
+			__func__);
+		rcu_read_unlock();
 		return PTR_ERR(clk);
+	}
+
+	rcu_read_unlock();
 
 	freq = clk_round_rate(clk, target_freq);
 	if ((long)freq <= 0)
@@ -605,14 +599,7 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	rcu_read_lock();
 
-	opp_table = _find_opp_table(dev);
-	if (IS_ERR(opp_table)) {
-		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
-		rcu_read_unlock();
-		return PTR_ERR(opp_table);
-	}
-
-	old_opp = dev_pm_opp_find_freq_ceil(dev, &old_freq);
+	old_opp = _find_freq_ceil(opp_table, &old_freq);
 	if (!IS_ERR(old_opp)) {
 		ou_volt = old_opp->u_volt;
 		ou_volt_min = old_opp->u_volt_min;
@@ -622,7 +609,7 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 			__func__, old_freq, PTR_ERR(old_opp));
 	}
 
-	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+	opp = _find_freq_ceil(opp_table, &freq);
 	if (IS_ERR(opp)) {
 		ret = PTR_ERR(opp);
 		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",