[PATCH 0/3] staging: wilc1000: code style patches

[PATCH 0/3] staging: wilc1000: code style patches

[Raphaël Beamonte]

Hello,

Please find in following emails code style patches for the driver
wilc1000 in staging.

Raphaël


Raphaël Beamonte (3):
  staging: wilc1000: code style: fix macro with multiple statements
  staging: wilc1000: code style: fix globals initialized to false
  staging: wilc1000: code style: fix open brace { on wrong line

 drivers/staging/wilc1000/host_interface.c         |  4 ++--
 drivers/staging/wilc1000/linux_wlan.c             |  3 +--
 drivers/staging/wilc1000/wilc_exported_buf.c      | 14 ++++++++------
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c |  3 +--
 drivers/staging/wilc1000/wilc_wlan.c              |  2 +-
 5 files changed, 13 insertions(+), 13 deletions(-)

-- 
2.1.4

[PATCH 1/3] staging: wilc1000: code style: fix macro with multiple statements

[Raphaël Beamonte]

Macros with multiple statements should be enclosed in a do - while loop

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index 5294578..45c2c7e 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -12,11 +12,13 @@
 	void *exported_ ## name = NULL;
 
 #define MALLOC_WILC_BUFFER(name, size)	\
-	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
-	if (!exported_ ## name) {   \
-		printk("fail to alloc: %s memory\n", exported_ ## name);  \
-		return -ENOBUFS;	\
-	}
+	do { \
+		exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
+		if (!exported_ ## name) {   \
+			printk("fail to alloc: %s memory\n", exported_ ## name);  \
+			return -ENOBUFS;	\
+		}
+	} while (0)
 
 #define FREE_WILC_BUFFER(name)	\
 	kfree(exported_ ## name);
@@ -73,4 +75,4 @@ MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Tony Cho");
 MODULE_DESCRIPTION("WILC1xxx Memory Manager");
 pure_initcall(wilc_module_init);
-module_exit(wilc_module_deinit);
\ No newline at end of file
+module_exit(wilc_module_deinit);
-- 
2.1.4

[PATCH 2/3] staging: wilc1000: code style: fix globals initialized to false

[Raphaël Beamonte]

Globals should not be initialized to 0 or NULL.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/host_interface.c | 4 ++--
 drivers/staging/wilc1000/wilc_wlan.c      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index bab5319..53c4ca9 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -536,7 +536,7 @@ typedef enum {
 tstrWILC_WFIDrv *terminated_handle;
 tstrWILC_WFIDrv *gWFiDrvHandle;
 #ifdef DISABLE_PWRSAVE_AND_SCAN_DURING_IP
-bool g_obtainingIP = false;
+bool g_obtainingIP;
 #endif
 u8 P2P_LISTEN_STATE;
 static struct task_struct *HostIFthreadHandler;
@@ -558,7 +558,7 @@ static u8 gapu8RcvdSurveyResults[2][MAX_SURVEY_RESULT_FRAG_SIZE];
 
 static u8 gapu8RcvdAssocResp[MAX_ASSOC_RESP_FRAME_SIZE];
 
-bool gbScanWhileConnected = false;
+bool gbScanWhileConnected;
 
 static s8 gs8Rssi;
 static s8 gs8lnkspd;
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index fac16db..4c40955 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -482,7 +482,7 @@ static int wilc_wlan_txq_filter_dup_tcp_ack(void)
 #endif
 
 #ifdef TCP_ENHANCEMENTS
-bool EnableTCPAckFilter = false;
+bool EnableTCPAckFilter;
 
 void Enable_TCP_ACK_Filter(bool value)
 {
-- 
2.1.4

[PATCH 3/3] staging: wilc1000: code style: fix open brace { on wrong line

[Raphaël Beamonte]

Open braces should be on the same line as if and for statements.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/linux_wlan.c             | 3 +--
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 7eacc2f..040e55d 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -2103,8 +2103,7 @@ static void wilc_set_multicast_list(struct net_device *dev)
 	}
 
 	/* Store all of the multicast addresses in the hardware filter */
-	netdev_for_each_mc_addr(ha, dev)
-	{
+	netdev_for_each_mc_addr(ha, dev) {
 		memcpy(gau8MulticastMacAddrList[i], ha->addr, ETH_ALEN);
 		PRINT_D(INIT_DBG, "Entry[%d]: %x:%x:%x:%x:%x:%x\n", i,
 			gau8MulticastMacAddrList[i][0], gau8MulticastMacAddrList[i][1], gau8MulticastMacAddrList[i][2], gau8MulticastMacAddrList[i][3], gau8MulticastMacAddrList[i][4], gau8MulticastMacAddrList[i][5]);
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index c2f8d60..29f43d6 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -1527,8 +1527,7 @@ static int WILC_WFI_get_key(struct wiphy *wiphy, struct net_device *netdev, u8 k
 	priv = wiphy_priv(wiphy);
 
 
-	if (!pairwise)
-	{
+	if (!pairwise) {
 		PRINT_D(CFG80211_DBG, "Getting group key idx: %x\n", key_index);
 
 		key_params.key = priv->wilc_gtk[key_index]->key;
-- 
2.1.4

Re: [PATCH 1/3] staging: wilc1000: code style: fix macro with multiple statements

[Dan Carpenter]

On Sun, Aug 16, 2015 at 01:30:12AM -0400, Raphaël Beamonte wrote:
>  #define MALLOC_WILC_BUFFER(name, size)	\
> -	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
> -	if (!exported_ ## name) {   \
> -		printk("fail to alloc: %s memory\n", exported_ ## name);  \
> -		return -ENOBUFS;	\
> -	}
> +	do { \
> +		exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
> +		if (!exported_ ## name) {   \
> +			printk("fail to alloc: %s memory\n", exported_ ## name);  \
> +			return -ENOBUFS;	\
> +		}
> +	} while (0)

Pull it in one indent level...  But actually this macro has a return in
the middle of it, so it just introduces bugs all over the place like
eating cookies in bed.  We should just delete it instead.

regards,
dan carpenter

Re: [PATCH 1/3] staging: wilc1000: code style: fix macro with multiple statements

[Raphaël Beamonte]

2015-08-17 5:08 GMT-04:00 Dan Carpenter <dan.carpenter@oracle.com>:
> Pull it in one indent level...  But actually this macro has a return in
> the middle of it, so it just introduces bugs all over the place like
> eating cookies in bed.  We should just delete it instead.

You're right!
I'll clean those macro up and send a new set of patches to replace this one.

Thanks for the feedback!
Raphaël

[PATCH 0/5] staging: wilc1000: code improvements

[Raphaël Beamonte]

Hi,

The first 3 patches of the following 5 are aimed to simplify the
wilc_exported_buf.c macros as well as correct a potential memory
leak from the use of the MALLOC_WILC_BUFFER one.

The next 2 patches are correcting two kind of checkpatch warning
reports in different files of the wilc1000 driver.

Raphaël


Raphaël Beamonte (5):
  staging: wilc1000: remove DECLARE_WILC_BUFFER()
  staging: wilc1000: remove FREE_WILC_BUFFER()
  staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid
    possible memory leak
  staging: wilc1000: use pr_* instead of printk
  staging: wilc1000: remove void function return statements that are not
    useful

 drivers/staging/wilc1000/coreconfigurator.c  |  4 +-
 drivers/staging/wilc1000/host_interface.c    |  4 --
 drivers/staging/wilc1000/linux_wlan.c        |  9 ++--
 drivers/staging/wilc1000/linux_wlan_common.h | 32 ++++++-------
 drivers/staging/wilc1000/linux_wlan_sdio.c   |  2 +-
 drivers/staging/wilc1000/linux_wlan_spi.c    |  2 +-
 drivers/staging/wilc1000/wilc_debugfs.c      | 16 +++----
 drivers/staging/wilc1000/wilc_exported_buf.c | 69 +++++++++++++++++-----------
 drivers/staging/wilc1000/wilc_wlan.c         |  3 --
 drivers/staging/wilc1000/wilc_wlan_cfg.c     |  2 -
 10 files changed, 75 insertions(+), 68 deletions(-)

-- 
2.1.4

[PATCH 1/5] staging: wilc1000: remove DECLARE_WILC_BUFFER()

[Raphaël Beamonte]

It was just a wrapper to initialize a variable. Initialize it
directly instead.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index 5294578..985b0e1 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -8,9 +8,6 @@
 #define LINUX_TX_SIZE	(64 * 1024)
 #define WILC1000_FW_SIZE (4 * 1024)
 
-#define DECLARE_WILC_BUFFER(name)	\
-	void *exported_ ## name = NULL;
-
 #define MALLOC_WILC_BUFFER(name, size)	\
 	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
 	if (!exported_ ## name) {   \
@@ -24,9 +21,9 @@
 /*
  * Add necessary buffer pointers
  */
-DECLARE_WILC_BUFFER(g_tx_buf)
-DECLARE_WILC_BUFFER(g_rx_buf)
-DECLARE_WILC_BUFFER(g_fw_buf)
+void *exported_g_tx_buf;
+void *exported_g_rx_buf;
+void *exported_g_fw_buf;
 
 void *get_tx_buffer(void)
 {
@@ -73,4 +70,4 @@ MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Tony Cho");
 MODULE_DESCRIPTION("WILC1xxx Memory Manager");
 pure_initcall(wilc_module_init);
-module_exit(wilc_module_deinit);
\ No newline at end of file
+module_exit(wilc_module_deinit);
-- 
2.1.4

[PATCH 2/5] staging: wilc1000: remove FREE_WILC_BUFFER()

[Raphaël Beamonte]

It was just a wrapper around kfree(), so call that instead.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index 985b0e1..bf392fb 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -15,9 +15,6 @@
 		return -ENOBUFS;	\
 	}
 
-#define FREE_WILC_BUFFER(name)	\
-	kfree(exported_ ## name);
-
 /*
  * Add necessary buffer pointers
  */
@@ -59,9 +56,15 @@ static int __init wilc_module_init(void)
 static void __exit wilc_module_deinit(void)
 {
 	printk("wilc_module_deinit\n");
-	FREE_WILC_BUFFER(g_tx_buf)
-	FREE_WILC_BUFFER(g_rx_buf)
-	FREE_WILC_BUFFER(g_fw_buf)
+
+	kfree(exported_g_tx_buf);
+	exported_g_tx_buf = NULL;
+
+	kfree(exported_g_rx_buf);
+	exported_g_rx_buf = NULL;
+
+	kfree(exported_g_fw_buf);
+	exported_g_fw_buf = NULL;
 
 	return;
 }
-- 
2.1.4

[PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Raphaël Beamonte]

The MACRO_WILC_BUFFER() macro was using a return statement, and didn't
take care of possible memory leaks and subsequent bugs when it was failing
after succeeding some allocations. This patch corrects this behavior.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 39 +++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index bf392fb..c3aff9a 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -8,13 +8,6 @@
 #define LINUX_TX_SIZE	(64 * 1024)
 #define WILC1000_FW_SIZE (4 * 1024)
 
-#define MALLOC_WILC_BUFFER(name, size)	\
-	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
-	if (!exported_ ## name) {   \
-		printk("fail to alloc: %s memory\n", exported_ ## name);  \
-		return -ENOBUFS;	\
-	}
-
 /*
  * Add necessary buffer pointers
  */
@@ -40,17 +33,43 @@ void *get_fw_buffer(void)
 }
 EXPORT_SYMBOL(get_fw_buffer);
 
+static inline int kmalloc_wilc_buffer(void *buf, int size)
+{
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf) {
+		printk("fail to alloc memory\n");
+		return -ENOBUFS;
+	}
+	return 0;
+}
+
 static int __init wilc_module_init(void)
 {
 	printk("wilc_module_init\n");
 	/*
 	 * alloc necessary memory
 	 */
-	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
-	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
-	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
+	if (kmalloc_wilc_buffer(exported_g_tx_buf, LINUX_TX_SIZE))
+		goto error_g_tx_buf;
+
+	if (kmalloc_wilc_buffer(exported_g_rx_buf, LINUX_RX_SIZE))
+		goto error_g_rx_buf;
+
+	if (kmalloc_wilc_buffer(exported_g_fw_buf, WILC1000_FW_SIZE))
+		goto error_g_fw_buf;
 
 	return 0;
+
+error_g_fw_buf:
+	kfree(exported_g_rx_buf);
+	exported_g_rx_buf = NULL;
+
+error_g_rx_buf:
+	kfree(exported_g_tx_buf);
+	exported_g_tx_buf = NULL;
+
+error_g_tx_buf:
+	return -ENOBUFS;
 }
 
 static void __exit wilc_module_deinit(void)
-- 
2.1.4

[PATCH 4/5] staging: wilc1000: use pr_* instead of printk

[Raphaël Beamonte]

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/coreconfigurator.c  |  4 ++--
 drivers/staging/wilc1000/linux_wlan.c        |  8 +++----
 drivers/staging/wilc1000/linux_wlan_common.h | 32 ++++++++++++++--------------
 drivers/staging/wilc1000/linux_wlan_sdio.c   |  2 +-
 drivers/staging/wilc1000/linux_wlan_spi.c    |  2 +-
 drivers/staging/wilc1000/wilc_debugfs.c      | 16 +++++++-------
 drivers/staging/wilc1000/wilc_exported_buf.c |  6 +++---
 7 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
index c81c41d..0cbf4a9c 100644
--- a/drivers/staging/wilc1000/coreconfigurator.c
+++ b/drivers/staging/wilc1000/coreconfigurator.c
@@ -2007,7 +2007,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
 							pstrWIDs[counter].u16WIDid,
 							(counter == u32WIDsCount - 1), drvHandler)) {
 				ret = -1;
-				printk("[Sendconfigpkt]Get Timed out\n");
+				pr_debug("[Sendconfigpkt]Get Timed out\n");
 				break;
 			}
 		}
@@ -2029,7 +2029,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
 							pstrWIDs[counter].s32ValueSize,
 							(counter == u32WIDsCount - 1), drvHandler)) {
 				ret = -1;
-				printk("[Sendconfigpkt]Set Timed out\n");
+				pr_debug("[Sendconfigpkt]Set Timed out\n");
 				break;
 			}
 		}
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 040e55d..1f32c36 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1306,7 +1306,7 @@ void wilc1000_wlan_deinit(linux_wlan_t *nic)
 
 	if (g_linux_wlan->wilc1000_initialized)	{
 
-		printk("Deinitializing wilc1000  ...\n");
+		pr_info("Deinitializing wilc1000  ...\n");
 
 		if (nic == NULL) {
 			PRINT_ER("nic is NULL\n");
@@ -2565,8 +2565,8 @@ static int __init init_wilc_driver(void)
 	}
 #endif
 
-	printk("IN INIT FUNCTION\n");
-	printk("*** WILC1000 driver VERSION=[10.2] FW_VER=[10.2] ***\n");
+	pr_info("IN INIT FUNCTION\n");
+	pr_info("*** WILC1000 driver VERSION=[10.2] FW_VER=[10.2] ***\n");
 
 	linux_wlan_device_power(1);
 	msleep(100);
@@ -2662,7 +2662,7 @@ static void __exit exit_wilc_driver(void)
 			kfree(g_linux_wlan);
 			g_linux_wlan = NULL;
 		}
-		printk("Module_exit Done.\n");
+		pr_info("Module_exit Done.\n");
 
 #if defined(WILC_DEBUGFS)
 		wilc_debugfs_remove();
diff --git a/drivers/staging/wilc1000/linux_wlan_common.h b/drivers/staging/wilc1000/linux_wlan_common.h
index e6ebf3e..14743ae 100644
--- a/drivers/staging/wilc1000/linux_wlan_common.h
+++ b/drivers/staging/wilc1000/linux_wlan_common.h
@@ -54,8 +54,8 @@ extern atomic_t DEBUG_LEVEL;
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & DEBUG) &&		\
 		   ((atomic_read(&REGION)) & (region))) {		\
-			printk("DBG [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_debug("DBG [%s: %d]", __func__, __LINE__);	\
+			pr_debug(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
@@ -63,8 +63,8 @@ extern atomic_t DEBUG_LEVEL;
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & INFO) &&		\
 		   ((atomic_read(&REGION)) & (region))) {		\
-			printk("INFO [%s]", __func__);			\
-			printk(__VA_ARGS__);				\
+			pr_info("INFO [%s]", __func__);			\
+			pr_info(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
@@ -72,16 +72,16 @@ extern atomic_t DEBUG_LEVEL;
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & WRN) &&		\
 		   ((atomic_read(&REGION)) & (region))) {		\
-			printk("WRN [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_warning("WRN [%s: %d]", __func__, __LINE__);	\
+			pr_warning(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_ER(...)							\
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & ERR)) {		\
-			printk("ERR [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_err("ERR [%s: %d]", __func__, __LINE__);	\
+			pr_err(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
@@ -96,31 +96,31 @@ extern atomic_t DEBUG_LEVEL;
 #define PRINT_D(region, ...)						\
 	do {								\
 		if (DEBUG == 1 && ((REGION)&(region))) {		\
-			printk("DBG [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_debug("DBG [%s: %d]", __func__, __LINE__);	\
+			pr_debug(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_INFO(region, ...)						\
 	do {								\
 		if (INFO == 1 && ((REGION)&(region))) {			\
-			printk("INFO [%s]", __func__);			\
-			printk(__VA_ARGS__);				\
+			pr_info("INFO [%s]", __func__);			\
+			pr_info(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_WRN(region, ...)						\
 	do {								\
 		if (WRN == 1 && ((REGION)&(region))) {			\
-			printk("WRN [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_warning("WRN [%s: %d]", __func__, __LINE__);	\
+			pr_warning(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_ER(...)							\
 	do {								\
-		printk("ERR [%s: %d]", __func__, __LINE__);		\
-		printk(__VA_ARGS__);					\
+		pr_err("ERR [%s: %d]", __func__, __LINE__);		\
+		pr_err(__VA_ARGS__);					\
 	} while (0)
 #endif
 
diff --git a/drivers/staging/wilc1000/linux_wlan_sdio.c b/drivers/staging/wilc1000/linux_wlan_sdio.c
index 37f31f4..13d7f84 100644
--- a/drivers/staging/wilc1000/linux_wlan_sdio.c
+++ b/drivers/staging/wilc1000/linux_wlan_sdio.c
@@ -136,7 +136,7 @@ static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_id
 		return -1;
 	}
 
-	printk("Driver Initializing success\n");
+	pr_info("Driver Initializing success\n");
 	return 0;
 }
 
diff --git a/drivers/staging/wilc1000/linux_wlan_spi.c b/drivers/staging/wilc1000/linux_wlan_spi.c
index 236669c..0bb68a5 100644
--- a/drivers/staging/wilc1000/linux_wlan_spi.c
+++ b/drivers/staging/wilc1000/linux_wlan_spi.c
@@ -50,7 +50,7 @@ static int __init wilc_bus_probe(struct spi_device *spi)
 	PRINT_D(BUS_DBG, "spiMax-Speed: %d\n", spi->max_speed_hz);
 	wilc_spi_dev = spi;
 
-	printk("Driver Initializing success\n");
+	pr_info("Driver Initializing success\n");
 	return 0;
 }
 
diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
index be2e901..588b167 100644
--- a/drivers/staging/wilc1000/wilc_debugfs.c
+++ b/drivers/staging/wilc1000/wilc_debugfs.c
@@ -68,16 +68,16 @@ static ssize_t wilc_debug_level_write(struct file *filp, const char *buf, size_t
 		flag = 100;
 
 	if (flag > DBG_LEVEL_ALL) {
-		printk("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&DEBUG_LEVEL));
+		pr_err("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&DEBUG_LEVEL));
 		return -EFAULT;
 	}
 
 	atomic_set(&DEBUG_LEVEL, (int)flag);
 
 	if (flag == 0)
-		printk("Debug-level disabled\n");
+		pr_info("Debug-level disabled\n");
 	else
-		printk("Debug-level enabled\n");
+		pr_info("Debug-level enabled\n");
 	return count;
 }
 
@@ -110,12 +110,12 @@ static ssize_t wilc_debug_region_write(struct file *filp, const char *buf, size_
 	flag = buffer[0] - '0';
 
 	if (flag > DBG_REGION_ALL) {
-		printk("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&REGION));
+		pr_err("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&REGION));
 		return -EFAULT;
 	}
 
 	atomic_set(&REGION, (int)flag);
-	printk("new debug-region is %x\n", atomic_read(&REGION));
+	pr_info("new debug-region is %x\n", atomic_read(&REGION));
 
 	return count;
 }
@@ -154,12 +154,12 @@ int wilc_debugfs_init(void)
 	wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
 	if (wilc_dir ==  ERR_PTR(-ENODEV)) {
 		/* it's not error. the debugfs is just not being enabled. */
-		printk("ERR, kernel has built without debugfs support\n");
+		pr_err("kernel has built without debugfs support\n");
 		return 0;
 	}
 
 	if (!wilc_dir) {
-		printk("ERR, debugfs create dir\n");
+		pr_err("debugfs create dir\n");
 		return -1;
 	}
 
@@ -172,7 +172,7 @@ int wilc_debugfs_init(void)
 						    &info->fops);
 
 		if (!debugfs_files) {
-			printk("ERR fail to create the debugfs file, %s\n", info->name);
+			pr_err("fail to create the debugfs file, %s\n", info->name);
 			debugfs_remove_recursive(wilc_dir);
 			return -1;
 		}
diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index c3aff9a..ceacfe2 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -37,7 +37,7 @@ static inline int kmalloc_wilc_buffer(void *buf, int size)
 {
 	buf = kmalloc(size, GFP_KERNEL);
 	if (!buf) {
-		printk("fail to alloc memory\n");
+		pr_err("fail to alloc memory\n");
 		return -ENOBUFS;
 	}
 	return 0;
@@ -45,7 +45,7 @@ static inline int kmalloc_wilc_buffer(void *buf, int size)
 
 static int __init wilc_module_init(void)
 {
-	printk("wilc_module_init\n");
+	pr_info("wilc_module_init\n");
 	/*
 	 * alloc necessary memory
 	 */
@@ -74,7 +74,7 @@ error_g_tx_buf:
 
 static void __exit wilc_module_deinit(void)
 {
-	printk("wilc_module_deinit\n");
+	pr_info("wilc_module_deinit\n");
 
 	kfree(exported_g_tx_buf);
 	exported_g_tx_buf = NULL;
-- 
2.1.4

[PATCH 5/5] staging: wilc1000: remove void function return statements that are not useful

[Raphaël Beamonte]

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/host_interface.c    | 4 ----
 drivers/staging/wilc1000/linux_wlan.c        | 1 -
 drivers/staging/wilc1000/wilc_exported_buf.c | 2 --
 drivers/staging/wilc1000/wilc_wlan.c         | 3 ---
 drivers/staging/wilc1000/wilc_wlan_cfg.c     | 2 --
 5 files changed, 12 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 53c4ca9..a000eaf 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -6779,9 +6779,6 @@ void NetworkInfoReceived(u8 *pu8Buffer, u32 u32Length)
 	s32Error = WILC_MsgQueueSend(&gMsgQHostIF, &strHostIFmsg, sizeof(tstrHostIFmsg), NULL);
 	if (s32Error)
 		PRINT_ER("Error in sending network info message queue message parameters: Error(%d)\n", s32Error);
-
-
-	return;
 }
 
 /**
@@ -6845,7 +6842,6 @@ void GnrlAsyncInfoReceived(u8 *pu8Buffer, u32 u32Length)
 
 	/*BugID_5348*/
 	up(&hSemHostIntDeinit);
-	return;
 }
 
 /**
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 1f32c36..507aab6 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1385,7 +1385,6 @@ void wilc1000_wlan_deinit(linux_wlan_t *nic)
 	} else {
 		PRINT_D(INIT_DBG, "wilc1000 is not initialized\n");
 	}
-	return;
 }
 
 int wlan_init_locks(linux_wlan_t *p_nic)
diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index ceacfe2..3f07852 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -84,8 +84,6 @@ static void __exit wilc_module_deinit(void)
 
 	kfree(exported_g_fw_buf);
 	exported_g_fw_buf = NULL;
-
-	return;
 }
 
 MODULE_LICENSE("Dual BSD/GPL");
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index 4c40955..e9552f8 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -124,8 +124,6 @@ static void wilc_debug(uint32_t flag, char *fmt, ...)
 		if (g_wlan.os_func.os_debug)
 			g_wlan.os_func.os_debug(buf);
 	}
-
-	return;
 }
 
 static CHIP_PS_STATE_T genuChipPSstate = CHIP_WAKEDUP;
@@ -1325,7 +1323,6 @@ static void wilc_wlan_handle_rxq(void)
 
 	p->rxq_exit = 1;
 	PRINT_D(RX_DBG, "THREAD: Exiting RX thread\n");
-	return;
 }
 
 /********************************************
diff --git a/drivers/staging/wilc1000/wilc_wlan_cfg.c b/drivers/staging/wilc1000/wilc_wlan_cfg.c
index c10dffe..e2842d3 100644
--- a/drivers/staging/wilc1000/wilc_wlan_cfg.c
+++ b/drivers/staging/wilc1000/wilc_wlan_cfg.c
@@ -363,8 +363,6 @@ static void wilc_wlan_parse_response_frame(uint8_t *info, int size)
 		size -= (2 + len);
 		info += (2 + len);
 	}
-
-	return;
 }
 
 static int wilc_wlan_parse_info_frame(uint8_t *info, int size)
-- 
2.1.4

Re: [PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Dan Carpenter]

On Mon, Aug 17, 2015 at 12:08:35PM -0400, Raphaël Beamonte wrote:
> The MACRO_WILC_BUFFER() macro was using a return statement, and didn't
> take care of possible memory leaks and subsequent bugs when it was failing
> after succeeding some allocations. This patch corrects this behavior.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  drivers/staging/wilc1000/wilc_exported_buf.c | 39 +++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
> index bf392fb..c3aff9a 100644
> --- a/drivers/staging/wilc1000/wilc_exported_buf.c
> +++ b/drivers/staging/wilc1000/wilc_exported_buf.c
> @@ -8,13 +8,6 @@
>  #define LINUX_TX_SIZE	(64 * 1024)
>  #define WILC1000_FW_SIZE (4 * 1024)
>  
> -#define MALLOC_WILC_BUFFER(name, size)	\
> -	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
> -	if (!exported_ ## name) {   \
> -		printk("fail to alloc: %s memory\n", exported_ ## name);  \
> -		return -ENOBUFS;	\
> -	}
> -
>  /*
>   * Add necessary buffer pointers
>   */
> @@ -40,17 +33,43 @@ void *get_fw_buffer(void)
>  }
>  EXPORT_SYMBOL(get_fw_buffer);
>  
> +static inline int kmalloc_wilc_buffer(void *buf, int size)
> +{
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf) {
> +		printk("fail to alloc memory\n");
> +		return -ENOBUFS;
> +	}
> +	return 0;
> +}
> +
>  static int __init wilc_module_init(void)
>  {
>  	printk("wilc_module_init\n");
>  	/*
>  	 * alloc necessary memory
>  	 */
> -	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
> -	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
> -	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
> +	if (kmalloc_wilc_buffer(exported_g_tx_buf, LINUX_TX_SIZE))
> +		goto error_g_tx_buf;

Don't do it this way.  Just do:

	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
	if (!exported_g_tx_buf)
		return -ENOMEM;

	exported_g_rx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
	if (!exported_g_rx_buf)
		goto free_tx_buf;

1) Avoid abstraction where possible.
2) The error code should be -ENOMEM
3) This is not a universal rule, but I feel it's better to return
   directly instead of using a do-nothing-goto.  Some people think
   using gotos everywhere forces you to think about error handling so
   it is worth making the code slightly less readable.  Based on my
   static analysis results, I do not believe that it makes people think
   about error handling.
4) Name the label after the where the label is (what it does on the next
   line) instead of basing it on the goto location.
5) Preserve the error codes.  Use ret = kmalloc_wilc_buffer(); if (ret).

regards,
dan carpenter

Re: [PATCH 2/5] staging: wilc1000: remove FREE_WILC_BUFFER()

[Dan Carpenter]

On Mon, Aug 17, 2015 at 12:08:34PM -0400, Raphaël Beamonte wrote:
> It was just a wrapper around kfree(), so call that instead.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
> +	kfree(exported_g_tx_buf);
> +	exported_g_tx_buf = NULL;

No need to add these new NULL assignments.  The module is unloading so
no one cat re-use these pointers.  Also as a process rule, you should
write down any behaviour changes in the changelog and why you think they
are needed.

regards,
dan carpenter

Re: [PATCH 4/5] staging: wilc1000: use pr_* instead of printk

[Dan Carpenter]

On Mon, Aug 17, 2015 at 12:08:36PM -0400, Raphaël Beamonte wrote:
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  drivers/staging/wilc1000/coreconfigurator.c  |  4 ++--
>  drivers/staging/wilc1000/linux_wlan.c        |  8 +++----
>  drivers/staging/wilc1000/linux_wlan_common.h | 32 ++++++++++++++--------------
>  drivers/staging/wilc1000/linux_wlan_sdio.c   |  2 +-
>  drivers/staging/wilc1000/linux_wlan_spi.c    |  2 +-
>  drivers/staging/wilc1000/wilc_debugfs.c      | 16 +++++++-------
>  drivers/staging/wilc1000/wilc_exported_buf.c |  6 +++---
>  7 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
> index c81c41d..0cbf4a9c 100644
> --- a/drivers/staging/wilc1000/coreconfigurator.c
> +++ b/drivers/staging/wilc1000/coreconfigurator.c
> @@ -2007,7 +2007,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
>  							pstrWIDs[counter].u16WIDid,
>  							(counter == u32WIDsCount - 1), drvHandler)) {
>  				ret = -1;
> -				printk("[Sendconfigpkt]Get Timed out\n");
> +				pr_debug("[Sendconfigpkt]Get Timed out\n");


Possibly pr_err()?


>  				break;
>  			}
>  		}
> @@ -2029,7 +2029,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
>  							pstrWIDs[counter].s32ValueSize,
>  							(counter == u32WIDsCount - 1), drvHandler)) {
>  				ret = -1;
> -				printk("[Sendconfigpkt]Set Timed out\n");
> +				pr_debug("[Sendconfigpkt]Set Timed out\n");
>  				break;
>  			}
>  		}
> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
> index 040e55d..1f32c36 100644
> --- a/drivers/staging/wilc1000/linux_wlan.c
> +++ b/drivers/staging/wilc1000/linux_wlan.c
> @@ -1306,7 +1306,7 @@ void wilc1000_wlan_deinit(linux_wlan_t *nic)
>  
>  	if (g_linux_wlan->wilc1000_initialized)	{
>  
> -		printk("Deinitializing wilc1000  ...\n");
> +		pr_info("Deinitializing wilc1000  ...\n");

debug.

>  
>  		if (nic == NULL) {
>  			PRINT_ER("nic is NULL\n");
> @@ -2565,8 +2565,8 @@ static int __init init_wilc_driver(void)
>  	}
>  #endif
>  
> -	printk("IN INIT FUNCTION\n");
> -	printk("*** WILC1000 driver VERSION=[10.2] FW_VER=[10.2] ***\n");
> +	pr_info("IN INIT FUNCTION\n");

debug.

> +	pr_info("*** WILC1000 driver VERSION=[10.2] FW_VER=[10.2] ***\n");
>  
>  	linux_wlan_device_power(1);
>  	msleep(100);
> @@ -2662,7 +2662,7 @@ static void __exit exit_wilc_driver(void)
>  			kfree(g_linux_wlan);
>  			g_linux_wlan = NULL;
>  		}
> -		printk("Module_exit Done.\n");
> +		pr_info("Module_exit Done.\n");

debug.

>  
>  #if defined(WILC_DEBUGFS)
>  		wilc_debugfs_remove();
> diff --git a/drivers/staging/wilc1000/linux_wlan_common.h b/drivers/staging/wilc1000/linux_wlan_common.h
> index e6ebf3e..14743ae 100644
> --- a/drivers/staging/wilc1000/linux_wlan_common.h
> +++ b/drivers/staging/wilc1000/linux_wlan_common.h
> @@ -54,8 +54,8 @@ extern atomic_t DEBUG_LEVEL;
>  	do {								\
>  		if ((atomic_read(&DEBUG_LEVEL) & DEBUG) &&		\
>  		   ((atomic_read(&REGION)) & (region))) {		\
> -			printk("DBG [%s: %d]", __func__, __LINE__);	\
> -			printk(__VA_ARGS__);				\
> +			pr_debug("DBG [%s: %d]", __func__, __LINE__);	\
> +			pr_debug(__VA_ARGS__);				\

This is a behavior change, I think.  pr_debug() needs to be turned on?

>  		}							\
>  	} while (0)
>  
> @@ -63,8 +63,8 @@ extern atomic_t DEBUG_LEVEL;
>  	do {								\
>  		if ((atomic_read(&DEBUG_LEVEL) & INFO) &&		\
>  		   ((atomic_read(&REGION)) & (region))) {		\
> -			printk("INFO [%s]", __func__);			\
> -			printk(__VA_ARGS__);				\
> +			pr_info("INFO [%s]", __func__);			\
> +			pr_info(__VA_ARGS__);				\
>  		}							\
>  	} while (0)
>  
> @@ -72,16 +72,16 @@ extern atomic_t DEBUG_LEVEL;
>  	do {								\
>  		if ((atomic_read(&DEBUG_LEVEL) & WRN) &&		\
>  		   ((atomic_read(&REGION)) & (region))) {		\
> -			printk("WRN [%s: %d]", __func__, __LINE__);	\
> -			printk(__VA_ARGS__);				\
> +			pr_warning("WRN [%s: %d]", __func__, __LINE__);	\
> +			pr_warning(__VA_ARGS__);				\
>  		}							\
>  	} while (0)
>  
>  #define PRINT_ER(...)							\
>  	do {								\
>  		if ((atomic_read(&DEBUG_LEVEL) & ERR)) {		\
> -			printk("ERR [%s: %d]", __func__, __LINE__);	\
> -			printk(__VA_ARGS__);				\
> +			pr_err("ERR [%s: %d]", __func__, __LINE__);	\
> +			pr_err(__VA_ARGS__);				\
>  		}							\
>  	} while (0)
>  
> @@ -96,31 +96,31 @@ extern atomic_t DEBUG_LEVEL;
>  #define PRINT_D(region, ...)						\
>  	do {								\
>  		if (DEBUG == 1 && ((REGION)&(region))) {		\
> -			printk("DBG [%s: %d]", __func__, __LINE__);	\
> -			printk(__VA_ARGS__);				\
> +			pr_debug("DBG [%s: %d]", __func__, __LINE__);	\
> +			pr_debug(__VA_ARGS__);				\
>  		}							\
>  	} while (0)
>  
>  #define PRINT_INFO(region, ...)						\
>  	do {								\
>  		if (INFO == 1 && ((REGION)&(region))) {			\
> -			printk("INFO [%s]", __func__);			\
> -			printk(__VA_ARGS__);				\
> +			pr_info("INFO [%s]", __func__);			\
> +			pr_info(__VA_ARGS__);				\
>  		}							\
>  	} while (0)
>  
>  #define PRINT_WRN(region, ...)						\
>  	do {								\
>  		if (WRN == 1 && ((REGION)&(region))) {			\
> -			printk("WRN [%s: %d]", __func__, __LINE__);	\
> -			printk(__VA_ARGS__);				\
> +			pr_warning("WRN [%s: %d]", __func__, __LINE__);	\
> +			pr_warning(__VA_ARGS__);				\
>  		}							\
>  	} while (0)
>  
>  #define PRINT_ER(...)							\
>  	do {								\
> -		printk("ERR [%s: %d]", __func__, __LINE__);		\
> -		printk(__VA_ARGS__);					\
> +		pr_err("ERR [%s: %d]", __func__, __LINE__);		\
> +		pr_err(__VA_ARGS__);					\
>  	} while (0)
>  #endif
>  
> diff --git a/drivers/staging/wilc1000/linux_wlan_sdio.c b/drivers/staging/wilc1000/linux_wlan_sdio.c
> index 37f31f4..13d7f84 100644
> --- a/drivers/staging/wilc1000/linux_wlan_sdio.c
> +++ b/drivers/staging/wilc1000/linux_wlan_sdio.c
> @@ -136,7 +136,7 @@ static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_id
>  		return -1;
>  	}
>  
> -	printk("Driver Initializing success\n");
> +	pr_info("Driver Initializing success\n");
>  	return 0;
>  }
>  
> diff --git a/drivers/staging/wilc1000/linux_wlan_spi.c b/drivers/staging/wilc1000/linux_wlan_spi.c
> index 236669c..0bb68a5 100644
> --- a/drivers/staging/wilc1000/linux_wlan_spi.c
> +++ b/drivers/staging/wilc1000/linux_wlan_spi.c
> @@ -50,7 +50,7 @@ static int __init wilc_bus_probe(struct spi_device *spi)
>  	PRINT_D(BUS_DBG, "spiMax-Speed: %d\n", spi->max_speed_hz);
>  	wilc_spi_dev = spi;
>  
> -	printk("Driver Initializing success\n");
> +	pr_info("Driver Initializing success\n");
>  	return 0;
>  }
>  
> diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
> index be2e901..588b167 100644
> --- a/drivers/staging/wilc1000/wilc_debugfs.c
> +++ b/drivers/staging/wilc1000/wilc_debugfs.c
> @@ -68,16 +68,16 @@ static ssize_t wilc_debug_level_write(struct file *filp, const char *buf, size_t
>  		flag = 100;
>  
>  	if (flag > DBG_LEVEL_ALL) {
> -		printk("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&DEBUG_LEVEL));
> +		pr_err("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&DEBUG_LEVEL));
>  		return -EFAULT;
>  	}
>  
>  	atomic_set(&DEBUG_LEVEL, (int)flag);
>  
>  	if (flag == 0)
> -		printk("Debug-level disabled\n");
> +		pr_info("Debug-level disabled\n");
>  	else
> -		printk("Debug-level enabled\n");
> +		pr_info("Debug-level enabled\n");

Ugh...  I guess debug?  Eventually we will want to remove most of this
debug code.  It's just lazy and messy.


>  	return count;
>  }
>  
> @@ -110,12 +110,12 @@ static ssize_t wilc_debug_region_write(struct file *filp, const char *buf, size_
>  	flag = buffer[0] - '0';
>  
>  	if (flag > DBG_REGION_ALL) {
> -		printk("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&REGION));
> +		pr_err("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&REGION));
>  		return -EFAULT;
>  	}
>  
>  	atomic_set(&REGION, (int)flag);
> -	printk("new debug-region is %x\n", atomic_read(&REGION));
> +	pr_info("new debug-region is %x\n", atomic_read(&REGION));

debug.

>  
>  	return count;
>  }
> @@ -154,12 +154,12 @@ int wilc_debugfs_init(void)
>  	wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
>  	if (wilc_dir ==  ERR_PTR(-ENODEV)) {
>  		/* it's not error. the debugfs is just not being enabled. */
> -		printk("ERR, kernel has built without debugfs support\n");
> +		pr_err("kernel has built without debugfs support\n");
>  		return 0;
>  	}
>  
>  	if (!wilc_dir) {
> -		printk("ERR, debugfs create dir\n");
> +		pr_err("debugfs create dir\n");
>  		return -1;
>  	}
>  
> @@ -172,7 +172,7 @@ int wilc_debugfs_init(void)
>  						    &info->fops);
>  
>  		if (!debugfs_files) {
> -			printk("ERR fail to create the debugfs file, %s\n", info->name);
> +			pr_err("fail to create the debugfs file, %s\n", info->name);
>  			debugfs_remove_recursive(wilc_dir);
>  			return -1;
>  		}
> diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
> index c3aff9a..ceacfe2 100644
> --- a/drivers/staging/wilc1000/wilc_exported_buf.c
> +++ b/drivers/staging/wilc1000/wilc_exported_buf.c
> @@ -37,7 +37,7 @@ static inline int kmalloc_wilc_buffer(void *buf, int size)
>  {
>  	buf = kmalloc(size, GFP_KERNEL);
>  	if (!buf) {
> -		printk("fail to alloc memory\n");
> +		pr_err("fail to alloc memory\n");

Delete this.

>  		return -ENOBUFS;
>  	}
>  	return 0;
> @@ -45,7 +45,7 @@ static inline int kmalloc_wilc_buffer(void *buf, int size)
>  
>  static int __init wilc_module_init(void)
>  {
> -	printk("wilc_module_init\n");
> +	pr_info("wilc_module_init\n");

Delete this.  We have ftrae.

>  	/*
>  	 * alloc necessary memory
>  	 */
> @@ -74,7 +74,7 @@ error_g_tx_buf:
>  
>  static void __exit wilc_module_deinit(void)
>  {
> -	printk("wilc_module_deinit\n");
> +	pr_info("wilc_module_deinit\n");

Delete this.

>  
>  	kfree(exported_g_tx_buf);
>  	exported_g_tx_buf = NULL;
> -- 
> 2.1.4

Re: [PATCH 4/5] staging: wilc1000: use pr_* instead of printk

[Raphaël Beamonte]

2015-08-17 13:47 GMT-04:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> -                             printk("[Sendconfigpkt]Get Timed out\n");
>> +                             pr_debug("[Sendconfigpkt]Get Timed out\n");
>
>
> Possibly pr_err()?

Yep. My mistake. I'll do the same for Set Timed Out also!

>> -                     printk("DBG [%s: %d]", __func__, __LINE__);     \
>> -                     printk(__VA_ARGS__);                            \
>> +                     pr_debug("DBG [%s: %d]", __func__, __LINE__);   \
>> +                     pr_debug(__VA_ARGS__);                          \
>
> This is a behavior change, I think.  pr_debug() needs to be turned on?

Yes... I didn't pay attention to that! pr_debug needs -DDEBUG in the makefile.
Should I use pr_info here? Or just acknowledge the behavior change for
the moment,
as the next aim is probably, as you said, to remove all the local
debug code? (it is
actually part of the TODO of this driver... So I could just work on that next.)

I'll include your other log level advices and send a new version.
Thanks for your
reviewing time!

Re: [PATCH 4/5] staging: wilc1000: use pr_* instead of printk

[Dan Carpenter]

On Mon, Aug 17, 2015 at 01:59:44PM -0400, Raphaël Beamonte wrote:
> 2015-08-17 13:47 GMT-04:00 Dan Carpenter <dan.carpenter@oracle.com>:
> >> -                             printk("[Sendconfigpkt]Get Timed out\n");
> >> +                             pr_debug("[Sendconfigpkt]Get Timed out\n");
> >
> >
> > Possibly pr_err()?
> 
> Yep. My mistake. I'll do the same for Set Timed Out also!
> 
> >> -                     printk("DBG [%s: %d]", __func__, __LINE__);     \
> >> -                     printk(__VA_ARGS__);                            \
> >> +                     pr_debug("DBG [%s: %d]", __func__, __LINE__);   \
> >> +                     pr_debug(__VA_ARGS__);                          \
> >
> > This is a behavior change, I think.  pr_debug() needs to be turned on?
> 
> Yes... I didn't pay attention to that! pr_debug needs -DDEBUG in the makefile.
> Should I use pr_info here? Or just acknowledge the behavior change for
> the moment,
> as the next aim is probably, as you said, to remove all the local
> debug code? (it is
> actually part of the TODO of this driver... So I could just work on that next.)

I would probably just do the rest and leave this part as-is since you're
planning to redo it all anyway.  I guess just do stuff which is obvious
and hopefully more and more stuff will become obvious as you go along.
This is a lazy answer but I don't want to think about this driver very
hard...  :P

Also always try to order your patches from least controversial to most
controversial.  It makes it easier to redo things or sometimes Greg
applies the first part of a patch series.

regards,
dan carpenter

[PATCHv2 0/5] staging: wilc1000: code improvements

[Raphaël Beamonte]

Hi,

Following comments from Dan Carpenter, please find the following
revised patches.

Raphaël


Raphaël Beamonte (5):
  staging: wilc1000: remove void function return statements that are not
    useful
  staging: wilc1000: use pr_* instead of printk
  staging: wilc1000: remove DECLARE_WILC_BUFFER()
  staging: wilc1000: remove FREE_WILC_BUFFER()
  staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid
    possible memory leak

 drivers/staging/wilc1000/coreconfigurator.c  |  4 +-
 drivers/staging/wilc1000/host_interface.c    |  4 --
 drivers/staging/wilc1000/linux_wlan.c        |  9 ++--
 drivers/staging/wilc1000/linux_wlan_common.h | 28 ++++++-------
 drivers/staging/wilc1000/linux_wlan_sdio.c   |  2 +-
 drivers/staging/wilc1000/linux_wlan_spi.c    |  2 +-
 drivers/staging/wilc1000/wilc_debugfs.c      | 16 ++++----
 drivers/staging/wilc1000/wilc_exported_buf.c | 61 ++++++++++++++++------------
 drivers/staging/wilc1000/wilc_wlan.c         |  3 --
 drivers/staging/wilc1000/wilc_wlan_cfg.c     |  2 -
 10 files changed, 64 insertions(+), 67 deletions(-)

-- 
2.1.4

[PATCHv2 1/5] staging: wilc1000: remove void function return statements that are not useful

[Raphaël Beamonte]

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/host_interface.c    | 4 ----
 drivers/staging/wilc1000/linux_wlan.c        | 1 -
 drivers/staging/wilc1000/wilc_exported_buf.c | 4 +---
 drivers/staging/wilc1000/wilc_wlan.c         | 3 ---
 drivers/staging/wilc1000/wilc_wlan_cfg.c     | 2 --
 5 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index c473877..0cfc97d 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -6782,9 +6782,6 @@ void NetworkInfoReceived(u8 *pu8Buffer, u32 u32Length)
 	s32Error = WILC_MsgQueueSend(&gMsgQHostIF, &strHostIFmsg, sizeof(tstrHostIFmsg));
 	if (s32Error)
 		PRINT_ER("Error in sending network info message queue message parameters: Error(%d)\n", s32Error);
-
-
-	return;
 }
 
 /**
@@ -6848,7 +6845,6 @@ void GnrlAsyncInfoReceived(u8 *pu8Buffer, u32 u32Length)
 
 	/*BugID_5348*/
 	up(&hSemHostIntDeinit);
-	return;
 }
 
 /**
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 7eacc2f..e6e8a20 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1385,7 +1385,6 @@ void wilc1000_wlan_deinit(linux_wlan_t *nic)
 	} else {
 		PRINT_D(INIT_DBG, "wilc1000 is not initialized\n");
 	}
-	return;
 }
 
 int wlan_init_locks(linux_wlan_t *p_nic)
diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index 5294578..deba6bd 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -65,12 +65,10 @@ static void __exit wilc_module_deinit(void)
 	FREE_WILC_BUFFER(g_tx_buf)
 	FREE_WILC_BUFFER(g_rx_buf)
 	FREE_WILC_BUFFER(g_fw_buf)
-
-	return;
 }
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Tony Cho");
 MODULE_DESCRIPTION("WILC1xxx Memory Manager");
 pure_initcall(wilc_module_init);
-module_exit(wilc_module_deinit);
\ No newline at end of file
+module_exit(wilc_module_deinit);
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index fac16db..7c53a2b 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -124,8 +124,6 @@ static void wilc_debug(uint32_t flag, char *fmt, ...)
 		if (g_wlan.os_func.os_debug)
 			g_wlan.os_func.os_debug(buf);
 	}
-
-	return;
 }
 
 static CHIP_PS_STATE_T genuChipPSstate = CHIP_WAKEDUP;
@@ -1325,7 +1323,6 @@ static void wilc_wlan_handle_rxq(void)
 
 	p->rxq_exit = 1;
 	PRINT_D(RX_DBG, "THREAD: Exiting RX thread\n");
-	return;
 }
 
 /********************************************
diff --git a/drivers/staging/wilc1000/wilc_wlan_cfg.c b/drivers/staging/wilc1000/wilc_wlan_cfg.c
index c10dffe..e2842d3 100644
--- a/drivers/staging/wilc1000/wilc_wlan_cfg.c
+++ b/drivers/staging/wilc1000/wilc_wlan_cfg.c
@@ -363,8 +363,6 @@ static void wilc_wlan_parse_response_frame(uint8_t *info, int size)
 		size -= (2 + len);
 		info += (2 + len);
 	}
-
-	return;
 }
 
 static int wilc_wlan_parse_info_frame(uint8_t *info, int size)
-- 
2.1.4

[PATCHv2 2/5] staging: wilc1000: use pr_* instead of printk

[Raphaël Beamonte]

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/coreconfigurator.c  |  4 ++--
 drivers/staging/wilc1000/linux_wlan.c        |  8 ++++----
 drivers/staging/wilc1000/linux_wlan_common.h | 28 ++++++++++++++--------------
 drivers/staging/wilc1000/linux_wlan_sdio.c   |  2 +-
 drivers/staging/wilc1000/linux_wlan_spi.c    |  2 +-
 drivers/staging/wilc1000/wilc_debugfs.c      | 16 ++++++++--------
 drivers/staging/wilc1000/wilc_exported_buf.c |  2 --
 7 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
index 1a5b165..8a0ebba 100644
--- a/drivers/staging/wilc1000/coreconfigurator.c
+++ b/drivers/staging/wilc1000/coreconfigurator.c
@@ -2005,7 +2005,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
 							pstrWIDs[counter].u16WIDid,
 							(counter == u32WIDsCount - 1), drvHandler)) {
 				ret = -1;
-				printk("[Sendconfigpkt]Get Timed out\n");
+				pr_err("[Sendconfigpkt]Get Timed out\n");
 				break;
 			}
 		}
@@ -2027,7 +2027,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
 							pstrWIDs[counter].s32ValueSize,
 							(counter == u32WIDsCount - 1), drvHandler)) {
 				ret = -1;
-				printk("[Sendconfigpkt]Set Timed out\n");
+				pr_err("[Sendconfigpkt]Set Timed out\n");
 				break;
 			}
 		}
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index e6e8a20..a02be31 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1306,7 +1306,7 @@ void wilc1000_wlan_deinit(linux_wlan_t *nic)
 
 	if (g_linux_wlan->wilc1000_initialized)	{
 
-		printk("Deinitializing wilc1000  ...\n");
+		pr_debug("Deinitializing wilc1000  ...\n");
 
 		if (nic == NULL) {
 			PRINT_ER("nic is NULL\n");
@@ -2565,8 +2565,8 @@ static int __init init_wilc_driver(void)
 	}
 #endif
 
-	printk("IN INIT FUNCTION\n");
-	printk("*** WILC1000 driver VERSION=[10.2] FW_VER=[10.2] ***\n");
+	pr_debug("IN INIT FUNCTION\n");
+	pr_info("*** WILC1000 driver VERSION=[10.2] FW_VER=[10.2] ***\n");
 
 	linux_wlan_device_power(1);
 	msleep(100);
@@ -2662,7 +2662,7 @@ static void __exit exit_wilc_driver(void)
 			kfree(g_linux_wlan);
 			g_linux_wlan = NULL;
 		}
-		printk("Module_exit Done.\n");
+		pr_debug("Module_exit Done.\n");
 
 #if defined(WILC_DEBUGFS)
 		wilc_debugfs_remove();
diff --git a/drivers/staging/wilc1000/linux_wlan_common.h b/drivers/staging/wilc1000/linux_wlan_common.h
index e6ebf3e..8f3cc1a 100644
--- a/drivers/staging/wilc1000/linux_wlan_common.h
+++ b/drivers/staging/wilc1000/linux_wlan_common.h
@@ -63,8 +63,8 @@ extern atomic_t DEBUG_LEVEL;
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & INFO) &&		\
 		   ((atomic_read(&REGION)) & (region))) {		\
-			printk("INFO [%s]", __func__);			\
-			printk(__VA_ARGS__);				\
+			pr_info("INFO [%s]", __func__);			\
+			pr_info(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
@@ -72,16 +72,16 @@ extern atomic_t DEBUG_LEVEL;
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & WRN) &&		\
 		   ((atomic_read(&REGION)) & (region))) {		\
-			printk("WRN [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_warning("WRN [%s: %d]", __func__, __LINE__);	\
+			pr_warning(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_ER(...)							\
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & ERR)) {		\
-			printk("ERR [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_err("ERR [%s: %d]", __func__, __LINE__);	\
+			pr_err(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
@@ -96,31 +96,31 @@ extern atomic_t DEBUG_LEVEL;
 #define PRINT_D(region, ...)						\
 	do {								\
 		if (DEBUG == 1 && ((REGION)&(region))) {		\
-			printk("DBG [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_debug("DBG [%s: %d]", __func__, __LINE__);	\
+			pr_debug(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_INFO(region, ...)						\
 	do {								\
 		if (INFO == 1 && ((REGION)&(region))) {			\
-			printk("INFO [%s]", __func__);			\
-			printk(__VA_ARGS__);				\
+			pr_info("INFO [%s]", __func__);			\
+			pr_info(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_WRN(region, ...)						\
 	do {								\
 		if (WRN == 1 && ((REGION)&(region))) {			\
-			printk("WRN [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			pr_warning("WRN [%s: %d]", __func__, __LINE__);	\
+			pr_warning(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_ER(...)							\
 	do {								\
-		printk("ERR [%s: %d]", __func__, __LINE__);		\
-		printk(__VA_ARGS__);					\
+		pr_err("ERR [%s: %d]", __func__, __LINE__);		\
+		pr_err(__VA_ARGS__);					\
 	} while (0)
 #endif
 
diff --git a/drivers/staging/wilc1000/linux_wlan_sdio.c b/drivers/staging/wilc1000/linux_wlan_sdio.c
index 37f31f4..13d7f84 100644
--- a/drivers/staging/wilc1000/linux_wlan_sdio.c
+++ b/drivers/staging/wilc1000/linux_wlan_sdio.c
@@ -136,7 +136,7 @@ static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_id
 		return -1;
 	}
 
-	printk("Driver Initializing success\n");
+	pr_info("Driver Initializing success\n");
 	return 0;
 }
 
diff --git a/drivers/staging/wilc1000/linux_wlan_spi.c b/drivers/staging/wilc1000/linux_wlan_spi.c
index 236669c..0bb68a5 100644
--- a/drivers/staging/wilc1000/linux_wlan_spi.c
+++ b/drivers/staging/wilc1000/linux_wlan_spi.c
@@ -50,7 +50,7 @@ static int __init wilc_bus_probe(struct spi_device *spi)
 	PRINT_D(BUS_DBG, "spiMax-Speed: %d\n", spi->max_speed_hz);
 	wilc_spi_dev = spi;
 
-	printk("Driver Initializing success\n");
+	pr_info("Driver Initializing success\n");
 	return 0;
 }
 
diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
index be2e901..ac65eef 100644
--- a/drivers/staging/wilc1000/wilc_debugfs.c
+++ b/drivers/staging/wilc1000/wilc_debugfs.c
@@ -68,16 +68,16 @@ static ssize_t wilc_debug_level_write(struct file *filp, const char *buf, size_t
 		flag = 100;
 
 	if (flag > DBG_LEVEL_ALL) {
-		printk("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&DEBUG_LEVEL));
+		pr_err("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&DEBUG_LEVEL));
 		return -EFAULT;
 	}
 
 	atomic_set(&DEBUG_LEVEL, (int)flag);
 
 	if (flag == 0)
-		printk("Debug-level disabled\n");
+		pr_debug("Debug-level disabled\n");
 	else
-		printk("Debug-level enabled\n");
+		pr_debug("Debug-level enabled\n");
 	return count;
 }
 
@@ -110,12 +110,12 @@ static ssize_t wilc_debug_region_write(struct file *filp, const char *buf, size_
 	flag = buffer[0] - '0';
 
 	if (flag > DBG_REGION_ALL) {
-		printk("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&REGION));
+		pr_err("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&REGION));
 		return -EFAULT;
 	}
 
 	atomic_set(&REGION, (int)flag);
-	printk("new debug-region is %x\n", atomic_read(&REGION));
+	pr_debug("new debug-region is %x\n", atomic_read(&REGION));
 
 	return count;
 }
@@ -154,12 +154,12 @@ int wilc_debugfs_init(void)
 	wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
 	if (wilc_dir ==  ERR_PTR(-ENODEV)) {
 		/* it's not error. the debugfs is just not being enabled. */
-		printk("ERR, kernel has built without debugfs support\n");
+		pr_err("kernel has built without debugfs support\n");
 		return 0;
 	}
 
 	if (!wilc_dir) {
-		printk("ERR, debugfs create dir\n");
+		pr_err("debugfs create dir\n");
 		return -1;
 	}
 
@@ -172,7 +172,7 @@ int wilc_debugfs_init(void)
 						    &info->fops);
 
 		if (!debugfs_files) {
-			printk("ERR fail to create the debugfs file, %s\n", info->name);
+			pr_err("fail to create the debugfs file, %s\n", info->name);
 			debugfs_remove_recursive(wilc_dir);
 			return -1;
 		}
diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index deba6bd..55b6232 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -48,7 +48,6 @@ EXPORT_SYMBOL(get_fw_buffer);
 
 static int __init wilc_module_init(void)
 {
-	printk("wilc_module_init\n");
 	/*
 	 * alloc necessary memory
 	 */
@@ -61,7 +60,6 @@ static int __init wilc_module_init(void)
 
 static void __exit wilc_module_deinit(void)
 {
-	printk("wilc_module_deinit\n");
 	FREE_WILC_BUFFER(g_tx_buf)
 	FREE_WILC_BUFFER(g_rx_buf)
 	FREE_WILC_BUFFER(g_fw_buf)
-- 
2.1.4

[PATCHv2 3/5] staging: wilc1000: remove DECLARE_WILC_BUFFER()

[Raphaël Beamonte]

It was just a wrapper to initialize a variable. Initialize it
directly instead.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index 55b6232..148d608 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -8,9 +8,6 @@
 #define LINUX_TX_SIZE	(64 * 1024)
 #define WILC1000_FW_SIZE (4 * 1024)
 
-#define DECLARE_WILC_BUFFER(name)	\
-	void *exported_ ## name = NULL;
-
 #define MALLOC_WILC_BUFFER(name, size)	\
 	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
 	if (!exported_ ## name) {   \
@@ -24,9 +21,9 @@
 /*
  * Add necessary buffer pointers
  */
-DECLARE_WILC_BUFFER(g_tx_buf)
-DECLARE_WILC_BUFFER(g_rx_buf)
-DECLARE_WILC_BUFFER(g_fw_buf)
+void *exported_g_tx_buf;
+void *exported_g_rx_buf;
+void *exported_g_fw_buf;
 
 void *get_tx_buffer(void)
 {
-- 
2.1.4

[PATCHv2 4/5] staging: wilc1000: remove FREE_WILC_BUFFER()

[Raphaël Beamonte]

It was just a wrapper around kfree(), so call that instead.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index 148d608..c9a5943 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -15,9 +15,6 @@
 		return -ENOBUFS;	\
 	}
 
-#define FREE_WILC_BUFFER(name)	\
-	kfree(exported_ ## name);
-
 /*
  * Add necessary buffer pointers
  */
@@ -57,9 +54,9 @@ static int __init wilc_module_init(void)
 
 static void __exit wilc_module_deinit(void)
 {
-	FREE_WILC_BUFFER(g_tx_buf)
-	FREE_WILC_BUFFER(g_rx_buf)
-	FREE_WILC_BUFFER(g_fw_buf)
+	kfree(exported_g_tx_buf);
+	kfree(exported_g_rx_buf);
+	kfree(exported_g_fw_buf);
 }
 
 MODULE_LICENSE("Dual BSD/GPL");
-- 
2.1.4

[PATCHv2 5/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Raphaël Beamonte]

The MACRO_WILC_BUFFER() macro was using a return statement, and didn't
take care of possible memory leaks and subsequent bugs when it was failing
after succeeding some allocations. This patch corrects this behavior.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 37 ++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index c9a5943..0f3bdad 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -8,13 +8,6 @@
 #define LINUX_TX_SIZE	(64 * 1024)
 #define WILC1000_FW_SIZE (4 * 1024)
 
-#define MALLOC_WILC_BUFFER(name, size)	\
-	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
-	if (!exported_ ## name) {   \
-		printk("fail to alloc: %s memory\n", exported_ ## name);  \
-		return -ENOBUFS;	\
-	}
-
 /*
  * Add necessary buffer pointers
  */
@@ -45,11 +38,35 @@ static int __init wilc_module_init(void)
 	/*
 	 * alloc necessary memory
 	 */
-	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
-	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
-	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
+	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
+	if (!exported_g_tx_buf) {
+		pr_err("fail to alloc tx buf");
+		return -ENOMEM;
+	}
+
+	exported_g_rx_buf = kmalloc(LINUX_RX_SIZE, GFP_KERNEL);
+	if (!exported_g_rx_buf) {
+		pr_err("fail to alloc rx buf");
+		goto free_g_tx_buf;
+	}
+
+	exported_g_fw_buf = kmalloc(WILC1000_FW_SIZE, GFP_KERNEL);
+	if (!exported_g_fw_buf) {
+		pr_err("fail to alloc fw buf");
+		goto free_g_rx_buf;
+	}
 
 	return 0;
+
+free_g_rx_buf:
+	kfree(exported_g_rx_buf);
+	exported_g_rx_buf = NULL;
+
+free_g_tx_buf:
+	kfree(exported_g_tx_buf);
+	exported_g_tx_buf = NULL;
+
+	return -ENOMEM;
 }
 
 static void __exit wilc_module_deinit(void)
-- 
2.1.4

Re: [PATCHv2 5/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Arend van Spriel]

On 08/17/2015 09:28 PM, Raphaël Beamonte wrote:
> The MACRO_WILC_BUFFER() macro was using a return statement, and didn't

Probable MACRO_WILC_BUFFER should be MALLOC_WILC_BUFFER here.

> take care of possible memory leaks and subsequent bugs when it was failing
> after succeeding some allocations. This patch corrects this behavior.
>
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>   drivers/staging/wilc1000/wilc_exported_buf.c | 37 ++++++++++++++++++++--------
>   1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
> index c9a5943..0f3bdad 100644
> --- a/drivers/staging/wilc1000/wilc_exported_buf.c
> +++ b/drivers/staging/wilc1000/wilc_exported_buf.c
> @@ -8,13 +8,6 @@
>   #define LINUX_TX_SIZE	(64 * 1024)
>   #define WILC1000_FW_SIZE (4 * 1024)
>
> -#define MALLOC_WILC_BUFFER(name, size)	\
> -	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
> -	if (!exported_ ## name) {   \
> -		printk("fail to alloc: %s memory\n", exported_ ## name);  \
> -		return -ENOBUFS;	\
> -	}
> -
>   /*
>    * Add necessary buffer pointers
>    */
> @@ -45,11 +38,35 @@ static int __init wilc_module_init(void)
>   	/*
>   	 * alloc necessary memory
>   	 */
> -	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
> -	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
> -	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
> +	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
> +	if (!exported_g_tx_buf) {
> +		pr_err("fail to alloc tx buf");

There is really no need to print an error message here. kmalloc will 
blurb enough info when it fails.

So these buffers are globals? So does this driver support multiple 
devices, ie. how are these used when two wilc1000 supported devices are 
present.

Regards,
Arend

> +		return -ENOMEM;
> +	}
> +
> +	exported_g_rx_buf = kmalloc(LINUX_RX_SIZE, GFP_KERNEL);
> +	if (!exported_g_rx_buf) {
> +		pr_err("fail to alloc rx buf");
> +		goto free_g_tx_buf;
> +	}
> +
> +	exported_g_fw_buf = kmalloc(WILC1000_FW_SIZE, GFP_KERNEL);
> +	if (!exported_g_fw_buf) {
> +		pr_err("fail to alloc fw buf");
> +		goto free_g_rx_buf;
> +	}
>
>   	return 0;
> +
> +free_g_rx_buf:
> +	kfree(exported_g_rx_buf);
> +	exported_g_rx_buf = NULL;
> +
> +free_g_tx_buf:
> +	kfree(exported_g_tx_buf);
> +	exported_g_tx_buf = NULL;
> +
> +	return -ENOMEM;
>   }
>
>   static void __exit wilc_module_deinit(void)
>

Re: [PATCHv2 2/5] staging: wilc1000: use pr_* instead of printk

[Greg Kroah-Hartman]

On Mon, Aug 17, 2015 at 03:28:27PM -0400, Raphaël Beamonte wrote:
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  drivers/staging/wilc1000/coreconfigurator.c  |  4 ++--
>  drivers/staging/wilc1000/linux_wlan.c        |  8 ++++----
>  drivers/staging/wilc1000/linux_wlan_common.h | 28 ++++++++++++++--------------
>  drivers/staging/wilc1000/linux_wlan_sdio.c   |  2 +-
>  drivers/staging/wilc1000/linux_wlan_spi.c    |  2 +-
>  drivers/staging/wilc1000/wilc_debugfs.c      | 16 ++++++++--------
>  drivers/staging/wilc1000/wilc_exported_buf.c |  2 --
>  7 files changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
> index 1a5b165..8a0ebba 100644
> --- a/drivers/staging/wilc1000/coreconfigurator.c
> +++ b/drivers/staging/wilc1000/coreconfigurator.c
> @@ -2005,7 +2005,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
>  							pstrWIDs[counter].u16WIDid,
>  							(counter == u32WIDsCount - 1), drvHandler)) {
>  				ret = -1;
> -				printk("[Sendconfigpkt]Get Timed out\n");
> +				pr_err("[Sendconfigpkt]Get Timed out\n");

This is a network driver, please use the proper logging statement for
such a thing (i.e. netdev_err()).

thanks,

greg k-h

Re: [PATCHv2 4/5] staging: wilc1000: remove FREE_WILC_BUFFER()

[Greg Kroah-Hartman]

On Mon, Aug 17, 2015 at 03:28:29PM -0400, Raphaël Beamonte wrote:
> It was just a wrapper around kfree(), so call that instead.

Someone else already did this, sorry.

greg k-h

[PATCHv3] staging: wilc1000: use netdev_* instead of printk

[Raphaël Beamonte]

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/coreconfigurator.c  |  4 ++--
 drivers/staging/wilc1000/linux_wlan.c        |  8 ++++----
 drivers/staging/wilc1000/linux_wlan_common.h | 28 ++++++++++++++--------------
 drivers/staging/wilc1000/linux_wlan_sdio.c   |  2 +-
 drivers/staging/wilc1000/linux_wlan_spi.c    |  2 +-
 drivers/staging/wilc1000/wilc_debugfs.c      | 16 ++++++++--------
 drivers/staging/wilc1000/wilc_exported_buf.c |  2 --
 7 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
index 16a0abc..9a36b78 100644
--- a/drivers/staging/wilc1000/coreconfigurator.c
+++ b/drivers/staging/wilc1000/coreconfigurator.c
@@ -2001,7 +2001,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
 							pstrWIDs[counter].u16WIDid,
 							(counter == u32WIDsCount - 1), drvHandler)) {
 				ret = -1;
-				printk("[Sendconfigpkt]Get Timed out\n");
+				netdev_err("[Sendconfigpkt]Get Timed out\n");
 				break;
 			}
 		}
@@ -2023,7 +2023,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
 							pstrWIDs[counter].s32ValueSize,
 							(counter == u32WIDsCount - 1), drvHandler)) {
 				ret = -1;
-				printk("[Sendconfigpkt]Set Timed out\n");
+				netdev_err("[Sendconfigpkt]Set Timed out\n");
 				break;
 			}
 		}
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index b3cc9f5..7903572 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1306,7 +1306,7 @@ void wilc1000_wlan_deinit(linux_wlan_t *nic)
 
 	if (g_linux_wlan->wilc1000_initialized)	{
 
-		printk("Deinitializing wilc1000  ...\n");
+		netdev_debug("Deinitializing wilc1000  ...\n");
 
 		if (nic == NULL) {
 			PRINT_ER("nic is NULL\n");
@@ -2565,8 +2565,8 @@ static int __init init_wilc_driver(void)
 	}
 #endif
 
-	printk("IN INIT FUNCTION\n");
-	printk("*** WILC1000 driver VERSION=[10.2] FW_VER=[10.2] ***\n");
+	netdev_debug("IN INIT FUNCTION\n");
+	netdev_info("*** WILC1000 driver VERSION=[10.2] FW_VER=[10.2] ***\n");
 
 	linux_wlan_device_power(1);
 	msleep(100);
@@ -2662,7 +2662,7 @@ static void __exit exit_wilc_driver(void)
 			kfree(g_linux_wlan);
 			g_linux_wlan = NULL;
 		}
-		printk("Module_exit Done.\n");
+		netdev_debug("Module_exit Done.\n");
 
 #if defined(WILC_DEBUGFS)
 		wilc_debugfs_remove();
diff --git a/drivers/staging/wilc1000/linux_wlan_common.h b/drivers/staging/wilc1000/linux_wlan_common.h
index e6ebf3e..7f53d8c 100644
--- a/drivers/staging/wilc1000/linux_wlan_common.h
+++ b/drivers/staging/wilc1000/linux_wlan_common.h
@@ -63,8 +63,8 @@ extern atomic_t DEBUG_LEVEL;
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & INFO) &&		\
 		   ((atomic_read(&REGION)) & (region))) {		\
-			printk("INFO [%s]", __func__);			\
-			printk(__VA_ARGS__);				\
+			netdev_info("INFO [%s]", __func__);			\
+			netdev_info(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
@@ -72,16 +72,16 @@ extern atomic_t DEBUG_LEVEL;
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & WRN) &&		\
 		   ((atomic_read(&REGION)) & (region))) {		\
-			printk("WRN [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			netdev_warning("WRN [%s: %d]", __func__, __LINE__);	\
+			netdev_warning(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_ER(...)							\
 	do {								\
 		if ((atomic_read(&DEBUG_LEVEL) & ERR)) {		\
-			printk("ERR [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			netdev_err("ERR [%s: %d]", __func__, __LINE__);	\
+			netdev_err(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
@@ -96,31 +96,31 @@ extern atomic_t DEBUG_LEVEL;
 #define PRINT_D(region, ...)						\
 	do {								\
 		if (DEBUG == 1 && ((REGION)&(region))) {		\
-			printk("DBG [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			netdev_debug("DBG [%s: %d]", __func__, __LINE__);	\
+			netdev_debug(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_INFO(region, ...)						\
 	do {								\
 		if (INFO == 1 && ((REGION)&(region))) {			\
-			printk("INFO [%s]", __func__);			\
-			printk(__VA_ARGS__);				\
+			netdev_info("INFO [%s]", __func__);			\
+			netdev_info(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_WRN(region, ...)						\
 	do {								\
 		if (WRN == 1 && ((REGION)&(region))) {			\
-			printk("WRN [%s: %d]", __func__, __LINE__);	\
-			printk(__VA_ARGS__);				\
+			netdev_warning("WRN [%s: %d]", __func__, __LINE__);	\
+			netdev_warning(__VA_ARGS__);				\
 		}							\
 	} while (0)
 
 #define PRINT_ER(...)							\
 	do {								\
-		printk("ERR [%s: %d]", __func__, __LINE__);		\
-		printk(__VA_ARGS__);					\
+		netdev_err("ERR [%s: %d]", __func__, __LINE__);		\
+		netdev_err(__VA_ARGS__);					\
 	} while (0)
 #endif
 
diff --git a/drivers/staging/wilc1000/linux_wlan_sdio.c b/drivers/staging/wilc1000/linux_wlan_sdio.c
index 37f31f4..ef2cfa9 100644
--- a/drivers/staging/wilc1000/linux_wlan_sdio.c
+++ b/drivers/staging/wilc1000/linux_wlan_sdio.c
@@ -136,7 +136,7 @@ static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_id
 		return -1;
 	}
 
-	printk("Driver Initializing success\n");
+	netdev_info("Driver Initializing success\n");
 	return 0;
 }
 
diff --git a/drivers/staging/wilc1000/linux_wlan_spi.c b/drivers/staging/wilc1000/linux_wlan_spi.c
index 236669c..675db07 100644
--- a/drivers/staging/wilc1000/linux_wlan_spi.c
+++ b/drivers/staging/wilc1000/linux_wlan_spi.c
@@ -50,7 +50,7 @@ static int __init wilc_bus_probe(struct spi_device *spi)
 	PRINT_D(BUS_DBG, "spiMax-Speed: %d\n", spi->max_speed_hz);
 	wilc_spi_dev = spi;
 
-	printk("Driver Initializing success\n");
+	netdev_info("Driver Initializing success\n");
 	return 0;
 }
 
diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
index ae11186..7e88723 100644
--- a/drivers/staging/wilc1000/wilc_debugfs.c
+++ b/drivers/staging/wilc1000/wilc_debugfs.c
@@ -59,16 +59,16 @@ static ssize_t wilc_debug_level_write(struct file *filp, const char __user *buf,
 		return ret;
 
 	if (flag > DBG_LEVEL_ALL) {
-		printk("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&DEBUG_LEVEL));
+		netdev_err("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&DEBUG_LEVEL));
 		return -EINVAL;
 	}
 
 	atomic_set(&DEBUG_LEVEL, (int)flag);
 
 	if (flag == 0)
-		printk("Debug-level disabled\n");
+		netdev_debug("Debug-level disabled\n");
 	else
-		printk("Debug-level enabled\n");
+		netdev_debug("Debug-level enabled\n");
 
 	return count;
 }
@@ -102,12 +102,12 @@ static ssize_t wilc_debug_region_write(struct file *filp, const char *buf, size_
 	flag = buffer[0] - '0';
 
 	if (flag > DBG_REGION_ALL) {
-		printk("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&REGION));
+		netdev_err("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n", __func__, flag, atomic_read(&REGION));
 		return -EFAULT;
 	}
 
 	atomic_set(&REGION, (int)flag);
-	printk("new debug-region is %x\n", atomic_read(&REGION));
+	netdev_debug("new debug-region is %x\n", atomic_read(&REGION));
 
 	return count;
 }
@@ -146,12 +146,12 @@ int wilc_debugfs_init(void)
 	wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
 	if (wilc_dir ==  ERR_PTR(-ENODEV)) {
 		/* it's not error. the debugfs is just not being enabled. */
-		printk("ERR, kernel has built without debugfs support\n");
+		netdev_err("kernel has built without debugfs support\n");
 		return 0;
 	}
 
 	if (!wilc_dir) {
-		printk("ERR, debugfs create dir\n");
+		netdev_err("debugfs create dir\n");
 		return -1;
 	}
 
@@ -164,7 +164,7 @@ int wilc_debugfs_init(void)
 						    &info->fops);
 
 		if (!debugfs_files) {
-			printk("ERR fail to create the debugfs file, %s\n", info->name);
+			netdev_err("fail to create the debugfs file, %s\n", info->name);
 			debugfs_remove_recursive(wilc_dir);
 			return -1;
 		}
diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index c3f6a0a..148d608 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -45,7 +45,6 @@ EXPORT_SYMBOL(get_fw_buffer);
 
 static int __init wilc_module_init(void)
 {
-	printk("wilc_module_init\n");
 	/*
 	 * alloc necessary memory
 	 */
@@ -58,7 +57,6 @@ static int __init wilc_module_init(void)
 
 static void __exit wilc_module_deinit(void)
 {
-	printk("wilc_module_deinit\n");
 	FREE_WILC_BUFFER(g_tx_buf)
 	FREE_WILC_BUFFER(g_rx_buf)
 	FREE_WILC_BUFFER(g_fw_buf)
-- 
2.1.4

[PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Raphaël Beamonte]

The MALLOC_WILC_BUFFER() macro was using a return statement, and didn't
take care of possible memory leaks and subsequent bugs when it was failing
after succeeding some allocations. This patch corrects this behavior.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 31 +++++++++++++++++++---------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index c3f6a0a..ec8d8e5 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -8,13 +8,6 @@
 #define LINUX_TX_SIZE	(64 * 1024)
 #define WILC1000_FW_SIZE (4 * 1024)
 
-#define MALLOC_WILC_BUFFER(name, size)	\
-	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
-	if (!exported_ ## name) {   \
-		printk("fail to alloc: %s memory\n", exported_ ## name);  \
-		return -ENOBUFS;	\
-	}
-
 #define FREE_WILC_BUFFER(name)	\
 	kfree(exported_ ## name);
 
@@ -49,11 +42,29 @@ static int __init wilc_module_init(void)
 	/*
 	 * alloc necessary memory
 	 */
-	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
-	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
-	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
+	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
+	if (!exported_g_tx_buf)
+		return -ENOMEM;
+
+	exported_g_rx_buf = kmalloc(LINUX_RX_SIZE, GFP_KERNEL);
+	if (!exported_g_rx_buf)
+		goto free_g_tx_buf;
+
+	exported_g_fw_buf = kmalloc(WILC1000_FW_SIZE, GFP_KERNEL);
+	if (!exported_g_fw_buf)
+		goto free_g_rx_buf;
 
 	return 0;
+
+free_g_rx_buf:
+	kfree(exported_g_rx_buf);
+	exported_g_rx_buf = NULL;
+
+free_g_tx_buf:
+	kfree(exported_g_tx_buf);
+	exported_g_tx_buf = NULL;
+
+	return -ENOMEM;
 }
 
 static void __exit wilc_module_deinit(void)
-- 
2.1.4

Re: [PATCHv2 5/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Raphaël Beamonte]

2015-08-17 15:41 GMT-04:00 Arend van Spriel <arend@broadcom.com>:
> Probable MACRO_WILC_BUFFER should be MALLOC_WILC_BUFFER here.

Good catch!

> There is really no need to print an error message here. kmalloc will blurb
> enough info when it fails.

Ok!

> So these buffers are globals? So does this driver support multiple devices,
> ie. how are these used when two wilc1000 supported devices are present.

Not sure. I mostly did code refactoring to have a clearer source code
and try to respect the kernel code style. I don't have a compatible
device to try and test it unfortunately.

Thanks for the feedback. I just sent a revised version of the patch
taking your comments into account.

Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Dan Carpenter]

On Mon, Aug 17, 2015 at 07:12:47PM -0400, Raphaël Beamonte wrote:
> The MALLOC_WILC_BUFFER() macro was using a return statement, and didn't
> take care of possible memory leaks and subsequent bugs when it was failing
> after succeeding some allocations. This patch corrects this behavior.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---

Yes.  This looks good now.

regards,
dan carpenter

Re: [PATCHv3] staging: wilc1000: use netdev_* instead of printk

[Sudip Mukherjee]

On Mon, Aug 17, 2015 at 07:06:40PM -0400, Raphaël Beamonte wrote:
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  drivers/staging/wilc1000/coreconfigurator.c  |  4 ++--
>  drivers/staging/wilc1000/linux_wlan.c        |  8 ++++----
>  drivers/staging/wilc1000/linux_wlan_common.h | 28 ++++++++++++++--------------
>  drivers/staging/wilc1000/linux_wlan_sdio.c   |  2 +-
>  drivers/staging/wilc1000/linux_wlan_spi.c    |  2 +-
>  drivers/staging/wilc1000/wilc_debugfs.c      | 16 ++++++++--------
>  drivers/staging/wilc1000/wilc_exported_buf.c |  2 --
>  7 files changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
> index 16a0abc..9a36b78 100644
> --- a/drivers/staging/wilc1000/coreconfigurator.c
> +++ b/drivers/staging/wilc1000/coreconfigurator.c
> @@ -2001,7 +2001,7 @@ s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
>  							pstrWIDs[counter].u16WIDid,
>  							(counter == u32WIDsCount - 1), drvHandler)) {
>  				ret = -1;
> -				printk("[Sendconfigpkt]Get Timed out\n");
> +				netdev_err("[Sendconfigpkt]Get Timed out\n");
This will not compile. you can not just replace printk with
netdev_*, you need to mention a net_device.

regards
sudip

Re: [PATCHv3] staging: wilc1000: use netdev_* instead of printk

[Raphaël Beamonte]

2015-08-18 0:24 GMT-04:00 Sudip Mukherjee <sudipm.mukherjee@gmail.com>:
>> +                             netdev_err("[Sendconfigpkt]Get Timed out\n");
> This will not compile. you can not just replace printk with
> netdev_*, you need to mention a net_device.

You're right! I'm making a lot of mistakes. It seems I called the make
command in the wrong git tree... Thanks for catching that, and taking
time to review!
I just saw there isn't any netdev_debug available. Do you know the
reason for that? Is there a replacement besides pr_debug?
Also, it seems that in all the functions where it's another netdev
that's available that I could use, I don't have access to the
net_device struct. Is there a general way to get that device
information? I saw an old lkml post talking about dev_*() functions to
do that? Would you have some hint on the matter?

Thanks a lot!
Raphaël

Re: [PATCHv3] staging: wilc1000: use netdev_* instead of printk

[Sudip Mukherjee]

On Tue, Aug 18, 2015 at 01:27:31AM -0400, Raphaël Beamonte wrote:
> 2015-08-18 0:24 GMT-04:00 Sudip Mukherjee <sudipm.mukherjee@gmail.com>:
> >> +                             netdev_err("[Sendconfigpkt]Get Timed out\n");
> > This will not compile. you can not just replace printk with
> > netdev_*, you need to mention a net_device.
> 
> You're right! I'm making a lot of mistakes. It seems I called the make
> command in the wrong git tree... Thanks for catching that, and taking
> time to review!
> I just saw there isn't any netdev_debug available. Do you know the
> reason for that? Is there a replacement besides pr_debug?
netdev_dbg
> Also, it seems that in all the functions where it's another netdev
> that's available that I could use, I don't have access to the
> net_device struct. Is there a general way to get that device
> information? I saw an old lkml post talking about dev_*() functions to
> do that? Would you have some hint on the matter?
If the function is not having the net_device try to see if you can
pass it the struct as an argument. if not possible then in any way then
no other way but to use pr_*

regards
sudip

Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Dan Carpenter]

To be honest, I have lost track of this patchset.  If you are planning
to redo the other patches can you send it in a new thread?

regards,
dan carpenter

Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Raphaël Beamonte]

2015-08-18 5:15 GMT-04:00 Dan Carpenter <dan.carpenter@oracle.com>:
> To be honest, I have lost track of this patchset.  If you are planning
> to redo the other patches can you send it in a new thread?

Actually, Greg already included the "return statement" and
"DECLARE_WILC_BUFFER" ones.
The replacement of printk by netdev_* needs more work on my side to
get the net_device to be able to use the netdev_* functions.
And apparently Greg already received another patch with the
"FREE_WILC_BUFFER" replacement, though I don't see it in the
staging-testing tree yet.

So, I think this patch is the last one of this patchset that has to be
treated! That's why I rebased it on top of the current staging-testing
tree on my last send.

Thanks,
Raphaël

Re: [PATCHv3] staging: wilc1000: use netdev_* instead of printk

[Greg Kroah-Hartman]

On Mon, Aug 17, 2015 at 07:06:40PM -0400, Raphaël Beamonte wrote:
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>

You can't submit a patch with no changelog information, sorry.

Always build your patches, otherwise you make maintainers really grumpy
as it breaks their build.

greg k-h

Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Greg Kroah-Hartman]

On Tue, Aug 18, 2015 at 01:06:39PM -0400, Raphaël Beamonte wrote:
> 2015-08-18 5:15 GMT-04:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > To be honest, I have lost track of this patchset.  If you are planning
> > to redo the other patches can you send it in a new thread?
> 
> Actually, Greg already included the "return statement" and
> "DECLARE_WILC_BUFFER" ones.
> The replacement of printk by netdev_* needs more work on my side to
> get the net_device to be able to use the netdev_* functions.
> And apparently Greg already received another patch with the
> "FREE_WILC_BUFFER" replacement, though I don't see it in the
> staging-testing tree yet.

Maybe it was something else, but it would not apply.  Please use git
rebase to figure it out and resend all of your outstanding patches, I
too am confused at this point.

thanks,

greg k-h

[PATCHv4 0/2] staging: wilc1000: code improvements

[Raphaël Beamonte]

Hi,

As requested, here are the two remaining ready patches of this
patchset. I pulled and rebased against staging-testing just now.
They should thus be usable without problem!

Thanks,
Raphaël


Raphaël Beamonte (2):
  staging: wilc1000: remove FREE_WILC_BUFFER()
  staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid
    possible memory leak

 drivers/staging/wilc1000/wilc_exported_buf.c | 40 +++++++++++++++++-----------
 1 file changed, 24 insertions(+), 16 deletions(-)

-- 
2.1.4

[PATCHv4 1/2] staging: wilc1000: remove FREE_WILC_BUFFER()

[Raphaël Beamonte]

It was just a wrapper around kfree(), so call that instead.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index c3f6a0a..44db496 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -15,9 +15,6 @@
 		return -ENOBUFS;	\
 	}
 
-#define FREE_WILC_BUFFER(name)	\
-	kfree(exported_ ## name);
-
 /*
  * Add necessary buffer pointers
  */
@@ -59,9 +56,9 @@ static int __init wilc_module_init(void)
 static void __exit wilc_module_deinit(void)
 {
 	printk("wilc_module_deinit\n");
-	FREE_WILC_BUFFER(g_tx_buf)
-	FREE_WILC_BUFFER(g_rx_buf)
-	FREE_WILC_BUFFER(g_fw_buf)
+	kfree(exported_g_tx_buf);
+	kfree(exported_g_rx_buf);
+	kfree(exported_g_fw_buf);
 }
 
 MODULE_LICENSE("Dual BSD/GPL");
-- 
2.1.4

[PATCHv4 2/2] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

[Raphaël Beamonte]

The MALLOC_WILC_BUFFER() macro was using a return statement, and didn't
take care of possible memory leaks and subsequent bugs when it was failing
after succeeding some allocations. This patch corrects this behavior.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 31 +++++++++++++++++++---------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index 44db496..e617b77 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -8,13 +8,6 @@
 #define LINUX_TX_SIZE	(64 * 1024)
 #define WILC1000_FW_SIZE (4 * 1024)
 
-#define MALLOC_WILC_BUFFER(name, size)	\
-	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
-	if (!exported_ ## name) {   \
-		printk("fail to alloc: %s memory\n", exported_ ## name);  \
-		return -ENOBUFS;	\
-	}
-
 /*
  * Add necessary buffer pointers
  */
@@ -46,11 +39,29 @@ static int __init wilc_module_init(void)
 	/*
 	 * alloc necessary memory
 	 */
-	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
-	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
-	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
+	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
+	if (!exported_g_tx_buf)
+		return -ENOMEM;
+
+	exported_g_rx_buf = kmalloc(LINUX_RX_SIZE, GFP_KERNEL);
+	if (!exported_g_rx_buf)
+		goto free_g_tx_buf;
+
+	exported_g_fw_buf = kmalloc(WILC1000_FW_SIZE, GFP_KERNEL);
+	if (!exported_g_fw_buf)
+		goto free_g_rx_buf;
 
 	return 0;
+
+free_g_rx_buf:
+	kfree(exported_g_rx_buf);
+	exported_g_rx_buf = NULL;
+
+free_g_tx_buf:
+	kfree(exported_g_tx_buf);
+	exported_g_tx_buf = NULL;
+
+	return -ENOMEM;
 }
 
 static void __exit wilc_module_deinit(void)
-- 
2.1.4

Re: [PATCHv4 1/2] staging: wilc1000: remove FREE_WILC_BUFFER()

[Greg Kroah-Hartman]

On Tue, Aug 18, 2015 at 11:14:49PM -0400, Raphaël Beamonte wrote:
> It was just a wrapper around kfree(), so call that instead.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  drivers/staging/wilc1000/wilc_exported_buf.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

Turns out this file is never even built, you should just remove it :)

Re: [PATCHv4 1/2] staging: wilc1000: remove FREE_WILC_BUFFER()

[Raphaël Beamonte]

2015-09-02 21:19 GMT-04:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> Turns out this file is never even built, you should just remove it :)

You're right, although it seems that is one of the "To-dos" of that
module, as the references I find about the config variable to allow
the compiling of that file is the following:

config WILC1000_PREALLOCATE_DURING_SYSTEM_BOOT
          bool "Preallocate memory pool during system boot"
          ---help---
                    To do.

Found on https://github.com/linux4sc/wireless-driver/blob/master/wilc1000/Kconfig
However, it seems that entry of the Kconfig has been removed in the
kernel. It thus can probably be safe to remove all occurences linked
to that option from the driver in the kernel, while the authors will
be able to add them back when it will be a working configuration
option. I'll do that!

Re: [PATCHv4 1/2] staging: wilc1000: remove FREE_WILC_BUFFER()

[Raphaël Beamonte]

Oh well. Actually you did it. I answered while pulling the git...
Sorry for that unuseful mail! :)

2015-09-05 12:25 GMT-04:00 Raphaël Beamonte <raphael.beamonte@gmail.com>:
> 2015-09-02 21:19 GMT-04:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>> Turns out this file is never even built, you should just remove it :)
>
> You're right, although it seems that is one of the "To-dos" of that
> module, as the references I find about the config variable to allow
> the compiling of that file is the following:
>
> config WILC1000_PREALLOCATE_DURING_SYSTEM_BOOT
>           bool "Preallocate memory pool during system boot"
>           ---help---
>                     To do.
>
> Found on https://github.com/linux4sc/wireless-driver/blob/master/wilc1000/Kconfig
> However, it seems that entry of the Kconfig has been removed in the
> kernel. It thus can probably be safe to remove all occurences linked
> to that option from the driver in the kernel, while the authors will
> be able to add them back when it will be a working configuration
> option. I'll do that!