[PATCH 00/13] coverity

[PATCH 00/13] coverity

[Daniel Vetter]

Hi all,

Rainy w/e here and I got a bit bored, so looked at coverity again. I've closed
all outstanding issues in drivers/gpu now either as false positives or fixed in
this series expect for vmwgfx/ttm stuff (not enough clue) and one insane savage
init issue (no desire to wake dragons today).

Cheers, Daniel

Daniel Vetter (13):
  drm/mgag200: Remove unnecessary NULL check in bo_unref
  drm/mgag200: Remove unecessary NULL check in gem_free
  drm/cirrus: Remove unnecessary NULL check in bo_unref
  drm/cirrus: Remove unecessary NULL check in gem_free
  drm/ast: Remove unnecessary NULL check in bo_unref
  drm/mgag200: Remove unecessary NULL check in gem_free
  drm/via: Remove unecessary NULL check
  drm/ast: Remove dead code from cbr_scan2
  drm/udl: Initialize ret in udl_driver_load
  drm/bochs: Remove unnecessary NULL check in bo_unref
  drm/bochs: Remove unecessary NULL check in gem_free
  drm/i2c/tda998x: Fix signed overflow issue
  drm: Fix error handling in drm_master_create

 drivers/gpu/drm/ast/ast_main.c         | 7 ++-----
 drivers/gpu/drm/ast/ast_post.c         | 2 --
 drivers/gpu/drm/bochs/bochs_mm.c       | 6 +-----
 drivers/gpu/drm/cirrus/cirrus_main.c   | 6 +-----
 drivers/gpu/drm/drm_stub.c             | 5 ++++-
 drivers/gpu/drm/i2c/tda998x_drv.c      | 2 +-
 drivers/gpu/drm/mgag200/mgag200_main.c | 6 +-----
 drivers/gpu/drm/udl/udl_main.c         | 1 +
 drivers/gpu/drm/via/via_mm.c           | 2 +-
 9 files changed, 12 insertions(+), 25 deletions(-)

-- 
1.8.5.2

[PATCH 00/13] coverity

[Daniel Vetter]

Hi all,

Rainy w/e here and I got a bit bored, so looked at coverity again. I've closed
all outstanding issues in drivers/gpu now either as false positives or fixed in
this series expect for vmwgfx/ttm stuff (not enough clue) and one insane savage
init issue (no desire to wake dragons today).

Cheers, Daniel

Daniel Vetter (13):
  drm/mgag200: Remove unnecessary NULL check in bo_unref
  drm/mgag200: Remove unecessary NULL check in gem_free
  drm/cirrus: Remove unnecessary NULL check in bo_unref
  drm/cirrus: Remove unecessary NULL check in gem_free
  drm/ast: Remove unnecessary NULL check in bo_unref
  drm/mgag200: Remove unecessary NULL check in gem_free
  drm/via: Remove unecessary NULL check
  drm/ast: Remove dead code from cbr_scan2
  drm/udl: Initialize ret in udl_driver_load
  drm/bochs: Remove unnecessary NULL check in bo_unref
  drm/bochs: Remove unecessary NULL check in gem_free
  drm/i2c/tda998x: Fix signed overflow issue
  drm: Fix error handling in drm_master_create

 drivers/gpu/drm/ast/ast_main.c         | 7 ++-----
 drivers/gpu/drm/ast/ast_post.c         | 2 --
 drivers/gpu/drm/bochs/bochs_mm.c       | 6 +-----
 drivers/gpu/drm/cirrus/cirrus_main.c   | 6 +-----
 drivers/gpu/drm/drm_stub.c             | 5 ++++-
 drivers/gpu/drm/i2c/tda998x_drv.c      | 2 +-
 drivers/gpu/drm/mgag200/mgag200_main.c | 6 +-----
 drivers/gpu/drm/udl/udl_main.c         | 1 +
 drivers/gpu/drm/via/via_mm.c           | 2 +-
 9 files changed, 12 insertions(+), 25 deletions(-)

-- 
1.8.5.2

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

[Daniel Vetter]

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

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)
-- 
1.8.5.2

[PATCH 02/13] drm/mgag200: Remove unecessary NULL check in gem_free

[Daniel Vetter]

The ->gem_free_object never gets called with a NULL pointer, the check
is redundant. Also checking after the upcast allows compilers to elide
it anyway.

Spotted by coverity.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 0722d18992f4..f6b283b8375e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -329,8 +329,6 @@ void mgag200_gem_free_object(struct drm_gem_object *obj)
 {
 	struct mgag200_bo *mgag200_bo = gem_to_mga_bo(obj);
 
-	if (!mgag200_bo)
-		return;
 	mgag200_bo_unref(&mgag200_bo);
 }
 
-- 
1.8.5.2

[PATCH 03/13] drm/cirrus: Remove unnecessary NULL check in bo_unref

[Daniel Vetter]

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/cirrus/cirrus_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
index 4b0170cf53fd..b119f66fed92 100644
--- a/drivers/gpu/drm/cirrus/cirrus_main.c
+++ b/drivers/gpu/drm/cirrus/cirrus_main.c
@@ -264,9 +264,7 @@ static void cirrus_bo_unref(struct cirrus_bo **bo)
 
 	tbo = &((*bo)->bo);
 	ttm_bo_unref(&tbo);
-	if (tbo == NULL)
-		*bo = NULL;
-
+	*bo = NULL;
 }
 
 void cirrus_gem_free_object(struct drm_gem_object *obj)
-- 
1.8.5.2

[PATCH 04/13] drm/cirrus: Remove unecessary NULL check in gem_free

[Daniel Vetter]

The ->gem_free_object never gets called with a NULL pointer, the check
is redundant. Also checking after the upcast allows compilers to elide
it anyway.

Spotted by coverity.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/cirrus/cirrus_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
index b119f66fed92..99c1983f99d2 100644
--- a/drivers/gpu/drm/cirrus/cirrus_main.c
+++ b/drivers/gpu/drm/cirrus/cirrus_main.c
@@ -271,8 +271,6 @@ void cirrus_gem_free_object(struct drm_gem_object *obj)
 {
 	struct cirrus_bo *cirrus_bo = gem_to_cirrus_bo(obj);
 
-	if (!cirrus_bo)
-		return;
 	cirrus_bo_unref(&cirrus_bo);
 }
 
-- 
1.8.5.2

[PATCH 05/13] drm/ast: Remove unnecessary NULL check in bo_unref

[Daniel Vetter]

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/ast/ast_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 50535fd5a88d..38941a656312 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -411,10 +411,9 @@ static void ast_bo_unref(struct ast_bo **bo)
 
 	tbo = &((*bo)->bo);
 	ttm_bo_unref(&tbo);
-	if (tbo == NULL)
-		*bo = NULL;
-
+	*bo = NULL;
 }
+
 void ast_gem_free_object(struct drm_gem_object *obj)
 {
 	struct ast_bo *ast_bo = gem_to_ast_bo(obj);
-- 
1.8.5.2

[PATCH 06/13] drm/mgag200: Remove unecessary NULL check in gem_free

[Daniel Vetter]

The ->gem_free_object never gets called with a NULL pointer, the check
is redundant. Also checking after the upcast allows compilers to elide
it anyway.

Spotted by coverity.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 38941a656312..01bf9e730acf 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -418,8 +418,6 @@ void ast_gem_free_object(struct drm_gem_object *obj)
 {
 	struct ast_bo *ast_bo = gem_to_ast_bo(obj);
 
-	if (!ast_bo)
-		return;
 	ast_bo_unref(&ast_bo);
 }
 
-- 
1.8.5.2

[PATCH 07/13] drm/via: Remove unecessary NULL check

[Daniel Vetter]

The context_dtor callback is only called once we've successfully loaded
the driver, which means dev->dev_private is set up. The check is hence
pointless.

Also dev->dev_private is deref already above, so compilers are free
to elide it anyway.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/via/via_mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/via/via_mm.c b/drivers/gpu/drm/via/via_mm.c
index 927889105483..d70b1e1544bf 100644
--- a/drivers/gpu/drm/via/via_mm.c
+++ b/drivers/gpu/drm/via/via_mm.c
@@ -79,7 +79,7 @@ int via_final_context(struct drm_device *dev, int context)
 
 	/* Linux specific until context tracking code gets ported to BSD */
 	/* Last context, perform cleanup */
-	if (list_is_singular(&dev->ctxlist) && dev->dev_private) {
+	if (list_is_singular(&dev->ctxlist)) {
 		DRM_DEBUG("Last Context\n");
 		drm_irq_uninstall(dev);
 		via_cleanup_futex(dev_priv);
-- 
1.8.5.2

[PATCH 08/13] drm/ast: Remove dead code from cbr_scan2

[Daniel Vetter]

The outer if already checks for data != 0, so it can't really be
0. Hence remove it.

Now I don't have specs or anything for this beast, so I have no
idea whether this was actually intended or whether the logic
should be different. At least the code still seems to be doing
something useful.

Spotted by coverity.

Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_post.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index 977cfb35837a..6263116054b6 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -572,8 +572,6 @@ static u32 cbr_scan2(struct ast_private *ast)
 		for (loop = 0; loop < CBR_PASSNUM2; loop++) {
 			if ((data = cbr_test2(ast)) != 0) {
 				data2 &= data;
-				if (!data)
-					return 0;
 				break;
 			}
 		}
-- 
1.8.5.2

[PATCH 09/13] drm/udl: Initialize ret in udl_driver_load

[Daniel Vetter]

We need to set it to -ENODEV when we don't recognize the device.
Otherwise we return/print stack garbage.

Spotted by coverity.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/udl/udl_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index f5ae57406f34..e1038a945f40 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -294,6 +294,7 @@ int udl_driver_load(struct drm_device *dev, unsigned long flags)
 	dev->dev_private = udl;
 
 	if (!udl_parse_vendor_descriptor(dev, dev->usbdev)) {
+		ret = -ENODEV;
 		DRM_ERROR("firmware not recognized. Assume incompatible device\n");
 		goto err;
 	}
-- 
1.8.5.2

[PATCH 10/13] drm/bochs: Remove unnecessary NULL check in bo_unref

[Daniel Vetter]

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/bochs/bochs_mm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index f488be55d650..4a239e931aff 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -434,9 +434,7 @@ static void bochs_bo_unref(struct bochs_bo **bo)
 
 	tbo = &((*bo)->bo);
 	ttm_bo_unref(&tbo);
-	if (tbo == NULL)
-		*bo = NULL;
-
+	*bo = NULL;
 }
 
 void bochs_gem_free_object(struct drm_gem_object *obj)
-- 
1.8.5.2

[PATCH 11/13] drm/bochs: Remove unecessary NULL check in gem_free

[Daniel Vetter]

The ->gem_free_object never gets called with a NULL pointer, the check
is redundant. Also checking after the upcast allows compilers to elide
it anyway.

Noticed while chasing coverity reports, somehow this one here was not
flagged.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/bochs/bochs_mm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index 4a239e931aff..b9a695d92792 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -441,8 +441,6 @@ void bochs_gem_free_object(struct drm_gem_object *obj)
 {
 	struct bochs_bo *bochs_bo = gem_to_bochs_bo(obj);
 
-	if (!bochs_bo)
-		return;
 	bochs_bo_unref(&bochs_bo);
 }
 
-- 
1.8.5.2

[PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue

[Daniel Vetter]

This is C standard hair-splitting, but afaict
- sum will be promoted to signed int in computation since
  uint8_t fits
- signed overflow is undefined.

No we need to add up an awful lot of bytes to actually make it
overflow. But I guess the real risk is gcc spotting this and going
bananas. Fix this by simply using unsigned in to force all computations
to use the well-defined unsigned behaviour.

Spotted by coverity.

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Jean-Francois Moine <moinejf@free.fr>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 48af5cac1902..ae2754760d77 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -568,7 +568,7 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
 
 static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
 {
-	uint8_t sum = 0;
+	unsigned sum = 0;
 
 	while (bytes--)
 		sum += *buf++;
-- 
1.8.5.2

[PATCH 13/13] drm: Fix error handling in drm_master_create

[Daniel Vetter]

We need to check whether drm_ht_create succeed and clean up
if not.

Spotted by coverity.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_stub.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index dc2c6095d850..a7f22822371c 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -131,7 +131,10 @@ struct drm_master *drm_master_create(struct drm_minor *minor)
 	kref_init(&master->refcount);
 	spin_lock_init(&master->lock.spinlock);
 	init_waitqueue_head(&master->lock.lock_queue);
-	drm_ht_create(&master->magiclist, DRM_MAGIC_HASH_ORDER);
+	if (drm_ht_create(&master->magiclist, DRM_MAGIC_HASH_ORDER)) {
+		kfree(master);
+		return NULL;
+	}
 	INIT_LIST_HEAD(&master->magicfree);
 	master->minor = minor;
 
-- 
1.8.5.2

Re: [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue

[Jean-Francois Moine]

On Sat Apr 5 02:45:01 PDT 2014,
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> This is C standard hair-splitting, but afaict
> - sum will be promoted to signed int in computation since
>   uint8_t fits
> - signed overflow is undefined.
	[snip]
>  drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 48af5cac1902..ae2754760d77 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -568,7 +568,7 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
>  
>  static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
>  {
> -	uint8_t sum = 0;
> +	unsigned sum = 0;
>  
>  	while (bytes--)
>  		sum += *buf++;

This function may be simplified by:

--- tda998x_drv.c~
+++ tda998x_drv.c
@@ -568,11 +568,11 @@
 
 static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
 {
-	uint8_t sum = 0;
+	int sum = 0;
 
 	while (bytes--)
-		sum += *buf++;
-	return (255 - sum) + 1;
+		sum -= *buf++;
+	return sum;
 }
 
 #define HB(x) (x)


and the same may be done in hdmi.c:

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9e758a8..b6c9030 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -31,14 +31,14 @@
 static void hdmi_infoframe_checksum(void *buffer, size_t size)
 {
 	u8 *ptr = buffer;
-	u8 csum = 0;
+	int csum = 0;
 	size_t i;
 
 	/* compute checksum */
 	for (i = 0; i < size; i++)
-		csum += ptr[i];
+		csum -= ptr[i];
 
-	ptr[3] = 256 - csum;
+	ptr[3] = csum;
 }
 
 /**


-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/ast: Remove unecessary NULL check in gem_free

[Daniel Vetter]

The ->gem_free_object never gets called with a NULL pointer, the check
is redundant. Also checking after the upcast allows compilers to elide
it anyway.

Spotted by coverity.

v2: Fix patch subject.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/ast/ast_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 38941a656312..01bf9e730acf 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -418,8 +418,6 @@ void ast_gem_free_object(struct drm_gem_object *obj)
 {
 	struct ast_bo *ast_bo = gem_to_ast_bo(obj);
 
-	if (!ast_bo)
-		return;
 	ast_bo_unref(&ast_bo);
 }
 
-- 
1.8.5.2

Re: [PATCH 01/13] drm/mgag200: Remove unnecessary NULL check in bo_unref

[Ian Romanick]

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

Re: [PATCH 02/13] drm/mgag200: Remove unecessary NULL check in gem_free

[Ian Romanick]

On 04/05/2014 02:44 AM, Daniel Vetter wrote:
> The ->gem_free_object never gets called with a NULL pointer, the check
> is redundant. Also checking after the upcast allows compilers to elide
> it anyway.

Right... because if obj was NULL, subtracting some offset from it won't
still be NULL.

> Spotted by coverity.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

> ---
>  drivers/gpu/drm/mgag200/mgag200_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 0722d18992f4..f6b283b8375e 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -329,8 +329,6 @@ void mgag200_gem_free_object(struct drm_gem_object *obj)
>  {
>  	struct mgag200_bo *mgag200_bo = gem_to_mga_bo(obj);
>  
> -	if (!mgag200_bo)
> -		return;
>  	mgag200_bo_unref(&mgag200_bo);
>  }
>  
> 

Re: [PATCH 04/13] drm/cirrus: Remove unecessary NULL check in gem_free

[Ian Romanick]

On 04/05/2014 02:44 AM, Daniel Vetter wrote:
> The ->gem_free_object never gets called with a NULL pointer, the check
> is redundant. Also checking after the upcast allows compilers to elide
> it anyway.
> 
> Spotted by coverity.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Same as MGA change.

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

> ---
>  drivers/gpu/drm/cirrus/cirrus_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
> index b119f66fed92..99c1983f99d2 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_main.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_main.c
> @@ -271,8 +271,6 @@ void cirrus_gem_free_object(struct drm_gem_object *obj)
>  {
>  	struct cirrus_bo *cirrus_bo = gem_to_cirrus_bo(obj);
>  
> -	if (!cirrus_bo)
> -		return;
>  	cirrus_bo_unref(&cirrus_bo);
>  }
>  
> 

Re: [PATCH 06/13] drm/mgag200: Remove unecessary NULL check in gem_free

[Ian Romanick]

On 04/05/2014 02:44 AM, Daniel Vetter wrote:
> The ->gem_free_object never gets called with a NULL pointer, the check
> is redundant. Also checking after the upcast allows compilers to elide
> it anyway.
> 
> Spotted by coverity.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Same as MGA change.

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

> ---
>  drivers/gpu/drm/ast/ast_main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 38941a656312..01bf9e730acf 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -418,8 +418,6 @@ void ast_gem_free_object(struct drm_gem_object *obj)
>  {
>  	struct ast_bo *ast_bo = gem_to_ast_bo(obj);
>  
> -	if (!ast_bo)
> -		return;
>  	ast_bo_unref(&ast_bo);
>  }
>  
> 

Re: [PATCH 08/13] drm/ast: Remove dead code from cbr_scan2

[Ian Romanick]

On 04/05/2014 02:44 AM, Daniel Vetter wrote:
> The outer if already checks for data != 0, so it can't really be
> 0. Hence remove it.
> 
> Now I don't have specs or anything for this beast, so I have no
> idea whether this was actually intended or whether the logic
> should be different. At least the code still seems to be doing
> something useful.
> 
> Spotted by coverity.
> 
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/ast/ast_post.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index 977cfb35837a..6263116054b6 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -572,8 +572,6 @@ static u32 cbr_scan2(struct ast_private *ast)
>  		for (loop = 0; loop < CBR_PASSNUM2; loop++) {
>  			if ((data = cbr_test2(ast)) != 0) {
>  				data2 &= data;
> -				if (!data)
> -					return 0;

That feels like a typo... was that supposed to be 'if (!data2)'?

>  				break;
>  			}
>  		}
> 

Re: [PATCH 11/13] drm/bochs: Remove unecessary NULL check in gem_free

[Ian Romanick]

On 04/05/2014 02:45 AM, Daniel Vetter wrote:
> The ->gem_free_object never gets called with a NULL pointer, the check
> is redundant. Also checking after the upcast allows compilers to elide
> it anyway.
> 
> Noticed while chasing coverity reports, somehow this one here was not
> flagged.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Same as MGA change.

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

> ---
>  drivers/gpu/drm/bochs/bochs_mm.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
> index 4a239e931aff..b9a695d92792 100644
> --- a/drivers/gpu/drm/bochs/bochs_mm.c
> +++ b/drivers/gpu/drm/bochs/bochs_mm.c
> @@ -441,8 +441,6 @@ void bochs_gem_free_object(struct drm_gem_object *obj)
>  {
>  	struct bochs_bo *bochs_bo = gem_to_bochs_bo(obj);
>  
> -	if (!bochs_bo)
> -		return;
>  	bochs_bo_unref(&bochs_bo);
>  }
>  
> 

Re: [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue

[Ian Romanick]

On 04/05/2014 02:45 AM, Daniel Vetter wrote:
> This is C standard hair-splitting, but afaict
> - sum will be promoted to signed int in computation since
>   uint8_t fits
> - signed overflow is undefined.
> 
> No we need to add up an awful lot of bytes to actually make it
  ^^
Now

> overflow. But I guess the real risk is gcc spotting this and going
> bananas. Fix this by simply using unsigned in to force all computations
> to use the well-defined unsigned behaviour.

Seems reasonable... it also seems impossible (ha!) to break anything.

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

> Spotted by coverity.
> 
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Jean-Francois Moine <moinejf@free.fr>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 48af5cac1902..ae2754760d77 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -568,7 +568,7 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
>  
>  static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
>  {
> -	uint8_t sum = 0;
> +	unsigned sum = 0;
>  
>  	while (bytes--)
>  		sum += *buf++;
> 

Re: [PATCH 07/13] drm/via: Remove unecessary NULL check

[David Herrmann]

Hi

On Sat, Apr 5, 2014 at 11:44 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The context_dtor callback is only called once we've successfully loaded
> the driver, which means dev->dev_private is set up. The check is hence
> pointless.
>
> Also dev->dev_private is deref already above, so compilers are free
> to elide it anyway.

Are you sure compilers can assume "*ptr" implies "ptr != NULL"? I
doubt that and depending on CONFIG_DEFAULT_MMAP_MIN_ADDR I think you
can even build user-space that can successfully mmap(MAP_FIXED) at
address 0. Anyhow, I guess no-one cares besides me, so patch looks
good :)

Thanks
David

Re: [PATCH 00/13] coverity

[David Herrmann]

Hi

On Sat, Apr 5, 2014 at 11:44 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hi all,
>
> Rainy w/e here and I got a bit bored, so looked at coverity again. I've closed
> all outstanding issues in drivers/gpu now either as false positives or fixed in
> this series expect for vmwgfx/ttm stuff (not enough clue) and one insane savage
> init issue (no desire to wake dragons today).

All patches besides "drm/ast: Remove dead code from cbr_scan2" are:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

However, a few notes:
- ttm_bo_unref() sets "bo = NULL;", so the following condition is
always true. Your commit-message argues with an unconditional
kref_unref(). I think that's misleading (but still true) as that does
not change the fact that bo is always NULL afterwards.
- I am a big fan of "if (!obj) return;" in destructors. It simplifies
error-paths considerably and we can call them despite objects being
already cleared. But that's just personal style, and I don't deal much
with TTM anyway. But generally, I think we shouldn't remove these
checks blindly.
- The AST patch should be reviewed by someone who knows that hw. The
code is obviously wrong, but that doesn't mean we cannot fix it
properly.

Thanks for the cleanups, the coverity reports have been pending here
for months..
David

Re: [PATCH 07/13] drm/via: Remove unecessary NULL check

[Daniel Vetter]

On Mon, Apr 7, 2014 at 5:51 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> On Sat, Apr 5, 2014 at 11:44 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> The context_dtor callback is only called once we've successfully loaded
>> the driver, which means dev->dev_private is set up. The check is hence
>> pointless.
>>
>> Also dev->dev_private is deref already above, so compilers are free
>> to elide it anyway.
>
> Are you sure compilers can assume "*ptr" implies "ptr != NULL"? I
> doubt that and depending on CONFIG_DEFAULT_MMAP_MIN_ADDR I think you
> can even build user-space that can successfully mmap(MAP_FIXED) at
> address 0. Anyhow, I guess no-one cares besides me, so patch looks
> good :)

Yeah, my understand has been that every time you deref a pointer
somewhere the compiler is allowed to presume that the pointer isn't
NULL. Which makes mmap(MAP_FIXED) at address NULL such a dangerous
thing and iirc there's been patches floating around to severely
restrict that to make exploiting such bugs much harder. Iirc it's only
emulators like dosemu who really need to be able to map something at
NULL. Since if gcc drops the NULL check the last line of defense
(namely Oopsing on the NULL deref) can be disabled by userspace. The
usual exploit is to put a real data structure at NULL and use that
(thorugh vtables if possible) to take over the kernel.

I'm not always entirely sure on what the precise rules are really in
detail, but since coverity screamed at me about this here I've figured
coverity is probably right ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 08/13] drm/ast: Remove dead code from cbr_scan2

[Daniel Vetter]

On Mon, Apr 7, 2014 at 5:28 PM, Ian Romanick <idr@freedesktop.org> wrote:
> On 04/05/2014 02:44 AM, Daniel Vetter wrote:
>> The outer if already checks for data != 0, so it can't really be
>> 0. Hence remove it.
>>
>> Now I don't have specs or anything for this beast, so I have no
>> idea whether this was actually intended or whether the logic
>> should be different. At least the code still seems to be doing
>> something useful.
>>
>> Spotted by coverity.
>>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  drivers/gpu/drm/ast/ast_post.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
>> index 977cfb35837a..6263116054b6 100644
>> --- a/drivers/gpu/drm/ast/ast_post.c
>> +++ b/drivers/gpu/drm/ast/ast_post.c
>> @@ -572,8 +572,6 @@ static u32 cbr_scan2(struct ast_private *ast)
>>               for (loop = 0; loop < CBR_PASSNUM2; loop++) {
>>                       if ((data = cbr_test2(ast)) != 0) {
>>                               data2 &= data;
>> -                             if (!data)
>> -                                     return 0;
>
> That feels like a typo... was that supposed to be 'if (!data2)'?

Yeah this one really needs a close look, since I have no idea what's
actually intended behaviour. The patch just removes the dead code as
it is now, and the double-loop still makes some sense imo after this
change. But I really don't know the spec for this hw.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/13] drm/mgag200: Remove unnecessary NULL check in bo_unref

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/13] drm/mgag200: Remove unnecessary NULL check in bo_unref

[Ian Romanick]

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