| Message ID | 5c2d6548aef35c690535fd8c985b980316745e91.1578077228.git.mirq-linux@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [1/2] opp: fix of_node leak for unsupported entries | expand |
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.
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
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().
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
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.
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.
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;
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(-)