opp: Reinitialize the list_kref before adding the static OPPs again

The list_kref reaches a count of 0 when all the static OPPs are removed,
for example when dev_pm_opp_of_cpumask_remove_table() is called, though
the actual OPP table may not get freed as it may still be referenced by
other parts of the kernel, like from a call to
dev_pm_opp_set_supported_hw(). And if we call
dev_pm_opp_of_cpumask_add_table() again at this point, we must
reinitialize the list_kref otherwise the kernel will hit a WARN() in
kref infrastructure for incrementing a kref with value 0.

Fixes: 11e1a1648298 ("opp: Don't decrement uninitialized list_kref")
Reported-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/opp/of.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.21.0.rc0.269.g1a574e7a288b

Quoting Viresh Kumar (2019-10-20 19:25:16)
> On 18-10-19, 14:12, Stephen Boyd wrote:

> > Quoting Viresh Kumar (2019-10-18 02:28:41)

> > > The list_kref reaches a count of 0 when all the static OPPs are removed,

> > > for example when dev_pm_opp_of_cpumask_remove_table() is called, though

> > > the actual OPP table may not get freed as it may still be referenced by

> > > other parts of the kernel, like from a call to

> > > dev_pm_opp_set_supported_hw(). And if we call

> > > dev_pm_opp_of_cpumask_add_table() again at this point, we must

> > > reinitialize the list_kref otherwise the kernel will hit a WARN() in

> > > kref infrastructure for incrementing a kref with value 0.

> > > 

> > > Fixes: 11e1a1648298 ("opp: Don't decrement uninitialized list_kref")

> > > Reported-by: Dmitry Osipenko <digetx@gmail.com>

> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > > ---

> > >  drivers/opp/of.c | 7 +++++++

> > >  1 file changed, 7 insertions(+)

> > > 

> > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c

> > > index 6dc41faf74b5..1cbb58240b80 100644

> > > --- a/drivers/opp/of.c

> > > +++ b/drivers/opp/of.c

> > > @@ -663,6 +663,13 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)

> > >                 return 0;

> > >         }

> > >  

> > > +       /*

> > > +        * Re-initialize list_kref every time we add static OPPs to the OPP

> > > +        * table as the reference count may be 0 after the last tie static OPPs

> > 

> > s/tie/time/

> > 

> > > +        * were removed.

> > > +        */

> > > +       kref_init(&opp_table->list_kref);

> > 

> > It seems racy.

> 

> I am not sure if I see a race here, but maybe I am missing something.

> Care to explain ?


Some static OPP is removed at the same time that this function is
called?

> 

> > Why are we doing this vs. making an entirely new and

> > different OPP structure? Or why is the count reaching 0 when something

> > is obviously still referencing it?

> 

> The kref for the opp table is opp_table->kref and the one here is

> different. This is list_kref which is used for freeing OPPs added

> statically from the DT. The static OPPs get added to the OPP table

> when one calls dev_pm_opp_of_cpumask_add_table() and must be removed

> on a call to dev_pm_opp_of_cpumask_remove_table(). The opp table

> structure may not get freed at this moment though as it is still

> referenced by the caller of dev_pm_opp_set_supported_hw().

> 

> And now when we try to add the static OPPs again (re-insertion of

> cpufreq module), we need to reinitialize the list_kref again as its

> count reached 0 earlier and the resources (static OPPs) were freed.

> 


Right. I don't understand why the count reaches 0 if we can still get a
pointer to something. I guess we've got this kref thing that has a
lifetime beyond the life of what it's tracking, which is weird. Usually
the kref is embedded inside the pointer that is returned by the "get"
call, but here it's outside it and used to track when we should free
static OPPs. Why are we removing static OPPs? Shouldn't they just stick
around forever until the device is deleted vs. populated over and over
again?

On 30-10-19, 07:33, Stephen Boyd wrote:
> I agree a simple refcount_t makes more sense here instead of using a

> kref. That would be clearer.


I was using kref as I wanted to call the cleanup routine when kref
reaches 0. A refcount_t will have the same problem as the warning in
this came from refcount mechanism only (which is used by kref). You
can't increment a refcount_t if it is zero :)

Any other suggestions other than local counting ?

-- 
viresh

On 11-11-19, 13:51, Viresh Kumar wrote:
> On 30-10-19, 07:33, Stephen Boyd wrote:

> > I agree a simple refcount_t makes more sense here instead of using a

> > kref. That would be clearer.

> 

> I was using kref as I wanted to call the cleanup routine when kref

> reaches 0. A refcount_t will have the same problem as the warning in

> this came from refcount mechanism only (which is used by kref). You

> can't increment a refcount_t if it is zero :)

> 

> Any other suggestions other than local counting ?


i.e. something like this, untested.

---
 drivers/opp/core.c | 26 ++++++++------------------
 drivers/opp/of.c   | 14 +++++---------
 drivers/opp/opp.h  |  6 ++----
 3 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 467b2348a289..ea1d89177511 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -988,7 +988,6 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
 	INIT_LIST_HEAD(&opp_table->opp_list);
 	kref_init(&opp_table->kref);
-	kref_init(&opp_table->list_kref);
 
 	/* Secure the device table modification */
 	list_add(&opp_table->node, &opp_tables);
@@ -1076,27 +1075,18 @@ static void _opp_remove_all_static(struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp, *tmp;
 
+	mutex_lock(&opp_table->lock);
+
+	if (!opp_table->parsed_static_opps || --opp_table->parsed_static_opps)
+		goto unlock;
+
 	list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) {
 		if (!opp->dynamic)
 			dev_pm_opp_put(opp);
 	}
 
-	opp_table->parsed_static_opps = false;
-}
-
-static void _opp_table_list_kref_release(struct kref *kref)
-{
-	struct opp_table *opp_table = container_of(kref, struct opp_table,
-						   list_kref);
-
-	_opp_remove_all_static(opp_table);
-	mutex_unlock(&opp_table_lock);
-}
-
-void _put_opp_list_kref(struct opp_table *opp_table)
-{
-	kref_put_mutex(&opp_table->list_kref, _opp_table_list_kref_release,
-		       &opp_table_lock);
+unlock:
+	mutex_unlock(&opp_table->lock);
 }
 
 void dev_pm_opp_put_opp_table(struct opp_table *opp_table)
@@ -2276,7 +2266,7 @@ void _dev_pm_opp_find_and_remove_table(struct device *dev)
 		return;
 	}
 
-	_put_opp_list_kref(opp_table);
+	_opp_remove_all_static(opp_table);
 
 	/* Drop reference taken by _find_opp_table() */
 	dev_pm_opp_put_opp_table(opp_table);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 1cbb58240b80..2c433e9f9223 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -658,17 +658,15 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 	struct dev_pm_opp *opp;
 
 	/* OPP table is already initialized for the device */
+	mutex_lock(&opp_table->lock);
 	if (opp_table->parsed_static_opps) {
-		kref_get(&opp_table->list_kref);
+		opp_table->parsed_static_opps++;
+		mutex_unlock(&opp_table->lock);
 		return 0;
 	}
 
-	/*
-	 * Re-initialize list_kref every time we add static OPPs to the OPP
-	 * table as the reference count may be 0 after the last tie static OPPs
-	 * were removed.
-	 */
-	kref_init(&opp_table->list_kref);
+	opp_table->parsed_static_opps = 1;
+	mutex_unlock(&opp_table->lock);
 
 	/* We have opp-table node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_table->np, np) {
@@ -701,8 +699,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 	if (pstate_count)
 		opp_table->genpd_performance_state = true;
 
-	opp_table->parsed_static_opps = true;
-
 	return 0;
 }
 
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 51ad942d1b6b..4e69b855a6a0 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -127,11 +127,10 @@ enum opp_table_access {
  * @dev_list:	list of devices that share these OPPs
  * @opp_list:	table of opps
  * @kref:	for reference count of the table.
- * @list_kref:	for reference count of the OPP list.
  * @lock:	mutex protecting the opp_list and dev_list.
  * @np:		struct device_node pointer for opp's DT node.
  * @clock_latency_ns_max: Max clock latency in nanoseconds.
- * @parsed_static_opps: True if OPPs are initialized from DT.
+ * @parsed_static_opps: Count of devices for which OPPs are initialized from DT.
  * @shared_opp: OPP is shared between multiple devices.
  * @suspend_opp: Pointer to OPP to be used during device suspend.
  * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
@@ -167,7 +166,6 @@ struct opp_table {
 	struct list_head dev_list;
 	struct list_head opp_list;
 	struct kref kref;
-	struct kref list_kref;
 	struct mutex lock;
 
 	struct device_node *np;
@@ -176,7 +174,7 @@ struct opp_table {
 	/* For backward compatibility with v1 bindings */
 	unsigned int voltage_tolerance_v1;
 
-	bool parsed_static_opps;
+	unsigned int parsed_static_opps;
 	enum opp_table_access shared_opp;
 	struct dev_pm_opp *suspend_opp;

-- 
viresh