Hi,
Happy new year guys !!
This series fixes a memory leak (first patch) and does some cleanups to the OPP core.
It is based of pm/linux-next branch.
This series is tested for few days by various build and boot bots: - Kernel CI (Linaro) - Fengguang Wu's bot (Intel)
V2->V3: - Only the 7th patch is updated based on Stephen's comment. - All the patches now have a Reviewed-by tag.
V1->V2: - 6 out of 10 patches have received Acks from Stephen/MyungJoo. - dev_pm_opp_get_suspend_opp_freq() return freq in Hz now - s/opp_table/table in _opp_allocate() - Improved comment over _opp_add() defining its return types - opp_rcu_lockdep_assert() added at few places - Dropped a useless comment
-- viresh
Viresh Kumar (10): PM / OPP: Fix memory leak while adding duplicate OPPs PM / OPP: Remove useless TODO PM / OPP: Rename _allocate_opp() to _opp_allocate() PM / OPP: Error out on failing to add static OPPs for v1 bindings PM / OPP: Add light weight _opp_free() routine PM / OPP: Rename and split _dev_pm_opp_remove_table() PM / OPP: Don't allocate OPP table from _opp_allocate() PM / OPP: Rename dev_pm_opp_get_suspend_opp() and return OPP rate PM / OPP: Don't expose srcu_head to register notifiers PM / OPP: Split out part of _add_opp_table() and _remove_opp_table()
drivers/base/power/opp/core.c | 293 ++++++++++++++++++++++++++---------------- drivers/base/power/opp/of.c | 89 +++++++------ drivers/base/power/opp/opp.h | 11 +- drivers/cpufreq/cpufreq-dt.c | 7 +- drivers/devfreq/devfreq.c | 26 +--- include/linux/pm_opp.h | 20 ++- 6 files changed, 253 insertions(+), 193 deletions(-)
There are two types of duplicate OPPs that get different behavior from the core: A) An earlier OPP is marked 'available' and has same freq/voltages as the new one. B) An earlier OPP with same frequency, but is marked 'unavailable' OR doesn't have same voltages as the new one.
The OPP core returns 0 for the first one, but -EEXIST for the second.
While the OPP core returns 0 for the first case, its callers don't free the newly allocated OPP structure which isn't used anymore. Fix that by returning -EBUSY instead of 0, but make the callers return 0 eventually.
As this isn't a critical fix, its not getting marked for stable kernel.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 18 ++++++++++++++++-- drivers/base/power/opp/of.c | 6 +++++- 2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 35ff06283738..a8a5e01b7756 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1067,6 +1067,16 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, return true; }
+/* + * Returns: + * 0: On success. And appropriate error message for duplicate OPPs. + * -EBUSY: For OPP with same freq/volt and is available. The callers of + * _opp_add() must return 0 if they receive -EBUSY from it. This is to make + * sure we don't print error messages unnecessarily if different parts of + * kernel try to initialize the OPP table. + * -EEXIST: For OPP with same freq but different volt or is unavailable. This + * should be considered an error by the callers of _opp_add(). + */ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table) { @@ -1099,7 +1109,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
/* Should we compare voltages for all regulators here ? */ return opp->available && - new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST; + new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? -EBUSY : -EEXIST; }
new_opp->opp_table = opp_table; @@ -1173,8 +1183,12 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, new_opp->dynamic = dynamic;
ret = _opp_add(dev, new_opp, opp_table); - if (ret) + if (ret) { + /* Don't return error for duplicate OPPs */ + if (ret == -EBUSY) + ret = 0; goto free_opp; + }
mutex_unlock(&opp_table_lock);
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index 3f7d2591b173..356c75edd656 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -327,8 +327,12 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) goto free_opp;
ret = _opp_add(dev, new_opp, opp_table); - if (ret) + if (ret) { + /* Don't return error for duplicate OPPs */ + if (ret == -EBUSY) + ret = 0; goto free_opp; + }
/* OPP to select on device suspend */ if (of_property_read_bool(np, "opp-suspend")) {
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. */
Make the naming consistent with how other routines are named.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 4 ++-- drivers/base/power/opp/of.c | 2 +- drivers/base/power/opp/opp.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index a8a5e01b7756..422482575954 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1014,7 +1014,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) } EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
-struct dev_pm_opp *_allocate_opp(struct device *dev, +struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table) { struct dev_pm_opp *opp; @@ -1167,7 +1167,7 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, /* Hold our table modification lock here */ mutex_lock(&opp_table_lock);
- new_opp = _allocate_opp(dev, &opp_table); + new_opp = _opp_allocate(dev, &opp_table); if (!new_opp) { ret = -ENOMEM; goto unlock; diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index 996ca3b42f47..f8512ca2bf41 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -287,7 +287,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) /* Hold our table modification lock here */ mutex_lock(&opp_table_lock);
- new_opp = _allocate_opp(dev, &opp_table); + new_opp = _opp_allocate(dev, &opp_table); if (!new_opp) { ret = -ENOMEM; goto unlock; diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index af9f2b849a66..0f23a1059605 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -193,7 +193,7 @@ struct opp_table { struct opp_table *_find_opp_table(struct device *dev); struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table); void _dev_pm_opp_remove_table(struct device *dev, bool remove_all); -struct dev_pm_opp *_allocate_opp(struct device *dev, struct opp_table **opp_table); +struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table); int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table); void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp, bool notify); int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, bool dynamic);
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; }
The OPPs which are never successfully added using _opp_add() are not required to be freed with the _opp_remove() routine, as a simple kfree() is enough for them.
Introduce a new light weight routine _opp_free(), which will do that.
That also helps us removing the 'notify' parameter to _opp_remove(), which isn't required anymore.
Note that _opp_free() contains a call to _remove_opp_table() as the OPP table might have been added for this very OPP only. The _remove_opp_table() routine returns quickly if there are more OPPs in the table. This will be simplified in later patches though.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 20 +++++++++++--------- drivers/base/power/opp/of.c | 2 +- drivers/base/power/opp/opp.h | 2 +- 3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 422482575954..ea5b90e0882c 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -929,6 +929,12 @@ static void _remove_opp_table(struct opp_table *opp_table) _kfree_device_rcu); }
+void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table) +{ + kfree(opp); + _remove_opp_table(opp_table); +} + /** * _kfree_opp_rcu() - Free OPP RCU handler * @head: RCU head @@ -944,7 +950,6 @@ static void _kfree_opp_rcu(struct rcu_head *head) * _opp_remove() - Remove an OPP from a table definition * @opp_table: points back to the opp_table struct this opp belongs to * @opp: pointer to the OPP to remove - * @notify: OPP_EVENT_REMOVE notification should be sent or not * * This function removes an opp definition from the opp table. * @@ -952,16 +957,13 @@ static void _kfree_opp_rcu(struct rcu_head *head) * It is assumed that the caller holds required mutex for an RCU updater * strategy. */ -void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp, - bool notify) +static void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp) { /* * Notify the changes in the availability of the operable * frequency/voltage list. */ - if (notify) - srcu_notifier_call_chain(&opp_table->srcu_head, - OPP_EVENT_REMOVE, opp); + srcu_notifier_call_chain(&opp_table->srcu_head, OPP_EVENT_REMOVE, opp); opp_debug_remove_one(opp); list_del_rcu(&opp->node); call_srcu(&opp_table->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu); @@ -1008,7 +1010,7 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) goto unlock; }
- _opp_remove(opp_table, opp, true); + _opp_remove(opp_table, opp); unlock: mutex_unlock(&opp_table_lock); } @@ -1200,7 +1202,7 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, return 0;
free_opp: - _opp_remove(opp_table, new_opp, false); + _opp_free(new_opp, opp_table); unlock: mutex_unlock(&opp_table_lock); return ret; @@ -1912,7 +1914,7 @@ void _dev_pm_opp_remove_table(struct device *dev, bool remove_all) /* Free static OPPs */ list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) { if (remove_all || !opp->dynamic) - _opp_remove(opp_table, opp, true); + _opp_remove(opp_table, opp); } } else { _remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table); diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index c8fe815774ff..67c9eeded4e1 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -362,7 +362,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) return 0;
free_opp: - _opp_remove(opp_table, new_opp, false); + _opp_free(new_opp, opp_table); unlock: mutex_unlock(&opp_table_lock); return ret; diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 0f23a1059605..334f7570df32 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -194,8 +194,8 @@ struct opp_table *_find_opp_table(struct device *dev); struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table); void _dev_pm_opp_remove_table(struct device *dev, bool remove_all); struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table); +void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table); int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table); -void _opp_remove(struct opp_table *opp_table, struct dev_pm_opp *opp, bool notify); int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, bool dynamic); void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of);
Later patches would want to remove OPP table (and its OPPs) using the opp_table pointer instead of 'dev'.
In order to prepare for that, rename _dev_pm_opp_remove_table() as _dev_pm_opp_find_and_remove_table() split out part of it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 35 ++++++++++++++++++++++------------- drivers/base/power/opp/of.c | 2 +- drivers/base/power/opp/opp.h | 2 +- 3 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index ea5b90e0882c..b3a624a4af81 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1888,11 +1888,29 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier); * Free OPPs either created using static entries present in DT or even the * dynamically added entries based on remove_all param. */ -void _dev_pm_opp_remove_table(struct device *dev, bool remove_all) +static void _dev_pm_opp_remove_table(struct opp_table *opp_table, + struct device *dev, bool remove_all) { - struct opp_table *opp_table; struct dev_pm_opp *opp, *tmp;
+ opp_rcu_lockdep_assert(); + + /* Find if opp_table manages a single device */ + if (list_is_singular(&opp_table->dev_list)) { + /* Free static OPPs */ + list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) { + if (remove_all || !opp->dynamic) + _opp_remove(opp_table, opp); + } + } else { + _remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table); + } +} + +void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all) +{ + struct opp_table *opp_table; + /* Hold our table modification lock here */ mutex_lock(&opp_table_lock);
@@ -1909,16 +1927,7 @@ void _dev_pm_opp_remove_table(struct device *dev, bool remove_all) goto unlock; }
- /* Find if opp_table manages a single device */ - if (list_is_singular(&opp_table->dev_list)) { - /* Free static OPPs */ - list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) { - if (remove_all || !opp->dynamic) - _opp_remove(opp_table, opp); - } - } else { - _remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table); - } + _dev_pm_opp_remove_table(opp_table, dev, remove_all);
unlock: mutex_unlock(&opp_table_lock); @@ -1939,6 +1948,6 @@ void _dev_pm_opp_remove_table(struct device *dev, bool remove_all) */ void dev_pm_opp_remove_table(struct device *dev) { - _dev_pm_opp_remove_table(dev, true); + _dev_pm_opp_find_and_remove_table(dev, true); } EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table); diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index 67c9eeded4e1..442fa46c4f5c 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -238,7 +238,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, */ void dev_pm_opp_of_remove_table(struct device *dev) { - _dev_pm_opp_remove_table(dev, false); + _dev_pm_opp_find_and_remove_table(dev, false); } EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 334f7570df32..c4b539a8533a 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -192,7 +192,7 @@ struct opp_table { /* Routines internal to opp core */ struct opp_table *_find_opp_table(struct device *dev); struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table); -void _dev_pm_opp_remove_table(struct device *dev, bool remove_all); +void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all); struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table); void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table); int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table);
There is no point in trying to find/allocate the table for every OPP that is added for a device. It would be far more efficient to allocate the table only once and pass its pointer to the routines that add the OPP entry.
Locking is removed from _opp_add_static_v2() and _opp_add_v1() now as the callers call them with that lock already held.
Call to _remove_opp_table() routine is also removed from _opp_free() now, as opp_table isn't allocated from within _opp_allocate(). This is handled by the routines which created the OPP table in the first place.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 67 +++++++++++++++++++------------------- drivers/base/power/opp/of.c | 75 ++++++++++++++++++++++--------------------- drivers/base/power/opp/opp.h | 9 ++++-- 3 files changed, 78 insertions(+), 73 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index b3a624a4af81..dde7dd099aa0 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -829,7 +829,7 @@ struct opp_device *_add_opp_dev(const struct device *dev, * * Return: valid opp_table pointer if success, else NULL. */ -static struct opp_table *_add_opp_table(struct device *dev) +struct opp_table *_add_opp_table(struct device *dev) { struct opp_table *opp_table; struct opp_device *opp_dev; @@ -929,10 +929,9 @@ static void _remove_opp_table(struct opp_table *opp_table) _kfree_device_rcu); }
-void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table) +void _opp_free(struct dev_pm_opp *opp) { kfree(opp); - _remove_opp_table(opp_table); }
/** @@ -1016,16 +1015,10 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) } EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
-struct dev_pm_opp *_opp_allocate(struct device *dev, - struct opp_table **opp_table) +struct dev_pm_opp *_opp_allocate(struct opp_table *table) { struct dev_pm_opp *opp; int count, supply_size; - struct opp_table *table; - - table = _add_opp_table(dev); - if (!table) - return NULL;
/* Allocate space for at least one supply */ count = table->regulator_count ? table->regulator_count : 1; @@ -1033,17 +1026,13 @@ struct dev_pm_opp *_opp_allocate(struct device *dev,
/* allocate new OPP node and supplies structures */ opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL); - if (!opp) { - kfree(table); + if (!opp) return NULL; - }
/* Put the supplies at the end of the OPP structure as an empty array */ opp->supplies = (struct dev_pm_opp_supply *)(opp + 1); INIT_LIST_HEAD(&opp->node);
- *opp_table = table; - return opp; }
@@ -1133,6 +1122,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
/** * _opp_add_v1() - Allocate a OPP based on v1 bindings. + * @opp_table: OPP table * @dev: device for which we do this operation * @freq: Frequency in Hz for this OPP * @u_volt: Voltage in uVolts for this OPP @@ -1158,22 +1148,18 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, * Duplicate OPPs (both freq and volt are same) and !opp->available * -ENOMEM Memory allocation failure */ -int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, - bool dynamic) +int _opp_add_v1(struct opp_table *opp_table, struct device *dev, + unsigned long freq, long u_volt, bool dynamic) { - struct opp_table *opp_table; struct dev_pm_opp *new_opp; unsigned long tol; int ret;
- /* Hold our table modification lock here */ - mutex_lock(&opp_table_lock); + opp_rcu_lockdep_assert();
- new_opp = _opp_allocate(dev, &opp_table); - if (!new_opp) { - ret = -ENOMEM; - goto unlock; - } + new_opp = _opp_allocate(opp_table); + if (!new_opp) + return -ENOMEM;
/* populate the opp table */ new_opp->rate = freq; @@ -1192,8 +1178,6 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, goto free_opp; }
- mutex_unlock(&opp_table_lock); - /* * Notify the changes in the availability of the operable * frequency/voltage list. @@ -1202,9 +1186,8 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, return 0;
free_opp: - _opp_free(new_opp, opp_table); -unlock: - mutex_unlock(&opp_table_lock); + _opp_free(new_opp); + return ret; }
@@ -1722,7 +1705,25 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_opp_helper); */ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) { - return _opp_add_v1(dev, freq, u_volt, true); + struct opp_table *opp_table; + int ret; + + /* Hold our table modification lock here */ + mutex_lock(&opp_table_lock); + + opp_table = _add_opp_table(dev); + if (!opp_table) { + ret = -ENOMEM; + goto unlock; + } + + ret = _opp_add_v1(opp_table, dev, freq, u_volt, true); + if (ret) + _remove_opp_table(opp_table); + +unlock: + mutex_unlock(&opp_table_lock); + return ret; } EXPORT_SYMBOL_GPL(dev_pm_opp_add);
@@ -1888,8 +1889,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier); * Free OPPs either created using static entries present in DT or even the * dynamically added entries based on remove_all param. */ -static void _dev_pm_opp_remove_table(struct opp_table *opp_table, - struct device *dev, bool remove_all) +void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev, + bool remove_all) { struct dev_pm_opp *opp, *tmp;
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index 442fa46c4f5c..cdbf733ac9b1 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -255,6 +255,7 @@ static struct device_node *_of_get_opp_desc_node(struct device *dev)
/** * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings) + * @opp_table: OPP table * @dev: device for which we do this operation * @np: device node * @@ -276,22 +277,17 @@ static struct device_node *_of_get_opp_desc_node(struct device *dev) * -ENOMEM Memory allocation failure * -EINVAL Failed parsing the OPP node */ -static int _opp_add_static_v2(struct device *dev, struct device_node *np) +static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev, + struct device_node *np) { - struct opp_table *opp_table; struct dev_pm_opp *new_opp; u64 rate; u32 val; int ret;
- /* Hold our table modification lock here */ - mutex_lock(&opp_table_lock); - - new_opp = _opp_allocate(dev, &opp_table); - if (!new_opp) { - ret = -ENOMEM; - goto unlock; - } + new_opp = _opp_allocate(opp_table); + if (!new_opp) + return -ENOMEM;
ret = of_property_read_u64(np, "opp-hz", &rate); if (ret < 0) { @@ -347,8 +343,6 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) if (new_opp->clock_latency_ns > opp_table->clock_latency_ns_max) opp_table->clock_latency_ns_max = new_opp->clock_latency_ns;
- mutex_unlock(&opp_table_lock); - pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", __func__, new_opp->turbo, new_opp->rate, new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min, @@ -362,9 +356,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) return 0;
free_opp: - _opp_free(new_opp, opp_table); -unlock: - mutex_unlock(&opp_table_lock); + _opp_free(new_opp); + return ret; }
@@ -382,16 +375,20 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np) /* OPPs are already managed */ if (!_add_opp_dev(dev, opp_table)) ret = -ENOMEM; - mutex_unlock(&opp_table_lock); - return ret; + goto unlock; + } + + opp_table = _add_opp_table(dev); + if (!opp_table) { + ret = -ENOMEM; + goto unlock; } - mutex_unlock(&opp_table_lock);
/* We have opp-table node now, iterate over it and add OPPs */ for_each_available_child_of_node(opp_np, np) { count++;
- ret = _opp_add_static_v2(dev, np); + ret = _opp_add_static_v2(opp_table, dev, np); if (ret) { dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, ret); @@ -400,15 +397,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np) }
/* There should be one of more OPP defined */ - if (WARN_ON(!count)) - return -ENOENT; - - mutex_lock(&opp_table_lock); - - opp_table = _find_opp_table(dev); - if (WARN_ON(IS_ERR(opp_table))) { - ret = PTR_ERR(opp_table); - mutex_unlock(&opp_table_lock); + if (WARN_ON(!count)) { + ret = -ENOENT; goto free_table; }
@@ -418,12 +408,12 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np) else opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE;
- mutex_unlock(&opp_table_lock); - - return 0; + goto unlock;
free_table: - dev_pm_opp_of_remove_table(dev); + _dev_pm_opp_remove_table(opp_table, dev, false); +unlock: + mutex_unlock(&opp_table_lock);
return ret; } @@ -431,9 +421,10 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np) /* Initializes OPP tables based on old-deprecated bindings */ static int _of_add_opp_table_v1(struct device *dev) { + struct opp_table *opp_table; const struct property *prop; const __be32 *val; - int nr, ret; + int nr, ret = 0;
prop = of_find_property(dev->of_node, "operating-points", NULL); if (!prop) @@ -451,22 +442,32 @@ static int _of_add_opp_table_v1(struct device *dev) return -EINVAL; }
+ mutex_lock(&opp_table_lock); + + opp_table = _add_opp_table(dev); + if (!opp_table) { + ret = -ENOMEM; + goto unlock; + } + val = prop->value; while (nr) { unsigned long freq = be32_to_cpup(val++) * 1000; unsigned long volt = be32_to_cpup(val++);
- ret = _opp_add_v1(dev, freq, volt, false); + ret = _opp_add_v1(opp_table, dev, freq, volt, false); if (ret) { dev_err(dev, "%s: Failed to add OPP %ld (%d)\n", __func__, freq, ret); - dev_pm_opp_of_remove_table(dev); - return ret; + _dev_pm_opp_remove_table(opp_table, dev, false); + break; } nr -= 2; }
- return 0; +unlock: + mutex_unlock(&opp_table_lock); + return ret; }
/** diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index c4b539a8533a..0a5eb4d1e8f7 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -191,13 +191,16 @@ struct opp_table {
/* Routines internal to opp core */ struct opp_table *_find_opp_table(struct device *dev); +struct opp_table *_add_opp_table(struct device *dev); struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table); +void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev, bool remove_all); void _dev_pm_opp_find_and_remove_table(struct device *dev, bool remove_all); -struct dev_pm_opp *_opp_allocate(struct device *dev, struct opp_table **opp_table); -void _opp_free(struct dev_pm_opp *opp, struct opp_table *opp_table); +struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table); +void _opp_free(struct dev_pm_opp *opp); int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table); -int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, bool dynamic); +int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic); void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, bool of); +struct opp_table *_add_opp_table(struct device *dev);
#ifdef CONFIG_OF void _of_init_opp_table(struct opp_table *opp_table, struct device *dev);
There is only one user of dev_pm_opp_get_suspend_opp() and that uses it to get the OPP rate for the suspend_opp.
Rename dev_pm_opp_get_suspend_opp() as dev_pm_opp_get_suspend_opp_freq() and return the rate directly from it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 27 +++++++++++++-------------- drivers/cpufreq/cpufreq-dt.c | 7 +------ include/linux/pm_opp.h | 6 +++--- 3 files changed, 17 insertions(+), 23 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index dde7dd099aa0..614d779ab6a2 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -328,32 +328,31 @@ unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev) EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_transition_latency);
/** - * dev_pm_opp_get_suspend_opp() - Get suspend opp + * dev_pm_opp_get_suspend_opp_freq() - Get frequency of suspend opp in Hz * @dev: device for which we do this operation * - * Return: This function returns pointer to the suspend opp if it is - * defined and available, otherwise it returns NULL. - * - * Locking: This function must be called under rcu_read_lock(). opp is a rcu - * protected pointer. The reason for the same is that the opp pointer which is - * returned will remain valid for use with opp_get_{voltage, freq} only while - * under the locked area. The pointer returned must be used prior to unlocking - * with rcu_read_unlock() to maintain the integrity of the pointer. + * Return: This function returns the frequency of the OPP marked as suspend_opp + * if one is available, else returns 0; */ -struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev) +unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev) { struct opp_table *opp_table; + unsigned long freq = 0;
- opp_rcu_lockdep_assert(); + rcu_read_lock();
opp_table = _find_opp_table(dev); if (IS_ERR(opp_table) || !opp_table->suspend_opp || !opp_table->suspend_opp->available) - return NULL; + goto unlock;
- return opp_table->suspend_opp; + freq = dev_pm_opp_get_freq(opp_table->suspend_opp); + +unlock: + rcu_read_unlock(); + return freq; } -EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp); +EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
/** * dev_pm_opp_get_opp_count() - Get number of opps available in the opp table diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 269013311e79..c943787d761e 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -148,7 +148,6 @@ static int cpufreq_init(struct cpufreq_policy *policy) struct private_data *priv; struct device *cpu_dev; struct clk *cpu_clk; - struct dev_pm_opp *suspend_opp; unsigned int transition_latency; bool fallback = false; const char *name; @@ -252,11 +251,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) policy->driver_data = priv; policy->clk = cpu_clk;
- rcu_read_lock(); - suspend_opp = dev_pm_opp_get_suspend_opp(cpu_dev); - if (suspend_opp) - policy->suspend_freq = dev_pm_opp_get_freq(suspend_opp) / 1000; - rcu_read_unlock(); + policy->suspend_freq = dev_pm_opp_get_suspend_opp_freq(cpu_dev) / 1000;
ret = cpufreq_table_validate_and_show(policy, freq_table); if (ret) { diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 0edd88f93904..7483ff291a8e 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -88,7 +88,7 @@ int dev_pm_opp_get_opp_count(struct device *dev); unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev); unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev); unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev); -struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev); +unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev);
struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev, unsigned long freq, @@ -159,9 +159,9 @@ static inline unsigned long dev_pm_opp_get_max_transition_latency(struct device return 0; }
-static inline struct dev_pm_opp *dev_pm_opp_get_suspend_opp(struct device *dev) +static inline unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev) { - return NULL; + return 0; }
static inline struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
Let the OPP core provide helpers to register notifiers for any device, instead of exposing srcu_head outside of the core.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Acked-by: MyungJoo Ham myungjoo.ham@samsung.com Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 66 ++++++++++++++++++++++++++++++++----------- drivers/devfreq/devfreq.c | 26 ++--------------- include/linux/pm_opp.h | 14 ++++++--- 3 files changed, 62 insertions(+), 44 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 614d779ab6a2..f35effad60d1 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1860,29 +1860,63 @@ int dev_pm_opp_disable(struct device *dev, unsigned long freq) EXPORT_SYMBOL_GPL(dev_pm_opp_disable);
/** - * dev_pm_opp_get_notifier() - find notifier_head of the device with opp - * @dev: device pointer used to lookup OPP table. + * dev_pm_opp_register_notifier() - Register OPP notifier for the device + * @dev: Device for which notifier needs to be registered + * @nb: Notifier block to be registered * - * Return: pointer to notifier head if found, otherwise -ENODEV or - * -EINVAL based on type of error casted as pointer. value must be checked - * with IS_ERR to determine valid pointer or error result. + * Return: 0 on success or a negative error value. + */ +int dev_pm_opp_register_notifier(struct device *dev, struct notifier_block *nb) +{ + struct opp_table *opp_table; + int ret; + + rcu_read_lock(); + + opp_table = _find_opp_table(dev); + if (IS_ERR(opp_table)) { + ret = PTR_ERR(opp_table); + goto unlock; + } + + ret = srcu_notifier_chain_register(&opp_table->srcu_head, nb); + +unlock: + rcu_read_unlock(); + + return ret; +} +EXPORT_SYMBOL(dev_pm_opp_register_notifier); + +/** + * dev_pm_opp_unregister_notifier() - Unregister OPP notifier for the device + * @dev: Device for which notifier needs to be unregistered + * @nb: Notifier block to be unregistered * - * Locking: This function must be called under rcu_read_lock(). opp_table is a - * RCU protected pointer. The reason for the same is that the opp pointer which - * is returned will remain valid for use with opp_get_{voltage, freq} only while - * under the locked area. The pointer returned must be used prior to unlocking - * with rcu_read_unlock() to maintain the integrity of the pointer. + * Return: 0 on success or a negative error value. */ -struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev) +int dev_pm_opp_unregister_notifier(struct device *dev, + struct notifier_block *nb) { - struct opp_table *opp_table = _find_opp_table(dev); + struct opp_table *opp_table; + int ret;
- if (IS_ERR(opp_table)) - return ERR_CAST(opp_table); /* matching type */ + rcu_read_lock(); + + opp_table = _find_opp_table(dev); + if (IS_ERR(opp_table)) { + ret = PTR_ERR(opp_table); + goto unlock; + } + + ret = srcu_notifier_chain_unregister(&opp_table->srcu_head, nb);
- return &opp_table->srcu_head; +unlock: + rcu_read_unlock(); + + return ret; } -EXPORT_SYMBOL_GPL(dev_pm_opp_get_notifier); +EXPORT_SYMBOL(dev_pm_opp_unregister_notifier);
/* * Free OPPs either created using static entries present in DT or even the diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a324801d6a66..b0de42972b74 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -1260,18 +1260,7 @@ EXPORT_SYMBOL(devfreq_recommended_opp); */ int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq) { - struct srcu_notifier_head *nh; - int ret = 0; - - rcu_read_lock(); - nh = dev_pm_opp_get_notifier(dev); - if (IS_ERR(nh)) - ret = PTR_ERR(nh); - rcu_read_unlock(); - if (!ret) - ret = srcu_notifier_chain_register(nh, &devfreq->nb); - - return ret; + return dev_pm_opp_register_notifier(dev, &devfreq->nb); } EXPORT_SYMBOL(devfreq_register_opp_notifier);
@@ -1287,18 +1276,7 @@ EXPORT_SYMBOL(devfreq_register_opp_notifier); */ int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq) { - struct srcu_notifier_head *nh; - int ret = 0; - - rcu_read_lock(); - nh = dev_pm_opp_get_notifier(dev); - if (IS_ERR(nh)) - ret = PTR_ERR(nh); - rcu_read_unlock(); - if (!ret) - ret = srcu_notifier_chain_unregister(nh, &devfreq->nb); - - return ret; + return dev_pm_opp_unregister_notifier(dev, &devfreq->nb); } EXPORT_SYMBOL(devfreq_unregister_opp_notifier);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 7483ff291a8e..66a02deeb03f 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -108,7 +108,9 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
int dev_pm_opp_disable(struct device *dev, unsigned long freq);
-struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev); +int dev_pm_opp_register_notifier(struct device *dev, struct notifier_block *nb); +int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nb); + int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count); void dev_pm_opp_put_supported_hw(struct device *dev); @@ -202,10 +204,14 @@ static inline int dev_pm_opp_disable(struct device *dev, unsigned long freq) return 0; }
-static inline struct srcu_notifier_head *dev_pm_opp_get_notifier( - struct device *dev) +static inline int dev_pm_opp_register_notifier(struct device *dev, struct notifier_block *nb) { - return ERR_PTR(-ENOTSUPP); + return -ENOTSUPP; +} + +static inline int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nb) +{ + return -ENOTSUPP; }
static inline int dev_pm_opp_set_supported_hw(struct device *dev,
Split out parts of _add_opp_table() and _remove_opp_table() into separate routines. This improves readability as well.
Should result in no functional changes.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 76 +++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 32 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index f35effad60d1..622dd32f8dda 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -819,26 +819,12 @@ struct opp_device *_add_opp_dev(const struct device *dev, return opp_dev; }
-/** - * _add_opp_table() - Find OPP table or allocate a new one - * @dev: device for which we do this operation - * - * It tries to find an existing table first, if it couldn't find one, it - * allocates a new OPP table and returns that. - * - * Return: valid opp_table pointer if success, else NULL. - */ -struct opp_table *_add_opp_table(struct device *dev) +static struct opp_table *_allocate_opp_table(struct device *dev) { struct opp_table *opp_table; struct opp_device *opp_dev; int ret;
- /* Check for existing table for 'dev' first */ - opp_table = _find_opp_table(dev); - if (!IS_ERR(opp_table)) - return opp_table; - /* * Allocate a new OPP table. In the infrequent case where a new * device is needed to be added, we pay this penalty. @@ -875,6 +861,27 @@ struct opp_table *_add_opp_table(struct device *dev) }
/** + * _add_opp_table() - Find OPP table or allocate a new one + * @dev: device for which we do this operation + * + * It tries to find an existing table first, if it couldn't find one, it + * allocates a new OPP table and returns that. + * + * Return: valid opp_table pointer if success, else NULL. + */ +struct opp_table *_add_opp_table(struct device *dev) +{ + struct opp_table *opp_table; + + /* Check for existing table for 'dev' first */ + opp_table = _find_opp_table(dev); + if (!IS_ERR(opp_table)) + return opp_table; + + return _allocate_opp_table(dev); +} + +/** * _kfree_device_rcu() - Free opp_table RCU handler * @head: RCU head */ @@ -886,6 +893,27 @@ static void _kfree_device_rcu(struct rcu_head *head) kfree_rcu(opp_table, rcu_head); }
+static void _free_opp_table(struct opp_table *opp_table) +{ + struct opp_device *opp_dev; + + /* Release clk */ + if (!IS_ERR(opp_table->clk)) + clk_put(opp_table->clk); + + opp_dev = list_first_entry(&opp_table->dev_list, struct opp_device, + node); + + _remove_opp_dev(opp_dev, opp_table); + + /* dev_list must be empty now */ + WARN_ON(!list_empty(&opp_table->dev_list)); + + list_del_rcu(&opp_table->node); + call_srcu(&opp_table->srcu_head.srcu, &opp_table->rcu_head, + _kfree_device_rcu); +} + /** * _remove_opp_table() - Removes a OPP table * @opp_table: OPP table to be removed. @@ -894,8 +922,6 @@ static void _kfree_device_rcu(struct rcu_head *head) */ static void _remove_opp_table(struct opp_table *opp_table) { - struct opp_device *opp_dev; - if (!list_empty(&opp_table->opp_list)) return;
@@ -911,21 +937,7 @@ static void _remove_opp_table(struct opp_table *opp_table) if (opp_table->set_opp) return;
- /* Release clk */ - if (!IS_ERR(opp_table->clk)) - clk_put(opp_table->clk); - - opp_dev = list_first_entry(&opp_table->dev_list, struct opp_device, - node); - - _remove_opp_dev(opp_dev, opp_table); - - /* dev_list must be empty now */ - WARN_ON(!list_empty(&opp_table->dev_list)); - - list_del_rcu(&opp_table->node); - call_srcu(&opp_table->srcu_head.srcu, &opp_table->rcu_head, - _kfree_device_rcu); + _free_opp_table(opp_table); }
void _opp_free(struct dev_pm_opp *opp)
linaro-kernel@lists.linaro.org
-
Viresh Kumar