Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1205035 > unrolled thread

[PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure Viresh Kumar <viresh.kumar@linaro.org> - 2015-08-11 12:40 +0200
    Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Dan Carpenter <dan.carpenter@oracle.com> - 2015-08-11 16:50 +0200
      Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Viresh Kumar <viresh.kumar@linaro.org> - 2015-08-11 17:00 +0200
        Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Dan Carpenter <dan.carpenter@oracle.com> - 2015-08-11 19:20 +0200
          Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Viresh Kumar <viresh.kumar@linaro.org> - 2015-08-12 08:50 +0200
            Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Dan Carpenter <dan.carpenter@oracle.com> - 2015-08-12 10:20 +0200
              Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Viresh Kumar <viresh.kumar@linaro.org> - 2015-08-12 10:30 +0200
                Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Dan Carpenter <dan.carpenter@oracle.com> - 2015-08-12 11:10 +0200
                  Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Viresh Kumar <viresh.kumar@linaro.org> - 2015-08-12 12:20 +0200
                    Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return  error on failure Dan Carpenter <dan.carpenter@oracle.com> - 2015-08-12 13:00 +0200

#1205035 — [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

_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(-)

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);
 
-- 
2.4.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1205187 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

On Tue, Aug 11, 2015 at 04:04:34PM +0530, Viresh Kumar wrote:
> _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(-)
> 
> 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;
> +	}

This is weird to me, because we are going backwards.  What happens if
we goto free_table without adding anything?  I suspect it's fine, but if
it's a bug then this code still has problems.

What about if we only increment count when _opp_add_static_v2()
succeeds, and change it back to the original where we break, check
count, then check ret.  That way we don't need to know the details of
free_table to see that the code is correct.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205198 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

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.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205303 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

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.

> 
> > 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().

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205614 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

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 ?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205690 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

On Wed, Aug 12, 2015 at 12:13:09PM +0530, Viresh Kumar wrote:
> > 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 ?

If it doesn't WARN() then it's not buggy, but it's still ugly.  We
should not call of_free_opp_table() because we *tried* to add an OPP, we
should only call it if we *succeeded*.

The way the code is written and from your emails I was afraid that if
you tried to call _opp_add_static_v2() and it fails then it leaves
artifacts lying around that need to be cleaned up by the caller.  This
would be the ugliest scenario.  But I looked at _opp_add_static_v2()
and looks fine.  It cleans up properly on failure.  We only need to
clean up if it succeeds.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205695 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

On 12-08-15, 11:11, Dan Carpenter wrote:
> If it doesn't WARN() then it's not buggy, but it's still ugly.  We
> should not call of_free_opp_table() because we *tried* to add an OPP, we
> should only call it if we *succeeded*.

This is done in order to write lesser code. Otherwise we need
something like this:

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index bcbd92c3b717..650e92e2f2f0 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1317,14 +1317,15 @@ static int _of_init_opp_table_v2(struct device *dev,
 
 	/* We have opp-list node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
-		count++;
-
 		ret = _opp_add_static_v2(dev, np);
 		if (ret) {
 			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
 				ret);
+			if (!count)
+				goto put_opp_np;
 			goto free_table;
 		}
+		count++;
 	}
 
 	/* There should be one of more OPP defined */


To some people, this will look even more *ugly*. And so I just called
free_table() on error.

> The way the code is written and from your emails I was afraid that if
> you tried to call _opp_add_static_v2() and it fails then it leaves
> artifacts lying around that need to be cleaned up by the caller.

No. The problem is that we are trying to add OPPs in a while loop and
on failure we need to free all we added earlier.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205743 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

On Wed, Aug 12, 2015 at 01:53:02PM +0530, Viresh Kumar wrote:
> On 12-08-15, 11:11, Dan Carpenter wrote:
> > If it doesn't WARN() then it's not buggy, but it's still ugly.  We
> > should not call of_free_opp_table() because we *tried* to add an OPP, we
> > should only call it if we *succeeded*.
> 
> This is done in order to write lesser code.

I have been trying to discourage you from focusing so much on the
writing fewer lines.  That little bunny hop "goto put_opp_np;" at the
end of the success path should be replaced with:

	of_node_put(opp_np);
	return 0;

It's one extra line of code, but "return 0;" is so much more clear.  A
lot of my focus for the past few years has been on error handling so I
am perhaps more sensitive than many devs.  But here is what I like:

	success_path();
	if (fail)
		fail_path();
	success_path();
	success_path();
	success_path();
	if (fail)
		fail_path();
	success_path();

	return 0;

more_fail:
	fail_path();
fail:
	fail_path();
	return ret;

The success path is a list of commands indented one tab.  The fail path
is intended two tabs or in reverse order at the bottom of the function
indented one tab.  This uses the minimum amount of if statements and
indenting.

In the original code, we had:

	success_path();
	if (fail)
		fail_path();
	success_path();
	success_path();
	success_path();
	if (success) {
		success_path();
		if (fail)
			fail_path();
	} else {
		fail_path();
	}
free:
	success_and_fail_path();
	return ret;

The original code was confusing and, as a direct result, buggy.  Now we
have improved the code so it looks something like this:

	success_path();
	if (fail)
		fail_path();
	success_path();
	success_path();
	success_path();
	if (fail)
		fail_path();

	goto free;

more_fail:
	fail_path();
free:
	success_and_fail_path();
	return ret;

It's a lot better but it would be better yet with a return 0;.  There is
an earlier goto put_opp_np on the success path, but that's fine.  It's
not the normal success path so it's necessarily complicated.

Anyway, here is what I would suggest:

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 51b220e..243317c 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1317,19 +1317,23 @@ static int _of_init_opp_table_v2(struct device *dev,
 
 	/* We have opp-list node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
-		count++;
-
 		ret = _opp_add_static_v2(dev, np);
 		if (ret) {
 			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
 				ret);
 			break;
 		}
+		count++;
 	}
 
 	/* There should be one of more OPP defined */
-	if (WARN_ON(!count))
+	if (WARN_ON(!count)) {
+		ret = -ENOENT;
 		goto put_opp_np;
+	}
+	if (ret)
+		goto free_table;
 

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205898 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

On 12-08-15, 12:03, Dan Carpenter wrote:
> It's a lot better but it would be better yet with a return 0;.  There is
> an earlier goto put_opp_np on the success path, but that's fine.  It's
> not the normal success path so it's necessarily complicated.

I see where you are coming from and it makes lot of sense. Thanks for
the teaching part :)

> Anyway, here is what I would suggest:
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 51b220e..243317c 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -1317,19 +1317,23 @@ static int _of_init_opp_table_v2(struct device *dev,
>  
>  	/* We have opp-list node now, iterate over it and add OPPs */
>  	for_each_available_child_of_node(opp_np, np) {
> -		count++;
> -
>  		ret = _opp_add_static_v2(dev, np);
>  		if (ret) {
>  			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
>  				ret);
>  			break;
>  		}
> +		count++;
>  	}
>  
>  	/* There should be one of more OPP defined */
> -	if (WARN_ON(!count))
> +	if (WARN_ON(!count)) {

This is wrong. _opp_add_static_v2() failed and so count was 0. And we
aren't supposed to WARN() in this case. We only WARN if the user
hasn't added any available nodes in the opp table.

> +		ret = -ENOENT;
>  		goto put_opp_np;
> +	}
> +	if (ret)
> +		goto free_table;

Also the 'put_opp_np' thing is getting removed, check 2/6 patch of
this series.

What about this:

Message-Id: <e2412c33aa6923767b394adffee9d3f7be1ee27f.1439373912.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 11 Aug 2015 14:23:34 +0530
Subject: [PATCH] PM / OPP: Free resources and properly return error on failure

_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.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 204c6c945168..4d6c4576f7ae 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1323,28 +1323,30 @@ 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");
+
+	of_node_put(opp_np);
+	return 0;
+
+free_table:
+	of_free_opp_table(dev);
 put_opp_np:
 	of_node_put(opp_np);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205955 — Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure

Fine, fine.  Much improved etc.  :)  Thanks.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web