Messages in this thread Patch in this message | | | Date | Fri, 6 Nov 2020 12:56:24 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH 2/3] opp: Allocate the OPP table outside of opp_table_lock |
| |
On 02-11-20, 10:51, Viresh Kumar wrote: > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > +/* > + * We need to make sure that the OPP table for a device doesn't get added twice, > + * if this routine gets called in parallel with the same device pointer. > + * > + * The simplest way to enforce that is to perform everything (find existing > + * table and if not found, create a new one) under the opp_table_lock, so only > + * one creator gets access to the same. But that expands the critical section > + * under the lock and may end up causing circular dependencies with frameworks > + * like debugfs, interconnect or clock framework as they may be direct or > + * indirect users of OPP core. > + * > + * And for that reason we have to go for a bit tricky implementation here, which > + * uses the opp_tables_busy flag to indicate if another creator is in the middle > + * of adding an OPP table and others should wait for it to finish. > + */ > static struct opp_table *_opp_get_opp_table(struct device *dev, int index) > { > struct opp_table *opp_table; > > - /* Hold our table modification lock here */ > +again: > mutex_lock(&opp_table_lock); > > opp_table = _find_opp_table_unlocked(dev); > if (!IS_ERR(opp_table)) > goto unlock; > > + /* > + * The opp_tables list or an OPP table's dev_list is getting updated by > + * another user, wait for it to finish. > + */ > + if (unlikely(opp_tables_busy)) { > + mutex_unlock(&opp_table_lock); > + cpu_relax(); > + goto again; > + } > + > + opp_tables_busy = true; > opp_table = _managed_opp(dev, index); > + > + /* Drop the lock to reduce the size of critical section */ > + mutex_unlock(&opp_table_lock); > + > if (opp_table) { > if (!_add_opp_dev(dev, opp_table)) { > dev_pm_opp_put_opp_table(opp_table); > opp_table = ERR_PTR(-ENOMEM); > } > - goto unlock; > + > + mutex_lock(&opp_table_lock); > + } else { > + opp_table = _allocate_opp_table(dev, index); > + > + mutex_lock(&opp_table_lock); > + if (!IS_ERR(opp_table)) > + list_add(&opp_table->node, &opp_tables); > } > > - opp_table = _allocate_opp_table(dev, index); > + opp_tables_busy = false;
And here is a fix that will be merged with this patch while applying. It is required as _allocate_opp_table() (which calls _find_table_of_opp_np()) isn't called with the opp_table_lock anymore.
diff --git a/drivers/opp/of.c b/drivers/opp/of.c index c718092757d9..6b7f0066942d 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -112,8 +112,6 @@ static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np) struct opp_table *opp_table; struct device_node *opp_table_np; - lockdep_assert_held(&opp_table_lock); - opp_table_np = of_get_parent(opp_np); if (!opp_table_np) goto err; @@ -121,12 +119,15 @@ static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np) /* It is safe to put the node now as all we need now is its address */ of_node_put(opp_table_np); + mutex_lock(&opp_table_lock); list_for_each_entry(opp_table, &opp_tables, node) { if (opp_table_np == opp_table->np) { _get_opp_table_kref(opp_table); + mutex_unlock(&opp_table_lock); return opp_table; } } + mutex_unlock(&opp_table_lock); err: return ERR_PTR(-ENODEV); -- viresh
| |