Source code review around jump label usage

From: Markus Elfring <[email protected]>
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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index df7ab245..2709ebd 100644
@@ -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

Applied. thanks.

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 2709ebd..96a2457 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Sun, 18 Sep 2016 17:35:24 +0200

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

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

index 96a2457..2b8ba97 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Sun, 18 Sep 2016 17:50:09 +0200

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

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

index 2b8ba97..fed4854 100644
@@ -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

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBTRiBNYXJrdXMgRWxmcmluZyBb bWFpbHRvOmVsZnJpbmdAdXNlcnMuc291cmNlZm9yZ2UubmV0XQ0KPiBTZW50OiBTdW5kYXksIFNl cHRlbWJlciAxOCwgMjAxNiAxMjo1MyBQTQ0KPiBUbzogZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNr dG9wLm9yZzsgRGV1Y2hlciwgQWxleGFuZGVyOyBLb2VuaWcsIENocmlzdGlhbjsNCj4gWmhvdSwg RGF2aWQoQ2h1bk1pbmcpOyBEYXZpZCBBaXJsaWU7IExpdSwgTW9uazsgU3REZW5pcywgVG9tDQo+ IENjOiBMS01MOyBrZXJuZWwtamFuaXRvcnNAdmdlci5rZXJuZWwub3JnOyBKdWxpYSBMYXdhbGwN Cj4gU3ViamVjdDogW1BBVENIIDQvNV0gZHJtL2FtZGdwdTogUmVuYW1lIGEganVtcCBsYWJlbCBp bg0KPiBhbWRncHVfZGV2aWNlX2luaXQoKQ0KPiANCj4gRnJvbTogTWFya3VzIEVsZnJpbmcgPGVs ZnJpbmdAdXNlcnMuc291cmNlZm9yZ2UubmV0Pg0KPiBEYXRlOiBTdW4sIDE4IFNlcCAyMDE2IDE3 OjUwOjA5ICswMjAwDQo+IA0KPiBBZGp1c3QganVtcCBsYWJlbHMgYWNjb3JkaW5nIHRvIHRoZSBj dXJyZW50IExpbnV4IGNvZGluZyBzdHlsZSBjb252ZW50aW9uLg0KPiANCj4gU2lnbmVkLW9mZi1i eTogTWFya3VzIEVsZnJpbmcgPGVsZnJpbmdAdXNlcnMuc291cmNlZm9yZ2UubmV0Pg0KPiAtLS0N Cj4gIGRyaXZlcnMvZ3B1L2RybS9hbWQvYW1kZ3B1L2FtZGdwdV9kZXZpY2UuYyB8IDIxICsrKysr KysrKystLS0tLS0tLS0tDQo+IC0NCj4gIDEgZmlsZSBjaGFuZ2VkLCAxMCBpbnNlcnRpb25zKCsp LCAxMSBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vYW1k L2FtZGdwdS9hbWRncHVfZGV2aWNlLmMNCj4gYi9kcml2ZXJzL2dwdS9kcm0vYW1kL2FtZGdwdS9h bWRncHVfZGV2aWNlLmMNCj4gaW5kZXggMmI4YmE5Ny4uZmVkNDg1NCAxMDA2NDQNCj4gLS0tIGEv ZHJpdmVycy9ncHUvZHJtL2FtZC9hbWRncHUvYW1kZ3B1X2RldmljZS5jDQo+ICsrKyBiL2RyaXZl cnMvZ3B1L2RybS9hbWQvYW1kZ3B1L2FtZGdwdV9kZXZpY2UuYw0KPiBAQCAtMTU2NiwxOCArMTU2 NiwxOCBAQCBpbnQgYW1kZ3B1X2RldmljZV9pbml0KHN0cnVjdCBhbWRncHVfZGV2aWNlDQo+ICph ZGV2LA0KPiAgCS8qIFJlYWQgQklPUyAqLw0KPiAgCWlmICghYW1kZ3B1X2dldF9iaW9zKGFkZXYp KSB7DQo+ICAJCXIgPSAtRUlOVkFMOw0KPiAtCQlnb3RvIGZhaWxlZDsNCj4gKwkJZ290byBjaGVj a19ydW50aW1lOw0KDQpOQUNLLiAgRmFpbGVkIGlzIGEgbW9yZSBhcHByb3ByaWF0ZSBsYWJlbCBo ZXJlLiAgVGhlIHJ1bnRpbWUgY2hlY2sgaXMganVzdCBwYXJ0IG9mIHRoZSBmYWlsdXJlIGNsZWFu dXAuDQoNCkFsZXgNCg0KPiAgCX0NCj4gIAkvKiBNdXN0IGJlIGFuIEFUT01CSU9TICovDQo+ICAJ aWYgKCFhZGV2LT5pc19hdG9tX2Jpb3MpIHsNCj4gIAkJZGV2X2VycihhZGV2LT5kZXYsICJFeHBl Y3RpbmcgYXRvbWJpb3MgZm9yIEdQVVxuIik7DQo+ICAJCXIgPSAtRUlOVkFMOw0KPiAtCQlnb3Rv IGZhaWxlZDsNCj4gKwkJZ290byBjaGVja19ydW50aW1lOw0KPiAgCX0NCj4gIAlyID0gYW1kZ3B1 X2F0b21iaW9zX2luaXQoYWRldik7DQo+ICAJaWYgKHIpIHsNCj4gIAkJZGV2X2VycihhZGV2LT5k ZXYsICJhbWRncHVfYXRvbWJpb3NfaW5pdCBmYWlsZWRcbiIpOw0KPiAtCQlnb3RvIGZhaWxlZDsN Cj4gKwkJZ290byBjaGVja19ydW50aW1lOw0KPiAgCX0NCj4gDQo+ICAJLyogU2VlIGlmIHRoZSBh c2ljIHN1cHBvcnRzIFNSLUlPViAqLw0KPiBAQCAtMTU5NSw3ICsxNTk1LDcgQEAgaW50IGFtZGdw dV9kZXZpY2VfaW5pdChzdHJ1Y3QgYW1kZ3B1X2RldmljZQ0KPiAqYWRldiwNCj4gIAkJaWYgKCFh ZGV2LT5iaW9zKSB7DQo+ICAJCQlkZXZfZXJyKGFkZXYtPmRldiwgIkNhcmQgbm90IHBvc3RlZCBh bmQgbm8gQklPUyAtDQo+IGlnbm9yaW5nXG4iKTsNCj4gIAkJCXIgPSAtRUlOVkFMOw0KPiAtCQkJ Z290byBmYWlsZWQ7DQo+ICsJCQlnb3RvIGNoZWNrX3J1bnRpbWU7DQo+ICAJCX0NCj4gIAkJRFJN X0lORk8oIkdQVSBub3QgcG9zdGVkLiBwb3N0aW5nIG5vdy4uLlxuIik7DQo+ICAJCWFtZGdwdV9h dG9tX2FzaWNfaW5pdChhZGV2LT5tb2RlX2luZm8uYXRvbV9jb250ZXh0KTsNCj4gQEAgLTE2MDUs NyArMTYwNSw3IEBAIGludCBhbWRncHVfZGV2aWNlX2luaXQoc3RydWN0IGFtZGdwdV9kZXZpY2UN Cj4gKmFkZXYsDQo+ICAJciA9IGFtZGdwdV9hdG9tYmlvc19nZXRfY2xvY2tfaW5mbyhhZGV2KTsN Cj4gIAlpZiAocikgew0KPiAgCQlkZXZfZXJyKGFkZXYtPmRldiwgImFtZGdwdV9hdG9tYmlvc19n ZXRfY2xvY2tfaW5mbw0KPiBmYWlsZWRcbiIpOw0KPiAtCQlnb3RvIGZhaWxlZDsNCj4gKwkJZ290 byBjaGVja19ydW50aW1lOw0KPiAgCX0NCj4gIAkvKiBpbml0IGkyYyBidXNlcyAqLw0KPiAgCWFt ZGdwdV9hdG9tYmlvc19pMmNfaW5pdChhZGV2KTsNCj4gQEAgLTE2MTQsNyArMTYxNCw3IEBAIGlu dCBhbWRncHVfZGV2aWNlX2luaXQoc3RydWN0IGFtZGdwdV9kZXZpY2UNCj4gKmFkZXYsDQo+ICAJ ciA9IGFtZGdwdV9mZW5jZV9kcml2ZXJfaW5pdChhZGV2KTsNCj4gIAlpZiAocikgew0KPiAgCQlk ZXZfZXJyKGFkZXYtPmRldiwgImFtZGdwdV9mZW5jZV9kcml2ZXJfaW5pdCBmYWlsZWRcbiIpOw0K PiAtCQlnb3RvIGZhaWxlZDsNCj4gKwkJZ290byBjaGVja19ydW50aW1lOw0KPiAgCX0NCj4gDQo+ ICAJLyogaW5pdCB0aGUgbW9kZSBjb25maWcgKi8NCj4gQEAgLTE2MjQsNyArMTYyNCw3IEBAIGlu dCBhbWRncHVfZGV2aWNlX2luaXQoc3RydWN0IGFtZGdwdV9kZXZpY2UNCj4gKmFkZXYsDQo+ICAJ aWYgKHIpIHsNCj4gIAkJZGV2X2VycihhZGV2LT5kZXYsICJhbWRncHVfaW5pdCBmYWlsZWRcbiIp Ow0KPiAgCQlhbWRncHVfZmluaShhZGV2KTsNCj4gLQkJZ290byBmYWlsZWQ7DQo+ICsJCWdvdG8g Y2hlY2tfcnVudGltZTsNCj4gIAl9DQo+IA0KPiAgCWFkZXYtPmFjY2VsX3dvcmtpbmcgPSB0cnVl Ow0KPiBAQCAtMTYzNCw3ICsxNjM0LDcgQEAgaW50IGFtZGdwdV9kZXZpY2VfaW5pdChzdHJ1Y3Qg YW1kZ3B1X2RldmljZQ0KPiAqYWRldiwNCj4gIAlyID0gYW1kZ3B1X2liX3Bvb2xfaW5pdChhZGV2 KTsNCj4gIAlpZiAocikgew0KPiAgCQlkZXZfZXJyKGFkZXYtPmRldiwgIklCIGluaXRpYWxpemF0 aW9uIGZhaWxlZCAoJWQpLlxuIiwgcik7DQo+IC0JCWdvdG8gZmFpbGVkOw0KPiArCQlnb3RvIGNo ZWNrX3J1bnRpbWU7DQo+ICAJfQ0KPiANCj4gIAlyID0gYW1kZ3B1X2liX3JpbmdfdGVzdHMoYWRl dik7DQo+IEBAIC0xNjgyLDEyICsxNjgyLDExIEBAIGludCBhbWRncHVfZGV2aWNlX2luaXQoc3Ry dWN0IGFtZGdwdV9kZXZpY2UNCj4gKmFkZXYsDQo+ICAJciA9IGFtZGdwdV9sYXRlX2luaXQoYWRl dik7DQo+ICAJaWYgKHIpIHsNCj4gIAkJZGV2X2VycihhZGV2LT5kZXYsICJhbWRncHVfbGF0ZV9p bml0IGZhaWxlZFxuIik7DQo+IC0JCWdvdG8gZmFpbGVkOw0KPiArCQlnb3RvIGNoZWNrX3J1bnRp bWU7DQo+ICAJfQ0KPiANCj4gIAlyZXR1cm4gMDsNCj4gLQ0KPiAtZmFpbGVkOg0KPiArIGNoZWNr X3J1bnRpbWU6DQo+ICAJaWYgKHJ1bnRpbWUpDQo+ICAJCXZnYV9zd2l0Y2hlcm9vX2ZpbmlfZG9t YWluX3BtX29wcyhhZGV2LT5kZXYpOw0KPiAgCXJldHVybiByOw0KPiAtLQ0KPiAyLjEwLjANCg0K

From: Markus Elfring <[email protected]>
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.

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

index fed4854..b5b7cfb 100644
@@ -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

Is this patch series still in review queues?

Regards,
Markus

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

Alex

From: Markus Elfring <[email protected]>
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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/drm_bufs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 3219151..ed33f43 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

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

index ed33f43..8a31dac 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/drm_bufs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8a31dac..36dd685 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/drm_bufs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 36dd685..adb1dd7 100644
@@ -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

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

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

From: Markus Elfring <[email protected]>
Date: Mon, 19 Sep 2016 17:37:27 +0200

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

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

index adb1dd7..0d5ee1e 100644
@@ -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

From: Markus Elfring <[email protected]>
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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/gma500/mid_bios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d75ecb3..a833568 100644
@@ -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

Did you test this running on the hardware?

BR,
Jani.

Jani Nikula, Intel Open Source Technology Center

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

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Tue, 20 Sep 2016 09:09:10 +0200

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

---
 drivers/gpu/drm/gma500/mid_bios.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index a833568..cf4e605 100644
@@ -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

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.

Jani Nikula, Intel Open Source Technology Center

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/gma500/mid_bios.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index cf4e605..3caee42 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Tue, 20 Sep 2016 10:34:28 +0200

Adjust the indentation for a single function call parameter here.

---
 drivers/gpu/drm/gma500/mid_bios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 3caee42..9004d30 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/gma500/mid_bios.c | 1 +
 1 file changed, 1 insertion(+)

index 9004d30..e5cece0 100644
@@ -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

Did you test this?

BR,
Jani.

Jani Nikula, Intel Open Source Technology Center

What is your software development opinion for this update suggestion?

Regards,
Markus

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

Thanks for this kind of constructive feedback.

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

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

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

Thanks for another bit of constructive feedback.

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

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

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

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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/gma500/mid_bios.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

index e5cece0..602d16f 100644
@@ -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

Please don't.

BR,
Jani.

Jani Nikula, Intel Open Source Technology Center

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

From: Markus Elfring <[email protected]>
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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 4ceed7a9..7b32dd3 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 7b32dd3..3a4f91b 100644
@@ -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

From: Markus Elfring <[email protected]>
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"

* 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.

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

index 3a4f91b..60beeb9 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 60beeb9..c262ef5 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 60beeb9..c262ef5 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c262ef5..f110965 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index f110965..c6a7197 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Wed, 21 Sep 2016 17:30:25 +0200

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

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

index c6a7197..5f6f21b 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Wed, 21 Sep 2016 17:32:42 +0200

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

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 5f6f21b..c8ced158 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c8ced158..c5c3793 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 505dee0..e4f1924 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e4f1924..26f1212 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_gem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 26f1212..3c49ad9 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Wed, 21 Sep 2016 17:45:04 +0200

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

---
 drivers/gpu/drm/omapdrm/omap_gem.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index 3c49ad9..92510de 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/omapdrm/omap_gem.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

index 92510de..ea7ad1c 100644
@@ -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

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

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

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.

Regards,

Laurent Pinchart

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?

It's a pity.

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

I am curious on useful responses.

Thanks for another bit of constructive feedback.

I propose to dare another look at corresponding information sources.

I have got an other impression.

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

I hope not.

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

Thanks for your opinion on this issue.

I chose a specific patch granularity for this proposal.

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

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

Regards,
Markus

From: Markus Elfring <[email protected]>
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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index f9c79da..8faa28f 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Thu, 22 Sep 2016 09:29:23 +0200

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

---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8faa28f..6204405 100644
@@ -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

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

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

Regards,
Markus

From: Markus Elfring <[email protected]>
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"

---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

index 6204405..6ee5865 100644
@@ -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

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

This kind of feedback can be fine at first glance.

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

There is inherent software complexity involved.

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

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.

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

Yes, of course.

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

This can occasionally happen.

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

I can agree to this view.

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

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

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

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

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

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

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.

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

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?

Thanks for this information.

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

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

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

This kind of software change is not needed here.

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

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?

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

Regards,
Markus

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.

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

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

Will benchmark statistics change such a change rejection ever?

Regards,
Markus

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

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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 6ee5865..ae90728 100644
@@ -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

From: Markus Elfring <[email protected]>
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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index a37de5d..bfc51cb 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Thu, 22 Sep 2016 14:16:05 +0200

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

---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index bfc51cb..13fdd19 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Thu, 22 Sep 2016 14:30:12 +0200

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

---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 13fdd19..f33f6f6 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Thu, 22 Sep 2016 14:36:47 +0200

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

---
 drivers/gpu/drm/ttm/ttm_page_alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index f33f6f6..3dd603f 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index bef9f6f..194818d 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Thu, 22 Sep 2016 14:54:12 +0200

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

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 194818d..9dc1632 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Thu, 22 Sep 2016 15:32:32 +0200

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

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index 9dc1632..ce3d361 100644
@@ -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

From: Markus Elfring <[email protected]>
Date: Thu, 22 Sep 2016 16:02:36 +0200

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

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ce3d361..e3f5542 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index e3f5542..feba278 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index feba278..c21f45f 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index c21f45f..d5f41ed 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index d5f41ed..4c50196 100644
@@ -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

From: Markus Elfring <[email protected]>
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.

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 4c50196..aa1679d 100644
@@ -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

From: Markus Elfring <[email protected]>
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".

---
 drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index aa1679d..e21c1b3 100644
@@ -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

[]

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:

I am suggesting changes for another collateral software evolution.

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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Am 23.09.2016 um 12:20 schrieb SF Markus Elfring:

No, not really.

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.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I have got an other impression.

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

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

Am 23.09.2016 um 13:07 schrieb SF Markus Elfring:

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.

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

Christian.

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

This was not my intention for such an use case.

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

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?

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?idyc70c304b0b443429b2a0019518532c5162817a

Regards,
Markus

Am 23.09.2016 um 13:49 schrieb SF Markus Elfring:

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.

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

I have got an other impression.

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

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.

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.

This happened under different software update contexts occasionally.

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Fri, 23 Sep 2016 21:38:21 +0200

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

Markus Elfring (4):
Use kmalloc_array() in i2cdev_ioctl_rdwr() Improve another size determination in i2cdev_ioctl_rdwr() Rename jump labels in i2cdev_attach_adapter() Adjust checks for null pointers in three functions

 drivers/i2c/i2c-dev.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

2.10.0

From: Markus Elfring <[email protected]>
Date: Fri, 23 Sep 2016 20:30: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.

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

---
 drivers/i2c/i2c-dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 66f323f..6d8226d 100644
@@ -260,7 +260,9 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 	if (IS_ERR(rdwr_pa))
 		return PTR_ERR(rdwr_pa);
 
-	data_ptrs = kmalloc(rdwr_arg.nmsgs * sizeof(u8 __user *), GFP_KERNEL);
+	data_ptrs = kmalloc_array(rdwr_arg.nmsgs,
+				  sizeof(*data_ptrs),
+				  GFP_KERNEL);
 	if (data_ptrs = NULL) {
 		kfree(rdwr_pa);
 		return -ENOMEM;

2.10.0

From: Markus Elfring <[email protected]>
Date: Fri, 23 Sep 2016 20:45:40 +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.

---
 drivers/i2c/i2c-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 6d8226d..a6e35ce 100644
@@ -256,7 +256,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		return -EINVAL;
 
 	rdwr_pa = memdup_user(rdwr_arg.msgs,
-			      rdwr_arg.nmsgs * sizeof(struct i2c_msg));
+			      rdwr_arg.nmsgs * sizeof(*rdwr_pa));
 	if (IS_ERR(rdwr_pa))
 		return PTR_ERR(rdwr_pa);
 

2.10.0

From: Markus Elfring <[email protected]>
Date: Fri, 23 Sep 2016 21:21:39 +0200

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

---
 drivers/i2c/i2c-dev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index a6e35ce..7d3e3ca 100644
@@ -555,7 +555,7 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
 	i2c_dev->cdev.owner = THIS_MODULE;
 	res = cdev_add(&i2c_dev->cdev, MKDEV(I2C_MAJOR, adap->nr), 1);
 	if (res)
-		goto error_cdev;
+		goto put_i2c;
 
 	/* register this i2c device with the driver core */
 	i2c_dev->dev = device_create(i2c_dev_class, &adap->dev,

@@ -563,15 +563,15 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
 				     "i2c-%d", adap->nr);
 	if (IS_ERR(i2c_dev->dev)) {
 		res = PTR_ERR(i2c_dev->dev);
-		goto error;
+		goto delete_cdev;
 	}
 
 	pr_debug("i2c-dev: adapter [%s] registered as minor %d\n",
 		 adap->name, adap->nr);
 	return 0;
-error:
+delete_cdev:
 	cdev_del(&i2c_dev->cdev);
-error_cdev:
+put_i2c:
 	put_i2c_dev(i2c_dev);
 	return res;
 }

2.10.0

From: Markus Elfring <[email protected]>
Date: Fri, 23 Sep 2016 21:30:20 +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.

---
 drivers/i2c/i2c-dev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 7d3e3ca..8f7eddd 100644
@@ -147,7 +147,7 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
 		count = 8192;
 
 	tmp = kmalloc(count, GFP_KERNEL);
-	if (tmp = NULL)
+	if (!tmp)
 		return -ENOMEM;
 
 	pr_debug("i2c-dev: i2c-%d reading %zu bytes.\n",

@@ -263,7 +263,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 	data_ptrs = kmalloc_array(rdwr_arg.nmsgs,
 				  sizeof(*data_ptrs),
 				  GFP_KERNEL);
-	if (data_ptrs = NULL) {
+	if (!data_ptrs) {
 		kfree(rdwr_pa);
 		return -ENOMEM;
 	}

@@ -374,7 +374,7 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
 				      client->flags, data_arg.read_write,
 				      data_arg.command, data_arg.size, NULL);
 
-	if (data_arg.data = NULL) {
+	if (!data_arg.data) {
 		dev_dbg(&client->adapter->dev,
 			"data is NULL pointer in ioctl I2C_SMBUS.\n");
 		return -EINVAL;

2.10.0

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Markus Elfring <[email protected]>
Date: Sat, 24 Sep 2016 08:10:08 +0200

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

Markus Elfring (7):
Use kmalloc_array() in iio_scan_mask_set() Rename a jump label in iio_buffer_store_watermark() Rename a jump label in iio_buffer_store_enable() Rename a jump label in iio_buffer_write_length() Rename a jump label in iio_scan_el_ts_store() Rename a jump label in iio_scan_el_store() Adjust checks for null pointers in six functions

 drivers/iio/industrialio-buffer.c | 55 ++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 29 deletions(-)

2.10.0

From: Markus Elfring <[email protected]>
Date: Fri, 23 Sep 2016 22:30:32 +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.

---
 drivers/iio/industrialio-buffer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

index 49bf9c5..7a4d9499 100644
@@ -307,10 +307,9 @@ static int iio_scan_mask_set(struct iio_dev *indio_dev,
 	const unsigned long *mask;
 	unsigned long *trialmask;
 
-	trialmask = kmalloc(sizeof(*trialmask)*
-			    BITS_TO_LONGS(indio_dev->masklength),
-			    GFP_KERNEL);
-
+	trialmask = kmalloc_array(BITS_TO_LONGS(indio_dev->masklength),
+				  sizeof(*trialmask),
+				  GFP_KERNEL);
 	if (trialmask = NULL)
 		return -ENOMEM;
 	if (!indio_dev->masklength) {

2.10.0

Applied to the togreg branch of iio.git - initially pushed out as testing for the autobuilders to play with it.

Thanks,

Jonathan