[PATCH 0/2] GPU-DRM-Savage: Fine-tuning for savage_bci_cmdbuf()

[PATCH 0/2] GPU-DRM-Savage: Fine-tuning for savage_bci_cmdbuf()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Aug 2016 21:38:37 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Use memdup_user() rather than duplicating its implementation
  Less function calls after error detection

 drivers/gpu/drm/savage/savage_state.c | 42 +++++++++++++++--------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] GPU-DRM-Savage: Use memdup_user() rather than duplicating

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Aug 2016 18:12:03 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/savage/savage_state.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/savage/savage_state.c b/drivers/gpu/drm/savage/savage_state.c
index c01ad0a..3dc0d8f 100644
--- a/drivers/gpu/drm/savage/savage_state.c
+++ b/drivers/gpu/drm/savage/savage_state.c
@@ -1001,15 +1001,9 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 		cmdbuf->cmd_addr = kcmd_addr;
 	}
 	if (cmdbuf->vb_size) {
-		kvb_addr = kmalloc(cmdbuf->vb_size, GFP_KERNEL);
-		if (kvb_addr == NULL) {
-			ret = -ENOMEM;
-			goto done;
-		}
-
-		if (copy_from_user(kvb_addr, cmdbuf->vb_addr,
-				       cmdbuf->vb_size)) {
-			ret = -EFAULT;
+		kvb_addr = memdup_user(cmdbuf->vb_addr, cmdbuf->vb_size);
+		if (IS_ERR(kvb_addr)) {
+			ret = PTR_ERR(kvb_addr);
 			goto done;
 		}
 		cmdbuf->vb_addr = kvb_addr;
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] GPU-DRM-Savage: Less function calls in savage_bci_cmdbuf() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Aug 2016 21:28:58 +0200

The kfree() function was called in a few cases by the
savage_bci_cmdbuf() function during error handling
even if a passed variable contained a null pointer.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/savage/savage_state.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/savage/savage_state.c b/drivers/gpu/drm/savage/savage_state.c
index 3dc0d8f..5b484aa 100644
--- a/drivers/gpu/drm/savage/savage_state.c
+++ b/drivers/gpu/drm/savage/savage_state.c
@@ -1004,7 +1004,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 		kvb_addr = memdup_user(cmdbuf->vb_addr, cmdbuf->vb_size);
 		if (IS_ERR(kvb_addr)) {
 			ret = PTR_ERR(kvb_addr);
-			goto done;
+			goto free_cmd;
 		}
 		cmdbuf->vb_addr = kvb_addr;
 	}
@@ -1013,13 +1013,13 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 					  GFP_KERNEL);
 		if (kbox_addr == NULL) {
 			ret = -ENOMEM;
-			goto done;
+			goto free_vb;
 		}
 
 		if (copy_from_user(kbox_addr, cmdbuf->box_addr,
 				       cmdbuf->nbox * sizeof(struct drm_clip_rect))) {
 			ret = -EFAULT;
-			goto done;
+			goto free_vb;
 		}
 	cmdbuf->box_addr = kbox_addr;
 	}
@@ -1052,7 +1052,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 					  "beyond end of command buffer\n");
 				DMA_FLUSH();
 				ret = -EINVAL;
-				goto done;
+				goto free_box;
 			}
 			/* fall through */
 		case SAVAGE_CMD_DMA_PRIM:
@@ -1071,7 +1071,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 				      cmdbuf->vb_stride,
 				      cmdbuf->nbox, cmdbuf->box_addr);
 				if (ret != 0)
-					goto done;
+					goto free_box;
 				first_draw_cmd = NULL;
 			}
 		}
@@ -1086,7 +1086,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 					  "beyond end of command buffer\n");
 				DMA_FLUSH();
 				ret = -EINVAL;
-				goto done;
+				goto free_box;
 			}
 			ret = savage_dispatch_state(dev_priv, &cmd_header,
 				(const uint32_t *)cmdbuf->cmd_addr);
@@ -1099,7 +1099,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 					  "beyond end of command buffer\n");
 				DMA_FLUSH();
 				ret = -EINVAL;
-				goto done;
+				goto free_box;
 			}
 			ret = savage_dispatch_clear(dev_priv, &cmd_header,
 						    cmdbuf->cmd_addr,
@@ -1117,12 +1117,12 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 				  cmd_header.cmd.cmd);
 			DMA_FLUSH();
 			ret = -EINVAL;
-			goto done;
+			goto free_box;
 		}
 
 		if (ret != 0) {
 			DMA_FLUSH();
-			goto done;
+			goto free_box;
 		}
 	}
 
@@ -1133,7 +1133,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 			cmdbuf->nbox, cmdbuf->box_addr);
 		if (ret != 0) {
 			DMA_FLUSH();
-			goto done;
+			goto free_box;
 		}
 	}
 
@@ -1147,11 +1147,11 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
 		savage_freelist_put(dev, dmabuf);
 	}
 
-done:
-	/* If we didn't need to allocate them, these'll be NULL */
-	kfree(kcmd_addr);
-	kfree(kvb_addr);
+free_box:
 	kfree(kbox_addr);
-
+free_vb:
+	kfree(kvb_addr);
+free_cmd:
+	kfree(kcmd_addr);
 	return ret;
 }
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/2] GPU-DRM-Savage: Fine-tuning for savage_bci_cmdbuf()

[Daniel Vetter]

On Thu, Aug 18, 2016 at 09:42:33PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 18 Aug 2016 21:38:37 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.

savage is one of the dri1 legacy drivers, imo not really worth it to spend
time on them. otoh no one will notice any breakage either ;-)

I guess I'll apply.
-Daniel

> 
> Markus Elfring (2):
>   Use memdup_user() rather than duplicating its implementation
>   Less function calls after error detection
> 
>  drivers/gpu/drm/savage/savage_state.c | 42 +++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)
> 
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] GPU-DRM-Savage: Less function calls in savage_bci_cmdbuf() after error detection

[Daniel Vetter]

On Thu, Aug 18, 2016 at 09:48:04PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 18 Aug 2016 21:28:58 +0200
> 
> The kfree() function was called in a few cases by the
> savage_bci_cmdbuf() function during error handling
> even if a passed variable contained a null pointer.
> 
> Adjust jump targets according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Not sure this is worth it, I'll pass. Patch 1 merged. Btw I consider
cocci patches a good way to get started somewhere, but then it's much more
useful to do a bit more involved things. We keep a list of small&big
janitor tasks:

https://www.x.org/wiki/DRMJanitors/

Cleaning up all the cocci errors in drm isn't good since then the next
person won't have something easy to get started, i.e. consider you're
budget used up ;-)
-Daniel

> ---
>  drivers/gpu/drm/savage/savage_state.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/savage/savage_state.c b/drivers/gpu/drm/savage/savage_state.c
> index 3dc0d8f..5b484aa 100644
> --- a/drivers/gpu/drm/savage/savage_state.c
> +++ b/drivers/gpu/drm/savage/savage_state.c
> @@ -1004,7 +1004,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  		kvb_addr = memdup_user(cmdbuf->vb_addr, cmdbuf->vb_size);
>  		if (IS_ERR(kvb_addr)) {
>  			ret = PTR_ERR(kvb_addr);
> -			goto done;
> +			goto free_cmd;
>  		}
>  		cmdbuf->vb_addr = kvb_addr;
>  	}
> @@ -1013,13 +1013,13 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  					  GFP_KERNEL);
>  		if (kbox_addr == NULL) {
>  			ret = -ENOMEM;
> -			goto done;
> +			goto free_vb;
>  		}
>  
>  		if (copy_from_user(kbox_addr, cmdbuf->box_addr,
>  				       cmdbuf->nbox * sizeof(struct drm_clip_rect))) {
>  			ret = -EFAULT;
> -			goto done;
> +			goto free_vb;
>  		}
>  	cmdbuf->box_addr = kbox_addr;
>  	}
> @@ -1052,7 +1052,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  					  "beyond end of command buffer\n");
>  				DMA_FLUSH();
>  				ret = -EINVAL;
> -				goto done;
> +				goto free_box;
>  			}
>  			/* fall through */
>  		case SAVAGE_CMD_DMA_PRIM:
> @@ -1071,7 +1071,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  				      cmdbuf->vb_stride,
>  				      cmdbuf->nbox, cmdbuf->box_addr);
>  				if (ret != 0)
> -					goto done;
> +					goto free_box;
>  				first_draw_cmd = NULL;
>  			}
>  		}
> @@ -1086,7 +1086,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  					  "beyond end of command buffer\n");
>  				DMA_FLUSH();
>  				ret = -EINVAL;
> -				goto done;
> +				goto free_box;
>  			}
>  			ret = savage_dispatch_state(dev_priv, &cmd_header,
>  				(const uint32_t *)cmdbuf->cmd_addr);
> @@ -1099,7 +1099,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  					  "beyond end of command buffer\n");
>  				DMA_FLUSH();
>  				ret = -EINVAL;
> -				goto done;
> +				goto free_box;
>  			}
>  			ret = savage_dispatch_clear(dev_priv, &cmd_header,
>  						    cmdbuf->cmd_addr,
> @@ -1117,12 +1117,12 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  				  cmd_header.cmd.cmd);
>  			DMA_FLUSH();
>  			ret = -EINVAL;
> -			goto done;
> +			goto free_box;
>  		}
>  
>  		if (ret != 0) {
>  			DMA_FLUSH();
> -			goto done;
> +			goto free_box;
>  		}
>  	}
>  
> @@ -1133,7 +1133,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  			cmdbuf->nbox, cmdbuf->box_addr);
>  		if (ret != 0) {
>  			DMA_FLUSH();
> -			goto done;
> +			goto free_box;
>  		}
>  	}
>  
> @@ -1147,11 +1147,11 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data, struct drm_file *file_
>  		savage_freelist_put(dev, dmabuf);
>  	}
>  
> -done:
> -	/* If we didn't need to allocate them, these'll be NULL */
> -	kfree(kcmd_addr);
> -	kfree(kvb_addr);
> +free_box:
>  	kfree(kbox_addr);
> -
> +free_vb:
> +	kfree(kvb_addr);
> +free_cmd:
> +	kfree(kcmd_addr);
>  	return ret;
>  }
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/5] drm/amdgpu: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 18 Sep 2016 18:38:48 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Use kmalloc_array() in amdgpu_debugfs_gca_config_read()
  Improve determination of sizes in two functions
  Rename a jump label in amdgpu_debugfs_regs_read()
  Rename a jump label in amdgpu_device_init()
  Adjust checks for null pointers in nine functions

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 68 +++++++++++++++---------------
 1 file changed, 33 insertions(+), 35 deletions(-)

-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm/amdgpu: Use kmalloc_array() in amdgpu_debugfs_gca_config_read()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 18 Sep 2016 17:00:52 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index df7ab245..2709ebd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2438,7 +2438,7 @@ static ssize_t amdgpu_debugfs_gca_config_read(struct file *f, char __user *buf,
 	if (size & 0x3 || *pos & 0x3)
 		return -EINVAL;
 
-	config = kmalloc(256 * sizeof(*config), GFP_KERNEL);
+	config = kmalloc_array(256, sizeof(*config), GFP_KERNEL);
 	if (!config)
 		return -ENOMEM;
 
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/amdgpu: Improve determination of sizes in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 18 Sep 2016 17:24:47 +0200

Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 2709ebd..96a2457 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -855,7 +855,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
 static int amdgpu_atombios_init(struct amdgpu_device *adev)
 {
 	struct card_info *atom_card_info =
-	    kzalloc(sizeof(struct card_info), GFP_KERNEL);
+	    kzalloc(sizeof(*atom_card_info), GFP_KERNEL);
 
 	if (!atom_card_info)
 		return -ENOMEM;
@@ -1224,7 +1224,8 @@ static int amdgpu_early_init(struct amdgpu_device *adev)
 	}
 
 	adev->ip_block_status = kcalloc(adev->num_ip_blocks,
-					sizeof(struct amdgpu_ip_block_status), GFP_KERNEL);
+					sizeof(*adev->ip_block_status),
+					GFP_KERNEL);
 	if (adev->ip_block_status == NULL)
 		return -ENOMEM;
 
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm/amdgpu: Rename a jump label in amdgpu_debugfs_regs_read()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 18 Sep 2016 17:35:24 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 96a2457..2b8ba97 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2208,13 +2208,13 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, char __user *buf,
 		uint32_t value;
 
 		if (*pos > adev->rmmio_size)
-			goto end;
+			goto check_bank;
 
 		value = RREG32(*pos >> 2);
 		r = put_user(value, (uint32_t *)buf);
 		if (r) {
 			result = r;
-			goto end;
+			goto check_bank;
 		}
 
 		result += 4;
@@ -2222,8 +2222,7 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, char __user *buf,
 		*pos += 4;
 		size -= 4;
 	}
-
-end:
+ check_bank:
 	if (use_bank) {
 		amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
 		mutex_unlock(&adev->grbm_idx_mutex);
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/amdgpu: Rename a jump label in amdgpu_device_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 18 Sep 2016 17:50:09 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 2b8ba97..fed4854 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1566,18 +1566,18 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	/* Read BIOS */
 	if (!amdgpu_get_bios(adev)) {
 		r = -EINVAL;
-		goto failed;
+		goto check_runtime;
 	}
 	/* Must be an ATOMBIOS */
 	if (!adev->is_atom_bios) {
 		dev_err(adev->dev, "Expecting atombios for GPU\n");
 		r = -EINVAL;
-		goto failed;
+		goto check_runtime;
 	}
 	r = amdgpu_atombios_init(adev);
 	if (r) {
 		dev_err(adev->dev, "amdgpu_atombios_init failed\n");
-		goto failed;
+		goto check_runtime;
 	}
 
 	/* See if the asic supports SR-IOV */
@@ -1595,7 +1595,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 		if (!adev->bios) {
 			dev_err(adev->dev, "Card not posted and no BIOS - ignoring\n");
 			r = -EINVAL;
-			goto failed;
+			goto check_runtime;
 		}
 		DRM_INFO("GPU not posted. posting now...\n");
 		amdgpu_atom_asic_init(adev->mode_info.atom_context);
@@ -1605,7 +1605,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	r = amdgpu_atombios_get_clock_info(adev);
 	if (r) {
 		dev_err(adev->dev, "amdgpu_atombios_get_clock_info failed\n");
-		goto failed;
+		goto check_runtime;
 	}
 	/* init i2c buses */
 	amdgpu_atombios_i2c_init(adev);
@@ -1614,7 +1614,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	r = amdgpu_fence_driver_init(adev);
 	if (r) {
 		dev_err(adev->dev, "amdgpu_fence_driver_init failed\n");
-		goto failed;
+		goto check_runtime;
 	}
 
 	/* init the mode config */
@@ -1624,7 +1624,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	if (r) {
 		dev_err(adev->dev, "amdgpu_init failed\n");
 		amdgpu_fini(adev);
-		goto failed;
+		goto check_runtime;
 	}
 
 	adev->accel_working = true;
@@ -1634,7 +1634,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	r = amdgpu_ib_pool_init(adev);
 	if (r) {
 		dev_err(adev->dev, "IB initialization failed (%d).\n", r);
-		goto failed;
+		goto check_runtime;
 	}
 
 	r = amdgpu_ib_ring_tests(adev);
@@ -1682,12 +1682,11 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	r = amdgpu_late_init(adev);
 	if (r) {
 		dev_err(adev->dev, "amdgpu_late_init failed\n");
-		goto failed;
+		goto check_runtime;
 	}
 
 	return 0;
-
-failed:
+ check_runtime:
 	if (runtime)
 		vga_switcheroo_fini_domain_pm_ops(adev->dev);
 	return r;
-- 
2.10.0

[PATCH 5/5] drm/amdgpu: Adjust checks for null pointers in nine functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 18 Sep 2016 18:32:28 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* The script "checkpatch.pl" can point information out like the following.

  Comparison to NULL could be written !…

  Thus fix the affected source code places.

* Do also not use curly brackets at corresponding source code places
  where a single statement should be sufficient.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index fed4854..b5b7cfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -251,7 +251,7 @@ static int amdgpu_vram_scratch_init(struct amdgpu_device *adev)
 {
 	int r;
 
-	if (adev->vram_scratch.robj == NULL) {
+	if (!adev->vram_scratch.robj) {
 		r = amdgpu_bo_create(adev, AMDGPU_GPU_PAGE_SIZE,
 				     PAGE_SIZE, true, AMDGPU_GEM_DOMAIN_VRAM,
 				     AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED,
@@ -283,9 +283,9 @@ static void amdgpu_vram_scratch_fini(struct amdgpu_device *adev)
 {
 	int r;
 
-	if (adev->vram_scratch.robj == NULL) {
+	if (!adev->vram_scratch.robj)
 		return;
-	}
+
 	r = amdgpu_bo_reserve(adev->vram_scratch.robj, false);
 	if (likely(r == 0)) {
 		amdgpu_bo_kunmap(adev->vram_scratch.robj);
@@ -359,9 +359,9 @@ static int amdgpu_doorbell_init(struct amdgpu_device *adev)
 		return -EINVAL;
 
 	adev->doorbell.ptr = ioremap(adev->doorbell.base, adev->doorbell.num_doorbells * sizeof(u32));
-	if (adev->doorbell.ptr == NULL) {
+	if (!adev->doorbell.ptr)
 		return -ENOMEM;
-	}
+
 	DRM_INFO("doorbell mmio base: 0x%08X\n", (uint32_t)adev->doorbell.base);
 	DRM_INFO("doorbell mmio size: %u\n", (unsigned)adev->doorbell.size);
 
@@ -456,7 +456,7 @@ static int amdgpu_wb_init(struct amdgpu_device *adev)
 {
 	int r;
 
-	if (adev->wb.wb_obj == NULL) {
+	if (!adev->wb.wb_obj) {
 		r = amdgpu_bo_create(adev, AMDGPU_MAX_WB * 4, PAGE_SIZE, true,
 				     AMDGPU_GEM_DOMAIN_GTT, 0,  NULL, NULL,
 				     &adev->wb.wb_obj);
@@ -657,7 +657,7 @@ int amdgpu_dummy_page_init(struct amdgpu_device *adev)
 	if (adev->dummy_page.page)
 		return 0;
 	adev->dummy_page.page = alloc_page(GFP_DMA32 | GFP_KERNEL | __GFP_ZERO);
-	if (adev->dummy_page.page == NULL)
+	if (!adev->dummy_page.page)
 		return -ENOMEM;
 	adev->dummy_page.addr = pci_map_page(adev->pdev, adev->dummy_page.page,
 					0, PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
@@ -679,7 +679,7 @@ int amdgpu_dummy_page_init(struct amdgpu_device *adev)
  */
 void amdgpu_dummy_page_fini(struct amdgpu_device *adev)
 {
-	if (adev->dummy_page.page == NULL)
+	if (!adev->dummy_page.page)
 		return;
 	pci_unmap_page(adev->pdev, adev->dummy_page.addr,
 			PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
@@ -1226,10 +1226,10 @@ static int amdgpu_early_init(struct amdgpu_device *adev)
 	adev->ip_block_status = kcalloc(adev->num_ip_blocks,
 					sizeof(*adev->ip_block_status),
 					GFP_KERNEL);
-	if (adev->ip_block_status == NULL)
+	if (!adev->ip_block_status)
 		return -ENOMEM;
 
-	if (adev->ip_blocks == NULL) {
+	if (!adev->ip_blocks) {
 		DRM_ERROR("No IP blocks found!\n");
 		return r;
 	}
@@ -1525,9 +1525,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	adev->rmmio_base = pci_resource_start(adev->pdev, 5);
 	adev->rmmio_size = pci_resource_len(adev->pdev, 5);
 	adev->rmmio = ioremap(adev->rmmio_base, adev->rmmio_size);
-	if (adev->rmmio == NULL) {
+	if (!adev->rmmio)
 		return -ENOMEM;
-	}
+
 	DRM_INFO("register mmio base: 0x%08X\n", (uint32_t)adev->rmmio_base);
 	DRM_INFO("register mmio size: %u\n", (unsigned)adev->rmmio_size);
 
@@ -1542,7 +1542,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 			break;
 		}
 	}
-	if (adev->rio_mem == NULL)
+	if (!adev->rio_mem)
 		DRM_ERROR("Unable to find PCI I/O BAR\n");
 
 	/* early init functions */
@@ -1758,9 +1758,8 @@ int amdgpu_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon)
 	struct drm_connector *connector;
 	int r;
 
-	if (dev == NULL || dev->dev_private == NULL) {
+	if (!dev || !dev->dev_private)
 		return -ENODEV;
-	}
 
 	adev = dev->dev_private;
 
@@ -1791,9 +1790,9 @@ int amdgpu_suspend_kms(struct drm_device *dev, bool suspend, bool fbcon)
 			}
 		}
 
-		if (rfb == NULL || rfb->obj == NULL) {
+		if (!rfb || !rfb->obj)
 			continue;
-		}
+
 		robj = gem_to_amdgpu_bo(rfb->obj);
 		/* don't unpin kernel fb objects */
 		if (!amdgpu_fbdev_robj_is_fb(adev, robj)) {
-- 
2.10.0

RE: [PATCH 4/5] drm/amdgpu: Rename a jump label in amdgpu_device_init()

[Deucher, Alexander]

> -----Original Message-----
> From: SF Markus Elfring [mailto:elfring@users.sourceforge.net]
> Sent: Sunday, September 18, 2016 12:53 PM
> To: dri-devel@lists.freedesktop.org; Deucher, Alexander; Koenig, Christian;
> Zhou, David(ChunMing); David Airlie; Liu, Monk; StDenis, Tom
> Cc: LKML; kernel-janitors@vger.kernel.org; Julia Lawall
> Subject: [PATCH 4/5] drm/amdgpu: Rename a jump label in
> amdgpu_device_init()
> 
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 18 Sep 2016 17:50:09 +0200
> 
> Adjust jump labels according to the current Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 21 ++++++++++----------
> -
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 2b8ba97..fed4854 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1566,18 +1566,18 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
>  	/* Read BIOS */
>  	if (!amdgpu_get_bios(adev)) {
>  		r = -EINVAL;
> -		goto failed;
> +		goto check_runtime;

NACK.  Failed is a more appropriate label here.  The runtime check is just part of the failure cleanup.

Alex

>  	}
>  	/* Must be an ATOMBIOS */
>  	if (!adev->is_atom_bios) {
>  		dev_err(adev->dev, "Expecting atombios for GPU\n");
>  		r = -EINVAL;
> -		goto failed;
> +		goto check_runtime;
>  	}
>  	r = amdgpu_atombios_init(adev);
>  	if (r) {
>  		dev_err(adev->dev, "amdgpu_atombios_init failed\n");
> -		goto failed;
> +		goto check_runtime;
>  	}
> 
>  	/* See if the asic supports SR-IOV */
> @@ -1595,7 +1595,7 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
>  		if (!adev->bios) {
>  			dev_err(adev->dev, "Card not posted and no BIOS -
> ignoring\n");
>  			r = -EINVAL;
> -			goto failed;
> +			goto check_runtime;
>  		}
>  		DRM_INFO("GPU not posted. posting now...\n");
>  		amdgpu_atom_asic_init(adev->mode_info.atom_context);
> @@ -1605,7 +1605,7 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
>  	r = amdgpu_atombios_get_clock_info(adev);
>  	if (r) {
>  		dev_err(adev->dev, "amdgpu_atombios_get_clock_info
> failed\n");
> -		goto failed;
> +		goto check_runtime;
>  	}
>  	/* init i2c buses */
>  	amdgpu_atombios_i2c_init(adev);
> @@ -1614,7 +1614,7 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
>  	r = amdgpu_fence_driver_init(adev);
>  	if (r) {
>  		dev_err(adev->dev, "amdgpu_fence_driver_init failed\n");
> -		goto failed;
> +		goto check_runtime;
>  	}
> 
>  	/* init the mode config */
> @@ -1624,7 +1624,7 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
>  	if (r) {
>  		dev_err(adev->dev, "amdgpu_init failed\n");
>  		amdgpu_fini(adev);
> -		goto failed;
> +		goto check_runtime;
>  	}
> 
>  	adev->accel_working = true;
> @@ -1634,7 +1634,7 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
>  	r = amdgpu_ib_pool_init(adev);
>  	if (r) {
>  		dev_err(adev->dev, "IB initialization failed (%d).\n", r);
> -		goto failed;
> +		goto check_runtime;
>  	}
> 
>  	r = amdgpu_ib_ring_tests(adev);
> @@ -1682,12 +1682,11 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
>  	r = amdgpu_late_init(adev);
>  	if (r) {
>  		dev_err(adev->dev, "amdgpu_late_init failed\n");
> -		goto failed;
> +		goto check_runtime;
>  	}
> 
>  	return 0;
> -
> -failed:
> + check_runtime:
>  	if (runtime)
>  		vga_switcheroo_fini_domain_pm_ops(adev->dev);
>  	return r;
> --
> 2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/5] GPU-DRM: Fine-tuning for four function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 19 Sep 2016 17:47:37 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Use kmalloc_array() in drm_legacy_addbufs_pci()
  Replace two kzalloc() calls by kcalloc() in drm_legacy_addbufs_pci()
  Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_agp()
  Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_sg()
  Rename a jump label in drm_legacy_mapbufs()

 drivers/gpu/drm/drm_bufs.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] GPU-DRM: Use kmalloc_array() in drm_legacy_addbufs_pci()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 19 Sep 2016 17:07:06 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_bufs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 3219151..ed33f43 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -923,8 +923,9 @@ int drm_legacy_addbufs_pci(struct drm_device *dev,
 	/* Keep the original pagelist until we know all the allocations
 	 * have succeeded
 	 */
-	temp_pagelist = kmalloc((dma->page_count + (count << page_order)) *
-			       sizeof(*dma->pagelist), GFP_KERNEL);
+	temp_pagelist = kmalloc_array(dma->page_count + (count << page_order),
+				      sizeof(*dma->pagelist),
+				      GFP_KERNEL);
 	if (!temp_pagelist) {
 		kfree(entry->buflist);
 		kfree(entry->seglist);
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] GPU-DRM: Replace two kzalloc() calls by kcalloc() in drm_legacy_addbufs_pci()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 19 Sep 2016 17:17:34 +0200

The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kcalloc over kzalloc with multiply

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_bufs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index ed33f43..8a31dac 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -905,14 +905,14 @@ int drm_legacy_addbufs_pci(struct drm_device *dev,
 		return -EINVAL;
 	}
 
-	entry->buflist = kzalloc(count * sizeof(*entry->buflist), GFP_KERNEL);
+	entry->buflist = kcalloc(count, sizeof(*entry->buflist), GFP_KERNEL);
 	if (!entry->buflist) {
 		mutex_unlock(&dev->struct_mutex);
 		atomic_dec(&dev->buf_alloc);
 		return -ENOMEM;
 	}
 
-	entry->seglist = kzalloc(count * sizeof(*entry->seglist), GFP_KERNEL);
+	entry->seglist = kcalloc(count, sizeof(*entry->seglist), GFP_KERNEL);
 	if (!entry->seglist) {
 		kfree(entry->buflist);
 		mutex_unlock(&dev->struct_mutex);
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] GPU-DRM: Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_agp()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 19 Sep 2016 17:24:20 +0200

The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kcalloc over kzalloc with multiply

Thus fix the affected source code place.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_bufs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 8a31dac..36dd685 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -755,7 +755,7 @@ int drm_legacy_addbufs_agp(struct drm_device *dev,
 		return -EINVAL;
 	}
 
-	entry->buflist = kzalloc(count * sizeof(*entry->buflist), GFP_KERNEL);
+	entry->buflist = kcalloc(count, sizeof(*entry->buflist), GFP_KERNEL);
 	if (!entry->buflist) {
 		mutex_unlock(&dev->struct_mutex);
 		atomic_dec(&dev->buf_alloc);
-- 
2.10.0

[PATCH 4/5] GPU-DRM: Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_sg()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 19 Sep 2016 17:30:31 +0200

The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kcalloc over kzalloc with multiply

Thus fix the affected source code place.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_bufs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 36dd685..adb1dd7 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -1117,8 +1117,7 @@ static int drm_legacy_addbufs_sg(struct drm_device *dev,
 		return -EINVAL;
 	}
 
-	entry->buflist = kzalloc(count * sizeof(*entry->buflist),
-				GFP_KERNEL);
+	entry->buflist = kcalloc(count, sizeof(*entry->buflist), GFP_KERNEL);
 	if (!entry->buflist) {
 		mutex_unlock(&dev->struct_mutex);
 		atomic_dec(&dev->buf_alloc);
-- 
2.10.0

[PATCH 5/5] GPU-DRM: Rename a jump label in drm_legacy_mapbufs()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 19 Sep 2016 17:37:27 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_bufs.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index adb1dd7..0d5ee1e 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -1476,7 +1476,7 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data,
 
 			if (!map) {
 				retcode = -EINVAL;
-				goto done;
+				goto status_indication;
 			}
 			virtual = vm_mmap(file_priv->filp, 0, map->size,
 					  PROT_READ | PROT_WRITE,
@@ -1490,7 +1490,7 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data,
 		if (virtual > -1024UL) {
 			/* Real error */
 			retcode = (signed long)virtual;
-			goto done;
+			goto status_indication;
 		}
 		request->virtual = (void __user *)virtual;
 
@@ -1499,28 +1499,28 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data,
 					 &dma->buflist[i]->idx,
 					 sizeof(request->list[0].idx))) {
 				retcode = -EFAULT;
-				goto done;
+				goto status_indication;
 			}
 			if (copy_to_user(&request->list[i].total,
 					 &dma->buflist[i]->total,
 					 sizeof(request->list[0].total))) {
 				retcode = -EFAULT;
-				goto done;
+				goto status_indication;
 			}
 			if (copy_to_user(&request->list[i].used,
 					 &zero, sizeof(zero))) {
 				retcode = -EFAULT;
-				goto done;
+				goto status_indication;
 			}
 			address = virtual + dma->buflist[i]->offset;	/* *** */
 			if (copy_to_user(&request->list[i].address,
 					 &address, sizeof(address))) {
 				retcode = -EFAULT;
-				goto done;
+				goto status_indication;
 			}
 		}
 	}
-      done:
+ status_indication:
 	request->count = dma->buf_count;
 	DRM_DEBUG("%d buffers, retcode = %d\n", request->count, retcode);
 
-- 
2.10.0

Re: [PATCH 1/5] drm/amdgpu: Use kmalloc_array() in amdgpu_debugfs_gca_config_read()

[Alex Deucher]

On Sun, Sep 18, 2016 at 12:50 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 18 Sep 2016 17:00:52 +0200
>
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied.  thanks.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index df7ab245..2709ebd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2438,7 +2438,7 @@ static ssize_t amdgpu_debugfs_gca_config_read(struct file *f, char __user *buf,
>         if (size & 0x3 || *pos & 0x3)
>                 return -EINVAL;
>
> -       config = kmalloc(256 * sizeof(*config), GFP_KERNEL);
> +       config = kmalloc_array(256, sizeof(*config), GFP_KERNEL);
>         if (!config)
>                 return -ENOMEM;
>
> --
> 2.10.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/6] GPU-DRM-GMA500: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 20 Sep 2016 10:48:04 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (6):
  Use kmalloc_array() in mid_get_vbt_data_r10()
  Rename a jump label in mid_get_vbt_data_r10()
  Move a variable assignment in mid_get_vbt_data_r10()
  Fix indentation for a function call parameter in mid_get_vbt_data_r10()
  One error message less for a GCT revision mismatch in mid_get_vbt_data()
  Rename a jump label in mid_get_vbt_data()

 drivers/gpu/drm/gma500/mid_bios.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] GPU-DRM-GMA500: Use kmalloc_array() in mid_get_vbt_data_r10()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 20 Sep 2016 08:54:07 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/gma500/mid_bios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
index d75ecb3..a833568 100644
--- a/drivers/gpu/drm/gma500/mid_bios.c
+++ b/drivers/gpu/drm/gma500/mid_bios.c
@@ -235,7 +235,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
 	if (read_vbt_r10(addr, &vbt))
 		return -1;
 
-	gct = kmalloc(sizeof(*gct) * vbt.panel_count, GFP_KERNEL);
+	gct = kmalloc_array(vbt.panel_count, sizeof(*gct), GFP_KERNEL);
 	if (!gct)
 		return -1;
 
-- 
2.10.0

[PATCH 2/6] GPU-DRM-GMA500: Rename a jump label in mid_get_vbt_data_r10()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 20 Sep 2016 09:09:10 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/gma500/mid_bios.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
index a833568..cf4e605 100644
--- a/drivers/gpu/drm/gma500/mid_bios.c
+++ b/drivers/gpu/drm/gma500/mid_bios.c
@@ -242,7 +242,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
 	gct_virtual = ioremap(addr + sizeof(vbt),
 			sizeof(*gct) * vbt.panel_count);
 	if (!gct_virtual)
-		goto out;
+		goto free_gct;
 	memcpy_fromio(gct, gct_virtual, sizeof(*gct));
 	iounmap(gct_virtual);
 
@@ -270,7 +270,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
 	dp_ti->vsync_pulse_width_lo = ti->vsync_pulse_width_lo;
 
 	ret = 0;
-out:
+ free_gct:
 	kfree(gct);
 	return ret;
 }
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] GPU-DRM-GMA500: Move a variable assignment in mid_get_vbt_data_r10()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 20 Sep 2016 10:32:12 +0200

One local variable was set to an error code before a concrete
error situation was detected. Thus move the corresponding assignment into
an if branch to indicate a software failure there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/gma500/mid_bios.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
index cf4e605..3caee42 100644
--- a/drivers/gpu/drm/gma500/mid_bios.c
+++ b/drivers/gpu/drm/gma500/mid_bios.c
@@ -230,7 +230,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
 	struct gct_r10 *gct;
 	struct oaktrail_timing_info *dp_ti = &dev_priv->gct_data.DTD;
 	struct gct_r10_timing_info *ti;
-	int ret = -1;
+	int ret;
 
 	if (read_vbt_r10(addr, &vbt))
 		return -1;
@@ -241,8 +241,10 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
 
 	gct_virtual = ioremap(addr + sizeof(vbt),
 			sizeof(*gct) * vbt.panel_count);
-	if (!gct_virtual)
+	if (!gct_virtual) {
+		ret = -1;
 		goto free_gct;
+	}
 	memcpy_fromio(gct, gct_virtual, sizeof(*gct));
 	iounmap(gct_virtual);
 
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] GPU-DRM-GMA500: Fix indentation for a function call parameter in mid_get_vbt_data_r10()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 20 Sep 2016 10:34:28 +0200

Adjust the indentation for a single function call parameter here.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/gma500/mid_bios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
index 3caee42..9004d30 100644
--- a/drivers/gpu/drm/gma500/mid_bios.c
+++ b/drivers/gpu/drm/gma500/mid_bios.c
@@ -240,7 +240,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
 		return -1;
 
 	gct_virtual = ioremap(addr + sizeof(vbt),
-			sizeof(*gct) * vbt.panel_count);
+			      sizeof(*gct) * vbt.panel_count);
 	if (!gct_virtual) {
 		ret = -1;
 		goto free_gct;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 20 Sep 2016 10:36:19 +0200

A single error message should be sufficient to inform about
the detection of an unknown GCT revision at the end.
Thus return after the logging call in this case directly.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/gma500/mid_bios.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
index 9004d30..e5cece0 100644
--- a/drivers/gpu/drm/gma500/mid_bios.c
+++ b/drivers/gpu/drm/gma500/mid_bios.c
@@ -320,6 +320,7 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv)
 		break;
 	default:
 		dev_err(dev->dev, "Unknown revision of GCT!\n");
+		return;
 	}
 
 out:
-- 
2.10.0

[PATCH 6/6] GPU-DRM-GMA500: Rename a jump label in mid_get_vbt_data()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 20 Sep 2016 10:40:22 +0200

* Adjust a jump target.

* Delete the explicit initialisation for the local variable "ret"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/gma500/mid_bios.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
index e5cece0..602d16f 100644
--- a/drivers/gpu/drm/gma500/mid_bios.c
+++ b/drivers/gpu/drm/gma500/mid_bios.c
@@ -284,7 +284,7 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv)
 	u8 __iomem *vbt_virtual;
 	struct mid_vbt_header vbt_header;
 	struct pci_dev *pci_gfx_root = pci_get_bus_and_slot(0, PCI_DEVFN(2, 0));
-	int ret = -1;
+	int ret;
 
 	/* Get the address of the platform config vbt */
 	pci_read_config_dword(pci_gfx_root, 0xFC, &addr);
@@ -293,18 +293,18 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv)
 	dev_dbg(dev->dev, "drm platform config address is %x\n", addr);
 
 	if (!addr)
-		goto out;
+		goto report_failure;
 
 	/* get the virtual address of the vbt */
 	vbt_virtual = ioremap(addr, sizeof(vbt_header));
 	if (!vbt_virtual)
-		goto out;
+		goto report_failure;
 
 	memcpy_fromio(&vbt_header, vbt_virtual, sizeof(vbt_header));
 	iounmap(vbt_virtual);
 
 	if (memcmp(&vbt_header.signature, "$GCT", 4))
-		goto out;
+		goto report_failure;
 
 	dev_dbg(dev->dev, "GCT revision is %02x\n", vbt_header.revision);
 
@@ -322,9 +322,8 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv)
 		dev_err(dev->dev, "Unknown revision of GCT!\n");
 		return;
 	}
-
-out:
 	if (ret)
+ report_failure:
 		dev_err(dev->dev, "Unable to read GCT!");
 	else
 		dev_priv->has_gct = true;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] GPU-DRM-GMA500: Rename a jump label in mid_get_vbt_data_r10()

[Jani Nikula]

On Tue, 20 Sep 2016, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 20 Sep 2016 09:09:10 +0200
>
> Adjust a jump label according to the current Linux coding style convention.

Generally, please don't send patches to fix checkpatch issues in
existing code. Moreover, this particular "convention" is just something
someone sneaked into CodingStyle, it's subjective, and it's probably
going to be removed soon.

NAK.

BR,
Jani.

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/gma500/mid_bios.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
> index a833568..cf4e605 100644
> --- a/drivers/gpu/drm/gma500/mid_bios.c
> +++ b/drivers/gpu/drm/gma500/mid_bios.c
> @@ -242,7 +242,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
>  	gct_virtual = ioremap(addr + sizeof(vbt),
>  			sizeof(*gct) * vbt.panel_count);
>  	if (!gct_virtual)
> -		goto out;
> +		goto free_gct;
>  	memcpy_fromio(gct, gct_virtual, sizeof(*gct));
>  	iounmap(gct_virtual);
>  
> @@ -270,7 +270,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
>  	dp_ti->vsync_pulse_width_lo = ti->vsync_pulse_width_lo;
>  
>  	ret = 0;
> -out:
> + free_gct:
>  	kfree(gct);
>  	return ret;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/6] GPU-DRM-GMA500: Use kmalloc_array() in mid_get_vbt_data_r10()

[Jani Nikula]

On Tue, 20 Sep 2016, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 20 Sep 2016 08:54:07 +0200
>
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.

Did you test this running on the hardware?

BR,
Jani.

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/gma500/mid_bios.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
> index d75ecb3..a833568 100644
> --- a/drivers/gpu/drm/gma500/mid_bios.c
> +++ b/drivers/gpu/drm/gma500/mid_bios.c
> @@ -235,7 +235,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private *dev_priv, u32 addr)
>  	if (read_vbt_r10(addr, &vbt))
>  		return -1;
>  
> -	gct = kmalloc(sizeof(*gct) * vbt.panel_count, GFP_KERNEL);
> +	gct = kmalloc_array(vbt.panel_count, sizeof(*gct), GFP_KERNEL);
>  	if (!gct)
>  		return -1;

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 5/6] GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[Jani Nikula]

On Tue, 20 Sep 2016, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 20 Sep 2016 10:36:19 +0200
>
> A single error message should be sufficient to inform about
> the detection of an unknown GCT revision at the end.
> Thus return after the logging call in this case directly.

Did you test this?

BR,
Jani.

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/gma500/mid_bios.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
> index 9004d30..e5cece0 100644
> --- a/drivers/gpu/drm/gma500/mid_bios.c
> +++ b/drivers/gpu/drm/gma500/mid_bios.c
> @@ -320,6 +320,7 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv)
>  		break;
>  	default:
>  		dev_err(dev->dev, "Unknown revision of GCT!\n");
> +		return;
>  	}
>  
>  out:

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 6/6] GPU-DRM-GMA500: Rename a jump label in mid_get_vbt_data()

[Jani Nikula]

On Tue, 20 Sep 2016, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 20 Sep 2016 10:40:22 +0200
>
> * Adjust a jump target.

Please don't.

BR,
Jani.

>
> * Delete the explicit initialisation for the local variable "ret"
>   which became unnecessary with this refactoring.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/gma500/mid_bios.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/mid_bios.c b/drivers/gpu/drm/gma500/mid_bios.c
> index e5cece0..602d16f 100644
> --- a/drivers/gpu/drm/gma500/mid_bios.c
> +++ b/drivers/gpu/drm/gma500/mid_bios.c
> @@ -284,7 +284,7 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv)
>  	u8 __iomem *vbt_virtual;
>  	struct mid_vbt_header vbt_header;
>  	struct pci_dev *pci_gfx_root = pci_get_bus_and_slot(0, PCI_DEVFN(2, 0));
> -	int ret = -1;
> +	int ret;
>  
>  	/* Get the address of the platform config vbt */
>  	pci_read_config_dword(pci_gfx_root, 0xFC, &addr);
> @@ -293,18 +293,18 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv)
>  	dev_dbg(dev->dev, "drm platform config address is %x\n", addr);
>  
>  	if (!addr)
> -		goto out;
> +		goto report_failure;
>  
>  	/* get the virtual address of the vbt */
>  	vbt_virtual = ioremap(addr, sizeof(vbt_header));
>  	if (!vbt_virtual)
> -		goto out;
> +		goto report_failure;
>  
>  	memcpy_fromio(&vbt_header, vbt_virtual, sizeof(vbt_header));
>  	iounmap(vbt_virtual);
>  
>  	if (memcmp(&vbt_header.signature, "$GCT", 4))
> -		goto out;
> +		goto report_failure;
>  
>  	dev_dbg(dev->dev, "GCT revision is %02x\n", vbt_header.revision);
>  
> @@ -322,9 +322,8 @@ static void mid_get_vbt_data(struct drm_psb_private *dev_priv)
>  		dev_err(dev->dev, "Unknown revision of GCT!\n");
>  		return;
>  	}
> -
> -out:
>  	if (ret)
> + report_failure:
>  		dev_err(dev->dev, "Unable to read GCT!");
>  	else
>  		dev_priv->has_gct = true;

-- 
Jani Nikula, Intel Open Source Technology Center

Re: GPU-DRM-GMA500: Use kmalloc_array() in mid_get_vbt_data_r10()

[SF Markus Elfring]

>> A multiplication for the size determination of a memory allocation
>> indicated that an array data structure should be processed.
>> Thus use the corresponding function "kmalloc_array".
>>
>> This issue was detected by using the Coccinelle software.
> 
> Did you test this running on the hardware?

No. - My "test computer" does not provide the corresponding hardware for the
affected driver source files.

Regards,
Markus

Re: GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[SF Markus Elfring]

>> A single error message should be sufficient to inform about
>> the detection of an unknown GCT revision at the end.
>> Thus return after the logging call in this case directly.
> 
> Did you test this?

What is your software development opinion for this update suggestion?

Regards,
Markus

Re: [PATCH 5/6] GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[Dan Carpenter]

On Tue, Sep 20, 2016 at 01:07:35PM +0300, Jani Nikula wrote:
> On Tue, 20 Sep 2016, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Tue, 20 Sep 2016 10:36:19 +0200
> >
> > A single error message should be sufficient to inform about
> > the detection of an unknown GCT revision at the end.
> > Thus return after the logging call in this case directly.
> 
> Did you test this?
> 

Don't be a dummy...  This is easy to review an it fixes a bug.

I'm fine with you NAKing all these patches based on who they are from.
I mostly just delete these without responding because the guy has
history of introducing bugs and never listens to feedback.  But asking
pointless rhetorical questions is not helpful.

A lot of people are CC'd and you're wasting everyone's time by asking
questions where you know the answer.

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[SF Markus Elfring]

>>> A single error message should be sufficient to inform about
>>> the detection of an unknown GCT revision at the end.
>>> Thus return after the logging call in this case directly.
>>
>> Did you test this?
>>
> 
> Don't be a dummy...  This is easy to review an it fixes a bug.

Thanks for this kind of constructive feedback.


> I'm fine with you NAKing all these patches based on who they are from.

Would you like to clarify such an information a bit more?


> I mostly just delete these without responding because the guy has
> history of introducing bugs and never listens to feedback.

I admit that I'll stumble on programming mistakes again occasionally
as another ordinary free software developer who is struggling various open issues.

I am listening to various feedback. My responses might not be pleasing enough
for you. Are you looking for any special information to improve
a corresponding discussion?

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[Dan Carpenter]

On Tue, Sep 20, 2016 at 01:03:06PM +0200, SF Markus Elfring wrote:
> Are you looking for any special information to improve
> a corresponding discussion?

If you restricted yourself to only sending bug fixes and not sending
any more cleanups that would be good.

Please stop sending clean up patches.

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[SF Markus Elfring]

> If you restricted yourself to only sending bug fixes and not sending
> any more cleanups that would be good.

Thanks for another bit of constructive feedback.


> Please stop sending clean up patches.

This will not happen for a while.

I am in the process of informing various developers about some software
update opportunities. The proposed changes will belong to a mixture of error
categories as you observe them so far usually.

The involved static source code analysis will point more details out
for further considerations, won't it?

Regards,
Markus

Re: [PATCH 5/6] GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[Jani Nikula]

On Tue, 20 Sep 2016, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Don't be a dummy...  This is easy to review an it fixes a bug.
>
> I'm fine with you NAKing all these patches based on who they are from.
> I mostly just delete these without responding because the guy has
> history of introducing bugs and never listens to feedback.  But asking
> pointless rhetorical questions is not helpful.
>
> A lot of people are CC'd and you're wasting everyone's time by asking
> questions where you know the answer.

Fair enough, sorry for the noise.

To be honest, I did only look at the patches, not who they were from. We
have CI for drm/i915, but I don't think it's constructive to keep
changing drivers like this where the upstream isn't actively developed
and tested. But I digress. It's up to Patrik anyway.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 6/6] GPU-DRM-GMA500: Rename a jump label in mid_get_vbt_data()

[Dan Carpenter]

On Tue, Sep 20, 2016 at 01:08:12PM +0300, Jani Nikula wrote:
> On Tue, 20 Sep 2016, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Tue, 20 Sep 2016 10:40:22 +0200
> >
> > * Adjust a jump target.
> 
> Please don't.
> 

Also there is nothing in CodingStyle that prohibits out: labels.  I
wrote that section and I wrote it in a deliberately way because no one
wants to see a bunch of "cleanup" patches that change the label names
for no reason.

People have been complaining about this in the kernel-summit mailing
list as if CodingStyle bans out labels and that it's my fault.  Neither
of these complaints are accurate.

regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] GPU-DRM-GMA500: One error message less for a GCT revision mismatch in mid_get_vbt_data()

[Patrik Jakobsson]

On Tue, Sep 20, 2016 at 2:08 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 20 Sep 2016, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> Don't be a dummy...  This is easy to review an it fixes a bug.

In this particular case it might not be clear that an unknown GCT
version causes a complete GCT failure so both messages are useful.

>>
>> I'm fine with you NAKing all these patches based on who they are from.
>> I mostly just delete these without responding because the guy has
>> history of introducing bugs and never listens to feedback.  But asking
>> pointless rhetorical questions is not helpful.
>>
>> A lot of people are CC'd and you're wasting everyone's time by asking
>> questions where you know the answer.
>
> Fair enough, sorry for the noise.
>
> To be honest, I did only look at the patches, not who they were from. We
> have CI for drm/i915, but I don't think it's constructive to keep
> changing drivers like this where the upstream isn't actively developed
> and tested. But I digress. It's up to Patrik anyway.

Nothing in this series is very helpful so NAK. In general I'm not fond
of trivial changes like this since it's hard to say what motivates the
author. In theory it shouldn't matter but so far it's been directly
related to the quality of the patches. I can help test changes for
gma500 if needed but please make it worth my while.

Best regards
Patrik

>
> BR,
> Jani.
>
>
>
> --
> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 4/5] GPU-DRM: Replace a kzalloc() call by kcalloc() in drm_legacy_addbufs_sg()

[Daniel Vetter]

On Mon, Sep 19, 2016 at 05:56:49PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 19 Sep 2016 17:30:31 +0200
> 
> The script "checkpatch.pl" can point information out like the following.
> 
> WARNING: Prefer kcalloc over kzalloc with multiply
> 
> Thus fix the affected source code place.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Merged patches 1-4. I agree with Jani that changing jump labels is a pure
bikeshed, so didn't apply them.

Also I'll repeat that imo checkpatch patches are great to get started, but
it's much better to do more involved work. Both since that tends to be
more interesting, and fixing all the checkpatch issues will rob the next
newbies of some great starting opportunity. Which means from now on I'll
only selectively apply your checkpatch patches.

We have todo list with some ideas at:

https://www.x.org/wiki/DRMJanitors/

Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_bufs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> index 36dd685..adb1dd7 100644
> --- a/drivers/gpu/drm/drm_bufs.c
> +++ b/drivers/gpu/drm/drm_bufs.c
> @@ -1117,8 +1117,7 @@ static int drm_legacy_addbufs_sg(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> -	entry->buflist = kzalloc(count * sizeof(*entry->buflist),
> -				GFP_KERNEL);
> +	entry->buflist = kcalloc(count, sizeof(*entry->buflist), GFP_KERNEL);
>  	if (!entry->buflist) {
>  		mutex_unlock(&dev->struct_mutex);
>  		atomic_dec(&dev->buf_alloc);
> -- 
> 2.10.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 00/14] GPU-DRM-OMAP: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 18:28:38 +0200

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (14):
  Use kmalloc_array() in tiler_map_show()
  Replace another kmalloc() call by kmalloc_array() in tiler_map_show()
  Less function calls in tiler_map_show() after error detection
  Delete an unnecessary variable initialisation in tiler_map_show()
  Improve a size determination in dmm_txn_append()
  Improve a size determination in omap_dmm_probe()
  Rename a jump label in omap_dmm_probe()
  Rename a jump label in dmm_txn_commit()
  Delete an unnecessary variable initialisation in dmm_txn_commit()
  Use kmalloc_array() in omap_gem_attach_pages()
  Replace a kzalloc() call by kcalloc() in omap_gem_attach_pages()
  Move a variable assignment in omap_gem_attach_pages()
  Rename a jump label in omap_gem_new_dmabuf()
  Rename a jump label in four functions

 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 58 ++++++++++++++++----------------
 drivers/gpu/drm/omapdrm/omap_gem.c       | 44 +++++++++++-------------
 2 files changed, 49 insertions(+), 53 deletions(-)

-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/14] GPU-DRM-OMAP: Use kmalloc_array() in tiler_map_show()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 12:23:46 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 4ceed7a9..7b32dd3 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -917,8 +917,7 @@ int tiler_map_show(struct seq_file *s, void *arg)
 
 	h_adj = omap_dmm->container_height / ydiv;
 	w_adj = omap_dmm->container_width / xdiv;
-
-	map = kmalloc(h_adj * sizeof(*map), GFP_KERNEL);
+	map = kmalloc_array(h_adj, sizeof(*map), GFP_KERNEL);
 	global_map = kmalloc((w_adj + 1) * h_adj, GFP_KERNEL);
 
 	if (!map || !global_map)
-- 
2.10.0

[PATCH 02/14] GPU-DRM-OMAP: Replace another kmalloc() call by kmalloc_array() in tiler_map_show()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 12:54:07 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array" at another place.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 7b32dd3..3a4f91b 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -918,8 +918,7 @@ int tiler_map_show(struct seq_file *s, void *arg)
 	h_adj = omap_dmm->container_height / ydiv;
 	w_adj = omap_dmm->container_width / xdiv;
 	map = kmalloc_array(h_adj, sizeof(*map), GFP_KERNEL);
-	global_map = kmalloc((w_adj + 1) * h_adj, GFP_KERNEL);
-
+	global_map = kmalloc_array(h_adj, w_adj + 1, GFP_KERNEL);
 	if (!map || !global_map)
 		goto error;
 
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/14] GPU-DRM-OMAP: Less function calls in tiler_map_show() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 13:16:20 +0200

The kfree() function was called in up to two cases
by the tiler_map_show() function during error handling even if
the passed variable contained a null pointer.

* Adjust jump targets according to the Linux coding style convention.

* Split a condition check for memory allocation failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

* Return directly after a call of the function "kmalloc_array" failed
  at the beginning.

* Move an assignment for the local variable "w_adj" behind the first
  memory allocation.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 3a4f91b..60beeb9 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -916,11 +916,14 @@ int tiler_map_show(struct seq_file *s, void *arg)
 	}
 
 	h_adj = omap_dmm->container_height / ydiv;
-	w_adj = omap_dmm->container_width / xdiv;
 	map = kmalloc_array(h_adj, sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return 0;
+
+	w_adj = omap_dmm->container_width / xdiv;
 	global_map = kmalloc_array(h_adj, w_adj + 1, GFP_KERNEL);
-	if (!map || !global_map)
-		goto error;
+	if (!global_map)
+		goto free_map;
 
 	for (lut_idx = 0; lut_idx < omap_dmm->num_lut; lut_idx++) {
 		memset(map, 0, h_adj * sizeof(*map));
@@ -982,10 +985,9 @@ int tiler_map_show(struct seq_file *s, void *arg)
 		}
 	}
 
-error:
-	kfree(map);
 	kfree(global_map);
-
+ free_map:
+	kfree(map);
 	return 0;
 }
 #endif
-- 
2.10.0

[PATCH 04/14] GPU-DRM-OMAP: Delete an unnecessary variable initialisation in tiler_map_show()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 13:31:45 +0200

The local variable "map" will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 60beeb9..c262ef5 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -896,7 +896,7 @@ static void map_2d_info(char **map, int xdiv, int ydiv, char *nice,
 int tiler_map_show(struct seq_file *s, void *arg)
 {
 	int xdiv = 2, ydiv = 1;
-	char **map = NULL, *global_map;
+	char **map, *global_map;
 	struct tiler_block *block;
 	struct tcm_area a, p;
 	int i;
-- 
2.10.0

[PATCH 04/14] GPU-DRM-OMAP: Delete an unnecessary variable initialisation in tiler_map_show()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 13:31:45 +0200

The local variable "map" will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 60beeb9..c262ef5 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -896,7 +896,7 @@ static void map_2d_info(char **map, int xdiv, int ydiv, char *nice,
 int tiler_map_show(struct seq_file *s, void *arg)
 {
 	int xdiv = 2, ydiv = 1;
-	char **map = NULL, *global_map;
+	char **map, *global_map;
 	struct tiler_block *block;
 	struct tcm_area a, p;
 	int i;
-- 
2.10.0

[PATCH 05/14] GPU-DRM-OMAP: Improve a size determination in dmm_txn_append()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 13:53:11 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index c262ef5..f110965 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -224,7 +224,7 @@ static void dmm_txn_append(struct dmm_txn *txn, struct pat_area *area,
 	int rows = (1 + area->y1 - area->y0);
 	int i = columns*rows;
 
-	pat = alloc_dma(txn, sizeof(struct pat), &pat_pa);
+	pat = alloc_dma(txn, sizeof(*pat), &pat_pa);
 
 	if (txn->last_pat)
 		txn->last_pat->next_pa = (uint32_t)pat_pa;
-- 
2.10.0

[PATCH 06/14] GPU-DRM-OMAP: Improve a size determination in omap_dmm_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 17:21:57 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index f110965..c6a7197 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -735,7 +735,8 @@ static int omap_dmm_probe(struct platform_device *dev)
 
 	/* alloc engines */
 	omap_dmm->engines = kcalloc(omap_dmm->num_engines,
-				    sizeof(struct refill_engine), GFP_KERNEL);
+				    sizeof(*omap_dmm->engines),
+				    GFP_KERNEL);
 	if (!omap_dmm->engines) {
 		ret = -ENOMEM;
 		goto fail;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/14] GPU-DRM-OMAP: Rename a jump label in omap_dmm_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 17:30:25 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index c6a7197..5f6f21b 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -624,7 +624,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 
 	omap_dmm = kzalloc(sizeof(*omap_dmm), GFP_KERNEL);
 	if (!omap_dmm)
-		goto fail;
+		goto check_dmm_removal;
 
 	/* initialize lists */
 	INIT_LIST_HEAD(&omap_dmm->alloc_head);
@@ -648,20 +648,20 @@ static int omap_dmm_probe(struct platform_device *dev)
 	mem = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	if (!mem) {
 		dev_err(&dev->dev, "failed to get base address resource\n");
-		goto fail;
+		goto check_dmm_removal;
 	}
 
 	omap_dmm->base = ioremap(mem->start, SZ_2K);
 
 	if (!omap_dmm->base) {
 		dev_err(&dev->dev, "failed to get dmm base address\n");
-		goto fail;
+		goto check_dmm_removal;
 	}
 
 	omap_dmm->irq = platform_get_irq(dev, 0);
 	if (omap_dmm->irq < 0) {
 		dev_err(&dev->dev, "failed to get IRQ resource\n");
-		goto fail;
+		goto check_dmm_removal;
 	}
 
 	omap_dmm->dev = &dev->dev;
@@ -699,7 +699,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 		dev_err(&dev->dev, "couldn't register IRQ %d, error %d\n",
 			omap_dmm->irq, ret);
 		omap_dmm->irq = -1;
-		goto fail;
+		goto check_dmm_removal;
 	}
 
 	/* Enable all interrupts for each refill engine except
@@ -714,13 +714,13 @@ static int omap_dmm_probe(struct platform_device *dev)
 	if (!omap_dmm->dummy_page) {
 		dev_err(&dev->dev, "could not allocate dummy page\n");
 		ret = -ENOMEM;
-		goto fail;
+		goto check_dmm_removal;
 	}
 
 	/* set dma mask for device */
 	ret = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
 	if (ret)
-		goto fail;
+		goto check_dmm_removal;
 
 	omap_dmm->dummy_pa = page_to_phys(omap_dmm->dummy_page);
 
@@ -730,7 +730,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 					   &omap_dmm->refill_pa, GFP_KERNEL);
 	if (!omap_dmm->refill_va) {
 		dev_err(&dev->dev, "could not allocate refill memory\n");
-		goto fail;
+		goto check_dmm_removal;
 	}
 
 	/* alloc engines */
@@ -739,7 +739,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 				    GFP_KERNEL);
 	if (!omap_dmm->engines) {
 		ret = -ENOMEM;
-		goto fail;
+		goto check_dmm_removal;
 	}
 
 	for (i = 0; i < omap_dmm->num_engines; i++) {
@@ -758,7 +758,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 				GFP_KERNEL);
 	if (!omap_dmm->tcm) {
 		ret = -ENOMEM;
-		goto fail;
+		goto check_dmm_removal;
 	}
 
 	/* init containers */
@@ -772,7 +772,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 		if (!omap_dmm->tcm[i]) {
 			dev_err(&dev->dev, "failed to allocate container\n");
 			ret = -ENOMEM;
-			goto fail;
+			goto check_dmm_removal;
 		}
 
 		omap_dmm->tcm[i]->lut_id = i;
@@ -812,8 +812,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 	dev_info(omap_dmm->dev, "initialized all PAT entries\n");
 
 	return 0;
-
-fail:
+ check_dmm_removal:
 	if (omap_dmm_remove(dev))
 		dev_err(&dev->dev, "cleanup failed\n");
 	return ret;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/14] GPU-DRM-OMAP: Rename a jump label in dmm_txn_commit()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 17:32:42 +0200

Adjust a jump target so that redundant checks can be avoided at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 5f6f21b..c8ced158 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -269,7 +269,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 	if (!txn->last_pat) {
 		dev_err(engine->dmm->dev, "need at least one txn\n");
 		ret = -EINVAL;
-		goto cleanup;
+		goto release_engine;
 	}
 
 	txn->last_pat->next_pa = 0;
@@ -281,7 +281,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 	ret = wait_status(engine, DMM_PATSTATUS_READY);
 	if (ret) {
 		ret = -EFAULT;
-		goto cleanup;
+		goto release_engine;
 	}
 
 	/* mark whether it is async to denote list management in IRQ handler */
@@ -301,9 +301,9 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 		}
 	}
 
-cleanup:
 	/* only place engine back on list if we are done with it */
 	if (ret || wait)
+ release_engine:
 		release_engine(engine);
 
 	return ret;
-- 
2.10.0

[PATCH 09/14] GPU-DRM-OMAP: Delete an unnecessary variable initialisation in dmm_txn_commit()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 17:34:40 +0200

The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index c8ced158..c5c3793 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -262,7 +262,7 @@ static void dmm_txn_append(struct dmm_txn *txn, struct pat_area *area,
  */
 static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 {
-	int ret = 0;
+	int ret;
 	struct refill_engine *engine = txn->engine_handle;
 	struct dmm *dmm = engine->dmm;
 
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/14] GPU-DRM-OMAP: Use kmalloc_array() in omap_gem_attach_pages()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 17:37:04 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 505dee0..e4f1924 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -259,7 +259,7 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
 	 * DSS, GPU, etc. are not cache coherent:
 	 */
 	if (omap_obj->flags & (OMAP_BO_WC|OMAP_BO_UNCACHED)) {
-		addrs = kmalloc(npages * sizeof(*addrs), GFP_KERNEL);
+		addrs = kmalloc_array(npages, sizeof(*addrs), GFP_KERNEL);
 		if (!addrs) {
 			ret = -ENOMEM;
 			goto free_pages;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/14] GPU-DRM-OMAP: Replace a kzalloc() call by kcalloc() in omap_gem_attach_pages()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 17:40:20 +0200

The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kcalloc over kzalloc with multiply

Thus fix the affected source code place.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index e4f1924..26f1212 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -283,7 +283,7 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
 			}
 		}
 	} else {
-		addrs = kzalloc(npages * sizeof(*addrs), GFP_KERNEL);
+		addrs = kcalloc(npages, sizeof(*addrs), GFP_KERNEL);
 		if (!addrs) {
 			ret = -ENOMEM;
 			goto free_pages;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/14] GPU-DRM-OMAP: Move a variable assignment in omap_gem_attach_pages()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 17:42:28 +0200

Move one assignment for the local variable "npages" so that its setting
will only be performed after a call of the function "drm_gem_get_pages"
succeeded by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 26f1212..3c49ad9 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -243,7 +243,7 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
 	struct drm_device *dev = obj->dev;
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
 	struct page **pages;
-	int npages = obj->size >> PAGE_SHIFT;
+	int npages;
 	int i, ret;
 	dma_addr_t *addrs;
 
@@ -255,6 +255,8 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
 		return PTR_ERR(pages);
 	}
 
+	npages = obj->size >> PAGE_SHIFT;
+
 	/* for non-cached buffers, ensure the new pages are clean because
 	 * DSS, GPU, etc. are not cache coherent:
 	 */
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/14] GPU-DRM-OMAP: Rename a jump label in omap_gem_new_dmabuf()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 17:45:04 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 3c49ad9..92510de 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1442,7 +1442,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
 	obj = omap_gem_new(dev, gsize, OMAP_BO_MEM_DMABUF | OMAP_BO_WC);
 	if (!obj) {
 		obj = ERR_PTR(-ENOMEM);
-		goto done;
+		goto unlock;
 	}
 
 	omap_obj = to_omap_bo(obj);
@@ -1462,7 +1462,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
 		if (!pages) {
 			omap_gem_free_object(obj);
 			obj = ERR_PTR(-ENOMEM);
-			goto done;
+			goto unlock;
 		}
 
 		omap_obj->pages = pages;
@@ -1476,11 +1476,10 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
 		if (WARN_ON(i != npages)) {
 			omap_gem_free_object(obj);
 			obj = ERR_PTR(-ENOMEM);
-			goto done;
+			goto unlock;
 		}
 	}
-
-done:
+ unlock:
 	mutex_unlock(&dev->struct_mutex);
 	return obj;
 }
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/14] GPU-DRM-OMAP: Rename a jump label in four functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 21 Sep 2016 18:00:23 +0200

Adjust jump labels according to the current Linux coding style convention.
Thus replace the identifier "fail" by "unlock" for this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 92510de..ea7ad1c 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -549,7 +549,7 @@ int omap_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	/* if a shmem backed object, make sure we have pages attached now */
 	ret = get_pages(obj, &pages);
 	if (ret)
-		goto fail;
+		goto unlock;
 
 	/* where should we do corresponding put_pages().. we are mapping
 	 * the original page, rather than thru a GART, so we can't rely
@@ -561,9 +561,7 @@ int omap_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 		ret = fault_2d(obj, vma, vmf);
 	else
 		ret = fault_1d(obj, vma, vmf);
-
-
-fail:
+ unlock:
 	mutex_unlock(&dev->struct_mutex);
 	switch (ret) {
 	case 0:
@@ -682,14 +680,13 @@ int omap_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 	obj = drm_gem_object_lookup(file, handle);
 	if (obj == NULL) {
 		ret = -ENOENT;
-		goto fail;
+		goto unlock;
 	}
 
 	*offset = omap_gem_mmap_offset(obj);
 
 	drm_gem_object_unreference_unlocked(obj);
-
-fail:
+ unlock:
 	return ret;
 }
 
@@ -719,13 +716,12 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t roll)
 		struct page **pages;
 		ret = get_pages(obj, &pages);
 		if (ret)
-			goto fail;
+			goto unlock;
 		ret = tiler_pin(omap_obj->block, pages, npages, roll, true);
 		if (ret)
 			dev_err(obj->dev->dev, "could not repin: %d\n", ret);
 	}
-
-fail:
+ unlock:
 	mutex_unlock(&obj->dev->struct_mutex);
 
 	return ret;
@@ -825,7 +821,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 
 			ret = get_pages(obj, &pages);
 			if (ret)
-				goto fail;
+				goto unlock;
 
 			if (omap_obj->flags & OMAP_BO_TILED) {
 				block = tiler_reserve_2d(fmt,
@@ -839,7 +835,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 				ret = PTR_ERR(block);
 				dev_err(obj->dev->dev,
 					"could not remap: %d (%d)\n", ret, fmt);
-				goto fail;
+				goto unlock;
 			}
 
 			/* TODO: enable async refill.. */
@@ -849,7 +845,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 				tiler_release(block);
 				dev_err(obj->dev->dev,
 						"could not pin: %d\n", ret);
-				goto fail;
+				goto unlock;
 			}
 
 			omap_obj->paddr = tiler_ssptr(block);
@@ -865,10 +861,9 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 		*paddr = omap_obj->paddr;
 	} else {
 		ret = -EINVAL;
-		goto fail;
+		goto unlock;
 	}
-
-fail:
+ unlock:
 	mutex_unlock(&obj->dev->struct_mutex);
 
 	return ret;
-- 
2.10.0

Re: [PATCH 00/14] GPU-DRM-OMAP: Fine-tuning for several function implementations

[Daniel Vetter]

On Wed, Sep 21, 2016 at 06:35:59PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 21 Sep 2016 18:28:38 +0200
> 
> Several update suggestions were taken into account
> from static source code analysis.

For the next pile of driver patches _please_ talk with driver maintainers
before starting to create&submit patches. Like I said I won't take them,
and many of your changes are not clear-cut at all, so I expect many driver
maintaines also won't take them. Again, your contributions are welcome,
but blindly following suggestions from code checkers in drivers you cant
test isn't really all that useful. At the scale you're doing it, I think
it's mostly wasting everyone's time :( I'd like to avoid that.

Thanks, Daniel
> 
> Markus Elfring (14):
>   Use kmalloc_array() in tiler_map_show()
>   Replace another kmalloc() call by kmalloc_array() in tiler_map_show()
>   Less function calls in tiler_map_show() after error detection
>   Delete an unnecessary variable initialisation in tiler_map_show()
>   Improve a size determination in dmm_txn_append()
>   Improve a size determination in omap_dmm_probe()
>   Rename a jump label in omap_dmm_probe()
>   Rename a jump label in dmm_txn_commit()
>   Delete an unnecessary variable initialisation in dmm_txn_commit()
>   Use kmalloc_array() in omap_gem_attach_pages()
>   Replace a kzalloc() call by kcalloc() in omap_gem_attach_pages()
>   Move a variable assignment in omap_gem_attach_pages()
>   Rename a jump label in omap_gem_new_dmabuf()
>   Rename a jump label in four functions
> 
>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 58 ++++++++++++++++----------------
>  drivers/gpu/drm/omapdrm/omap_gem.c       | 44 +++++++++++-------------
>  2 files changed, 49 insertions(+), 53 deletions(-)
> 
> -- 
> 2.10.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 00/14] GPU-DRM-OMAP: Fine-tuning for several function implementations

[Laurent Pinchart]

On Thursday 22 Sep 2016 08:45:01 Daniel Vetter wrote:
> On Wed, Sep 21, 2016 at 06:35:59PM +0200, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 21 Sep 2016 18:28:38 +0200
> > 
> > Several update suggestions were taken into account
> > from static source code analysis.
> 
> For the next pile of driver patches _please_ talk with driver maintainers
> before starting to create&submit patches. Like I said I won't take them,
> and many of your changes are not clear-cut at all, so I expect many driver
> maintaines also won't take them. Again, your contributions are welcome,
> but blindly following suggestions from code checkers in drivers you cant
> test isn't really all that useful. At the scale you're doing it, I think
> it's mostly wasting everyone's time :( I'd like to avoid that.

I second that. After a very quick review, I see that the series splits related 
changes in multiple patches. I've already commented in reply to another series 
submitted by Markus that patches should then be combined. I will thus ignore 
this series completely for the time being.

> > Markus Elfring (14):
> >   Use kmalloc_array() in tiler_map_show()
> >   Replace another kmalloc() call by kmalloc_array() in tiler_map_show()
> >   Less function calls in tiler_map_show() after error detection
> >   Delete an unnecessary variable initialisation in tiler_map_show()
> >   Improve a size determination in dmm_txn_append()
> >   Improve a size determination in omap_dmm_probe()
> >   Rename a jump label in omap_dmm_probe()
> >   Rename a jump label in dmm_txn_commit()
> >   Delete an unnecessary variable initialisation in dmm_txn_commit()
> >   Use kmalloc_array() in omap_gem_attach_pages()
> >   Replace a kzalloc() call by kcalloc() in omap_gem_attach_pages()
> >   Move a variable assignment in omap_gem_attach_pages()
> >   Rename a jump label in omap_gem_new_dmabuf()
> >   Rename a jump label in four functions
> >  
> >  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 58 +++++++++++++--------------
> >  drivers/gpu/drm/omapdrm/omap_gem.c       | 44 +++++++++++-------------
> >  2 files changed, 49 insertions(+), 53 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 0/4] GPU-DRM-TILCDC: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 10:25:43 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Use kmalloc_array()
  Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()
  Less function calls in tilcdc_convert_slave_node() after error detection
  Delete unnecessary variable initialisations

 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 36 +++++++++++++++-------------
 1 file changed, 20 insertions(+), 16 deletions(-)

-- 
2.10.0

[PATCH 1/4] GPU-DRM-TILCDC: Use kmalloc_array() in kfree_table_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 09:05:14 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index f9c79da..8faa28f 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -32,8 +32,7 @@ static int __init kfree_table_init(struct kfree_table *kft)
 {
 	kft->total = 32;
 	kft->num = 0;
-	kft->table = kmalloc(kft->total * sizeof(*kft->table),
-			     GFP_KERNEL);
+	kft->table = kmalloc_array(kft->total, sizeof(*kft->table), GFP_KERNEL);
 	if (!kft->table)
 		return -ENOMEM;
 
-- 
2.10.0

[PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 09:29:23 +0200

Return directly after a memory allocation failed in this function
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 8faa28f..6204405 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -206,7 +206,7 @@ void __init tilcdc_convert_slave_node(void)
 	int ret;
 
 	if (kfree_table_init(&kft))
-		goto out;
+		return;
 
 	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
 	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
-- 
2.10.0

[PATCH 3/4] GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 10:06:50 +0200

The of_node_put() function was called in some cases
by the tilcdc_convert_slave_node() function during error handling
even if the passed variable contained a null pointer.

* Adjust jump targets according to the Linux coding style convention.

* Split a condition check for resource detection failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 6204405..6ee5865 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -209,25 +209,27 @@ void __init tilcdc_convert_slave_node(void)
 		return;
 
 	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
-	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
+	if (!of_device_is_available(lcdc))
+		goto free_table;
 
-	if (!slave || !of_device_is_available(lcdc))
-		goto out;
+	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
+	if (!slave)
+		goto put_node_lcdc;
 
 	i2c = of_parse_phandle(slave, "i2c", 0);
 	if (!i2c) {
 		pr_err("%s: Can't find i2c node trough phandle\n", __func__);
-		goto out;
+		goto put_node_slave;
 	}
 
 	overlay = tilcdc_get_overlay(&kft);
 	if (!overlay)
-		goto out;
+		goto put_node_i2c;
 
 	encoder = of_find_matching_node(overlay, tilcdc_tda998x_of_match);
 	if (!encoder) {
 		pr_err("%s: Failed to find tda998x node\n", __func__);
-		goto out;
+		goto put_node_i2c;
 	}
 
 	tilcdc_copy_props(slave, encoder, tilcdc_slave_props, &kft);
@@ -238,10 +240,10 @@ void __init tilcdc_convert_slave_node(void)
 			continue;
 		if (!strncmp("i2c", (char *)prop->value, prop->length))
 			if (tilcdc_prop_str_update(prop, i2c->full_name, &kft))
-				goto out;
+				goto put_node_fragment;
 		if (!strncmp("lcdc", (char *)prop->value, prop->length))
 			if (tilcdc_prop_str_update(prop, lcdc->full_name, &kft))
-				goto out;
+				goto put_node_fragment;
 	}
 
 	tilcdc_node_disable(slave);
@@ -252,12 +254,16 @@ void __init tilcdc_convert_slave_node(void)
 	else
 		pr_info("%s: ti,tilcdc,slave node successfully converted\n",
 			__func__);
-out:
-	kfree_table_free(&kft);
+ put_node_fragment:
+	of_node_put(fragment);
+ put_node_i2c:
 	of_node_put(i2c);
+ put_node_slave:
 	of_node_put(slave);
+ put_node_lcdc:
 	of_node_put(lcdc);
-	of_node_put(fragment);
+ free_table:
+	kfree_table_free(&kft);
 }
 
 int __init tilcdc_slave_compat_init(void)
-- 
2.10.0

[PATCH 4/4] GPU-DRM-TILCDC: Delete unnecessary variable initialisations in tilcdc_convert_slave_node()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 10:15:36 +0200

Four local variables will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation which became unnecessary with
a previous update step.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
index 6ee5865..ae90728 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
@@ -196,8 +196,7 @@ static const char * const tilcdc_slave_props[] __initconst = {
 
 void __init tilcdc_convert_slave_node(void)
 {
-	struct device_node *slave = NULL, *lcdc = NULL;
-	struct device_node *i2c = NULL, *fragment = NULL;
+	struct device_node *slave, *lcdc, *i2c, *fragment;
 	struct device_node *overlay, *encoder;
 	struct property *prop;
 	/* For all memory needed for the overlay tree. This memory can
-- 
2.10.0

Re: GPU-DRM-OMAP: Fine-tuning for several function implementations

[SF Markus Elfring]

>> For the next pile of driver patches _please_ talk with driver maintainers
>> before starting to create&submit patches.

Did the software development discussion start a bit here?

Would you like to support an other "talking style" on a conference
like in Berlin next month?


>> Like I said I won't take them,

It's a pity.


>> and many of your changes are not clear-cut at all,

I know that specific update suggestions could be interpreted as controversial.


>> so I expect many driver maintaines also won't take them.

I am curious on useful responses.


>> Again, your contributions are welcome,

Thanks for another bit of constructive feedback.


>> but blindly following suggestions from code checkers in drivers

I propose to dare another look at corresponding information sources.


>> you cant test isn't really all that useful.

I have got an other impression.

How many improvements can still be achieved by usual (advanced) collaboration
techniques for free software development?


>> At the scale you're doing it, I think it's mostly wasting everyone's time

I hope not.


> :( I'd like to avoid that.

I am going to point more update opportunities out also for various Linux software.


> I second that.

Thanks for your opinion on this issue.


> After a very quick review, I see that the series splits related changes
> in multiple patches.

I chose a specific patch granularity for this proposal.


> I've already commented in reply to another series submitted by Markus
> that patches should then be combined.

Will such a combination depend on any more agreements between the involved contributors?


> I will thus ignore this series completely for the time being.

I hope that you can give similar ideas a second chance somehow.

Regards,
Markus

Re: [PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()

[Dan Carpenter]

This one is actually a bug fix...  But finding bug fixes in this series
is like looking for kernels of edible corn in piles of monkey poop.

Also, classic "One Err" bug.

regards,
dan carpenter

Re: [PATCH 1/4] GPU-DRM-TILCDC: Use kmalloc_array() in kfree_table_init()

[Jyri Sarha]

On 09/22/16 11:31, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 22 Sep 2016 09:05:14 +0200
> 
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> index f9c79da..8faa28f 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> @@ -32,8 +32,7 @@ static int __init kfree_table_init(struct kfree_table *kft)
>  {
>  	kft->total = 32;
>  	kft->num = 0;
> -	kft->table = kmalloc(kft->total * sizeof(*kft->table),
> -			     GFP_KERNEL);
> +	kft->table = kmalloc_array(kft->total, sizeof(*kft->table), GFP_KERNEL);

I was not sure if it is Ok to call kremalloc() for a pointer that was
previously allocated with kmalloc_array(). And at least it felt pointless.

But if you can confirm that it is ok, then sure I can take the patch.

Thanks,
Jyri

>  	if (!kft->table)
>  		return -ENOMEM;
>  
> 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] GPU-DRM-TILCDC: Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()

[Jyri Sarha]

On 09/22/16 11:32, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 22 Sep 2016 09:29:23 +0200
> 
> Return directly after a memory allocation failed in this function
> at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> index 8faa28f..6204405 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> @@ -206,7 +206,7 @@ void __init tilcdc_convert_slave_node(void)
>  	int ret;
>  
>  	if (kfree_table_init(&kft))
> -		goto out;
> +		return;
>  
>  	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
>  	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
> 

Thanks,
This is a real bug. I'll pick this up, but with your permission I change
the commit subject to follow the current convention.

Best regards,
Jyri

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Jyri Sarha]

On 09/22/16 11:33, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 22 Sep 2016 10:06:50 +0200
> 
> The of_node_put() function was called in some cases
> by the tilcdc_convert_slave_node() function during error handling
> even if the passed variable contained a null pointer.
> 
> * Adjust jump targets according to the Linux coding style convention.
> 
> * Split a condition check for resource detection failures so that
>   each pointer from these function calls will be checked immediately.
> 
>   See also background information:
>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>   Link: https://cwe.mitre.org/data/definitions/754.html
> 

I don't really agree with this patch. There is no harm in calling
of_node_put() with NULL as an argument and because of that there is no
point in making the function more complex and harder to maintain.

Best regards,
Jyri

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> index 6204405..6ee5865 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c
> @@ -209,25 +209,27 @@ void __init tilcdc_convert_slave_node(void)
>  		return;
>  
>  	lcdc = of_find_matching_node(NULL, tilcdc_of_match);
> -	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
> +	if (!of_device_is_available(lcdc))
> +		goto free_table;
>  
> -	if (!slave || !of_device_is_available(lcdc))
> -		goto out;
> +	slave = of_find_matching_node(NULL, tilcdc_slave_of_match);
> +	if (!slave)
> +		goto put_node_lcdc;
>  
>  	i2c = of_parse_phandle(slave, "i2c", 0);
>  	if (!i2c) {
>  		pr_err("%s: Can't find i2c node trough phandle\n", __func__);
> -		goto out;
> +		goto put_node_slave;
>  	}
>  
>  	overlay = tilcdc_get_overlay(&kft);
>  	if (!overlay)
> -		goto out;
> +		goto put_node_i2c;
>  
>  	encoder = of_find_matching_node(overlay, tilcdc_tda998x_of_match);
>  	if (!encoder) {
>  		pr_err("%s: Failed to find tda998x node\n", __func__);
> -		goto out;
> +		goto put_node_i2c;
>  	}
>  
>  	tilcdc_copy_props(slave, encoder, tilcdc_slave_props, &kft);
> @@ -238,10 +240,10 @@ void __init tilcdc_convert_slave_node(void)
>  			continue;
>  		if (!strncmp("i2c", (char *)prop->value, prop->length))
>  			if (tilcdc_prop_str_update(prop, i2c->full_name, &kft))
> -				goto out;
> +				goto put_node_fragment;
>  		if (!strncmp("lcdc", (char *)prop->value, prop->length))
>  			if (tilcdc_prop_str_update(prop, lcdc->full_name, &kft))
> -				goto out;
> +				goto put_node_fragment;
>  	}
>  
>  	tilcdc_node_disable(slave);
> @@ -252,12 +254,16 @@ void __init tilcdc_convert_slave_node(void)
>  	else
>  		pr_info("%s: ti,tilcdc,slave node successfully converted\n",
>  			__func__);
> -out:
> -	kfree_table_free(&kft);
> + put_node_fragment:
> +	of_node_put(fragment);
> + put_node_i2c:
>  	of_node_put(i2c);
> + put_node_slave:
>  	of_node_put(slave);
> + put_node_lcdc:
>  	of_node_put(lcdc);
> -	of_node_put(fragment);
> + free_table:
> +	kfree_table_free(&kft);
>  }
>  
>  int __init tilcdc_slave_compat_init(void)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 00/14] GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 19:00:01 +0200

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (14):
  Use kmalloc_array() in two functions
  Rename a jump label in ttm_alloc_new_pages()
  Rename jump labels in ttm_page_pool_free()
  Rename a jump label in ttm_page_pool_get_pages()
  Use kmalloc_array() in two more functions
  Rename a jump label in ttm_dma_pool_alloc_new_pages()
  Rename jump labels in ttm_dma_page_pool_free()
  Rename a jump label in ttm_dma_pool_shrink_scan()
  Return directly after a failed kzalloc() in ttm_dma_page_alloc_init()
  Return directly after a failed kobject_init_and_add() in ttm_dma_page_alloc_init()
  Return an error code only as a constant in ttm_dma_pool_init()
  Less function calls in ttm_dma_pool_init() after error detection
  Delete unnecessary variable initialisations in ttm_dma_pool_init()
  Mark an array of text strings as "const" in ttm_dma_pool_init()

 drivers/gpu/drm/ttm/ttm_page_alloc.c     | 30 ++++++++---------
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 58 +++++++++++++++-----------------
 2 files changed, 42 insertions(+), 46 deletions(-)

-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/14] GPU-DRM-TTM: Use kmalloc_array() in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 14:00:31 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of data types by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index a37de5d..bfc51cb 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -315,8 +315,9 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, unsigned nr_free,
 	if (use_static)
 		pages_to_free = static_buf;
 	else
-		pages_to_free = kmalloc(npages_to_free * sizeof(struct page *),
-					GFP_KERNEL);
+		pages_to_free = kmalloc_array(npages_to_free,
+					      sizeof(*pages_to_free),
+					      GFP_KERNEL);
 	if (!pages_to_free) {
 		pr_err("Failed to allocate memory for pool free operation\n");
 		return 0;
@@ -501,8 +502,9 @@ static int ttm_alloc_new_pages(struct list_head *pages, gfp_t gfp_flags,
 			(unsigned)(PAGE_SIZE/sizeof(struct page *)));
 
 	/* allocate array for page caching change */
-	caching_array = kmalloc(max_cpages*sizeof(struct page *), GFP_KERNEL);
-
+	caching_array = kmalloc_array(max_cpages,
+				      sizeof(*caching_array),
+				      GFP_KERNEL);
 	if (!caching_array) {
 		pr_err("Unable to allocate table for new pages\n");
 		return -ENOMEM;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/14] GPU-DRM-TTM: Rename a jump label in ttm_alloc_new_pages()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 14:16:05 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index bfc51cb..13fdd19 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -527,7 +527,7 @@ static int ttm_alloc_new_pages(struct list_head *pages, gfp_t gfp_flags,
 						caching_array, cpages);
 			}
 			r = -ENOMEM;
-			goto out;
+			goto free_array;
 		}
 
 #ifdef CONFIG_HIGHMEM
@@ -546,7 +546,7 @@ static int ttm_alloc_new_pages(struct list_head *pages, gfp_t gfp_flags,
 					ttm_handle_caching_state_failure(pages,
 						ttm_flags, cstate,
 						caching_array, cpages);
-					goto out;
+					goto free_array;
 				}
 				cpages = 0;
 			}
@@ -562,7 +562,7 @@ static int ttm_alloc_new_pages(struct list_head *pages, gfp_t gfp_flags,
 					ttm_flags, cstate,
 					caching_array, cpages);
 	}
-out:
+ free_array:
 	kfree(caching_array);
 
 	return r;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/14] GPU-DRM-TTM: Rename jump labels in ttm_page_pool_free()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 14:30:12 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index 13fdd19..f33f6f6 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -322,8 +322,7 @@ static int ttm_page_pool_free(struct ttm_page_pool *pool, unsigned nr_free,
 		pr_err("Failed to allocate memory for pool free operation\n");
 		return 0;
 	}
-
-restart:
+ lock_restart:
 	spin_lock_irqsave(&pool->lock, irq_flags);
 
 	list_for_each_entry_reverse(p, &pool->list, lru) {
@@ -356,14 +355,13 @@ restart:
 
 			/* free all so restart the processing */
 			if (nr_free)
-				goto restart;
+				goto lock_restart;
 
 			/* Not allowed to fall through or break because
 			 * following context is inside spinlock while we are
 			 * outside here.
 			 */
-			goto out;
-
+			goto check_pages_to_free;
 		}
 	}
 
@@ -379,7 +377,7 @@ restart:
 
 	if (freed_pages)
 		ttm_pages_put(pages_to_free, freed_pages);
-out:
+ check_pages_to_free:
 	if (pages_to_free != static_buf)
 		kfree(pages_to_free);
 	return nr_free;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/14] GPU-DRM-TTM: Rename a jump label in ttm_page_pool_get_pages()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 14:36:47 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
index f33f6f6..3dd603f 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
@@ -646,7 +646,7 @@ static unsigned ttm_page_pool_get_pages(struct ttm_page_pool *pool,
 		list_splice_init(&pool->list, pages);
 		count -= pool->npages;
 		pool->npages = 0;
-		goto out;
+		goto unlock;
 	}
 	/* find the last pages to include for requested number of pages. Split
 	 * pool to begin and halve it to reduce search space. */
@@ -667,7 +667,7 @@ static unsigned ttm_page_pool_get_pages(struct ttm_page_pool *pool,
 	list_cut_position(pages, &pool->list, p);
 	pool->npages -= count;
 	count = 0;
-out:
+ unlock:
 	spin_unlock_irqrestore(&pool->lock, irq_flags);
 	return count;
 }
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/14] GPU-DRM-TTM: Use kmalloc_array() in two more functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 14:48:39 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of data types by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index bef9f6f..194818d 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -439,8 +439,9 @@ static unsigned ttm_dma_page_pool_free(struct dma_pool *pool, unsigned nr_free,
 	if (use_static)
 		pages_to_free = static_buf;
 	else
-		pages_to_free = kmalloc(npages_to_free * sizeof(struct page *),
-					GFP_KERNEL);
+		pages_to_free = kmalloc_array(npages_to_free,
+					      sizeof(*pages_to_free),
+					      GFP_KERNEL);
 
 	if (!pages_to_free) {
 		pr_err("%s: Failed to allocate memory for pool free operation\n",
@@ -726,8 +727,9 @@ static int ttm_dma_pool_alloc_new_pages(struct dma_pool *pool,
 			(unsigned)(PAGE_SIZE/sizeof(struct page *)));
 
 	/* allocate array for page caching change */
-	caching_array = kmalloc(max_cpages*sizeof(struct page *), GFP_KERNEL);
-
+	caching_array = kmalloc_array(max_cpages,
+				      sizeof(*caching_array),
+				      GFP_KERNEL);
 	if (!caching_array) {
 		pr_err("%s: Unable to allocate table for new pages\n",
 		       pool->dev_name);
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/14] GPU-DRM-TTM: Rename a jump label in ttm_dma_pool_alloc_new_pages()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 14:54:12 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index 194818d..9dc1632 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -758,7 +758,7 @@ static int ttm_dma_pool_alloc_new_pages(struct dma_pool *pool,
 						cpages);
 			}
 			r = -ENOMEM;
-			goto out;
+			goto free_array;
 		}
 		p = dma_p->p;
 #ifdef CONFIG_HIGHMEM
@@ -777,7 +777,7 @@ static int ttm_dma_pool_alloc_new_pages(struct dma_pool *pool,
 					ttm_dma_handle_caching_state_failure(
 						pool, d_pages, caching_array,
 						cpages);
-					goto out;
+					goto free_array;
 				}
 				cpages = 0;
 			}
@@ -791,7 +791,7 @@ static int ttm_dma_pool_alloc_new_pages(struct dma_pool *pool,
 			ttm_dma_handle_caching_state_failure(pool, d_pages,
 					caching_array, cpages);
 	}
-out:
+ free_array:
 	kfree(caching_array);
 	return r;
 }
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/14] GPU-DRM-TTM: Rename jump labels in ttm_dma_page_pool_free()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 15:32:32 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index 9dc1632..ce3d361 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -449,7 +449,7 @@ static unsigned ttm_dma_page_pool_free(struct dma_pool *pool, unsigned nr_free,
 		return 0;
 	}
 	INIT_LIST_HEAD(&d_pages);
-restart:
+ lock_restart:
 	spin_lock_irqsave(&pool->lock, irq_flags);
 
 	/* We picking the oldest ones off the list */
@@ -489,14 +489,13 @@ restart:
 
 			/* free all so restart the processing */
 			if (nr_free)
-				goto restart;
+				goto lock_restart;
 
 			/* Not allowed to fall through or break because
 			 * following context is inside spinlock while we are
 			 * outside here.
 			 */
-			goto out;
-
+			goto check_pages_to_free;
 		}
 	}
 
@@ -510,7 +509,7 @@ restart:
 
 	if (freed_pages)
 		ttm_dma_pages_put(pool, &d_pages, pages_to_free, freed_pages);
-out:
+ check_pages_to_free:
 	if (pages_to_free != static_buf)
 		kfree(pages_to_free);
 	return nr_free;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/14] GPU-DRM-TTM: Rename a jump label in ttm_dma_pool_shrink_scan()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 16:02:36 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index ce3d361..e3f5542 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -1025,7 +1025,7 @@ ttm_dma_pool_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
 	if (!mutex_trylock(&_manager->lock))
 		return SHRINK_STOP;
 	if (!_manager->npools)
-		goto out;
+		goto unlock;
 	pool_offset = ++start_pool % _manager->npools;
 	list_for_each_entry(p, &_manager->pools, pools) {
 		unsigned nr_free;
@@ -1046,7 +1046,7 @@ ttm_dma_pool_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
 			 p->pool->dev_name, p->pool->name, current->pid,
 			 nr_free, shrink_pages);
 	}
-out:
+ unlock:
 	mutex_unlock(&_manager->lock);
 	return freed;
 }
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/14] GPU-DRM-TTM: Return directly after a failed kzalloc() in ttm_dma_page_alloc_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 16:16:36 +0200

* Return directly after a memory allocation failed in this function
  at the beginning.

* Delete the explicit initialisation for the local variable "ret"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index e3f5542..feba278 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -1080,7 +1080,7 @@ static void ttm_dma_pool_mm_shrink_fini(struct ttm_pool_manager *manager)
 
 int ttm_dma_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages)
 {
-	int ret = -ENOMEM;
+	int ret;
 
 	WARN_ON(_manager);
 
@@ -1088,7 +1088,7 @@ int ttm_dma_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages)
 
 	_manager = kzalloc(sizeof(*_manager), GFP_KERNEL);
 	if (!_manager)
-		goto err;
+		return -ENOMEM;
 
 	mutex_init(&_manager->lock);
 	INIT_LIST_HEAD(&_manager->pools);
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/14] GPU-DRM-TTM: Return directly after a failed kobject_init_and_add() in ttm_dma_page_alloc_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 16:24:43 +0200

* Return directly after a call of the function "kobject_init_and_add"
  failed here.

* Delete the jump target "err" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index feba278..c21f45f 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -1102,12 +1102,10 @@ int ttm_dma_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages)
 				   &glob->kobj, "dma_pool");
 	if (unlikely(ret != 0)) {
 		kobject_put(&_manager->kobj);
-		goto err;
+		return ret;
 	}
 	ttm_dma_pool_mm_shrink_init(_manager);
 	return 0;
-err:
-	return ret;
 }
 
 void ttm_dma_page_alloc_fini(void)
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/14] GPU-DRM-TTM: Return an error code only as a constant in ttm_dma_pool_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 17:17:19 +0200

* Return an error code without storing it in a local variable.

* Delete the local variable "ret" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index c21f45f..d5f41ed 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -579,7 +579,6 @@ static struct dma_pool *ttm_dma_pool_init(struct device *dev, gfp_t flags,
 	struct device_pools *sec_pool = NULL;
 	struct dma_pool *pool = NULL, **ptr;
 	unsigned i;
-	int ret = -ENODEV;
 	char *p;
 
 	if (!dev)
@@ -589,8 +588,6 @@ static struct dma_pool *ttm_dma_pool_init(struct device *dev, gfp_t flags,
 	if (!ptr)
 		return NULL;
 
-	ret = -ENOMEM;
-
 	pool = kmalloc_node(sizeof(struct dma_pool), GFP_KERNEL,
 			    dev_to_node(dev));
 	if (!pool)
@@ -644,7 +641,7 @@ err_mem:
 	devres_free(ptr);
 	kfree(sec_pool);
 	kfree(pool);
-	return ERR_PTR(ret);
+	return ERR_PTR(-ENOMEM);
 }
 
 static struct dma_pool *ttm_dma_find_pool(struct device *dev,
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/14] GPU-DRM-TTM: Less function calls in ttm_dma_pool_init() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 17:30:52 +0200

The kfree() function was called in up to two cases
by the ttm_dma_pool_init() function during error handling
even if the passed variable contained a null pointer.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index d5f41ed..4c50196 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -591,12 +591,12 @@ static struct dma_pool *ttm_dma_pool_init(struct device *dev, gfp_t flags,
 	pool = kmalloc_node(sizeof(struct dma_pool), GFP_KERNEL,
 			    dev_to_node(dev));
 	if (!pool)
-		goto err_mem;
+		goto free_devres;
 
 	sec_pool = kmalloc_node(sizeof(struct device_pools), GFP_KERNEL,
 				dev_to_node(dev));
 	if (!sec_pool)
-		goto err_mem;
+		goto free_pool;
 
 	INIT_LIST_HEAD(&sec_pool->pools);
 	sec_pool->dev = dev;
@@ -637,10 +637,10 @@ static struct dma_pool *ttm_dma_pool_init(struct device *dev, gfp_t flags,
 	devres_add(dev, ptr);
 
 	return pool;
-err_mem:
-	devres_free(ptr);
-	kfree(sec_pool);
+ free_pool:
 	kfree(pool);
+ free_devres:
+	devres_free(ptr);
 	return ERR_PTR(-ENOMEM);
 }
 
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/14] GPU-DRM-TTM: Delete unnecessary variable initialisations in ttm_dma_pool_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 17:37:24 +0200

Two local variables will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation which became unnecessary with
a previous update step.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index 4c50196..aa1679d 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -576,8 +576,8 @@ static struct dma_pool *ttm_dma_pool_init(struct device *dev, gfp_t flags,
 {
 	char *n[] = {"wc", "uc", "cached", " dma32", "unknown",};
 	enum pool_type t[] = {IS_WC, IS_UC, IS_CACHED, IS_DMA32, IS_UNDEFINED};
-	struct device_pools *sec_pool = NULL;
-	struct dma_pool *pool = NULL, **ptr;
+	struct device_pools *sec_pool;
+	struct dma_pool *pool, **ptr;
 	unsigned i;
 	char *p;
 
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/14] GPU-DRM-TTM: Mark an array of text strings as "const" in ttm_dma_pool_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 22 Sep 2016 18:46:01 +0200

The local variable "n" was not modified after it was initialized with
a few text strings.
Thus express this detail also by the data type qualifier "const".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index aa1679d..e21c1b3 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -574,7 +574,7 @@ static int ttm_dma_pool_match(struct device *dev, void *res, void *match_data)
 static struct dma_pool *ttm_dma_pool_init(struct device *dev, gfp_t flags,
 					  enum pool_type type)
 {
-	char *n[] = {"wc", "uc", "cached", " dma32", "unknown",};
+	char const * const n[] = {"wc", "uc", "cached", " dma32", "unknown",};
 	enum pool_type t[] = {IS_WC, IS_UC, IS_CACHED, IS_DMA32, IS_UNDEFINED};
 	struct device_pools *sec_pool;
 	struct dma_pool *pool, **ptr;
-- 
2.10.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Return directly after a failed kfree_table_init() in tilcdc_convert_slave_node()

[SF Markus Elfring]

> I'll pick this up, but with your permission I change
> the commit subject to follow the current convention.

I guess that I can accept the integration of this update suggestion
also with an adjusted prefix in the commit title.   ;-)

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

>> The of_node_put() function was called in some cases
>> by the tilcdc_convert_slave_node() function during error handling
>> even if the passed variable contained a null pointer.
>>
>> * Adjust jump targets according to the Linux coding style convention.
>>
>> * Split a condition check for resource detection failures so that
>>   each pointer from these function calls will be checked immediately.
>>
>>   See also background information:
>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>
> 
> I don't really agree with this patch.

This kind of feedback can be fine at first glance.


> There is no harm in calling of_node_put() with NULL as an argument

The cost of additional function calls will be eventually not noticed
just because they belong to an exception handling implementation so far.


> and because of that there is no point in making the function more complex

There is inherent software complexity involved.


> and harder to maintain.

How do you think about to discuss this aspect a bit more?


I suggest to reconsider this design detail if it is really acceptable
for the safe implementation of such a software module.

* How much will it matter in general that one function call was performed
  in this use case without checking its return value immediately?

* Should it usually be determined quicker if a required resource
  could be acquired before trying the next allocation?

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/14] GPU-DRM-TTM: Mark an array of text strings as "const" in ttm_dma_pool_init()

[Joe Perches]

On Thu, 2016-09-22 at 19:46 +0200, SF Markus Elfring wrote:
> The local variable "n" was not modified after it was initialized with
> a few text strings.
> Thus express this detail also by the data type qualifier "const".
[]
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
[]
> @@ -574,7 +574,7 @@ static int ttm_dma_pool_match(struct device *dev, void *res, void *match_data)
>  static struct dma_pool *ttm_dma_pool_init(struct device *dev, gfp_t flags,
>  					  enum pool_type type)
>  {
> -	char *n[] = {"wc", "uc", "cached", " dma32", "unknown",};
> +	char const * const n[] = {"wc", "uc", "cached", " dma32", "unknown",};
>  	enum pool_type t[] = {IS_WC, IS_UC, IS_CACHED, IS_DMA32, IS_UNDEFINED};


Please think a little deeper about what you are changing here
and look at the line immediately below it too.

Both should be static const to avoid unnecessary reload.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Jyri Sarha]

On 09/22/16 21:38, SF Markus Elfring wrote:
>>> The of_node_put() function was called in some cases
>>> by the tilcdc_convert_slave_node() function during error handling
>>> even if the passed variable contained a null pointer.
>>>
>>> * Adjust jump targets according to the Linux coding style convention.
>>>
>>> * Split a condition check for resource detection failures so that
>>>   each pointer from these function calls will be checked immediately.
>>>
>>>   See also background information:
>>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>>
>>
>> I don't really agree with this patch.
> 
> This kind of feedback can be fine at first glance.
> 
> 
>> There is no harm in calling of_node_put() with NULL as an argument
> 
> The cost of additional function calls will be eventually not noticed
> just because they belong to an exception handling implementation so far.
> 
> 
>> and because of that there is no point in making the function more complex
> 
> There is inherent software complexity involved.
> 

I think the "if (node)" in the of_node_put() is there on purpose,
because it potentially saves the caller one extra if()-statement and
keeps the caller code simpler.

> 
>> and harder to maintain.
> 
> How do you think about to discuss this aspect a bit more?
> 

Keeping the goto labels in right order needs precision and can lead to
subtle errors. Sometimes there is no way to avoid that, but here there is.

Best regards,
Jyri
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

> I think the "if (node)" in the of_node_put() is there on purpose,

Yes, of course.

Does such an implementation detail correspond to a general software design pattern?


> because it potentially saves the caller one extra if()-statement

This can occasionally happen.


> and keeps the caller code simpler.

A special view on software simplicity can also lead to questionable intermediate
function implementation, can't it?


> Keeping the goto labels in right order needs precision

I can agree to this view.


> and can lead to subtle errors.

The management of jump labels is just another software development challenge
as usual, isn't it?


> Sometimes there is no way to avoid that,

How do you think about to clarify the constraints which you imagine a bit more?


> but here there is.

I disagree to this conclusion.

Would you like to care a bit more for efficiency and software correctness
around the discussed exception handling?

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/14] GPU-DRM-TTM: Fine-tuning for several function implementations

[Christian König]

First of all please stop sending your patches as a reply to an earlier 
and completely unrelated series.

Second please prefix all TTM related patches with "drm/ttm:".

Additional to that I don't really see the point in renaming some of the 
jump labels, if you call it "restart" or "lock_restart" doesn't make 
much difference.

Regards,
Christian.

Am 22.09.2016 um 19:32 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 22 Sep 2016 19:00:01 +0200
>
> Several update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (14):
>    Use kmalloc_array() in two functions
>    Rename a jump label in ttm_alloc_new_pages()
>    Rename jump labels in ttm_page_pool_free()
>    Rename a jump label in ttm_page_pool_get_pages()
>    Use kmalloc_array() in two more functions
>    Rename a jump label in ttm_dma_pool_alloc_new_pages()
>    Rename jump labels in ttm_dma_page_pool_free()
>    Rename a jump label in ttm_dma_pool_shrink_scan()
>    Return directly after a failed kzalloc() in ttm_dma_page_alloc_init()
>    Return directly after a failed kobject_init_and_add() in ttm_dma_page_alloc_init()
>    Return an error code only as a constant in ttm_dma_pool_init()
>    Less function calls in ttm_dma_pool_init() after error detection
>    Delete unnecessary variable initialisations in ttm_dma_pool_init()
>    Mark an array of text strings as "const" in ttm_dma_pool_init()
>
>   drivers/gpu/drm/ttm/ttm_page_alloc.c     | 30 ++++++++---------
>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 58 +++++++++++++++-----------------
>   2 files changed, 42 insertions(+), 46 deletions(-)
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

> Additional to that I don't really see the point in renaming some of the jump labels,

I am suggesting changes for another collateral software evolution.


> if you call it "restart" or "lock_restart" doesn't make much difference.

Do other identifiers fit better to a specification from the document "CodingStyle"
like the following?

"…
Choose label names which say what the goto does or why the goto exists.
…"


Does this wording need any more adjustments?

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Jyri Sarha]

On 09/23/16 10:36, SF Markus Elfring wrote:
>> I think the "if (node)" in the of_node_put() is there on purpose,
> 
> Yes, of course.
> 
> Does such an implementation detail correspond to a general software design pattern?
> 

Yes it does. For instance standard malloc()/free() implementation [1].

> 
>> because it potentially saves the caller one extra if()-statement
> 
> This can occasionally happen.
> 
> 
>> and keeps the caller code simpler.
> 
> A special view on software simplicity can also lead to questionable intermediate
> function implementation, can't it?
> 

I don't really follow. But in any case I do not see anything
questionable in the current tilcdc_convert_slave_node() implementation.

> 
>> Keeping the goto labels in right order needs precision
> 
> I can agree to this view.
> 
> 
>> and can lead to subtle errors.
> 
> The management of jump labels is just another software development challenge
> as usual, isn't it?
> 

Yes. But usually it pays of to avoid complexity when possible.

> 
>> Sometimes there is no way to avoid that,
> 
> How do you think about to clarify the constraints which you imagine a bit more?
> 

If the the of_node_put() behaviour would not be specified with null
pointer as parameter, there would be such a constraint.

I am beginning to have a feeling that this discussion is not going anywhere.

> 
>> but here there is.
> 
> I disagree to this conclusion.
> 
> Would you like to care a bit more for efficiency and software correctness
> around the discussed exception handling?
> 

No, I would not. I think we have reached the bottom of this discussion.
For the moment I have more important tasks to do.

Best regards,
Jyri

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TTM: Fine-tuning for several function implementations

[Christian König]

Am 23.09.2016 um 12:20 schrieb SF Markus Elfring:
>> Additional to that I don't really see the point in renaming some of the jump labels,
> I am suggesting changes for another collateral software evolution.
>
>
>> if you call it "restart" or "lock_restart" doesn't make much difference.
> Do other identifiers fit better to a specification from the document "CodingStyle"
> like the following?

No, not really.

>
> "…
> Choose label names which say what the goto does or why the goto exists.
> …"
>
>
> Does this wording need any more adjustments?

Of hand I can't find any better wording.

It's just the names like "out" or "restart" perfectly explain why the 
labels exists. So they fulfill this requirement from the coding style as 
far as I can see.

So why do you want to change them?

Regards,
Christian.

>
> Regards,
> Markus


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

>> A special view on software simplicity can also lead to questionable intermediate
>> function implementation, can't it?
> 
> I don't really follow. But in any case I do not see anything
> questionable in the current tilcdc_convert_slave_node() implementation.

I identified update candidates there like the following.

1. Delayed checking for null pointers
…
	if (!slave || !of_device_is_available(lcdc))
…

2. Usage of a single jump label for (too many?) cases
…
		goto out;
…

   Can the corresponding exception handling become also a bit more efficient?


>> Would you like to care a bit more for efficiency and software correctness
>> around the discussed exception handling?
> 
> No, I would not.

Thanks for this information.

I hope that the software situation can also be improved around
this design aspect somehow.


> For the moment I have more important tasks to do.

I know also that various open issues are competing for your software
development attention as usual.

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Rob Clark]

On Thu, Sep 22, 2016 at 2:38 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> The of_node_put() function was called in some cases
>>> by the tilcdc_convert_slave_node() function during error handling
>>> even if the passed variable contained a null pointer.
>>>
>>> * Adjust jump targets according to the Linux coding style convention.
>>>
>>> * Split a condition check for resource detection failures so that
>>>   each pointer from these function calls will be checked immediately.
>>>
>>>   See also background information:
>>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>>
>>
>> I don't really agree with this patch.
>
> This kind of feedback can be fine at first glance.
>
>
>> There is no harm in calling of_node_put() with NULL as an argument
>
> The cost of additional function calls will be eventually not noticed
> just because they belong to an exception handling implementation so far.

iirc, there are Coccinelle rules that find code with unnecessary null
checks and removes them..

Although you probably made this complex enough that cocinelle would
not find it.  That is not a complement.  One should not make error
handling/cleanup more complex than needed.

BR,
-R

>
>> and because of that there is no point in making the function more complex
>
> There is inherent software complexity involved.
>
>
>> and harder to maintain.
>
> How do you think about to discuss this aspect a bit more?
>
>
> I suggest to reconsider this design detail if it is really acceptable
> for the safe implementation of such a software module.
>
> * How much will it matter in general that one function call was performed
>   in this use case without checking its return value immediately?
>
> * Should it usually be determined quicker if a required resource
>   could be acquired before trying the next allocation?
>
> Regards,
> Markus
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

> It's just the names like "out" or "restart" perfectly explain why the labels exists.

I have got an other impression.


> So they fulfill this requirement from the coding style as far as I can see.

Short identifiers might look more convenient in some cases because
they are quicker to type.


> So why do you want to change them?

1. I suggest to select identifiers also for jump labels which are more meaningful
   and eventually unique for some function implementations.

2. How do you think about to add a single space character before any label?

Regards,
Markus

Re: GPU-DRM-TTM: Fine-tuning for several function implementations

[Christian König]

Am 23.09.2016 um 13:07 schrieb SF Markus Elfring:
>> It's just the names like "out" or "restart" perfectly explain why the labels exists.
> I have got an other impression.
>
>
>> So they fulfill this requirement from the coding style as far as I can see.
> Short identifiers might look more convenient in some cases because
> they are quicker to type.
>
>
>> So why do you want to change them?
> 1. I suggest to select identifiers also for jump labels which are more meaningful
>     and eventually unique for some function implementations.

I completely disagree. A longer identifier is not necessarily more 
meaningful than a shorter one.

The difference between calling a label "retry" and "lock_retry" is 
negligible, doesn't improve readability as far as I can see and is 
actually incorrect because the main meaning of the label is that we 
don't take the lock but rather that we restart the allocation operation.

Calling the label "unlock" instead of "out" is arguable a little better, 
but nothing I would call a major improvement either.

So that is a clear NAK to all those patches.

> 2. How do you think about to add a single space character before any label?

Bad as well. Why would anybody want to do this?

Christian.

>
> Regards,
> Markus

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

> iirc, there are Coccinelle rules that find code with unnecessary null
> checks and removes them.

This kind of software change is not needed here.

I find that a corresponding return value check happens one function call
too late.


> Although you probably made this complex enough that cocinelle would
> not find it.  That is not a complement.

I imagine that scripts for the semantic patch language can find more
source code places where questionable disjunctions are used so far.
Would you dare to split any more condition checks?


> One should not make error handling/cleanup more complex than needed.

I see a need to improve not only correctness there but also a bit of
software efficiency.

Regards,
Markus

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Rob Clark]

On Fri, Sep 23, 2016 at 7:19 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> iirc, there are Coccinelle rules that find code with unnecessary null
>> checks and removes them.
>
> This kind of software change is not needed here.
>
> I find that a corresponding return value check happens one function call
> too late.
>

I think you misunderstand my point.. which is that additional
conditional checks wrapping a function call to something that already
checks for null should be removed.. not introduced.

>
>> Although you probably made this complex enough that cocinelle would
>> not find it.  That is not a complement.
>
> I imagine that scripts for the semantic patch language can find more
> source code places where questionable disjunctions are used so far.
> Would you dare to split any more condition checks?
>
>
>> One should not make error handling/cleanup more complex than needed.
>
> I see a need to improve not only correctness there but also a bit of
> software efficiency.

If you can measure any performance difference and present some results
(esp. considering that this is something that just happens when the
driver is loaded), then we'll talk.  Until then, please don't send
this sort of patch.  Thank you.

BR,
-R

> Regards,
> Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

> Calling the label "unlock" instead of "out" is arguable a little better,

Thanks that you can follow a renaming for this direction in principle.


> but nothing I would call a major improvement either.

This was not my intention for such an use case.

I am proposing some small software updates according to such a design pattern.


> So that is a clear NAK to all those patches.

Do you reject also update steps like the following then?

* drm/ttm: Use kmalloc_array() in two (or four?) functions"

* drm/ttm: Less function calls in ttm_dma_pool_init() after error detection

* Would you like to improve the usage of the variables "n" and "t"
  in the function "ttm_dma_pool_init" any further as Joe Perches suggested it?


>> 2. How do you think about to add a single space character before any label?
> 
> Bad as well. Why would anybody want to do this?

Do you find another software evolution interesting according to a recent commit?

"docs: Remove space-before-label guidance from CodingStyle"
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a


Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

>> I see a need to improve not only correctness there but also a bit of
>> software efficiency.
> 
> If you can measure any performance difference and present some results
> (esp. considering that this is something that just happens when the
> driver is loaded), then we'll talk.

Are you really interested to increase your software development attention
for a quicker exception handling implementation in this function?


> Until then, please don't send this sort of patch.  Thank you.

Will benchmark statistics change such a change rejection ever?

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TTM: Fine-tuning for several function implementations

[Dan Carpenter]

On Fri, Sep 23, 2016 at 12:20:54PM +0200, SF Markus Elfring wrote:
> > if you call it "restart" or "lock_restart" doesn't make much difference.
> 
> Do other identifiers fit better to a specification from the document "CodingStyle"
> like the following?
> 
> "…
> Choose label names which say what the goto does or why the goto exists.
> …"
>
> 
> Does this wording need any more adjustments?

No.  I wrote that and "restart" seems like a pretty clear name to me.  I
never wrote that you should harrass people with your nonsense patches.
In fact, I have asked you over and over again to stop.

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Rob Clark]

On Fri, Sep 23, 2016 at 8:17 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> I see a need to improve not only correctness there but also a bit of
>>> software efficiency.
>>
>> If you can measure any performance difference and present some results
>> (esp. considering that this is something that just happens when the
>> driver is loaded), then we'll talk.
>
> Are you really interested to increase your software development attention
> for a quicker exception handling implementation in this function?
>

No one is interested in making error handling more complex for a
non-existent benefit ;-)

>
>> Until then, please don't send this sort of patch.  Thank you.
>
> Will benchmark statistics change such a change rejection ever?

If you could demonstrate a real benefit to additional complexity for
something that actually matters (ie. not something like this that runs
once at bootup) I would care.

I don't recommend that you waste your time, since there is
approximately nothing in modesetting path that happens with a high
enough frequency to benefit from such a micro-optimization.
Especially not initialization code that runs once at boot up.

Fixing actual bugs is useful and valuable.  Premature "optimization"
at the expense of extra complexity is very much not useful.

BR,
-R

> Regards,
> Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TTM: Fine-tuning for several function implementations

[Christian König]

Am 23.09.2016 um 13:49 schrieb SF Markus Elfring:
>> Calling the label "unlock" instead of "out" is arguable a little better,
> Thanks that you can follow a renaming for this direction in principle.
>
>
>> but nothing I would call a major improvement either.
> This was not my intention for such an use case.
>
> I am proposing some small software updates according to such a design pattern.
>
>
>> So that is a clear NAK to all those patches.
> Do you reject also update steps like the following then?
>
> * drm/ttm: Use kmalloc_array() in two (or four?) functions"
>
> * drm/ttm: Less function calls in ttm_dma_pool_init() after error detection

The reason behind the advise to use kmalloc_array() is to avoid overruns 
when one of the parameters come from an IOCTL and so are controllable by 
user space.

Those overruns where the source of numerous security problems, but in 
this case the parameters don't come from an IOCTL and aren't user space 
controllable.

So this change actually doesn't make to much sense either, but I'm 
leaning towards accepting them for coding style consistency.

Regards,
Christian.

> * Would you like to improve the usage of the variables "n" and "t"
>    in the function "ttm_dma_pool_init" any further as Joe Perches suggested it?
>
>
>>> 2. How do you think about to add a single space character before any label?
>> Bad as well. Why would anybody want to do this?
> Do you find another software evolution interesting according to a recent commit?
>
> "docs: Remove space-before-label guidance from CodingStyle"
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a
>
>
> Regards,
> Markus


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

>> Do other identifiers fit better to a specification from the document "CodingStyle"
>> like the following?
>>
>> "…
>> Choose label names which say what the goto does or why the goto exists.
>> …"
>>
>>
>> Does this wording need any more adjustments?
> 
> No.

I have got an other impression.

The terse description can trigger disagreements about the "what" and "why",
can't it?


> I wrote that and "restart" seems like a pretty clear name to me.

This identifier might be good enough to some degree.
I imagined that it would become better by the addition of a bit of information
from the jump target.


> I never wrote that you should harrass people with your nonsense patches.

This is true in principle.

But your adjustment for the document "CodingStyle" supported also a reconsideration
of the corresponding identifier selection.

Some developers disagreed with a proposed renaming while others reacted
in a positive way.


> In fact, I have asked you over and over again to stop.

This happened under different software update contexts occasionally.

Regards,
Markus

Re: GPU-DRM-Savage: Less function calls in savage_bci_cmdbuf() after error detection

[SF Markus Elfring]

>> Date: Thu, 18 Aug 2016 21:28:58 +0200
>>
>> The kfree() function was called in a few cases by the
>> savage_bci_cmdbuf() function during error handling
>> even if a passed variable contained a null pointer.
>>
>> Adjust jump targets according to the Linux coding style convention.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> Not sure this is worth it, I'll pass. Patch 1 merged.

Unfortunately, it seems that this selection of only one update step
from this small patch series has got unwanted consequences.

Will the update suggestion “[patch] drm/savage: dereferencing an error pointer”
by Dan Carpenter (from today) trigger further software development discussions?

https://patchwork.kernel.org/patch/9372127/
https://lkml.kernel.org/r/<20161012062227.GU12841@mwanda>


Will an update step like “[PATCH 2/2] GPU-DRM-Savage: Less function calls in
savage_bci_cmdbuf() after error detection” (from 2016-08-18) become worth
for related consideratons once more?

https://patchwork.kernel.org/patch/9289183/
https://lkml.kernel.org/r/<c97563c0-d463-8b15-5956-26d93641a54f@users.sourceforge.net>

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/amdgpu: Fine-tuning for several function implementations

[Markus Elfring]

> Date: Sun, 18 Sep 2016 18:38:48 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (5):
>   Use kmalloc_array() in amdgpu_debugfs_gca_config_read()
>   Improve determination of sizes in two functions
>   Rename a jump label in amdgpu_debugfs_regs_read()
>   Rename a jump label in amdgpu_device_init()
>   Adjust checks for null pointers in nine functions
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 68 +++++++++++++++---------------
>  1 file changed, 33 insertions(+), 35 deletions(-)

Is this patch series still in review queues?

Regards,
Markus

Re: [PATCH 0/5] drm/amdgpu: Fine-tuning for several function implementations

[Alex Deucher]

On Fri, Jan 5, 2024 at 1:15 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Date: Sun, 18 Sep 2016 18:38:48 +0200
> >
> > Some update suggestions were taken into account
> > from static source code analysis.
> >
> > Markus Elfring (5):
> >   Use kmalloc_array() in amdgpu_debugfs_gca_config_read()
> >   Improve determination of sizes in two functions
> >   Rename a jump label in amdgpu_debugfs_regs_read()
> >   Rename a jump label in amdgpu_device_init()
> >   Adjust checks for null pointers in nine functions
> >
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 68 +++++++++++++++---------------
> >  1 file changed, 33 insertions(+), 35 deletions(-)
>
> Is this patch series still in review queues?

Doesn't look like I ever received it.  Can you resend?

Alex

>
> Regards,
> Markus