| Message ID | 70a36dba8745a6df3a79d00e995caab1e66c9eee.1527050748.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / Domain: Return 0 on error from of_genpd_opp_to_performance_state() | expand |
On 23 May 2018 at 06:46, Viresh Kumar <viresh.kumar@linaro.org> wrote: > of_genpd_opp_to_performance_state() should return 0 on errors, as its > doc comment describes. While it follows that mostly, it returns a > negative error number on one of the failures. > > Fix that. > > Fixes: 6e41766a6a50 ("PM / Domain: Implement of_genpd_opp_to_performance_state()") > Reported-by: Rajendra Nayak <rnayak@codeaurora.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/domain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 29e25dc0584c..d668bb892d3f 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2446,7 +2446,7 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev, > > opp = of_dev_pm_opp_find_required_opp(&genpd->dev, opp_node); > if (IS_ERR(opp)) { > - state = PTR_ERR(opp); > + dev_err(dev, "Failed to find required OPP: %d\n", PTR_ERR(opp)); > goto unlock; > } > > -- > 2.15.0.194.g9af6a3dea062 >
On 05/23/2018 10:16 AM, Viresh Kumar wrote: > of_genpd_opp_to_performance_state() should return 0 on errors, as its > doc comment describes. While it follows that mostly, it returns a > negative error number on one of the failures. > > Fix that. So this would mean in case of failures, we still add a new OPP with new_opp->pstate as 0 based on the below code in _opp_add_static_v2()? Is that expected, should we not fail adding the OPP too? @@ -329,6 +330,8 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev, if (!of_property_read_u32(np, "clock-latency-ns", &val)) new_opp->clock_latency_ns = val; + new_opp->pstate = of_genpd_opp_to_performance_state(dev, np); > > Fixes: 6e41766a6a50 ("PM / Domain: Implement of_genpd_opp_to_performance_state()") > Reported-by: Rajendra Nayak <rnayak@codeaurora.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/base/power/domain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 29e25dc0584c..d668bb892d3f 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2446,7 +2446,7 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev, > > opp = of_dev_pm_opp_find_required_opp(&genpd->dev, opp_node); > if (IS_ERR(opp)) { > - state = PTR_ERR(opp); > + dev_err(dev, "Failed to find required OPP: %d\n", PTR_ERR(opp)); > goto unlock; > } > > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On 23-05-18, 12:19, Rajendra Nayak wrote: > > On 05/23/2018 10:16 AM, Viresh Kumar wrote: > > of_genpd_opp_to_performance_state() should return 0 on errors, as its > > doc comment describes. While it follows that mostly, it returns a > > negative error number on one of the failures. > > > > Fix that. > > So this would mean in case of failures, we still add a new OPP with > new_opp->pstate as 0 based on the below code in _opp_add_static_v2()? Yes, that's how it is designed for now. We call the below routine unconditionally even if there is no genpd for the device. > Is that expected, should we not fail adding the OPP too? Maybe, I am not sure at this point. We should be in the safe zone, i.e. we shouldn't be harming the hardware in any case by going forward. Else we may want to revisit this code. -- viresh
On 23 May 2018 at 10:05, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 23-05-18, 12:19, Rajendra Nayak wrote: >> >> On 05/23/2018 10:16 AM, Viresh Kumar wrote: >> > of_genpd_opp_to_performance_state() should return 0 on errors, as its >> > doc comment describes. While it follows that mostly, it returns a >> > negative error number on one of the failures. >> > >> > Fix that. >> >> So this would mean in case of failures, we still add a new OPP with >> new_opp->pstate as 0 based on the below code in _opp_add_static_v2()? > > Yes, that's how it is designed for now. We call the below routine > unconditionally even if there is no genpd for the device. > >> Is that expected, should we not fail adding the OPP too? > > Maybe, I am not sure at this point. We should be in the safe zone, > i.e. we shouldn't be harming the hardware in any case by going > forward. Else we may want to revisit this code. If we want to change, I can think of a simple genpd helper, like pm_domain_is_genpd(), which checks if dev->pm_domain is set and of type genpd. That would be useful, right? Kind regards Uffe
On 23-05-18, 10:51, Ulf Hansson wrote: > If we want to change, I can think of a simple genpd helper, like > pm_domain_is_genpd(), which checks if dev->pm_domain is set and of > type genpd. That would be useful, right? Not completely. We also need to test if the genpd supports performance-state updates. But I think return 0 for all the errors is just fine, with a print message. Trying to run devices at a frequency over and above what the voltage rail's current configuration supports is safe, right ? As that is the worst we may end up doing :) And there are other cases as well. For example what if the device really has a genpd which updates performance state, but due to a bug somewhere (maybe in DT), we couldn't attach the genpd with the device? The same problem will happen there as well. And that's why I would like to keep the failure cases as returning 0 for now. But I am open to change it if it is really required. -- viresh
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 29e25dc0584c..d668bb892d3f 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -2446,7 +2446,7 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev, opp = of_dev_pm_opp_find_required_opp(&genpd->dev, opp_node); if (IS_ERR(opp)) { - state = PTR_ERR(opp); + dev_err(dev, "Failed to find required OPP: %d\n", PTR_ERR(opp)); goto unlock; }
of_genpd_opp_to_performance_state() should return 0 on errors, as its doc comment describes. While it follows that mostly, it returns a negative error number on one of the failures. Fix that. Fixes: 6e41766a6a50 ("PM / Domain: Implement of_genpd_opp_to_performance_state()") Reported-by: Rajendra Nayak <rnayak@codeaurora.org> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/base/power/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.15.0.194.g9af6a3dea062