linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3 00/10] PM / OPP: Fixes and cleanups
@ 2017-01-02  9:10 Viresh Kumar
  2017-01-02  9:10 ` [PATCH V3 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs Viresh Kumar
                   ` (9 more replies)
  0 siblings, 10 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:10 UTC (permalink / raw)
  To: Rafael Wysocki
  Cc: linaro-kernel, linux-pm, linux-kernel, Stephen Boyd,
	Nishanth Menon, Vincent Guittot, Viresh Kumar

Hi,

Happy new year guys !!

This series fixes a memory leak (first patch) and does some cleanups to
the OPP core.

It is based of pm/linux-next branch.

This series is tested for few days by various build and boot bots:
- Kernel CI (Linaro)
- Fengguang Wu's bot (Intel)

V2->V3:
- Only the 7th patch is updated based on Stephen's comment.
- All the patches now have a Reviewed-by tag.

V1->V2:
- 6 out of 10 patches have received Acks from Stephen/MyungJoo.
- dev_pm_opp_get_suspend_opp_freq() return freq in Hz now
- s/opp_table/table in _opp_allocate()
- Improved comment over _opp_add() defining its return types
- opp_rcu_lockdep_assert() added at few places
- Dropped a useless comment

--
viresh

Viresh Kumar (10):
  PM / OPP: Fix memory leak while adding duplicate OPPs
  PM / OPP: Remove useless TODO
  PM / OPP: Rename _allocate_opp() to _opp_allocate()
  PM / OPP: Error out on failing to add static OPPs for v1 bindings
  PM / OPP: Add light weight _opp_free() routine
  PM / OPP: Rename and split _dev_pm_opp_remove_table()
  PM / OPP: Don't allocate OPP table from _opp_allocate()
  PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate
  PM / OPP: Don't expose srcu_head to register notifiers
  PM / OPP: Split out part of _add_opp_table() and _remove_opp_table()

 drivers/base/power/opp/core.c | 293 ++++++++++++++++++++++++++----------------
 drivers/base/power/opp/of.c   |  89 +++++++------
 drivers/base/power/opp/opp.h  |  11 +-
 drivers/cpufreq/cpufreq-dt.c  |   7 +-
 drivers/devfreq/devfreq.c     |  26 +---
 include/linux/pm_opp.h        |  20 ++-
 6 files changed, 253 insertions(+), 193 deletions(-)

-- 
2.7.1.410.g6faf27b

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH V3 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
@ 2017-01-02  9:10 ` Viresh Kumar
  2017-01-02  9:10 ` [PATCH V3 02/10] PM / OPP: Remove useless TODO Viresh Kumar
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:10 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

There are two types of duplicate OPPs that get different behavior from
the core:
A) An earlier OPP is marked 'available' and has same freq/voltages as
   the new one.
B) An earlier OPP with same frequency, but is marked 'unavailable' OR
   doesn't have same voltages as the new one.

The OPP core returns 0 for the first one, but -EEXIST for the second.

While the OPP core returns 0 for the first case, its callers don't free
the newly allocated OPP structure which isn't used anymore. Fix that by
returning -EBUSY instead of 0, but make the callers return 0 eventually.

As this isn't a critical fix, its not getting marked for stable kernel.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/core.c | 18 ++++++++++++++++--
 drivers/base/power/opp/of.c   |  6 +++++-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 35ff06283738..a8a5e01b7756 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1067,6 +1067,16 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 
+/*
+ * Returns:
+ * 0: On success. And appropriate error message for duplicate OPPs.
+ * -EBUSY: For OPP with same freq/volt and is available. The callers of
+ *  _opp_add() must return 0 if they receive -EBUSY from it. This is to make
+ *  sure we don't print error messages unnecessarily if different parts of
+ *  kernel try to initialize the OPP table.
+ * -EEXIST: For OPP with same freq but different volt or is unavailable. This
+ *  should be considered an error by the callers of _opp_add().
+ */
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	     struct opp_table *opp_table)
 {
@@ -1099,7 +1109,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 
 		/* Should we compare voltages for all regulators here ? */
 		return opp->available &&
-		       new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
+		       new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? -EBUSY : -EEXIST;
 	}
 
 	new_opp->opp_table = opp_table;
@@ -1173,8 +1183,12 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 	new_opp->dynamic = dynamic;
 
 	ret = _opp_add(dev, new_opp, opp_table);
-	if (ret)
+	if (ret) {
+		/* Don't return error for duplicate OPPs */
+		if (ret == -EBUSY)
+			ret = 0;
 		goto free_opp;
+	}
 
 	mutex_unlock(&opp_table_lock);
 
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 3f7d2591b173..356c75edd656 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -327,8 +327,12 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 		goto free_opp;
 
 	ret = _opp_add(dev, new_opp, opp_table);
-	if (ret)
+	if (ret) {
+		/* Don't return error for duplicate OPPs */
+		if (ret == -EBUSY)
+			ret = 0;
 		goto free_opp;
+	}
 
 	/* OPP to select on device suspend */
 	if (of_property_read_bool(np, "opp-suspend")) {
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 02/10] PM / OPP: Remove useless TODO
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
  2017-01-02  9:10 ` [PATCH V3 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs Viresh Kumar
@ 2017-01-02  9:10 ` Viresh Kumar
  2017-01-02  9:10 ` [PATCH V3 03/10] PM / OPP: Rename _allocate_opp() to _opp_allocate() Viresh Kumar
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:10 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

This TODO doesn't make sense anymore as we have all the information in a
single OPP table. Remove it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/of.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 356c75edd656..996ca3b42f47 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -246,8 +246,6 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
 static struct device_node *_of_get_opp_desc_node(struct device *dev)
 {
 	/*
-	 * TODO: Support for multiple OPP tables.
-	 *
 	 * There should be only ONE phandle present in "operating-points-v2"
 	 * property.
 	 */
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 03/10] PM / OPP: Rename _allocate_opp() to _opp_allocate()
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
  2017-01-02  9:10 ` [PATCH V3 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs Viresh Kumar
  2017-01-02  9:10 ` [PATCH V3 02/10] PM / OPP: Remove useless TODO Viresh Kumar
@ 2017-01-02  9:10 ` Viresh Kumar
  2017-01-02  9:10 ` [PATCH V3 04/10] PM / OPP: Error out on failing to add static OPPs for v1 bindings Viresh Kumar
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:10 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

Make the naming consistent with how other routines are named.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/core.c | 4 ++--
 drivers/base/power/opp/of.c   | 2 +-
 drivers/base/power/opp/opp.h  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index a8a5e01b7756..422482575954 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1014,7 +1014,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
 
-struct dev_pm_opp *_allocate_opp(struct device *dev,
+struct dev_pm_opp *_opp_allocate(struct device *dev,
 				 struct opp_table **opp_table)
 {
 	struct dev_pm_opp *opp;
@@ -1167,7 +1167,7 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 	/* Hold our table modification lock here */
 	mutex_lock(&opp_table_lock);
 
-	new_opp = _allocate_opp(dev, &opp_table);
+	new_opp = _opp_allocate(dev, &opp_table);
 	if (!new_opp) {
 		ret = -ENOMEM;
 		goto unlock;
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 996ca3b42f47..f8512ca2bf41 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -287,7 +287,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	/* Hold our table modification lock here */
 	mutex_lock(&opp_table_lock);
 
-	new_opp = _allocate_opp(dev, &opp_table);
+	new_opp = _opp_allocate(dev, &opp_table);
 	if (!new_opp) {
 		ret = -ENOMEM;
 		goto unlock;
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index af9f2b849a66..0f23a1059605 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -193,7 +193,7 @@ struct opp_table {
 struct opp_table *_find_opp_table(struct device *dev);
 struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
 void _dev_pm_opp_remove_table(struct device *dev, bool remove_all);
-struct dev_pm_opp *_allocate_opp(struct device *dev, struct opp_table **opp_table);
+struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table);
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
 void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp, bool notify);
 int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, bool dynamic);
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 04/10] PM / OPP: Error out on failing to add static OPPs for v1 bindings
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
                   ` (2 preceding siblings ...)
  2017-01-02  9:10 ` [PATCH V3 03/10] PM / OPP: Rename _allocate_opp() to _opp_allocate() Viresh Kumar
@ 2017-01-02  9:10 ` Viresh Kumar
  2017-01-02  9:10 ` [PATCH V3 05/10] PM / OPP: Add light weight _opp_free() routine Viresh Kumar
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:10 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

The code adding static OPPs for V2 bindings already does so. Make the V1
bindings specific code behave the same.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/of.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index f8512ca2bf41..c8fe815774ff 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -433,7 +433,7 @@ static int _of_add_opp_table_v1(struct device *dev)
 {
 	const struct property *prop;
 	const __be32 *val;
-	int nr;
+	int nr, ret;
 
 	prop = of_find_property(dev->of_node, "operating-points", NULL);
 	if (!prop)
@@ -456,9 +456,13 @@ static int _of_add_opp_table_v1(struct device *dev)
 		unsigned long freq = be32_to_cpup(val++) * 1000;
 		unsigned long volt = be32_to_cpup(val++);
 
-		if (_opp_add_v1(dev, freq, volt, false))
-			dev_warn(dev, "%s: Failed to add OPP %ld\n",
-				 __func__, freq);
+		ret = _opp_add_v1(dev, freq, volt, false);
+		if (ret) {
+			dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
+				__func__, freq, ret);
+			dev_pm_opp_of_remove_table(dev);
+			return ret;
+		}
 		nr -= 2;
 	}
 
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 05/10] PM / OPP: Add light weight _opp_free() routine
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
                   ` (3 preceding siblings ...)
  2017-01-02  9:10 ` [PATCH V3 04/10] PM / OPP: Error out on failing to add static OPPs for v1 bindings Viresh Kumar
@ 2017-01-02  9:10 ` Viresh Kumar
  2017-01-02  9:11 ` [PATCH V3 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table() Viresh Kumar
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:10 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

The OPPs which are never successfully added using _opp_add() are not
required to be freed with the _opp_remove() routine, as a simple kfree()
is enough for them.

Introduce a new light weight routine _opp_free(), which will do that.

That also helps us removing the 'notify' parameter to _opp_remove(),
which isn't required anymore.

Note that _opp_free() contains a call to _remove_opp_table() as the OPP
table might have been added for this very OPP only. The
_remove_opp_table() routine returns quickly if there are more OPPs in
the table. This will be simplified in later patches though.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/core.c | 20 +++++++++++---------
 drivers/base/power/opp/of.c   |  2 +-
 drivers/base/power/opp/opp.h  |  2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 422482575954..ea5b90e0882c 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -929,6 +929,12 @@ static void _remove_opp_table(struct opp_table *opp_table)
 		  _kfree_device_rcu);
 }
 
+void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table)
+{
+	kfree(opp);
+	_remove_opp_table(opp_table);
+}
+
 /**
  * _kfree_opp_rcu() - Free OPP RCU handler
  * @head:	RCU head
@@ -944,7 +950,6 @@ static void _kfree_opp_rcu(struct rcu_head *head)
  * _opp_remove()  - Remove an OPP from a table definition
  * @opp_table:	points back to the opp_table struct this opp belongs to
  * @opp:	pointer to the OPP to remove
- * @notify:	OPP_EVENT_REMOVE notification should be sent or not
  *
  * This function removes an opp definition from the opp table.
  *
@@ -952,16 +957,13 @@ static void _kfree_opp_rcu(struct rcu_head *head)
  * It is assumed that the caller holds required mutex for an RCU updater
  * strategy.
  */
-void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp,
-		 bool notify)
+static void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp)
 {
 	/*
 	 * Notify the changes in the availability of the operable
 	 * frequency/voltage list.
 	 */
-	if (notify)
-		srcu_notifier_call_chain(&opp_table->srcu_head,
-					 OPP_EVENT_REMOVE, opp);
+	srcu_notifier_call_chain(&opp_table->srcu_head, OPP_EVENT_REMOVE, opp);
 	opp_debug_remove_one(opp);
 	list_del_rcu(&opp->node);
 	call_srcu(&opp_table->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
@@ -1008,7 +1010,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 		goto unlock;
 	}
 
-	_opp_remove(opp_table, opp, true);
+	_opp_remove(opp_table, opp);
 unlock:
 	mutex_unlock(&opp_table_lock);
 }
@@ -1200,7 +1202,7 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 	return 0;
 
 free_opp:
-	_opp_remove(opp_table, new_opp, false);
+	_opp_free(new_opp, opp_table);
 unlock:
 	mutex_unlock(&opp_table_lock);
 	return ret;
@@ -1912,7 +1914,7 @@ void _dev_pm_opp_remove_table(struct device *dev, bool remove_all)
 		/* Free static OPPs */
 		list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) {
 			if (remove_all || !opp->dynamic)
-				_opp_remove(opp_table, opp, true);
+				_opp_remove(opp_table, opp);
 		}
 	} else {
 		_remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table);
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index c8fe815774ff..67c9eeded4e1 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -362,7 +362,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	return 0;
 
 free_opp:
-	_opp_remove(opp_table, new_opp, false);
+	_opp_free(new_opp, opp_table);
 unlock:
 	mutex_unlock(&opp_table_lock);
 	return ret;
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 0f23a1059605..334f7570df32 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -194,8 +194,8 @@ struct opp_table *_find_opp_table(struct device *dev);
 struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
 void _dev_pm_opp_remove_table(struct device *dev, bool remove_all);
 struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table);
+void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table);
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
-void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp, bool notify);
 int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, bool dynamic);
 void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of);
 
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table()
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
                   ` (4 preceding siblings ...)
  2017-01-02  9:10 ` [PATCH V3 05/10] PM / OPP: Add light weight _opp_free() routine Viresh Kumar
@ 2017-01-02  9:11 ` Viresh Kumar
  2017-01-02  9:11 ` [PATCH V3 07/10] PM / OPP: Don't allocate OPP table from _opp_allocate() Viresh Kumar
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:11 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

Later patches would want to remove OPP table (and its OPPs) using the
opp_table pointer instead of 'dev'.

In order to prepare for that, rename _dev_pm_opp_remove_table() as
_dev_pm_opp_find_and_remove_table() split out part of it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/core.c | 35 ++++++++++++++++++++++-------------
 drivers/base/power/opp/of.c   |  2 +-
 drivers/base/power/opp/opp.h  |  2 +-
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index ea5b90e0882c..b3a624a4af81 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1888,11 +1888,29 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier);
  * Free OPPs either created using static entries present in DT or even the
  * dynamically added entries based on remove_all param.
  */
-void _dev_pm_opp_remove_table(struct device *dev, bool remove_all)
+static void _dev_pm_opp_remove_table(struct opp_table *opp_table,
+				     struct device *dev, bool remove_all)
 {
-	struct opp_table *opp_table;
 	struct dev_pm_opp *opp, *tmp;
 
+	opp_rcu_lockdep_assert();
+
+	/* Find if opp_table manages a single device */
+	if (list_is_singular(&opp_table->dev_list)) {
+		/* Free static OPPs */
+		list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) {
+			if (remove_all || !opp->dynamic)
+				_opp_remove(opp_table, opp);
+		}
+	} else {
+		_remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table);
+	}
+}
+
+void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all)
+{
+	struct opp_table *opp_table;
+
 	/* Hold our table modification lock here */
 	mutex_lock(&opp_table_lock);
 
@@ -1909,16 +1927,7 @@ void _dev_pm_opp_remove_table(struct device *dev, bool remove_all)
 		goto unlock;
 	}
 
-	/* Find if opp_table manages a single device */
-	if (list_is_singular(&opp_table->dev_list)) {
-		/* Free static OPPs */
-		list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) {
-			if (remove_all || !opp->dynamic)
-				_opp_remove(opp_table, opp);
-		}
-	} else {
-		_remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table);
-	}
+	_dev_pm_opp_remove_table(opp_table, dev, remove_all);
 
 unlock:
 	mutex_unlock(&opp_table_lock);
@@ -1939,6 +1948,6 @@ void _dev_pm_opp_remove_table(struct device *dev, bool remove_all)
  */
 void dev_pm_opp_remove_table(struct device *dev)
 {
-	_dev_pm_opp_remove_table(dev, true);
+	_dev_pm_opp_find_and_remove_table(dev, true);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table);
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 67c9eeded4e1..442fa46c4f5c 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -238,7 +238,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
  */
 void dev_pm_opp_of_remove_table(struct device *dev)
 {
-	_dev_pm_opp_remove_table(dev, false);
+	_dev_pm_opp_find_and_remove_table(dev, false);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
 
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 334f7570df32..c4b539a8533a 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -192,7 +192,7 @@ struct opp_table {
 /* Routines internal to opp core */
 struct opp_table *_find_opp_table(struct device *dev);
 struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
-void _dev_pm_opp_remove_table(struct device *dev, bool remove_all);
+void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all);
 struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table);
 void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table);
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 07/10] PM / OPP: Don't allocate OPP table from _opp_allocate()
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
                   ` (5 preceding siblings ...)
  2017-01-02  9:11 ` [PATCH V3 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table() Viresh Kumar
@ 2017-01-02  9:11 ` Viresh Kumar
  2017-01-02  9:11 ` [PATCH V3 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate Viresh Kumar
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:11 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

There is no point in trying to find/allocate the table for every OPP
that is added for a device. It would be far more efficient to allocate
the table only once and pass its pointer to the routines that add the
OPP entry.

Locking is removed from _opp_add_static_v2() and _opp_add_v1() now as
the callers call them with that lock already held.

Call to _remove_opp_table() routine is also removed from _opp_free()
now, as opp_table isn't allocated from within _opp_allocate(). This is
handled by the routines which created the OPP table in the first place.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/core.c | 67 +++++++++++++++++++-------------------
 drivers/base/power/opp/of.c   | 75 ++++++++++++++++++++++---------------------
 drivers/base/power/opp/opp.h  |  9 ++++--
 3 files changed, 78 insertions(+), 73 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index b3a624a4af81..dde7dd099aa0 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -829,7 +829,7 @@ struct opp_device *_add_opp_dev(const struct device *dev,
  *
  * Return: valid opp_table pointer if success, else NULL.
  */
-static struct opp_table *_add_opp_table(struct device *dev)
+struct opp_table *_add_opp_table(struct device *dev)
 {
 	struct opp_table *opp_table;
 	struct opp_device *opp_dev;
@@ -929,10 +929,9 @@ static void _remove_opp_table(struct opp_table *opp_table)
 		  _kfree_device_rcu);
 }
 
-void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table)
+void _opp_free(struct dev_pm_opp *opp)
 {
 	kfree(opp);
-	_remove_opp_table(opp_table);
 }
 
 /**
@@ -1016,16 +1015,10 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
 
-struct dev_pm_opp *_opp_allocate(struct device *dev,
-				 struct opp_table **opp_table)
+struct dev_pm_opp *_opp_allocate(struct opp_table *table)
 {
 	struct dev_pm_opp *opp;
 	int count, supply_size;
-	struct opp_table *table;
-
-	table = _add_opp_table(dev);
-	if (!table)
-		return NULL;
 
 	/* Allocate space for at least one supply */
 	count = table->regulator_count ? table->regulator_count : 1;
@@ -1033,17 +1026,13 @@ struct dev_pm_opp *_opp_allocate(struct device *dev,
 
 	/* allocate new OPP node and supplies structures */
 	opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
-	if (!opp) {
-		kfree(table);
+	if (!opp)
 		return NULL;
-	}
 
 	/* Put the supplies at the end of the OPP structure as an empty array */
 	opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
 	INIT_LIST_HEAD(&opp->node);
 
-	*opp_table = table;
-
 	return opp;
 }
 
@@ -1133,6 +1122,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 
 /**
  * _opp_add_v1() - Allocate a OPP based on v1 bindings.
+ * @opp_table:	OPP table
  * @dev:	device for which we do this operation
  * @freq:	Frequency in Hz for this OPP
  * @u_volt:	Voltage in uVolts for this OPP
@@ -1158,22 +1148,18 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
  *		Duplicate OPPs (both freq and volt are same) and !opp->available
  * -ENOMEM	Memory allocation failure
  */
-int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
-		bool dynamic)
+int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
+		unsigned long freq, long u_volt, bool dynamic)
 {
-	struct opp_table *opp_table;
 	struct dev_pm_opp *new_opp;
 	unsigned long tol;
 	int ret;
 
-	/* Hold our table modification lock here */
-	mutex_lock(&opp_table_lock);
+	opp_rcu_lockdep_assert();
 
-	new_opp = _opp_allocate(dev, &opp_table);
-	if (!new_opp) {
-		ret = -ENOMEM;
-		goto unlock;
-	}
+	new_opp = _opp_allocate(opp_table);
+	if (!new_opp)
+		return -ENOMEM;
 
 	/* populate the opp table */
 	new_opp->rate = freq;
@@ -1192,8 +1178,6 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 		goto free_opp;
 	}
 
-	mutex_unlock(&opp_table_lock);
-
 	/*
 	 * Notify the changes in the availability of the operable
 	 * frequency/voltage list.
@@ -1202,9 +1186,8 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 	return 0;
 
 free_opp:
-	_opp_free(new_opp, opp_table);
-unlock:
-	mutex_unlock(&opp_table_lock);
+	_opp_free(new_opp);
+
 	return ret;
 }
 
@@ -1722,7 +1705,25 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_opp_helper);
  */
 int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 {
-	return _opp_add_v1(dev, freq, u_volt, true);
+	struct opp_table *opp_table;
+	int ret;
+
+	/* Hold our table modification lock here */
+	mutex_lock(&opp_table_lock);
+
+	opp_table = _add_opp_table(dev);
+	if (!opp_table) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	ret = _opp_add_v1(opp_table, dev, freq, u_volt, true);
+	if (ret)
+		_remove_opp_table(opp_table);
+
+unlock:
+	mutex_unlock(&opp_table_lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
 
@@ -1888,8 +1889,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier);
  * Free OPPs either created using static entries present in DT or even the
  * dynamically added entries based on remove_all param.
  */
-static void _dev_pm_opp_remove_table(struct opp_table *opp_table,
-				     struct device *dev, bool remove_all)
+void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
+			      bool remove_all)
 {
 	struct dev_pm_opp *opp, *tmp;
 
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 442fa46c4f5c..cdbf733ac9b1 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -255,6 +255,7 @@ static struct device_node *_of_get_opp_desc_node(struct device *dev)
 
 /**
  * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
+ * @opp_table:	OPP table
  * @dev:	device for which we do this operation
  * @np:		device node
  *
@@ -276,22 +277,17 @@ static struct device_node *_of_get_opp_desc_node(struct device *dev)
  * -ENOMEM	Memory allocation failure
  * -EINVAL	Failed parsing the OPP node
  */
-static int _opp_add_static_v2(struct device *dev, struct device_node *np)
+static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev,
+			      struct device_node *np)
 {
-	struct opp_table *opp_table;
 	struct dev_pm_opp *new_opp;
 	u64 rate;
 	u32 val;
 	int ret;
 
-	/* Hold our table modification lock here */
-	mutex_lock(&opp_table_lock);
-
-	new_opp = _opp_allocate(dev, &opp_table);
-	if (!new_opp) {
-		ret = -ENOMEM;
-		goto unlock;
-	}
+	new_opp = _opp_allocate(opp_table);
+	if (!new_opp)
+		return -ENOMEM;
 
 	ret = of_property_read_u64(np, "opp-hz", &rate);
 	if (ret < 0) {
@@ -347,8 +343,6 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	if (new_opp->clock_latency_ns > opp_table->clock_latency_ns_max)
 		opp_table->clock_latency_ns_max = new_opp->clock_latency_ns;
 
-	mutex_unlock(&opp_table_lock);
-
 	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
 		 __func__, new_opp->turbo, new_opp->rate,
 		 new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min,
@@ -362,9 +356,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	return 0;
 
 free_opp:
-	_opp_free(new_opp, opp_table);
-unlock:
-	mutex_unlock(&opp_table_lock);
+	_opp_free(new_opp);
+
 	return ret;
 }
 
@@ -382,16 +375,20 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 		/* OPPs are already managed */
 		if (!_add_opp_dev(dev, opp_table))
 			ret = -ENOMEM;
-		mutex_unlock(&opp_table_lock);
-		return ret;
+		goto unlock;
+	}
+
+	opp_table = _add_opp_table(dev);
+	if (!opp_table) {
+		ret = -ENOMEM;
+		goto unlock;
 	}
-	mutex_unlock(&opp_table_lock);
 
 	/* We have opp-table node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
 		count++;
 
-		ret = _opp_add_static_v2(dev, np);
+		ret = _opp_add_static_v2(opp_table, dev, np);
 		if (ret) {
 			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
 				ret);
@@ -400,15 +397,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 	}
 
 	/* There should be one of more OPP defined */
-	if (WARN_ON(!count))
-		return -ENOENT;
-
-	mutex_lock(&opp_table_lock);
-
-	opp_table = _find_opp_table(dev);
-	if (WARN_ON(IS_ERR(opp_table))) {
-		ret = PTR_ERR(opp_table);
-		mutex_unlock(&opp_table_lock);
+	if (WARN_ON(!count)) {
+		ret = -ENOENT;
 		goto free_table;
 	}
 
@@ -418,12 +408,12 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 	else
 		opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE;
 
-	mutex_unlock(&opp_table_lock);
-
-	return 0;
+	goto unlock;
 
 free_table:
-	dev_pm_opp_of_remove_table(dev);
+	_dev_pm_opp_remove_table(opp_table, dev, false);
+unlock:
+	mutex_unlock(&opp_table_lock);
 
 	return ret;
 }
@@ -431,9 +421,10 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 /* Initializes OPP tables based on old-deprecated bindings */
 static int _of_add_opp_table_v1(struct device *dev)
 {
+	struct opp_table *opp_table;
 	const struct property *prop;
 	const __be32 *val;
-	int nr, ret;
+	int nr, ret = 0;
 
 	prop = of_find_property(dev->of_node, "operating-points", NULL);
 	if (!prop)
@@ -451,22 +442,32 @@ static int _of_add_opp_table_v1(struct device *dev)
 		return -EINVAL;
 	}
 
+	mutex_lock(&opp_table_lock);
+
+	opp_table = _add_opp_table(dev);
+	if (!opp_table) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
 	val = prop->value;
 	while (nr) {
 		unsigned long freq = be32_to_cpup(val++) * 1000;
 		unsigned long volt = be32_to_cpup(val++);
 
-		ret = _opp_add_v1(dev, freq, volt, false);
+		ret = _opp_add_v1(opp_table, dev, freq, volt, false);
 		if (ret) {
 			dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
 				__func__, freq, ret);
-			dev_pm_opp_of_remove_table(dev);
-			return ret;
+			_dev_pm_opp_remove_table(opp_table, dev, false);
+			break;
 		}
 		nr -= 2;
 	}
 
-	return 0;
+unlock:
+	mutex_unlock(&opp_table_lock);
+	return ret;
 }
 
 /**
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index c4b539a8533a..0a5eb4d1e8f7 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -191,13 +191,16 @@ struct opp_table {
 
 /* Routines internal to opp core */
 struct opp_table *_find_opp_table(struct device *dev);
+struct opp_table *_add_opp_table(struct device *dev);
 struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
+void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev, bool remove_all);
 void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all);
-struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table);
-void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table);
+struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
+void _opp_free(struct dev_pm_opp *opp);
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
-int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, bool dynamic);
+int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
 void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of);
+struct opp_table *_add_opp_table(struct device *dev);
 
 #ifdef CONFIG_OF
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev);
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
                   ` (6 preceding siblings ...)
  2017-01-02  9:11 ` [PATCH V3 07/10] PM / OPP: Don't allocate OPP table from _opp_allocate() Viresh Kumar
@ 2017-01-02  9:11 ` Viresh Kumar
  2017-01-02  9:11 ` [PATCH V3 09/10] PM / OPP: Don't expose srcu_head to register notifiers Viresh Kumar
  2017-01-02  9:11 ` [PATCH V3 10/10] PM / OPP: Split out part of _add_opp_table() and _remove_opp_table() Viresh Kumar
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:11 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

There is only one user of dev_pm_opp_get_suspend_opp() and that uses it
to get the OPP rate for the suspend_opp.

Rename dev_pm_opp_get_suspend_opp() as dev_pm_opp_get_suspend_opp_freq()
and return the rate directly from it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/core.c | 27 +++++++++++++--------------
 drivers/cpufreq/cpufreq-dt.c  |  7 +------
 include/linux/pm_opp.h        |  6 +++---
 3 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index dde7dd099aa0..614d779ab6a2 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -328,32 +328,31 @@ unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_transition_latency);
 
 /**
- * dev_pm_opp_get_suspend_opp() - Get suspend opp
+ * dev_pm_opp_get_suspend_opp_freq() - Get frequency of suspend opp in Hz
  * @dev:	device for which we do this operation
  *
- * Return: This function returns pointer to the suspend opp if it is
- * defined and available, otherwise it returns NULL.
- *
- * Locking: This function must be called under rcu_read_lock(). opp is a rcu
- * protected pointer. The reason for the same is that the opp pointer which is
- * returned will remain valid for use with opp_get_{voltage, freq} only while
- * under the locked area. The pointer returned must be used prior to unlocking
- * with rcu_read_unlock() to maintain the integrity of the pointer.
+ * Return: This function returns the frequency of the OPP marked as suspend_opp
+ * if one is available, else returns 0;
  */
-struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev)
+unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
 {
 	struct opp_table *opp_table;
+	unsigned long freq = 0;
 
-	opp_rcu_lockdep_assert();
+	rcu_read_lock();
 
 	opp_table = _find_opp_table(dev);
 	if (IS_ERR(opp_table) || !opp_table->suspend_opp ||
 	    !opp_table->suspend_opp->available)
-		return NULL;
+		goto unlock;
 
-	return opp_table->suspend_opp;
+	freq = dev_pm_opp_get_freq(opp_table->suspend_opp);
+
+unlock:
+	rcu_read_unlock();
+	return freq;
 }
-EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp);
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
 
 /**
  * dev_pm_opp_get_opp_count() - Get number of opps available in the opp table
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 269013311e79..c943787d761e 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -148,7 +148,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	struct private_data *priv;
 	struct device *cpu_dev;
 	struct clk *cpu_clk;
-	struct dev_pm_opp *suspend_opp;
 	unsigned int transition_latency;
 	bool fallback = false;
 	const char *name;
@@ -252,11 +251,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	policy->driver_data = priv;
 	policy->clk = cpu_clk;
 
-	rcu_read_lock();
-	suspend_opp = dev_pm_opp_get_suspend_opp(cpu_dev);
-	if (suspend_opp)
-		policy->suspend_freq = dev_pm_opp_get_freq(suspend_opp) / 1000;
-	rcu_read_unlock();
+	policy->suspend_freq = dev_pm_opp_get_suspend_opp_freq(cpu_dev) / 1000;
 
 	ret = cpufreq_table_validate_and_show(policy, freq_table);
 	if (ret) {
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0edd88f93904..7483ff291a8e 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -88,7 +88,7 @@ int dev_pm_opp_get_opp_count(struct device *dev);
 unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev);
 unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev);
 unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev);
-struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev);
+unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 					      unsigned long freq,
@@ -159,9 +159,9 @@ static inline unsigned long dev_pm_opp_get_max_transition_latency(struct device
 	return 0;
 }
 
-static inline struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev)
+static inline unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
 {
-	return NULL;
+	return 0;
 }
 
 static inline struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 09/10] PM / OPP: Don't expose srcu_head to register notifiers
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
                   ` (7 preceding siblings ...)
  2017-01-02  9:11 ` [PATCH V3 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate Viresh Kumar
@ 2017-01-02  9:11 ` Viresh Kumar
  2017-01-02  9:11 ` [PATCH V3 10/10] PM / OPP: Split out part of _add_opp_table() and _remove_opp_table() Viresh Kumar
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:11 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	MyungJoo Ham, Kyungmin Park
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

Let the OPP core provide helpers to register notifiers for any device,
instead of exposing srcu_head outside of the core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/core.c | 66 ++++++++++++++++++++++++++++++++-----------
 drivers/devfreq/devfreq.c     | 26 ++---------------
 include/linux/pm_opp.h        | 14 ++++++---
 3 files changed, 62 insertions(+), 44 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 614d779ab6a2..f35effad60d1 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1860,29 +1860,63 @@ int dev_pm_opp_disable(struct device *dev, unsigned long freq)
 EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
 
 /**
- * dev_pm_opp_get_notifier() - find notifier_head of the device with opp
- * @dev:	device pointer used to lookup OPP table.
+ * dev_pm_opp_register_notifier() - Register OPP notifier for the device
+ * @dev:	Device for which notifier needs to be registered
+ * @nb:		Notifier block to be registered
  *
- * Return: pointer to  notifier head if found, otherwise -ENODEV or
- * -EINVAL based on type of error casted as pointer. value must be checked
- *  with IS_ERR to determine valid pointer or error result.
+ * Return: 0 on success or a negative error value.
+ */
+int dev_pm_opp_register_notifier(struct device *dev, struct notifier_block *nb)
+{
+	struct opp_table *opp_table;
+	int ret;
+
+	rcu_read_lock();
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		ret = PTR_ERR(opp_table);
+		goto unlock;
+	}
+
+	ret = srcu_notifier_chain_register(&opp_table->srcu_head, nb);
+
+unlock:
+	rcu_read_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL(dev_pm_opp_register_notifier);
+
+/**
+ * dev_pm_opp_unregister_notifier() - Unregister OPP notifier for the device
+ * @dev:	Device for which notifier needs to be unregistered
+ * @nb:		Notifier block to be unregistered
  *
- * Locking: This function must be called under rcu_read_lock(). opp_table is a
- * RCU protected pointer. The reason for the same is that the opp pointer which
- * is returned will remain valid for use with opp_get_{voltage, freq} only while
- * under the locked area. The pointer returned must be used prior to unlocking
- * with rcu_read_unlock() to maintain the integrity of the pointer.
+ * Return: 0 on success or a negative error value.
  */
-struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev)
+int dev_pm_opp_unregister_notifier(struct device *dev,
+				   struct notifier_block *nb)
 {
-	struct opp_table *opp_table = _find_opp_table(dev);
+	struct opp_table *opp_table;
+	int ret;
 
-	if (IS_ERR(opp_table))
-		return ERR_CAST(opp_table); /* matching type */
+	rcu_read_lock();
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		ret = PTR_ERR(opp_table);
+		goto unlock;
+	}
+
+	ret = srcu_notifier_chain_unregister(&opp_table->srcu_head, nb);
 
-	return &opp_table->srcu_head;
+unlock:
+	rcu_read_unlock();
+
+	return ret;
 }
-EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier);
+EXPORT_SYMBOL(dev_pm_opp_unregister_notifier);
 
 /*
  * Free OPPs either created using static entries present in DT or even the
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index a324801d6a66..b0de42972b74 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1260,18 +1260,7 @@ EXPORT_SYMBOL(devfreq_recommended_opp);
  */
 int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
 {
-	struct srcu_notifier_head *nh;
-	int ret = 0;
-
-	rcu_read_lock();
-	nh = dev_pm_opp_get_notifier(dev);
-	if (IS_ERR(nh))
-		ret = PTR_ERR(nh);
-	rcu_read_unlock();
-	if (!ret)
-		ret = srcu_notifier_chain_register(nh, &devfreq->nb);
-
-	return ret;
+	return dev_pm_opp_register_notifier(dev, &devfreq->nb);
 }
 EXPORT_SYMBOL(devfreq_register_opp_notifier);
 
@@ -1287,18 +1276,7 @@ EXPORT_SYMBOL(devfreq_register_opp_notifier);
  */
 int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
 {
-	struct srcu_notifier_head *nh;
-	int ret = 0;
-
-	rcu_read_lock();
-	nh = dev_pm_opp_get_notifier(dev);
-	if (IS_ERR(nh))
-		ret = PTR_ERR(nh);
-	rcu_read_unlock();
-	if (!ret)
-		ret = srcu_notifier_chain_unregister(nh, &devfreq->nb);
-
-	return ret;
+	return dev_pm_opp_unregister_notifier(dev, &devfreq->nb);
 }
 EXPORT_SYMBOL(devfreq_unregister_opp_notifier);
 
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 7483ff291a8e..66a02deeb03f 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -108,7 +108,9 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
 
 int dev_pm_opp_disable(struct device *dev, unsigned long freq);
 
-struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
+int dev_pm_opp_register_notifier(struct device *dev, struct notifier_block *nb);
+int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nb);
+
 int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
 				unsigned int count);
 void dev_pm_opp_put_supported_hw(struct device *dev);
@@ -202,10 +204,14 @@ static inline int dev_pm_opp_disable(struct device *dev, unsigned long freq)
 	return 0;
 }
 
-static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
-							struct device *dev)
+static inline int dev_pm_opp_register_notifier(struct device *dev, struct notifier_block *nb)
 {
-	return ERR_PTR(-ENOTSUPP);
+	return -ENOTSUPP;
+}
+
+static inline int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nb)
+{
+	return -ENOTSUPP;
 }
 
 static inline int dev_pm_opp_set_supported_hw(struct device *dev,
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V3 10/10] PM / OPP: Split out part of _add_opp_table() and _remove_opp_table()
  2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
                   ` (8 preceding siblings ...)
  2017-01-02  9:11 ` [PATCH V3 09/10] PM / OPP: Don't expose srcu_head to register notifiers Viresh Kumar
@ 2017-01-02  9:11 ` Viresh Kumar
  9 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2017-01-02  9:11 UTC (permalink / raw)
  To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, Viresh Kumar

Split out parts of _add_opp_table() and _remove_opp_table() into
separate routines. This improves readability as well.

Should result in no functional changes.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/base/power/opp/core.c | 76 +++++++++++++++++++++++++------------------
 1 file changed, 44 insertions(+), 32 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index f35effad60d1..622dd32f8dda 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -819,26 +819,12 @@ struct opp_device *_add_opp_dev(const struct device *dev,
 	return opp_dev;
 }
 
-/**
- * _add_opp_table() - Find OPP table or allocate a new one
- * @dev:	device for which we do this operation
- *
- * It tries to find an existing table first, if it couldn't find one, it
- * allocates a new OPP table and returns that.
- *
- * Return: valid opp_table pointer if success, else NULL.
- */
-struct opp_table *_add_opp_table(struct device *dev)
+static struct opp_table *_allocate_opp_table(struct device *dev)
 {
 	struct opp_table *opp_table;
 	struct opp_device *opp_dev;
 	int ret;
 
-	/* Check for existing table for 'dev' first */
-	opp_table = _find_opp_table(dev);
-	if (!IS_ERR(opp_table))
-		return opp_table;
-
 	/*
 	 * Allocate a new OPP table. In the infrequent case where a new
 	 * device is needed to be added, we pay this penalty.
@@ -875,6 +861,27 @@ struct opp_table *_add_opp_table(struct device *dev)
 }
 
 /**
+ * _add_opp_table() - Find OPP table or allocate a new one
+ * @dev:	device for which we do this operation
+ *
+ * It tries to find an existing table first, if it couldn't find one, it
+ * allocates a new OPP table and returns that.
+ *
+ * Return: valid opp_table pointer if success, else NULL.
+ */
+struct opp_table *_add_opp_table(struct device *dev)
+{
+	struct opp_table *opp_table;
+
+	/* Check for existing table for 'dev' first */
+	opp_table = _find_opp_table(dev);
+	if (!IS_ERR(opp_table))
+		return opp_table;
+
+	return _allocate_opp_table(dev);
+}
+
+/**
  * _kfree_device_rcu() - Free opp_table RCU handler
  * @head:	RCU head
  */
@@ -886,6 +893,27 @@ static void _kfree_device_rcu(struct rcu_head *head)
 	kfree_rcu(opp_table, rcu_head);
 }
 
+static void _free_opp_table(struct opp_table *opp_table)
+{
+	struct opp_device *opp_dev;
+
+	/* Release clk */
+	if (!IS_ERR(opp_table->clk))
+		clk_put(opp_table->clk);
+
+	opp_dev = list_first_entry(&opp_table->dev_list, struct opp_device,
+				   node);
+
+	_remove_opp_dev(opp_dev, opp_table);
+
+	/* dev_list must be empty now */
+	WARN_ON(!list_empty(&opp_table->dev_list));
+
+	list_del_rcu(&opp_table->node);
+	call_srcu(&opp_table->srcu_head.srcu, &opp_table->rcu_head,
+		  _kfree_device_rcu);
+}
+
 /**
  * _remove_opp_table() - Removes a OPP table
  * @opp_table: OPP table to be removed.
@@ -894,8 +922,6 @@ static void _kfree_device_rcu(struct rcu_head *head)
  */
 static void _remove_opp_table(struct opp_table *opp_table)
 {
-	struct opp_device *opp_dev;
-
 	if (!list_empty(&opp_table->opp_list))
 		return;
 
@@ -911,21 +937,7 @@ static void _remove_opp_table(struct opp_table *opp_table)
 	if (opp_table->set_opp)
 		return;
 
-	/* Release clk */
-	if (!IS_ERR(opp_table->clk))
-		clk_put(opp_table->clk);
-
-	opp_dev = list_first_entry(&opp_table->dev_list, struct opp_device,
-				   node);
-
-	_remove_opp_dev(opp_dev, opp_table);
-
-	/* dev_list must be empty now */
-	WARN_ON(!list_empty(&opp_table->dev_list));
-
-	list_del_rcu(&opp_table->node);
-	call_srcu(&opp_table->srcu_head.srcu, &opp_table->rcu_head,
-		  _kfree_device_rcu);
+	_free_opp_table(opp_table);
 }
 
 void _opp_free(struct dev_pm_opp *opp)
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-01-02  9:13 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-02  9:10 [PATCH V3 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
2017-01-02  9:10 ` [PATCH V3 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs Viresh Kumar
2017-01-02  9:10 ` [PATCH V3 02/10] PM / OPP: Remove useless TODO Viresh Kumar
2017-01-02  9:10 ` [PATCH V3 03/10] PM / OPP: Rename _allocate_opp() to _opp_allocate() Viresh Kumar
2017-01-02  9:10 ` [PATCH V3 04/10] PM / OPP: Error out on failing to add static OPPs for v1 bindings Viresh Kumar
2017-01-02  9:10 ` [PATCH V3 05/10] PM / OPP: Add light weight _opp_free() routine Viresh Kumar
2017-01-02  9:11 ` [PATCH V3 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table() Viresh Kumar
2017-01-02  9:11 ` [PATCH V3 07/10] PM / OPP: Don't allocate OPP table from _opp_allocate() Viresh Kumar
2017-01-02  9:11 ` [PATCH V3 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate Viresh Kumar
2017-01-02  9:11 ` [PATCH V3 09/10] PM / OPP: Don't expose srcu_head to register notifiers Viresh Kumar
2017-01-02  9:11 ` [PATCH V3 10/10] PM / OPP: Split out part of _add_opp_table() and _remove_opp_table() Viresh Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).