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.