[PATCH 1/2] opp: fix of_node leak for unsupported entries

[PATCH 1/2] opp: fix of_node leak for unsupported entries

[Michał Mirosław]

When parsing OPP v2 table, unsupported entries return NULL from
_opp_add_static_v2(). In this case node reference is leaked.
Make _opp_add_static_v2() always assume ownership of the reference
to fix this.

Commit 7978db344719 ("PM / OPP: Add missing of_node_put(np)") already
fixed this for the error returns. The leak remained for filtered-out
entries from the initial code commit. The Fixes-tagged commit is just
a last one that altered the code around.

Fixes: 11e1a1648298 ("opp: Don't decrement uninitialized list_kref")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/opp/of.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 1cbb58240b80..fba515e432a4 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -555,8 +555,10 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	bool rate_not_available = false;
 
 	new_opp = _opp_allocate(opp_table);
-	if (!new_opp)
-		return ERR_PTR(-ENOMEM);
+	if (!new_opp) {
+		ret = -ENOMEM;
+		goto free_node;
+	}
 
 	ret = of_property_read_u64(np, "opp-hz", &rate);
 	if (ret < 0) {
@@ -646,6 +648,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	_of_opp_free_required_opps(opp_table, new_opp);
 free_opp:
 	_opp_free(new_opp);
+free_node:
+	of_node_put(np);
 
 	return ERR_PTR(ret);
 }
@@ -677,7 +681,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 			ret = PTR_ERR(opp);
 			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
 				ret);
-			of_node_put(np);
 			return ret;
 		} else if (opp) {
 			count++;
-- 
2.20.1

[PATCH 2/2] opp: quiet down WARN when no valid OPPs remain

[Michał Mirosław]

Per CPU screenful of backtraces is not really that useful. Replace
WARN with a diagnostic discriminating common causes of empty OPP table.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/opp/of.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index fba515e432a4..59d7667b56f0 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -534,12 +534,13 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
  * Return:
  * Valid OPP pointer:
  *		On success
- * NULL:
+ * ERR_PTR(-EBUSY):
  *		Duplicate OPPs (both freq and volt are same) and opp->available
- *		OR if the OPP is not supported by hardware.
  * ERR_PTR(-EEXIST):
  *		Freq are same and volt are different OR
  *		Duplicate OPPs (both freq and volt are same) and !opp->available
+ * ERR_PTR(-ENODEV):
+ *		The OPP is not supported by hardware.
  * ERR_PTR(-ENOMEM):
  *		Memory allocation failure
  * ERR_PTR(-EINVAL):
@@ -583,6 +584,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 	/* Check if the OPP supports hardware's hierarchy of versions or not */
 	if (!_opp_is_supported(dev, opp_table, np)) {
 		dev_dbg(dev, "OPP not supported by hardware: %llu\n", rate);
+		ret = -ENODEV;
 		goto free_opp;
 	}
 
@@ -607,12 +609,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 		new_opp->pstate = pm_genpd_opp_to_performance_state(dev, new_opp);
 
 	ret = _opp_add(dev, new_opp, opp_table, rate_not_available);
-	if (ret) {
-		/* Don't return error for duplicate OPPs */
-		if (ret == -EBUSY)
-			ret = 0;
+	if (ret)
 		goto free_required_opps;
-	}
 
 	/* OPP to select on device suspend */
 	if (of_property_read_bool(np, "opp-suspend")) {
@@ -658,7 +656,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
 static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 {
 	struct device_node *np;
-	int ret, count = 0, pstate_count = 0;
+	int ret, count = 0, filtered = 0, pstate_count = 0;
 	struct dev_pm_opp *opp;
 
 	/* OPP table is already initialized for the device */
@@ -677,19 +675,32 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
 	/* We have opp-table node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_table->np, np) {
 		opp = _opp_add_static_v2(opp_table, dev, np);
-		if (IS_ERR(opp)) {
-			ret = PTR_ERR(opp);
+		ret = PTR_ERR_OR_ZERO(opp);
+		if (!ret) {
+			count++;
+		} else if (ret == -ENODEV) {
+			filtered++;
+		} else if (ret != -EBUSY) {
 			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
 				ret);
 			return ret;
-		} else if (opp) {
-			count++;
 		}
 	}
 
-	/* There should be one of more OPP defined */
-	if (WARN_ON(!count))
+	/* There should be one or more OPPs defined */
+	if (!count) {
+		if (!filtered)
+			/* all can't be duplicates, so there must be none */
+			dev_err(dev, "%s: OPP table empty", __func__);
+		else if (!opp_table->supported_hw)
+			dev_err(dev,
+				"%s: all OPPs match hw version, but platform did not provide it",
+				__func__);
+		else
+			dev_err(dev, "%s: no supported OPPs", __func__);
+
 		return -ENOENT;
+	}
 
 	list_for_each_entry(opp, &opp_table->opp_list, node)
 		pstate_count += !!opp->pstate;
-- 
2.20.1

Re: [PATCH 2/2] opp: quiet down WARN when no valid OPPs remain

[Viresh Kumar]

On 03-01-20, 20:36, Michał Mirosław wrote:
> Per CPU screenful of backtraces is not really that useful. Replace
> WARN with a diagnostic discriminating common causes of empty OPP table.

But why should a platform have an OPP table in DT where none of them works for
it ? I added the warn intentionally here just for that case.

-- 
viresh

Re: [PATCH 2/2] opp: quiet down WARN when no valid OPPs remain

[Michał Mirosław]

On Tue, Jan 07, 2020 at 12:11:29PM +0530, Viresh Kumar wrote:
> On 03-01-20, 20:36, Michał Mirosław wrote:
> > Per CPU screenful of backtraces is not really that useful. Replace
> > WARN with a diagnostic discriminating common causes of empty OPP table.
> But why should a platform have an OPP table in DT where none of them works for
> it ? I added the warn intentionally here just for that case.

Hmm. I guess we can make it WARN_ON_ONCE instead of removing it, but I
don't think the backtrace is ever useful in this case. Empty table can
be because eg. you run old DT on newer hardware version. This is why
it's still communicated via dev_err().

Best Regards,
Michał Mirosław

Re: [PATCH 2/2] opp: quiet down WARN when no valid OPPs remain

[Viresh Kumar]

On 07-01-20, 10:58, Michał Mirosław wrote:
> On Tue, Jan 07, 2020 at 12:11:29PM +0530, Viresh Kumar wrote:
> > On 03-01-20, 20:36, Michał Mirosław wrote:
> > > Per CPU screenful of backtraces is not really that useful. Replace
> > > WARN with a diagnostic discriminating common causes of empty OPP table.
> > But why should a platform have an OPP table in DT where none of them works for
> > it ? I added the warn intentionally here just for that case.
> 
> Hmm. I guess we can make it WARN_ON_ONCE instead of removing it

I am not sure this will get triggered more than once normally anyway, isn't it ?

> , but I
> don't think the backtrace is ever useful in this case.

Hmm, I am less concerned about backtraces than highlighting problem in a serious
way. The simple print messages are missed many times by people and probably
that's why I used a WARN instead.

> Empty table can
> be because eg. you run old DT on newer hardware version.

Hmm, but then a big warning isn't that bad as we need to highlight the issue to
everyone as cpufreq won't be working. isn't it ?

> This is why
> it's still communicated via dev_err().

-- 
viresh

Re: [PATCH 2/2] opp: quiet down WARN when no valid OPPs remain

[Michał Mirosław]

On Tue, Jan 07, 2020 at 05:00:55PM +0530, Viresh Kumar wrote:
> On 07-01-20, 10:58, Michał Mirosław wrote:
> > On Tue, Jan 07, 2020 at 12:11:29PM +0530, Viresh Kumar wrote:
> > > On 03-01-20, 20:36, Michał Mirosław wrote:
> > > > Per CPU screenful of backtraces is not really that useful. Replace
> > > > WARN with a diagnostic discriminating common causes of empty OPP table.
> > > But why should a platform have an OPP table in DT where none of them works for
> > > it ? I added the warn intentionally here just for that case.
> > Hmm. I guess we can make it WARN_ON_ONCE instead of removing it
> I am not sure this will get triggered more than once normally anyway, isn't it ?

It is triggered once per core.

> > , but I
> > don't think the backtrace is ever useful in this case.
> Hmm, I am less concerned about backtraces than highlighting problem in a serious
> way. The simple print messages are missed many times by people and probably
> that's why I used a WARN instead.

> 
> > Empty table can
> > be because eg. you run old DT on newer hardware version.
> 
> Hmm, but then a big warning isn't that bad as we need to highlight the issue to
> everyone as cpufreq won't be working. isn't it ?

A user normally can't do much about it. Rather this is a developer targeted
message. Maybe a rewording of the messages be better? Something to also
include consequences of the error?

Best Regards,
Michał Mirosław

Re: [PATCH 2/2] opp: quiet down WARN when no valid OPPs remain

[Viresh Kumar]

On 07-01-20, 14:57, Michał Mirosław wrote:
> On Tue, Jan 07, 2020 at 05:00:55PM +0530, Viresh Kumar wrote:
> > On 07-01-20, 10:58, Michał Mirosław wrote:
> > > On Tue, Jan 07, 2020 at 12:11:29PM +0530, Viresh Kumar wrote:
> > > > On 03-01-20, 20:36, Michał Mirosław wrote:
> > > > > Per CPU screenful of backtraces is not really that useful. Replace
> > > > > WARN with a diagnostic discriminating common causes of empty OPP table.
> > > > But why should a platform have an OPP table in DT where none of them works for
> > > > it ? I added the warn intentionally here just for that case.
> > > Hmm. I guess we can make it WARN_ON_ONCE instead of removing it
> > I am not sure this will get triggered more than once normally anyway, isn't it ?
> 
> It is triggered once per core.

What platform it is ? Maybe because cpufreq driver failed to initialize the
policy for all CPUs and so this is getting repeated.

> > > , but I
> > > don't think the backtrace is ever useful in this case.
> > Hmm, I am less concerned about backtraces than highlighting problem in a serious
> > way. The simple print messages are missed many times by people and probably
> > that's why I used a WARN instead.
> 
> > 
> > > Empty table can
> > > be because eg. you run old DT on newer hardware version.
> > 
> > Hmm, but then a big warning isn't that bad as we need to highlight the issue to
> > everyone as cpufreq won't be working. isn't it ?
> 
> A user normally can't do much about it. Rather this is a developer targeted
> message. Maybe a rewording of the messages be better? Something to also
> include consequences of the error?

I agree that a WARN may be a bit excessive here.

-- 
viresh

Re: [PATCH 2/2] opp: quiet down WARN when no valid OPPs remain

[Viresh Kumar]

On 03-01-20, 20:36, Michał Mirosław wrote:
> -	/* There should be one of more OPP defined */
> -	if (WARN_ON(!count))

Okay, you can remove the WARN but we don't need a lot of diagnostics around it.
Just print a single error message and drop all other changes from the patch.

-- 
viresh

Re: [PATCH 1/2] opp: fix of_node leak for unsupported entries

[Viresh Kumar]

On 03-01-20, 20:36, Michał Mirosław wrote:
> When parsing OPP v2 table, unsupported entries return NULL from
> _opp_add_static_v2().

Please fix that and return something sensible from there.

> In this case node reference is leaked.
> Make _opp_add_static_v2() always assume ownership of the reference
> to fix this.

The ownership lies with the routine which took the reference in the first place.

-- 
viresh

Re: [PATCH 1/2] opp: fix of_node leak for unsupported entries

[Viresh Kumar]

Discard my earlier reply, it wasn't accurate/correct.

On 03-01-20, 20:36, Michał Mirosław wrote:
> When parsing OPP v2 table, unsupported entries return NULL from
> _opp_add_static_v2().

Right, as we don't want parsing to fail here.

> In this case node reference is leaked.

Why do you think so ?

-- 
viresh

Re: [PATCH 1/2] opp: fix of_node leak for unsupported entries

[Michał Mirosław]

On Tue, Jan 07, 2020 at 12:06:16PM +0530, Viresh Kumar wrote:
> Discard my earlier reply, it wasn't accurate/correct.
> 
> On 03-01-20, 20:36, Michał Mirosław wrote:
> > When parsing OPP v2 table, unsupported entries return NULL from
> > _opp_add_static_v2().
> 
> Right, as we don't want parsing to fail here.
> 
> > In this case node reference is leaked.
> 
> Why do you think so ?

for_each_available_child_of_node() returns nodes with refcount
increased, and _opp_add_static_v2() returning NULL does not store the
pointer anywhere - the created (temporary) OPP structure is freed,
but _opp_free() does not release node at opp->np.

I guess maybe the _opp_free() should be fixed instead?

Best Regards,
Michał Mirosław

Re: [PATCH 1/2] opp: fix of_node leak for unsupported entries

[Viresh Kumar]

On 07-01-20, 15:04, Michał Mirosław wrote:
> On Tue, Jan 07, 2020 at 12:06:16PM +0530, Viresh Kumar wrote:
> > Discard my earlier reply, it wasn't accurate/correct.
> > 
> > On 03-01-20, 20:36, Michał Mirosław wrote:
> > > When parsing OPP v2 table, unsupported entries return NULL from
> > > _opp_add_static_v2().
> > 
> > Right, as we don't want parsing to fail here.
> > 
> > > In this case node reference is leaked.
> > 
> > Why do you think so ?
> 
> for_each_available_child_of_node() returns nodes with refcount
> increased

I believe it also drops the refcount of the previous node everytime the loop
goes to the next element. Else we would be required do that from within that
loop itself, isn't it ?

-- 
viresh

Re: [PATCH 1/2] opp: fix of_node leak for unsupported entries

[Michał Mirosław]

On Wed, Jan 08, 2020 at 01:03:38PM +0530, Viresh Kumar wrote:
> On 07-01-20, 15:04, Michał Mirosław wrote:
> > On Tue, Jan 07, 2020 at 12:06:16PM +0530, Viresh Kumar wrote:
> > > Discard my earlier reply, it wasn't accurate/correct.
> > > 
> > > On 03-01-20, 20:36, Michał Mirosław wrote:
> > > > When parsing OPP v2 table, unsupported entries return NULL from
> > > > _opp_add_static_v2().
> > > 
> > > Right, as we don't want parsing to fail here.
> > > 
> > > > In this case node reference is leaked.
> > > 
> > > Why do you think so ?
> > 
> > for_each_available_child_of_node() returns nodes with refcount
> > increased
> 
> I believe it also drops the refcount of the previous node everytime the loop
> goes to the next element. Else we would be required do that from within that
> loop itself, isn't it ?

Indeed it is! This means that _opp_add_static_v2() is storing a pointer
to a node without taking a reference to it. Is there something else that
guarantees the node won't disappear later?

Best Regards,
Michał Mirosław

Re: [PATCH 1/2] opp: fix of_node leak for unsupported entries

[Viresh Kumar]

On 08-01-20, 11:38, Michał Mirosław wrote:
> On Wed, Jan 08, 2020 at 01:03:38PM +0530, Viresh Kumar wrote:
> > On 07-01-20, 15:04, Michał Mirosław wrote:
> > > On Tue, Jan 07, 2020 at 12:06:16PM +0530, Viresh Kumar wrote:
> > > > Discard my earlier reply, it wasn't accurate/correct.
> > > > 
> > > > On 03-01-20, 20:36, Michał Mirosław wrote:
> > > > > When parsing OPP v2 table, unsupported entries return NULL from
> > > > > _opp_add_static_v2().
> > > > 
> > > > Right, as we don't want parsing to fail here.
> > > > 
> > > > > In this case node reference is leaked.
> > > > 
> > > > Why do you think so ?
> > > 
> > > for_each_available_child_of_node() returns nodes with refcount
> > > increased
> > 
> > I believe it also drops the refcount of the previous node everytime the loop
> > goes to the next element. Else we would be required do that from within that
> > loop itself, isn't it ?
> 
> Indeed it is! This means that _opp_add_static_v2() is storing a pointer
> to a node without taking a reference to it. Is there something else that
> guarantees the node won't disappear later?

We do store them, but don't use them other than comparing the value of
the pointer itself which is harmless.

-- 
viresh