Staging: Android: logger: module_exit implementation
PATCH 1/1 Staging: Android: logger: module_exit implementation
Created the module_exit for the android logger so that it can be loaded and unloaded as a module. Fixed module_init and some other minor issues.
--- drivers/staging/android/logger.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
drivers/staging/android/logger.c +29 -1
@@ -676,4 +676,32 @@ static int __init logger_init(void) out:return ret; } -device_initcall (logger_init); + +static void __exit logger_exit(void) +{ +struct logger_log *current_log, *next_log; + +list_for_each_entry_safe (current_log, next_log, &log_list, logs) { +/* we have to delete all the entry inside log_list */ + ret =misc_deregister (¤t_log->misc); +if (unlikely (ret)) { +pr_err ("failed to deregister misc device for log '%s'!\n" , + current_log->misc.name); + } +pr_info ("removed loggger '%s'\n" , current_log->misc.name); +vfree (current_log->buffer); +kfree (current_log->misc.name); +kfree (current_log); + } + +return ; +} + + +module_init (logger_init); +module_exit (logger_exit); +MODULE_LICENSE ("GPL" ); +MODULE_AUTHOR ("Brian Swetland, <[email protected]>" ); +MODULE_DESCRIPTION ("Android Logger" ); + +
Signature · 1 line
1.7.9.5
Greg Kroah-Hartman
maintainerRe: [PATCH] Staging: Android: logger: module_exit implementationg
Quoted 3 lines · Luca Clementi
Created the
module_exitfor the android logger so that it can be loaded and unloaded as a module. Fixedmodule_initand some other minor issues.
That's doing more than one thing here at once, care to break it up? Yeah, I know it seems funny for such a small patch, but it helps.
Also, now that you've added this, the logger driver still can't be built as a module, as the build system isn't changed to let that happen, right?
Also, why do you want to build this as a module?
Quoted 19 lines
Signed-off-by Luca Clementi <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Brian Swetland <[email protected]>--- drivers/staging/android/logger.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)drivers/staging/android/logger.c +3
-1@@ -676,4 +676,32 @@ static int __init logger_init(void) out:return ret; } -device_initcall (logger_init); + +static void __exit logger_exit(void) +{
drivers/staging/android/logger.c static int __init logger_init(void) · line 691
Quoted 10 lines
drivers/staging/android/logger.c +10
-0+struct logger_log *current_log, *next_log; + +list_for_each_entry_safe (current_log, next_log, &log_list, logs) { +/* we have to delete all the entry inside log_list */ + ret =misc_deregister (¤t_log->misc); +if (unlikely (ret)) { +pr_err ("failed to deregister misc device for log '%s'!\n" , + current_log->misc.name); + } +pr_info ("removed loggger '%s'\n" , current_log->misc.name);
Is that message really needed?
drivers/staging/android/logger.c static int __init logger_init(void) · line 701
Quoted 10 lines
drivers/staging/android/logger.c +10
-0+vfree (current_log->buffer); +kfree (current_log->misc.name); +kfree (current_log); + } + +return ; +} + + +module_init (logger_init);
Is module_init() the same "level" as device_initcall()? Did you test this out in an Android system?
drivers/staging/android/logger.c static int __init logger_init(void) · line 707
Quoted 6 lines
drivers/staging/android/logger.c +6
-0+module_exit (logger_exit); +MODULE_LICENSE ("GPL" ); +MODULE_AUTHOR ("Brian Swetland, <[email protected]>" ); +MODULE_DESCRIPTION ("Android Logger" ); + +
What's with the unneeded trailing empty lines?
thanks,
greg k-h
Re: [PATCH] Staging: Android: logger: module_exit implementationg
Quoted 8 lines · Greg Kroah-Hartman
drivers/staging/android/logger.c static int __init logger_init(void) · line 701
Quoted 3 lines · Luca Clementi
drivers/staging/android/logger.c +3
-0+ + +module_init (logger_init);Is
module_init()the same "level" asdevice_initcall()? Did you test this out in an Android system?
Quoted 8 lines
drivers/staging/android/logger.c static int __init logger_init(void) · line 707
Quoted 6 lines
drivers/staging/android/logger.c +6
-0+module_exit (logger_exit); +MODULE_LICENSE ("GPL" ); +MODULE_AUTHOR ("Brian Swetland, <[email protected]>" ); +MODULE_DESCRIPTION ("Android Logger" ); + +What's with the unneeded trailing empty lines?
Also, module author should be Robert Love (cc'd), unless he'd rather not be credited, in which case "Google, Inc" or no listed author is fine.
Brian
Re: [PATCH] Staging: Android: logger: module_exit implementationg
In reply to Greg Kroah-HartmanQuoted 8 lines · Greg Kroah-Hartman
Quoted 3 lines · Luca Clementi
Created the
module_exitfor the android logger so that it can be loaded and unloaded as a module. Fixedmodule_initand some other minor issues.That's doing more than one thing here at once, care to break it up? Yeah, I know it seems funny for such a small patch, but it helps.
Sure, no problem.
Quoted 3 lines
Also, now that you've added this, the logger driver still can't be built as a module, as the build system isn't changed to let that happen, right?
The Kconfig declares this as a module, although since there is not a module_exit
it was possible to compile it as a module and load it but then it was not possible
to rmmod it.
drivers/staging/android/Kconfig:
config ANDROID_LOGGER
tristate "Android log driver"
default n
....
So the alternative is to put the ANDROID_LOGGER to bool.
Quoted 1 line
Also, why do you want to build this as a module?
Since the original developer declared it as module I thought that was the final goal,
but it was left unfinished only for time reason.
Quoted 23 lines
Quoted 23 lines
Signed-off-by Luca Clementi <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Brian Swetland <[email protected]>--- drivers/staging/android/logger.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)drivers/staging/android/logger.c +7
-1@@ -676,4 +676,32 @@ static int __init logger_init(void) out:return ret; } -device_initcall (logger_init); + +static void __exit logger_exit(void) +{ +struct logger_log *current_log, *next_log; + +list_for_each_entry_safe (current_log, next_log, &log_list, logs) { +/* we have to delete all the entry inside log_list */
Quoted 8 lines
drivers/staging/android/logger.c static int __init logger_init(void) · line 691
Quoted 6 lines
drivers/staging/android/logger.c +6
-0+ ret =misc_deregister (¤t_log->misc); +if (unlikely (ret)) { +pr_err ("failed to deregister misc device for log '%s'!\n" , + current_log->misc.name); + } +pr_info ("removed loggger '%s'\n" , current_log->misc.name);Is that message really needed?
I'll remove it.
Quoted 5 lines
Quoted 5 lines
drivers/staging/android/logger.c +5
-0+vfree (current_log->buffer); +kfree (current_log->misc.name); +kfree (current_log); + } +
Quoted 8 lines
drivers/staging/android/logger.c static int __init logger_init(void) · line 701
Quoted 5 lines
drivers/staging/android/logger.c +5
-0+return ; +} + + +module_init (logger_init);Is
module_init()the same "level" asdevice_initcall()? Did you test this out in an Android system?
Honestly I haven't tested it on Android, but in include/linux/init.h there is:
#define module_init(x) __initcall(x);
...
#define __initcall(fn) device_initcall(fn)
Which lead me to think that there is not much difference between the two call.
Quoted 9 lines
drivers/staging/android/logger.c static int __init logger_init(void) · line 707
Quoted 6 lines
drivers/staging/android/logger.c +6
-0+module_exit (logger_exit); +MODULE_LICENSE ("GPL" ); +MODULE_AUTHOR ("Brian Swetland, <[email protected]>" ); +MODULE_DESCRIPTION ("Android Logger" ); + +What's with the unneeded trailing empty lines?
I can fix them and change the author as per request of Brian.
Should I make the requested fixes or do you prefer to change the Kconfig to a bool?
Thanks for the comment,
Luca
Re: [PATCH] Staging: Android: logger: module_exit implementationg
Quoted 3 lines · Luca Clementi
Quoted 1 line · Greg Kroah-Hartman
On Thu, Nov 01, 2012 at 11:15:52PM -0700, Luca Clementi wrote:
<snip>
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
drivers/staging/android/logger.c +5
-0+vfree (current_log->buffer); +kfree (current_log->misc.name); +kfree (current_log); + } +Quoted 8 lines
drivers/staging/android/logger.c static int __init logger_init(void) · line 701
Quoted 5 lines
drivers/staging/android/logger.c +5
-0+return ; +} + + +module_init (logger_init);Is
module_init()the same "level" asdevice_initcall()? Did you test this out in an Android system?
Quoted 7 lines
Honestly I haven't tested it on Android, but in
include/linux/init.hthere is:#define
module_init(x)__initcall(x);
...
#define__initcall(fn)device_initcall(fn)Which lead me to think that there is not much difference between the two call.
The different initcalls run at different times. Often modules run with something other than module_init if there are other dependencies on the module/subsystem (see i2c core/busses for example). You would need to check why logger was using device_initcall and make sure that it works correctly (e.g. doesn't miss some early logs) in order to make this change. It should be done as a separate patch anyway to make it easier to identify any issues with it later.
~Ryan
Quoted 18 lines · Luca Clementi
Created the
module_exitfor the android logger so that it can be loaded and unloaded as a module. Fixedmodule_initand some other minor issues.Signed-off-by Luca Clementi <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Brian Swetland <[email protected]>--- drivers/staging/android/logger.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)drivers/staging/android/logger.c +0
-0@@ -676,4 +676,32 @@ static int __init logger_init(void) out:return ret;
drivers/staging/android/logger.c static int __init logger_init(void) · line 686
Quoted 10 lines
drivers/staging/android/logger.c +8
-1} -device_initcall (logger_init); + +static void __exit logger_exit(void) +{ +struct logger_log *current_log, *next_log; + +list_for_each_entry_safe (current_log, next_log, &log_list, logs) { +/* we have to delete all the entry inside log_list */ + ret =misc_deregister (¤t_log->misc);
ret is undeclared? Has this been tested?
drivers/staging/android/logger.c static int __init logger_init(void) · line 690
Quoted 4 lines
drivers/staging/android/logger.c +4
-0+if (unlikely (ret)) { +pr_err ("failed to deregister misc device for log '%s'!\n" , + current_log->misc.name); + }
Don't need braces on single line if statements, and unlikely is not needed here (it isn't a hotpath). The whole error can probably just be removed since it looks like misc_deregister already does a WARN_ON if it fails.
drivers/staging/android/logger.c static int __init logger_init(void) · line 691
Quoted 1 line
drivers/staging/android/logger.c +1
-0+pr_info ("removed loggger '%s'\n" , current_log->misc.name);
Don't spam the log with stuff like this. Either change to pr_debug, or probably just remove it.
drivers/staging/android/logger.c static int __init logger_init(void) · line 694
Quoted 3 lines
drivers/staging/android/logger.c +3
-0+vfree (current_log->buffer); +kfree (current_log->misc.name); +kfree (current_log);
Missing:
list_del(¤t_log->logs);
?
drivers/staging/android/logger.c static int __init logger_init(void) · line 697
Quoted 3 lines
drivers/staging/android/logger.c +3
-0+ } + +return ;
This is pointless.
drivers/staging/android/logger.c static int __init logger_init(void) · line 702
Quoted 5 lines
drivers/staging/android/logger.c +5
-0+} + + +module_init (logger_init); +module_exit (logger_exit);
Nitpick - Blank line here.
drivers/staging/android/logger.c static int __init logger_init(void) · line 704
Quoted 2 lines
drivers/staging/android/logger.c +2
-0+MODULE_LICENSE ("GPL" ); +MODULE_AUTHOR ("Brian Swetland, <[email protected]>" );
Should be Robert Love according to the header comment.
drivers/staging/android/logger.c static int __init logger_init(void) · line 707
Quoted 3 lines
drivers/staging/android/logger.c +3
-0+MODULE_DESCRIPTION ("Android Logger" ); + +
Don't need extra blank lines at the end of the file.
~Ryan