[PATCH] Staging: Android: logger: module_exit implementation

[PATCH] Staging: Android: logger: module_exit implementation

[Luca Clementi]

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.

Signed-off-by: Luca Clementi <luca.clementi@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Brian Swetland <swetland@google.com>
---
 drivers/staging/android/logger.c |   30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 1d5ed47..050be01 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -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(&current_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, <swetland@google.com>");
+MODULE_DESCRIPTION("Android Logger");
+
+
-- 
1.7.9.5

Re: [PATCH] Staging: Android: logger: module_exit implementationg

[Greg Kroah-Hartman]

On Thu, Nov 01, 2012 at 11:15:52PM -0700, Luca Clementi wrote:
> 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.

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?

> Signed-off-by: Luca Clementi <luca.clementi@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Brian Swetland <swetland@google.com>
> ---
>  drivers/staging/android/logger.c |   30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 1d5ed47..050be01 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -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(&current_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?

> +		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?

> +module_exit(logger_exit);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Brian Swetland, <swetland@google.com>");
> +MODULE_DESCRIPTION("Android Logger");
> +
> +

What's with the unneeded trailing empty lines?

thanks,

greg k-h

Re: [PATCH] Staging: Android: logger: module_exit implementationg

[Brian Swetland]

On Fri, Nov 2, 2012 at 11:29 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 01, 2012 at 11:15:52PM -0700, Luca Clementi wrote:
>> +
>> +
>> +module_init(logger_init);
>
> Is module_init() the same "level" as device_initcall()?  Did you test
> this out in an Android system?
>
>> +module_exit(logger_exit);
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Brian Swetland, <swetland@google.com>");
>> +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

[Luca Clementi]

On Fri, Nov 2, 2012 at 11:29 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 01, 2012 at 11:15:52PM -0700, Luca Clementi wrote:
>> 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.
>
> 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.

> 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.

> 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.

>> Signed-off-by: Luca Clementi <luca.clementi@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Brian Swetland <swetland@google.com>
>> ---
>>  drivers/staging/android/logger.c |   30 +++++++++++++++++++++++++++++-
>>  1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
>> index 1d5ed47..050be01 100644
>> --- a/drivers/staging/android/logger.c
>> +++ b/drivers/staging/android/logger.c
>> @@ -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(&current_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.

>> +             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?

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.

>> +module_exit(logger_exit);
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Brian Swetland, <swetland@google.com>");
>> +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 implementation

[Ryan Mallon]

On 02/11/12 17:15, Luca Clementi wrote:
> 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.
> 
> Signed-off-by: Luca Clementi <luca.clementi@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Brian Swetland <swetland@google.com>
> ---
>  drivers/staging/android/logger.c |   30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 1d5ed47..050be01 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -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(&current_log->misc);

ret is undeclared? Has this been tested?

> +		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.

> +		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.

> +		vfree(current_log->buffer);
> +		kfree(current_log->misc.name);
> +		kfree(current_log);

Missing:

	list_del(&current_log->logs);

?

> +	}
> +
> +	return;

This is pointless.

> +}
> +
> +
> +module_init(logger_init);
> +module_exit(logger_exit);

Nitpick - Blank line here.

> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Brian Swetland, <swetland@google.com>");

Should be Robert Love according to the header comment.

> +MODULE_DESCRIPTION("Android Logger");
> +
> +

Don't need extra blank lines at the end of the file.

~Ryan

Re: [PATCH] Staging: Android: logger: module_exit implementationg

[Ryan Mallon]

On 04/11/12 04:45, Luca Clementi wrote:
> On Fri, Nov 2, 2012 at 11:29 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Thu, Nov 01, 2012 at 11:15:52PM -0700, Luca Clementi wrote:

<snip>

>>> +             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?
> 
> 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.

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

[PATCH] Staging: Android: logger: module_exit implementation

[Luca Clementi]

This patch creates the module_exit for the android logger
so that it can be loaded and unloaded as a module.

The android logger is already declared as a tristate in the
Kconfig but the module_exit function was missing.

device_initcall works also with modprobe since include/linux/init.h:

 #define module_init(x)  __initcall(x);
 ...
 #define __initcall(fn) device_initcall(fn)

Tested against f4a75d2eb7b1e2206094b901be09adb31ba63681 Linux 3.7-rc6

Signed-off-by: Luca Clementi <luca.clementi@gmail.com>
---
 drivers/staging/android/logger.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 1d5ed47..dbc63cb 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -676,4 +676,25 @@ static int __init logger_init(void)
 out:
 	return ret;
 }
+
+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 */
+		misc_deregister(&current_log->misc);
+		vfree(current_log->buffer);
+		kfree(current_log->misc.name);
+		list_del(&current_log->logs);
+		kfree(current_log);
+	}
+}
+
+
 device_initcall(logger_init);
+module_exit(logger_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Robert Love, <rlove@google.com>");
+MODULE_DESCRIPTION("Android Logger");
-- 
1.7.9.5