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. */

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; }

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);

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,

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:

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;
  

  goto free_opp;
• }

mutex_unlock(&opp_table_lock);

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: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);