| Message ID | 334a9052264630b9157fa9bfc3d4efe945054c34.1439288881.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 11-08-15, 17:43, Dan Carpenter wrote: > > @@ -1323,28 +1323,29 @@ static int _of_init_opp_table_v2(struct device *dev, > > if (ret) { > > dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, > > ret); > > - break; > > + goto free_table; > > } > > } > > > > /* There should be one of more OPP defined */ > > - if (WARN_ON(!count)) > > + if (WARN_ON(!count)) { > > + ret = -ENOENT; > > goto put_opp_np; > > + } The purpose of 'count' here is to see if the dtb contained any OPP nodes or not. i.e. if we ever entered the body of for_each_available_child_of_node() or not.. Its different than, "if we were able to add any OPPs"; > This is weird to me, because we are going backwards. What happens if > we goto free_table without adding anything? It will WARN() today. > I suspect it's fine, but if > it's a bug then this code still has problems. I don't think we have a bug here, we never added anything and so don't need to free it. > What about if we only increment count when _opp_add_static_v2() > succeeds That's not what we want.
On 11-08-15, 20:11, Dan Carpenter wrote: > On Tue, Aug 11, 2015 at 08:29:38PM +0530, Viresh Kumar wrote: > > > This is weird to me, because we are going backwards. What happens if > > > we goto free_table without adding anything? > > > > It will WARN() today. > > Then the current code is buggy. Urg, it wouldn't WARN in this case. Sorry, didn't read it correctly earlier. > > > I suspect it's fine, but if > > > it's a bug then this code still has problems. > > > > I don't think we have a bug here, we never added anything and so don't > > need to free it. > > > > > What about if we only increment count when _opp_add_static_v2() > > > succeeds > > > > That's not what we want. > > If the first call to _opp_add_static_v2() fails we call > of_free_opp_table() and you say that triggers a WARN(). No it doesn't. So, coming back to the point you made about freeing table on !count, because there were no nodes present in the DT opp table, we have never tried to add any OPPs. And so there is no need to call of_free_opp_table() in that case. Do you still think the current code is wrong ?
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 204c6c945168..bcbd92c3b717 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -1323,28 +1323,29 @@ static int _of_init_opp_table_v2(struct device *dev, if (ret) { dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, ret); - break; + goto free_table; } } /* There should be one of more OPP defined */ - if (WARN_ON(!count)) + if (WARN_ON(!count)) { + ret = -ENOENT; goto put_opp_np; + } - if (!ret) { - if (!dev_opp) { - dev_opp = _find_device_opp(dev); - if (WARN_ON(!dev_opp)) - goto put_opp_np; - } - - dev_opp->np = opp_np; - dev_opp->shared_opp = of_property_read_bool(opp_np, - "opp-shared"); - } else { - of_free_opp_table(dev); + dev_opp = _find_device_opp(dev); + if (WARN_ON(IS_ERR(dev_opp))) { + ret = PTR_ERR(dev_opp); + goto free_table; } + dev_opp->np = opp_np; + dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared"); + + goto put_opp_np; + +free_table: + of_free_opp_table(dev); put_opp_np: of_node_put(opp_np);
_of_init_opp_table_v2() isn't freeing up resources on some errors and the error values returned are also not correct always. This fixes following problems: - Return -ENOENT, if no entries are found in the table. - Use IS_ERR() to properly check return value of _find_device_opp(). - Return error value with PTR_ERR() in above case. - Free table if _find_device_opp() fails. Fixes: 274659029c9d ("PM / OPP: Add support to parse operating-points-v2" bindings") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/base/power/opp.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)