Remove unnecessary null pointer checks?

From: Markus Elfring <[email protected]>
Date: Tue, 12 Jul 2016 22:00:55 +0200

The drm_gem_object_unreference_unlocked() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

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

drivers/gpu/drm/radeon/radeon_display.c +1 -4

index 5f1cd69..1efab5f 100644
@@ -1324,9 +1324,7 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb)
 {
        struct radeon_framebuffer *radeon_fb = to_radeon_framebuffer(fb);
 
-       if (radeon_fb->obj) {
-               drm_gem_object_unreference_unlocked(radeon_fb->obj);
-       }
+       drm_gem_object_unreference_unlocked(radeon_fb->obj);
        drm_framebuffer_cleanup(fb);
        kfree(radeon_fb);
 }
--

2.9.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Markus Elfring <[email protected]>
Date: Wed, 13 Jul 2016 19:46:45 +0200

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

Markus Elfring (3):
HDMI: Delete an unnecessary check before the function call "kfree" Delete unnecessary checks before drm_gem_object_unreference_unlocked() Delete an unnecessary check before drm_gem_object_unreference()

 drivers/gpu/drm/msm/hdmi/hdmi_hdcp.c    | 2 +-
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 3 +--
 drivers/gpu/drm/msm/msm_fb.c            | 4 ++--
 drivers/gpu/drm/msm/msm_gem.c           | 8 ++------
 4 files changed, 6 insertions(+), 11 deletions(-)

--
2.9.0

From: Markus Elfring <[email protected]>
Date: Fri, 15 Jul 2016 15:23:22 +0200

The drm_gem_object_unreference_unlocked() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

---
  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 8 ++------
  1 file changed, 2 insertions(+), 6 deletions(-)

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +0 -1

index 20f12bc..6f64e79 100644
@@ -43,14 +43,10 @@ struct drm_gem_object *rockchip_fb_get_gem_obj(struct drm_framebuffer *fb,
  static void rockchip_drm_fb_destroy(struct drm_framebuffer *fb)
  {
  	struct rockchip_drm_fb *rockchip_fb = to_rockchip_fb(fb);
-	struct drm_gem_object *obj;

drivers/gpu/drm/rockchip/rockchip_drm_fb.c +0 -0

  	drm_framebuffer_cleanup(fb);
  	kfree(rockchip_fb);

From: Markus Elfring <[email protected]>
Date: Fri, 15 Jul 2016 21:52:40 +-0200

The drm_fbdev_cma_hotplug_event() function tests whether its argument is NULL and then returns immediately.
Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 09:10:40 +0200

The drm_fbdev_cma_hotplug_event() function tests whether its argument is NULL and then returns immediately.
Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

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

drivers/gpu/drm/arm/hdlcd_drv.c +1 -2

index 28b8cd1..9b1aefe 100644
@@ -102,8 +102,7 @@ static void hdlcd_fb_output_poll_changed(struct drm_device *drm)
 {
 	struct hdlcd_drm_private *hdlcd = drm->dev_private;
 
-	if (hdlcd->fbdev)
-		drm_fbdev_cma_hotplug_event(hdlcd->fbdev);
+	drm_fbdev_cma_hotplug_event(hdlcd->fbdev);
 }
 
 static const struct drm_mode_config_funcs hdlcd_mode_config_funcs = {

Signature · 6 lines

2.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 09:54:22 +0200

The drm_gem_object_unreference_unlocked() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

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

drivers/gpu/drm/ast/ast_main.c +1 -2

index 7bc3aa6..904beaa 100644
@@ -295,9 +295,8 @@ static int ast_get_dram_info(struct drm_device *dev)
 static void ast_user_framebuffer_destroy(struct drm_framebuffer *fb)
 {
 	struct ast_framebuffer *ast_fb = to_ast_framebuffer(fb);
-	if (ast_fb->obj)
-		drm_gem_object_unreference_unlocked(ast_fb->obj);
 
+	drm_gem_object_unreference_unlocked(ast_fb->obj);
 	drm_framebuffer_cleanup(fb);
 	kfree(fb);
 }

Signature · 6 lines

2.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 20:15:17 +0200

The pci_dev_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

---
 arch/s390/pci/pci_event.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/s390/pci/pci_event.c +0 -0

index fb2a9a5..c2b27ad 100644

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 21:21:05 +0200

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 21:21:05 +0200

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 21:42:42 +0200

The kfree() function was called in one case by the scsiback_device_action() function during error handling even if the passed variable "tmr" contained a null pointer.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/xen/xen-scsiback.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/xen/xen-scsiback.c +0 -0

index 4a48c06..7612bc9 100644

drivers/xen/xen-scsiback.c +3 -2

 	init_waitqueue_head(&tmr->tmr_wait);

@@ -616,7 +616,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 			       unpacked_lun, tmr, act, GFP_KERNEL,
 			       tag, TARGET_SCF_ACK_KREF);
 	if (rc)
-		goto err;
+		goto free_tmr;
 
 	wait_event(tmr->tmr_wait, atomic_read(&tmr->tmr_complete));
 

@@ -626,8 +626,9 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
 	transport_generic_free_cmd(&pending_req->se_cmd, 1);
 	return;
-err:
+free_tmr:
 	kfree(tmr);
+do_resp:
 	scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
 }
 

drivers/xen/xen-scsiback.c static void scsiback_device_action(struct vscsibk_pend *pending_req, · line 610

Quoted 7 lines

drivers/xen/xen-scsiback.c +1 -1

@@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
 	if (!tmr) {
 		target_put_sess_cmd(se_cmd);
-		goto err;
+		goto do_resp;
 	}

Hmm, I'm not convinced this is an improvement.

I'd rather rename the new error label to "put_cmd" and get rid of the braces in above if statement:

(quoted diff) +2 -4

-	if (!tmr) {
-		target_put_sess_cmd(se_cmd);
-		goto err;
-	}
+	if (!tmr)
+		goto put_cmd;

and then in the error path:

+free_tmr:

	kfree(tmr);

Quoted 20 lines

drivers/xen/xen-scsiback.c static void scsiback_device_action(struct vscsibk_pend *pending_req, · line 610

Quoted 7 lines

drivers/xen/xen-scsiback.c +1 -1

@@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
 	if (!tmr) {
 		target_put_sess_cmd(se_cmd);
-		goto err;
+		goto do_resp;
 	}

Hmm, I'm not convinced this is an improvement.

I'd rather rename the new error label to "put_cmd" and get rid of the braces in above if statement:

(quoted diff) +2 -4

-	if (!tmr) {
-		target_put_sess_cmd(se_cmd);
-		goto err;
-	}
+	if (!tmr)
+		goto put_cmd;

Review

Quoted 5 lines

and then in the error path:

(quoted diff) +2 -1

-err:
+put_cmd:
+	target_put_sess_cmd(se_cmd);

I am unsure on the relevance of this function on such a source position. Would it make sense to move it further down at the end?

Quoted 2 lines

+free_tmr:

	kfree(tmr);

How do you think about to skip this function call after a memory allocation failure?

Quoted 22 lines

Quoted 22 lines

drivers/xen/xen-scsiback.c static void scsiback_device_action(struct vscsibk_pend *pending_req, · line 610

Quoted 7 lines

drivers/xen/xen-scsiback.c +1 -1

@@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
 	if (!tmr) {
 		target_put_sess_cmd(se_cmd);
-		goto err;
+		goto do_resp;
 	}

Hmm, I'm not convinced this is an improvement.

I'd rather rename the new error label to "put_cmd" and get rid of the braces in above if statement:

(quoted diff) +2 -4

-	if (!tmr) {
-		target_put_sess_cmd(se_cmd);
-		goto err;
-	}
+	if (!tmr)
+		goto put_cmd;

and then in the error path:

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +2 -1

-err:
+put_cmd:
+	target_put_sess_cmd(se_cmd);

I am unsure on the relevance of this function on such a source position. Would it make sense to move it further down at the end?

You only want to call it in the first error case (allocation failure).

Quoted 5 lines

Quoted 2 lines

+free_tmr:

	kfree(tmr);

How do you think about to skip this function call after a memory allocation failure?

I think this just doesn't matter. If it were a hot path, yes. But trying to do micro-optimizations in an error path is just not worth the effort.

I like a linear error path containing all the needed cleanups best.

Quoted 29 lines

Quoted 28 lines

Quoted 22 lines

drivers/xen/xen-scsiback.c static void scsiback_device_action(struct vscsibk_pend *pending_req, · line 610

Quoted 7 lines

drivers/xen/xen-scsiback.c +1 -1

@@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
 	if (!tmr) {
 		target_put_sess_cmd(se_cmd);
-		goto err;
+		goto do_resp;
 	}

Hmm, I'm not convinced this is an improvement.

I'd rather rename the new error label to "put_cmd" and get rid of the braces in above if statement:

(quoted diff) +2 -4

-	if (!tmr) {
-		target_put_sess_cmd(se_cmd);
-		goto err;
-	}
+	if (!tmr)
+		goto put_cmd;

and then in the error path:

Review

Quoted 3 lines

(quoted diff) +2 -1

-err:
+put_cmd:
+	target_put_sess_cmd(se_cmd);

I am unsure on the relevance of this function on such a source position. Would it make sense to move it further down at the end?

Quoted 1 line

You only want to call it in the first error case (allocation failure).

Thanks for your clarification.

I find that my update suggestion (from Saturday) is still appropriate in this case.
https://lkml.org/lkml/2016/7/16/172

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

+free_tmr:

	kfree(tmr);

How do you think about to skip this function call after a memory allocation failure?

I think this just doesn't matter. If it were a hot path, yes. But trying to do micro-optimizations in an error path is just not worth the effort.

Would you like to reduce also the amount of function calls in such special run-time situations?

Quoted 1 line

I like a linear error path containing all the needed cleanups best.

I would prefer to keep the discussed single function call within the basic block of the if statement.

Have we got different opinions about the shown implementation details?

From: Markus Elfring <[email protected]>
Date: Tue, 19 Jul 2016 15:42:19 +0200

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Tue, 19 Jul 2016 15:42:19 +0200

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Wed, 20 Jul 2016 13:03:16 +0200

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

* A bit of refactoring for the control flow

Suggested-by Jürgen Groß <[email protected]>

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Wed, 20 Jul 2016 13:03:16 +0200

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

* A bit of refactoring for the control flow

Suggested-by Jürgen Groß <[email protected]>

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Wed, 20 Jul 2016 13:12:33 +0200

Pass the constant "FAILED" in a function call directly instead of using an intialisation for a local variable.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Wed, 20 Jul 2016 13:12:33 +0200

Pass the constant "FAILED" in a function call directly instead of using an intialisation for a local variable.

Signed-off-by Markus Elfring <[email protected]>

Quoted 29 lines

Quoted 28 lines

Quoted 22 lines

drivers/xen/xen-scsiback.c static void scsiback_device_action(struct vscsibk_pend *pending_req, · line 610

Quoted 7 lines

drivers/xen/xen-scsiback.c +1 -1

@@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
 	if (!tmr) {
 		target_put_sess_cmd(se_cmd);
-		goto err;
+		goto do_resp;
 	}

Hmm, I'm not convinced this is an improvement.

I'd rather rename the new error label to "put_cmd" and get rid of the braces in above if statement:

(quoted diff) +2 -4

-	if (!tmr) {
-		target_put_sess_cmd(se_cmd);
-		goto err;
-	}
+	if (!tmr)
+		goto put_cmd;

and then in the error path:

Review

Quoted 3 lines

(quoted diff) +2 -1

-err:
+put_cmd:
+	target_put_sess_cmd(se_cmd);

I am unsure on the relevance of this function on such a source position. Would it make sense to move it further down at the end?

Quoted 1 line

You only want to call it in the first error case (allocation failure).

Thanks for your clarification.

I find that my update suggestion (from Saturday) is still appropriate in this case.
https://lkml.org/lkml/2016/7/16/172

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

+free_tmr:

	kfree(tmr);

How do you think about to skip this function call after a memory allocation failure?

I think this just doesn't matter. If it were a hot path, yes. But trying to do micro-optimizations in an error path is just not worth the effort.

Would you like to reduce also the amount of function calls in such special run-time situations?

Quoted 1 line

I like a linear error path containing all the needed cleanups best.

I would prefer to keep the discussed single function call within the basic block of the if statement.

Have we got different opinions about the shown implementation details?

Quoted 20 lines

drivers/xen/xen-scsiback.c static void scsiback_device_action(struct vscsibk_pend *pending_req, · line 610

Quoted 7 lines

drivers/xen/xen-scsiback.c +1 -1

@@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
 	if (!tmr) {
 		target_put_sess_cmd(se_cmd);
-		goto err;
+		goto do_resp;
 	}

Hmm, I'm not convinced this is an improvement.

I'd rather rename the new error label to "put_cmd" and get rid of the braces in above if statement:

(quoted diff) +2 -4

-	if (!tmr) {
-		target_put_sess_cmd(se_cmd);
-		goto err;
-	}
+	if (!tmr)
+		goto put_cmd;

Review

Quoted 5 lines

and then in the error path:

(quoted diff) +2 -1

-err:
+put_cmd:
+	target_put_sess_cmd(se_cmd);

I am unsure on the relevance of this function on such a source position. Would it make sense to move it further down at the end?

Quoted 2 lines

+free_tmr:

	kfree(tmr);

How do you think about to skip this function call after a memory allocation failure?

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 21:42:42 +0200

The kfree() function was called in one case by the scsiback_device_action() function during error handling even if the passed variable "tmr" contained a null pointer.

Adjust jump targets according to the Linux coding style convention.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/xen/xen-scsiback.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/xen/xen-scsiback.c +0 -0

index 4a48c06..7612bc9 100644

drivers/xen/xen-scsiback.c +1 -1

 	init_waitqueue_head(&tmr->tmr_wait);

@@ -616,7 +616,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 			       unpacked_lun, tmr, act, GFP_KERNEL,
 			       tag, TARGET_SCF_ACK_KREF);
 	if (rc)
-		goto err;
+		goto free_tmr;
 
 	wait_event(tmr->tmr_wait, atomic_read(&tmr->tmr_complete));
 

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 21:55:01 +0200

Pass the constant "FAILED" in a function call directly instead of using an intialisation for a local variable.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 21:55:01 +0200

Pass the constant "FAILED" in a function call directly instead of using an intialisation for a local variable.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Sun, 17 Jul 2016 13:14:57 +0200

The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Sun, 17 Jul 2016 13:38:46 +0200

A few variables were assigned a null pointer despite of the detail that they were immediately reassigned by the following statement. Thus remove such unnecessary assignments.

Signed-off-by Markus Elfring <[email protected]>

From: Markus Elfring <[email protected]>
Date: Sun, 17 Jul 2016 15:55:02 +0200

Return directly after a memory allocation failed at the beginning.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/staging/ks7010/ks7010_sdio.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

drivers/staging/ks7010/ks7010_sdio.c +0 -0

index 3622fba..9b954cb 100644

drivers/staging/ks7010/ks7010_sdio.c +5 -9

 	memcpy(data_buf, &index, sizeof(index));
 	retval = ks7010_sdio_write(priv, WRITE_INDEX, data_buf, sizeof(index));

@@ -744,10 +742,9 @@ static int ks7010_sdio_data_compare(struct ks_wlan_private *priv, u32 address,
 	unsigned char *read_buf;
 
 	read_buf = kmalloc(ROM_BUFF_SIZE, GFP_KERNEL);
-	if (!read_buf) {
-		rc = 1;
-		goto error_out;
-	}
+	if (!read_buf)
+		return 1;
+
 	retval = ks7010_sdio_read(priv, address, read_buf, size);
 	if (retval) {
 		rc = 2;

@@ -777,10 +774,8 @@ static int ks7010_upload_firmware(struct ks_wlan_private *priv,
 
 	/* buffer allocate */
 	rom_buf = kmalloc(ROM_BUFF_SIZE, GFP_KERNEL);
-	if (!rom_buf) {
-		rc = 3;
-		goto error_out0;
-	}
+	if (!rom_buf)
+		return 3;
 
 	sdio_claim_host(card->func);
 
--

2.9.1

Quoted 17 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sun, 17 Jul 2016 15:55:02 +0200

Return directly after a memory allocation failed at the beginning.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/staging/ks7010/ks7010_sdio.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

drivers/staging/ks7010/ks7010_sdio.c +0 -0

index 3622fba..9b954cb 100644
@@ -713,10 +713,8 @@ static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index)
 	unsigned char *data_buf;
 

drivers/staging/ks7010/ks7010_sdio.c static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index) · line 717

Quoted 7 lines

drivers/staging/ks7010/ks7010_sdio.c +2 -4

 	data_buf = kmalloc(sizeof(u32), GFP_KERNEL);
-	if (!data_buf) {
-		rc = 1;
-		goto error_out;
-	}
+	if (!data_buf)
+		return 1;

One could rather wonder why the function has such strange error values...

Quoted 12 lines

drivers/staging/ks7010/ks7010_sdio.c static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index) · line 717

Quoted 10 lines

drivers/staging/ks7010/ks7010_sdio.c +2 -4

@@ -713,10 +713,8 @@ static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index)
 	unsigned char *data_buf;
 
 	data_buf = kmalloc(sizeof(u32), GFP_KERNEL);
-	if (!data_buf) {
-		rc = 1;
-		goto error_out;
-	}
+	if (!data_buf)
+		return 1;

One could rather wonder why the function has such strange error values...

Agreed. Markus, can you check if we can use -ENOMEM in those places.

Quoted 13 lines

Quoted 12 lines

drivers/staging/ks7010/ks7010_sdio.c static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index) · line 717

Quoted 10 lines

drivers/staging/ks7010/ks7010_sdio.c +2 -4

@@ -713,10 +713,8 @@ static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index)
 	unsigned char *data_buf;
 
 	data_buf = kmalloc(sizeof(u32), GFP_KERNEL);
-	if (!data_buf) {
-		rc = 1;
-		goto error_out;
-	}
+	if (!data_buf)
+		return 1;

One could rather wonder why the function has such strange error values...

Quoted 1 line

Agreed. Markus, can you check if we can use -ENOMEM in those places.

I find that I do not know this software good enough at the moment so that I could safely decide on the shown special error values. I guess that further clarification might be needed for affected implementation details.

Quoted 2 lines

I guess that further clarification might be needed for affected implementation details.

That's OK, too.

Acked-by Wolfram Sang <[email protected]>

Quoted 6 lines

Quoted 2 lines

I guess that further clarification might be needed for affected implementation details.

That's OK, too.

Acked-by Wolfram Sang <[email protected]>

Does this acknowledgement include also the acceptance for the suggested change around calls of the functions "sdio_claim_host" and "sdio_release_host" within the implementation of the function "ks7010_upload_firmware"?

From: Markus Elfring <[email protected]>
Date: Sun, 17 Jul 2016 16:26:18 +0200

Adjust jump targets according to the Linux coding style convention.

avoids breaking the -p option of diff."

Quoted 1 line

Adjust jump targets according to the Linux coding style convention.

Really? Is that documented somewhere?

Quoting Jean Delvare:

"> It is generally accepted to indent labels with a single space. This

Quoted 1 line

avoids breaking the -p option of diff."

So, NACK for now unless we know 'diff' has been fixed.

Quoted 3 lines

Quoted 1 line

Adjust jump targets according to the Linux coding style convention.

Really? Is that documented somewhere?

How do you think about information from the chapter "7: Centralized exiting of functions"?
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?idGef4ad2684d380dd6d596140fb79395115c3950#n389

Quoted 1 line

Quoting Jean Delvare:

Quoted 2 lines

"> It is generally accepted to indent labels with a single space. This

Quoted 1 line

avoids breaking the -p option of diff."

Would you like to take another look at the warning "LEADING_SPACE"?
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?idGef4ad2684d380dd6d596140fb79395115c3950#n3004

Does such a check need further considerations?

Quoted 1 line

So, NACK for now unless we know 'diff' has been fixed.

I am also curious on corresponding software evolution.

Quoted 2 lines

How do you think about information from the chapter "7: Centralized exiting of functions"?
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?idGef4ad2684d380dd6d596140fb79395115c3950#n389

I'm not impressed by this piece of documentation.

Back to the lack of space before labels, it's at best a personal preference. If you insist on standardizing, I'd call it a bug in the documentation, which should be fixed. One space before label is the way to go.

That being said... checkpatch does not complain about leading space before labels. Not even with --strict. So why are you mentioning it here?

Just looking at the thread on lkml makes me feel dizzy.

When you are about to send that amount of messages, you should pause and think again.

Is it really worth it?

I think I'd be less annoyed by regular spam.

Quoted 4 lines

Quoted 2 lines

How do you think about information from the chapter "7: Centralized exiting of functions"?
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle?idGef4ad2684d380dd6d596140fb79395115c3950#n389

I'm not impressed by this piece of documentation.

I would also appreciate further improvements there.

Quoted 3 lines

Back to the lack of space before labels, it's at best a personal preference. If you insist on standardizing, I'd call it a bug in the documentation, which should be fixed. One space before label is the way to go.

Would you like to contribute another patch for such a coding style issue?

Quoted 2 lines

That being said... checkpatch does not complain about leading space before labels. Not even with --strict. So why are you mentioning it here?

How do you think about a similar software update?

staging: lustre: Fix a jump label position in osc_get_info()
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?idÇ1d264543f759fea147734cb63de36397817534
https://lkml.org/lkml/2015/12/21/401

That being said... checkpatch does not complain about leading space before labels. Not even with --strict. So why are you mentioning it here?

Quoted 2 lines

That being said... checkpatch does not complain about leading space before labels. Not even with --strict. So why are you mentioning it here?

I remembered a warning like "INDENTED_LABEL" instead.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id’d21ac74a9e3c09b0b01c764e530657e4c85c49#n4326

How do you generally think about jump label renaming?

Quoted 1 line

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id’d21ac74a9e3c09b0b01c764e530657e4c85c49#n4326

"#goto labels aren't indented, allow a single space however"

Can't be clearer :-)

Quoted 1 line

How do you generally think about jump label renaming?

Renaming from "out0:", "out1:" etc to something meaningful, yes.

Did you have anything else in mind?

Quoted 3 lines

Quoted 1 line

How do you generally think about jump label renaming?

Renaming from "out0:", "out1:" etc to something meaningful, yes.

I suggest to take another look at such identifiers.

Would you like to support the renaming of a label like "error_out1" (in the function "ks7010_upload_firmware" for example)?

Will the software evolution be continued also with information from the topic "Source code review around jump label usage"?
https://lkml.org/lkml/2015/12/11/378
http://article.gmane.org/gmane.linux.kernel/2106190

Quoted 2 lines

Would you like to support the renaming of a label like "error_out1" (in the function "ks7010_upload_firmware" for example)?

They should be renamed too. Anything using numbers instead of explicit

Anything using numbers instead of explicit labels should be updated.

I included the reasons in the patch I just sent,
hopefully the documentation is clearer now.

Quoted 3 lines

"Source code review around jump label usage"?
https://lkml.org/lkml/2015/12/11/378
http://article.gmane.org/gmane.linux.kernel/2106190

Personally I see no value in such statistics.

Either labels are wrong (either wrong indentation or wrong name) and should be fixed, or they are correct and you should not touch them.

Quoted 4 lines

Quoted 2 lines

Would you like to support the renaming of a label like "error_out1" (in the function "ks7010_upload_firmware" for example)?

They should be renamed too. Anything using numbers instead of explicit

Interesting …

Quoted 1 line

Anything using numbers instead of explicit labels should be updated.

Would you dare to search for corresponding update candidates explicitly by special semantic patch scripts?

Quoted 2 lines

I included the reasons in the patch I just sent,
hopefully the documentation is clearer now.

I am curious on how feedback will evolve for your suggestion "CodingStyle: Clarify and complete chapter 7".
https://lkml.org/lkml/2016/7/25/207

How do you think about to show a shorter label like "free_bar" (instead of "err_free_bar") as an example?

Quoted 5 lines

Quoted 3 lines

"Source code review around jump label usage"?
https://lkml.org/lkml/2015/12/11/378
http://article.gmane.org/gmane.linux.kernel/2106190

Personally I see no value in such statistics.

Do they indicate any code smells eventually?

Quoted 2 lines

Either labels are wrong (either wrong indentation or wrong name) and should be fixed, or they are correct and you should not touch them.

Do you find such changes worthwhile (without touching also any surrounding source code)?

I have a lot of other things to work on, of much greater interest (to me.)

Quoted 3 lines

Quoted 1 line

Personally I see no value in such statistics.

Do they indicate any code smells eventually?

I have no idea what you mean, sorry.

You keep asking more and more from me.

May I remind you this is your "project" in the first place, not mine?

If you have no idea what should be done,

or even whether anything should be done, then just move on to something else.

I have already expressed all my views on this topic

and am not willing to say anything more about it.

From: Markus Elfring <[email protected]>
Date: Sun, 17 Jul 2016 18:15:23 +0200

Some return values can also be directly used for various condition checks. Thus remove a local variable for intermediate assignments.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/staging/ks7010/ks7010_sdio.c | 81 +++++++++++++++---------------------
 1 file changed, 34 insertions(+), 47 deletions(-)

drivers/staging/ks7010/ks7010_sdio.c +0 -1

index eea18fb..b3ca8e2 100644
@@ -90,7 +90,6 @@ static int ks7010_sdio_write(struct ks_wlan_private *priv, unsigned int address,
 void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
 {
 	unsigned char rw_data;
-	int retval;
 
 	DPRINTK(4, "\n");
 

drivers/staging/ks7010/ks7010_sdio.c +30 -36

 			DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
 			goto out;
 		}

@@ -121,7 +121,6 @@ out:
 void ks_wlan_hw_sleep_wakeup_request(struct ks_wlan_private *priv)
 {
 	unsigned char rw_data;
-	int retval;
 
 	DPRINTK(4, "\n");
 

@@ -130,9 +129,10 @@ void ks_wlan_hw_sleep_wakeup_request(struct ks_wlan_private *priv)
 
 	if (atomic_read(&priv->sleepstatus.status) = 1) {
 		rw_data = WAKEUP_REQ;
-		retval > -		    ks7010_sdio_write(priv, WAKEUP, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      WAKEUP,
+				      &rw_data,
+				      sizeof(rw_data))) {
 			DPRINTK(1, " error : WAKEUP=%02X\n", rw_data);
 			goto out;
 		}

@@ -152,16 +152,15 @@ out:
 void ks_wlan_hw_wakeup_request(struct ks_wlan_private *priv)
 {
 	unsigned char rw_data;
-	int retval;
 
 	DPRINTK(4, "\n");
 	if (atomic_read(&priv->psstatus.status) = PS_SNOOZE) {
 		rw_data = WAKEUP_REQ;
-		retval > -		    ks7010_sdio_write(priv, WAKEUP, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      WAKEUP,
+				      &rw_data,
+				      sizeof(rw_data)))
 			DPRINTK(1, " error : WAKEUP=%02X\n", rw_data);
-		}
 		DPRINTK(4, "wake up : WAKEUP=%02X\n", rw_data);
 		priv->last_wakeup = jiffies;
 		++priv->wakeup_count;

@@ -175,7 +174,6 @@ int _ks_wlan_hw_power_save(struct ks_wlan_private *priv)
 {
 	int rc = 0;
 	unsigned char rw_data;
-	int retval;
 
 	if (priv->reg.powermgt = POWMGT_ACTIVE_MODE)
 		return rc;

@@ -198,11 +196,11 @@ int _ks_wlan_hw_power_save(struct ks_wlan_private *priv)
 				if (!atomic_read(&priv->psstatus.confirm_wait)
 				    && !atomic_read(&priv->psstatus.snooze_guard)
 				    && !cnt_txqbody(priv)) {
-					retval > -					    ks7010_sdio_read(priv, INT_PENDING,
+					if (ks7010_sdio_read(priv,
+							     INT_PENDING,
 							     &rw_data,
-							     sizeof(rw_data));
-					if (retval) {
+							     sizeof(rw_data))
+							     ) {
 						DPRINTK(1,
 							" error : INT_PENDING=%02X\n",
 							rw_data);

@@ -212,12 +210,11 @@ int _ks_wlan_hw_power_save(struct ks_wlan_private *priv)
 					}
 					if (!rw_data) {
 						rw_data = GCR_B_DOZE;
-						retval > -						    ks7010_sdio_write(priv,
+						if (ks7010_sdio_write(priv,
 								      GCR_B,
 								      &rw_data,
-								      sizeof(rw_data));
-						if (retval) {
+								      sizeof(rw_data))
+								      ) {
 							DPRINTK(1,
 								" error : GCR_B=%02X\n",
 								rw_data);

@@ -413,7 +410,6 @@ static void rx_event_task(unsigned long dev)
 static void ks_wlan_hw_rx(void *dev, uint16_t size)
 {
 	struct ks_wlan_private *priv = (struct ks_wlan_private *)dev;
-	int retval;
 	struct rx_device_buffer *rx_buffer;
 	struct hostif_hdr *hdr;
 	unsigned char read_status;

@@ -429,12 +425,11 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size)
 	}
 	rx_buffer = &priv->rx_dev.rx_dev_buff[priv->rx_dev.qtail];
 
-	retval > -	    ks7010_sdio_read(priv, DATA_WINDOW, &rx_buffer->data[0],
-			     hif_align_size(size));
-	if (retval) {
+	if (ks7010_sdio_read(priv,
+			     DATA_WINDOW,
+			     &rx_buffer->data[0],
+			     hif_align_size(size)))
 		goto error_out;
-	}
 
 	/* length check */
 	if (size > 2046 || size = 0) {

@@ -446,12 +441,11 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size)
 #endif
 		/* rx_status update */
 		read_status = READ_STATUS_IDLE;
-		retval > -		    ks7010_sdio_write(priv, READ_STATUS, &read_status,
-				      sizeof(read_status));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      READ_STATUS,
+				      &read_status,
+				      sizeof(read_status)))
 			DPRINTK(1, " error : READ_STATUS=%02X\n", read_status);
-		}
 		goto error_out;
 	}
 

@@ -462,12 +456,11 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size)
 
 	/* read status update */
 	read_status = READ_STATUS_IDLE;
-	retval > -	    ks7010_sdio_write(priv, READ_STATUS, &read_status,
-			      sizeof(read_status));
-	if (retval) {
+	if (ks7010_sdio_write(priv,
+			      READ_STATUS,
+			      &read_status,
+			      sizeof(read_status)))
 		DPRINTK(1, " error : READ_STATUS=%02X\n", read_status);
-	}
 	DPRINTK(4, "READ_STATUS=%02X\n", read_status);
 
 	if (atomic_read(&priv->psstatus.confirm_wait)) {

@@ -489,7 +482,6 @@ static void ks7010_rw_function(struct work_struct *work)
 	struct hw_info_t *hw;
 	struct ks_wlan_private *priv;
 	unsigned char rw_data;
-	int retval;
 
 	hw = container_of(work, struct hw_info_t, rw_wq.work);
 	priv = container_of(hw, struct ks_wlan_private, ks_wlan_hw);

@@ -538,9 +530,7 @@ static void ks7010_rw_function(struct work_struct *work)
 	}
 
 	/* read (WriteStatus/ReadDataSize FN1:00_0014) */
-	retval > -	    ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data, sizeof(rw_data));
-	if (retval) {
+	if (ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data, sizeof(rw_data))) {
 		DPRINTK(1, " error : WSTATUS_RSIZE=%02X psstatus=%d\n", rw_data,
 			atomic_read(&priv->psstatus.status));
 		goto release_host;

@@ -708,7 +698,6 @@ static void trx_device_exit(struct ks_wlan_private *priv)
 static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index)
 {
 	int rc = 0;
-	int retval;
 	unsigned char *data_buf;
 
 	data_buf = kmalloc(sizeof(u32), GFP_KERNEL);

@@ -716,14 +705,12 @@ static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index)
 		return 1;
 
 	memcpy(data_buf, &index, sizeof(index));
-	retval = ks7010_sdio_write(priv, WRITE_INDEX, data_buf, sizeof(index));
-	if (retval) {
+	if (ks7010_sdio_write(priv, WRITE_INDEX, data_buf, sizeof(index))) {
 		rc = 2;
 		goto free_buf;
 	}
 
-	retval = ks7010_sdio_write(priv, READ_INDEX, data_buf, sizeof(index));
-	if (retval)
+	if (ks7010_sdio_write(priv, READ_INDEX, data_buf, sizeof(index)))
 		rc = 3;
 free_buf:
 	kfree(data_buf);
--

2.9.1

Quoted 25 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sun, 17 Jul 2016 18:15:23 +0200

Some return values can also be directly used for various condition checks. Thus remove a local variable for intermediate assignments.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/staging/ks7010/ks7010_sdio.c | 81 +++++++++++++++---------------------
 1 file changed, 34 insertions(+), 47 deletions(-)

drivers/staging/ks7010/ks7010_sdio.c +0 -1

index eea18fb..b3ca8e2 100644
@@ -90,7 +90,6 @@ static int ks7010_sdio_write(struct ks_wlan_private *priv, unsigned int address,
 void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
 {
 	unsigned char rw_data;
-	int retval;
 
 	DPRINTK(4, "\n");
 

@@ -99,9 +98,10 @@ void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
 

drivers/staging/ks7010/ks7010_sdio.c void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv) · line 104

Quoted 9 lines

drivers/staging/ks7010/ks7010_sdio.c +4 -3

 	if (atomic_read(&priv->sleepstatus.status) == 0) {
 		rw_data = GCR_B_DOZE;
-		retval =
-		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      GCR_B,
+				      &rw_data,
+				      sizeof(rw_data))) {

A multi-line function call in an if test does not look nice at all. The original code was an easy-to-read expectable pattern.

Quoted 10 lines

Quoted 10 lines

drivers/staging/ks7010/ks7010_sdio.c +0 -1

@@ -90,7 +90,6 @@ static int ks7010_sdio_write(struct ks_wlan_private *priv, unsigned int address,
 void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
 {
 	unsigned char rw_data;
-	int retval;
 
 	DPRINTK(4, "\n");
 

@@ -99,9 +98,10 @@ void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
 

Quoted 11 lines

drivers/staging/ks7010/ks7010_sdio.c void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv) · line 104

Quoted 8 lines

drivers/staging/ks7010/ks7010_sdio.c +4 -2

 	if (atomic_read(&priv->sleepstatus.status) = 0) {
 		rw_data = GCR_B_DOZE;
-		retval >>> -		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      GCR_B,
+				      &rw_data,
+				      sizeof(rw_data))) {

A multi-line function call in an if test does not look nice at all. The original code was an easy-to-read expectable pattern.

I agree. I am not strict on the 80 char limit, especially in cases like the above.

Quoted 14 lines

Quoted 13 lines

drivers/staging/ks7010/ks7010_sdio.c void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv) · line 104

Quoted 10 lines

drivers/staging/ks7010/ks7010_sdio.c +4 -3

 	if (atomic_read(&priv->sleepstatus.status) == 0) {
 		rw_data = GCR_B_DOZE;
-		retval =
-		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      GCR_B,
+				      &rw_data,
+				      sizeof(rw_data))) {

A multi-line function call in an if test does not look nice at all. The original code was an easy-to-read expectable pattern.

Quoted 2 lines

I agree. I am not strict on the 80 char limit, especially in cases like the above.

Would you try an other source code formatting for the suggested change pattern?

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

drivers/staging/ks7010/ks7010_sdio.c void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv) · line 104

Quoted 9 lines

drivers/staging/ks7010/ks7010_sdio.c +4 -2

 	if (atomic_read(&priv->sleepstatus.status) = 0) {
 		rw_data = GCR_B_DOZE;
-		retval > > >>> -		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      GCR_B,
+				      &rw_data,
+				      sizeof(rw_data))) {

A multi-line function call in an if test does not look nice at all. The original code was an easy-to-read expectable pattern.

Quoted 4 lines

Quoted 2 lines

I agree. I am not strict on the 80 char limit, especially in cases like the above.

Would you try an other source code formatting for the suggested change pattern?

I don't understand the question?

Quoted 17 lines · Wolfram Sang

Quoted 17 lines

Quoted 16 lines

Quoted 13 lines

drivers/staging/ks7010/ks7010_sdio.c void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv) · line 104

Quoted 10 lines

drivers/staging/ks7010/ks7010_sdio.c +4 -3

 	if (atomic_read(&priv->sleepstatus.status) == 0) {
 		rw_data = GCR_B_DOZE;
-		retval =
-		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      GCR_B,
+				      &rw_data,
+				      sizeof(rw_data))) {

A multi-line function call in an if test does not look nice at all. The original code was an easy-to-read expectable pattern.

I agree. I am not strict on the 80 char limit, especially in cases like the above.

Quoted 3 lines

Quoted 1 line

Would you try an other source code formatting for the suggested change pattern?

I don't understand the question?

I think the original code was fine. x = blah(); if (x) ... is a perfectly familiar kernel coding pattern. There is no benefit in terms of performance or understandability in dropping the variable.

I think the original code was fine.

x = blah(); if (x) ... is a perfectly familiar kernel coding pattern.

There is no benefit in terms of performance

or understandability in dropping the variable.

Quoted 1 line

I think the original code was fine.

I suggest to reconsider involved implementation details once more.

Quoted 1 line

x = blah(); if (x) ... is a perfectly familiar kernel coding pattern.

I can agree to such a general information.

Quoted 1 line

There is no benefit in terms of performance

It might be possible that a good compiler can also optimise some unnecessary variable accesses away.

Examples for further background information:
* "Minimize local variables"
https://eventhelix.com/realtimemantra/basics/optimizingcandcppcode.htm#Minimize%20Local%20Variables

* "Temporary Objects" by Danny Kalev
http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum8

Quoted 1 line

or understandability in dropping the variable.

I guess that we have got different opinions on such an aspect.

* Do you really want to assign every return value from a function call to an extra variable before it is used again?

* How many reading and understanding capacity do you need for each extra variable?

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

Quoted 3 lines

* Do you really want to assign every return value from a function call to an extra variable before it is used again?

If it's a choice between 1) function call on many lines, 2) function call over 80 characters, and 3) extra variable, I definitely prefer the extra variable.

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

drivers/staging/ks7010/ks7010_sdio.c void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv) · line 104

Quoted 8 lines

drivers/staging/ks7010/ks7010_sdio.c +4 -2

 	if (atomic_read(&priv->sleepstatus.status) = 0) {
 		rw_data = GCR_B_DOZE;
-		retval >>>>> -		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      GCR_B,
+				      &rw_data,
+				      sizeof(rw_data))) {

A multi-line function call in an if test does not look nice at all. The original code was an easy-to-read expectable pattern.

Quoted 2 lines

Quoted 2 lines

I agree. I am not strict on the 80 char limit, especially in cases like the above.

Quoted 1 line

Would you try an other source code formatting for the suggested change pattern?

I don't understand the question?

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

drivers/staging/ks7010/ks7010_sdio.c void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv) · line 104

Quoted 9 lines

drivers/staging/ks7010/ks7010_sdio.c +4 -3

 	if (atomic_read(&priv->sleepstatus.status) == 0) {
 		rw_data = GCR_B_DOZE;
-		retval =
-		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv,
+				      GCR_B,
+				      &rw_data,
+				      sizeof(rw_data))) {

A multi-line function call in an if test does not look nice at all. The original code was an easy-to-read expectable pattern.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I agree. I am not strict on the 80 char limit, especially in cases like the above.

Will this line length limitation trigger any more collateral evolution in the discussed software module?

Quoted 3 lines

Quoted 1 line

Would you try an other source code formatting for the suggested change pattern?

I don't understand the question?

Can you follow expectations around the proposed refactoring of any function implementations?

Hello,

I am trying the software combination "spatch version 1.0.5-00058-g36edc2b-dirty compiled with OCaml version 4.03.0" out a bit more with the following approach.

@checking_function_calls_directly@
identifier checker, retval, work;
parameter list pl;
statement is, es;
type rt;
@@
rt checker(...)
{
...

(quoted diff) +1 -2

-retval = work(pl);
 if (
-    retval
+    work(pl)
    )
    is
 else
    es
...
 }

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt -sp-file checking_function_calls_directly1.cocci ~/Projekte/Linux/next-patched/drivers/staging/ks7010/ks7010_sdio.c
init_defs_builtins: /usr/local/lib64/coccinelle/standard.h
minus: parse error:
File "checking_function_calls_directly1.cocci", line 10, column 15, charpos = 151
around = 'pl',
whole content = -retval = work(pl);

Do I overlook anything in my SmPL script?

A parameter list is something that comes after the name of a function in the function header. Something like int x, int y, in foo (int x, int y) { ... }. Probably you want an expression list.

Quoted 3 lines

A parameter list is something that comes after the name of a function in the function header. Something like int x, int y, in foo (int x, int y) { ... }. Probably you want an expression list.

* How do you call the list usually that is used for the definition before the function body?

* Did I get inappropriate expectations from a description around a "parameter-list-declarator"?
http://en.cppreference.com/w/c/language/function_definition

* The following SmPL script variant seems to work better.

@checking_function_calls_directly@
identifier checker, retval, work;
expression list el;
statement is, es;
type rt;
@@
rt checker(...)
{
<+...

(quoted diff) +1 -2

-retval = work(el);
 if (
-    retval
+    work(el)
    )
    is
 else
    es
  ...+>
 }

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt -sp-file checking_function_calls_directly2.cocci ~/Projekte/Linux/next-patched/drivers/staging/ks7010/ks7010_sdio.c
init_defs_builtins: /usr/local/lib64/coccinelle/standard.h
HANDLING: /home/elfring/Projekte/Linux/next-patched/drivers/staging/ks7010/ks7010_sdio.c
diff =

tmp/cocci-output-1578-3a5e93-ks7010_sdio.c renamed from home/elfring/Projekte/Linux/next-patched/drivers/staging/ks7010/ks7010_sdio.c +1 -3

@@ -99,9 +99,7 @@ void ks_wlan_hw_sleep_doze_request(struc
 
 	if (atomic_read(&priv->sleepstatus.status) == 0) {
 		rw_data = GCR_B_DOZE;
-		retval =
-		    ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
-		if (retval) {
+		if (ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data))) {
 			DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
 			goto out;
 		}

?

Regards,
Markus

Quoted 6 lines

Quoted 3 lines

A parameter list is something that comes after the name of a function in the function header. Something like int x, int y, in foo (int x, int y) { ... }. Probably you want an expression list.

* How do you call the list usually that is used for the definition before the function body?

parameter list

Quoted 2 lines

* Did I get inappropriate expectations from a description around a "parameter-list-declarator"?
http://en.cppreference.com/w/c/language/function_definition

Apparently.

parameter-list - either the single keyword void or a comma-separated list of parameters, which may end with an ellipsis parameter

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

A parameter list is something that comes after the name of a function in the function header. Something like int x, int y, in foo (int x, int y) { ... }. Probably you want an expression list.

* How do you call the list usually that is used for the definition before the function body?

parameter list

It seems that we have got the same understanding here.

Quoted 4 lines

Quoted 2 lines

* Did I get inappropriate expectations from a description around a "parameter-list-declarator"?
http://en.cppreference.com/w/c/language/function_definition

Apparently.

I am surprised by this feedback.

Quoted 2 lines

parameter-list - either the single keyword void or a comma-separated list of parameters, which may end with an ellipsis parameter

How does this description fit to the term "expression list" in the semantic patch language?

int f(int x, int y) <---- parameter list

{
   g(1, 2, 3, 4, 5): <---- expression list
}

Quoted 2 lines

Can you follow expectations around the proposed refactoring of any function implementations?

I don't understand both questions. Maybe you need to give examples?