* [PATCH V2 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
@ 2016-12-07 4:41 ` Viresh Kumar
2016-12-22 0:39 ` Stephen Boyd
2016-12-07 4:41 ` [PATCH V2 02/10] PM / OPP: Remove useless TODO Viresh Kumar
` (9 subsequent siblings)
10 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:41 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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>
---
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 a0e6294baf1d..9ff816217c6e 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1080,6 +1080,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)
{
@@ -1112,7 +1122,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;
@@ -1186,8 +1196,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] 16+ messages in thread* Re: [PATCH V2 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs
2016-12-07 4:41 ` [PATCH V2 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs Viresh Kumar
@ 2016-12-22 0:39 ` Stephen Boyd
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2016-12-22 0:39 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Viresh Kumar, Nishanth Menon, linaro-kernel,
linux-pm, Vincent Guittot
On 12/07, Viresh Kumar wrote:
> 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
perhaps just "A)" instead of "A)."
> 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>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V2 02/10] PM / OPP: Remove useless TODO
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
2016-12-07 4:41 ` [PATCH V2 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs Viresh Kumar
@ 2016-12-07 4:41 ` Viresh Kumar
2016-12-07 4:41 ` [PATCH V2 03/10] PM / OPP: Rename _allocate_opp() to _opp_allocate() Viresh Kumar
` (8 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:41 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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] 16+ messages in thread* [PATCH V2 03/10] PM / OPP: Rename _allocate_opp() to _opp_allocate()
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
2016-12-07 4:41 ` [PATCH V2 01/10] PM / OPP: Fix memory leak while adding duplicate OPPs Viresh Kumar
2016-12-07 4:41 ` [PATCH V2 02/10] PM / OPP: Remove useless TODO Viresh Kumar
@ 2016-12-07 4:41 ` Viresh Kumar
2016-12-07 4:42 ` [PATCH V2 04/10] PM / OPP: Error out on failing to add static OPPs for v1 bindings Viresh Kumar
` (7 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:41 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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 9ff816217c6e..621a3dc94bfe 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1027,7 +1027,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;
@@ -1180,7 +1180,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] 16+ messages in thread* [PATCH V2 04/10] PM / OPP: Error out on failing to add static OPPs for v1 bindings
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
` (2 preceding siblings ...)
2016-12-07 4:41 ` [PATCH V2 03/10] PM / OPP: Rename _allocate_opp() to _opp_allocate() Viresh Kumar
@ 2016-12-07 4:42 ` Viresh Kumar
2016-12-07 4:42 ` [PATCH V2 05/10] PM / OPP: Add light weight _opp_free() routine Viresh Kumar
` (6 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:42 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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] 16+ messages in thread* [PATCH V2 05/10] PM / OPP: Add light weight _opp_free() routine
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
` (3 preceding siblings ...)
2016-12-07 4:42 ` [PATCH V2 04/10] PM / OPP: Error out on failing to add static OPPs for v1 bindings Viresh Kumar
@ 2016-12-07 4:42 ` Viresh Kumar
2016-12-07 4:42 ` [PATCH V2 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table() Viresh Kumar
` (5 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:42 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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 621a3dc94bfe..2f3dbca282ba 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -942,6 +942,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
@@ -957,7 +963,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.
*
@@ -965,16 +970,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);
@@ -1021,7 +1023,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);
}
@@ -1213,7 +1215,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;
@@ -1925,7 +1927,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] 16+ messages in thread* [PATCH V2 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table()
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
` (4 preceding siblings ...)
2016-12-07 4:42 ` [PATCH V2 05/10] PM / OPP: Add light weight _opp_free() routine Viresh Kumar
@ 2016-12-07 4:42 ` Viresh Kumar
2016-12-22 0:40 ` Stephen Boyd
2016-12-07 4:42 ` [PATCH V2 07/10] PM / OPP: Don't allocate OPP table from _opp_allocate() Viresh Kumar
` (4 subsequent siblings)
10 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:42 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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>
---
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 2f3dbca282ba..11496fbb2ee2 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1901,11 +1901,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);
@@ -1922,16 +1940,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);
@@ -1952,6 +1961,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] 16+ messages in thread* Re: [PATCH V2 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table()
2016-12-07 4:42 ` [PATCH V2 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table() Viresh Kumar
@ 2016-12-22 0:40 ` Stephen Boyd
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2016-12-22 0:40 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Viresh Kumar, Nishanth Menon, linaro-kernel,
linux-pm, Vincent Guittot
On 12/07, Viresh Kumar wrote:
> 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>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V2 07/10] PM / OPP: Don't allocate OPP table from _opp_allocate()
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
` (5 preceding siblings ...)
2016-12-07 4:42 ` [PATCH V2 06/10] PM / OPP: Rename and split _dev_pm_opp_remove_table() Viresh Kumar
@ 2016-12-07 4:42 ` Viresh Kumar
2016-12-07 4:42 ` [PATCH V2 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate Viresh Kumar
` (3 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:42 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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>
---
drivers/base/power/opp/core.c | 66 +++++++++++++++++++------------------
drivers/base/power/opp/of.c | 75 ++++++++++++++++++++++---------------------
drivers/base/power/opp/opp.h | 9 ++++--
3 files changed, 78 insertions(+), 72 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 11496fbb2ee2..6f61809c5760 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -842,7 +842,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;
@@ -942,10 +942,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);
}
/**
@@ -1030,15 +1029,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 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;
@@ -1046,17 +1040,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;
}
@@ -1146,6 +1136,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
@@ -1171,22 +1162,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(dev, opp_table);
+ if (!new_opp)
+ return -ENOMEM;
/* populate the opp table */
new_opp->rate = freq;
@@ -1205,8 +1192,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.
@@ -1215,9 +1200,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;
}
@@ -1735,7 +1719,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);
@@ -1901,8 +1903,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..38efc14d829c 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(dev, 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..e32dc80ddc12 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 device *dev, 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] 16+ messages in thread* [PATCH V2 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
` (6 preceding siblings ...)
2016-12-07 4:42 ` [PATCH V2 07/10] PM / OPP: Don't allocate OPP table from _opp_allocate() Viresh Kumar
@ 2016-12-07 4:42 ` Viresh Kumar
2016-12-22 0:37 ` Stephen Boyd
2016-12-07 4:42 ` [PATCH V2 09/10] PM / OPP: Don't expose srcu_head to register notifiers Viresh Kumar
` (2 subsequent siblings)
10 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:42 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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>
---
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 6f61809c5760..dfbe0610b675 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] 16+ messages in thread* Re: [PATCH V2 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate
2016-12-07 4:42 ` [PATCH V2 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate Viresh Kumar
@ 2016-12-22 0:37 ` Stephen Boyd
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2016-12-22 0:37 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Viresh Kumar, Nishanth Menon, linaro-kernel,
linux-pm, Vincent Guittot
On 12/07, Viresh Kumar wrote:
> 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>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V2 09/10] PM / OPP: Don't expose srcu_head to register notifiers
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
` (7 preceding siblings ...)
2016-12-07 4:42 ` [PATCH V2 08/10] PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate Viresh Kumar
@ 2016-12-07 4:42 ` Viresh Kumar
2016-12-07 4:42 ` [PATCH V2 10/10] PM / OPP: Split out part of _add_opp_table() and _remove_opp_table() Viresh Kumar
2016-12-08 0:07 ` [PATCH V2 00/10] PM / OPP: Fixes and cleanups Rafael J. Wysocki
10 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:42 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd,
MyungJoo Ham, Kyungmin Park
Cc: linaro-kernel, linux-pm, 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 dfbe0610b675..074a42185819 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1874,29 +1874,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] 16+ messages in thread* [PATCH V2 10/10] PM / OPP: Split out part of _add_opp_table() and _remove_opp_table()
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
` (8 preceding siblings ...)
2016-12-07 4:42 ` [PATCH V2 09/10] PM / OPP: Don't expose srcu_head to register notifiers Viresh Kumar
@ 2016-12-07 4:42 ` Viresh Kumar
2016-12-08 0:07 ` [PATCH V2 00/10] PM / OPP: Fixes and cleanups Rafael J. Wysocki
10 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-12-07 4:42 UTC (permalink / raw)
To: Rafael Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: linaro-kernel, linux-pm, 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 074a42185819..7db672f632d9 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -832,26 +832,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.
@@ -888,6 +874,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
*/
@@ -899,6 +906,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.
@@ -907,8 +935,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;
@@ -924,21 +950,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] 16+ messages in thread* Re: [PATCH V2 00/10] PM / OPP: Fixes and cleanups
2016-12-07 4:41 [PATCH V2 00/10] PM / OPP: Fixes and cleanups Viresh Kumar
` (9 preceding siblings ...)
2016-12-07 4:42 ` [PATCH V2 10/10] PM / OPP: Split out part of _add_opp_table() and _remove_opp_table() Viresh Kumar
@ 2016-12-08 0:07 ` Rafael J. Wysocki
2016-12-08 4:03 ` Viresh Kumar
10 siblings, 1 reply; 16+ messages in thread
From: Rafael J. Wysocki @ 2016-12-08 0:07 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Lists linaro-kernel, Linux PM, Stephen Boyd,
Nishanth Menon, Vincent Guittot
On Wed, Dec 7, 2016 at 5:41 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi,
>
> This series fixes a memory leak (first patch) and does some cleanups to
> the OPP core.
>
> It is based of pm/bleeding-edge branch.
BTW, please don't post patches based on bleeding-edge. That branch is
volatile (which you should be aware of) and it is not guaranteed that
the patches will apply to anything else going forward.
It simply is not reasonable to expect that reviewers will pull my
bleeding-edge branch just in order to review your patches (which may
become inapplicable going forward anyway).
Thanks,
Rafael
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH V2 00/10] PM / OPP: Fixes and cleanups
2016-12-08 0:07 ` [PATCH V2 00/10] PM / OPP: Fixes and cleanups Rafael J. Wysocki
@ 2016-12-08 4:03 ` Viresh Kumar
0 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-12-08 4:03 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael Wysocki, Lists linaro-kernel, Linux PM, Stephen Boyd,
Nishanth Menon, Vincent Guittot
On 08-12-16, 01:07, Rafael J. Wysocki wrote:
> On Wed, Dec 7, 2016 at 5:41 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Hi,
> >
> > This series fixes a memory leak (first patch) and does some cleanups to
> > the OPP core.
> >
> > It is based of pm/bleeding-edge branch.
>
> BTW, please don't post patches based on bleeding-edge. That branch is
> volatile (which you should be aware of) and it is not guaranteed that
> the patches will apply to anything else going forward.
>
> It simply is not reasonable to expect that reviewers will pull my
> bleeding-edge branch just in order to review your patches (which may
> become inapplicable going forward anyway).
Sure. Will take care of it from now on. Thanks.
--
viresh
^ permalink raw reply [flat|nested] 16+ messages in thread