[PATCH 0/3 v3] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[PATCH 0/3 v3] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[Pedro Marzo Perez]

The checkpatch.pl script reports several errors and warnings at file
ieee80211_crypt_wep.c, this series of patches fixes them.

Pedro Marzo Perez (3):
  ieee80211_crypt_wep: Simplify error check code at prism2_wep_init
  ieee80211_crypt_wep: Remove two useless lines at ieee80211_wep_null
  ieee80211_crypt_wep: Correct include indentation and openning braces at new line

 .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 33 ++++++++--------------
 1 file changed, 12 insertions(+), 21 deletions(-)

-- 
1.9.1

[PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep_init

[Pedro Marzo Perez]

Merge two pr_debug lines with literal strings splitted across several lines
into one single line, simplifying prism2_wep_init error check code.

Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
---
 .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
index 0a17f84..388ed3c 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
@@ -9,6 +9,8 @@
  * more details.
  */
 
+#define pr_fmt(fmt) "ieee80211_crypt_wep: " fmt
+
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
@@ -43,20 +45,16 @@ static void *prism2_wep_init(int keyidx)
 
 	priv = kzalloc(sizeof(*priv), GFP_ATOMIC);
 	if (priv == NULL)
-		goto fail;
+		return NULL;
 	priv->key_idx = keyidx;
 
 	priv->tx_tfm = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(priv->tx_tfm)) {
-		pr_debug("ieee80211_crypt_wep: could not allocate "
-		       "crypto API arc4\n");
 		priv->tx_tfm = NULL;
 		goto fail;
 	}
 	priv->rx_tfm = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(priv->rx_tfm)) {
-		pr_debug("ieee80211_crypt_wep: could not allocate "
-		       "crypto API arc4\n");
 		priv->rx_tfm = NULL;
 		goto fail;
 	}
@@ -67,14 +65,12 @@ static void *prism2_wep_init(int keyidx)
 	return priv;
 
 fail:
-	if (priv) {
-		if (priv->tx_tfm)
-			crypto_free_blkcipher(priv->tx_tfm);
-		if (priv->rx_tfm)
-			crypto_free_blkcipher(priv->rx_tfm);
-		kfree(priv);
-	}
-
+	pr_debug("could not allocate crypto API arc4\n");
+	if (priv->tx_tfm)
+		crypto_free_blkcipher(priv->tx_tfm);
+	if (priv->rx_tfm)
+		crypto_free_blkcipher(priv->rx_tfm);
+	kfree(priv);
 	return NULL;
 }
 
-- 
1.9.1

Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep_init

[Sudip Mukherjee]

On Tue, May 19, 2015 at 01:32:22AM +0200, Pedro Marzo Perez wrote:
> Merge two pr_debug lines with literal strings splitted across several lines
> into one single line, simplifying prism2_wep_init error check code.

I would have split this patch into three.
1) introduce pr_fmt and remove "ieee80211_crypt_wep" from pr_debug
2) return NULL instead of that goto fail.
3) combine two pr_debug into a single at the fail block.

But Dan can say if its ok. (I am still taking ideas from Dan about my
patches)

regards
sudip
> 
> Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>

Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep_init

[pmarzo]

On Tue, 2015-05-19 at 11:04 +0530, Sudip Mukherjee wrote:
> On Tue, May 19, 2015 at 01:32:22AM +0200, Pedro Marzo Perez wrote:
> > Merge two pr_debug lines with literal strings splitted across several lines
> > into one single line, simplifying prism2_wep_init error check code.
> 
> I would have split this patch into three.
> 1) introduce pr_fmt and remove "ieee80211_crypt_wep" from pr_debug
> 2) return NULL instead of that goto fail.
> 3) combine two pr_debug into a single at the fail block.
> 
I firmly agree with the "one logical change-one patch" rule but this
seems to me a bit overkilling.Anyway I am a completely newbie sending
kernel patches so you are probably right.
> But Dan can say if its ok. (I am still taking ideas from Dan about my
> patches)
Sure. Dan, we need your wisdom!
> 
> regards
> sudip
thanku for your comments sudip!

regards 
pedro
> > 
> > Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>

Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep_init

[Dan Carpenter]

I was planning to leave this one for Greg but since you asked me to
comment...

This patch is ok as one patch.  The subject is "clean up
prism2_wep_init()" and that's one thing.  Breaking it up into tiny
patches would probably make it harder to review.

On Tue, May 19, 2015 at 01:32:22AM +0200, Pedro Marzo Perez wrote:
> Merge two pr_debug lines with literal strings splitted across several lines
> into one single line, simplifying prism2_wep_init error check code.
> 
> Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
> ---
>  .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> index 0a17f84..388ed3c 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> @@ -9,6 +9,8 @@
>   * more details.
>   */
>  
> +#define pr_fmt(fmt) "ieee80211_crypt_wep: " fmt

Greg doesn't like pr_fmt() in driver code, use dev_dbg() and friends
instead.  I don't like debug output generally so I say just delete it.
Especially in this case the debug output is pretty useless.

> +
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> @@ -43,20 +45,16 @@ static void *prism2_wep_init(int keyidx)
>  
>  	priv = kzalloc(sizeof(*priv), GFP_ATOMIC);
>  	if (priv == NULL)
> -		goto fail;
> +		return NULL;
>  	priv->key_idx = keyidx;
>  
>  	priv->tx_tfm = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
>  	if (IS_ERR(priv->tx_tfm)) {
> -		pr_debug("ieee80211_crypt_wep: could not allocate "
> -		       "crypto API arc4\n");
>  		priv->tx_tfm = NULL;
>  		goto fail;

This error handling is overly complicated.  It's supposed to be you
climb up a mountain to go hang gliding (return zero), but if you hit an
error on the way to the talk you walk backwards the way you came to the
bottom of the mountain.  You see all the landmarks in reverse order and
it makes you have a sad face.

allocate priv
allocate tx
allocate rx

return success;  <-- made it to the top

free tx <- walking back down.  :(
free priv

It should be:

	if (IS_ERR(priv->tx_tfm))
		goto free_priv;

>  	}
>  	priv->rx_tfm = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
>  	if (IS_ERR(priv->rx_tfm)) {
> -		pr_debug("ieee80211_crypt_wep: could not allocate "
> -		       "crypto API arc4\n");
>  		priv->rx_tfm = NULL;
>  		goto fail;
>  	}

	if (IS_ERR(priv->rx_tfm))
		goto free_tx;

If this allocation succeeds then we've made it to the top.  No need to
call crypto_free_blkcipher(priv->rx_tfm) in this function.

> @@ -67,14 +65,12 @@ static void *prism2_wep_init(int keyidx)
>  	return priv;
>  
>  fail:
> -	if (priv) {
> -		if (priv->tx_tfm)
> -			crypto_free_blkcipher(priv->tx_tfm);
> -		if (priv->rx_tfm)
> -			crypto_free_blkcipher(priv->rx_tfm);
> -		kfree(priv);
> -	}

In the original code there isn't a nice error path down the mountain,
it's all mixed together and not in any particular order.

> -
> +	pr_debug("could not allocate crypto API arc4\n");
> +	if (priv->tx_tfm)
> +		crypto_free_blkcipher(priv->tx_tfm);
> +	if (priv->rx_tfm)
> +		crypto_free_blkcipher(priv->rx_tfm);
> +	kfree(priv);
>  	return NULL;


free_tx:
	crypto_free_blkcipher(priv->tx_tfm);
free_priv:
	kfree(priv);

	return NULL;

regards,
dan carpenter

Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep_init

[pmarzo]

On mié, 2015-05-20 at 00:46 +0300, Dan Carpenter wrote:
> I was planning to leave this one for Greg but since you asked me to
> comment...
> 
> This patch is ok as one patch.  The subject is "clean up
> prism2_wep_init()" and that's one thing.  Breaking it up into tiny
> patches would probably make it harder to review.
> 
> On Tue, May 19, 2015 at 01:32:22AM +0200, Pedro Marzo Perez wrote:
> > Merge two pr_debug lines with literal strings splitted across several lines
> > into one single line, simplifying prism2_wep_init error check code.
> > 
> > Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
> > ---
> >  .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 22 +++++++++-------------
> >  1 file changed, 9 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > index 0a17f84..388ed3c 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > @@ -9,6 +9,8 @@
> >   * more details.
> >   */
> >  
> > +#define pr_fmt(fmt) "ieee80211_crypt_wep: " fmt
> 
> Greg doesn't like pr_fmt() in driver code, use dev_dbg() and friends
> instead.  I don't like debug output generally so I say just delete it.
> Especially in this case the debug output is pretty useless.
dev_dbg needs the device parameter, I don't see a way to get the device
pointer within this function. Anyway I think you are right, this debug
output is not very useful, if Greg agrees I will delete the line.
I am curious, why you dont like debug output? I usually have to adapt
the kernel to run in ARM based boards and I find debug output really
useful.

> > +
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/slab.h>
> > @@ -43,20 +45,16 @@ static void *prism2_wep_init(int keyidx)
> >  
> >  	priv = kzalloc(sizeof(*priv), GFP_ATOMIC);
> >  	if (priv == NULL)
> > -		goto fail;
> > +		return NULL;
> >  	priv->key_idx = keyidx;
> >  
> >  	priv->tx_tfm = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
> >  	if (IS_ERR(priv->tx_tfm)) {
> > -		pr_debug("ieee80211_crypt_wep: could not allocate "
> > -		       "crypto API arc4\n");
> >  		priv->tx_tfm = NULL;
> >  		goto fail;
> 
> This error handling is overly complicated.  It's supposed to be you
> climb up a mountain to go hang gliding (return zero), but if you hit an
> error on the way to the talk you walk backwards the way you came to the
> bottom of the mountain.  You see all the landmarks in reverse order and
> it makes you have a sad face.
Agree, it is still too complicated. The patch simplifies the original
code but it is still not completely right, and it does not follow kernel
error check style if error => goto

> 
> allocate priv
> allocate tx
> allocate rx
> 
> return success;  <-- made it to the top
> 
> free tx <- walking back down.  :(
> free priv
> 
> It should be:
> 
> 	if (IS_ERR(priv->tx_tfm))
> 		goto free_priv;
> 
> >  	}
> >  	priv->rx_tfm = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
> >  	if (IS_ERR(priv->rx_tfm)) {
> > -		pr_debug("ieee80211_crypt_wep: could not allocate "
> > -		       "crypto API arc4\n");
> >  		priv->rx_tfm = NULL;
> >  		goto fail;
> >  	}
> 
> 	if (IS_ERR(priv->rx_tfm))
> 		goto free_tx;
> 
> If this allocation succeeds then we've made it to the top.  No need to
> call crypto_free_blkcipher(priv->rx_tfm) in this function.
Agree, this check is useless, will never be called

> 
> > @@ -67,14 +65,12 @@ static void *prism2_wep_init(int keyidx)
> >  	return priv;
> >  
> >  fail:
> > -	if (priv) {
> > -		if (priv->tx_tfm)
> > -			crypto_free_blkcipher(priv->tx_tfm);
> > -		if (priv->rx_tfm)
> > -			crypto_free_blkcipher(priv->rx_tfm);
> > -		kfree(priv);
> > -	}
> 
> In the original code there isn't a nice error path down the mountain,
> it's all mixed together and not in any particular order.
> 
> > -
> > +	pr_debug("could not allocate crypto API arc4\n");
> > +	if (priv->tx_tfm)
> > +		crypto_free_blkcipher(priv->tx_tfm);
> > +	if (priv->rx_tfm)
> > +		crypto_free_blkcipher(priv->rx_tfm);
> > +	kfree(priv);
> >  	return NULL;
> 
> 
> free_tx:
> 	crypto_free_blkcipher(priv->tx_tfm);
> free_priv:
> 	kfree(priv);
> 
> 	return NULL;
> 

I will send an V4 patch, I am glad you don't charge a fee per patch
submitted and reviewed :-)
Anyway I will wait a couple of days to Greg's comments, so hopefully V4
will be the last.


> regards,
> dan carpenter

Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep_init

[Dan Carpenter]

On Wed, May 20, 2015 at 10:26:30AM +0200, pmarzo wrote:
> On mié, 2015-05-20 at 00:46 +0300, Dan Carpenter wrote:
> > I was planning to leave this one for Greg but since you asked me to
> > comment...
> > 
> > This patch is ok as one patch.  The subject is "clean up
> > prism2_wep_init()" and that's one thing.  Breaking it up into tiny
> > patches would probably make it harder to review.
> > 
> > On Tue, May 19, 2015 at 01:32:22AM +0200, Pedro Marzo Perez wrote:
> > > Merge two pr_debug lines with literal strings splitted across several lines
> > > into one single line, simplifying prism2_wep_init error check code.
> > > 
> > > Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
> > > ---
> > >  .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 22 +++++++++-------------
> > >  1 file changed, 9 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > > index 0a17f84..388ed3c 100644
> > > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > > @@ -9,6 +9,8 @@
> > >   * more details.
> > >   */
> > >  
> > > +#define pr_fmt(fmt) "ieee80211_crypt_wep: " fmt
> > 
> > Greg doesn't like pr_fmt() in driver code, use dev_dbg() and friends
> > instead.  I don't like debug output generally so I say just delete it.
> > Especially in this case the debug output is pretty useless.
> dev_dbg needs the device parameter, I don't see a way to get the device
> pointer within this function. Anyway I think you are right, this debug
> output is not very useful, if Greg agrees I will delete the line.

Greg doesn't care.  Don't wait for Greg.  He is a busy guy.

Greg is very predictable so I can normally tell you which patches will
be merged and which will not.

> I am curious, why you dont like debug output? I usually have to adapt
> the kernel to run in ARM based boards and I find debug output really
> useful.

These debug statements are never going to be printed in real life.  They
are a waste of RAM for no reason.  They make the code more complicated
which is why we are debating "how should we handle this thing?"  They
make the code slightly messier.  They can introduce NULL deref bugs in
code which never gets tested.

It's not that all debug code is bad, but so often people do it without
thinking because they think it's something they must do for every
function.  They think they are doing the right thing without realizing
that sometimes they are making the code worse.

Low level functions like crypto_alloc_blkcipher() should have debug code
and stack dumps on error.  It mostly does I think.  I think it assumes
that /lib/modules/ is not corrupt...

regards,
dan carpenter

Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep_init

[Greg KH]

On Tue, May 19, 2015 at 01:32:22AM +0200, Pedro Marzo Perez wrote:
> Merge two pr_debug lines with literal strings splitted across several lines
> into one single line, simplifying prism2_wep_init error check code.
> 
> Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
> ---
>  .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> index 0a17f84..388ed3c 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> @@ -9,6 +9,8 @@
>   * more details.
>   */
>  
> +#define pr_fmt(fmt) "ieee80211_crypt_wep: " fmt

Like Dan said, don't do this in a driver.  It's a driver, you almost
always have access to the device being operated on, so use the dev_*
functions.

This is a network driver, so use the netdev_* functions, which are even
better.  But never the pr_* functions, that's not ok.

Please fix up and resend.

thanks,

greg k-h

[PATCH 2/3 v3] Staging: rtl8192u: Remove two useless lines at ieee80211_wep_null

[Pedro Marzo Perez]

Removed two lines at ieee80211_wep_null which checkpatch.pl reported as errors.
The first one because it has a C99 comment style and the second one because it is a void
return which is useless.

Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
index 388ed3c..2ce7b54 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
@@ -289,6 +289,4 @@ void __exit ieee80211_crypto_wep_exit(void)
 
 void ieee80211_wep_null(void)
 {
-//	printk("============>%s()\n", __func__);
-	return;
 }
-- 
1.9.1

Re: [PATCH 2/3 v3] Staging: rtl8192u: Remove two useless lines at ieee80211_wep_null

[Greg KH]

On Tue, May 19, 2015 at 01:32:23AM +0200, Pedro Marzo Perez wrote:
> Removed two lines at ieee80211_wep_null which checkpatch.pl reported as errors.
> The first one because it has a C99 comment style and the second one because it is a void
> return which is useless.
> 
> Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
> ---
>  drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> index 388ed3c..2ce7b54 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> @@ -289,6 +289,4 @@ void __exit ieee80211_crypto_wep_exit(void)
>  
>  void ieee80211_wep_null(void)
>  {
> -//	printk("============>%s()\n", __func__);
> -	return;
>  }

You now have a function that does nothing, why not just delete it
entirely?

[PATCH 3/3 v3] Staging: rtl8192u: Correct include indentation and openning braces at new line

[Pedro Marzo Perez]

Opening braces should never be in a new line.
Correct include indentation.

Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
index 2ce7b54..4be468c 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
@@ -21,7 +21,7 @@
 #include "ieee80211.h"
 
 #include <linux/crypto.h>
-    #include <linux/scatterlist.h>
+#include <linux/scatterlist.h>
 #include <linux/crc32.h>
 
 MODULE_AUTHOR("Jouni Malinen");
@@ -138,9 +138,7 @@ static int prism2_wep_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
 	/* Copy rest of the WEP key (the secret part) */
 	memcpy(key + 3, wep->key, wep->key_len);
 
-	if (!tcb_desc->bHwSec)
-	{
-
+	if (!tcb_desc->bHwSec) {
 		/* Append little-endian CRC32 and encrypt it to produce ICV */
 		crc = ~crc32_le(~0, pos, len);
 		icv = skb_put(skb, 4);
@@ -197,8 +195,7 @@ static int prism2_wep_decrypt(struct sk_buff *skb, int hdr_len, void *priv)
 	/* Apply RC4 to data and compute CRC32 over decrypted data */
 	plen = skb->len - hdr_len - 8;
 
-	if (!tcb_desc->bHwSec)
-	{
+	if (!tcb_desc->bHwSec) {
 		crypto_blkcipher_setkey(wep->rx_tfm, key, klen);
 		sg_init_one(&sg, pos, plen+4);
 
-- 
1.9.1