| Message ID | 1396691102-22904-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/05/2014 02:44 AM, Daniel Vetter wrote: > ttm_bo_unref unconditionally calls kref_put on it's argument, so the > thing can't be NULL without already causing Oopses. Doesn't this mean the NULL check is in the wrong place (rather than the NULL check should be removed)? > Spotted by coverity. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/mgag200/mgag200_main.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c > index 26868e5c55b0..0722d18992f4 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_main.c > +++ b/drivers/gpu/drm/mgag200/mgag200_main.c > @@ -322,9 +322,7 @@ static void mgag200_bo_unref(struct mgag200_bo **bo) > > tbo = &((*bo)->bo); > ttm_bo_unref(&tbo); > - if (tbo == NULL) > - *bo = NULL; > - > + *bo = NULL; > } > > void mgag200_gem_free_object(struct drm_gem_object *obj) >
On Mon, Apr 7, 2014 at 5:19 PM, Ian Romanick <idr@freedesktop.org> wrote: > On 04/05/2014 02:44 AM, Daniel Vetter wrote: >> ttm_bo_unref unconditionally calls kref_put on it's argument, so the >> thing can't be NULL without already causing Oopses. > > Doesn't this mean the NULL check is in the wrong place (rather than the > NULL check should be removed)? Afaics chasing callchains it's a bug to call this with NULL pointer and no one really should be doing it. Like David Herrmann said it's sometimes useful if unref/free functions automatically cope with NULL, but ttm buffers don't seem to be of this kind. So consistency with other ttm drivers seems better, same with all the gem_free_object callbacks. -Daniel
On 04/07/2014 12:56 PM, Daniel Vetter wrote: > On Mon, Apr 7, 2014 at 5:19 PM, Ian Romanick <idr@freedesktop.org> wrote: >> On 04/05/2014 02:44 AM, Daniel Vetter wrote: >>> ttm_bo_unref unconditionally calls kref_put on it's argument, so the >>> thing can't be NULL without already causing Oopses. >> >> Doesn't this mean the NULL check is in the wrong place (rather than the >> NULL check should be removed)? > > Afaics chasing callchains it's a bug to call this with NULL pointer > and no one really should be doing it. Like David Herrmann said it's > sometimes useful if unref/free functions automatically cope with NULL, > but ttm buffers don't seem to be of this kind. So consistency with > other ttm drivers seems better, same with all the gem_free_object > callbacks. That's fair. I'm convinced. Patches 1, 3, and 5 are also Reviewed-by: Ian Romanick <ian.d.romanick@intel.com> > -Daniel
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 26868e5c55b0..0722d18992f4 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -322,9 +322,7 @@ static void mgag200_bo_unref(struct mgag200_bo **bo) tbo = &((*bo)->bo); ttm_bo_unref(&tbo); - if (tbo == NULL) - *bo = NULL; - + *bo = NULL; } void mgag200_gem_free_object(struct drm_gem_object *obj)
ttm_bo_unref unconditionally calls kref_put on it's argument, so the thing can't be NULL without already causing Oopses. Spotted by coverity. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/mgag200/mgag200_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)