Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[Dan Carpenter]

On Mon, May 18, 2015 at 07:36:23PM +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.
> Openning braces should never be in a new line, move them to the end of
> the previous line.
> Removed two useless lines at ieee80211_wep_null.
> 

When Joe said "Some will say this is doing too many things in a single
patch." he meant Greg.  Break this up into multiple patches.  Especially
now that you've added even more stuff to it.

regards,
dan carpenter

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[Sudip Mukherjee]

On Mon, May 18, 2015 at 07:36:23PM +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.
> Openning braces should never be in a new line, move them to the end of
> the previous line.
> Removed two useless lines at ieee80211_wep_null.

Like Joe Perches said in your last patch - you are doing too many things
in a single patch. break it into separate patches each doing a single
change.

> 
> Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
> ---
>  .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 33 ++++++++--------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> index 0a17f84..1074916 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.
>   */
>  
<snip>
> -
> +	pr_debug(pr_fmt("could not allocate crypto API arc4\n"));
this is not right.

regards
sudip

[PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[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.
Openning braces should never be in a new line, move them to the end of
the previous line.
Removed two useless lines at ieee80211_wep_null.

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

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
index 0a17f84..1074916 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>
@@ -19,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");
@@ -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(pr_fmt("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;
 }
 
@@ -142,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);
@@ -201,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);
 
@@ -293,6 +286,4 @@ void __exit ieee80211_crypto_wep_exit(void)
 
 void ieee80211_wep_null(void)
 {
-//	printk("============>%s()\n", __func__);
-	return;
 }
-- 
1.9.1

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[Joe Perches]

On Mon, 2015-05-18 at 23:56 +0200, pmarzo wrote:
> On Mon, 2015-05-18 at 19:15 +0300, Dan Carpenter wrote:
> > On Mon, May 18, 2015 at 07:36:23PM +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.
> > > Openning braces should never be in a new line, move them to the end of
> > > the previous line.
> > > Removed two useless lines at ieee80211_wep_null.
> > > 
> > 
> > When Joe said "Some will say this is doing too many things in a single
> > patch." he meant Greg.  Break this up into multiple patches.  Especially
> > now that you've added even more stuff to it.
> > 
> > regards,
> > dan carpenter
> 
> Sorry Joe, English is not my mother tongue and I misunderstand your
> comment, I thought "Some will say this is doing too many things in a
> single patch" did meant "You are on the limit, but it is ok to send it
> in one patch" but I see it really means "please, rewrite this whole crap
> into several patches" :-)

What I meant to say was I don't care as much about mixing
multiple types of fixes in a single patch as others might.

Greg does and generally rejects patches that mix fix types.

My preference is to do _all_ whitespace changes in a single
patch.  If "git diff -w" shows no source code changes, and
objdiff shows no object code change, that's a "single type"
of change to me.

Others disagree and may want whitespace changes broken down
into finer-grained blocks.

I think it's not necessary to modify the same line multiple
times just to get this granularity.

btw: using:

$ ./scripts/checkpatch.pl -f --strict --fix-inplace \
   --types=spacing,space_before_tab,pointer_location,trailing_whitespace,bracket_space,space_before_tab,indented_label,parenthesis_alignment \
   $file

can do this moderately well and there's a trivial script to
use checkpatch and git to create a patch series for various
checkpatch type style messages.

https://lkml.org/lkml/2014/7/11/794

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[Joe Perches]

On Tue, 2015-05-19 at 00:35 +0200, pmarzo wrote:
> On Mon, 2015-05-18 at 13:04 -0700, Joe Perches wrote:
> > btw: using:
> > 
> > $ ./scripts/checkpatch.pl -f --strict --fix-inplace \
> >    --types=spacing,space_before_tab,pointer_location,trailing_whitespace,bracket_space,space_before_tab,indented_label,parenthesis_alignment \
> >    $file
> > 
> Nice to see a script programs far better than me :-(

Nah...

The script is a stupid little tool.  People like you are
much better at making actually useful improvements to code.

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[Dan Carpenter]

On Mon, May 18, 2015 at 11:56:08PM +0200, pmarzo wrote:
> On Mon, 2015-05-18 at 19:15 +0300, Dan Carpenter wrote:
> > On Mon, May 18, 2015 at 07:36:23PM +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.
> > > Openning braces should never be in a new line, move them to the end of
> > > the previous line.
> > > Removed two useless lines at ieee80211_wep_null.
> > > 
> > 
> > When Joe said "Some will say this is doing too many things in a single
> > patch." he meant Greg.  Break this up into multiple patches.  Especially
> > now that you've added even more stuff to it.
> > 
> > regards,
> > dan carpenter
> 
> Sorry Joe, English is not my mother tongue and I misunderstand your
> comment, I thought "Some will say this is doing too many things in a
> single patch" did meant "You are on the limit, but it is ok to send it
> in one patch" but I see it really means "please, rewrite this whole crap
> into several patches" :-)

The line is a fuzzy line.  You were maybe on the line, maybe but then
you crossed over.

> 
> So I guess an v3 patch should be a chain of several patches, I think a
> sensible approach would be three patches
>   1/3 => rewrite of the error check code
>   2/3 => remove of the two useless lines at ieee80211_wep_null.
>   3/3 => move openning braces and correct the indentation of the
> #include
> 
> What do you think about it?

That sounds fine.

regards,
dan carpenter

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[pmarzo]

On Mon, 2015-05-18 at 21:53 +0530, Sudip Mukherjee wrote:
> On Mon, May 18, 2015 at 07:36:23PM +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.
> > Openning braces should never be in a new line, move them to the end of
> > the previous line.
> > Removed two useless lines at ieee80211_wep_null.
> 
> Like Joe Perches said in your last patch - you are doing too many things
> in a single patch. break it into separate patches each doing a single
> change.
> 
Agree
> > 
> > Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
> > ---
> >  .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 33 ++++++++--------------
> >  1 file changed, 12 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > index 0a17f84..1074916 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.
> >   */
> >  
> <snip>
> > -
> > +	pr_debug(pr_fmt("could not allocate crypto API arc4\n"));
> this is not right.
Completely agree
Will be fixed at v3

> 
> regards
> sudip

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[pmarzo]

On Mon, 2015-05-18 at 19:15 +0300, Dan Carpenter wrote:
> On Mon, May 18, 2015 at 07:36:23PM +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.
> > Openning braces should never be in a new line, move them to the end of
> > the previous line.
> > Removed two useless lines at ieee80211_wep_null.
> > 
> 
> When Joe said "Some will say this is doing too many things in a single
> patch." he meant Greg.  Break this up into multiple patches.  Especially
> now that you've added even more stuff to it.
> 
> regards,
> dan carpenter

Sorry Joe, English is not my mother tongue and I misunderstand your
comment, I thought "Some will say this is doing too many things in a
single patch" did meant "You are on the limit, but it is ok to send it
in one patch" but I see it really means "please, rewrite this whole crap
into several patches" :-)

So I guess an v3 patch should be a chain of several patches, I think a
sensible approach would be three patches
  1/3 => rewrite of the error check code
  2/3 => remove of the two useless lines at ieee80211_wep_null.
  3/3 => move openning braces and correct the indentation of the
#include

What do you think about it?

regards, Pedro

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[pmarzo]

On Mon, 2015-05-18 at 13:04 -0700, Joe Perches wrote:
> On Mon, 2015-05-18 at 23:56 +0200, pmarzo wrote:
> > On Mon, 2015-05-18 at 19:15 +0300, Dan Carpenter wrote:
> > > On Mon, May 18, 2015 at 07:36:23PM +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.
> > > > Openning braces should never be in a new line, move them to the end of
> > > > the previous line.
> > > > Removed two useless lines at ieee80211_wep_null.
> > > > 
> > > 
> > > When Joe said "Some will say this is doing too many things in a single
> > > patch." he meant Greg.  Break this up into multiple patches.  Especially
> > > now that you've added even more stuff to it.
> > > 
> > > regards,
> > > dan carpenter
> > 
> > Sorry Joe, English is not my mother tongue and I misunderstand your
> > comment, I thought "Some will say this is doing too many things in a
> > single patch" did meant "You are on the limit, but it is ok to send it
> > in one patch" but I see it really means "please, rewrite this whole crap
> > into several patches" :-)
> 
> What I meant to say was I don't care as much about mixing
> multiple types of fixes in a single patch as others might.
> 
> Greg does and generally rejects patches that mix fix types.
> 
> My preference is to do _all_ whitespace changes in a single
> patch.  If "git diff -w" shows no source code changes, and
> objdiff shows no object code change, that's a "single type"
> of change to me.
> 
> Others disagree and may want whitespace changes broken down
> into finer-grained blocks.
> 
> I think it's not necessary to modify the same line multiple
> times just to get this granularity.
> 
Thanku Joe, as Greg is the maintainer of the staging tree and is his
final decission I will split the patch

> btw: using:
> 
> $ ./scripts/checkpatch.pl -f --strict --fix-inplace \
>    --types=spacing,space_before_tab,pointer_location,trailing_whitespace,bracket_space,space_before_tab,indented_label,parenthesis_alignment \
>    $file
> 
Nice to see a script programs far better than me :-(

> can do this moderately well and there's a trivial script to
> use checkpatch and git to create a patch series for various
> checkpatch type style messages.
> 
> https://lkml.org/lkml/2014/7/11/794
> 
> 
Nice script to check I did not break anything!
However I have modified the error check code, so I guess the generated
object will be different. I will use it next time I try to correct style
issues (that would certainly not be in the near future :-) )

regards, Pedro.

Re: [PATCH v2] Staging: rtl8192u: Fix coding style issues at ieee80211_crypt_wep.c

[pmarzo]

On Tue, 2015-05-19 at 00:06 +0300, Dan Carpenter wrote:
> On Mon, May 18, 2015 at 11:56:08PM +0200, pmarzo wrote:
> > On Mon, 2015-05-18 at 19:15 +0300, Dan Carpenter wrote:
> > > On Mon, May 18, 2015 at 07:36:23PM +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.
> > > > Openning braces should never be in a new line, move them to the end of
> > > > the previous line.
> > > > Removed two useless lines at ieee80211_wep_null.
> > > > 
> > > 
> > > When Joe said "Some will say this is doing too many things in a single
> > > patch." he meant Greg.  Break this up into multiple patches.  Especially
> > > now that you've added even more stuff to it.
> > > 
> > > regards,
> > > dan carpenter
> > 
> > Sorry Joe, English is not my mother tongue and I misunderstand your
> > comment, I thought "Some will say this is doing too many things in a
> > single patch" did meant "You are on the limit, but it is ok to send it
> > in one patch" but I see it really means "please, rewrite this whole crap
> > into several patches" :-)
> 
> The line is a fuzzy line.  You were maybe on the line, maybe but then
> you crossed over.
Indeed I did, but this was mainly Joe's fault for giving good ideas
about how to refactor the code. Joe,next time be quiet please :-)

> 
> > 
> > So I guess an v3 patch should be a chain of several patches, I think a
> > sensible approach would be three patches
> >   1/3 => rewrite of the error check code
> >   2/3 => remove of the two useless lines at ieee80211_wep_null.
> >   3/3 => move openning braces and correct the indentation of the
> > #include
> > 
> > What do you think about it?
> 
> That sounds fine.
Ok!

> 
> regards,
> dan carpenter
regards, Pedro.