| Message ID | 20170113074900.GB30524@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 :)
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
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.
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;
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.