[PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection

[PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index c24cb2a..21e1579 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -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;
 }
 
-- 
2.6.3

Re: [PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection

[Robert Richter]

On 31.12.15 22:47:31, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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".

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. So let's keep the code as it is with the
flavor of the original author.

-Robert

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/ethernet/cavium/thunder/nicvf_main.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index c24cb2a..21e1579 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -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;
>  }

Re: [PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection

[SF Markus Elfring]

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


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


> 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?
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/net/ethernet/cavium/thunder/nicvf_main.c?id=40fb5f8a60f33133d36afde35a9ad865d35e4423#n940


> and will be compiler optimized anyway.

How sure are you about automatic software optimisations?

Can it occasionally help to jump to the really intended source code
location directly?


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

Re: [PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection

[Joe Perches]

On Thu, 2016-01-07 at 20:30 +0100, SF Markus Elfring wrote:
> > > 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.
> > 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"?

When there is only one type of error possible, yes.

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

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

Re: net-thunder: One check less in nicvf_register_interrupts() after error detection

[SF Markus Elfring]

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

Regards,
Markus

Re: net-thunder: One check less in nicvf_register_interrupts() after error detection

[Joe Perches]

On Thu, 2016-01-07 at 20:56 +0100, SF Markus Elfring wrote:
> > > 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?

Given the feedback you've already received,
it seems so.

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

Re: net-thunder: One check less in nicvf_register_interrupts() after error detection

[SF Markus Elfring]

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


> when with compiled with optimizations.

Which combinations of hardware and software would you recommend
for corresponding system checks?

Regards,
Markus

Re: net-thunder: One check less in nicvf_register_interrupts() after error detection

[Joe Perches]

On Thu, 2016-01-07 at 21:07 +0100, SF Markus Elfring wrote:
> > > * 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?

It's not a matter of "looks a bit more efficient".
it's taste, style, and repetition for various functions.

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.

Re: net-thunder: One check less in nicvf_register_interrupts() after error detection

[SF Markus Elfring]

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

This is another interesting view on involved implementation details.


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


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

Regards,
Markus

Re: net-thunder: One check less in nicvf_register_interrupts() after error detection

[Joe Perches]

On Thu, 2016-01-07 at 21:38 +0100, SF Markus Elfring wrote:
> > 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.
> 
> 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"?

No.

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

Whatever the developer wants.

There is no _best_ or _only_ style for this.

[PATCH] be2net: Delete an unnecessary check in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 drivers/net/ethernet/emulex/benet/be_cmds.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
index b63d8ad..ba98297 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.c
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
@@ -4366,9 +4366,7 @@ int be_cmd_get_profile_config(struct be_adapter *adapter,
 	if (vf_res)
 		res->vf_if_cap_flags = vf_res->cap_flags;
 err:
-	if (cmd.va)
-		dma_free_coherent(&adapter->pdev->dev, cmd.size, cmd.va,
-				  cmd.dma);
+	dma_free_coherent(&adapter->pdev->dev, cmd.size, cmd.va, cmd.dma);
 	return status;
 }
 
@@ -4398,10 +4396,7 @@ static int be_cmd_set_profile_config(struct be_adapter *adapter, void *desc,
 	memcpy(req->desc, desc, size);
 
 	status = be_cmd_notify_wait(adapter, &wrb);
-
-	if (cmd.va)
-		dma_free_coherent(&adapter->pdev->dev, cmd.size, cmd.va,
-				  cmd.dma);
+	dma_free_coherent(&adapter->pdev->dev, cmd.size, cmd.va, cmd.dma);
 	return status;
 }
 
-- 
2.6.3

Re: [PATCH] be2net: Delete an unnecessary check in two functions

[Sathya Perla]

On Fri, Jan 1, 2016 at 4:52 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>

Acked-by: Sathya Perla <sathya.perla@avagotech.com>

This patch is suitable for the net-next tree. Thanks!

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

[SF Markus Elfring]

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

[PATCH 1/3] net-gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

[SF Markus Elfring]

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

Re: [PATCH 1/3] net-gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

[Julia Lawall]

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

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

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

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

Re: [PATCH 1/3] net-gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

[SF Markus Elfring]

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

Re: [PATCH 1/3] net-gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

[Julia Lawall]

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

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

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

julia

Re: [PATCH 1/3] net-gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

[Francois Romieu]

Julia Lawall <julia.lawall@lip6.fr> :
> On Fri, 1 Jan 2016, SF Markus Elfring wrote:
[...]
> > > 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

[PATCH v2 1/3] net-gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

[SF Markus Elfring]

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

Re: [PATCH v2 1/3] net-gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection

[David Miller]

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.

[PATCH 2/3] net-gianfar: Delete unnecessary variable initialisations in gfar_ethflow_to_filer_table()

[SF Markus Elfring]

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

[PATCH 3/3] net-gianfar: Extend an initialisation clause of a for loop in gfar_ethflow_to_filer_table()

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

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

[Joe Perches]

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

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

[SF Markus Elfring]

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

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

[Joe Perches]

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.

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

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

[SF Markus Elfring]

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

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

[Claudiu Manoil]

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

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

[SF Markus Elfring]

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

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

[Dan Carpenter]

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

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

[SF Markus Elfring]

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

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

[Dan Carpenter]

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

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

[David Miller]

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.

Re: gianfar: Delete unnecessary variable initialisations in gfar_ethflow_to_filer_table()

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

[PATCH] net-i40e: Replace variable initialisations by assignments in i40e_vc_get_vf_resources_msg()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index aa58a49..e0874f5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -1172,16 +1172,18 @@ static int i40e_vc_get_version_msg(struct i40e_vf *vf, u8 *msg)
  **/
 static int i40e_vc_get_vf_resources_msg(struct i40e_vf *vf, u8 *msg)
 {
-	struct i40e_virtchnl_vf_resource *vfres = NULL;
-	struct i40e_pf *pf = vf->pf;
-	i40e_status aq_ret = 0;
+	struct i40e_virtchnl_vf_resource *vfres;
+	struct i40e_pf *pf;
+	i40e_status aq_ret;
 	struct i40e_vsi *vsi;
-	int i = 0, len = 0;
+	int i = 0;
 	int num_vsis = 1;
-	int ret;
+	int len, ret;
 
 	if (!test_bit(I40E_VF_STAT_INIT, &vf->vf_states)) {
 		aq_ret = I40E_ERR_PARAM;
+		vfres = NULL;
+		len = 0;
 		goto err;
 	}
 
@@ -1202,6 +1204,7 @@ static int i40e_vc_get_vf_resources_msg(struct i40e_vf *vf, u8 *msg)
 				  I40E_VIRTCHNL_VF_OFFLOAD_VLAN;
 
 	vfres->vf_offload_flags = I40E_VIRTCHNL_VF_OFFLOAD_L2;
+	pf = vf->pf;
 	vsi = pf->vsi[vf->lan_vsi_idx];
 	if (!vsi->info.pvid)
 		vfres->vf_offload_flags |= I40E_VIRTCHNL_VF_OFFLOAD_VLAN;
@@ -1231,7 +1234,7 @@ static int i40e_vc_get_vf_resources_msg(struct i40e_vf *vf, u8 *msg)
 		i++;
 	}
 	set_bit(I40E_VF_STAT_ACTIVE, &vf->vf_states);
-
+	aq_ret = 0;
 err:
 	/* send the response back to the VF */
 	ret = i40e_vc_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_GET_VF_RESOURCES,
-- 
2.6.3

net-i40e: Reconsider further usage of variable "i" in i40e_vc_get_vf_resources_msg()

[SF Markus Elfring]

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?

Regards,
Markus

RE: net-i40e: Reconsider further usage of variable "i" in i40e_vc_get_vf_resources_msg()

[Nelson, Shannon]

> From: SF Markus Elfring [mailto:elfring@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?

Thanks for pointing this out.  We've got an internal patch coming that will address this.

Cheers,
sln

RE: [PATCH] net-i40e: Replace variable initialisations by assignments in i40e_vc_get_vf_resources_msg()

[Nelson, Shannon]

> From: SF Markus Elfring [mailto:elfring@users.sourceforge.net]
> Sent: Friday, January 01, 2016 6:33 AM
> 
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---

This seems to me to be unnecessary fussing with the code.

sln

Re: [PATCH] net-i40e: Replace variable initialisations by assignments in i40e_vc_get_vf_resources_msg()

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On Fri, 2016-01-01 at 15:32 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 15 +++++++++---
> ---
>  1 file changed, 9 insertions(+), 6 deletions(-)

Dropping this patch based on feedback from Shannon.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] net-huawei_cdc_ncm: Delete an unnecessary variable initialisation in huawei_cdc_ncm_bind()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 16:54:13 +0100

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/usb/huawei_cdc_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/huawei_cdc_ncm.c b/drivers/net/usb/huawei_cdc_ncm.c
index 2680a65..53caeb3 100644
--- a/drivers/net/usb/huawei_cdc_ncm.c
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -71,7 +71,7 @@ static int huawei_cdc_ncm_bind(struct usbnet *usbnet_dev,
 {
 	struct cdc_ncm_ctx *ctx;
 	struct usb_driver *subdriver = ERR_PTR(-ENODEV);
-	int ret = -ENODEV;
+	int ret;
 	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
 	int drvflags = 0;
 
-- 
2.6.3

[PATCH 0/2] net-qmi_wwan: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Date: Fri, 1 Jan 2016 17:47:46 +0100

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

Markus Elfring (2):
  Refactoring for qmi_wwan_bind()
  Delete an unnecessary variable initialisation in qmi_wwan_register_subdriver()

 drivers/net/usb/qmi_wwan.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.6.3

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

[PATCH 1/2] net-qmi_wwan: Refactoring for qmi_wwan_bind()

[SF Markus Elfring]

From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Date: Fri, 1 Jan 2016 17:32:07 +0100

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

Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/net/usb/qmi_wwan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index d0b2973..5b8af06 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -345,7 +345,6 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
 	int status = -1;
 	u8 *buf = intf->cur_altsetting->extra;
 	int len = intf->cur_altsetting->extralen;
-	struct usb_interface_descriptor *desc = &intf->cur_altsetting->desc;
 	struct usb_cdc_union_desc *cdc_union;
 	struct usb_cdc_ether_desc *cdc_ether;
 	struct usb_driver *driver = driver_of(intf);
@@ -366,6 +365,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
 
 	/* Use separate control and data interfaces if we found a CDC Union */
 	if (cdc_union) {
+		struct usb_interface_descriptor *desc = &intf->cur_altsetting->desc;
+
 		info->data = usb_ifnum_to_if(dev->udev,
 					     cdc_union->bSlaveInterface0);
 		if (desc->bInterfaceNumber != cdc_union->bMasterInterface0 ||
-- 
2.6.3

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

Re: [PATCH 1/2] net-qmi_wwan: Refactoring for qmi_wwan_bind()

[Bjørn Mork]

SF Markus Elfring <elfring@users.sourceforge.net> writes:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 17:32:07 +0100
>
> Reduce the scope for the local variable "desc" to one branch
> of an if statement.

This patch is harmless.  But is also pointless.

You could at least try to explain why this must be changed.  I'm not
interested in why you think it is better this way - I might agree with
that.  what I am interested in is the advantage changing the code gives
us.  Some analysis of the risk and work involved would also be nice.  Is
this change really worth it?

Personally I am convinced that I wasted any time I used writing this,
and you wasted any time you used reading it.  Sorry.

Note:  This patch would have been fine if it was a natural part of some
*improvement* of the driver, i.e. a bugfix or feaure addition. As a
standalone patch I see it as noise.

Please stop the noise and start writing something useful.  I'm sure you
can fix bugs instead.  Wouldn't that be more interesting?  More
challenging?  These mindless robotic code refactoring patches are really
best left for robots.




Bjørn
--
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

[PATCH 2/2] net-qmi_wwan: Delete an unnecessary variable initialisation in qmi_wwan_register_subdriver()

[SF Markus Elfring]

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/usb/qmi_wwan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 5b8af06..5962099 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -286,7 +286,7 @@ static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on)
 static int qmi_wwan_register_subdriver(struct usbnet *dev)
 {
 	int rv;
-	struct usb_driver *subdriver = NULL;
+	struct usb_driver *subdriver;
 	struct qmi_wwan_state *info = (void *)&dev->data;
 
 	/* collect bulk endpoints */
-- 
2.6.3

Re: [PATCH 2/2] net-qmi_wwan: Delete an unnecessary variable initialisation in qmi_wwan_register_subdriver()

[Bjørn Mork]

SF Markus Elfring <elfring@users.sourceforge.net> writes:

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

I'd like to keep this particular redundant initialisation as a safe
guard against future code refactoring, causing for example the err label
to move up.  Yes, I do understand that any patch with such a bug should
be rejected, but I do know what happens in the real world and how easy
it is for something like that to slip through in a stream of unnecessary
"cleanup" patches.

Reducing redundancy in the kernel is only making the code less robust.
It is harmful. Please stop.  Thanks.


Bjørn
--
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

Re: [PATCH 2/2] net-qmi_wwan: Delete an unnecessary variable initialisation in qmi_wwan_register_subdriver()

[David Miller]

From: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
Date: Sat, 02 Jan 2016 22:30:48 +0100

> SF Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> writes:
> 
>> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>> 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.

+1

I'm getting really tired of these changes.  And a lot of them are
coming in right now...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] net-ath9k_htc: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Date: Fri, 1 Jan 2016 19:16:05 +0100

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

Markus Elfring (2):
  Delete an unnecessary variable initialisation in ath9k_hif_usb_rx_stream()
  Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

 drivers/net/wireless/ath/ath9k/hif_usb.c      | 2 +-
 drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

-- 
2.6.3

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

[PATCH 1/2] net-ath9k_htc: Delete an unnecessary variable initialisation in ath9k_hif_usb_rx_stream()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 drivers/net/wireless/ath/ath9k/hif_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 165dd20..51bd61b 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -525,7 +525,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
 				    struct sk_buff *skb)
 {
 	struct sk_buff *nskb, *skb_pool[MAX_PKT_NUM_IN_TRANSFER];
-	int index = 0, i = 0, len = skb->len;
+	int index = 0, i, len = skb->len;
 	int rx_remain_len, rx_pkt_len;
 	u16 pool_index = 0;
 	u8 *ptr;
-- 
2.6.3

Re: [PATCH 1/2] net-ath9k_htc: Delete an unnecessary variable initialisation in ath9k_hif_usb_rx_stream()

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

Am 01.01.2016 um 19:23 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> 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 <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  drivers/net/wireless/ath/ath9k/hif_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
> index 165dd20..51bd61b 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.c
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
> @@ -525,7 +525,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
>  				    struct sk_buff *skb)
>  {
>  	struct sk_buff *nskb, *skb_pool[MAX_PKT_NUM_IN_TRANSFER];
> -	int index = 0, i = 0, len = skb->len;
> +	int index = 0, i, len = skb->len;
>  	int rx_remain_len, rx_pkt_len;
>  	u16 pool_index = 0;
>  	u8 *ptr;
> 


Reviewed-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

[PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index a680a97..30bd59e 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -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

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

Am 01.01.2016 um 19:25 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> index a680a97..30bd59e 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> @@ -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,
> 

Reviewed-by: Oleksij Rempel <linux@rempel-privat.de>

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Julian Calaby]

Hi Kalle,

On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>

This looks sane to me.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

Thanks,

Julian Calaby

> ---
>  drivers/net/wireless/ath/ath9k/htc_drv_main.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> index a680a97..30bd59e 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
> @@ -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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> Hi Kalle,
>
> On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> 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 <elfring@users.sourceforge.net>
>
> This looks sane to me.
>
> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

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

-- 
Kalle Valo

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Julian Calaby]

Hi Kalle,

On Fri, Apr 15, 2016 at 10:09 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Julian Calaby <julian.calaby@gmail.com> writes:
>
>> Hi Kalle,
>>
>> On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
>> <elfring@users.sourceforge.net> wrote:
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> 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 <elfring@users.sourceforge.net>
>>
>> This looks sane to me.
>>
>> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>
>
> 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()

Sounds good to me.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel()

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> On Sat, Jan 2, 2016 at 5:25 AM, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> 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 <elfring@users.sourceforge.net>
>
> This looks sane to me.
>
> Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

Applied, thanks.

-- 
Kalle Valo

[PATCH] net-brcmfmac: Delete an unnecessary variable initialisation in brcmf_sdio_download_firmware()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index ceb2a75..c21eeb1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -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]);
-- 
2.6.3

Re: [PATCH] net-brcmfmac: Delete an unnecessary variable initialisation in brcmf_sdio_download_firmware()

[Arend van Spriel]

On 01/01/2016 08:26 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> 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-'.

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

That being said here is my....

Acked-by: Arend van Spriel <arend-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index ceb2a75..c21eeb1 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -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]);
>

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

Re: [PATCH] net-brcmfmac: Delete an unnecessary variable initialisation in brcmf_sdio_download_firmware()

[Kalle Valo]

Arend van Spriel <arend@broadcom.com> writes:

> On 01/01/2016 08:26 PM, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> 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-'.

Yes, please use existing naming schemes. This time I can fix it before
I commit the patch, but in the future please use correct prefixes. It's
easy to check what has been used previously:

$ git log --oneline --no-merges --follow drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | head -10
63ce3d5db093 brcmfmac: use msecs_to_jiffies() in macro definitions
4011fc499690 brcmfmac: change brcmf_sdio_wd_timer() prototype
a7decc44a002 brcmfmac: fix waitqueue_active without memory barrier in brcmfmac driver
46d703a77539 brcmfmac: Unify methods to define and map firmware files.
64d66c30c37e brcmfmac: no retries on rxglom superframe errors
6866a64a0f9b brcmfmac: constify brcmf_bus_ops structures
05491d2ccf20 brcm80211: move under broadcom vendor directory
ff4445a8502c brcmfmac: expose device memory to devcoredump subsystem
a32be0177252 brcmfmac: include linux/atomic.h
9d6c1dc4f913 brcmfmac: add dedicated debug level for firmware console logging
$

-- 
Kalle Valo

[PATCH 0/3] net-iwlegacy: Fine-tuning for il_eeprom_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 21:25:43 +0100

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

Markus Elfring (3):
  Refactoring
  One check less after error detection
  Another refactoring

 drivers/net/wireless/intel/iwlegacy/common.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

-- 
2.6.3

[PATCH 1/3] net-iwlegacy: Refactoring for il_eeprom_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 20:54:25 +0100

Return directly if a memory allocation failed at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intel/iwlegacy/common.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index eb5cb60..c3afaf7 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -723,10 +723,9 @@ il_eeprom_init(struct il_priv *il)
 	sz = il->cfg->eeprom_size;
 	D_EEPROM("NVM size = %d\n", sz);
 	il->eeprom = kzalloc(sz, GFP_KERNEL);
-	if (!il->eeprom) {
-		ret = -ENOMEM;
-		goto alloc_err;
-	}
+	if (!il->eeprom)
+		return -ENOMEM;
+
 	e = (__le16 *) il->eeprom;
 
 	il->ops->apm_init(il);
@@ -778,7 +777,6 @@ err:
 		il_eeprom_free(il);
 	/* Reset chip to save power until we load uCode during "up". */
 	il_apm_stop(il);
-alloc_err:
 	return ret;
 }
 EXPORT_SYMBOL(il_eeprom_init);
-- 
2.6.3

Re: [PATCH 1/3] net-iwlegacy: Refactoring for il_eeprom_init()

[Stanislaw Gruszka]

On Fri, Jan 01, 2016 at 09:30:10PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Date: Fri, 1 Jan 2016 20:54:25 +0100
> 
> Return directly if a memory allocation failed at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>

Acked-by: Stanislaw Gruszka <sgruszka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[PATCH 2/3] net-iwlegacy: One check less in il_eeprom_init() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 21:12:29 +0100

This issue was detected by using the Coccinelle software.

Adjust a jump target to avoid a check repetition before the function
call "il_eeprom_free".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intel/iwlegacy/common.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index c3afaf7..ae45fd3 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -734,7 +734,7 @@ il_eeprom_init(struct il_priv *il)
 	if (ret < 0) {
 		IL_ERR("EEPROM not found, EEPROM_GP=0x%08x\n", gp);
 		ret = -ENOENT;
-		goto err;
+		goto free_eeprom;
 	}
 
 	/* Make sure driver (instead of uCode) is allowed to read EEPROM */
@@ -742,7 +742,7 @@ il_eeprom_init(struct il_priv *il)
 	if (ret < 0) {
 		IL_ERR("Failed to acquire EEPROM semaphore.\n");
 		ret = -ENOENT;
-		goto err;
+		goto free_eeprom;
 	}
 
 	/* eeprom is an array of 16bit values */
@@ -772,9 +772,11 @@ il_eeprom_init(struct il_priv *il)
 done:
 	il->ops->eeprom_release_semaphore(il);
 
-err:
-	if (ret)
+	if (ret) {
+free_eeprom:
 		il_eeprom_free(il);
+	}
+
 	/* Reset chip to save power until we load uCode during "up". */
 	il_apm_stop(il);
 	return ret;
-- 
2.6.3

Re: [PATCH 2/3] net-iwlegacy: One check less in il_eeprom_init() after error detection

[Sergei Shtylyov]

Hello.

On 1/1/2016 11:31 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 21:12:29 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Adjust a jump target to avoid a check repetition before the function
> call "il_eeprom_free".

    One question: why?

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/net/wireless/intel/iwlegacy/common.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
> index c3afaf7..ae45fd3 100644
> --- a/drivers/net/wireless/intel/iwlegacy/common.c
> +++ b/drivers/net/wireless/intel/iwlegacy/common.c
[...]
> @@ -772,9 +772,11 @@ il_eeprom_init(struct il_priv *il)
>   done:
>   	il->ops->eeprom_release_semaphore(il);
>
> -err:
> -	if (ret)
> +	if (ret) {
> +free_eeprom:

    This is ugly, I'd say.

[...]

MBR, Sergei

[PATCH 3/3] net-iwlegacy: Another refactoring for il_eeprom_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 21:16:01 +0100

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intel/iwlegacy/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index ae45fd3..660ab2b 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -759,7 +759,7 @@ il_eeprom_init(struct il_priv *il)
 				 IL_EEPROM_ACCESS_TIMEOUT);
 		if (ret < 0) {
 			IL_ERR("Time out reading EEPROM[%d]\n", addr);
-			goto done;
+			goto release_semaphore;
 		}
 		r = _il_rd(il, CSR_EEPROM_REG);
 		e[addr / 2] = cpu_to_le16(r >> 16);
@@ -769,7 +769,7 @@ il_eeprom_init(struct il_priv *il)
 		 il_eeprom_query16(il, EEPROM_VERSION));
 
 	ret = 0;
-done:
+release_semaphore:
 	il->ops->eeprom_release_semaphore(il);
 
 	if (ret) {
-- 
2.6.3

Re: [PATCH 3/3] net-iwlegacy: Another refactoring for il_eeprom_init()

[Souptick Joarder]

On Sat, Jan 2, 2016 at 2:02 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 21:16:01 +0100
>
> Rename a jump label according to the current Linux coding style convention.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/intel/iwlegacy/common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
> index ae45fd3..660ab2b 100644
> --- a/drivers/net/wireless/intel/iwlegacy/common.c
> +++ b/drivers/net/wireless/intel/iwlegacy/common.c
> @@ -759,7 +759,7 @@ il_eeprom_init(struct il_priv *il)
>                                  IL_EEPROM_ACCESS_TIMEOUT);
>                 if (ret < 0) {
>                         IL_ERR("Time out reading EEPROM[%d]\n", addr);
> -                       goto done;
> +                       goto release_semaphore;

Current code looks good.
>                 }
>                 r = _il_rd(il, CSR_EEPROM_REG);
>                 e[addr / 2] = cpu_to_le16(r >> 16);
> @@ -769,7 +769,7 @@ il_eeprom_init(struct il_priv *il)
>                  il_eeprom_query16(il, EEPROM_VERSION));
>
>         ret = 0;
> -done:
> +release_semaphore:
>         il->ops->eeprom_release_semaphore(il);
>
>         if (ret) {
> --
> 2.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-Souptick

[PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 1 Jan 2016 22:27:20 +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 function call failed.
Use the identifier "report_failure" instead of the label "err".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/marvell/libertas/if_spi.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
index 82c0796..c9ae27e 100644
--- a/drivers/net/wireless/marvell/libertas/if_spi.c
+++ b/drivers/net/wireless/marvell/libertas/if_spi.c
@@ -880,18 +880,18 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
 				&hiStatus);
 	if (err) {
 		netdev_err(priv->dev, "I/O error\n");
-		goto err;
+		goto report_failure;
 	}
 
 	if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) {
 		err = if_spi_c2h_cmd(card);
 		if (err)
-			goto err;
+			goto report_failure;
 	}
 	if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) {
 		err = if_spi_c2h_data(card);
 		if (err)
-			goto err;
+			goto report_failure;
 	}
 
 	/*
@@ -940,9 +940,10 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
 	if (hiStatus & IF_SPI_HIST_CARD_EVENT)
 		if_spi_e2h(card);
 
-err:
-	if (err)
+	if (err) {
+report_failure:
 		netdev_err(priv->dev, "%s: got error %d\n", __func__, err);
+	}
 
 	lbs_deb_leave(LBS_DEB_SPI);
 }
-- 
2.6.3

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Julia Lawall]

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

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 22:27:20 +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 function call failed.

Putting a label inside an if is ugly.

julia

> Use the identifier "report_failure" instead of the label "err".
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/marvell/libertas/if_spi.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
> index 82c0796..c9ae27e 100644
> --- a/drivers/net/wireless/marvell/libertas/if_spi.c
> +++ b/drivers/net/wireless/marvell/libertas/if_spi.c
> @@ -880,18 +880,18 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
>  				&hiStatus);
>  	if (err) {
>  		netdev_err(priv->dev, "I/O error\n");
> -		goto err;
> +		goto report_failure;
>  	}
>  
>  	if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) {
>  		err = if_spi_c2h_cmd(card);
>  		if (err)
> -			goto err;
> +			goto report_failure;
>  	}
>  	if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) {
>  		err = if_spi_c2h_data(card);
>  		if (err)
> -			goto err;
> +			goto report_failure;
>  	}
>  
>  	/*
> @@ -940,9 +940,10 @@ static void if_spi_host_to_card_worker(struct work_struct *work)
>  	if (hiStatus & IF_SPI_HIST_CARD_EVENT)
>  		if_spi_e2h(card);
>  
> -err:
> -	if (err)
> +	if (err) {
> +report_failure:
>  		netdev_err(priv->dev, "%s: got error %d\n", __func__, err);
> +	}
>  
>  	lbs_deb_leave(LBS_DEB_SPI);
>  }
> -- 
> 2.6.3
> 
> --
> 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
> 

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Sergei Shtylyov]

Hello

On 1/2/2016 12:33 AM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 1 Jan 2016 22:27:20 +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 function call failed.
> Use the identifier "report_failure" instead of the label "err".

    Why? The code was smart enough and you're making it uglier that it needs 
to be.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

MBR, Sergei

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

>> 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 function call failed.
>> Use the identifier "report_failure" instead of the label "err".
> 
>    Why?

I suggest to reconsider the places with which such a jump label
is connected.


> The code was smart enough

Which action should really be performed after a failure was detected
and handled a bit already?

* Another condition check

* Just additional error logging


> and you're making it uglier that it needs to be.

I assume that a software development taste can evolve, can't it?

Regards,
Markus

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Julia Lawall]

On Sat, 2 Jan 2016, SF Markus Elfring wrote:

> >> 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 function call failed.
> >> Use the identifier "report_failure" instead of the label "err".
> > 
> >    Why?
> 
> I suggest to reconsider the places with which such a jump label
> is connected.
> 
> 
> > The code was smart enough
> 
> Which action should really be performed after a failure was detected
> and handled a bit already?
> 
> * Another condition check
> 
> * Just additional error logging
> 
> 
> > and you're making it uglier that it needs to be.
> 
> I assume that a software development taste can evolve, can't it?

So far, you have gotten several down votes for this kind of change, and no 
enthusiasm.

Admittedly, this is a trivial case, because there are no local variables, 
but do you actually know the semantics in C of a jump into a block?  And 
if you do know, do you think that this semantics is common knowledge?  And 
do you really think that introducing poorly understandable code is really 
worth saving an if test of a single variable on a non-critical path?

Most of the kernel code is not performance critical at the level of a 
single if test.  So the goal should be for the code to be easy to 
understand and robust to change.  The code that is performance critical, 
you should probably not touch, ever.  The people who wrote it knew what 
was important and what was not.

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

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

>> I assume that a software development taste can evolve, can't it?
> 
> So far, you have gotten several down votes for this kind of change,

I am curious when more contributors will share corresponding opinions.


> and no enthusiasm.

How many software designers and developers can become enthusiastic
about better exception handling to some degree?


> The code that is performance critical, you should probably not touch, ever.

I imagine that technical evolution will result in further considerations
so that "unchangeable" components can be adjusted once more.


> The people who wrote it knew what was important and what was not.

I might come along at some places where the affected knowledge will also evolve.

Regards,
Markus

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Arend van Spriel]

On 02-01-16 10:08, SF Markus Elfring wrote:
>>> I assume that a software development taste can evolve, can't it?
>>
>> So far, you have gotten several down votes for this kind of change,
> 
> I am curious when more contributors will share corresponding opinions.

Let's burn some cycles on this while the holidays give me time to do so.
"software development taste" is another term for "coding style". In
every project battles are fought over this between friends and foes. I
have never seen much evolution going on in this area.

>> and no enthusiasm.
> 
> How many software designers and developers can become enthusiastic
> about better exception handling to some degree?

I had to  take a look at this particular patch and I have to say that I
don't see, using your favorite term, evolution at work. It looks more
like the result of inbred. What the patch tries to do is avoid the extra
'if (err)'. Setting coding style aside, the question is whether there is
a good metric for the patch. So does it really safe processing time? Did
you look at the resulting assembly code for different target architectures?

You got pushed back on the change so you have to come up with solid
arguments for your change instead of spewing ideas about evolution in
software development. Running Coccinelle is one thing, but understanding
the results and what you are ultimately proposing to be changed is more
important.

Regards,
Arend

>> The code that is performance critical, you should probably not touch, ever.
> 
> I imagine that technical evolution will result in further considerations
> so that "unchangeable" components can be adjusted once more.
> 
> 
>> The people who wrote it knew what was important and what was not.
> 
> I might come along at some places where the affected knowledge will also evolve.
> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

> I have never seen much evolution going on in this area.

I can get an other impression from a specific document for example.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/Documentation/CodingStyle


> What the patch tries to do is avoid the extra 'if (err)'.

Yes. - I propose to look at related consequences together with the usage
of a popular short jump label once more.


> Setting coding style aside, the question is whether there is
> a good metric for the patch.

A software development challenge is to accept changes also around a topic
like "error handling". My update suggestion for the source file
"drivers/net/wireless/marvell/libertas/if_spi.c" should only improve
exception handling. (I came along other source files where more improvements
will eventually be possible.)

When will the run-time behaviour matter also for exceptional situations?


> Did you look at the resulting assembly code for different target architectures?

Not yet. - Which execution system variants would you recommend for
further comparisons?

Regards,
Markus

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Arend van Spriel]

On 02-01-16 12:21, SF Markus Elfring wrote:
>> I have never seen much evolution going on in this area.
> 
> I can get an other impression from a specific document for example.
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/Documentation/CodingStyle
> 
> 
>> What the patch tries to do is avoid the extra 'if (err)'.
> 
> Yes. - I propose to look at related consequences together with the usage
> of a popular short jump label once more.

When I read a subject saying "Better exception handling" it sounds like
a functional improvement. Your change does not change anything
functionally and may or may not save a bit of execution time depending
on how smart the compiler is. What you change does is confuse people
reading the code. So please explain why your update improves exception
handling here. I don't see it. The code is not making the driver more
robust against failures in this function, which is what I think of
reading "better exception handling".

>> Setting coding style aside, the question is whether there is
>> a good metric for the patch.
> 
> A software development challenge is to accept changes also around a topic
> like "error handling". My update suggestion for the source file
> "drivers/net/wireless/marvell/libertas/if_spi.c" should only improve
> exception handling. (I came along other source files where more improvements
> will eventually be possible.)
> 
> When will the run-time behaviour matter also for exceptional situations?
> 
> 
>> Did you look at the resulting assembly code for different target architectures?
> 
> Not yet. - Which execution system variants would you recommend for
> further comparisons?

Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.

Regards,
Arend

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[SF Markus Elfring]

>>> What the patch tries to do is avoid the extra 'if (err)'.
>>
>> Yes. - I propose to look at related consequences together with the usage
>> of a popular short jump label once more.
> 
> When I read a subject saying "Better exception handling" it sounds like
> a functional improvement. Your change does not change anything
> functionally and may or may not save a bit of execution time depending
> on how smart the compiler is.

Can it eventually matter to skip another condition check in three cases?


> What you change does is confuse people reading the code.

A few software developers might find this proposal unusual.


> So please explain why your update improves exception handling here.
> I don't see it.

How does this feedback fit to the mentioned check avoidance?


> The code is not making the driver more robust against failures

That's true for this update suggestion.


> in this function, which is what I think of reading "better exception handling".

Other implementation details are affected by the shown fine-tuning.

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

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Rafał Miłecki]

On 3 January 2016 at 10:36, Arend van Spriel <aspriel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 02-01-16 12:21, SF Markus Elfring wrote:
>>> Did you look at the resulting assembly code for different target architectures?
>>
>> Not yet. - Which execution system variants would you recommend for
>> further comparisons?
>
> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.

Oh, don't forget about MIPS with its fancy branches handling. You know
about it, don't you?

I'm against this patch as well.

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

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Arend van Spriel]

On 03-01-16 16:18, Rafał Miłecki wrote:
> On 3 January 2016 at 10:36, Arend van Spriel <aspriel@gmail.com> wrote:
>> On 02-01-16 12:21, SF Markus Elfring wrote:
>>>> Did you look at the resulting assembly code for different target architectures?
>>>
>>> Not yet. - Which execution system variants would you recommend for
>>> further comparisons?
>>
>> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.
> 
> Oh, don't forget about MIPS with its fancy branches handling. You know
> about it, don't you?

You are asking me, right ;-) ? I have come across my share of MIPS
platforms here at Broadcom, but I still try to avoid them as much as
possible.

> I'm against this patch as well.

and counting... :-p

Regards,
Arend
--
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

Re: net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Rafał Miłecki]

On 4 January 2016 at 11:05, Arend van Spriel <aspriel@gmail.com> wrote:
> On 03-01-16 16:18, Rafał Miłecki wrote:
>> On 3 January 2016 at 10:36, Arend van Spriel <aspriel@gmail.com> wrote:
>>> On 02-01-16 12:21, SF Markus Elfring wrote:
>>>>> Did you look at the resulting assembly code for different target architectures?
>>>>
>>>> Not yet. - Which execution system variants would you recommend for
>>>> further comparisons?
>>>
>>> Guess x86{,_64} and arm would be good candidates, ie. CISC vs. RISC.
>>
>> Oh, don't forget about MIPS with its fancy branches handling. You know
>> about it, don't you?
>
> You are asking me, right ;-) ? I have come across my share of MIPS
> platforms here at Broadcom, but I still try to avoid them as much as
> possible.

I was more thinking about author on this patch. But it's indeed an
interesting thing, just to know, how MIPS CPU handles branches ;)

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

Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()

[Kalle Valo]

Julia Lawall <julia.lawall@lip6.fr> writes:

> On Sat, 2 Jan 2016, SF Markus Elfring wrote:
>
>> >> 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 function call failed.
>> >> Use the identifier "report_failure" instead of the label "err".
>> > 
>> >    Why?
>> 
>> I suggest to reconsider the places with which such a jump label
>> is connected.
>> 
>> 
>> > The code was smart enough
>> 
>> Which action should really be performed after a failure was detected
>> and handled a bit already?
>> 
>> * Another condition check
>> 
>> * Just additional error logging
>> 
>> 
>> > and you're making it uglier that it needs to be.
>> 
>> I assume that a software development taste can evolve, can't it?
>
> So far, you have gotten several down votes for this kind of change, and no 
> enthusiasm.
>
> Admittedly, this is a trivial case, because there are no local variables, 
> but do you actually know the semantics in C of a jump into a block?  And 
> if you do know, do you think that this semantics is common knowledge?  And 
> do you really think that introducing poorly understandable code is really 
> worth saving an if test of a single variable on a non-critical path?
>
> Most of the kernel code is not performance critical at the level of a 
> single if test.  So the goal should be for the code to be easy to 
> understand and robust to change.  The code that is performance critical, 
> you should probably not touch, ever.  The people who wrote it knew what 
> was important and what was not.

Very well said! Only optimise something you can measure.

I'm dropping this patch.

-- 
Kalle Valo

[PATCH 0/3] net-rsi: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 15:36:25 +0100

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

Markus Elfring (3):
  Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()
  Delete unnecessary variable initialisations in rsi_send_data_pkt()
  Replace variable initialisations by assignments in rsi_send_data_pkt()

 drivers/net/wireless/rsi/rsi_91x_pkt.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

-- 
2.6.3

[PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 14:54:30 +0100

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/rsi/rsi_91x_pkt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 702593f..ee98f5b 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -123,15 +123,15 @@ int rsi_send_mgmt_pkt(struct rsi_common *common,
 		      struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *wh = NULL;
+	struct ieee80211_hdr *wh;
 	struct ieee80211_tx_info *info;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	struct ieee80211_hw *hw = adapter->hw;
 	struct ieee80211_conf *conf = &hw->conf;
 	struct skb_info *tx_params;
-	int status = -E2BIG;
-	__le16 *msg = NULL;
-	u8 extnd_size = 0;
+	int status;
+	__le16 *msg;
+	u8 extnd_size;
 	u8 vap_id = 0;
 
 	info = IEEE80211_SKB_CB(skb);
-- 
2.6.3

net-rsi: Reconsider usage of variable "vap_id" in rsi_send_mgmt_pkt()

[SF Markus Elfring]

Hello,

I have taken another look at the implementation of the function "rsi_send_mgmt_pkt".
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/net/wireless/rsi/rsi_91x_pkt.c?id=e8c58e7a5a106c3d557fccd01cd4d1128f9bab38#n114

I find the following statement combination interesting there.

…
	u8 vap_id = 0;
…
	msg[7] |= cpu_to_le16(vap_id << 8);
…

I would appreciate a further clarification.
Does a shift operation for a variable which contains zero indicate an open issue?

Regards,
Markus

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2786 bytes --]

Hi Markus,

[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on v4.4-rc7 next-20151231]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/net-rsi-Fine-tuning-for-two-function-implementations/20160102-224740
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: x86_64-randconfig-s2-01030012 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/net/wireless/rsi/rsi_91x_pkt.c: In function 'rsi_send_mgmt_pkt':
>> drivers/net/wireless/rsi/rsi_91x_pkt.c:211:2: warning: 'status' may be used uninitialized in this function [-Wmaybe-uninitialized]
     rsi_indicate_tx_status(common->priv, skb, status);
     ^

vim +/status +211 drivers/net/wireless/rsi/rsi_91x_pkt.c

dad0d04f Fariya Fatima 2014-03-16  195  	/* Indicate to firmware to give cfm */
dad0d04f Fariya Fatima 2014-03-16  196  	if ((skb->data[16] == IEEE80211_STYPE_PROBE_REQ) && (!bss->assoc)) {
dad0d04f Fariya Fatima 2014-03-16  197  		msg[1] |= cpu_to_le16(BIT(10));
dad0d04f Fariya Fatima 2014-03-16  198  		msg[7] = cpu_to_le16(PROBEREQ_CONFIRM);
dad0d04f Fariya Fatima 2014-03-16  199  		common->mgmt_q_block = true;
dad0d04f Fariya Fatima 2014-03-16  200  	}
dad0d04f Fariya Fatima 2014-03-16  201  
dad0d04f Fariya Fatima 2014-03-16  202  	msg[7] |= cpu_to_le16(vap_id << 8);
dad0d04f Fariya Fatima 2014-03-16  203  
dad0d04f Fariya Fatima 2014-03-16  204  	status = adapter->host_intf_write_pkt(common->priv,
dad0d04f Fariya Fatima 2014-03-16  205  					      (u8 *)msg,
dad0d04f Fariya Fatima 2014-03-16  206  					      skb->len);
dad0d04f Fariya Fatima 2014-03-16  207  	if (status)
dad0d04f Fariya Fatima 2014-03-16  208  		rsi_dbg(ERR_ZONE, "%s: Failed to write the packet\n", __func__);
dad0d04f Fariya Fatima 2014-03-16  209  
dad0d04f Fariya Fatima 2014-03-16  210  err:
dad0d04f Fariya Fatima 2014-03-16 @211  	rsi_indicate_tx_status(common->priv, skb, status);
dad0d04f Fariya Fatima 2014-03-16  212  	return status;
dad0d04f Fariya Fatima 2014-03-16  213  }

:::::: The code at line 211 was first introduced by commit
:::::: dad0d04fa7ba41ce603a01e8e64967650303e9a2 rsi: Add RS9113 wireless driver

:::::: TO: Fariya Fatima <fariyaf@gmail.com>
:::::: CC: John W. Linville <linville@tuxdriver.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 27399 bytes --]

[PATCH v2 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 19:22:36 +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/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 702593f..571eaba 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -123,15 +123,15 @@ int rsi_send_mgmt_pkt(struct rsi_common *common,
 		      struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *wh = NULL;
+	struct ieee80211_hdr *wh;
 	struct ieee80211_tx_info *info;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	struct ieee80211_hw *hw = adapter->hw;
 	struct ieee80211_conf *conf = &hw->conf;
 	struct skb_info *tx_params;
 	int status = -E2BIG;
-	__le16 *msg = NULL;
-	u8 extnd_size = 0;
+	__le16 *msg;
+	u8 extnd_size;
 	u8 vap_id = 0;
 
 	info = IEEE80211_SKB_CB(skb);
-- 
2.6.3

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Dan Carpenter]

These patches are labour intensive to review because you can't just do
it in the email client.  Also you were not able to review it properly
yourself and introduced a bug.

I am often remove initializers but it's normally because I am changing
something else which makes it worthwhile.  This patch is the correct
thing but it's not "worthwhile".  It is not a good use of my time.

Please stop sending cleanup patches, Markus.  Just send fixes.

regards,
dan carpenter

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Dan Carpenter]

Btw, GCC misses a lot of uninitialized variable bugs.  I have a Smatch
check which sometimes catches the bugs that GCC misses but you should
not rely on the tools here.  These patches need to be reviewed manually.

And the "goto err" before the initialization makes everything more
complicated (that's actually what caused the bug in this patch, in fact).

regards,
dan carpenter

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

> These patches are labour intensive to review because you can't just do
> it in the email client.

Thanks for your general interest.


> Also you were not able to review it properly yourself and introduced
> a bug.

I admit that it can happen during my software development that I overlook
implementation details somehow.


> I am often remove initializers but it's normally because I am changing
> something else which makes it worthwhile.

It is nice to hear that you are also occasionally looking for similar
update candidates.


> This patch is the correct thing but it's not "worthwhile".

I find this view interesting.


> Please stop sending cleanup patches, Markus.  Just send fixes.

How often will source code clean-up fix something?


May I resend a consistent patch series for the source file
"drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future?

Regards,
Markus

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Dan Carpenter]

On Mon, Jan 04, 2016 at 11:44:15AM +0100, SF Markus Elfring wrote:
> > Please stop sending cleanup patches, Markus.  Just send fixes.
> 
> How often will source code clean-up fix something?
> 
> 
> May I resend a consistent patch series for the source file
> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future?

If you were sending checkpatch.pl fixes that would be easier to deal
with but you are sending hundreds of "controversial" cleanups.  They are
controversial in the sense that they don't fix anything against official
kernel style and they go against the author's original intention.  I
tend to agree that useless initializers are bad and disable GCCs
uninitialized variable warnings but just because I agree with you
doesn't make it official kernel style.  It's slightly rude to go against
the author's intention.

regards,
dan carpenter

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

>> May I resend a consistent patch series for the source file
>> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future?
> 
> If you were sending checkpatch.pl fixes that would be easier to deal with

Does this feedback mean that you would accept any more suggestions around
source code updates which are derived from recommendations of this script?


> but you are sending hundreds of "controversial" cleanups.

It depends on the time range you look at for my proposals.


> They are controversial in the sense that they don't fix anything
> against official kernel style

I find that I suggested also few changes that fit to this aspect.


> and they go against the author's original intention.

Can it occasionally help to reconsider the "first approach"?


> I tend to agree that useless initializers are bad

Would any more software developers like to share their opinions on this detail?


> and disable GCCs uninitialized variable warnings

I hope that this software area can be also improved.


> but just because I agree with you doesn't make it official kernel style.

That is fine. - Will it become useful to clarify any extensions
to a document like "CodingStyle"?


> It's slightly rude to go against the author's intention.

I just dare to propose further special changes.

Regards,
Markus

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Julian Calaby]

Hi Markus,

On Mon, Jan 4, 2016 at 11:33 PM, SF Markus Elfring
<elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote:
>>> May I resend a consistent patch series for the source file
>>> "drivers/net/wireless/rsi/rsi_91x_pkt.c" in the near future?
>>
>> If you were sending checkpatch.pl fixes that would be easier to deal with
>
> Does this feedback mean that you would accept any more suggestions around
> source code updates which are derived from recommendations of this script?

A good rule of thumb here would be that if people start complaining
about a particular type of change, stop sending them.

Another good rule of thumb is to try to "rock the boat" on coding
style and conventions as little as possible. Just because it's
possible doesn't mean that people want to do it.

That said, if you figure out some change that produces significant
reductions in code or binary size on multiple architectures without
making things more complicated, less readable or making the code or
binary size larger, then by all means propose it. "This makes things
smaller" carries much more weight than "I think this is better".
Almost all of the changes you've proposed that have seen any
discussion whatsoever fall into the latter category.

Thanks,

-- 
Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

> That said, if you figure out some change that produces significant
> reductions in code or binary size on multiple architectures without
> making things more complicated, less readable or making the code or
> binary size larger, then by all means propose it.

Are you looking also for "a proof" that such changes are worthwhile?


> "This makes things smaller" carries much more weight than
> "I think this is better".

Can the discussed implementation of a function like "rsi_send_mgmt_pkt"
become a bit smaller by the deletion of extra variable initialisations


> Almost all of the changes you've proposed that have seen any
> discussion whatsoever fall into the latter category.

Thanks for your interesting feedback.

Can a further constructive dialogue evolve from the presented information?

Regards,
Markus

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Julian Calaby]

Hi Markus,

On Tue, Jan 5, 2016 at 7:29 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> That said, if you figure out some change that produces significant
>> reductions in code or binary size on multiple architectures without
>> making things more complicated, less readable or making the code or
>> binary size larger, then by all means propose it.
>
> Are you looking also for "a proof" that such changes are worthwhile?

It'd be better than "I think doing things this way is better", which
is the hallmark of most of your patch sets. (Admittedly not this one,
but this one is where the discussion is now, so that's where we're
discussing it.)

>> "This makes things smaller" carries much more weight than
>> "I think this is better".
>
> Can the discussed implementation of a function like "rsi_send_mgmt_pkt"
> become a bit smaller by the deletion of extra variable initialisations

I'm talking in general.

In this case you're asking people to review a patch which requires a
lot of careful review for a fairly minor improvement. I must also note
that you haven't CC'd the people who wrote this driver, so it's
possible that the only people who have reviewed it aren't experts in
the code.

The patches you sent recently which moved labels into if statements
were a clear case of "I think this is better" where any actual benefit
from the changes was eclipsed by the style and readability issues they
introduced.

>> Almost all of the changes you've proposed that have seen any
>> discussion whatsoever fall into the latter category.
>
> Thanks for your interesting feedback.

No problem.

> Can a further constructive dialogue evolve from the presented information?

Part of the issue here is that you don't seem to be listening to the
discussion of your patches, or if you are, you're not significantly
changing your approach or attitude in response.

Every time you send a set of patches, there are legitimate issues
which people raise, and every time they are discussed, you assert that
your patches improve things and seem to ignore the concerns people
raise.

I've seen this same pattern of discussion here with these patches,
with your patches to move labels into if statements, with the patches
you sent late June last year, your patches to remove conditions before
kfree() and friends, etc.

You need to change you attitude: just because you can see some benefit
from your patches doesn't mean others do and it doesn't mean that
they're willing to accept them.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

> Every time you send a set of patches,

I suggested some updates for Linux source files since October 2014.


> there are legitimate issues which people raise,

There was usual feedback.


> and every time they are discussed,

The discussion results were mixed between acceptance
and usual disagreement.


> you assert that your patches improve things

I guess that should be the default intention of every patch, shouldn't it?


> and seem to ignore the concerns people raise.

I hope not. - But I can imagine that you might understand some responses
from contributors in this way.
Are you waiting for another clarification on a specific issue?


> I've seen this same pattern of discussion here with these patches,
> with your patches to move labels into if statements, with the patches
> you sent late June last year, your patches to remove conditions before
> kfree() and friends, etc.

It seems that communication difficulties come partly from the fact
that I chose search patterns from static source code analysis so far
which belong to an error category that gets a lower priority.


> You need to change you attitude: just because you can see some benefit
> from your patches doesn't mean others do and it doesn't mean that
> they're willing to accept them.

I understand your advice.

Further update suggestions with higher importance might follow for various
software areas in the future.

Regards,
Markus

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Bjørn Mork]

Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> writes:

> Please stop sending cleanup patches, Markus.  Just send fixes.

Thanks for your continued but unwarranted belief in AI.

Do you mind if I remind you of https://lkml.org/lkml/2014/11/3/162 ?
I am sure there are lots and lots of other examples.  There is no reason
to believe this will ever stop.  He just goes into Eliza mode.


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

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[Dan Carpenter]

On Mon, Jan 04, 2016 at 02:17:40PM +0100, Bjørn Mork wrote:
> Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Please stop sending cleanup patches, Markus.  Just send fixes.
> 
> Thanks for your continued but unwarranted belief in AI.
> 

I always tell people that I am very mechanical and you can rely on me to
send predictable responses...

> Do you mind if I remind you of https://lkml.org/lkml/2014/11/3/162 ?
> I am sure there are lots and lots of other examples.  There is no reason
> to believe this will ever stop.  He just goes into Eliza mode.

Yup.

I feel some sense of responsibility for any patches where kernel-janitors
is on the CC but I'm having a hard time dealing with all of Markus's
patches.  Normally you just respond to the first patch and people change
the later patches but, as you put it, Markus just goes into ELIZA mode.

regards,
dan carpenter

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

Re: [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 4 Jan 2016 12:28:57 +0300

> Please stop sending cleanup patches, Markus.  Just send fixes.

+1

[PATCH 2/3] rsi: Delete unnecessary variable initialisations in rsi_send_data_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 15:15:12 +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/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index ee98f5b..ec65e1c 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -27,15 +27,15 @@
 int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *tmp_hdr = NULL;
+	struct ieee80211_hdr *tmp_hdr;
 	struct ieee80211_tx_info *info;
 	struct skb_info *tx_params;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	int status = -EINVAL;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
-	u8 extnd_size = 0;
+	u8 extnd_size;
 	__le16 *frame_desc;
-	u16 seq_num = 0;
+	u16 seq_num;
 
 	info = IEEE80211_SKB_CB(skb);
 	bss = &info->control.vif->bss_conf;
-- 
2.6.3

[PATCH 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 15:25:34 +0100

Replace explicit initialisation for two local variables at the beginning
by assignments.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/rsi/rsi_91x_pkt.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index ec65e1c..fe36e7d 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -26,12 +26,12 @@
  */
 int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 {
-	struct rsi_hw *adapter = common->priv;
+	struct rsi_hw *adapter;
 	struct ieee80211_hdr *tmp_hdr;
 	struct ieee80211_tx_info *info;
 	struct skb_info *tx_params;
 	struct ieee80211_bss_conf *bss;
-	int status = -EINVAL;
+	int status;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
 	u8 extnd_size;
 	__le16 *frame_desc;
@@ -41,8 +41,10 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 	bss = &info->control.vif->bss_conf;
 	tx_params = (struct skb_info *)info->driver_data;
 
-	if (!bss->assoc)
+	if (!bss->assoc) {
+		status = -EINVAL;
 		goto err;
+	}
 
 	tmp_hdr = (struct ieee80211_hdr *)&skb->data[0];
 	seq_num = (le16_to_cpu(tmp_hdr->seq_ctrl) >> 4);
@@ -97,7 +99,7 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 	frame_desc[7] = cpu_to_le16(((tx_params->tid & 0xf) << 4) |
 				    (skb->priority & 0xf) |
 				    (tx_params->sta_id << 8));
-
+	adapter = common->priv;
 	status = adapter->host_intf_write_pkt(common->priv,
 					      skb->data,
 					      skb->len);
-- 
2.6.3

Re: [PATCH 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()

[Francois Romieu]

SF Markus Elfring <elfring@users.sourceforge.net> :
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Jan 2016 15:25:34 +0100
> 
> Replace explicit initialisation for two local variables at the beginning
> by assignments.

It makes no sense for the 'adapter' variable.

-- 
Ueimor

[PATCH v3 0/3] net-rsi: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Date: Fri, 15 Jan 2016 13:54:43 +0100

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

Markus Elfring (3):
  Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()
  Delete unnecessary variable initialisations in rsi_send_data_pkt()
  Replace variable initialisations by assignments in rsi_send_data_pkt()

 drivers/net/wireless/rsi/rsi_91x_pkt.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

---

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

-- 
2.6.3

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

[PATCH v3 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 15 Jan 2016 13:30:39 +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/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 702593f..571eaba 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -123,15 +123,15 @@ int rsi_send_mgmt_pkt(struct rsi_common *common,
 		      struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *wh = NULL;
+	struct ieee80211_hdr *wh;
 	struct ieee80211_tx_info *info;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	struct ieee80211_hw *hw = adapter->hw;
 	struct ieee80211_conf *conf = &hw->conf;
 	struct skb_info *tx_params;
 	int status = -E2BIG;
-	__le16 *msg = NULL;
-	u8 extnd_size = 0;
+	__le16 *msg;
+	u8 extnd_size;
 	u8 vap_id = 0;
 
 	info = IEEE80211_SKB_CB(skb);
-- 
2.6.3

[PATCH v3 2/3] rsi: Delete unnecessary variable initialisations in rsi_send_data_pkt()

[SF Markus Elfring]

>From 017d1bb49f46266ffeb33178ddd3022d6b341d71 Mon Sep 17 00:00:00 2001
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 15 Jan 2016 13:35:47 +0100
Subject: [PATCH 2/3] rsi: Delete unnecessary variable initialisations in
 rsi_send_data_pkt()

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/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 571eaba..4322df1 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -27,15 +27,15 @@
 int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 {
 	struct rsi_hw *adapter = common->priv;
-	struct ieee80211_hdr *tmp_hdr = NULL;
+	struct ieee80211_hdr *tmp_hdr;
 	struct ieee80211_tx_info *info;
 	struct skb_info *tx_params;
-	struct ieee80211_bss_conf *bss = NULL;
+	struct ieee80211_bss_conf *bss;
 	int status = -EINVAL;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
-	u8 extnd_size = 0;
+	u8 extnd_size;
 	__le16 *frame_desc;
-	u16 seq_num = 0;
+	u16 seq_num;
 
 	info = IEEE80211_SKB_CB(skb);
 	bss = &info->control.vif->bss_conf;
-- 
2.6.3

[PATCH v3 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 15 Jan 2016 13:40:22 +0100

Replace explicit initialisation for two local variables at the beginning
by assignments.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/rsi/rsi_91x_pkt.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 4322df1..2c18c01 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -26,12 +26,12 @@
  */
 int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 {
-	struct rsi_hw *adapter = common->priv;
+	struct rsi_hw *adapter;
 	struct ieee80211_hdr *tmp_hdr;
 	struct ieee80211_tx_info *info;
 	struct skb_info *tx_params;
 	struct ieee80211_bss_conf *bss;
-	int status = -EINVAL;
+	int status;
 	u8 ieee80211_size = MIN_802_11_HDR_LEN;
 	u8 extnd_size;
 	__le16 *frame_desc;
@@ -41,8 +41,10 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 	bss = &info->control.vif->bss_conf;
 	tx_params = (struct skb_info *)info->driver_data;
 
-	if (!bss->assoc)
+	if (!bss->assoc) {
+		status = -EINVAL;
 		goto err;
+	}
 
 	tmp_hdr = (struct ieee80211_hdr *)&skb->data[0];
 	seq_num = (le16_to_cpu(tmp_hdr->seq_ctrl) >> 4);
@@ -97,7 +99,7 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
 	frame_desc[7] = cpu_to_le16(((tx_params->tid & 0xf) << 4) |
 				    (skb->priority & 0xf) |
 				    (tx_params->sta_id << 8));
-
+	adapter = common->priv;
 	status = adapter->host_intf_write_pkt(common->priv,
 					      skb->data,
 					      skb->len);
-- 
2.6.3

Re: [PATCH v3 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()

[Dan Carpenter]

Still makes no sense for adapter like Francois Romieu said.

regards,
dan carpenter

[PATCH 0/5] xen-netback: Fine-tuning for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 18:46:45 +0100

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

Markus Elfring (5):
  Delete an unnecessary assignment in connect_rings()
  Delete an unnecessary goto statement in connect_rings()
  Replace a variable initialisation by an assignment in read_xenbus_vif_flags()
  Replace a variable initialisation by an assignment in xen_register_watchers()
  Delete an unnecessary variable initialisation in xen_register_watchers()

 drivers/net/xen-netback/xenbus.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

-- 
2.6.3

[PATCH 1/5] xen-netback: Delete an unnecessary assignment in connect_rings()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 17:32:40 +0100

Remove the assignment for a local variable because its value is not
changed compared to the one from a previous function call.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/xen-netback/xenbus.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 56ebd82..7f2895d 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -941,7 +941,6 @@ static int connect_rings(struct backend_info *be, struct xenvif_queue *queue)
 		goto err;
 	}
 
-	err = 0;
 err: /* Regular return falls through with err == 0 */
 	kfree(xspath);
 	return err;
-- 
2.6.3

[PATCH 2/5] xen-netback: Delete an unnecessary goto statement in connect_rings()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 17:50:21 +0100

One goto statement referred to a source code position directly behind it.
Thus omit such an unnecessary jump.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/xen-netback/xenbus.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 7f2895d..d4947e1 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -933,13 +933,11 @@ static int connect_rings(struct backend_info *be, struct xenvif_queue *queue)
 	/* Map the shared frame, irq etc. */
 	err = xenvif_connect(queue, tx_ring_ref, rx_ring_ref,
 			     tx_evtchn, rx_evtchn);
-	if (err) {
+	if (err)
 		xenbus_dev_fatal(dev, err,
 				 "mapping shared-frames %lu/%lu port tx %u rx %u",
 				 tx_ring_ref, rx_ring_ref,
 				 tx_evtchn, rx_evtchn);
-		goto err;
-	}
 
 err: /* Regular return falls through with err == 0 */
 	kfree(xspath);
-- 
2.6.3

[PATCH 3/5] xen-netback: Replace a variable initialisation by an assignment in read_xenbus_vif_flags()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 18:01:57 +0100

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/xen-netback/xenbus.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index d4947e1..aff963f 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -946,7 +946,7 @@ err: /* Regular return falls through with err == 0 */
 
 static int read_xenbus_vif_flags(struct backend_info *be)
 {
-	struct xenvif *vif = be->vif;
+	struct xenvif *vif;
 	struct xenbus_device *dev = be->dev;
 	unsigned int rx_copy;
 	int err, val;
@@ -968,13 +968,14 @@ static int read_xenbus_vif_flags(struct backend_info *be)
 	if (xenbus_scanf(XBT_NIL, dev->otherend,
 			 "feature-rx-notify", "%d", &val) < 0)
 		val = 0;
+	vif = be->vif;
 	if (!val) {
 		/* - Reduce drain timeout to poll more frequently for
 		 *   Rx requests.
 		 * - Disable Rx stall detection.
 		 */
-		be->vif->drain_timeout = msecs_to_jiffies(30);
-		be->vif->stall_timeout = 0;
+		vif->drain_timeout = msecs_to_jiffies(30);
+		vif->stall_timeout = 0;
 	}
 
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
-- 
2.6.3

[PATCH 4/5] xen-netback: Replace a variable initialisation by an assignment in xen_register_watchers()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 18:23:16 +0100

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/xen-netback/xenbus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index aff963f..e8dfc3d 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -687,11 +687,12 @@ static int xen_register_watchers(struct xenbus_device *dev, struct xenvif *vif)
 {
 	int err = 0;
 	char *node;
-	unsigned maxlen = strlen(dev->nodename) + sizeof("/rate");
+	unsigned maxlen;
 
 	if (vif->credit_watch.node)
 		return -EADDRINUSE;
 
+	maxlen = strlen(dev->nodename) + sizeof("/rate");
 	node = kmalloc(maxlen, GFP_KERNEL);
 	if (!node)
 		return -ENOMEM;
-- 
2.6.3

[PATCH 5/5] xen-netback: Delete an unnecessary variable initialisation in xen_register_watchers()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Jan 2016 18:28:26 +0100

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/xen-netback/xenbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index e8dfc3d..55f0735 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -685,7 +685,7 @@ static void xen_net_rate_changed(struct xenbus_watch *watch,
 
 static int xen_register_watchers(struct xenbus_device *dev, struct xenvif *vif)
 {
-	int err = 0;
+	int err;
 	char *node;
 	unsigned maxlen;
 
-- 
2.6.3

Re: [PATCH 0/5] xen-netback: Fine-tuning for three function implementations

[Joe Perches]

On Sat, 2016-01-02 at 18:50 +0100, SF Markus Elfring wrote:
> A few update suggestions were taken into account
> from static source code analysis.

While static analysis can be useful, I don't think these
specific conversions are generally useful.

Perhaps it would be more useful to convert the string
duplication or snprintf logic to kstrdup/kasprintf

This:

	if (num_queues == 1) {
		xspath = kzalloc(strlen(dev->otherend) + 1, GFP_KERNEL);
		if (!xspath) {
			xenbus_dev_fatal(dev, -ENOMEM,
					 "reading ring references");
			return -ENOMEM;
		}
		strcpy(xspath, dev->otherend);
	} else {
		xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
		xspath = kzalloc(xspathsize, GFP_KERNEL);
		if (!xspath) {
			xenbus_dev_fatal(dev, -ENOMEM,
					 "reading ring references");
			return -ENOMEM;
		}
		snprintf(xspath, xspathsize, "%s/queue-%u", dev->otherend,
			 queue->id);
	}

could be simplified to something like:

	if (num_queues == 1)
		xspath = kstrdup(dev->otherend, GFP_KERNEL);
	else
		xspath = kasprintf(GFP_KERNEL, "%s/queue-%u",
				   dev->otherend, queue->id);
	if (!xspath)
		etc...

Re: [PATCH 0/5] xen-netback: Fine-tuning for three function implementations

[Dan Carpenter]

The original code is fine.

regards,
dan carpenter

Re: [PATCH 0/5] xen-netback: Fine-tuning for three function implementations

[Wei Liu]

I think the original code is fine.

Wei.

[PATCH] mlx5/core: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 07:50:09 +0200

* Reuse existing functionality from memdup_user() instead of keeping
  duplicate source code.

  This issue was detected by using the Coccinelle software.

* Return directly if this copy operation failed.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
index 6388bc0..bb89f04 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
@@ -1132,7 +1132,6 @@ static ssize_t data_write(struct file *filp, const char __user *buf,
 	struct mlx5_core_dev *dev = filp->private_data;
 	struct mlx5_cmd_debug *dbg = &dev->cmd.dbg;
 	void *ptr;
-	int err;
 
 	if (*pos != 0)
 		return -EINVAL;
@@ -1140,25 +1139,15 @@ static ssize_t data_write(struct file *filp, const char __user *buf,
 	kfree(dbg->in_msg);
 	dbg->in_msg = NULL;
 	dbg->inlen = 0;
-
-	ptr = kzalloc(count, GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
-	if (copy_from_user(ptr, buf, count)) {
-		err = -EFAULT;
-		goto out;
-	}
+	ptr = memdup_user(buf, count);
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
 	dbg->in_msg = ptr;
 	dbg->inlen = count;
 
 	*pos = count;
 
 	return count;
-
-out:
-	kfree(ptr);
-	return err;
 }
 
 static ssize_t data_read(struct file *filp, char __user *buf, size_t count,
-- 
2.9.3

Re: [PATCH] mlx5/core: Use memdup_user() rather than duplicating its implementation

[walter harms]

Am 20.08.2016 08:01, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Date: Sat, 20 Aug 2016 07:50:09 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.
> 
> Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 6388bc0..bb89f04 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> @@ -1132,7 +1132,6 @@ static ssize_t data_write(struct file *filp, const char __user *buf,
>  	struct mlx5_core_dev *dev = filp->private_data;
>  	struct mlx5_cmd_debug *dbg = &dev->cmd.dbg;
>  	void *ptr;
> -	int err;
>  
>  	if (*pos != 0)
>  		return -EINVAL;
> @@ -1140,25 +1139,15 @@ static ssize_t data_write(struct file *filp, const char __user *buf,
>  	kfree(dbg->in_msg);
>  	dbg->in_msg = NULL;
>  	dbg->inlen = 0;
> -
> -	ptr = kzalloc(count, GFP_KERNEL);
> -	if (!ptr)
> -		return -ENOMEM;
> -
> -	if (copy_from_user(ptr, buf, count)) {
> -		err = -EFAULT;
> -		goto out;
> -	}
> +	ptr = memdup_user(buf, count);
> +	if (IS_ERR(ptr))
> +		return PTR_ERR(ptr);
>  	dbg->in_msg = ptr;
>  	dbg->inlen = count;
>  
>  	*pos = count;
>  

maybe i am missing something here but why do you need ptr ?

The use of count looks even more confusing it is stored in
 dbg->inlen, *pos and is returned.
is that realy needed ?

re,
 wh

>  	return count;
> -
> -out:
> -	kfree(ptr);
> -	return err;
>  }
>  
>  static ssize_t data_read(struct file *filp, char __user *buf, size_t count,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlx5/core: Use memdup_user() rather than duplicating its implementation

[David Miller]

From: SF Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Date: Sat, 20 Aug 2016 08:01:22 +0200

> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Date: Sat, 20 Aug 2016 07:50:09 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.
> 
> Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>

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

[PATCH 0/2] tun: Fine-tuning for update_filter()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 09:16:16 +0200

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

Markus Elfring (2):
  Use memdup_user()
  Rename a jump label

 drivers/net/tun.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

-- 
2.9.3

[PATCH 1/2] tun: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 08:54:15 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/tun.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9c8b5bc..a1aeccb 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -731,14 +731,9 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
 	}
 
 	alen = ETH_ALEN * uf.count;
-	addr = kmalloc(alen, GFP_KERNEL);
-	if (!addr)
-		return -ENOMEM;
-
-	if (copy_from_user(addr, arg + sizeof(uf), alen)) {
-		err = -EFAULT;
-		goto done;
-	}
+	addr = memdup_user(arg + sizeof(uf), alen);
+	if (IS_ERR(addr))
+		return PTR_ERR(addr);
 
 	/* The filter is updated without holding any locks. Which is
 	 * perfectly safe. We disable it first and in the worst
-- 
2.9.3

Re: [PATCH 1/2] tun: Use memdup_user() rather than duplicating its implementation

[Shmulik Ladkani]

Hi,

On Sat, 20 Aug 2016 09:34:56 +0200 SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 08:54:15 +0200
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Reviewed-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Re: [PATCH 1/2] tun: Use memdup_user() rather than duplicating its implementation

[Michael S. Tsirkin]

On Sat, Aug 20, 2016 at 09:34:56AM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 08:54:15 +0200
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/tun.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9c8b5bc..a1aeccb 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -731,14 +731,9 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
>  	}
>  
>  	alen = ETH_ALEN * uf.count;
> -	addr = kmalloc(alen, GFP_KERNEL);
> -	if (!addr)
> -		return -ENOMEM;
> -
> -	if (copy_from_user(addr, arg + sizeof(uf), alen)) {
> -		err = -EFAULT;
> -		goto done;
> -	}
> +	addr = memdup_user(arg + sizeof(uf), alen);
> +	if (IS_ERR(addr))
> +		return PTR_ERR(addr);
>  
>  	/* The filter is updated without holding any locks. Which is
>  	 * perfectly safe. We disable it first and in the worst
> -- 
> 2.9.3

[PATCH 2/2] tun: Rename a jump label in update_filter()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 09:00:34 +0200

Adjust a jump target according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/tun.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a1aeccb..e249428 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -753,7 +753,7 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
 	for (; n < uf.count; n++) {
 		if (!is_multicast_ether_addr(addr[n].u)) {
 			err = 0; /* no filter */
-			goto done;
+			goto free_addr;
 		}
 		addr_hash_set(filter->mask, addr[n].u);
 	}
@@ -769,8 +769,7 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
 
 	/* Return the number of exact filters */
 	err = nexact;
-
-done:
+free_addr:
 	kfree(addr);
 	return err;
 }
-- 
2.9.3

Re: [PATCH 2/2] tun: Rename a jump label in update_filter()

[Michael S. Tsirkin]

On Sat, Aug 20, 2016 at 09:37:16AM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 09:00:34 +0200
> 
> Adjust a jump target according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

I don't have an opinion of this one. Which convention do you refer to?

> ---
>  drivers/net/tun.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index a1aeccb..e249428 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -753,7 +753,7 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
>  	for (; n < uf.count; n++) {
>  		if (!is_multicast_ether_addr(addr[n].u)) {
>  			err = 0; /* no filter */
> -			goto done;
> +			goto free_addr;
>  		}
>  		addr_hash_set(filter->mask, addr[n].u);
>  	}
> @@ -769,8 +769,7 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
>  
>  	/* Return the number of exact filters */
>  	err = nexact;
> -
> -done:
> +free_addr:
>  	kfree(addr);
>  	return err;
>  }
> -- 
> 2.9.3

Re: [PATCH 2/2] tun: Rename a jump label in update_filter()

[Mike Rapoport]

On Mon, Aug 22, 2016 at 04:41:11AM +0300, Michael S. Tsirkin wrote:
> On Sat, Aug 20, 2016 at 09:37:16AM +0200, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sat, 20 Aug 2016 09:00:34 +0200
> > 
> > Adjust a jump target according to the Linux coding style convention.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> I don't have an opinion of this one. Which convention do you refer to?

Citing Documentation/CodingStyle:

Choose label names which say what the goto does or why the goto exists.  An
example of a good name could be "out_buffer:" if the goto frees "buffer".
Avoid using GW-BASIC names like "err1:" and "err2:".  Also don't name them after
the goto location like "err_kmalloc_failed:"

> 
> > ---
> >  drivers/net/tun.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index a1aeccb..e249428 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -753,7 +753,7 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
> >  	for (; n < uf.count; n++) {
> >  		if (!is_multicast_ether_addr(addr[n].u)) {
> >  			err = 0; /* no filter */
> > -			goto done;
> > +			goto free_addr;
> >  		}
> >  		addr_hash_set(filter->mask, addr[n].u);
> >  	}
> > @@ -769,8 +769,7 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
> >  
> >  	/* Return the number of exact filters */
> >  	err = nexact;
> > -
> > -done:
> > +free_addr:
> >  	kfree(addr);
> >  	return err;
> >  }
> > -- 
> > 2.9.3
> 

Re: [PATCH 0/2] tun: Fine-tuning for update_filter()

[David Miller]

From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 09:27:39 +0200

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

Series applied.

[PATCH 0/3] hostap: Fine-tuning for a few functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:35:43 +0200

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

Markus Elfring (3):
  Use memdup_user()
  Delete an unnecessary jump label
  Delete unnecessary variable initialisations

 .../net/wireless/intersil/hostap/hostap_ioctl.c    | 36 ++++++++--------------
 1 file changed, 12 insertions(+), 24 deletions(-)

-- 
2.9.3

[PATCH 1/3] hostap: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:19:43 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 .../net/wireless/intersil/hostap/hostap_ioctl.c    | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 3e5fa78..4e271f9 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -3041,14 +3041,9 @@ static int prism2_ioctl_priv_download(local_info_t *local, struct iw_point *p)
 	    p->length > 1024 || !p->pointer)
 		return -EINVAL;
 
-	param = kmalloc(p->length, GFP_KERNEL);
-	if (param == NULL)
-		return -ENOMEM;
-
-	if (copy_from_user(param, p->pointer, p->length)) {
-		ret = -EFAULT;
-		goto out;
-	}
+	param = memdup_user(p->pointer, p->length);
+	if (IS_ERR(param))
+		return PTR_ERR(param);
 
 	if (p->length < sizeof(struct prism2_download_param) +
 	    param->num_areas * sizeof(struct prism2_download_area)) {
@@ -3803,14 +3798,9 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 	    p->length > PRISM2_HOSTAPD_MAX_BUF_SIZE || !p->pointer)
 		return -EINVAL;
 
-	param = kmalloc(p->length, GFP_KERNEL);
-	if (param == NULL)
-		return -ENOMEM;
-
-	if (copy_from_user(param, p->pointer, p->length)) {
-		ret = -EFAULT;
-		goto out;
-	}
+	param = memdup_user(p->pointer, p->length);
+	if (IS_ERR(param))
+		return PTR_ERR(param);
 
 	switch (param->cmd) {
 	case PRISM2_SET_ENCRYPTION:
-- 
2.9.3

Re: [1/3] hostap: Use memdup_user() rather than duplicating its implementation

[Kalle Valo]

SF Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote:
> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Date: Sat, 20 Aug 2016 18:19:43 +0200
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>

Patch set to Rejected.

[1/3] hostap: Use memdup_user() rather than duplicating i... 2016-08-20 SF Markus El Rejected

Reason: A similar patch is already applied.

Applying: hostap: Use memdup_user() rather than duplicating its implementation
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging drivers/net/wireless/intersil/hostap/hostap_ioctl.c
CONFLICT (content): Merge conflict in drivers/net/wireless/intersil/hostap/hostap_ioctl.c
Failed to merge in the changes.
Patch failed at 0001 hostap: Use memdup_user() rather than duplicating its implementation

-- 
https://patchwork.kernel.org/patch/9306999/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:21:29 +0200

Remove a jump label which is unneeded in this function at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 4e271f9..5942917 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 	}
 
 	if (ret == 1 || !ap_ioctl) {
-		if (copy_to_user(p->pointer, param, p->length)) {
+		if (copy_to_user(p->pointer, param, p->length))
 			ret = -EFAULT;
-			goto out;
-		} else if (ap_ioctl)
+		else if (ap_ioctl)
 			ret = 0;
 	}
 
- out:
 	kfree(param);
 	return ret;
 }
-- 
2.9.3

Re: [PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Julian Calaby]

Hi Marcus,

On Sun, Aug 21, 2016 at 2:46 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:21:29 +0200
>
> Remove a jump label which is unneeded in this function at the end.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> index 4e271f9..5942917 100644
> --- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> +++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
> @@ -3835,14 +3835,12 @@ static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
>         }
>
>         if (ret == 1 || !ap_ioctl) {
> -               if (copy_to_user(p->pointer, param, p->length)) {
> +               if (copy_to_user(p->pointer, param, p->length))
>                         ret = -EFAULT;
> -                       goto out;
> -               } else if (ap_ioctl)
> +               else if (ap_ioctl)
>                         ret = 0;
>         }
>
> - out:

Does this change make any difference to the compiled code?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Kalle Valo]

SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 20 Aug 2016 18:21:29 +0200
> 
> Remove a jump label which is unneeded in this function at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

2 patches set to Rejected.

9291771 [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()
9291775 [3/3] hostap: Delete unnecessary initialisations for the variable "ret"

Reason: The benefit is not clear.

-- 
https://patchwork.kernel.org/patch/9291771/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[SF Markus Elfring]

> 9291771 [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()
> 9291775 [3/3] hostap: Delete unnecessary initialisations for the variable "ret"
> 
> Reason: The benefit is not clear.

How do you think about to reduce the source code a bit at these places?

Regards,
Markus

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Kalle Valo]

SF Markus Elfring <elfring@users.sourceforge.net> writes:

>> 9291771 [2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()
>> 9291775 [3/3] hostap: Delete unnecessary initialisations for the variable "ret"
>> 
>> Reason: The benefit is not clear.
>
> How do you think about to reduce the source code a bit at these places?

hostap is an obsolete driver, it's waste of time doing style fixes to it
as nobody maintains it anymore.

-- 
Kalle Valo

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[SF Markus Elfring]

> hostap is an obsolete driver, it's waste of time doing style fixes to it
> as nobody maintains it anymore.

Thanks for another bit of your software development attention and this information.

Is it easier to understand than the previous response "Reason: The benefit is not clear."?

Regards,
Markus

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Joe Perches]

On Mon, 2016-09-26 at 21:01 +0300, Kalle Valo wrote:
> hostap is an obsolete driver, it's waste of time doing style fixes to it
> as nobody maintains it anymore.

Dunno know if Jouni is still maintaining this at all
but maybe a MAINTAINERS update to mark it obsolete so
checkpatch warns on unnecessary changes.
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6e0a912c3b13..ff293e70fae6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5722,7 +5722,7 @@ M:	Jouni Malinen 
 L:	hostap@shmoo.com (subscribers-only)
 L:	linux-wireless@vger.kernel.org
 W:	http://hostap.epitest.fi/
-S:	Maintained
+S:	Maintained / Obsolete
 F:	drivers/net/wireless/intersil/hostap/
 
 HP COMPAQ TC1100 TABLET WMI EXTRAS DRIVER

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Kalle Valo]

Joe Perches <joe@perches.com> writes:

> On Mon, 2016-09-26 at 21:01 +0300, Kalle Valo wrote:
>> hostap is an obsolete driver, it's waste of time doing style fixes to it
>> as nobody maintains it anymore.
>
> Dunno know if Jouni is still maintaining this at all
> but maybe a MAINTAINERS update to mark it obsolete so
> checkpatch warns on unnecessary changes.
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6e0a912c3b13..ff293e70fae6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5722,7 +5722,7 @@ M:	Jouni Malinen 
>  L:	hostap@shmoo.com (subscribers-only)
>  L:	linux-wireless@vger.kernel.org
>  W:	http://hostap.epitest.fi/
> -S:	Maintained
> +S:	Maintained / Obsolete
>  F:	drivers/net/wireless/intersil/hostap/

I talked with Jouni and we concluded marking this fully obsolete is the
best (so removing the "Maintained" part completely). Also the shmoo list
is not used anymore, that can be removed.

-- 
Kalle Valo

Re: hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd()

[Joe Perches]

On Mon, 2016-09-26 at 21:37 +0300, Kalle Valo wrote:
> I talked with Jouni and we concluded marking this fully obsolete is the
> best (so removing the "Maintained" part completelo the shmoo list
> is not used anymore, that can be removed.

Well, it would be best if Jouni submitted something.

[PATCH 3/3] hostap: Delete unnecessary initialisations for the variable "ret"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 20 Aug 2016 18:23:14 +0200

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intersil/hostap/hostap_ioctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
index 5942917..c37b0bb 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_ioctl.c
@@ -2895,7 +2895,7 @@ static int prism2_ioctl_priv_monitor(struct net_device *dev, int *i)
 {
 	struct hostap_interface *iface;
 	local_info_t *local;
-	int ret = 0;
+	int ret;
 	u32 mode;
 
 	iface = netdev_priv(dev);
@@ -3035,7 +3035,7 @@ static int ap_mac_cmd_ioctl(local_info_t *local, int *cmd)
 static int prism2_ioctl_priv_download(local_info_t *local, struct iw_point *p)
 {
 	struct prism2_download_param *param;
-	int ret = 0;
+	int ret;
 
 	if (p->length < sizeof(struct prism2_download_param) ||
 	    p->length > 1024 || !p->pointer)
@@ -3791,7 +3791,7 @@ static int prism2_ioctl_scan_req(local_info_t *local,
 static int prism2_ioctl_priv_hostapd(local_info_t *local, struct iw_point *p)
 {
 	struct prism2_hostapd_param *param;
-	int ret = 0;
+	int ret;
 	int ap_ioctl = 0;
 
 	if (p->length < sizeof(struct prism2_hostapd_param) ||
@@ -3954,7 +3954,7 @@ int hostap_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	struct iwreq *wrq = (struct iwreq *) ifr;
 	struct hostap_interface *iface;
 	local_info_t *local;
-	int ret = 0;
+	int ret;
 
 	iface = netdev_priv(dev);
 	local = iface->local;
-- 
2.9.3

Re: [PATCH 0/3] hostap: Fine-tuning for a few functions

[Arend van Spriel]

On 20-08-16 18:43, SF Markus Elfring wrote:
> From: Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Date: Sat, 20 Aug 2016 18:35:43 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.

Is it worth touching this old stuff especially when you are not making
any functional changes.

Regards,
Arend

> Markus Elfring (3):
>   Use memdup_user()
>   Delete an unnecessary jump label
>   Delete unnecessary variable initialisations
> 
>  .../net/wireless/intersil/hostap/hostap_ioctl.c    | 36 ++++++++--------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
> 

Re: [PATCH 0/3] hostap: Fine-tuning for a few functions

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 20-08-16 18:43, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Sat, 20 Aug 2016 18:35:43 +0200
>> 
>> A few update suggestions were taken into account
>> from static source code analysis.
>
> Is it worth touching this old stuff especially when you are not making
> any functional changes.

On the other hand if these patches break something this might be a good
way to get feedback if someone is really using this driver ;)

But yeah, not really sure what to do with these obsolete drivers like
hostap, ray_cs and wl3501.

-- 
Kalle Valo

Re: [PATCH 0/3] hostap: Fine-tuning for a few functions

[Joe Perches]

On Mon, 2016-08-22 at 18:49 +0300, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
[]
> But yeah, not really sure what to do with these obsolete drivers like
> hostap, ray_cs and wl3501.

Maybe marking sections obsolete in MAINTAINERS could
flag some "shouldn't touch this" warning for old code
in checkpatch.pl and/or get_maintainer.pl

[PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

[Joe Perches]

Use get_maintainer to check the status of individual files.
If "obsolete", suggest leaving the files alone.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4de3cc4..df5e9d9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -704,6 +704,16 @@ sub seed_camelcase_file {
 	}
 }
 
+sub is_maintained_obsolete {
+	my ($filename) = @_;
+
+	return 0 if (!(-e "$root/scripts/get_maintainer.pl"));
+
+	my $status = `perl $root/scripts/get_maintainer.pl --status --nom --nol --nogit --nogit-fallback $filename 2>&1`;
+
+	return $status =~ /obsolete/i;
+}
+
 my $camelcase_seeded = 0;
 sub seed_camelcase_includes {
 	return if ($camelcase_seeded);
@@ -2289,6 +2299,10 @@ sub process {
 		}
 
 		if ($found_file) {
+			if (is_maintained_obsolete($realfile)) {
+				WARN("OBSOLETE",
+				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
+			}
 			if ($realfile =~ m@^(?:drivers/net/|net/|drivers/staging/)@) {
 				$check = 1;
 			} else {
-- 
2.8.0.rc4.16.g56331f8

Re: [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

[SF Markus Elfring]

> @@ -2289,6 +2299,10 @@ sub process {
>  		}
>  
>  		if ($found_file) {
> +			if (is_maintained_obsolete($realfile)) {
> +				WARN("OBSOLETE",
> +				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
> +			}

How do you think about to avoid a double negation in such a warning message?

Would a wording like "… Only really necessary modifications please.\n"
be more useful here?

Regards,
Markus

Re: [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS

[Joe Perches]

On Mon, 2016-08-22 at 22:50 +0200, SF Markus Elfring wrote:
> > @@ -2289,6 +2299,10 @@ sub process {
> >  		}
> >  
> >  		if ($found_file) {
> > +			if (is_maintained_obsolete($realfile)) {
> > +				WARN("OBSOLETE",
> > +				     "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy.  No unnecessary modifications please.\n");
> > +			}
> How do you think about to avoid a double negation in such a warning message?
> 
> Would a wording like "… Only really necessary modifications please.\n"
> be more useful here?

No, probably not.

Re: checkpatch: See if modified files are marked obsolete in MAINTAINERS

[SF Markus Elfring]

> Use get_maintainer to check the status of individual files.
> If "obsolete", suggest leaving the files alone.

Will another software system like the "kbuild test robot"
need any more fine-tuning for this change?

Regards,
Markus

Re: checkpatch: See if modified files are marked obsolete in MAINTAINERS

[Julia Lawall]

On Tue, 23 Aug 2016, SF Markus Elfring wrote:

> > Use get_maintainer to check the status of individual files.
> > If "obsolete", suggest leaving the files alone.
>
> Will another software system like the "kbuild test robot"
> need any more fine-tuning for this change?

It only works on files in which there have been commits, thus by
definition not obsolete.

julia


>
> Regards,
> Markus
> --
> 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
>

[PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Sep 2016 17:27:17 +0200

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

Markus Elfring (5):
  Use kmalloc_array() in two functions
  Improve another size determination in gigaset_initcs()
  Delete an error message for a failed memory allocation
  Release memory in gigaset_initcs() after an allocation failure
  Enclose two expressions for the sizeof operator by parentheses

 drivers/isdn/gigaset/common.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

-- 
2.10.0

[PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 25 Sep 2016 22:22:04 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/isdn/gigaset/common.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index 7c78144..cecbb6a 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -709,8 +709,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 
 	cs->mode = M_UNKNOWN;
 	cs->mstate = MS_UNINITIALIZED;
-
-	cs->bcs = kmalloc(channels * sizeof(struct bc_state), GFP_KERNEL);
+	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);
 	cs->inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
 	if (!cs->bcs || !cs->inbuf) {
 		pr_err("out of memory\n");
@@ -1089,8 +1088,7 @@ struct gigaset_driver *gigaset_initdriver(unsigned minor, unsigned minors,
 	drv->ops = ops;
 	drv->owner = owner;
 	INIT_LIST_HEAD(&drv->list);
-
-	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
+	drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);
 	if (!drv->cs)
 		goto error;
 
-- 
2.10.0

Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

[Paul Bolle]

On Mon, 2016-09-26 at 17:38 +0200, SF Markus Elfring wrote:
> * Multiplications for the size determination of memory allocations
>   indicated that array data structures should be processed.
>   Thus use the corresponding function "kmalloc_array".

Was the current code incorrect? What makes kmalloc_array() better? None
of this is obvious to me.

I'm not going to change code just because some checker suggests to do
so.

>   This issue was detected by using the Coccinelle software.

So? And which coccinelle script was actually used? I couldn't spot a
coccinelle script doing that in the current tree.

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

I'm not happy with you mixing this with the above, less trivial,
change.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

> --- a/drivers/isdn/gigaset/common.c
> +++ b/drivers/isdn/gigaset/common.c
> @@ -709,8 +709,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>  
>  	cs->mode = M_UNKNOWN;
>  	cs->mstate = MS_UNINITIALIZED;
> -

Unrelated whitespace change.

> -	cs->bcs = kmalloc(channels * sizeof(struct bc_state), GFP_KERNEL);
> +	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);

For the record: "channels" is basically hardcoded in the three gigaset
hardware drivers.

>  	cs->inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
>  	if (!cs->bcs || !cs->inbuf) {
>  		pr_err("out of memory\n");
> @@ -1089,8 +1088,7 @@ struct gigaset_driver
> *gigaset_initdriver(unsigned minor, unsigned minors,
>  	drv->ops = ops;
>  	drv->owner = owner;
>  	INIT_LIST_HEAD(&drv->list);
> -

Again unrelated whitespace change.

> -	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
> +	drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);

For "minors" the same holds as for "channels", above.

And you snuck in a parentheses change. That should have probably been
merged with 5/5.

>  	if (!drv->cs)
>  		goto error;


Paul Bolle

Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

[SF Markus Elfring]

>> * Multiplications for the size determination of memory allocations
>>   indicated that array data structures should be processed.
>>   Thus use the corresponding function "kmalloc_array".
> 
> Was the current code incorrect?

I suggest to use a safer interface for array allocations.


> What makes kmalloc_array() better?

1. How do you think about the safety checks that this function provides?

2. Will you be also affected by further software evolution here?
   2016-07-26
   mm: faster kmalloc_array(), kcalloc()
   https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=91c6a05f72a996bee5133e76374ab3ad7d3b9b72


> I'm not going to change code just because some checker suggests to do so.

The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kmalloc_array over kmalloc with multiply


>>   This issue was detected by using the Coccinelle software.
> 
> So? And which coccinelle script was actually used?

How do you think about to look into related information sources?
https://github.com/coccinelle/coccinelle/issues/81

Would you like to experiment any further with an excerpt?


@replacement1@
expression count, target;
type T;
@@
 target =
-         kmalloc(sizeof(T) * (count)
+         kmalloc_array(count, sizeof(T)
                        , ...);

@replacement2@
expression count, pointer, target;
@@
 target =
-         kmalloc(sizeof(*pointer) * (count)
+         kmalloc_array(count, sizeof(*pointer)
                        , ...);


> I couldn't spot a coccinelle script doing that in the current tree.

This is true for such a software update opportunity.


>> * Replace the specification of a data structure by a pointer dereference
>>   to make the corresponding size determination a bit safer according to
>>   the Linux coding style convention.
> 
> I'm not happy with you mixing this with the above, less trivial, change.

I find that it is a useful combination. - A parameter is adjusted together
with a special function name.


>> -	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
>> +	drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);
> 
> For "minors" the same holds as for "channels", above.
> 
> And you snuck in a parentheses change. That should have probably been
> merged with 5/5.

Would you prefer to add them in another update step?

Regards,
Markus

Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

[Paul Bolle]

On Wed, 2016-09-28 at 18:38 +0200, SF Markus Elfring wrote:
> > I'm not going to change code just because some checker suggests to
> > do so.
> 
> The script "checkpatch.pl" can point information out like the
> following.
> 
> WARNING: Prefer kmalloc_array over kmalloc with multiply

Am I being trolled?


Paul Bolle

[PATCH 2/5] ISDN-Gigaset: Improve another size determination in gigaset_initcs()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 25 Sep 2016 22:32:14 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/isdn/gigaset/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index cecbb6a..f505b42 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -710,7 +710,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 	cs->mode = M_UNKNOWN;
 	cs->mstate = MS_UNINITIALIZED;
 	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);
-	cs->inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
+	cs->inbuf = kmalloc(sizeof(*cs->inbuf), GFP_KERNEL);
 	if (!cs->bcs || !cs->inbuf) {
 		pr_err("out of memory\n");
 		goto error;
-- 
2.10.0

[PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Sep 2016 15:35:47 +0200

Omit an extra message for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/isdn/gigaset/common.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index f505b42..c05a2a4 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -712,7 +712,6 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);
 	cs->inbuf = kmalloc(sizeof(*cs->inbuf), GFP_KERNEL);
 	if (!cs->bcs || !cs->inbuf) {
-		pr_err("out of memory\n");
 		goto error;
 	}
 	++cs->cs_init;
-- 
2.10.0

Re: [PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed memory allocation

[Tilman Schmidt]

On Mon, Sep 26, 2016, at 17:42, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 26 Sep 2016 15:35:47 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> Link:
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

The patch is fine but the link in the commit message is irrelevant.
Please remove it.
(Yes, I read through the whole presentation to verify that. It was fun,
even.)

-- 
Tilman Schmidt
tilman@imap.cc

Re: [PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed memory allocation

[Paul Bolle]

On Tue, 2016-09-27 at 12:57 +0200, Tilman Schmidt wrote:
> On Mon, Sep 26, 2016, at 17:42, SF Markus Elfring wrote:
> > Omit an extra message for a memory allocation failure in this
> > function.
> > 
> > Link:
> > http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-R
> > efactor_Strings-WSang_0.pdf
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> The patch is fine but the link in the commit message is irrelevant.
> Please remove it.
> (Yes, I read through the whole presentation to verify that. It was fun,
> even.)

Agree.

Except this patch assumes the superfluous braces would be removed in
4/5. But it turns out that other patch must be dropped. It would have
been better to remove the braces in this patch.


Paul Bolle

[PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Sep 2016 16:30:50 +0200

Memory was not released (as it would be expected) when one call
of further resource reservations failed.

* Split a condition check for memory allocation failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/isdn/gigaset/common.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index c05a2a4..2e9382f 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -710,10 +710,13 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 	cs->mode = M_UNKNOWN;
 	cs->mstate = MS_UNINITIALIZED;
 	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);
+	if (!cs->bcs)
+		goto report_failure;
+
 	cs->inbuf = kmalloc(sizeof(*cs->inbuf), GFP_KERNEL);
-	if (!cs->bcs || !cs->inbuf) {
-		goto error;
-	}
+	if (!cs->inbuf)
+		goto free_bcs;
+
 	++cs->cs_init;
 
 	gig_dbg(DEBUG_INIT, "setting up at_state");
@@ -737,14 +740,14 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 	gig_dbg(DEBUG_INIT, "setting up iif");
 	if (gigaset_isdn_regdev(cs, modulename) < 0) {
 		pr_err("error registering ISDN device\n");
-		goto error;
+		goto free_bcs;
 	}
 
 	make_valid(cs, VALID_ID);
 	++cs->cs_init;
 	gig_dbg(DEBUG_INIT, "setting up hw");
 	if (cs->ops->initcshw(cs) < 0)
-		goto error;
+		goto free_bcs;
 
 	++cs->cs_init;
 
@@ -759,7 +762,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 		gig_dbg(DEBUG_INIT, "setting up bcs[%d]", i);
 		if (gigaset_initbcs(cs->bcs + i, cs, i) < 0) {
 			pr_err("could not allocate channel %d data\n", i);
-			goto error;
+			goto free_bcs;
 		}
 	}
 
@@ -772,8 +775,9 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 
 	gig_dbg(DEBUG_INIT, "cs initialized");
 	return cs;
-
-error:
+free_bcs:
+	kfree(cs->bcs);
+report_failure:
 	gig_dbg(DEBUG_INIT, "failed");
 	gigaset_freecs(cs);
 	return NULL;
-- 
2.10.0

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

Markus,

On Mon, 2016-09-26 at 17:43 +0200, SF Markus Elfring wrote:
> Memory was not released (as it would be expected) when one call
> of further resource reservations failed.

This was the only thing in this series that triggered more than a, very
uninspired, "meh" on first read.

> * Split a condition check for memory allocation failures so that
>   each pointer from these function calls will be checked immediately.
> 
>   See also background information:
>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>   Link: https://cwe.mitre.org/data/definitions/754.html

A quick scan of that link suggests we can do without the above
"background information" in the commit explanation.

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

Another "meh".

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/isdn/gigaset/common.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/common.c
> b/drivers/isdn/gigaset/common.c
> index c05a2a4..2e9382f 100644
> --- a/drivers/isdn/gigaset/common.c
> +++ b/drivers/isdn/gigaset/common.c
> @@ -710,10 +710,13 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>  	cs->mode = M_UNKNOWN;
>  	cs->mstate = MS_UNINITIALIZED;
>  	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);
> +	if (!cs->bcs)
> +		goto report_failure;
> +
>  	cs->inbuf = kmalloc(sizeof(*cs->inbuf), GFP_KERNEL);
> -	if (!cs->bcs || !cs->inbuf) {
> -		goto error;
> -	}
> +	if (!cs->inbuf)
> +		goto free_bcs;
> +
>  	++cs->cs_init;
>  
>  	gig_dbg(DEBUG_INIT, "setting up at_state");
> @@ -737,14 +740,14 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>  	gig_dbg(DEBUG_INIT, "setting up iif");
>  	if (gigaset_isdn_regdev(cs, modulename) < 0) {
>  		pr_err("error registering ISDN device\n");
> -		goto error;
> +		goto free_bcs;
>  	}
>  
>  	make_valid(cs, VALID_ID);
>  	++cs->cs_init;
>  	gig_dbg(DEBUG_INIT, "setting up hw");
>  	if (cs->ops->initcshw(cs) < 0)
> -		goto error;
> +		goto free_bcs;
>  
>  	++cs->cs_init;
>  
> @@ -759,7 +762,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>  		gig_dbg(DEBUG_INIT, "setting up bcs[%d]", i);
>  		if (gigaset_initbcs(cs->bcs + i, cs, i) < 0) {
>  			pr_err("could not allocate channel %d data\n", i);
> -			goto error;
> +			goto free_bcs;
>  		}
>  	}
>  
> @@ -772,8 +775,9 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>  
>  	gig_dbg(DEBUG_INIT, "cs initialized");
>  	return cs;
> -
> -error:
> +free_bcs:
> +	kfree(cs->bcs);
> +report_failure:
>  	gig_dbg(DEBUG_INIT, "failed");
>  	gigaset_freecs(cs);

gigaset_freecs() is not a function I look at for the fun of it. But
still, in it we find:

	case 0: /* error in basic setup */
		[...]
		kfree(cs->inbuf);
		kfree(cs->bcs);

As far as I can tell we will call those two kfree()'s if we jump to
"error". So, contrary to your analysis, I don't think we leak cs->bcs.

>  	return NULL;


Paul Bolle

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> Memory was not released (as it would be expected) when one call
>> of further resource reservations failed.
> 
> This was the only thing in this series that triggered more than a,
> very uninspired, "meh" on first read.

Will it matter here if the function "kfree" will be called for the
data structure members "bcs" and "inbuf" after a later function call
failed within the implementation of "gigaset_initcs"?

Regards,
Markus

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

On Tue, 2016-09-27 at 07:20 +0200, SF Markus Elfring wrote:
> Will it matter here if the function "kfree" will be called for the
> data structure members "bcs" and "inbuf" after a later function call
> failed within the implementation of "gigaset_initcs"?

My translation of this question is: could you please hold my hand while
I read the code of a driver I do not use - a driver for hardware that I
don't even have, and therefor cannot really test - after I submitted a
patch that appears to be broken?

My answer to that question is: no, sorry, I won't do that.


Paul Bolle

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> Will it matter here if the function "kfree" will be called for the
>> data structure members "bcs" and "inbuf" after a later function call
>> failed within the implementation of "gigaset_initcs"?
> 
> My translation of this question is: could you please hold my hand while
> I read the code of a driver I do not use - a driver for hardware that I
> don't even have, and therefor cannot really test - after I submitted a
> patch that appears to be broken?

I got the impression that the exception handling  was incomplete in the
implementation of the function "gigaset_initcs".
Does anybody (besides me) care for improving the software situation there?


> My answer to that question is: no, sorry, I won't do that.

I find that the process has just started once more to clarify in which
directions this software module could evolve.

Do we discuss a potential memory leak under special conditions here?

Regards,
Markus

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

You're in Eliza mode again.

(Hat tip to Björn Mork, https://lkml.org/lkml/2016/1/4/259).


Paul Bolle

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

Hi,

as longtime maintainer of the code in question I feel compelled to chime
in at this point.

On Tue, Sep 27, 2016, at 11:34, SF Markus Elfring wrote:
> >> Will it matter here if the function "kfree" will be called for the
> >> data structure members "bcs" and "inbuf" after a later function call
> >> failed within the implementation of "gigaset_initcs"?
> > 
> > My translation of this question is: could you please hold my hand while
> > I read the code of a driver I do not use - a driver for hardware that I
> > don't even have, and therefor cannot really test - after I submitted a
> > patch that appears to be broken?
> 
> I got the impression that the exception handling  was incomplete in the
> implementation of the function "gigaset_initcs".

That impression is wrong. Careful reading of the code will confirm that.

> Does anybody (besides me) care for improving the software situation
> there?

There's no urgent need for improvement. The code is stable and there's
no demonstrated bug to be fixed.
You could improve the coding style, but that is of secondary importance,
and if you want to do that, as a minimum you have to make sure that you
don't introduce new bugs.

Thanks,
Tilman

-- 
Tilman Schmidt
tilman@imap.cc

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> I got the impression that the exception handling  was incomplete in the
>> implementation of the function "gigaset_initcs".
> 
> That impression is wrong. Careful reading of the code will confirm that.

* Is it still correct nowadays that the function "gigaset_initcs" did not
  call the function "kfree" after a later function call failed?

* Do you expect that allocated memory will be automatically reclaimed
  after it would return a null pointer?

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

On Tue, Sep 27, 2016, at 13:32, SF Markus Elfring wrote:
> >> I got the impression that the exception handling  was incomplete in the
> >> implementation of the function "gigaset_initcs".
> > 
> > That impression is wrong. Careful reading of the code will confirm that.
> 
> * Is it still correct nowadays that the function "gigaset_initcs" did not
>   call the function "kfree" after a later function call failed?

Wrong premise. That statement was never correct.

> * Do you expect that allocated memory will be automatically reclaimed
>   after it would return a null pointer?

No. Should I? Do you?

Regards,
Tilman

-- 
Tilman Schmidt
tilman@imap.cc

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[isdn]

Am 27.09.2016 um 13:32 schrieb SF Markus Elfring:
>>> I got the impression that the exception handling  was incomplete in the
>>> implementation of the function "gigaset_initcs".
>>
>> That impression is wrong. Careful reading of the code will confirm that.
> 
> * Is it still correct nowadays that the function "gigaset_initcs" did not
>   call the function "kfree" after a later function call failed?
> 

Yes, if it is handled in another place, Paul already did show you the place.


> * Do you expect that allocated memory will be automatically reclaimed
>   after it would return a null pointer?
> 
Of course not

Best regards
Karsten

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> * Is it still correct nowadays that the function "gigaset_initcs" did not
>>   call the function "kfree" after a later function call failed?
> 
> Yes, if it is handled in another place, Paul already did show you the place.

To which source code place do you refer here?


>> * Do you expect that allocated memory will be automatically reclaimed
>>   after it would return a null pointer?
>>
> Of course not

Thanks for this acknowledgement.

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

On Tue, Sep 27, 2016, at 14:52, SF Markus Elfring wrote:
> >> * Is it still correct nowadays that the function "gigaset_initcs" did not
> >>   call the function "kfree" after a later function call failed?
> > 
> > Yes, if it is handled in another place, Paul already did show you the place.
> 
> To which source code place do you refer here?

Obviously the one Paul pointed out to you in detail in his mail dated
Mon, 26 Sep 2016 23:13:54 +0200.

HTH,
Tilman

-- 
Tilman Schmidt
tilman@imap.cc

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Dan Carpenter]

Ah well...  Someone else discovered the double free bug first and gave
it away.  Reassuring, I guess.

regards,
dan carpenter

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> @@ -772,8 +775,9 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>>  
>>  	gig_dbg(DEBUG_INIT, "cs initialized");
>>  	return cs;
>> -
>> -error:
>> +free_bcs:
>> +	kfree(cs->bcs);
>> +report_failure:
>>  	gig_dbg(DEBUG_INIT, "failed");
>>  	gigaset_freecs(cs);
> 
> gigaset_freecs() is not a function I look at for the fun of it. But
> still, in it we find:
> 
> 	case 0: /* error in basic setup */
> 		[...]
> 		kfree(cs->inbuf);
> 		kfree(cs->bcs);
> 
> As far as I can tell we will call those two kfree()'s if we jump to
> "error". So, contrary to your analysis, I don't think we leak cs->bcs.

You are right.

Thanks that you pointed this source code place out again.

I imagined that the exception handling implementation could be more direct
somehow for a while. But this function takes extra care for data synchronisation
by a mutex.

Now I recognise also that this proposed update step "4" was inappropriate.
I'm sorry for the confusion I introduced here.

Regards,
Markus

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Dan Carpenter]

This patch creates new bugs.

I have a policy of not telling Markus where the bug is, because
otherwise he'll just resend the patch and I have told him many times to
stop sending these cleanup patches that just introduce bugs and waste
maintainer time.

regards,
dan carpenter

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

> This patch creates new bugs.

Thanks for your information.


> I have a policy of not telling Markus where the bug is,

I find this kind of response strange.


> because otherwise he'll just resend the patch

This can also happen when the other contributors request it.


> and I have told him many times to stop sending these cleanup patches

Software "cleanups" seem to stress the review process to some degree.


> that just introduce bugs and waste maintainer time.

I guess that the situation is mixed depending on the subsystem
or concrete software module, isn't it?

Regards,
Markus

[PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Sep 2016 17:03:56 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script "checkpatch.pl" can point information out like the following.

WARNING: sizeof … should be sizeof(…)

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/isdn/gigaset/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index 2e9382f..d901ed7 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
 {
 	unsigned char outbuf[80];
 	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf - 1);
 	unsigned char *out = outbuf;
 	size_t numin = len;
 
@@ -1079,7 +1079,7 @@ struct gigaset_driver *gigaset_initdriver(unsigned minor, unsigned minors,
 	unsigned long flags;
 	unsigned i;
 
-	drv = kmalloc(sizeof *drv, GFP_KERNEL);
+	drv = kmalloc(sizeof(*drv), GFP_KERNEL);
 	if (!drv)
 		return NULL;
 
-- 
2.10.0

RE: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[David Laight]

From: SF Markus Elfring
> Sent: 26 September 2016 16:45
...
> The script "checkpatch.pl" can point information out like the following.
> 
> WARNING: sizeof … should be sizeof(…)
...
> ---
>  drivers/isdn/gigaset/common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
> index 2e9382f..d901ed7 100644
> --- a/drivers/isdn/gigaset/common.c
> +++ b/drivers/isdn/gigaset/common.c
> @@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
>  {
>  	unsigned char outbuf[80];
>  	unsigned char c;
> -	size_t space = sizeof outbuf - 1;
> +	size_t space = sizeof(outbuf - 1);

wrong ...
think that is 7 instead of 79.

	David

Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[Joe Perches]

On Mon, 2016-09-26 at 16:00 +0000, David Laight wrote:
> From: SF Markus Elfring Sent: 26 September 2016 16:45
> > The script "checkpatch.pl" can point information out like the following.
> > WARNING: sizeof … should be sizeof(…)
> []
> > diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
[]
> > @@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
> >  {
> >  	unsigned char outbuf[80];
> >  	unsigned char c;
> > -	size_t space = sizeof outbuf - 1;
> > +	size_t space = sizeof(outbuf - 1);

> wrong ...
> think that is 7 instead of 79.


Well, it's sizeof(pointer) so either 4 or 8, but still,
what Markus proposes here is _wrong_.

Markus, do you know the script probably has a lower
style conversion defect introduction rate than you do
when doing these mechanical things.

Please use it to verify what you are doing before
submitting more defective patches.

Excuses and apologies aren't good enough at this point.

Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[SF Markus Elfring]

>>> @@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
>>>  {
>>>  	unsigned char outbuf[80];
>>>  	unsigned char c;
>>> -	size_t space = sizeof outbuf - 1;
>>> +	size_t space = sizeof(outbuf - 1);
> 
>> wrong ...
>> think that is 7 instead of 79.
> 
> 
> Well, it's sizeof(pointer) so either 4 or 8, but still,
> what Markus proposes here is _wrong_.

You are right at this place.


> Markus, do you know the script probably has a lower
> style conversion defect introduction rate than you do
> when doing these mechanical things.

Would it be nice if the script "checkpatch.pl" could avoid to point
such a false positive out anyhow?


> Please use it to verify what you are doing before
> submitting more defective patches.

Do you expect a resend for this update step so that an other statement
in the function "gigaset_initdriver" would eventually be adjusted?

Regards,
Markus

Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[Julia Lawall]

On Mon, 26 Sep 2016, SF Markus Elfring wrote:

> >>> @@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
> >>>  {
> >>>  	unsigned char outbuf[80];
> >>>  	unsigned char c;
> >>> -	size_t space = sizeof outbuf - 1;
> >>> +	size_t space = sizeof(outbuf - 1);
> >
> >> wrong ...
> >> think that is 7 instead of 79.
> >
> >
> > Well, it's sizeof(pointer) so either 4 or 8, but still,
> > what Markus proposes here is _wrong_.
>
> You are right at this place.
>
>
> > Markus, do you know the script probably has a lower
> > style conversion defect introduction rate than you do
> > when doing these mechanical things.
>
> Would it be nice if the script "checkpatch.pl" could avoid to point
> such a false positive out anyhow?

checkpatch reports:

WARNING: sizeof outbuf should be sizeof(outbuf)
#56: FILE: drivers/isdn/gigaset/common.c:56:
+	size_t space = sizeof outbuf - 1;

There is nothing wrong with the report,

julia

>
>
> > Please use it to verify what you are doing before
> > submitting more defective patches.
>
> Do you expect a resend for this update step so that an other statement
> in the function "gigaset_initdriver" would eventually be adjusted?
>
> Regards,
> Markus
> --
> 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
>

[PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Sep 2016 19:34:27 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script "checkpatch.pl" can point information out like the following.

WARNING: sizeof … should be sizeof(…)

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2: Position the desired closing parenthesis behind the variable name for
    a character buffer.

 drivers/isdn/gigaset/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index 2e9382f..dba44e1 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
 {
 	unsigned char outbuf[80];
 	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf) - 1;
 	unsigned char *out = outbuf;
 	size_t numin = len;
 
@@ -1079,7 +1079,7 @@ struct gigaset_driver *gigaset_initdriver(unsigned minor, unsigned minors,
 	unsigned long flags;
 	unsigned i;
 
-	drv = kmalloc(sizeof *drv, GFP_KERNEL);
+	drv = kmalloc(sizeof(*drv), GFP_KERNEL);
 	if (!drv)
 		return NULL;
 
-- 
2.10.0

Re: [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[Paul Bolle]

On Mon, 2016-09-26 at 19:44 +0200, SF Markus Elfring wrote:
> v2: Position the desired closing parenthesis behind the variable name for
>     a character buffer.

Please wait a reasonable amount of time (say a week or two) to collect
all feedback on all patches of a series. If there's feedback you should
resend the entire series (minus those patches that have been NAK-ed, of
course) as an update (v2, v3, etc.).

Don't resubmit one single patch of a series (within hours!) as you did
here.

I hope to have a look at your series within a few days. Show some
patience.

Thanks,


Paul Bolle

Re: [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[SF Markus Elfring]

> Don't resubmit one single patch of a series (within hours!)
> as you did here.

I hope that I could reduce the confusion a bit which I introduced
with a change in the last step of my questionable update suggestion.


> I hope to have a look at your series within a few days.
> Show some patience.

Yes, of course.

Regards,
Markus

Re: [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[David Miller]

When you need to make changes to patches that are part of a series,
you must resubmit the entire series, not just the things that
are changes.

Re: [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[SF Markus Elfring]

> When you need to make changes to patches that are part of a series,
> you must resubmit the entire series,

I imagine that will happen when the patch review time passed by a bit
more as Paul Bolle requested it yesterday.


> not just the things that are changes.

Thanks for your reminder.

Regards,
Markus

Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[Sergei Shtylyov]

Hello.

On 09/26/2016 06:44 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 26 Sep 2016 17:03:56 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The script "checkpatch.pl" can point information out like the following.
>
> WARNING: sizeof … should be sizeof(…)
>
> Thus fix the affected source code places.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/isdn/gigaset/common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
> index 2e9382f..d901ed7 100644
> --- a/drivers/isdn/gigaset/common.c
> +++ b/drivers/isdn/gigaset/common.c
> @@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
>  {
>  	unsigned char outbuf[80];
>  	unsigned char c;
> -	size_t space = sizeof outbuf - 1;
> +	size_t space = sizeof(outbuf - 1);

    What?! Does that compile?

[...]

MBR, Sergei

Re: ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[SF Markus Elfring]

>> @@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
>>  {
>>      unsigned char outbuf[80];
>>      unsigned char c;
>> -    size_t space = sizeof outbuf - 1;
>> +    size_t space = sizeof(outbuf - 1);
> 
>    What?! Does that compile?

Yes, of course. - The calculated value will be lower than intended.

Unfortunately, I stumbled on one of my own programming mistakes once again.


I sent a corrected update suggestion for further considerations a few minutes ago.

Regards,
Markus

Re: ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[Paul Bolle]

Where did the [PATCH 5/5] part of the subject go? You didn't drop it,
did you? Because that's surprisingly annoying.


Paul Bolle

Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[Dan Carpenter]

On Mon, Sep 26, 2016 at 08:38:14PM +0300, Sergei Shtylyov wrote:
> >@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
> > {
> > 	unsigned char outbuf[80];
> > 	unsigned char c;
> >-	size_t space = sizeof outbuf - 1;
> >+	size_t space = sizeof(outbuf - 1);
> 
>    What?! Does that compile?
> 
> [...]

It prints a Smatch warning.  Smatch ignores these if they happen inside
a macro where you pass a pointer and it takes the sizeof() the argument.

regards,
dan carpenter

Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof operator by parentheses

[Dan Carpenter]

On Tue, Sep 27, 2016 at 10:08:37AM +0300, Dan Carpenter wrote:
> On Mon, Sep 26, 2016 at 08:38:14PM +0300, Sergei Shtylyov wrote:
> > >@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
> > > {
> > > 	unsigned char outbuf[80];
> > > 	unsigned char c;
> > >-	size_t space = sizeof outbuf - 1;
> > >+	size_t space = sizeof(outbuf - 1);
> > 
> >    What?! Does that compile?
> > 
> > [...]
> 
> It prints a Smatch warning.  Smatch ignores these if they happen inside
> a macro where you pass a pointer and it takes the sizeof() the argument.

Reading that again, I realize it's not clear.  Smatch ignores these any
time they happen in a macro whether they're valid or not.  (Many times
they are valid).

regards,
dan carpenter

Re: [PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function implementations

[Paul Bolle]

On Mon, 2016-09-26 at 17:37 +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 26 Sep 2016 17:27:17 +0200
> 
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>   Use kmalloc_array() in two functions
>   Improve another size determination in gigaset_initcs()
>   Delete an error message for a failed memory allocation
>   Release memory in gigaset_initcs() after an allocation failure

Which "static source code analysis" was used for that discovery?

>   Enclose two expressions for the sizeof operator by parentheses
> 
>  drivers/isdn/gigaset/common.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)


Paul Bolle

Re: ISDN-Gigaset: Fine-tuning for three function implementations

[SF Markus Elfring]

>>   Use kmalloc_array() in two functions
>>   Improve another size determination in gigaset_initcs()
>>   Delete an error message for a failed memory allocation
>>   Release memory in gigaset_initcs() after an allocation failure
> 
> Which "static source code analysis" was used for that discovery?

Are you eventually asking more for the development tools which were involved here?

* Coccinelle software

* Script "checkpatch.pl"

* My own eyes with help of a current text editor and its programming support

Regards,
Markus

Re: [PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function implementations

[Paul Bolle]

On Mon, 2016-09-26 at 17:37 +0200, SF Markus Elfring wrote:
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>   Use kmalloc_array() in two functions
>   Improve another size determination in gigaset_initcs()
>   Delete an error message for a failed memory allocation
>   Release memory in gigaset_initcs() after an allocation failure
>   Enclose two expressions for the sizeof operator by parentheses
> 
>  drivers/isdn/gigaset/common.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)

Two of the five patches introduced bugs. The rest of the series isn't
free of various nits either. Of course, I was in no mood to be lenient
when I looked at those three patches.

I won't take any of these patches, sorry.


Paul Bolle

Re: ISDN-Gigaset: Fine-tuning for three function implementations

[SF Markus Elfring]

> Two of the five patches introduced bugs. The rest of the series isn't
> free of various nits either. Of course, I was in no mood to be lenient
> when I looked at those three patches.
> 
> I won't take any of these patches, sorry.

Would you like to look once more into an improved patch series for this
software module a bit later?

Regards,
Markus

Re: ISDN-Gigaset: Fine-tuning for three function implementations

[Paul Bolle]

On Wed, 2016-09-28 at 18:50 +0200, SF Markus Elfring wrote:
> Would you like to look once more into an improved patch series for this
> software module a bit later?

I'm afraid I'm not looking forward to receiving an update of this
series, sorry.

Thanks,


Paul Bolle