[2/2] opp: quiet down WARN when no valid OPPs remain
diff mbox series

Message ID 5c2d6548aef35c690535fd8c985b980316745e91.1578077228.git.mirq-linux@rere.qmqm.pl
State New, archived
Delegated to: viresh kumar
Headers show
Series
  • [1/2] opp: fix of_node leak for unsupported entries
Related show

Commit Message

Michał Mirosław Jan. 3, 2020, 7:36 p.m. UTC
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(-)

Comments

Viresh Kumar Jan. 7, 2020, 6:41 a.m. UTC | #1
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.
Michał Mirosław Jan. 7, 2020, 9:58 a.m. UTC | #2
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
Viresh Kumar Jan. 7, 2020, 11:30 a.m. UTC | #3
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().
Michał Mirosław Jan. 7, 2020, 1:57 p.m. UTC | #4
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
Viresh Kumar Jan. 8, 2020, 6:49 a.m. UTC | #5
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 Kumar Jan. 8, 2020, 8:17 a.m. UTC | #6
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.

Patch
diff mbox series

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;