Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310013 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 0/3] gianfar: Fine-tuning for gfar_ethflow_to_filer_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-15 11:10 +0100
    [PATCH v3 3/3] gianfar: Extend an initialisation clause of a for loop  in gfar_ethflow_to_filer_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-15 11:20 +0100
    [PATCH v3 2/3] gianfar: Delete unnecessary variable initialisations  in gfar_ethflow_to_filer_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-15 11:20 +0100
      Re: [PATCH v3 2/3] gianfar: Delete unnecessary variable  initialisations in gfar_ethflow_to_filer_table() Dan Carpenter <dan.carpenter@oracle.com> - 2016-01-15 11:40 +0100
        Re: [PATCH v3 2/3] gianfar: Delete unnecessary variable  initialisations in gfar_ethflow_to_filer_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-15 12:40 +0100
          Re: [PATCH v3 2/3] gianfar: Delete unnecessary variable  initialisations in gfar_ethflow_to_filer_table() Dan Carpenter <dan.carpenter@oracle.com> - 2016-01-15 13:20 +0100
          Re: [PATCH v3 2/3] gianfar: Delete unnecessary variable  initialisations in gfar_ethflow_to_filer_table() David Miller <davem@davemloft.net> - 2016-01-15 17:50 +0100
            Re: gianfar: Delete unnecessary variable initialisations in  gfar_ethflow_to_filer_table() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-15 18:20 +0100
    [PATCH v3 1/3] gianfar: Less function calls in  gfar_ethflow_to_filer_table() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-15 11:20 +0100
      Re: [PATCH v3 1/3] gianfar: Less function calls in  gfar_ethflow_to_filer_table() after error detection Joe Perches <joe@perches.com> - 2016-01-15 11:40 +0100
        Re: gianfar: Less function calls in gfar_ethflow_to_filer_table()  after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-15 12:50 +0100
          Re: gianfar: Less function calls in gfar_ethflow_to_filer_table()  after error detection Joe Perches <joe@perches.com> - 2016-01-15 13:10 +0100
            Re: gianfar: Less function calls in gfar_ethflow_to_filer_table()  after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-15 18:40 +0100
              RE: gianfar: Less function calls in gfar_ethflow_to_filer_table()  after error detection Claudiu Manoil <claudiu.manoil@nxp.com> - 2016-01-18 15:50 +0100

#1310013 — [PATCH v3 0/3] gianfar: Fine-tuning for gfar_ethflow_to_filer_table()

From: Markus Elfring <elfring@users.sourceforge.net>
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

[toc] | [next] | [standalone]

#1310018 — [PATCH v3 3/3] gianfar: Extend an initialisation clause of a for loop in gfar_ethflow_to_filer_table()

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

#1310020 — [PATCH v3 2/3] gianfar: Delete unnecessary variable initialisations in gfar_ethflow_to_filer_table()

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

#1310042 — Re: [PATCH v3 2/3] gianfar: Delete unnecessary variable initialisations in gfar_ethflow_to_filer_table()

On Fri, Jan 15, 2016 at 11:12:42AM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---
>  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;

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

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1310065 — Re: [PATCH v3 2/3] gianfar: Delete unnecessary variable initialisations in gfar_ethflow_to_filer_table()

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

[toc] | [prev] | [next] | [standalone]

#1310078 — Re: [PATCH v3 2/3] gianfar: Delete unnecessary variable initialisations in gfar_ethflow_to_filer_table()

On Fri, Jan 15, 2016 at 12:34:33PM +0100, SF Markus Elfring wrote:
> >> +++ 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

[toc] | [prev] | [next] | [standalone]

#1310269 — Re: [PATCH v3 2/3] gianfar: Delete unnecessary variable initialisations in gfar_ethflow_to_filer_table()

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 15 Jan 2016 12:34:33 +0100

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

[toc] | [prev] | [next] | [standalone]

#1310290 — Re: gianfar: Delete unnecessary variable initialisations in gfar_ethflow_to_filer_table()

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

[toc] | [prev] | [next] | [standalone]

#1310022 — [PATCH v3 1/3] gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

#1310041 — Re: [PATCH v3 1/3] gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

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

[toc] | [prev] | [next] | [standalone]

#1310071 — Re: gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

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

[toc] | [prev] | [next] | [standalone]

#1310076 — Re: gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

On Fri, 2016-01-15 at 12:47 +0100, SF Markus Elfring wrote:
> > > * 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?

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.

[toc] | [prev] | [next] | [standalone]

#1310306 — Re: gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

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

[toc] | [prev] | [next] | [standalone]

#1311589 — RE: gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

>-----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 <claudiu.manoil@freescale.com>; LKML <linux-
>kernel@vger.kernel.org>; kernel-janitors@vger.kernel.org; Julia Lawall
><julia.lawall@lip6.fr>
>Subject: Re: gianfar: Less function calls in gfar_ethflow_to_filer_table() after
>error detection
>
>>>> 	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?
>

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web