staging: wilc1000: code style patches
cover staging: wilc1000: code style patches
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(-)
Signature · 1 line
2.1.4
PATCH 1/3 staging: wilc1000: code style: fix macro with multiple statements
Macros with multiple statements should be enclosed in a do - while loop
--- drivers/staging/wilc1000/wilc_exported_buf.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +8 -6
@@ -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); +module_exit (wilc_module_deinit);
Signature · 1 line
2.1.4
Re: [PATCH 1/3] staging: wilc1000: code style: fix macro with multiple statements
drivers/staging/wilc1000/wilc_exported_buf.c line 21
Quoted 13 lines · Raphaël Beamonte
#define
MALLOC_WILC_BUFFER(name, size)\drivers/staging/wilc1000/wilc_exported_buf.c +7
-5- 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
2015-08-17 5:08 GMT-04:00 Dan Carpenter <[email protected]>:
Quoted 3 lines
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
cover staging: wilc1000: code improvements
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(-)
Signature · 1 line
2.1.4
PATCH 1/5 staging: wilc1000: remove DECLARE_WILC_BUFFER()
It was just a wrapper to initialize a variable. Initialize it directly instead.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +4 -7
@@ -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); +module_exit (wilc_module_deinit);
Signature · 1 line
2.1.4
PATCH 2/5 staging: wilc1000: remove FREE_WILC_BUFFER()
In reply to Raphaël BeamonteIt was just a wrapper around kfree(), so call that instead.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +9 -6
@@ -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 ; }
Signature · 1 line
2.1.4
Re: [PATCH 2/5] staging: wilc1000: remove FREE_WILC_BUFFER()
drivers/staging/wilc1000/wilc_exported_buf.c static int __init wilc_module_init(void) · line 61
Quoted 6 lines · Raphaël Beamonte
It was just a wrapper around
kfree(), so call that instead.Signed-off-by Raphaël Beamonte <[email protected]>---
drivers/staging/wilc1000/wilc_exported_buf.c +2
-0+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
PATCH 3/5 staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
In reply to Raphaël BeamonteThe 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.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 39 +++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +29 -10
@@ -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)
Signature · 1 line
2.1.4
Re: [PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
Quoted 42 lines · 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 <[email protected]>--- drivers/staging/wilc1000/wilc_exported_buf.c | 39 +++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-)drivers/staging/wilc1000/wilc_exported_buf.c +10
-7@@ -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 )
drivers/staging/wilc1000/wilc_exported_buf.c void *get_fw_buffer(void) · line 53
Quoted 10 lines
drivers/staging/wilc1000/wilc_exported_buf.c +2
-3{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
PATCH 4/5 staging: wilc1000: use pr_* instead of printk
In reply to Raphaël Beamonte--- 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(-)
drivers/staging/wilc1000/coreconfigurator.c +2 -2
@@ -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 ; } }
drivers/staging/wilc1000/linux_wlan.c +4 -4
@@ -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 ();
drivers/staging/wilc1000/linux_wlan_common.h +16 -16
@@ -54,8 +54,8 @@ extern atomic_t DEBUG_LEVEL;do { \if ((atomic_read (&DEBUG_LEVEL ) &DEBUG ) && \ ((atomic_read (®ION )) & (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 (®ION )) & (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 (®ION )) & (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
drivers/staging/wilc1000/linux_wlan_sdio.c +1 -1
@@ -136,7 +136,7 @@ static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_idreturn -1 ; } -printk ("Driver Initializing success\n" ); +pr_info ("Driver Initializing success\n" );return 0 ; }
drivers/staging/wilc1000/linux_wlan_spi.c +1 -1
@@ -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 ; }
drivers/staging/wilc1000/wilc_debugfs.c +8 -8
@@ -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 (®ION )); +pr_err ("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n" , __func__, flag,atomic_read (®ION ));return -EFAULT ; }atomic_set (®ION , (int )flag); -printk ("new debug-region is %x\n" ,atomic_read (®ION )); +pr_info ("new debug-region is %x\n" ,atomic_read (®ION ));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 ; }
drivers/staging/wilc1000/wilc_exported_buf.c +3 -3
@@ -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 ;
Signature · 1 line
2.1.4
Re: [PATCH 4/5] staging: wilc1000: use pr_* instead of printk
Quoted 16 lines · Raphaël Beamonte
Signed-off-by Raphaël Beamonte <[email protected]>--- 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(-)drivers/staging/wilc1000/coreconfigurator.c +0
-0
drivers/staging/wilc1000/coreconfigurator.c s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs, · line 2010
Quoted 5 lines
drivers/staging/wilc1000/coreconfigurator.c +1
-1@@ -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()?
Quoted 17 lines
drivers/staging/wilc1000/coreconfigurator.c +1
-1break ; } }@@ -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 ; } }drivers/staging/wilc1000/linux_wlan.c +0
-0
drivers/staging/wilc1000/linux_wlan.c void wilc1000_wlan_deinit(linux_wlan_t *nic) · line 1309
Quoted 5 lines
drivers/staging/wilc1000/linux_wlan.c +1
-1@@ -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.
drivers/staging/wilc1000/linux_wlan.c static int __init init_wilc_driver(void) · line 2568
Quoted 10 lines
drivers/staging/wilc1000/linux_wlan.c +1
-2if (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.
drivers/staging/wilc1000/linux_wlan.c static void __exit exit_wilc_driver(void) · line 2665
Quoted 10 lines
drivers/staging/wilc1000/linux_wlan.c +2
-1+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.
Quoted 8 lines
drivers/staging/wilc1000/linux_wlan.c +0
-0#if defined(WILC_DEBUGFS )wilc_debugfs_remove ();drivers/staging/wilc1000/linux_wlan_common.h +0
-0
drivers/staging/wilc1000/linux_wlan_common.h extern atomic_t DEBUG_LEVEL; · line 58
Quoted 7 lines
drivers/staging/wilc1000/linux_wlan_common.h +2
-2@@ -54,8 +54,8 @@ extern atomic_t DEBUG_LEVEL; do { \ if ((atomic_read(&DEBUG_LEVEL) & DEBUG) && \ ((atomic_read(®ION)) & (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?
Quoted 112 lines
drivers/staging/wilc1000/linux_wlan_common.h +14
-14} \ }while (0 )@@ -63,8 +63,8 @@ extern atomic_t DEBUG_LEVEL;do { \if ((atomic_read (&DEBUG_LEVEL ) &INFO ) && \ ((atomic_read (®ION )) & (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 (®ION )) & (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 drivers/staging/wilc1000/linux_wlan_sdio.c +1
-1@@ -136,7 +136,7 @@ static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_idreturn -1 ; } -printk ("Driver Initializing success\n" ); +pr_info ("Driver Initializing success\n" );return 0 ; }drivers/staging/wilc1000/linux_wlan_spi.c +1
-1@@ -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 ; }drivers/staging/wilc1000/wilc_debugfs.c +1
-1@@ -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 ;
drivers/staging/wilc1000/wilc_debugfs.c static ssize_t wilc_debug_level_write(struct file *filp, const char *buf, size_t · line 80
Quoted 10 lines
drivers/staging/wilc1000/wilc_debugfs.c +2
-2}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.
Quoted 5 lines
drivers/staging/wilc1000/wilc_debugfs.c +0
-0return count; }@@ -110,12 +110,12 @@ static ssize_t wilc_debug_region_write(struct file *filp, const char *buf, size_ flag = buffer[0 ] -'0' ;
drivers/staging/wilc1000/wilc_debugfs.c static ssize_t wilc_debug_region_write(struct file *filp, const char *buf, size_ · line 118
Quoted 10 lines
drivers/staging/wilc1000/wilc_debugfs.c +2
-2if (flag >DBG_REGION_ALL ) { -printk ("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n" , __func__, flag,atomic_read (®ION )); +pr_err ("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n" , __func__, flag,atomic_read (®ION ));return -EFAULT ; }atomic_set (®ION , (int )flag); -printk ("new debug-region is %x\n" ,atomic_read (®ION )); +pr_info ("new debug-region is %x\n" ,atomic_read (®ION ));
debug.
Quoted 32 lines
drivers/staging/wilc1000/wilc_debugfs.c +3
-3return 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 ; }drivers/staging/wilc1000/wilc_exported_buf.c +0
-0
drivers/staging/wilc1000/wilc_exported_buf.c static inline int kmalloc_wilc_buffer(void *buf, int size) · line 40
Quoted 5 lines
drivers/staging/wilc1000/wilc_exported_buf.c +1
-1@@ -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.
drivers/staging/wilc1000/wilc_exported_buf.c static inline int kmalloc_wilc_buffer(void *buf, int size) · line 48
Quoted 9 lines
drivers/staging/wilc1000/wilc_exported_buf.c +1
-1return -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.
drivers/staging/wilc1000/wilc_exported_buf.c error_g_tx_buf: · line 77
Quoted 9 lines
drivers/staging/wilc1000/wilc_exported_buf.c +1
-1/* * 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.
Quoted 5 lines
drivers/staging/wilc1000/wilc_exported_buf.c +0
-0kfree (exported_g_tx_buf); exported_g_tx_buf =NULL ;Signature · 1 line
2.1.4
Re: [PATCH 4/5] staging: wilc1000: use pr_* instead of printk
2015-08-17 13:47 GMT-04:00 Dan Carpenter <[email protected]>:
Quoted 5 lines
drivers/staging/wilc1000/coreconfigurator.c s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs, · line 2010
Quoted 2 lines
drivers/staging/wilc1000/coreconfigurator.c +1
-1-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!
Quoted 6 lines
drivers/staging/wilc1000/linux_wlan_common.h extern atomic_t DEBUG_LEVEL; · line 58
Quoted 4 lines
drivers/staging/wilc1000/linux_wlan_common.h +2
-2-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
Quoted 14 lines · Raphaël Beamonte
2015-08-17 13:47 GMT-04:00 Dan Carpenter <[email protected]>:
Quoted 5 lines
drivers/staging/wilc1000/coreconfigurator.c s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs, · line 2010
Quoted 2 lines
drivers/staging/wilc1000/coreconfigurator.c +1
-1-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!
Quoted 5 lines
Quoted 4 lines
drivers/staging/wilc1000/linux_wlan_common.h +2
-2-printk ("DBG [%s: %d]" , __func__, __LINE__); \ -printk (__VA_ARGS__); \ +pr_debug ("DBG [%s: %d]" , __func__, __LINE__); \ +pr_debug (__VA_ARGS__); \
Quoted 8 lines
Quoted 1 line
This is a behavior change, I think.
pr_debug()needs to be turned on?Yes... I didn't pay attention to that!
pr_debugneeds -DDEBUG in the makefile. Should I usepr_infohere? 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
PATCH 5/5 staging: wilc1000: remove void function return statements that are not useful
In reply to Raphaël Beamonte--- 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(-)
drivers/staging/wilc1000/host_interface.c +0 -4
@@ -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 ; }/**
drivers/staging/wilc1000/linux_wlan.c +0 -1
@@ -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)
drivers/staging/wilc1000/wilc_exported_buf.c +0 -2
@@ -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" );
drivers/staging/wilc1000/wilc_wlan.c +0 -3
@@ -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 ; }/********************************************
drivers/staging/wilc1000/wilc_wlan_cfg.c +0 -2
@@ -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)
Signature · 1 line
2.1.4
v2 cover staging: wilc1000: code improvements
In reply to Raphaël BeamonteHi,
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(-)
Signature · 1 line
2.1.4
PATCH v2 1/5 staging: wilc1000: remove void function return statements that are not useful
--- 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(-)
drivers/staging/wilc1000/host_interface.c +0 -4
@@ -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 ; }/**
drivers/staging/wilc1000/linux_wlan.c +0 -1
@@ -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)
drivers/staging/wilc1000/wilc_exported_buf.c +1 -3
@@ -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); +module_exit (wilc_module_deinit);
drivers/staging/wilc1000/wilc_wlan.c +0 -3
@@ -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 ; }/********************************************
drivers/staging/wilc1000/wilc_wlan_cfg.c +0 -2
@@ -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)
Signature · 1 line
2.1.4
PATCH v2 2/5 staging: wilc1000: use pr_* instead of printk
In reply to Raphaël Beamonte--- 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(-)
drivers/staging/wilc1000/coreconfigurator.c +2 -2
@@ -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 ; } }
drivers/staging/wilc1000/linux_wlan.c +4 -4
@@ -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 ();
drivers/staging/wilc1000/linux_wlan_common.h +14 -14
@@ -63,8 +63,8 @@ extern atomic_t DEBUG_LEVEL;do { \if ((atomic_read (&DEBUG_LEVEL ) &INFO ) && \ ((atomic_read (®ION )) & (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 (®ION )) & (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
drivers/staging/wilc1000/linux_wlan_sdio.c +1 -1
@@ -136,7 +136,7 @@ static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_idreturn -1 ; } -printk ("Driver Initializing success\n" ); +pr_info ("Driver Initializing success\n" );return 0 ; }
drivers/staging/wilc1000/linux_wlan_spi.c +1 -1
@@ -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 ; }
drivers/staging/wilc1000/wilc_debugfs.c +8 -8
@@ -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 (®ION )); +pr_err ("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n" , __func__, flag,atomic_read (®ION ));return -EFAULT ; }atomic_set (®ION , (int )flag); -printk ("new debug-region is %x\n" ,atomic_read (®ION )); +pr_debug ("new debug-region is %x\n" ,atomic_read (®ION ));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 ; }
drivers/staging/wilc1000/wilc_exported_buf.c +0 -2
@@ -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)
Signature · 1 line
2.1.4
Greg Kroah-Hartman
maintainerRe: [PATCHv2 2/5] staging: wilc1000: use pr_* instead of printk
Quoted 16 lines · Raphaël Beamonte
Signed-off-by Raphaël Beamonte <[email protected]>--- 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(-)drivers/staging/wilc1000/coreconfigurator.c +0
-0
drivers/staging/wilc1000/coreconfigurator.c s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs, · line 2008
Quoted 5 lines
drivers/staging/wilc1000/coreconfigurator.c +1
-1@@ -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
PATCH v3 1/1 staging: wilc1000: use netdev_* instead of printk
--- 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(-)
drivers/staging/wilc1000/coreconfigurator.c +2 -2
@@ -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 ; } }
drivers/staging/wilc1000/linux_wlan.c +4 -4
@@ -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 ();
drivers/staging/wilc1000/linux_wlan_common.h +14 -14
@@ -63,8 +63,8 @@ extern atomic_t DEBUG_LEVEL;do { \if ((atomic_read (&DEBUG_LEVEL ) &INFO ) && \ ((atomic_read (®ION )) & (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 (®ION )) & (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
drivers/staging/wilc1000/linux_wlan_sdio.c +1 -1
@@ -136,7 +136,7 @@ static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_idreturn -1 ; } -printk ("Driver Initializing success\n" ); +netdev_info ("Driver Initializing success\n" );return 0 ; }
drivers/staging/wilc1000/linux_wlan_spi.c +1 -1
@@ -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 ; }
drivers/staging/wilc1000/wilc_debugfs.c +8 -8
@@ -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 (®ION )); +netdev_err ("%s, value (0x%08x) is out of range, stay previous flag (0x%08x)\n" , __func__, flag,atomic_read (®ION ));return -EFAULT ; }atomic_set (®ION , (int )flag); -printk ("new debug-region is %x\n" ,atomic_read (®ION )); +netdev_debug ("new debug-region is %x\n" ,atomic_read (®ION ));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 ; }
drivers/staging/wilc1000/wilc_exported_buf.c +0 -2
@@ -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)
Signature · 1 line
2.1.4
Re: [PATCHv3] staging: wilc1000: use netdev_* instead of printk
Quoted 16 lines · Raphaël Beamonte
Signed-off-by Raphaël Beamonte <[email protected]>--- 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(-)drivers/staging/wilc1000/coreconfigurator.c +0
-0
drivers/staging/wilc1000/coreconfigurator.c s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs, · line 2004
Quoted 5 lines
drivers/staging/wilc1000/coreconfigurator.c +1
-1@@ -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
2015-08-18 0:24 GMT-04:00 Sudip Mukherjee <[email protected]>:
Quoted 3 lines
Quoted 1 line
+
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
Quoted 10 lines · Raphaël Beamonte
2015-08-18 0:24 GMT-04:00 Sudip Mukherjee <[email protected]>:
Quoted 3 lines
Quoted 1 line
+
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 anynetdev_debugavailable. Do you know the reason for that? Is there a replacement besidespr_debug?
netdev_dbg
Quoted 5 lines
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_devicestruct. 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
Greg Kroah-Hartman
maintainerRe: [PATCHv3] staging: wilc1000: use netdev_* instead of printk
In reply to Raphaël BeamonteQuoted 1 line · Raphaël Beamonte
Signed-off-by Raphaël Beamonte <[email protected]>
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
PATCH v2 3/5 staging: wilc1000: remove DECLARE_WILC_BUFFER()
In reply to Raphaël BeamonteIt was just a wrapper to initialize a variable. Initialize it directly instead.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +3 -6
@@ -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 ) {
Signature · 1 line
2.1.4
PATCH v2 4/5 staging: wilc1000: remove FREE_WILC_BUFFER()
In reply to Raphaël BeamonteIt was just a wrapper around kfree(), so call that instead.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +3 -6
@@ -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" );
Signature · 1 line
2.1.4
Greg Kroah-Hartman
maintainerRe: [PATCHv2 4/5] staging: wilc1000: remove FREE_WILC_BUFFER()
Quoted 1 line · Raphaël Beamonte
It was just a wrapper around
kfree(), so call that instead.
Someone else already did this, sorry.
greg k-h
PATCH v2 5/5 staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
In reply to Raphaël BeamonteThe 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.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 37 ++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 10 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +27 -10
@@ -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)
Signature · 1 line
2.1.4
Re: [PATCHv2 5/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
Quoted 1 line · Raphaël Beamonte
The
MACRO_WILC_BUFFER()macro was using a return statement, and didn't
Probable MACRO_WILC_BUFFER should be MALLOC_WILC_BUFFER here.
Quoted 27 lines
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 <[email protected]>--- drivers/staging/wilc1000/wilc_exported_buf.c | 37 ++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 10 deletions(-)drivers/staging/wilc1000/wilc_exported_buf.c +0
-7@@ -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 */
drivers/staging/wilc1000/wilc_exported_buf.c static int __init wilc_module_init(void) · line 43
Quoted 9 lines
drivers/staging/wilc1000/wilc_exported_buf.c +3
-3@@ -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
Quoted 30 lines
drivers/staging/wilc1000/wilc_exported_buf.c +24
-0+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)
PATCH v3 1/1 staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
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.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 31 +++++++++++++++++++--------- 1 file changed, 21 insertions(+), 10 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +21 -10
@@ -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)
Signature · 1 line
2.1.4
Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
Quoted 6 lines · 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 <[email protected]>---
Yes. This looks good now.
regards,
dan carpenter
Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
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
2015-08-18 5:15 GMT-04:00 Dan Carpenter <[email protected]>:
Quoted 2 lines
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
Greg Kroah-Hartman
maintainerRe: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
Quoted 11 lines · Raphaël Beamonte
2015-08-18 5:15 GMT-04:00 Dan Carpenter <[email protected]>:
Quoted 2 lines
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 thenet_deviceto 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
v4 cover staging: wilc1000: code improvements
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(-)
Signature · 1 line
2.1.4
PATCH v4 1/2 staging: wilc1000: remove FREE_WILC_BUFFER()
It was just a wrapper around kfree(), so call that instead.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +3 -6
@@ -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" );
Signature · 1 line
2.1.4
Greg Kroah-Hartman
maintainerRe: [PATCHv4 1/2] staging: wilc1000: remove FREE_WILC_BUFFER()
Quoted 6 lines · Raphaël Beamonte
It was just a wrapper around
kfree(), so call that instead.Signed-off-by Raphaël Beamonte <[email protected]>--- 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()
2015-09-02 21:19 GMT-04:00 Greg Kroah-Hartman <[email protected]>:
Quoted 1 line
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()
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 <[email protected]>:
Quoted 18 lines
2015-09-02 21:19 GMT-04:00 Greg Kroah-Hartman <[email protected]>:
Quoted 1 line
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 theKconfighas 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!
PATCH v4 2/2 staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
In reply to Raphaël BeamonteThe 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.
--- drivers/staging/wilc1000/wilc_exported_buf.c | 31 +++++++++++++++++++--------- 1 file changed, 21 insertions(+), 10 deletions(-)
drivers/staging/wilc1000/wilc_exported_buf.c +21 -10
@@ -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)
Signature · 1 line
2.1.4
Re: [PATCHv2 5/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak
In reply to Arend van Spriel2015-08-17 15:41 GMT-04:00 Arend van Spriel <[email protected]>:
Quoted 1 line
Probable
MACRO_WILC_BUFFERshould beMALLOC_WILC_BUFFERhere.
Good catch!
Quoted 2 lines
There is really no need to print an error message here. kmalloc will blurb enough info when it fails.
Ok!
Quoted 2 lines
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.
PATCH 2/3 staging: wilc1000: code style: fix globals initialized to false
In reply to Raphaël BeamonteGlobals should not be initialized to 0 or NULL.
--- drivers/staging/wilc1000/host_interface.c | 4 ++-- drivers/staging/wilc1000/wilc_wlan.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/staging/wilc1000/host_interface.c +2 -2
@@ -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;
drivers/staging/wilc1000/wilc_wlan.c +1 -1
@@ -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) {
Signature · 1 line
2.1.4
PATCH 3/3 staging: wilc1000: code style: fix open brace { on wrong line
In reply to Raphaël BeamonteOpen braces should be on the same line as if and for statements.
--- drivers/staging/wilc1000/linux_wlan.c | 3 +-- drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
drivers/staging/wilc1000/linux_wlan.c +1 -2
@@ -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 ]);
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +1 -2
@@ -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;
Signature · 1 line
2.1.4