[01/13] drm/mgag200: Remove unnecessary NULL check in bo_unref
diff mbox

Message ID 1396691102-22904-3-git-send-email-daniel.vetter@ffwll.ch
State New, archived
Headers show

Commit Message

Daniel Vetter April 5, 2014, 9:44 a.m. UTC
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(-)

Comments

Ian Romanick April 7, 2014, 3:19 p.m. UTC | #1
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)
>
Daniel Vetter April 7, 2014, 7:56 p.m. UTC | #2
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
Ian Romanick April 7, 2014, 8:08 p.m. UTC | #3
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

Patch
diff mbox

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)