proc fs entries need to be removed when module is removed, otherwise
when we try to insert the module again, kernel will complain
[ 493.068012] proc_dir_entry 'net/ieee80211' already registered
[ 493.271973] proc_mkdir+0x18/0x20
[ 493.272136] ieee80211_debug_init+0x28/0xde8 [r8192u_usb]
[ 493.272404] rtl8192_usb_module_init+0x10/0x161 [r8192u_usb]
[ 13.910616] proc_dir_entry 'net/rtl819xU' already registered
[ 13.918931] proc_mkdir+0x18/0x20
[ 13.919098] rtl8192_usb_module_init+0x142/0x16d [r8192u_usb]
Signed-off-by: Tong Zhang <[email protected]>
---
drivers/staging/rtl8192u/r8192U_core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
index 364e1ca94f70..683afdc667bc 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -4825,6 +4825,11 @@ static int __init rtl8192_usb_module_init(void)
static void __exit rtl8192_usb_module_exit(void)
{
usb_deregister(&rtl8192_usb_driver);
+ remove_proc_entry(RTL819XU_MODULE_NAME, init_net.proc_net);
+
+#ifdef CONFIG_IEEE80211_DEBUG
+ ieee80211_debug_exit();
+#endif
RT_TRACE(COMP_DOWN, "Exiting");
}
--
2.25.1
On Sun, Feb 20, 2022 at 03:15:53PM -0800, Tong Zhang wrote:
> proc fs entries need to be removed when module is removed, otherwise
> when we try to insert the module again, kernel will complain
>
> [ 493.068012] proc_dir_entry 'net/ieee80211' already registered
> [ 493.271973] proc_mkdir+0x18/0x20
> [ 493.272136] ieee80211_debug_init+0x28/0xde8 [r8192u_usb]
> [ 493.272404] rtl8192_usb_module_init+0x10/0x161 [r8192u_usb]
>
> [ 13.910616] proc_dir_entry 'net/rtl819xU' already registered
> [ 13.918931] proc_mkdir+0x18/0x20
> [ 13.919098] rtl8192_usb_module_init+0x142/0x16d [r8192u_usb]
>
> Signed-off-by: Tong Zhang <[email protected]>
> ---
This is a partial fix but there is a lot wrong with both the init() and
exit() function. It's not hard to just fix everything and it saves
time.
Here is how to write Free the Last thing style error handling for init()
and when you finish writing the error handling code then the exit()
function is just a matter of cut and paste.
The rules are: 1) Free the last successful allocation. 2) Every
function must have a matching release function. 3) Every function must
clean up after itself. No partial allocations. 4) Name your labels
with descriptive names to say what the goto does.
ret = ieee80211_debug_init();
if (ret) {
Here nothing has been allocated. Nothing to clean up. Just return an
error code.
pr_err("ieee80211_debug_init() failed %d\n", ret);
return ret;
}
ret = ieee80211_crypto_init();
if (ret) {
The last successful allocation was ieee80211_debug_init(). So we need
to goto debug_exit;
pr_err("ieee80211_crypto_init() failed %d\n", ret);
goto debug_exit;
}
ret = ieee80211_crypto_tkip_init();
if (ret) {
The last successful allocation was ieee80211_crypto_init() so we need to
goto crypto_exit;
pr_err("ieee80211_crypto_tkip_init() failed %d\n", ret);
goto crypto_exit;
}
Etc. At the end of the function it prints the pr_info() even though
usb_register(&rtl8192_usb_driver); can fail. It needs to do:
ret = usb_register(&rtl8192_usb_driver);
if (ret)
goto crypto_wep_exit;
pr_info("\nLinux kernel driver for RTL8192 based WLAN cards\n");
pr_info("Copyright (c) 2007-2008, Realsil Wlan\n");
return 0;
crypto_wep_exit:
ieee80211_crypto_wep_exit();
other_stuff:
free_other_stuff();
crypto_exit:
ieee80211_crypto_deinit();
debug_exit:
ieee80211_debug_exit();
return ret;
}
Now you copy and paste the cleanup block, delete the labels, and add a
usb_deregister() to create the rtl8192_usb_module_exit() function.
static void __exit rtl8192_usb_module_exit(void)
{
usb_deregister(&rtl8192_usb_driver);
ieee80211_crypto_wep_exit();
free_other_stuff();
ieee80211_crypto_deinit();
ieee80211_debug_exit();
}
Remember to replace "free other stuff" with a the correct code. ;)
regards,
dan carpenter
On Mon, Feb 21, 2022 at 04:00:06PM +0300, Dan Carpenter wrote:
> On Sun, Feb 20, 2022 at 03:15:53PM -0800, Tong Zhang wrote:
> > proc fs entries need to be removed when module is removed, otherwise
> > when we try to insert the module again, kernel will complain
> >
> > [ 493.068012] proc_dir_entry 'net/ieee80211' already registered
> > [ 493.271973] proc_mkdir+0x18/0x20
> > [ 493.272136] ieee80211_debug_init+0x28/0xde8 [r8192u_usb]
> > [ 493.272404] rtl8192_usb_module_init+0x10/0x161 [r8192u_usb]
> >
> > [ 13.910616] proc_dir_entry 'net/rtl819xU' already registered
> > [ 13.918931] proc_mkdir+0x18/0x20
> > [ 13.919098] rtl8192_usb_module_init+0x142/0x16d [r8192u_usb]
> >
> > Signed-off-by: Tong Zhang <[email protected]>
> > ---
>
> This is a partial fix but there is a lot wrong with both the init() and
> exit() function. It's not hard to just fix everything and it saves
> time.
>
> Here is how to write Free the Last thing style error handling for init()
> and when you finish writing the error handling code then the exit()
> function is just a matter of cut and paste.
>
> The rules are: 1) Free the last successful allocation. 2) Every
> function must have a matching release function. 3) Every function must
> clean up after itself. No partial allocations. 4) Name your labels
> with descriptive names to say what the goto does.
I meant to add:
5) Free things in reverse order from how they were allocated.
regards,
dan carpenter
On Sun, Feb 20, 2022 at 03:15:53PM -0800, Tong Zhang wrote:
> proc fs entries need to be removed when module is removed, otherwise
> when we try to insert the module again, kernel will complain
>
> [ 493.068012] proc_dir_entry 'net/ieee80211' already registered
> [ 493.271973] proc_mkdir+0x18/0x20
> [ 493.272136] ieee80211_debug_init+0x28/0xde8 [r8192u_usb]
> [ 493.272404] rtl8192_usb_module_init+0x10/0x161 [r8192u_usb]
>
> [ 13.910616] proc_dir_entry 'net/rtl819xU' already registered
> [ 13.918931] proc_mkdir+0x18/0x20
> [ 13.919098] rtl8192_usb_module_init+0x142/0x16d [r8192u_usb]
>
> Signed-off-by: Tong Zhang <[email protected]>
> ---
> drivers/staging/rtl8192u/r8192U_core.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index 364e1ca94f70..683afdc667bc 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -4825,6 +4825,11 @@ static int __init rtl8192_usb_module_init(void)
> static void __exit rtl8192_usb_module_exit(void)
> {
> usb_deregister(&rtl8192_usb_driver);
> + remove_proc_entry(RTL819XU_MODULE_NAME, init_net.proc_net);
> +
> +#ifdef CONFIG_IEEE80211_DEBUG
> + ieee80211_debug_exit();
> +#endif
Please do not put #ifdef in .c files. They should be in a .h file
instead.
thanks,
greg k-h
Thank you, Dan and Greg!
I will revise as suggested and send v2.
- Tong
The init and exit functions are not releasing resource properly. An error
can be observed when we load/unload/load r8192u_usb module due to this
issue. This patch rework init and exit functions to do proper resource
release on init error and module unload.
The __exit attribute is stripped from some functions since they are now
being used by module init functions.
[ 493.068012] proc_dir_entry 'net/ieee80211' already registered
[ 493.271973] proc_mkdir+0x18/0x20
[ 493.272136] ieee80211_debug_init+0x28/0xde8 [r8192u_usb]
[ 493.272404] rtl8192_usb_module_init+0x10/0x161 [r8192u_usb]
[ 13.910616] proc_dir_entry 'net/rtl819xU' already registered
[ 13.918931] proc_mkdir+0x18/0x20
[ 13.919098] rtl8192_usb_module_init+0x142/0x16d [r8192u_usb]
Signed-off-by: Tong Zhang <[email protected]>
---
.../rtl8192u/ieee80211/ieee80211_crypt.c | 2 +-
.../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 2 +-
.../rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 2 +-
.../rtl8192u/ieee80211/ieee80211_crypt_wep.c | 2 +-
.../rtl8192u/ieee80211/ieee80211_module.c | 2 +-
drivers/staging/rtl8192u/r8192U_core.c | 45 ++++++++++++++-----
6 files changed, 38 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt.c
index 01012dddcd73..840db6250b87 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt.c
@@ -214,7 +214,7 @@ int __init ieee80211_crypto_init(void)
return ret;
}
-void __exit ieee80211_crypto_deinit(void)
+void ieee80211_crypto_deinit(void)
{
struct list_head *ptr, *n;
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c
index ccff385cf1f8..101c28265e91 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c
@@ -415,7 +415,7 @@ int __init ieee80211_crypto_ccmp_init(void)
return ieee80211_register_crypto_ops(&ieee80211_crypt_ccmp);
}
-void __exit ieee80211_crypto_ccmp_exit(void)
+void ieee80211_crypto_ccmp_exit(void)
{
ieee80211_unregister_crypto_ops(&ieee80211_crypt_ccmp);
}
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
index e8fa1d385f24..689d8843f538 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c
@@ -712,7 +712,7 @@ int __init ieee80211_crypto_tkip_init(void)
return ieee80211_register_crypto_ops(&ieee80211_crypt_tkip);
}
-void __exit ieee80211_crypto_tkip_exit(void)
+void ieee80211_crypto_tkip_exit(void)
{
ieee80211_unregister_crypto_ops(&ieee80211_crypt_tkip);
}
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
index a41b6510481b..8a51ea1dd6e5 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
@@ -240,7 +240,7 @@ int __init ieee80211_crypto_wep_init(void)
return ieee80211_register_crypto_ops(&ieee80211_crypt_wep);
}
-void __exit ieee80211_crypto_wep_exit(void)
+void ieee80211_crypto_wep_exit(void)
{
ieee80211_unregister_crypto_ops(&ieee80211_crypt_wep);
}
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c
index c52540b734fd..b94fe9b449b6 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c
@@ -276,7 +276,7 @@ int __init ieee80211_debug_init(void)
return 0;
}
-void __exit ieee80211_debug_exit(void)
+void ieee80211_debug_exit(void)
{
if (ieee80211_proc) {
remove_proc_entry("debug_level", ieee80211_proc);
diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
index 364e1ca94f70..ce807c9d4219 100644
--- a/drivers/staging/rtl8192u/r8192U_core.c
+++ b/drivers/staging/rtl8192u/r8192U_core.c
@@ -4783,49 +4783,70 @@ static int __init rtl8192_usb_module_init(void)
{
int ret;
-#ifdef CONFIG_IEEE80211_DEBUG
+ pr_info("\nLinux kernel driver for RTL8192 based WLAN cards\n");
+ pr_info("Copyright (c) 2007-2008, Realsil Wlan\n");
+ RT_TRACE(COMP_INIT, "Initializing module");
+ RT_TRACE(COMP_INIT, "Wireless extensions version %d", WIRELESS_EXT);
+
ret = ieee80211_debug_init();
if (ret) {
pr_err("ieee80211_debug_init() failed %d\n", ret);
return ret;
}
-#endif
+
ret = ieee80211_crypto_init();
if (ret) {
pr_err("ieee80211_crypto_init() failed %d\n", ret);
- return ret;
+ goto debug_exit;
}
ret = ieee80211_crypto_tkip_init();
if (ret) {
pr_err("ieee80211_crypto_tkip_init() failed %d\n", ret);
- return ret;
+ goto crypto_exit;
}
ret = ieee80211_crypto_ccmp_init();
if (ret) {
pr_err("ieee80211_crypto_ccmp_init() failed %d\n", ret);
- return ret;
+ goto crypto_tkip_exit;
}
ret = ieee80211_crypto_wep_init();
if (ret) {
pr_err("ieee80211_crypto_wep_init() failed %d\n", ret);
- return ret;
+ goto crypto_ccmp_exit;
}
- pr_info("\nLinux kernel driver for RTL8192 based WLAN cards\n");
- pr_info("Copyright (c) 2007-2008, Realsil Wlan\n");
- RT_TRACE(COMP_INIT, "Initializing module");
- RT_TRACE(COMP_INIT, "Wireless extensions version %d", WIRELESS_EXT);
rtl8192_proc_module_init();
- return usb_register(&rtl8192_usb_driver);
+ ret = usb_register(&rtl8192_usb_driver);
+ if (ret)
+ goto rtl8192_proc_module_exit;
+ return ret;
+
+rtl8192_proc_module_exit:
+ remove_proc_entry(RTL819XU_MODULE_NAME, init_net.proc_net);
+ ieee80211_crypto_wep_exit();
+crypto_ccmp_exit:
+ ieee80211_crypto_ccmp_exit();
+crypto_tkip_exit:
+ ieee80211_crypto_tkip_exit();
+crypto_exit:
+ ieee80211_crypto_deinit();
+debug_exit:
+ ieee80211_debug_exit();
+ return ret;
}
static void __exit rtl8192_usb_module_exit(void)
{
usb_deregister(&rtl8192_usb_driver);
-
+ remove_proc_entry(RTL819XU_MODULE_NAME, init_net.proc_net);
+ ieee80211_crypto_wep_exit();
+ ieee80211_crypto_ccmp_exit();
+ ieee80211_crypto_tkip_exit();
+ ieee80211_crypto_deinit();
+ ieee80211_debug_exit();
RT_TRACE(COMP_DOWN, "Exiting");
}
--
2.25.1
There is an extra space in the debug macro, when CONFIG_IEEE80211_DEBUG
is switched off, compiler will complain.
drivers/staging/rtl8192u/ieee80211/ieee80211.h:470:42: error: expected ‘)’ before ‘...’ token
470 | #define IEEE80211_DEBUG (level, fmt, args...) do {} while (0)
drivers/staging/rtl8192u/ieee80211/ieee80211.h:470:47: error: expected ‘;’ before ‘do’
470 | #define IEEE80211_DEBUG (level, fmt, args...) do {} while (0)
Signed-off-by: Tong Zhang <[email protected]>
---
drivers/staging/rtl8192u/ieee80211/ieee80211.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
index 15207dc1f5c5..ab1e380688ee 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
@@ -467,8 +467,8 @@ do { if (ieee80211_debug_level & (level)) \
} \
} while (0)
#else
-#define IEEE80211_DEBUG (level, fmt, args...) do {} while (0)
-#define IEEE80211_DEBUG_DATA (level, data, datalen) do {} while (0)
+#define IEEE80211_DEBUG(level, fmt, args...)
+#define IEEE80211_DEBUG_DATA(level, data, datalen)
#endif /* CONFIG_IEEE80211_DEBUG */
/* debug macros not dependent on CONFIG_IEEE80211_DEBUG */
--
2.25.1
On Wed, Feb 23, 2022 at 10:40:31PM -0800, Tong Zhang wrote:
> The init and exit functions are not releasing resource properly. An error
> can be observed when we load/unload/load r8192u_usb module due to this
> issue. This patch rework init and exit functions to do proper resource
> release on init error and module unload.
> The __exit attribute is stripped from some functions since they are now
> being used by module init functions.
>
> [ 493.068012] proc_dir_entry 'net/ieee80211' already registered
> [ 493.271973] proc_mkdir+0x18/0x20
> [ 493.272136] ieee80211_debug_init+0x28/0xde8 [r8192u_usb]
> [ 493.272404] rtl8192_usb_module_init+0x10/0x161 [r8192u_usb]
>
> [ 13.910616] proc_dir_entry 'net/rtl819xU' already registered
> [ 13.918931] proc_mkdir+0x18/0x20
> [ 13.919098] rtl8192_usb_module_init+0x142/0x16d [r8192u_usb]
>
> Signed-off-by: Tong Zhang <[email protected]>
Good! Thanks!
Reviewed-by: Dan Carpenter <[email protected]>
regards,
dan carpenter
Add two empty functions to handle the case when CONFIG_IEEE80211_DEBUG
is turned off. These two functions will be used by module init() and
and exit().
Signed-off-by: Tong Zhang <[email protected]>
---
drivers/staging/rtl8192u/ieee80211/ieee80211.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
index ab1e380688ee..68c0bf9a191a 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
@@ -2315,8 +2315,13 @@ int ieee80211_wx_get_freq(struct ieee80211_device *ieee,
union iwreq_data *wrqu, char *b);
/* ieee80211_module.c */
+#ifdef CONFIG_IEEE80211_DEBUG
int ieee80211_debug_init(void);
void ieee80211_debug_exit(void);
+#else
+static inline int ieee80211_debug_init(void) { return 0; }
+static inline void ieee80211_debug_exit(void) { }
+#endif
//extern void ieee80211_wx_sync_scan_wq(struct ieee80211_device *ieee);
void ieee80211_wx_sync_scan_wq(struct work_struct *work);
--
2.25.1