[PATCH 0/3] net-gianfar: Fine-tuning for gfar_ethflow_to_filer_table()

From: Markus Elfring <redacted>
Date: Fri, 1 Jan 2016 13:15:34 +0100

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

Markus Elfring (3):
  Less function calls after error detection
  Delete unnecessary variable initialisations
  Extend an initialisation clause of a for loop

 drivers/net/ethernet/freescale/gianfar_ethtool.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

-- 
2.6.3

From: Markus Elfring <redacted>
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 <redacted>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 4b0ee85..be4941e 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -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;
 	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;
 }

-- 
2.6.3

From: Markus Elfring <redacted>
Date: Fri, 1 Jan 2016 12:56:23 +0100

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

Signed-off-by: Markus Elfring <redacted>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index be4941e..508be89 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -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;
 	unsigned int cmp_rqfpr;
 	unsigned int *local_rqfpr;
 	unsigned int *local_rqfcr;
-	int i = 0x0, k = 0x0;
-	int j = MAX_FILER_IDX, l = 0x0;
+	int i, k, l;
+	int j = MAX_FILER_IDX;
 	int ret = 1;
 
 	local_rqfpr = kmalloc_array(MAX_FILER_IDX + 1, sizeof(unsigned int),

-- 
2.6.3

From: Markus Elfring <redacted>
Date: Fri, 1 Jan 2016 13:00:06 +0100

Move the assignment for the variable "j" from the beginning
into an initialisation clause of a for loop.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 508be89..6a7b035 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -772,8 +772,7 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 	unsigned int cmp_rqfpr;
 	unsigned int *local_rqfpr;
 	unsigned int *local_rqfcr;
-	int i, k, l;
-	int j = MAX_FILER_IDX;
+	int i, j, k, l;
 	int ret = 1;
 
 	local_rqfpr = kmalloc_array(MAX_FILER_IDX + 1, sizeof(unsigned int),

@@ -807,7 +806,7 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 		goto free_fcr;
 	}
 
-	for (i = 0; i < MAX_FILER_IDX + 1; i++) {
+	for (i = 0, j = MAX_FILER_IDX; i < MAX_FILER_IDX + 1; i++) {
 		local_rqfpr[j] = priv->ftp_rqfpr[i];
 		local_rqfcr[j] = priv->ftp_rqfcr[i];
 		j--;

-- 
2.6.3

On Fri, 1 Jan 2016, SF Markus Elfring wrote:

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.

julia

quoted

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

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

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?

Regards,
Markus

From: Markus Elfring <redacted>
Date: Fri, 1 Jan 2016 13:56:09 +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 <redacted>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 4b0ee85..825b051 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -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 0;
 	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;
 }

-- 
2.6.3

On Fri, 1 Jan 2016, SF Markus Elfring wrote:

quoted

quoted

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

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

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

julia

Julia Lawall [off-list ref] :

On Fri, 1 Jan 2016, SF Markus Elfring wrote:

[...]

quoted

quoted

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

Your suggestion about -ENOMEM is consistent with the callchain. Nothing
else is needed.

Btw:
1. kfree does not care about NULL parameter, especially in this hardly
   timing sensitive path.
2. kmalloc_array for small kernel controlled arrays of integers (see
   drivers/net/ethernet/freescale/gianfar.h), seriously ?

   I'd suggest the janitor to introduce a dedicated struct to embed both
   gfar_private.ftp_rqf{p, c}r then use a single, plain kmalloc in
   gfar_ethflow_to_filer_table.

Happy tasteful 2016 :o)

-- 
Ueimor

This is not the proper way to resubmit patches when you are asked
to make changes to some portion of a multi-patch series.

You must always resubmit the entire series when this happens,
not just the patch that changes.

And in the revised cover "0/N" posting you list the revisions
that were made.

From: Markus Elfring <redacted>
Date: Fri, 15 Jan 2016 11:05:43 +0100

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

Markus Elfring (3):
  Less function calls after error detection
  Delete unnecessary variable initialisations
  Extend an initialisation clause of a for loop

---

v3: Rebase proposed changes on the source files for the software
    "Linux next-20160114".
    
v2: Unfortunately, an inappropriate return code was selected in the first
    update step from this series.
    Thus fix that.

 drivers/net/ethernet/freescale/gianfar_ethtool.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

-- 
2.6.3

From: Markus Elfring <redacted>
Date: Fri, 15 Jan 2016 10:30:37 +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 <redacted>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 4b0ee85..825b051 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -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 0;
 	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;
 }

-- 
2.6.3

From: Markus Elfring <redacted>
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 <redacted>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 825b051..8302f7d 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -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;
 	unsigned int cmp_rqfpr;
 	unsigned int *local_rqfpr;
 	unsigned int *local_rqfcr;
-	int i = 0x0, k = 0x0;
-	int j = MAX_FILER_IDX, l = 0x0;
+	int i, k, l;
+	int j = MAX_FILER_IDX;
 	int ret = 1;
 
 	local_rqfpr = kmalloc_array(MAX_FILER_IDX + 1, sizeof(unsigned int),

-- 
2.6.3

From: Markus Elfring <redacted>
Date: Fri, 15 Jan 2016 10:50:34 +0100

Move the assignment for the variable "j" from the beginning
into an initialisation clause of a for loop.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 8302f7d..2162adc 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c

@@ -772,8 +772,7 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 	unsigned int cmp_rqfpr;
 	unsigned int *local_rqfpr;
 	unsigned int *local_rqfcr;
-	int i, k, l;
-	int j = MAX_FILER_IDX;
+	int i, j, k, l;
 	int ret = 1;
 
 	local_rqfpr = kmalloc_array(MAX_FILER_IDX + 1, sizeof(unsigned int),

@@ -807,7 +806,7 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow,
 		goto free_fcr;
 	}
 
-	for (i = 0; i < MAX_FILER_IDX + 1; i++) {
+	for (i = 0, j = MAX_FILER_IDX; i < MAX_FILER_IDX + 1; i++) {
 		local_rqfpr[j] = priv->ftp_rqfpr[i];
 		local_rqfcr[j] = priv->ftp_rqfcr[i];
 		j--;

-- 
2.6.3

On Fri, Jan 15, 2016 at 11:12:42AM +0100, SF Markus Elfring wrote:

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

regards,
dan carpenter

On Fri, 2016-01-15 at 11:11 +0100, SF Markus Elfring wrote:

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.

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

Perhaps this would be better removing the ret variable
and using something like:

int gfar_ethflow_to_filer_table(...)
{
	...

	return 0;

err:
	kfree(local_rqfpt);
	return 1;
}

quoted

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

@@ -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=b75ec3af27bf011a760e2f44eb25a99b6fbb0fb3#n850

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

Regards,
Markus

quoted

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

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

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

Regards,
Markus

On Fri, 2016-01-15 at 12:47 +0100, SF Markus Elfring wrote:

quoted

quoted

* 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

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

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.

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

On Fri, Jan 15, 2016 at 12:34:33PM +0100, SF Markus Elfring wrote:

quoted

quoted

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

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

Yeah.  That's a good idea.  I will do that.

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

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

I think it's just an extra variable and you can just delete it.

regards,
dan carpenter

From: SF Markus Elfring <redacted>
Date: Fri, 15 Jan 2016 12:34:33 +0100

quoted

quoted

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

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

No, but a human can.

And a human should fully analyze any change he writes based upon static
analysis tool results.

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.

quoted

quoted

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,

I imagine that there are a few tools available which can point such update candidates out.
There are various software development challenges to consider.

but a human can.

Some software developers and source code reviewers are struggling with mentioned
implementation details as usual. Do they also wonder how the discussed variable assignment
was left over in a specific function?

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

I am curious if you would reconsider the affected source code places once more
when you will be notified about related issues by other tools or persons.

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

Would you like to explain this impression a bit more?

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

Thanks for your feedback.

Various open issues are competing for our attention as usual.

Regards,
Markus

quoted

quoted

	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.

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.

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

Is my wording "after error detection" insufficient eventually?

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?

Regards,
Markus

-----Original Message-----
From: SF Markus Elfring [mailto:elfring@users.sourceforge.net]
Sent: Friday, January 15, 2016 7:33 PM
To: Joe Perches <joe@perches.com>; netdev@vger.kernel.org
Cc: Claudiu Manoil <redacted>; LKML <linux-
kernel@vger.kernel.org>; kernel-janitors@vger.kernel.org; Julia Lawall
[off-list ref]
Subject: Re: gianfar: Less function calls in gfar_ethflow_to_filer_table() after
error detection

quoted

quoted

quoted

	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

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

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

Is my wording "after error detection" insufficient eventually?

quoted

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?

Hi,
This kind of fixes are net-next stuff at best, no need to push them into
the net tree right now.
So please wait with these submissions until net-next re-opens at least.
Thanks.