[PATCH 0/7] Staging: rtl8192e: rtllib_wx.c coding style

[PATCH 0/7] Staging: rtl8192e: rtllib_wx.c coding style

[Felix Schlepper]

This series addresses some issues raised by chechpatch.pl.
There are no logical changes, just addressing coding style.
I 'tested' it by bulding.

Felix Schlepper (7):
  Staging: rtl8192e: Added blank lines before/after struct
  Staging: rtl8192e: Added spaces around binary operators
  Staging: rtl8192e: Avoid multiple assignments
  Staging: rtl8192e: Remove unnecessary parentheses
  Staging: rtl8192e: Added braces around else
  Staging: rtl8192e: Remove unnecessary blank line
  Staging: rtl8192e: Added spaces around '+'

 drivers/staging/rtl8192e/rtllib_wx.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

-- 
2.36.1

[PATCH 1/7] Staging: rtl8192e: Added blank lines before/after struct

[Felix Schlepper]

This addresses an issue raised by checkpatch.pl:

     $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
     CHECK: Please use a blank line after function/struct/union/enum
     declarations

The additional blank line above the struct/before the headers is
just cleaner.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_wx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index cf9a240924f2..3b81a3395527 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -17,10 +17,12 @@
 #include <linux/module.h>
 #include <linux/etherdevice.h>
 #include "rtllib.h"
+
 struct modes_unit {
 	char *mode_string;
 	int mode_size;
 };
+
 static struct modes_unit rtllib_modes[] = {
 	{"a", 1},
 	{"b", 1},
-- 
2.36.1

Re: [PATCH 1/7] Staging: rtl8192e: Added blank lines before/after struct

[Joe Perches]

On Tue, 2022-06-28 at 10:30 +0200, Felix Schlepper wrote:
> This addresses an issue raised by checkpatch.pl:
> 
>      $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
>      CHECK: Please use a blank line after function/struct/union/enum
>      declarations
> 
> The additional blank line above the struct/before the headers is
> just cleaner.
[]
> diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
[]
> @@ -17,10 +17,12 @@
>  #include <linux/module.h>
>  #include <linux/etherdevice.h>
>  #include "rtllib.h"
> +
>  struct modes_unit {
>  	char *mode_string;
>  	int mode_size;
>  };
> +
>  static struct modes_unit rtllib_modes[] = {
>  	{"a", 1},
>  	{"b", 1},

Please always look deeper at the code and do not simply take
any checkpatch suggestion as the "best" fix.

Using this struct style with an embedded length is error prone
and would likely be better as a simple char * array.

Also, the existing sprintf use of mode_string and mode_size is
nonsensical.  There is nothing to format in any of the mode_size.
so the use of mode_size as an argument is silly.

Perhaps something like this would be better:
---
 drivers/staging/rtl8192e/rtllib_wx.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index cf9a240924f20..90dcce152d80c 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -17,17 +17,9 @@
 #include <linux/module.h>
 #include <linux/etherdevice.h>
 #include "rtllib.h"
-struct modes_unit {
-	char *mode_string;
-	int mode_size;
-};
-static struct modes_unit rtllib_modes[] = {
-	{"a", 1},
-	{"b", 1},
-	{"g", 1},
-	{"?", 1},
-	{"N-24G", 5},
-	{"N-5G", 4},
+
+static const char *rtllib_modes[] = {
+	"a", "b", "g", "?", "N-24G", "N-5G",
 };
 
 #define MAX_CUSTOM_LEN 64
@@ -72,11 +64,8 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
 	/* Add the protocol name */
 	iwe.cmd = SIOCGIWNAME;
 	for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
-		if (network->mode&(1<<i)) {
-			sprintf(pname, rtllib_modes[i].mode_string,
-				rtllib_modes[i].mode_size);
-			pname += rtllib_modes[i].mode_size;
-		}
+		if (network->mode & BIT(i))
+			pname = stpcpy(pname, rtllib_modes[i]);
 	}
 	*pname = '\0';
 	snprintf(iwe.u.name, IFNAMSIZ, "IEEE802.11%s", proto_name);

Re: [PATCH 1/7] Staging: rtl8192e: Added blank lines before/after struct

[Felix Schlepper]

On 28.06.2022 10:58, Joe Perches wrote:
> On Tue, 2022-06-28 at 10:30 +0200, Felix Schlepper wrote:
> > This addresses an issue raised by checkpatch.pl:
> > 
> >      $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
> >      CHECK: Please use a blank line after function/struct/union/enum
> >      declarations
> > 
> > The additional blank line above the struct/before the headers is
> > just cleaner.
> []
> > diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> []
> > @@ -17,10 +17,12 @@
> >  #include <linux/module.h>
> >  #include <linux/etherdevice.h>
> >  #include "rtllib.h"
> > +
> >  struct modes_unit {
> >  	char *mode_string;
> >  	int mode_size;
> >  };
> > +
> >  static struct modes_unit rtllib_modes[] = {
> >  	{"a", 1},
> >  	{"b", 1},
> 
> Please always look deeper at the code and do not simply take
> any checkpatch suggestion as the "best" fix.
> 
> Using this struct style with an embedded length is error prone
> and would likely be better as a simple char * array.
> 
> Also, the existing sprintf use of mode_string and mode_size is
> nonsensical.  There is nothing to format in any of the mode_size.
> so the use of mode_size as an argument is silly.
First of all, thank you for your suggestions.
I did realize that there is nothing to format, but I did not dare to
make that change in case a format could be added.
However, looking at it now. This has not been touched in over 10 years.
So it is probably safe to change that.
>
> Perhaps something like this would be better:
> ---
>  drivers/staging/rtl8192e/rtllib_wx.c | 21 +++++----------------
>  1 file changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> index cf9a240924f20..90dcce152d80c 100644
> --- a/drivers/staging/rtl8192e/rtllib_wx.c
> +++ b/drivers/staging/rtl8192e/rtllib_wx.c
> @@ -17,17 +17,9 @@
>  #include <linux/module.h>
>  #include <linux/etherdevice.h>
>  #include "rtllib.h"
> -struct modes_unit {
> -	char *mode_string;
> -	int mode_size;
> -};
> -static struct modes_unit rtllib_modes[] = {
> -	{"a", 1},
> -	{"b", 1},
> -	{"g", 1},
> -	{"?", 1},
> -	{"N-24G", 5},
> -	{"N-5G", 4},
> +
> +static const char *rtllib_modes[] = {
> +	"a", "b", "g", "?", "N-24G", "N-5G",
>  };
>  
>  #define MAX_CUSTOM_LEN 64
> @@ -72,11 +64,8 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
>  	/* Add the protocol name */
>  	iwe.cmd = SIOCGIWNAME;
>  	for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
> -		if (network->mode&(1<<i)) {
> -			sprintf(pname, rtllib_modes[i].mode_string,
> -				rtllib_modes[i].mode_size);
> -			pname += rtllib_modes[i].mode_size;
> -		}
> +		if (network->mode & BIT(i))
> +			pname = stpcpy(pname, rtllib_modes[i]);
>  	}
>  	*pname = '\0';
Would this not be also redundant since stpcpy already returns a pointer to the new
NUL-terminating character?
>  	snprintf(iwe.u.name, IFNAMSIZ, "IEEE802.11%s", proto_name);
>
Will resubmit later.

Cheers
Felix Schlepper

Re: [PATCH 1/7] Staging: rtl8192e: Added blank lines before/after struct

[Joe Perches]

On Wed, 2022-06-29 at 14:11 +0200, Felix Schlepper wrote:
> On 28.06.2022 10:58, Joe Perches wrote:
> > On Tue, 2022-06-28 at 10:30 +0200, Felix Schlepper wrote:
> > > This addresses an issue raised by checkpatch.pl:
> > > 
> > >      $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
> > >      CHECK: Please use a blank line after function/struct/union/enum
> > >      declarations
> > > 
> > > The additional blank line above the struct/before the headers is
> > > just cleaner.
> > []
> > > diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> > []
> > > @@ -17,10 +17,12 @@
> > >  #include <linux/module.h>
> > >  #include <linux/etherdevice.h>
> > >  #include "rtllib.h"
> > > +
> > >  struct modes_unit {
> > >  	char *mode_string;
> > >  	int mode_size;
> > >  };
> > > +
> > >  static struct modes_unit rtllib_modes[] = {
> > >  	{"a", 1},
> > >  	{"b", 1},
> > 
> > Please always look deeper at the code and do not simply take
> > any checkpatch suggestion as the "best" fix.
> > 
> > Using this struct style with an embedded length is error prone
> > and would likely be better as a simple char * array.
> > 
> > Also, the existing sprintf use of mode_string and mode_size is
> > nonsensical.  There is nothing to format in any of the mode_size.
> > so the use of mode_size as an argument is silly.
> First of all, thank you for your suggestions.
> I did realize that there is nothing to format, but I did not dare to
> make that change in case a format could be added.
> However, looking at it now. This has not been touched in over 10 years.
> So it is probably safe to change that.
> > 
> > Perhaps something like this would be better:
> > ---
> >  drivers/staging/rtl8192e/rtllib_wx.c | 21 +++++----------------
> >  1 file changed, 5 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> > index cf9a240924f20..90dcce152d80c 100644
> > --- a/drivers/staging/rtl8192e/rtllib_wx.c
> > +++ b/drivers/staging/rtl8192e/rtllib_wx.c
> > @@ -17,17 +17,9 @@
> >  #include <linux/module.h>
> >  #include <linux/etherdevice.h>
> >  #include "rtllib.h"
> > -struct modes_unit {
> > -	char *mode_string;
> > -	int mode_size;
> > -};
> > -static struct modes_unit rtllib_modes[] = {
> > -	{"a", 1},
> > -	{"b", 1},
> > -	{"g", 1},
> > -	{"?", 1},
> > -	{"N-24G", 5},
> > -	{"N-5G", 4},
> > +
> > +static const char *rtllib_modes[] = {
> > +	"a", "b", "g", "?", "N-24G", "N-5G",
> >  };
> >  
> >  #define MAX_CUSTOM_LEN 64
> > @@ -72,11 +64,8 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
> >  	/* Add the protocol name */
> >  	iwe.cmd = SIOCGIWNAME;
> >  	for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
> > -		if (network->mode&(1<<i)) {
> > -			sprintf(pname, rtllib_modes[i].mode_string,
> > -				rtllib_modes[i].mode_size);
> > -			pname += rtllib_modes[i].mode_size;
> > -		}
> > +		if (network->mode & BIT(i))
> > +			pname = stpcpy(pname, rtllib_modes[i]);
> >  	}
> >  	*pname = '\0';
> Would this not be also redundant since stpcpy already returns a pointer to the new
> NUL-terminating character?
> >  	snprintf(iwe.u.name, IFNAMSIZ, "IEEE802.11%s", proto_name);

Only if proto_name is initialized.  Is it?

Also I believe stpcpy is not a standard linux call, so maybe
this needs to be strcpy or strncpy then strlen.

[PATCH 2/7] Staging: rtl8192e: Added spaces around binary operators

[Felix Schlepper]

This addresses two issues raised by checkpatch.pl:

     $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
     CHECK: spaces preferred around that '&' (ctx:VxV)
     CHECK: spaces preferred around that '<<' (ctx:VxV)

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_wx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index 3b81a3395527..7bd1e829ff7e 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -74,7 +74,7 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
 	/* Add the protocol name */
 	iwe.cmd = SIOCGIWNAME;
 	for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
-		if (network->mode&(1<<i)) {
+		if (network->mode & (1 << i)) {
 			sprintf(pname, rtllib_modes[i].mode_string,
 				rtllib_modes[i].mode_size);
 			pname += rtllib_modes[i].mode_size;
-- 
2.36.1

[PATCH 3/7] Staging: rtl8192e: Avoid multiple assignments

[Felix Schlepper]

This addresses an issue raised by checkpatch.pl:

     $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
     CHECK: multiple assignments should be avoided

This patch does not change the logical of the assignments.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_wx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index 7bd1e829ff7e..c8fc66113b41 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -160,7 +160,8 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
 			max_rate = rate;
 	}
 	iwe.cmd = SIOCGIWRATE;
-	iwe.u.bitrate.fixed = iwe.u.bitrate.disabled = 0;
+	iwe.u.bitrate.disabled = 0;
+	iwe.u.bitrate.fixed = iwe.u.bitrate.disabled;
 	iwe.u.bitrate.value = max_rate * 500000;
 	start = iwe_stream_add_event_rsl(info, start, stop, &iwe, IW_EV_PARAM_LEN);
 	iwe.cmd = IWEVCUSTOM;
-- 
2.36.1

Re: [PATCH 3/7] Staging: rtl8192e: Avoid multiple assignments

[Dan Carpenter]

On Tue, Jun 28, 2022 at 10:30:52AM +0200, Felix Schlepper wrote:
> This addresses an issue raised by checkpatch.pl:
> 
>      $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
>      CHECK: multiple assignments should be avoided
> 
> This patch does not change the logical of the assignments.
> 
> Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
> ---
>  drivers/staging/rtl8192e/rtllib_wx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> index 7bd1e829ff7e..c8fc66113b41 100644
> --- a/drivers/staging/rtl8192e/rtllib_wx.c
> +++ b/drivers/staging/rtl8192e/rtllib_wx.c
> @@ -160,7 +160,8 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
>  			max_rate = rate;
>  	}
>  	iwe.cmd = SIOCGIWRATE;
> -	iwe.u.bitrate.fixed = iwe.u.bitrate.disabled = 0;
> +	iwe.u.bitrate.disabled = 0;
> +	iwe.u.bitrate.fixed = iwe.u.bitrate.disabled;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This assignment makes no sense.  Why are we setting fixed = disabled?
If you're going to split it apart do:

	iwe.u.bitrate.disabled = 0;
	iwe.u.bitrate.fixed = 0;

regards,
dan carpenter

[PATCH 4/7] Staging: rtl8192e: Remove unnecessary parentheses

[Felix Schlepper]

This addresses an issue raised by checkpatch.pl:

     $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
     Unnecessary parentheses around wrqu->encoding

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_wx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index c8fc66113b41..f1a497ff4aa6 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -288,7 +288,7 @@ int rtllib_wx_set_encode(struct rtllib_device *ieee,
 			 struct iw_request_info *info,
 			 union iwreq_data *wrqu, char *keybuf)
 {
-	struct iw_point *erq = &(wrqu->encoding);
+	struct iw_point *erq = &wrqu->encoding;
 	struct net_device *dev = ieee->dev;
 	struct rtllib_security sec = {
 		.flags = 0
@@ -460,7 +460,7 @@ int rtllib_wx_get_encode(struct rtllib_device *ieee,
 			 struct iw_request_info *info,
 			 union iwreq_data *wrqu, char *keybuf)
 {
-	struct iw_point *erq = &(wrqu->encoding);
+	struct iw_point *erq = &wrqu->encoding;
 	int len, key;
 	struct lib80211_crypt_data *crypt;
 
-- 
2.36.1

[PATCH 5/7] Staging: rtl8192e: Added braces around else

[Felix Schlepper]

This addresses two issues raised by checkpatch.pl:

     $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
     CHECK: braces {} should be used on all arms of this statement
     CHECK: Unbalanced braces around else statement

The coding style rule with not using unnecessary braces around if/else
does not apply if only one branch is a single statement.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_wx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index f1a497ff4aa6..976d81f0d7a6 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -315,8 +315,9 @@ int rtllib_wx_set_encode(struct rtllib_device *ieee,
 			netdev_dbg(ieee->dev,
 				   "Disabling encryption on key %d.\n", key);
 			lib80211_crypt_delayed_deinit(&ieee->crypt_info, crypt);
-		} else
+		} else {
 			netdev_dbg(ieee->dev, "Disabling encryption.\n");
+		}
 
 		/* Check all the keys to see if any are still configured,
 		 * and if no key index was provided, de-init them all
@@ -735,8 +736,9 @@ int rtllib_wx_set_auth(struct rtllib_device *ieee,
 		} else if (data->value & IW_AUTH_ALG_LEAP) {
 			ieee->open_wep = 1;
 			ieee->auth_mode = 2;
-		} else
+		} else {
 			return -EINVAL;
+		}
 		break;
 
 	case IW_AUTH_WPA_ENABLED:
-- 
2.36.1

[PATCH 6/7] Staging: rtl8192e: Remove unnecessary blank line

[Felix Schlepper]

This addresses an issue raised by checkpatch.pl:

     $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
     CHECK: Blank lines aren't necessary before a close brace '}'

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_wx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index 976d81f0d7a6..9945f3f136d1 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -612,7 +612,6 @@ int rtllib_wx_set_encode_ext(struct rtllib_device *ieee,
 			goto done;
 		}
 		*crypt = new_crypt;
-
 	}
 
 	if (ext->key_len > 0 && (*crypt)->ops->set_key &&
-- 
2.36.1

[PATCH 7/7] Staging: rtl8192e: Added spaces around '+'

[Felix Schlepper]

This addresses two issues raised by checkpatch.pl:

     $ ./scripts/checkpatch.pl --terse -f drivers/staging/rtl8192e/rtllib_wx.c
     CHECK: spaces preferred around that '+' (ctx:VxV)

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
 drivers/staging/rtl8192e/rtllib_wx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index 9945f3f136d1..52e5e4e5977b 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -780,7 +780,7 @@ int rtllib_wx_set_gen_ie(struct rtllib_device *ieee, u8 *ie, size_t len)
 	kfree(ieee->wps_ie);
 	ieee->wps_ie = NULL;
 	if (len) {
-		if (len != ie[1]+2)
+		if (len != ie[1] + 2)
 			return -EINVAL;
 		buf = kmemdup(ie, len, GFP_KERNEL);
 		if (!buf)
-- 
2.36.1