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. Though not compulsory, but it would be nice to get queued up for 4.10, of course once they are reviewed by other OPP guys.
This series is tested for few days by various build and boot bots: - Kernel CI (Linaro) - Fengguang Wu's bot (Intel)
-- 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 | 290 ++++++++++++++++++++++++++---------------- drivers/base/power/opp/of.c | 90 +++++++------ 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, 249 insertions(+), 195 deletions(-)
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 | 14 ++++++++++++-- drivers/base/power/opp/of.c | 6 +++++- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index a0e6294baf1d..cc69f903fd34 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1080,6 +1080,12 @@ 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. + * -EEXIST: For OPP with same freq but different volt or is unavailable. + */ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table) { @@ -1112,7 +1118,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 +1192,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")) {
On 12/06, 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 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
Makes sense.
drivers/base/power/opp/core.c | 14 ++++++++++++-- drivers/base/power/opp/of.c | 6 +++++- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index a0e6294baf1d..cc69f903fd34 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1080,6 +1080,12 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, return true; } +/*
- Returns:
- 0: On success. And appropriate error message for Duplicate OPPs.
lowercase duplicate please
- -EBUSY: For OPP with same freq/volt and is available.
- -EEXIST: For OPP with same freq but different volt or is unavailable.
- */
int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table) { @@ -1112,7 +1118,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 +1192,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 */
Yes, but why?
if (ret == -EBUSY)ret = 0;- }
mutex_unlock(&opp_table_lock);
On 06-12-16, 17:09, Stephen Boyd wrote:
@@ -1186,8 +1192,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 */Yes, but why?
Will update the comment over _opp_add() to describe that.
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 --- 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. */
On 12/06, Viresh Kumar wrote:
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
Was this for multiple phandles style of binding that we never merged?
On 06-12-16, 17:10, Stephen Boyd wrote:
On 12/06, Viresh Kumar wrote:
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
Was this for multiple phandles style of binding that we never merged?
Yes, for something like slow and fast OPP tables. We solved that problem in a different way.
Make the naming consistent with how other routines are named.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.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 cc69f903fd34..4e070d58d537 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; @@ -1176,7 +1176,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);
On 12/06, Viresh Kumar wrote:
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
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 --- 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; }
On 12/06, Viresh Kumar wrote:
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
It looks like that goes back to the original DT OPP binding support written by Shawn Guo. Any idea why we kept trying to add more OPPs if it failed to add one? Best effort or something?
On 06-12-16, 17:17, Stephen Boyd wrote:
On 12/06, Viresh Kumar wrote:
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
It looks like that goes back to the original DT OPP binding support written by Shawn Guo. Any idea why we kept trying to add more OPPs if it failed to add one? Best effort or something?
Not sure, maybe for handling duplicate OPPs.
On 12/07, Viresh Kumar wrote:
On 06-12-16, 17:17, Stephen Boyd wrote:
On 12/06, Viresh Kumar wrote:
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
It looks like that goes back to the original DT OPP binding support written by Shawn Guo. Any idea why we kept trying to add more OPPs if it failed to add one? Best effort or something?
Not sure, maybe for handling duplicate OPPs.
So perhaps we should Cc Shawn Guo who wrote the original code then?
Hi Shawn,
On 07-12-16, 13:13, Stephen Boyd wrote:
On 12/07, Viresh Kumar wrote:
On 06-12-16, 17:17, Stephen Boyd wrote:
On 12/06, Viresh Kumar wrote:
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
It looks like that goes back to the original DT OPP binding support written by Shawn Guo. Any idea why we kept trying to add more OPPs if it failed to add one? Best effort or something?
Can you please answer this question from Stephen?
Not sure, maybe for handling duplicate OPPs.
So perhaps we should Cc Shawn Guo who wrote the original code then?
I wouldn't mind doing that, but I don't think it needs to be a blocker for now.
On Thu, Dec 08, 2016 at 09:00:59AM +0530, Viresh Kumar wrote:
Hi Shawn,
On 07-12-16, 13:13, Stephen Boyd wrote:
On 12/07, Viresh Kumar wrote:
On 06-12-16, 17:17, Stephen Boyd wrote:
On 12/06, Viresh Kumar wrote:
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
It looks like that goes back to the original DT OPP binding support written by Shawn Guo. Any idea why we kept trying to add more OPPs if it failed to add one? Best effort or something?
Can you please answer this question from Stephen?
Yes, it's the best effort. For example, a duplicated OPP shouldn't be a stopper for the driver.
Shawn
On 08-12-16, 14:39, Shawn Guo wrote:
Yes, it's the best effort.
That may get a risky IMO. What if we fail to add all OPPs except the last one, which selects the highest frequency? We will surely overheat most of the SoCs and burn them out.
I think its better to fail and return in such cases.
For example, a duplicated OPP shouldn't be a stopper for the driver.
That's a different case and is already supported properly, as we wouldn't fail in that case.
Thanks for your inputs Shawn.
On Thu, Dec 08, 2016 at 12:15:13PM +0530, Viresh Kumar wrote:
On 08-12-16, 14:39, Shawn Guo wrote:
Yes, it's the best effort.
That may get a risky IMO. What if we fail to add all OPPs except the last one, which selects the highest frequency?
That's the same case when Performance governor is selected, and should be guaranteed safe, shouldn't it?
We will surely overheat most of the SoCs and burn them out.
IMO, the problem is on OPP table. Every entry in the table should be safe for running for any time.
Shawn
On 8 December 2016 at 19:57, Shawn Guo shawnguo@kernel.org wrote:
That's the same case when Performance governor is selected, and should be guaranteed safe, shouldn't it?
Depends on platform I would say. For example for a big LITTLE platform, running the big core on max not only increases the amount of power it uses but also the amount of heat it generates, which is really scary.
Performance governor is fine for testing but it is not used for battery operated devices in production code.
We will surely overheat most of the SoCs and burn them out.
IMO, the problem is on OPP table. Every entry in the table should be safe for running for any time.
Not necessarily. The highest OPPs might be suggested to run only for small period of time only. And there is nothing wrong in it.
Over that if some of the OPPs fail to get registered from the OPP table in DT, it surely hints at some sort of bugs in the code or DT.
Why should we try to go ahead if only a part of the DT table gets registered? I don't find a compelling reason for that. It will never happen normally anyway, as we wouldn't have any errors.
-- viresh
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 --- 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 4e070d58d537..ee5fbbd6da93 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); } @@ -1209,7 +1211,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; @@ -1921,7 +1923,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);
On 12/06, Viresh Kumar wrote:
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
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 | 33 ++++++++++++++++++++------------- drivers/base/power/opp/of.c | 2 +- drivers/base/power/opp/opp.h | 2 +- 3 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index ee5fbbd6da93..ef114cf9edcd 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1897,11 +1897,27 @@ 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;
+ /* 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);
@@ -1918,16 +1934,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); @@ -1948,6 +1955,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);
On 12/06, Viresh Kumar wrote:
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index ee5fbbd6da93..ef114cf9edcd 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1897,11 +1897,27 @@ 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;
Please add a comment or lockdep check to make sure we're holding the table lock.
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 | 68 +++++++++++++++++++------------------- drivers/base/power/opp/of.c | 76 ++++++++++++++++++++++--------------------- drivers/base/power/opp/opp.h | 9 +++-- 3 files changed, 79 insertions(+), 74 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index ef114cf9edcd..6bcbb64a5582 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,33 +1029,24 @@ 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 *opp_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; + count = opp_table->regulator_count ? opp_table->regulator_count : 1; supply_size = sizeof(*opp->supplies) * count;
/* 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; }
@@ -1142,6 +1132,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 @@ -1167,22 +1158,16 @@ 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); - - 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; @@ -1201,8 +1186,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. @@ -1211,9 +1194,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; }
@@ -1731,7 +1713,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);
@@ -1897,8 +1897,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..37dd79378f39 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,33 @@ static int _of_add_opp_table_v1(struct device *dev) return -EINVAL; }
+ /* Hold our table modification lock here */ + 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);
On 12/06, Viresh Kumar wrote:
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index ef114cf9edcd..6bcbb64a5582 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1030,33 +1029,24 @@ 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 *opp_table)
Please call it table instead.
{ struct dev_pm_opp *opp; int count, supply_size;
- struct opp_table *table;
- table = _add_opp_table(dev);
Is this the only user of dev? Why do we keep passing dev to this function then?
- if (!table)
return NULL;/* Allocate space for at least one supply */
- count = table->regulator_count ? table->regulator_count : 1;
- count = opp_table->regulator_count ? opp_table->regulator_count : 1;
We keep it table so that this doesn't change.
supply_size = sizeof(*opp->supplies) * count; @@ -1142,6 +1132,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
@@ -1167,22 +1158,16 @@ 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);
Can we have a mutex locked assertion here? Or a note in the comments that we assume the opp table lock is held?
- 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;
Also, now we call the srcu notifier chain with the opp_table_lock held? That seems not so good. Do we need to drop it and reaquire the lock across the table lock? Or perhaps we should rethink widening the lock this much across the notifier.
@@ -1731,7 +1713,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);
I'd call it table here too, given that we don't have other tables inside OPP anyway. But no problem either way.
- return ret;
} EXPORT_SYMBOL_GPL(dev_pm_opp_add); diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index 442fa46c4f5c..37dd79378f39 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -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,33 @@ static int _of_add_opp_table_v1(struct device *dev) return -EINVAL; }
- /* Hold our table modification lock here */
This comment is fairly worthless.
- 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);
dev_pm_opp_of_remove_table(dev);return ret;
_dev_pm_opp_remove_table(opp_table, dev, false);break;
- return 0;
+unlock:
- mutex_unlock(&opp_table_lock);
- return ret;
} /**
On 06-12-16, 17:02, Stephen Boyd wrote:
On 12/06, Viresh Kumar wrote:
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index ef114cf9edcd..6bcbb64a5582 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1030,33 +1029,24 @@ 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 *opp_table)Please call it table instead.
Sure.
{ struct dev_pm_opp *opp; int count, supply_size;
- struct opp_table *table;
- table = _add_opp_table(dev);
Is this the only user of dev? Why do we keep passing dev to this function then?
Because we are still working with struct list_dev, which needs to save a pointer to dev. We may simplify that with later series though, not sure yet.
+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);
Can we have a mutex locked assertion here? Or a note in the comments that we assume the opp table lock is held?
Done.
- 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;
Also, now we call the srcu notifier chain with the opp_table_lock held? That seems not so good. Do we need to drop it and reaquire the lock across the table lock? Or perhaps we should rethink widening the lock this much across the notifier.
Hmm, fair point but: - The OPP notifiers are used only by devfreq and no one else. So the most common case of cpufreq will be just fine. - The lock is taken across only OPP_EVENT_ADD event and that doesn't get called all the time. Normally it will happen only at boot (once for each OPP) and that's it. I am not sure if we should actually remove the notifier completely going forward. - Looking at devfreq implementation it seems that they are mostly interested in the updates to the OPP nodes. - The later series (which I may post today as this one is reviewed mostly), will simplify it a lot. The lock wouldn't be taken across any big parts as we will use kref instead. - So, I would like to keep this patch as is as this is going to be sorted out anyway.
@@ -1731,7 +1713,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);
I'd call it table here too, given that we don't have other tables inside OPP anyway. But no problem either way.
Its called as opp_table almost everywhere else in the core.
On 12/07, Viresh Kumar wrote:
On 06-12-16, 17:02, Stephen Boyd wrote:
On 12/06, Viresh Kumar wrote:
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index ef114cf9edcd..6bcbb64a5582 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1030,33 +1029,24 @@ 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 *opp_table)Please call it table instead.
Sure.
{ struct dev_pm_opp *opp; int count, supply_size;
- struct opp_table *table;
- table = _add_opp_table(dev);
Is this the only user of dev? Why do we keep passing dev to this function then?
Because we are still working with struct list_dev, which needs to save a pointer to dev. We may simplify that with later series though, not sure yet.
Sorry I don't understand. After this patch it looks like dev is unused in the function because we delete the only user _add_opp_table().
+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);
Can we have a mutex locked assertion here? Or a note in the comments that we assume the opp table lock is held?
Done.
- 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;
Also, now we call the srcu notifier chain with the opp_table_lock held? That seems not so good. Do we need to drop it and reaquire the lock across the table lock? Or perhaps we should rethink widening the lock this much across the notifier.
Hmm, fair point but:
- The OPP notifiers are used only by devfreq and no one else. So the most common case of cpufreq will be just fine.
- The lock is taken across only OPP_EVENT_ADD event and that doesn't get called all the time. Normally it will happen only at boot (once for each OPP) and that's it. I am not sure if we should actually remove the notifier completely going forward.
- Looking at devfreq implementation it seems that they are mostly interested in the updates to the OPP nodes.
- The later series (which I may post today as this one is reviewed mostly), will simplify it a lot. The lock wouldn't be taken across any big parts as we will use kref instead.
- So, I would like to keep this patch as is as this is going to be sorted out anyway.
I haven't looked at the next round of patches. If the lock is still held across the notifier for OPP_EVENT_ADD then it would seem that we should get some sort of lockdep splat if the notified functions want to call back into the OPP code and that takes the same lock we held across the notifier. Even if it would work for the other notifiers that aren't OPP_EVENT_ADD, lockdep wouldn't know that because of locking classes, hence the splat. That's my only concern. Obviously if the locking is going away then it's just a bisection hole problem that I'm willing to ignore.
On 07-12-16, 14:05, Stephen Boyd wrote:
Sorry I don't understand. After this patch it looks like dev is unused in the function because we delete the only user _add_opp_table().
Sorry, I misread your earlier comment :(
I haven't looked at the next round of patches. If the lock is still held across the notifier for OPP_EVENT_ADD then it would seem that we should get some sort of lockdep splat if the notified functions want to call back into the OPP code and that takes the same lock we held across the notifier.
Right.
Even if it would work for the other notifiers that aren't OPP_EVENT_ADD, lockdep wouldn't know that because of locking classes, hence the splat.
yes.
That's my only concern. Obviously if the locking is going away then it's just a bisection hole problem that I'm willing to ignore.
Yes it is surely going away and we wouldn't get the splat for now as well. I already confirmed that only devfreq uses OPP notifiers. I can also confirm that only two devfreq drivers (rk3399 and exynos) register the notifiers and they do that after adding the OPP table.
IOW, no one is listening to OPP_ADD event for now.
Here is the new version of the patch. I will send the complete series only after all the patches are reviewed now..
-------------------------8<-------------------------
Subject: [PATCH] PM / OPP: Don't allocate OPP table from _opp_allocate()
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 | 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);
On 12/08, Viresh Kumar wrote:
Subject: [PATCH] PM / OPP: Don't allocate OPP table from _opp_allocate()
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
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 6bcbb64a5582..6fd098618683 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 * @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) / 1000; + +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..d0a53119d8a2 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);
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,
On 12/06, 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
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 6bcbb64a5582..6fd098618683 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
Please indicate units of frequency.
- @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) / 1000;
Why do we include the / 1000? Perhaps we should just return freq and have the caller divide.
+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
@@ -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;
Dividing 0 by anything is just 0 anyway.
}
On 06-12-16, 17:21, Stephen Boyd wrote:
On 12/06, 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
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 6bcbb64a5582..6fd098618683 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
Please indicate units of frequency.
Sure.
- @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) / 1000;
Why do we include the / 1000? Perhaps we should just return freq and have the caller divide.
Stupid enough. Thanks for pointing out.
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 --- 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 6fd098618683..f4b935892f5b 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1868,29 +1868,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,
On 12/06, Viresh Kumar wrote:
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
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
Split out parts of _add_opp_table() and _remove_opp_table() into separate routines. This will be useful going forward (while doing further cleanups).
Should result in no functional changes.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.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 f4b935892f5b..9dd9d3287854 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)
On 12/06, Viresh Kumar wrote:
Split out parts of _add_opp_table() and _remove_opp_table() into separate routines. This will be useful going forward (while doing further cleanups).
Should result in no functional changes.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
But perhaps this should wait until we have patches that actually use them.
On 06-12-16, 17:24, Stephen Boyd wrote:
On 12/06, Viresh Kumar wrote:
Split out parts of _add_opp_table() and _remove_opp_table() into separate routines. This will be useful going forward (while doing further cleanups).
Should result in no functional changes.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
But perhaps this should wait until we have patches that actually use them.
Even if there are no more patches, it would be good to separate our allocation/removal to separate routines.
Will update the commit log a bit and will include your Reviewed-by tag. Thanks.
linaro-kernel@lists.linaro.org
-
Shawn Guo -
Stephen Boyd -
Viresh Kumar