[PATCHv2 0/3] staging: silicom: fix code styling according to checkpatch

[PATCHv2 0/3] staging: silicom: fix code styling according to checkpatch

[Michael Hoefler]

This patch series fixes most issues discovered by checkpatch. That includes
whitespaces, indentation, the wrong use of parenthesis and some more. Please
refer to the specific patch description to get more details.

Besides some minor issues not discovered by checkpatch are fixed. E.g. i moved some empty lines for a better readability and added missing braces.

This series does not perform functional changes to the code because i do not
have any hardware to test it.


Michael Hoefler (3):
  silicom: fix whitespace issues in bypass.c
  silicom: fix coding style issues in bypass.c
  silicom: remaining checkpatch issues in bypass.c

 drivers/staging/silicom/bypasslib/bypass.c | 158 ++++++++++++++++-------------
 1 file changed, 85 insertions(+), 73 deletions(-)

-- 
1.8.1.2

[PATCHv2 1/3] silicom: fix whitespace issues in bypass.c

[Michael Hoefler]

This patch addresses several problems in bypass.c found by checkpatch.
Furthermore it removes/adds some empty lines to make the code more readable.

The following problems are fixed:
 - line over 80 characters
 - space after logical operator !
 - spaces instead of tabs
 - missing empty line after local declarations
 - empty line after {

The empty line issues were not discovered by checkpatch but in my opinion these
changes make perfect sense in terms of readability.

Signed-off-by: Michael Hoefler <michael.hoefler@studium.uni-erlangen.de>
Signed-off-by: Christoph Kohl <christoph.kohl@t-online.de>
---
 drivers/staging/silicom/bypasslib/bypass.c | 111 ++++++++++++++++-------------
 1 file changed, 61 insertions(+), 50 deletions(-)

diff --git a/drivers/staging/silicom/bypasslib/bypass.c b/drivers/staging/silicom/bypasslib/bypass.c
index dda1b2a..14721a9 100644
--- a/drivers/staging/silicom/bypasslib/bypass.c
+++ b/drivers/staging/silicom/bypasslib/bypass.c
@@ -7,11 +7,11 @@
 /* the Free Software Foundation, located in the file LICENSE.                 */
 /*                                                                            */
 /*                                                                            */
-/* bypass.c                                                                    */
+/* bypass.c                                                                   */
 /*                                                                            */
 /******************************************************************************/
 
-#if defined(CONFIG_SMP) && ! defined(__SMP__)
+#if defined(CONFIG_SMP) && !defined(__SMP__)
 #define __SMP__
 #endif
 
@@ -66,13 +66,12 @@ static int doit(int cmd, int if_index, int *data)
 	int ret = -1;
 	struct net_device *dev;
 	struct net_device *n;
-	for_each_netdev_safe(&init_net, dev, n) {
 
+	for_each_netdev_safe(&init_net, dev, n) {
 		if (dev->ifindex == if_index) {
 			ret = do_cmd(dev, &ifr, cmd, data);
 			if (ret < 0)
 				ret = -1;
-
 		}
 	}
 
@@ -82,50 +81,59 @@ static int doit(int cmd, int if_index, int *data)
 #define bp_symbol_get(fn_name) symbol_get(fn_name)
 #define bp_symbol_put(fn_name) symbol_put(fn_name)
 
-#define SET_BPLIB_INT_FN(fn_name, arg_type, arg, ret) \
-    ({ int (* fn_ex)(arg_type)=NULL; \
-    fn_ex=bp_symbol_get(fn_name##_sd); \
-       if(fn_ex) {  \
-        ret= fn_ex(arg); \
-       bp_symbol_put(fn_name##_sd); \
-       } else ret=-1; \
-    })
-
-#define  SET_BPLIB_INT_FN2(fn_name, arg_type, arg, arg_type1, arg1, ret) \
-    ({ int (* fn_ex)(arg_type,arg_type1)=NULL; \
-        fn_ex=bp_symbol_get(fn_name##_sd); \
-       if(fn_ex) {  \
-        ret= fn_ex(arg,arg1); \
-        bp_symbol_put(fn_name##_sd); \
-       } else ret=-1; \
-    })
-#define SET_BPLIB_INT_FN3(fn_name, arg_type, arg, arg_type1, arg1,arg_type2, arg2, ret) \
-    ({ int (* fn_ex)(arg_type,arg_type1, arg_type2)=NULL; \
-        fn_ex=bp_symbol_get(fn_name##_sd); \
-       if(fn_ex) {  \
-        ret= fn_ex(arg,arg1,arg2); \
-        bp_symbol_put(fn_name##_sd); \
-       } else ret=-1; \
-    })
-
-#define DO_BPLIB_GET_ARG_FN(fn_name,ioctl_val, if_index) \
-    ({    int data, ret=0; \
-            if(is_dev_sd(if_index)){ \
-            SET_BPLIB_INT_FN(fn_name, int, if_index, ret); \
-            return ret; \
-            }  \
-            return doit(ioctl_val,if_index, &data); \
-    })
-
-#define DO_BPLIB_SET_ARG_FN(fn_name,ioctl_val,if_index,arg) \
-    ({    int data, ret=0; \
-            if(is_dev_sd(if_index)){ \
-            SET_BPLIB_INT_FN2(fn_name, int, if_index, int, arg, ret); \
-            return ret; \
-            }  \
-	    data=arg; \
-            return doit(ioctl_val,if_index, &data); \
-    })
+#define SET_BPLIB_INT_FN(fn_name, arg_type, arg, ret)	\
+({	int (*fn_ex)(arg_type) = NULL;			\
+	fn_ex = bp_symbol_get(fn_name##_sd);		\
+	if (fn_ex) {					\
+		ret = fn_ex(arg);			\
+		bp_symbol_put(fn_name##_sd);		\
+	} else {					\
+		ret = -1;				\
+	}						\
+})
+
+#define  SET_BPLIB_INT_FN2(fn_name, arg_type, arg, arg_type1, arg1, ret)\
+({	int (*fn_ex)(arg_type, arg_type1) = NULL;			\
+	fn_ex = bp_symbol_get(fn_name##_sd);				\
+	if (fn_ex) {							\
+		ret = fn_ex(arg, arg1);					\
+		bp_symbol_put(fn_name##_sd);				\
+	} else {							\
+		ret = -1;						\
+	}								\
+})
+
+#define SET_BPLIB_INT_FN3(fn_name, arg_type, arg, arg_type1, arg1,	\
+			  arg_type2, arg2, ret)				\
+({	int (*fn_ex)(arg_type, arg_type1, arg_type2) = NULL;		\
+	fn_ex = bp_symbol_get(fn_name##_sd);				\
+	if (fn_ex) {							\
+		ret = fn_ex(arg, arg1, arg2);				\
+		bp_symbol_put(fn_name##_sd);				\
+	} else {							\
+		ret = -1;						\
+	}								\
+})
+
+#define DO_BPLIB_GET_ARG_FN(fn_name, ioctl_val, if_index)	\
+({	int data, ret = 0;					\
+	if (is_dev_sd(if_index)) {				\
+		SET_BPLIB_INT_FN(fn_name, int, if_index, ret);	\
+		return ret;					\
+	}							\
+	return doit(ioctl_val, if_index, &data);		\
+})
+
+#define DO_BPLIB_SET_ARG_FN(fn_name, ioctl_val, if_index, arg)	\
+({	int data, ret = 0;					\
+	if (is_dev_sd(if_index)) {				\
+		SET_BPLIB_INT_FN2(fn_name, int, if_index, int,	\
+				  arg, ret);			\
+		return ret;					\
+	}							\
+	data = arg;						\
+	return doit(ioctl_val, if_index, &data);		\
+})
 
 static int is_dev_sd(int if_index)
 {
@@ -268,6 +276,7 @@ EXPORT_SYMBOL(get_bypass_pwup);
 static int set_bypass_wd(int if_index, int ms_timeout, int *ms_timeout_set)
 {
 	int data = ms_timeout, ret = 0;
+
 	if (is_dev_sd(if_index))
 		SET_BPLIB_INT_FN3(set_bypass_wd, int, if_index, int, ms_timeout,
 				  int *, ms_timeout_set, ret);
@@ -285,6 +294,7 @@ EXPORT_SYMBOL(set_bypass_wd);
 static int get_bypass_wd(int if_index, int *ms_timeout_set)
 {
 	int *data = ms_timeout_set, ret = 0;
+
 	if (is_dev_sd(if_index))
 		SET_BPLIB_INT_FN2(get_bypass_wd, int, if_index, int *,
 				  ms_timeout_set, ret);
@@ -297,6 +307,7 @@ EXPORT_SYMBOL(get_bypass_wd);
 static int get_wd_expire_time(int if_index, int *ms_time_left)
 {
 	int *data = ms_time_left, ret = 0;
+
 	if (is_dev_sd(if_index))
 		SET_BPLIB_INT_FN2(get_wd_expire_time, int, if_index, int *,
 				  ms_time_left, ret);
@@ -476,13 +487,14 @@ EXPORT_SYMBOL(get_bp_hw_reset);
 static int get_bypass_info(int if_index, struct bp_info *bp_info)
 {
 	int ret = 0;
+
 	if (is_dev_sd(if_index)) {
 		SET_BPLIB_INT_FN2(get_bypass_info, int, if_index,
 				  struct bp_info *, bp_info, ret);
 	} else {
 		struct net_device *dev;
-
 		struct net_device *n;
+
 		for_each_netdev_safe(&init_net, dev, n) {
 			if (dev->ifindex == if_index) {
 				struct if_bypass_info *bypass_cb;
@@ -514,7 +526,6 @@ EXPORT_SYMBOL(get_bypass_info);
 
 int init_lib_module(void)
 {
-
 	printk(VERSION);
 	return 0;
 }
-- 
1.8.1.2

[PATCHv2 2/3] silicom: fix coding style issues in bypass.c

[Michael Hoefler]

This patch improves bypass.c (in staging) in terms of coding style. This
includes different issues some of them mentioned by checkpatch:

 - a c++ one line comment
 - parenthesis at return statementes
 - multiple definitions in one line
 - missing braces according to the style guide

Signed-off-by: Michael Hoefler <michael.hoefler@studium.uni-erlangen.de>
Signed-off-by: Christoph Kohl <christoph.kohl@t-online.de>
---
 drivers/staging/silicom/bypasslib/bypass.c | 32 ++++++++++++++++--------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/silicom/bypasslib/bypass.c b/drivers/staging/silicom/bypasslib/bypass.c
index 14721a9..ff0c517 100644
--- a/drivers/staging/silicom/bypasslib/bypass.c
+++ b/drivers/staging/silicom/bypasslib/bypass.c
@@ -22,7 +22,7 @@
 #include <linux/sched.h>
 #include <linux/wait.h>
 
-#include <linux/netdevice.h>	// struct device, and other headers
+#include <linux/netdevice.h>	/* struct device, and other headers */
 #include <linux/kernel_stat.h>
 #include <linux/pci.h>
 #include <linux/rtnetlink.h>
@@ -139,7 +139,7 @@ static int is_dev_sd(int if_index)
 {
 	int ret = 0;
 	SET_BPLIB_INT_FN(is_bypass, int, if_index, ret);
-	return (ret >= 0 ? 1 : 0);
+	return ret >= 0 ? 1 : 0;
 }
 
 static int is_bypass_dev(int if_index)
@@ -147,7 +147,8 @@ static int is_bypass_dev(int if_index)
 	struct pci_dev *pdev = NULL;
 	struct net_device *dev = NULL;
 	struct ifreq ifr;
-	int ret = 0, data = 0;
+	int ret = 0;
+	int data = 0;
 
 	while ((pdev = pci_get_class(PCI_CLASS_NETWORK_ETHERNET << 8, pdev))) {
 		if ((dev = pci_get_drvdata(pdev)) != NULL)
@@ -155,8 +156,9 @@ static int is_bypass_dev(int if_index)
 			    (dev->ifindex == if_index)) {
 				if ((pdev->vendor == SILICOM_VID) &&
 				    (pdev->device >= SILICOM_BP_PID_MIN) &&
-				    (pdev->device <= SILICOM_BP_PID_MAX))
+				    (pdev->device <= SILICOM_BP_PID_MAX)) {
 					goto send_cmd;
+				}
 #if defined(BP_VENDOR_SUPPORT) && defined(ETHTOOL_GDRVINFO)
 				else {
 					struct ethtool_drvinfo info;
@@ -184,7 +186,7 @@ static int is_bypass_dev(int if_index)
 	}
  send_cmd:
 	ret = do_cmd(dev, &ifr, IS_BYPASS, &data);
-	return (ret < 0 ? -1 : ret);
+	return ret < 0 ? -1 : ret;
 }
 
 static int is_bypass(int if_index)
@@ -275,12 +277,13 @@ EXPORT_SYMBOL(get_bypass_pwup);
 
 static int set_bypass_wd(int if_index, int ms_timeout, int *ms_timeout_set)
 {
-	int data = ms_timeout, ret = 0;
+	int data = ms_timeout;
+	int ret = 0;
 
-	if (is_dev_sd(if_index))
+	if (is_dev_sd(if_index)) {
 		SET_BPLIB_INT_FN3(set_bypass_wd, int, if_index, int, ms_timeout,
 				  int *, ms_timeout_set, ret);
-	else {
+	} else {
 		ret = doit(SET_BYPASS_WD, if_index, &data);
 		if (ret > 0) {
 			*ms_timeout_set = ret;
@@ -293,7 +296,8 @@ EXPORT_SYMBOL(set_bypass_wd);
 
 static int get_bypass_wd(int if_index, int *ms_timeout_set)
 {
-	int *data = ms_timeout_set, ret = 0;
+	int *data = ms_timeout_set;
+	int ret = 0;
 
 	if (is_dev_sd(if_index))
 		SET_BPLIB_INT_FN2(get_bypass_wd, int, if_index, int *,
@@ -308,10 +312,10 @@ static int get_wd_expire_time(int if_index, int *ms_time_left)
 {
 	int *data = ms_time_left, ret = 0;
 
-	if (is_dev_sd(if_index))
+	if (is_dev_sd(if_index)) {
 		SET_BPLIB_INT_FN2(get_wd_expire_time, int, if_index, int *,
 				  ms_time_left, ret);
-	else {
+	} else {
 		ret = doit(GET_WD_EXPIRE_TIME, if_index, data);
 		if ((ret == 0) && (*data != 0))
 			ret = 1;
@@ -505,17 +509,15 @@ static int get_bypass_info(int if_index, struct bp_info *bp_info)
 				bypass_cb->cmd = GET_BYPASS_INFO;
 
 				if (dev->netdev_ops &&
-					dev->netdev_ops->ndo_do_ioctl) {
+					dev->netdev_ops->ndo_do_ioctl)
 					ret = dev->netdev_ops->ndo_do_ioctl(dev,
 						&ifr, SIOCGIFBYPASS);
-				}
-
 				else
 					ret = -1;
 				if (ret == 0)
 					memcpy(bp_info, &bypass_cb->bp_info,
 					       sizeof(struct bp_info));
-				ret = (ret < 0 ? -1 : 0);
+				ret = ret < 0 ? -1 : 0;
 				break;
 			}
 		}
-- 
1.8.1.2

[PATCHv2 3/3] silicom: remaining checkpatch issues in bypass.c

[Michael Hoefler]

In this patch we fix some "logical" errors in bypass.c of the silicom bypass
driver (in staging).

Checkpatch complains about the following errors:
 - unnecessary forward declarations in a source file
 - assignment in if condition

In addition to that the __init and __exit macros were missing at the init and
cleanup function.

There are still two warnings left for this file related to too many leadingtabs
at nested blocks. I did not touch this issue becasue the code needs really some
refactoring. And since i do not have the appropriate hardware to test the code,
i do not change the functionality in any way.

Signed-off-by: Michael Hoefler <michael.hoefler@studium.uni-erlangen.de>
Signed-off-by: Christoph Kohl <christoph.kohl@t-online.de>
---
 drivers/staging/silicom/bypasslib/bypass.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/silicom/bypasslib/bypass.c b/drivers/staging/silicom/bypasslib/bypass.c
index ff0c517..9b771c9 100644
--- a/drivers/staging/silicom/bypasslib/bypass.c
+++ b/drivers/staging/silicom/bypasslib/bypass.c
@@ -40,9 +40,6 @@ MODULE_AUTHOR("www.silicom.co.il");
 
 MODULE_LICENSE("GPL");
 
-int init_lib_module(void);
-void cleanup_lib_module(void);
-
 static int do_cmd(struct net_device *dev, struct ifreq *ifr, int cmd, int *data)
 {
 	int ret = -1;
@@ -151,9 +148,10 @@ static int is_bypass_dev(int if_index)
 	int data = 0;
 
 	while ((pdev = pci_get_class(PCI_CLASS_NETWORK_ETHERNET << 8, pdev))) {
-		if ((dev = pci_get_drvdata(pdev)) != NULL)
-			if (((dev = pci_get_drvdata(pdev)) != NULL) &&
-			    (dev->ifindex == if_index)) {
+		dev = pci_get_drvdata(pdev);
+		if (dev != NULL) {
+			dev = pci_get_drvdata(pdev);
+			if ((dev != NULL) && (dev->ifindex == if_index)) {
 				if ((pdev->vendor == SILICOM_VID) &&
 				    (pdev->device >= SILICOM_BP_PID_MIN) &&
 				    (pdev->device <= SILICOM_BP_PID_MAX)) {
@@ -183,6 +181,7 @@ static int is_bypass_dev(int if_index)
 #endif
 				return -1;
 			}
+		}
 	}
  send_cmd:
 	ret = do_cmd(dev, &ifr, IS_BYPASS, &data);
@@ -526,13 +525,13 @@ static int get_bypass_info(int if_index, struct bp_info *bp_info)
 }
 EXPORT_SYMBOL(get_bypass_info);
 
-int init_lib_module(void)
+int __init init_lib_module(void)
 {
 	printk(VERSION);
 	return 0;
 }
 
-void cleanup_lib_module(void)
+void __exit cleanup_lib_module(void)
 {
 }
 
-- 
1.8.1.2

Re: [PATCHv2 1/3] silicom: fix whitespace issues in bypass.c

[Gokulnath A]

Hello Michael,

On Fri, Dec 20, 2013 at 7:21 PM, Michael Hoefler
<michael.hoefler@studium.uni-erlangen.de> wrote:
> This patch addresses several problems in bypass.c found by checkpatch.
> Furthermore it removes/adds some empty lines to make the code more readable.
>
> The following problems are fixed:
>  - line over 80 characters
>  - space after logical operator !
>  - spaces instead of tabs
>  - missing empty line after local declarations
>  - empty line after {
>

Please do one thing per patch, As Greg suggested for me in previous mail
conversations. Here you could have at least 3 or 4 patches.
For example, first patch would be a fix for line over 80 characters,
followed by the other problems as mentioned above by you.

> The empty line issues were not discovered by checkpatch but in my opinion these
> changes make perfect sense in terms of readability.
>
> Signed-off-by: Michael Hoefler <michael.hoefler@studium.uni-erlangen.de>
> Signed-off-by: Christoph Kohl <christoph.kohl@t-online.de>
> ---
>  drivers/staging/silicom/bypasslib/bypass.c | 111 ++++++++++++++++-------------
>  1 file changed, 61 insertions(+), 50 deletions(-)
>
 1.8.1.2
>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCHv2 1/3] silicom: fix whitespace issues in bypass.c

[Greg Kroah-Hartman]

On Fri, Dec 20, 2013 at 08:36:48PM +0530, Gokulnath A wrote:
> Hello Michael,
> 
> On Fri, Dec 20, 2013 at 7:21 PM, Michael Hoefler
> <michael.hoefler@studium.uni-erlangen.de> wrote:
> > This patch addresses several problems in bypass.c found by checkpatch.
> > Furthermore it removes/adds some empty lines to make the code more readable.
> >
> > The following problems are fixed:
> >  - line over 80 characters
> >  - space after logical operator !
> >  - spaces instead of tabs
> >  - missing empty line after local declarations
> >  - empty line after {
> >
> 
> Please do one thing per patch, As Greg suggested for me in previous mail
> conversations. Here you could have at least 3 or 4 patches.
> For example, first patch would be a fix for line over 80 characters,
> followed by the other problems as mentioned above by you.

Nah, all of these are affecting the same macros, so it's ok, I'll take
it as-is.

thanks,

greg k-h

Re: [PATCHv2 1/3] silicom: fix whitespace issues in bypass.c

[Gokulnath A]

On Fri, Dec 20, 2013 at 10:31 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Dec 20, 2013 at 08:36:48PM +0530, Gokulnath A wrote:
>> Hello Michael,
>>
>> On Fri, Dec 20, 2013 at 7:21 PM, Michael Hoefler
>> <michael.hoefler@studium.uni-erlangen.de> wrote:
>> > This patch addresses several problems in bypass.c found by checkpatch.
>> > Furthermore it removes/adds some empty lines to make the code more readable.
>> >
>> > The following problems are fixed:
>> >  - line over 80 characters
>> >  - space after logical operator !
>> >  - spaces instead of tabs
>> >  - missing empty line after local declarations
>> >  - empty line after {
>> >
>>
>> Please do one thing per patch, As Greg suggested for me in previous mail
>> conversations. Here you could have at least 3 or 4 patches.
>> For example, first patch would be a fix for line over 80 characters,
>> followed by the other problems as mentioned above by you.
>
> Nah, all of these are affecting the same macros, so it's ok, I'll take
> it as-is.

ok. Sorry for the comments and thanks for information.

>
> thanks,
>
> greg k-h