Source code review around jump label usage

Quoted 3 lines

Do you request that I should resend my four update suggestions for different components as a single patch series for the software area "drivers/mfd"?

You have to make that decision yourself.

What I'm saying is, if the cover letter says there are 2 patches

in the set, that's what we should expect.

Quoted 5 lines

Quoted 3 lines

Do you request that I should resend my four update suggestions for different components as a single patch series for the software area "drivers/mfd"?

You have to make that decision yourself.

I chose on 2015-12-29 to send them in the combination you see.

Quoted 1 line

What I'm saying is, if the cover letter says there are 2 patches

These refer to the component "smsc-ece1099".

Quoted 1 line

in the set, that's what we should expect.

Can changes for the components "dm355evm_msp" and "twl-core" be clarified independently?

From: Markus Elfring <[email protected]>
Date: Tue, 29 Dec 2015 19:29:08 +0100

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

Implementation details could be improved by the adjustment of jump targets according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

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

---
 drivers/mfd/twl-core.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

drivers/mfd/twl-core.c +0 -0

index 831696e..0d9350c 100644

drivers/mfd/twl-core.c +1 -1

 	}
 
 	pdev->dev.parent = &twl->client->dev;

@@ -634,7 +634,7 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
 		status = platform_device_add_data(pdev, pdata, pdata_len);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add platform_data\n");
-			goto err;
+			goto put_device;
 		}
 	}
 

@@ -647,21 +647,20 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
 		status = platform_device_add_resources(pdev, r, irq1 ? 2 : 1);

drivers/mfd/twl-core.c +6 -7

 		device_init_wakeup(&pdev->dev, can_wakeup);
-
-err:
-	if (status < 0) {
-		platform_device_put(pdev);
-		dev_err(&twl->client->dev, "can't add %s dev\n", name);
-		return ERR_PTR(status);
+		return &pdev->dev;
 	}
-	return &pdev->dev;
+put_device:
+	platform_device_put(pdev);
+report_failure:
+	dev_err(&twl->client->dev, "can't add %s dev\n", name);
+	return ERR_PTR(status);
 }
 
 static inline struct device *add_child(unsigned mod_no, const char *name,

From: Markus Elfring <[email protected]>
Date: Sun, 15 May 2016 19:20:28 +0200

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

* Change an error message.

* Return directly in this case.

This issue was detected by using the Coccinelle software.

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

---
 drivers/mfd/twl-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/mfd/twl-core.c +4 -3

index 831696e..dc34e69 100644
@@ -623,9 +623,10 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
 
 	pdev = platform_device_alloc(name, num);
 	if (!pdev) {
-		dev_dbg(&twl->client->dev, "can't alloc dev\n");
-		status = -ENOMEM;
-		goto err;
+		dev_err(&twl->client->dev,
+			"Allocation failed for device: %s\n",
+			name);
+		return ERR_PTR(-ENOMEM);
 	}
 
 	pdev->dev.parent = &twl->client->dev;

Signature · 6 lines

2.8.2

--
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 1 line

* Change an error message.

Why? Is dev_err needed?

Quoted 3 lines

Quoted 1 line

* Change an error message.

Why? Is dev_err needed?

I interpreted Lee's response in this way.
https://lkml.org/lkml/2016/1/11/104

Quoted 4 lines · SF Markus Elfring

Quoted 3 lines

Quoted 1 line

* Change an error message.

Why? Is dev_err needed?

Quoted 2 lines

I interpreted Lee's response in this way.
https://lkml.org/lkml/2016/1/11/104

OK. He didn't ask for the message to be changed though. It's a bit unfortunate that it now takes up multiple lines. And I believe it also prints redundant information. Perhaps he will have some further thoughts on the matter.

Please remove the print altogether.

From: Markus Elfring <[email protected]>
Date: Sun, 15 May 2016 19:50:55 +0200

Adjust jump targets according to the Linux coding style convention. Another check for the variable "status" can be omitted then at the end.

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

---
 drivers/mfd/twl-core.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

drivers/mfd/twl-core.c +2 -4

index dc34e69..3e4f4e4 100644
@@ -635,7 +635,7 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
 		status = platform_device_add_data(pdev, pdata, pdata_len);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add platform_data\n");
-			goto err;
+			goto put_device;
 		}
 	}
 

@@ -648,21 +648,20 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
 		status = platform_device_add_resources(pdev, r, irq1 ? 2 : 1);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add irqs\n");
-			goto err;
+			goto put_device;
 		}
 	}
 
 	status = platform_device_add(pdev);
-	if (status == 0)
-		device_init_wakeup(&pdev->dev, can_wakeup);

From: Markus Elfring <[email protected]>
Date: Sun, 26 Jun 2016 12:25:36 +0200

The platform_device_put() function was called in one case by the add_numbered_child() function during error handling even if the passed variable "pdev" contained a null pointer.
Return directly in this case.

This issue was detected by using the Coccinelle software.

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

---
 drivers/mfd/twl-core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/mfd/twl-core.c +2 -5

index 831696e..9458c6d 100644
@@ -622,11 +622,8 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
 	twl = &twl_priv->twl_modules[sid];
 
 	pdev = platform_device_alloc(name, num);
-	if (!pdev) {
-		dev_dbg(&twl->client->dev, "can't alloc dev\n");
-		status = -ENOMEM;
-		goto err;
-	}
+	if (!pdev)
+		return ERR_PTR(-ENOMEM);
 
 	pdev->dev.parent = &twl->client->dev;
 

From: Markus Elfring <[email protected]>
Date: Sun, 26 Jun 2016 13:03:59 +0200

Adjust jump targets according to the Linux coding style convention. Another check for the variable "status" can be omitted then at the end.

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

Acked-by Lee Jones <[email protected]>

---
 drivers/mfd/twl-core.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

drivers/mfd/twl-core.c +11 -10

index 9458c6d..a49d3db 100644
@@ -631,7 +631,7 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
 		status = platform_device_add_data(pdev, pdata, pdata_len);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add platform_data\n");
-			goto err;
+			goto put_device;
 		}
 	}
 

@@ -644,21 +644,22 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
 		status = platform_device_add_resources(pdev, r, irq1 ? 2 : 1);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add irqs\n");
-			goto err;
+			goto put_device;
 		}
 	}
 
 	status = platform_device_add(pdev);
-	if (status == 0)
-		device_init_wakeup(&pdev->dev, can_wakeup);
+	if (status)
+		goto put_device;
+
+	device_init_wakeup(&pdev->dev, can_wakeup);
 
-err:
-	if (status < 0) {
-		platform_device_put(pdev);
-		dev_err(&twl->client->dev, "can't add %s dev\n", name);
-		return ERR_PTR(status);
-	}
 	return &pdev->dev;
+
+put_device:
+	platform_device_put(pdev);
+	dev_err(&twl->client->dev, "failed to add device %s\n", name);
+	return ERR_PTR(status);
 }
 
 static inline struct device *add_child(unsigned mod_no, const char *name,

From: Markus Elfring <[email protected]>
Date: Sun, 26 Jun 2016 13:40:35 +0200

The platform_device_put() function was called in one case by the add_child() function during error handling even if the passed variable "pdev" contained a null pointer.
Return directly in this case.

This issue was detected by using the Coccinelle software.

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

---
 drivers/mfd/dm355evm_msp.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/mfd/dm355evm_msp.c +2 -5

index 14661ec..270e19c 100644
@@ -199,11 +199,8 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 	int			status;
 
 	pdev = platform_device_alloc(name, -1);
-	if (!pdev) {
-		dev_dbg(&client->dev, "can't alloc dev\n");
-		status = -ENOMEM;
-		goto err;
-	}
+	if (!pdev)
+		return ERR_PTR(-ENOMEM);
 
 	device_init_wakeup(&pdev->dev, can_wakeup);
 	pdev->dev.parent = &client->dev;

From: Markus Elfring <[email protected]>
Date: Sun, 26 Jun 2016 13:56:58 +0200

Adjust jump targets according to the Linux coding style convention. Another check for the variable "status" can be omitted then at the end.

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

---
 drivers/mfd/dm355evm_msp.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/mfd/dm355evm_msp.c +1 -1

index 270e19c..baf6075 100644
@@ -209,7 +209,7 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_data(pdev, pdata, pdata_len);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add platform_data\n");
-			goto err;
+			goto put_device;
 		}
 	}
 

drivers/mfd/dm355evm_msp.c +8 -6

-err:
-	if (status < 0) {
-		platform_device_put(pdev);
-		dev_err(&client->dev, "can't add %s dev\n", name);
-		return ERR_PTR(status);
-	}
+	if (status)
+		goto put_device;
+
 	return &pdev->dev;
+
+put_device:
+	platform_device_put(pdev);
+	dev_err(&client->dev, "failed to add device %s\n", name);
+	return ERR_PTR(status);
 }
 
 static int add_children(struct i2c_client *client)

Quoted 9 lines

drivers/mfd/dm355evm_msp.c +1 -1

@@ -222,19 +222,21 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_resources(pdev, &r, 1);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add irq\n");
-			goto err;
+			goto put_device;
 		}
 	}
 

drivers/mfd/dm355evm_msp.c static struct device *add_child(struct i2c_client *client, const char *name, · line 230

Quoted 2 lines

drivers/mfd/dm355evm_msp.c +0 -0

 	status = platform_device_add(pdev);
 

Remove this line too.

Quoted 9 lines

Quoted 9 lines

drivers/mfd/dm355evm_msp.c +1 -1

@@ -222,19 +222,21 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_resources(pdev, &r, 1);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add irq\n");
-			goto err;
+			goto put_device;
 		}
 	}
 

Quoted 4 lines

drivers/mfd/dm355evm_msp.c static struct device *add_child(struct i2c_client *client, const char *name, · line 230

Quoted 2 lines

drivers/mfd/dm355evm_msp.c +0 -0

 	status = platform_device_add(pdev);
 

Remove this line too.

Do you propose the deletion of a blank line here?

Did you skip this update suggestion while the other patches were finally accepted?

From: Markus Elfring <[email protected]>
Date: Thu, 30 Jun 2016 21:54:51 +0200

Adjust jump targets according to the Linux coding style convention. Another check for the variable "status" can be omitted then at the end.

Quoted 6 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Thu, 30 Jun 2016 21:54:51 +0200

Adjust jump targets according to the Linux coding style convention. Another check for the variable "status" can be omitted then at the end.

Two headers in this email:

References: <[email protected]> <[email protected]> <20160111082922.GB14104@x1> <[email protected]> <[email protected]> <20160608111442.GB14888@dell> <[email protected]> <[email protected]> <20160628150725.GL24982@dell> <[email protected]> <20160628163146.GG29166@dell>
In-reply-to: <20160628163146.GG29166@dell>

Can you please fix your email client to _not_ send References:
and In-reply-to: headers when sending new patches?  Thanks.

Even better would be to use git send-email for these patches.

Can you please fix your email client to _not_ send References:
and In-reply-to: headers when sending new patches?

Even better would be to use git send-email for these patches.

Quoted 3 lines

Can you please fix your email client to _not_ send References:
and In-reply-to: headers when sending new patches?

I prefer to associate some patches with previous relevant discussion directly.

Quoted 2 lines

Even better would be to use git send-email for these patches.

This command can also support a parameter like "--thread", can't it?

Would you like point any more views out about communication styles?

From: Markus Elfring <[email protected]>
Date: Thu, 30 Jun 2016 21:54:51 +0200

Adjust jump targets according to the Linux coding style convention. Another check for the variable "status" can be omitted then at the end.

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

---
 drivers/mfd/dm355evm_msp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

drivers/mfd/dm355evm_msp.c +9 -8

index 270e19c..86eca61 100644
@@ -209,7 +209,7 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_data(pdev, pdata, pdata_len);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add platform_data\n");
-			goto err;
+			goto put_device;
 		}
 	}
 

@@ -222,19 +222,20 @@ static struct device *add_child(struct i2c_client *client, const char *name,
 		status = platform_device_add_resources(pdev, &r, 1);
 		if (status < 0) {
 			dev_dbg(&pdev->dev, "can't add irq\n");
-			goto err;
+			goto put_device;
 		}
 	}
 
 	status = platform_device_add(pdev);
+	if (status)
+		goto put_device;
 
-err:
-	if (status < 0) {
-		platform_device_put(pdev);
-		dev_err(&client->dev, "can't add %s dev\n", name);
-		return ERR_PTR(status);
-	}
 	return &pdev->dev;
+
+put_device:
+	platform_device_put(pdev);
+	dev_err(&client->dev, "failed to add device %s\n", name);
+	return ERR_PTR(status);
 }
 
 static int add_children(struct i2c_client *client)

FYI, code looks fine.

... but please take this opportunity to set-up your submission environment i.e. using git format-patch and git send-email.

you've done that, please re-submit this patch with my:

Quoted 1 line

FYI, code looks fine.

Thanks for your acknowledgement.

Quoted 2 lines

... but please take this opportunity to set-up your submission environment i.e. using git format-patch and git send-email.

Would you like to see any special settings to be mentioned in a section like "15) Explicit In-Reply-To headers" from the document "SubmittingPatches"?

Quoted 1 line

you've done that, please re-submit this patch with my:

Does the association of this patch with a bit relevant discussion really hinder the desired commit?

I'm unsure if you're not re-submitting because you're waiting for an answer for me or not.

If you are, here it is. And, don't do that. :)

Please submit your patch-set *not* connected to this, now very tangled web of submissions, complete with all the Acks you've accrued.

Quoted 2 lines

I'm unsure if you're not re-submitting because you're waiting for an answer for me or not.

I found your five commits (on 2016-06-29) for this patch series sufficient in principle.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=d313cdde71ec9a5c327a515c37a0dca2cca00de5

Quoted 1 line

If you are, here it is. And, don't do that. :)

I find this feedback a bit strange.

Quoted 3 lines

Please submit your patch-set *not* connected to this, now very tangled web of submissions, complete with all the Acks you've accrued.

I would find it nice if another one of my update suggestions could be integrated after a bit of feedback evolved for a special source code search pattern.

https://patchwork.kernel.org/patch/9199519/
https://patchwork.kernel.org/patch/9208925/
https://patchwork.kernel.org/patch/9210291/

It seems to be harder to achieve acceptance similar to the others.

Please rebase all of your unaccepted patches on v4.8-rc1.

Also ensure you do not send them attached to any other patch that you've previously sent.

Quoted 1 line

Please rebase all of your unaccepted patches on v4.8-rc1.

Thanks for your suggestion. - Would you like to add any comments to my remaining update suggestions for Linux modules (besides "dm355evm_msp")?

I would like to clarify still a few implementation details then.
https://patchwork.kernel.org/patch/9210291/

Quoted 2 lines

Also ensure you do not send them attached to any other patch that you've previously sent.

I imagine that another patch can evolve as a reply to a constructive software development discussion, can't it?

From: Markus Elfring <[email protected]>
Date: Sun, 26 Jun 2016 14:14:54 +0200

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

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

---
 drivers/mfd/smsc-ece1099.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mfd/smsc-ece1099.c +1 -1

index 7f89e89..2aaf89f 100644
@@ -36,7 +36,7 @@ static int smsc_i2c_probe(struct i2c_client *i2c,
 {
 	struct smsc *smsc;
 	int devid, rev, venid_l, venid_h;
-	int ret = 0;
+	int ret;
 
 	smsc = devm_kzalloc(&i2c->dev, sizeof(struct smsc),
 				GFP_KERNEL);

From: Markus Elfring <[email protected]>
Date: Sun, 26 Jun 2016 14:30:46 +0200

This issue was detected by using the Coccinelle software.

Return directly if a call of the function "devm_regmap_init_i2c" or "regmap_write" failed.

Delete the jump label "err" then.

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

---
 drivers/mfd/smsc-ece1099.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

drivers/mfd/smsc-ece1099.c +3 -6

index 2aaf89f..cd18c09 100644
@@ -46,10 +46,8 @@ static int smsc_i2c_probe(struct i2c_client *i2c,
 	}
 
 	smsc->regmap = devm_regmap_init_i2c(i2c, &smsc_regmap_config);
-	if (IS_ERR(smsc->regmap)) {
-		ret = PTR_ERR(smsc->regmap);
-		goto err;
-	}
+	if (IS_ERR(smsc->regmap))
+		return PTR_ERR(smsc->regmap);
 
 	i2c_set_clientdata(i2c, smsc);
 	smsc->dev = &i2c->dev;

@@ -68,7 +66,7 @@ static int smsc_i2c_probe(struct i2c_client *i2c,
 
 	ret = regmap_write(smsc->regmap, SMSC_CLK_CTRL, smsc->clk);
 	if (ret)
-		goto err;
+		return ret;
 
 #ifdef CONFIG_OF
 	if (i2c->dev.of_node)

@@ -76,7 +74,6 @@ static int smsc_i2c_probe(struct i2c_client *i2c,
 					   NULL, NULL, &i2c->dev);
 #endif
 
-err:
 	return ret;
 }
 

From: Markus Elfring <[email protected]>
Date: Sun, 26 Jun 2016 15:25:43 +0200

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

Markus Elfring (6):
twl-core: Return directly after a failed platform_device_alloc()
in add_numbered_child()
twl-core: Refactoring for add_numbered_child()
dm355evm_msp: Return directly after a failed platform_device_alloc()
in add_child()
dm355evm_msp: Refactoring for add_child()
smsc-ece1099: Delete an unnecessary variable initialisation

    in smsc_i2c_probe()
  smsc-ece1099: Return directly after a function failure
    in smsc_i2c_probe()

 drivers/mfd/dm355evm_msp.c | 25 ++++++++++++-------------
 drivers/mfd/smsc-ece1099.c | 11 ++++-------
 drivers/mfd/twl-core.c     | 28 +++++++++++++---------------
 3 files changed, 29 insertions(+), 35 deletions(-)

From: Markus Elfring <[email protected]>
Date: Tue, 29 Dec 2015 20:28:46 +0100

This issue was detected by using the Coccinelle software.

Move the jump label directly before the desired log statement so that the variable "err" will not be checked once more after it was determined that a call of the function "__mmc_set_signal_voltage" or "__mmc_switch" failed. Use the identifier "report_failure" instead of the label "err".

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

---
 drivers/mmc/core/mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/mmc/core/mmc.c +5 -5

index 549c56e..866f72b 100644
@@ -1256,7 +1256,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 
        /* If fails try again during next card power cycle */
        if (err)
-               goto err;
+               goto report_failure;
 
        mmc_select_driver_type(card);
 

@@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
                                   card->ext_csd.generic_cmd6_time,
                                   true, send_status, true);
                if (err)
-                       goto err;
+                       goto report_failure;
                old_timing = host->ios.timing;
                mmc_set_timing(host, MMC_TIMING_MMC_HS200);
                if (!send_status) {

@@ -1289,10 +1289,11 @@ static int mmc_select_hs200(struct mmc_card *card)
                                mmc_set_timing(host, old_timing);
                }
        }
-err:
-       if (err)
+       if (err) {
+report_failure:
                pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
                       __func__, err);
+       }
        return err;
 }
 
--

2.6.3

From: Markus Elfring <[email protected]>
Date: Tue, 29 Dec 2015 21:11:45 +0100

These variables will eventually be set to an appropriate value a bit later.
* host
* iobase
* result

Thus let us omit the explicit initialisation at the beginning.

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

From: Markus Elfring <[email protected]>
Date: Tue, 29 Dec 2015 21:45:34 +0100

This issue was detected by using the Coccinelle software.

Two pointer checks could be repeated by the sdricoh_init_mmc() function during error handling even if the relevant properties can be determined for the involved variables before by source code analysis.

* This implementation detail could be improved by adjustments for jump targets according to the Linux coding style convention.

* Drop an unnecessary initialisation for the variable "mmc" then.

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

---

From: Markus Elfring <[email protected]>
Date: Tue, 29 Dec 2015 21:54:14 +0100

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

Markus Elfring (2):
Delete unnecessary variable initialisations in sdricoh_init_mmc() Less checks in sdricoh_init_mmc() after error detection

 drivers/mmc/host/sdricoh_cs.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

--
2.6.3

From: Markus Elfring <[email protected]>
Date: Thu, 31 Dec 2015 21:15:15 +0100

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

From: Markus Elfring <[email protected]>
Date: Thu, 31 Dec 2015 22:40:39 +0100

Adjust a jump target to eliminate a check before error logging. Use the identifier "report_failure" instead of "err".

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

---
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

drivers/net/ethernet/cavium/thunder/nicvf_main.c +7 -8

index c24cb2a..21e1579 100644
@@ -922,7 +922,7 @@ static int nicvf_register_interrupts(struct nicvf *nic)
 		ret = request_irq(vector, nicvf_intr_handler,
 				  0, nic->irq_name[irq], nic->napi[irq]);
 		if (ret)
-			goto err;
+			goto report_failure;
 		nic->irq_allocated[irq] = true;
 	}
 

@@ -933,7 +933,7 @@ static int nicvf_register_interrupts(struct nicvf *nic)
 		ret = request_irq(vector, nicvf_rbdr_intr_handler,
 				  0, nic->irq_name[irq], nic);
 		if (ret)
-			goto err;
+			goto report_failure;
 		nic->irq_allocated[irq] = true;
 	}
 

@@ -944,13 +944,12 @@ static int nicvf_register_interrupts(struct nicvf *nic)
 	ret = request_irq(nic->msix_entries[irq].vector,
 			  nicvf_qs_err_intr_handler,
 			  0, nic->irq_name[irq], nic);
-	if (!ret)
+	if (!ret) {
 		nic->irq_allocated[irq] = true;
-
-err:
-	if (ret)
-		netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq);
-
+		return 0;
+	}
+report_failure:
+	netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq);
 	return ret;
 }

Quoted 2 lines

Adjust a jump target to eliminate a check before error logging. Use the identifier "report_failure" instead of "err".

I don't see much value in those changes.

Using the 'err' label is ok as it is not misleading and common use.

And, there is no need to optimize the check since this is not the fast path

and will be compiler optimized anyway.

Quoted 18 lines

drivers/net/ethernet/cavium/thunder/nicvf_main.c +5 -6

@@ -944,13 +944,12 @@ static int nicvf_register_interrupts(struct nicvf *nic)
 	ret = request_irq(nic->msix_entries[irq].vector,
 			  nicvf_qs_err_intr_handler,
 			  0, nic->irq_name[irq], nic);
-	if (!ret)
+	if (!ret) {
 		nic->irq_allocated[irq] = true;
-
-err:
-	if (ret)
-		netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq);
-
+		return 0;
+	}
+report_failure:
+	netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq);
 	return ret;
 }

Quoted 3 lines

Quoted 2 lines

Adjust a jump target to eliminate a check before error logging. Use the identifier "report_failure" instead of "err".

I don't see much value in those changes

Thanks for your feedback.

Quoted 1 line

Using the 'err' label is ok as it is not misleading and common use.

Is such a short jump label enough explanation for the information "what" and "why"?

Quoted 2 lines

And, there is no need to optimize the check since this is not the fast path

Really? - Is it a bit more efficient to avoid a double check for the variable "ret" at the end of the current implementation for the discussed function?

Quoted 3 lines

Is it a bit more efficient to avoid a double check for the variable "ret" at the end of the current implementation for the discussed function?

Before asking questions you could answer yourself, please look at object code produced by the compiler before and after your proposed changes.

Quoted 7 lines

Quoted 3 lines

Is it a bit more efficient to avoid a double check for the variable "ret" at the end of the current implementation for the discussed function?

Before asking questions you could answer yourself, please look at object code produced by the compiler before and after your proposed changes.

* Do any more source code reviewers wonder about the need for such a double check?

* Which object code representations would you find representative for a further constructive discussion around this software component?

Quoted 3 lines

* Which object code representations would you find representative for a further constructive discussion around this software component?

Evidence of actual object code improvement

when with compiled with optimizations.

Quoted 5 lines

Quoted 3 lines

* Which object code representations would you find representative for a further constructive discussion around this software component?

Evidence of actual object code improvement

How do you think about to provide a function implementation which looks a bit more efficient by default?

Some prefer that source code be "templatized" regardless of the number of exit points that any particular use of a specific function type.

Some of your patches are converting these templatized functions to a different form for no added value.

These patches make the local source code inconsistent and generally goes against the authors preferred style.

Quoted 3 lines

Some prefer that source code be "templatized" regardless of the number of exit points that any particular use of a specific function type.

Quoted 2 lines

Some of your patches are converting these templatized functions to a different form for no added value.

Would you like to distinguish a bit more between my evolving collection of update suggestions and the concrete proposal for the function "nicvf_register_interrupts"?

Quoted 2 lines

These patches make the local source code inconsistent and generally goes against the authors preferred style.

Which programming approach will be the leading one here finally?

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 00:11:57 +0100

Remove two checks for null pointers which would be handled by usual error detection before.

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

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 11:16:04 +0100

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

* Return directly if a memory allocation failed at the beginning.

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

This issue was detected by using the Coccinelle software.

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

---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/net/ethernet/freescale/gianfar_ethtool.c +0 -0

index 4b0ee85..be4941e 100644

drivers/net/ethernet/freescale/gianfar_ethtool.c +6 -6

 	local_rqfcr = kmalloc_array(MAX_FILER_IDX + 1, sizeof(unsigned int),
 				    GFP_KERNEL);
-	if (!local_rqfpr || !local_rqfcr) {
+	if (!local_rqfcr) {
 		ret = 0;
-		goto err;
+		goto free_fpr;
 	}
 
 	switch (class) {

@@ -802,7 +804,7 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 		netdev_err(priv->ndev,
 			   "Right now this class is not supported\n");
 		ret = 0;
-		goto err;
+		goto free_fcr;
 	}
 
 	for (i = 0; i < MAX_FILER_IDX + 1; i++) {

@@ -819,7 +821,7 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 		netdev_err(priv->ndev,
 			   "No parse rule found, can't create hash rules\n");
 		ret = 0;
-		goto err;
+		goto free_fcr;
 	}
 
 	/* If a match was found, then it begins the starting of a cluster rule

@@ -862,9 +864,9 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 			break;
 		priv->cur_filer_idx = priv->cur_filer_idx - 1;
 	}
-
-err:
+free_fcr:
 	kfree(local_rqfcr);
+free_fpr:
 	kfree(local_rqfpr);
 	return ret;
 }

Signature · 6 lines

2.6.3

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

drivers/net/ethernet/freescale/gianfar_ethtool.c static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, · line 782

Quoted 7 lines

+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

drivers/net/ethernet/freescale/gianfar_ethtool.c +2 -0

@@ -778,11 +778,13 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 
 	local_rqfpr = kmalloc_array(MAX_FILER_IDX + 1, sizeof(unsigned int),
 				    GFP_KERNEL);
+	if (!local_rqfpr)
+		return 1;

Why return 1? Previously 0 was returned.

Normally, one returns -ENOMEM for this case, but it looks like this function is returning 0 on failure.

Quoted 9 lines

drivers/net/ethernet/freescale/gianfar_ethtool.c static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, · line 782

Quoted 7 lines

+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

drivers/net/ethernet/freescale/gianfar_ethtool.c +2 -0

@@ -778,11 +778,13 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 
 	local_rqfpr = kmalloc_array(MAX_FILER_IDX + 1, sizeof(unsigned int),
 				    GFP_KERNEL);
+	if (!local_rqfpr)
+		return 1;

Why return 1? Previously 0 was returned.

You are right. - Unfortunately, I made a mistake at this place of my update suggestion.

Quoted 2 lines

Normally, one returns -ENOMEM for this case, but it looks like this function is returning 0 on failure.

Should a symbol like "false" be used instead of such a special number?

Quoted 4 lines

Quoted 2 lines

Normally, one returns -ENOMEM for this case, but it looks like this function is returning 0 on failure.

Should a symbol like "false" be used instead of such a special number?

Maybe it's better than 0 and 1...

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

* Return directly if a memory allocation failed at the beginning.

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

This issue was detected by using the Coccinelle software.

Quoted 5 lines

* Return directly if a memory allocation failed at the beginning.

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

This issue was detected by using the Coccinelle software.

Is this really better?

Perhaps this particular static analysis isn't too useful.

Why not just allocate once and assign a second pointer?

	local_rqfpr = kmalloc_array(2 * (MAX_FILER_IDX + 1),
				    sizeof(unsigned int), GFP_KERNEL);
	if (!local_rqfpr)
		goto err;

	local_rqfcr = &local_rqfpr[MAX_FILER_IDX + 1];

Quoted 10 lines

Quoted 6 lines

* Return directly if a memory allocation failed at the beginning.

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

This issue was detected by using the Coccinelle software.

Is this really better?

Perhaps this particular static analysis isn't too useful.

The opinions are still evolving for such a kind of search pattern.

Quoted 2 lines

Why not just allocate once and assign a second pointer?

Quoted 6 lines

	local_rqfpr = kmalloc_array(2 * (MAX_FILER_IDX + 1),
				    sizeof(unsigned int), GFP_KERNEL);
	if (!local_rqfpr)
		goto err;

	local_rqfcr = &local_rqfpr[MAX_FILER_IDX + 1];

Do you suggest to use only one array (instead of two as before) here?

Quoted 8 lines

Quoted 6 lines

	local_rqfpr = kmalloc_array(2 * (MAX_FILER_IDX + 1),
				    sizeof(unsigned int), GFP_KERNEL);
	if (!local_rqfpr)
		goto err;

	local_rqfcr = &local_rqfpr[MAX_FILER_IDX + 1];

Do you suggest to use only one array (instead of two as before) here?

That's a possibility.

If, as your title suggests, you really want fewer function calls,

(which as far as I saw, you didn't do)

that could be a mechanism to remove both an allocation and a free.

-----Original Message-----
From: SF Markus Elfring [mailto:[email protected]]
Sent: Friday, January 15, 2016 7:33 PM
To: Joe Perches <[email protected]>; [email protected]
Cc: Claudiu Manoil <[email protected]>; LKML <linux- [email protected]>; [email protected]; Julia Lawall <[email protected]>
Subject: Re: gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

Quoted 10 lines

Quoted 8 lines

Quoted 6 lines

	local_rqfpr = kmalloc_array(2 * (MAX_FILER_IDX + 1),
				    sizeof(unsigned int), GFP_KERNEL);
	if (!local_rqfpr)
		goto err;

	local_rqfcr = &local_rqfpr[MAX_FILER_IDX + 1];

Do you suggest to use only one array (instead of two as before) here?

That's a possibility.

Thanks for your clarification.

Quoted 1 line

If, as your title suggests, you really want fewer function calls,

I am unsure at the moment if more changes will make sense in this function implementation.

Quoted 1 line

(which as far as I saw, you didn't do)

Is my wording "after error detection" insufficient eventually?

Quoted 1 line

that could be a mechanism to remove both an allocation and a free.

Would any more software developers or source code reviewers like to share their opinions in such a direction?

From: Markus Elfring <[email protected]>
Date: Fri, 15 Jan 2016 10:40:24 +0100

Omit explicit initialisation at the beginning for four local variables which are redefined before their first use.

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

---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/net/ethernet/freescale/gianfar_ethtool.c +0 -0

index 825b051..8302f7d 100644

drivers/net/ethernet/freescale/gianfar_ethtool.c static void ethflow_to_filer_rules (struct gfar_private *priv, u64 ethflow) · line 771

Quoted 7 lines

+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

drivers/net/ethernet/freescale/gianfar_ethtool.c +1 -1

@@ -768,12 +768,12 @@ static void ethflow_to_filer_rules (struct gfar_private *priv, u64 ethflow)
 static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 				       u64 class)
 {
-	unsigned int last_rule_idx = priv->cur_filer_idx;
+	unsigned int last_rule_idx;

This is a write only variable. We can just remove it.

Quoted 9 lines

drivers/net/ethernet/freescale/gianfar_ethtool.c static void ethflow_to_filer_rules (struct gfar_private *priv, u64 ethflow) · line 771

Quoted 7 lines

+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

drivers/net/ethernet/freescale/gianfar_ethtool.c +1 -1

@@ -768,12 +768,12 @@ static void ethflow_to_filer_rules (struct gfar_private *priv, u64 ethflow)
 static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 				       u64 class)
 {
-	unsigned int last_rule_idx = priv->cur_filer_idx;
+	unsigned int last_rule_idx;

This is a write only variable. We can just remove it.

Can a static source code analysis tool like the software "http://smatch.sourceforge.net/" detect that such a variable is not read by this function implementation so far?

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/net/ethernet/freescale/gianfar_ethtool.c?id·5ec3af27bf011a760e2f44eb25a99b6fbb0fb3#n850

Does this place indicate an unwanted value assignment as a leftover, or are there any other actions missing?

Quoted 9 lines

drivers/net/ethernet/freescale/gianfar_ethtool.c static void ethflow_to_filer_rules (struct gfar_private *priv, u64 ethflow) · line 771

Quoted 7 lines

+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

drivers/net/ethernet/freescale/gianfar_ethtool.c +1 -1

@@ -768,12 +768,12 @@ static void ethflow_to_filer_rules (struct gfar_private *priv, u64 ethflow)
 static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 				       u64 class)
 {
-	unsigned int last_rule_idx = priv->cur_filer_idx;
+	unsigned int last_rule_idx;

This is a write only variable. We can just remove it.

Can a static source code analysis tool like the software "http://smatch.sourceforge.net/" detect that such a variable is not read by this function implementation so far?

Quoted 4 lines

Quoted 1 line

This is a write only variable. We can just remove it.

Can a static source code analysis tool like the software "http://smatch.sourceforge.net/" detect that such a variable is not read by this function implementation so far?

No,

but a human can.

I am going to be honest, and say that I am completely ignoring most of your static checker patches.

You don't put enough care and consideration into them,

and I really don't have time to waste on looking at something like that.

From: SF Markus Elfring [mailto:elfring at users.sourceforge.net]

Hello,

I have taken another look at the implementation of the function "i40e_vc_get_vf_resources_msg". I find the use of the variable "i"
strange there. It seems that the value from the increment operation in an
if branch is not reused so far.

I would appreciate a further clarification.
Can this variable be eventually deleted?

From: SF Markus Elfring [mailto:elfring at users.sourceforge.net]
Sent: Friday, January 01, 2016 6:33 AM

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 15:11:09 +0100

Replace explicit initialisations for four local variables at the beginning by assignments that will only be performed if the corresponding code will really be executed.

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

---

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 15:11:09 +0100

Replace explicit initialisations for four local variables at the beginning
by assignments that will only be performed if the corresponding code will really be executed.

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

---
?drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 15 +++++++++---
---
?1 file changed, 9 insertions(+), 6 deletions(-)

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 17:32:07 +0100

Reduce the scope for the local variable "desc" to one branch of an if statement.

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 17:35:03 +0100

Omit explicit initialisation at the beginning for one local variable that is redefined before its first use.

Quoted 5 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 17:35:03 +0100

Omit explicit initialisation at the beginning for one local variable that is redefined before its first use.

This patch is unnecessary. The variable initialisation is redundant. See the difference? Sending an unnecessary patch causes unnecessary load on reviewers and maintainers. Keeping redundant code has no measurable cost, and can save the same maintainers a lot of trouble later.

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 19:00:53 +0100

Omit explicit initialisation at the beginning for one local variable that is redefined before its first use.

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

---
 drivers/net/wireless/ath/ath9k/hif_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/ath/ath9k/hif_usb.c +0 -0

index 165dd20..51bd61b 100644

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 19:09:32 +0100

Replace an explicit initialisation for one local variable at the beginning by a conditional assignment.

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

---
 drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/net/wireless/ath/ath9k/htc_drv_main.c +1 -1

index a680a97..30bd59e 100644
@@ -246,7 +246,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
 	struct ieee80211_conf *conf = &common->hw->conf;
 	bool fastcc;
 	struct ieee80211_channel *channel = hw->conf.chandef.chan;
-	struct ath9k_hw_cal_data *caldata = NULL;
+	struct ath9k_hw_cal_data *caldata;
 	enum htc_phymode mode;
 	__be16 htc_mode;
 	u8 cmd_rsp;

@@ -274,10 +274,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
 		priv->ah->curchan->channel,

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 19:09:32 +0100

Replace an explicit initialisation for one local variable at the beginning by a conditional assignment.

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

---
 drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/net/wireless/ath/ath9k/htc_drv_main.c +2 -6

index a680a97..30bd59e 100644
@@ -246,7 +246,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
        struct ieee80211_conf *conf = &common->hw->conf;
        bool fastcc;
        struct ieee80211_channel *channel = hw->conf.chandef.chan;
-       struct ath9k_hw_cal_data *caldata = NULL;
+       struct ath9k_hw_cal_data *caldata;
        enum htc_phymode mode;
        __be16 htc_mode;
        u8 cmd_rsp;

@@ -274,10 +274,7 @@ static int ath9k_htc_set_channel(struct ath9k_htc_priv *priv,
                priv->ah->curchan->channel,
                channel->center_freq, conf_is_ht(conf), conf_is_ht40(conf),
                fastcc);
-
-       if (!fastcc)
-               caldata = &priv->caldata;
-
+       caldata = fastcc ? NULL : &priv->caldata;
        ret = ath9k_hw_reset(ah, hchan, caldata, fastcc);
        if (ret) {
                ath_err(common,
--

2.6.3

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

Hi Kalle,

Quoted 3 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 19:09:32 +0100

Quoted 4 lines

Replace an explicit initialisation for one local variable at the beginning by a conditional assignment.

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

This looks sane to me.

Reviewed-by Julian Calaby <[email protected]>

Quoted 12 lines · Julian Calaby

Hi Kalle,

Quoted 7 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 19:09:32 +0100

Replace an explicit initialisation for one local variable at the beginning by a conditional assignment.

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

Quoted 3 lines

This looks sane to me.

Reviewed-by Julian Calaby <[email protected]>

Before I commit I'll just change the commit title to:

ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

Quoted 3 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 19:09:32 +0100

Quoted 4 lines

Replace an explicit initialisation for one local variable at the beginning by a conditional assignment.

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

This looks sane to me.

Reviewed-by Julian Calaby <[email protected]>

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 20:20:15 +0100

Omit explicit initialisation at the beginning for one local variable that is redefined before its first use.

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

---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +1 -1

index ceb2a75..c21eeb1 100644
@@ -3260,7 +3260,7 @@ static int brcmf_sdio_download_firmware(struct brcmf_sdio *bus,
  					const struct firmware *fw,
  					void *nvram, u32 nvlen)
  {
-	int bcmerror = -EFAULT;
+	int bcmerror;
  	u32 rstvec;
 
  	sdio_claim_host(bus->sdiodev->func[1]);

Quoted 2 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Fri, 1 Jan 2016 20:20:15 +0100

I think it has been said over and over, but please use driver name only as prefix. I don't see value to prepend it with 'net-'.