| Message ID | 20200527192418.20169-1-sibis@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | OPP: Check for bandwidth values before creating icc paths | expand |
On 28-05-20, 00:54, Sibi Sankar wrote: > Prevent the core from creating and voting on icc paths when the > opp-table does not have the bandwidth values populated. Currently > this check is performed on the first OPP node. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > drivers/opp/of.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 61fce1284f012..95cf6f1312765 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -338,6 +338,21 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev, > struct device_node *np; > int ret = 0, i, count, num_paths; > struct icc_path **paths; > + struct property *prop; > + > + /* Check for bandwidth values on the first OPP node */ > + if (opp_table && opp_table->np) { > + np = of_get_next_available_child(opp_table->np, NULL); > + if (!np) { > + dev_err(dev, "Empty OPP table\n"); > + return 0; > + } > + > + prop = of_find_property(np, "opp-peak-kBps", NULL); > + of_node_put(np); > + if (!prop || !prop->length) > + return 0; > + } This doesn't support the call made from cpufreq-dt driver. Pushed this, please give this a try: From: Sibi Sankar <sibis@codeaurora.org> Date: Thu, 28 May 2020 00:54:18 +0530 Subject: [PATCH] opp: Don't parse icc paths unnecessarily The DT node of the device may contain interconnect paths while the OPP table doesn't have the bandwidth values. There is no need to parse the paths in such cases. Signed-off-by: Sibi Sankar <sibis@codeaurora.org> [ Viresh: Support the case of !opp_table and massaged changelog ] Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/opp/of.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 61fce1284f01..8c1bf01f0e50 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -332,13 +332,56 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table, return ret; } +static int _bandwidth_supported(struct device *dev, struct opp_table *opp_table) +{ + struct device_node *np, *opp_np; + struct property *prop; + + if (!opp_table) { + np = of_node_get(dev->of_node); + if (!np) + return -ENODEV; + + opp_np = _opp_of_get_opp_desc_node(np, 0); + of_node_put(np); + + /* Lets not fail in case we are parsing opp-v1 bindings */ + if (!opp_np) + return 0; + } else { + opp_np = of_node_get(opp_table->np); + } + + /* Checking only first OPP is sufficient */ + np = of_get_next_available_child(opp_np, NULL); + if (!np) { + dev_err(dev, "OPP table empty\n"); + return -EINVAL; + } + of_node_put(opp_np); + + prop = of_find_property(np, "opp-peak-kBps", NULL); + of_node_put(np); + + if (!prop || !prop->length) + return 0; + + return 1; +} + int dev_pm_opp_of_find_icc_paths(struct device *dev, struct opp_table *opp_table) { struct device_node *np; - int ret = 0, i, count, num_paths; + int ret, i, count, num_paths; struct icc_path **paths; + ret = _bandwidth_supported(dev, opp_table); + if (ret <= 0) + return ret; + + ret = 0; + np = of_node_get(dev->of_node); if (!np) return 0;
On 2020-05-29 10:50, Viresh Kumar wrote: > On 28-05-20, 00:54, Sibi Sankar wrote: >> Prevent the core from creating and voting on icc paths when the >> opp-table does not have the bandwidth values populated. Currently >> this check is performed on the first OPP node. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> drivers/opp/of.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/drivers/opp/of.c b/drivers/opp/of.c >> index 61fce1284f012..95cf6f1312765 100644 >> --- a/drivers/opp/of.c >> +++ b/drivers/opp/of.c >> @@ -338,6 +338,21 @@ int dev_pm_opp_of_find_icc_paths(struct device >> *dev, >> struct device_node *np; >> int ret = 0, i, count, num_paths; >> struct icc_path **paths; >> + struct property *prop; >> + >> + /* Check for bandwidth values on the first OPP node */ >> + if (opp_table && opp_table->np) { >> + np = of_get_next_available_child(opp_table->np, NULL); >> + if (!np) { >> + dev_err(dev, "Empty OPP table\n"); >> + return 0; >> + } >> + >> + prop = of_find_property(np, "opp-peak-kBps", NULL); >> + of_node_put(np); >> + if (!prop || !prop->length) >> + return 0; >> + } > > This doesn't support the call made from cpufreq-dt driver. Pushed > this, please give this a try: Viresh, Thanks for the patch! > > From: Sibi Sankar <sibis@codeaurora.org> > Date: Thu, 28 May 2020 00:54:18 +0530 > Subject: [PATCH] opp: Don't parse icc paths unnecessarily > > The DT node of the device may contain interconnect paths while the OPP > table doesn't have the bandwidth values. There is no need to parse the > paths in such cases. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > [ Viresh: Support the case of !opp_table and massaged changelog ] > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/opp/of.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 61fce1284f01..8c1bf01f0e50 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -332,13 +332,56 @@ static int _of_opp_alloc_required_opps(struct > opp_table *opp_table, > return ret; > } > > +static int _bandwidth_supported(struct device *dev, struct opp_table > *opp_table) > +{ > + struct device_node *np, *opp_np; > + struct property *prop; > + > + if (!opp_table) { > + np = of_node_get(dev->of_node); > + if (!np) > + return -ENODEV; > + > + opp_np = _opp_of_get_opp_desc_node(np, 0); > + of_node_put(np); > + > + /* Lets not fail in case we are parsing opp-v1 bindings */ > + if (!opp_np) > + return 0; > + } else { > + opp_np = of_node_get(opp_table->np); opp_np needs to be subjected to NULL check as well. Lets move "if (!opp_np)" to outside the if/else. With the above change in place: Tested-by: Sibi Sankar <sibis@codeaurora.org> Reviewed-by: Sibi Sankar <sibis@codeaurora.org> > + } > + > + /* Checking only first OPP is sufficient */ > + np = of_get_next_available_child(opp_np, NULL); > + if (!np) { > + dev_err(dev, "OPP table empty\n"); > + return -EINVAL; > + } > + of_node_put(opp_np); > + > + prop = of_find_property(np, "opp-peak-kBps", NULL); > + of_node_put(np); > + > + if (!prop || !prop->length) > + return 0; > + > + return 1; > +} > + > int dev_pm_opp_of_find_icc_paths(struct device *dev, > struct opp_table *opp_table) > { > struct device_node *np; > - int ret = 0, i, count, num_paths; > + int ret, i, count, num_paths; > struct icc_path **paths; > > + ret = _bandwidth_supported(dev, opp_table); > + if (ret <= 0) > + return ret; > + > + ret = 0; > + > np = of_node_get(dev->of_node); > if (!np) > return 0;
On 29-05-20, 19:47, Sibi Sankar wrote: > opp_np needs to be subjected > to NULL check as well. No, it isn't. It should already be valid and is set by the OPP core. Actually we don't need to do of_node_get(opp_table->np) and just use np, I did that to not have a special case while putting the resource. > Tested-by: Sibi Sankar <sibis@codeaurora.org> > Reviewed-by: Sibi Sankar <sibis@codeaurora.org> Thanks.
On 2020-06-01 09:37, Viresh Kumar wrote: > On 29-05-20, 19:47, Sibi Sankar wrote: >> opp_np needs to be subjected >> to NULL check as well. > > No, it isn't. It should already be valid and is set by the OPP core. > Actually we don't need to do of_node_get(opp_table->np) and just use > np, I did that to not have a special case while putting the resource. > I should have phrased it differently. opp_np needs to be checked to deal with cases where devices don't have "operating-points-v2" associated with it. diff --git a/drivers/opp/of.c b/drivers/opp/of.c index a5d87ca0ab571..06976d14e6ccb 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -344,14 +344,14 @@ static int _bandwidth_supported(struct device *dev, struct opp_table *opp_table) opp_np = _opp_of_get_opp_desc_node(np, 0); of_node_put(np); - - /* Lets not fail in case we are parsing opp-v1 bindings */ - if (!opp_np) - return 0; } else { opp_np = of_node_get(opp_table->np); } + /* Lets not fail in case we are parsing opp-v1 bindings */ + if (!opp_np) + return 0; + sdhci_msm 7c4000.sdhci: OPP table empty sdhci_msm 7c4000.sdhci: _allocate_opp_table: Error finding interconnect paths: -22 I see the following errors without the check. >> Tested-by: Sibi Sankar <sibis@codeaurora.org> >> Reviewed-by: Sibi Sankar <sibis@codeaurora.org> > > Thanks.
On 01-06-20, 12:09, Sibi Sankar wrote: > On 2020-06-01 09:37, Viresh Kumar wrote: > > On 29-05-20, 19:47, Sibi Sankar wrote: > > > opp_np needs to be subjected > > > to NULL check as well. > > > > No, it isn't. It should already be valid and is set by the OPP core. > > Actually we don't need to do of_node_get(opp_table->np) and just use > > np, I did that to not have a special case while putting the resource. > > > > I should have phrased it differently. > opp_np needs to be checked to deal > with cases where devices don't have > "operating-points-v2" associated with > it. > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index a5d87ca0ab571..06976d14e6ccb 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -344,14 +344,14 @@ static int _bandwidth_supported(struct device *dev, > struct opp_table *opp_table) > > opp_np = _opp_of_get_opp_desc_node(np, 0); > of_node_put(np); > - > - /* Lets not fail in case we are parsing opp-v1 bindings */ > - if (!opp_np) > - return 0; > } else { > opp_np = of_node_get(opp_table->np); > } > > + /* Lets not fail in case we are parsing opp-v1 bindings */ > + if (!opp_np) > + return 0; > + > > sdhci_msm 7c4000.sdhci: OPP table empty > sdhci_msm 7c4000.sdhci: _allocate_opp_table: Error finding interconnect > paths: -22 > > I see the following errors without > the check. My reply unfortunately only considered the case where this routine was called from within the opp table. Are you testing it for the case where you are adding OPPs dynamically from the code ?
On 2020-06-01 12:43, Viresh Kumar wrote: > On 01-06-20, 12:09, Sibi Sankar wrote: >> On 2020-06-01 09:37, Viresh Kumar wrote: >> > On 29-05-20, 19:47, Sibi Sankar wrote: >> > > opp_np needs to be subjected >> > > to NULL check as well. >> > >> > No, it isn't. It should already be valid and is set by the OPP core. >> > Actually we don't need to do of_node_get(opp_table->np) and just use >> > np, I did that to not have a special case while putting the resource. >> > >> >> I should have phrased it differently. >> opp_np needs to be checked to deal >> with cases where devices don't have >> "operating-points-v2" associated with >> it. >> >> diff --git a/drivers/opp/of.c b/drivers/opp/of.c >> index a5d87ca0ab571..06976d14e6ccb 100644 >> --- a/drivers/opp/of.c >> +++ b/drivers/opp/of.c >> @@ -344,14 +344,14 @@ static int _bandwidth_supported(struct device >> *dev, >> struct opp_table *opp_table) >> >> opp_np = _opp_of_get_opp_desc_node(np, 0); >> of_node_put(np); >> - >> - /* Lets not fail in case we are parsing opp-v1 >> bindings */ >> - if (!opp_np) >> - return 0; >> } else { >> opp_np = of_node_get(opp_table->np); >> } >> >> + /* Lets not fail in case we are parsing opp-v1 bindings */ >> + if (!opp_np) >> + return 0; >> + >> >> sdhci_msm 7c4000.sdhci: OPP table empty >> sdhci_msm 7c4000.sdhci: _allocate_opp_table: Error finding >> interconnect >> paths: -22 >> >> I see the following errors without >> the check. > > My reply unfortunately only considered the case where this routine was > called from within the opp table. Are you testing it for the case > where you are adding OPPs dynamically from the code ? Yeah dev_pm_opp_add/dev_pm_opp_set_clkname or pretty much any api doing a dev_pm_opp_get_opp_table without a opp_table node associated with it will run into this issue.
On 01-06-20, 15:30, Sibi Sankar wrote: > Yeah dev_pm_opp_add/dev_pm_opp_set_clkname > or pretty much any api doing a > dev_pm_opp_get_opp_table without > a opp_table node associated with > it will run into this issue. Not sure if what you wrote now is correct, the problem shouldn't happen from within dev_pm_opp_set_clkname() but only when we try to do bw thing. Anyway, I have pushed the change already.
On 2020-06-01 15:45, Viresh Kumar wrote: > On 01-06-20, 15:30, Sibi Sankar wrote: >> Yeah dev_pm_opp_add/dev_pm_opp_set_clkname >> or pretty much any api doing a >> dev_pm_opp_get_opp_table without >> a opp_table node associated with >> it will run into this issue. > > Not sure if what you wrote now is correct, the problem shouldn't > happen from within dev_pm_opp_set_clkname() but only when we try to do > bw thing. > > Anyway, I have pushed the change already. cool, thanks!
diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 61fce1284f012..95cf6f1312765 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -338,6 +338,21 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev, struct device_node *np; int ret = 0, i, count, num_paths; struct icc_path **paths; + struct property *prop; + + /* Check for bandwidth values on the first OPP node */ + if (opp_table && opp_table->np) { + np = of_get_next_available_child(opp_table->np, NULL); + if (!np) { + dev_err(dev, "Empty OPP table\n"); + return 0; + } + + prop = of_find_property(np, "opp-peak-kBps", NULL); + of_node_put(np); + if (!prop || !prop->length) + return 0; + } np = of_node_get(dev->of_node); if (!np)
Prevent the core from creating and voting on icc paths when the opp-table does not have the bandwidth values populated. Currently this check is performed on the first OPP node. Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- drivers/opp/of.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)