[patch] drm/vc4: fix a bounds check

[patch] drm/vc4: fix a bounds check

[Dan Carpenter]

We accidentally return success even if vc4_full_res_bounds_check() fails.

Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Not tested.

diff --git a/drivers/gpu/drm/vc4/vc4_render_cl.c b/drivers/gpu/drm/vc4/vc4_render_cl.c
index 08886a3..5cdd003 100644
--- a/drivers/gpu/drm/vc4/vc4_render_cl.c
+++ b/drivers/gpu/drm/vc4/vc4_render_cl.c
@@ -461,7 +461,7 @@ static int vc4_rcl_surface_setup(struct vc4_exec_info *exec,
 		}
 
 		ret = vc4_full_res_bounds_check(exec, *obj, surf);
-		if (!ret)
+		if (ret)
 			return ret;
 
 		return 0;

Re: [patch] drm/vc4: fix a bounds check

[Eric Engestrom]

On Friday, 2017-01-13 10:49:00 +0300, Dan Carpenter wrote:
> We accidentally return success even if vc4_full_res_bounds_check() fails.
> 
> Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Not tested.

It would be good to test it, but the previous code would always return 0,
and from a quick look the callers expect non-zero values on error, so
this makes more sense at least.
Reviewed-by: Eric Engestrom <eric@engestrom.ch>

> 
> diff --git a/drivers/gpu/drm/vc4/vc4_render_cl.c b/drivers/gpu/drm/vc4/vc4_render_cl.c
> index 08886a3..5cdd003 100644
> --- a/drivers/gpu/drm/vc4/vc4_render_cl.c
> +++ b/drivers/gpu/drm/vc4/vc4_render_cl.c
> @@ -461,7 +461,7 @@ static int vc4_rcl_surface_setup(struct vc4_exec_info *exec,
>  		}
>  
>  		ret = vc4_full_res_bounds_check(exec, *obj, surf);
> -		if (!ret)
> +		if (ret)
>  			return ret;
>  
>  		return 0;

This now boils down to `return vc4_full_res_bounds_check(...);`, so you
could get rid of the `ret` variable completely :)

Re: [patch] drm/vc4: fix a bounds check

[Dan Carpenter]

On Mon, Jan 16, 2017 at 11:40:10PM +0000, Eric Engestrom wrote:
> > diff --git a/drivers/gpu/drm/vc4/vc4_render_cl.c b/drivers/gpu/drm/vc4/vc4_render_cl.c
> > index 08886a3..5cdd003 100644
> > --- a/drivers/gpu/drm/vc4/vc4_render_cl.c
> > +++ b/drivers/gpu/drm/vc4/vc4_render_cl.c
> > @@ -461,7 +461,7 @@ static int vc4_rcl_surface_setup(struct vc4_exec_info *exec,
> >  		}
> >  
> >  		ret = vc4_full_res_bounds_check(exec, *obj, surf);
> > -		if (!ret)
> > +		if (ret)
> >  			return ret;
> >  
> >  		return 0;
> 
> This now boils down to `return vc4_full_res_bounds_check(...);`, so you
> could get rid of the `ret` variable completely :)

I tried to leave it the original style which the author intended.
That's also my prefered style.  I like big chunky "return 0;".  I
actually found this bug by looking at places where people return a
variable where a literal would work:

	if (!ret)
		return ret;

It's ambiguos if they intended to return a negative, or if they
condition is reversed.  The other reason why I slightly prefer his style
is because people get so "clever" with the last condition in a function
and it drives me nuts.  They'll do a series of checks like this:

	if (fail)
		goto;
	if (fail)
		goto;
	if (fail)
		goto;
	if (success)
		return;
label:

We should be testing for failure generally, but this particular kind of
success check is like nails on a chalk board for me.  My younger self
is guilty of this cleverness as well....

Of course, the other way: "return vc4_full_res_bounds_check();" is
fine too.  It's not something that bothers me.  I guess I just would
do whatever the original author prefers.

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

Re: [patch] drm/vc4: fix a bounds check

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> We accidentally return success even if vc4_full_res_bounds_check() fails.
>
> Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks.  Reviewed, added Eric's review, and pushed to -fixes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]