This changes the log initialization to be dynamic, but still
at boot time. These changes are a predecessor to implementing
runtime allocation and freeing of logs, to make the Android logger
less hard-coded.
This makes no ABI changes with user space.
Signed-off-by: Tim Bird <[email protected]>
---
drivers/staging/android/logger.c | 117 ++++++++++++++++++++++++--------------
1 file changed, 75 insertions(+), 42 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index ea69b6a..fa5fd71 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -25,6 +25,7 @@
#include <linux/poll.h>
#include <linux/slab.h>
#include <linux/time.h>
+#include <linux/vmalloc.h>
#include "logger.h"
#include <asm/ioctls.h>
@@ -564,81 +565,113 @@ static const struct file_operations logger_fops = {
.release = logger_release,
};
-/*
- * Defines a log structure with name 'NAME' and a size of 'SIZE' bytes, which
- * must be a power of two, greater than LOGGER_ENTRY_MAX_LEN, and less than
- * LONG_MAX minus LOGGER_ENTRY_MAX_LEN.
- */
-#define DEFINE_LOGGER_DEVICE(VAR, NAME, SIZE) \
-static unsigned char _buf_ ## VAR[SIZE]; \
-static struct logger_log VAR = { \
- .buffer = _buf_ ## VAR, \
- .misc = { \
- .minor = MISC_DYNAMIC_MINOR, \
- .name = NAME, \
- .fops = &logger_fops, \
- .parent = NULL, \
- }, \
- .wq = __WAIT_QUEUE_HEAD_INITIALIZER(VAR .wq), \
- .readers = LIST_HEAD_INIT(VAR .readers), \
- .mutex = __MUTEX_INITIALIZER(VAR .mutex), \
- .w_off = 0, \
- .head = 0, \
- .size = SIZE, \
-};
-
-DEFINE_LOGGER_DEVICE(log_main, LOGGER_LOG_MAIN, 256*1024)
-DEFINE_LOGGER_DEVICE(log_events, LOGGER_LOG_EVENTS, 256*1024)
-DEFINE_LOGGER_DEVICE(log_radio, LOGGER_LOG_RADIO, 256*1024)
-DEFINE_LOGGER_DEVICE(log_system, LOGGER_LOG_SYSTEM, 256*1024)
+#define MAX_LOGS 5
+struct logger_log *logs_array[MAX_LOGS];
static struct logger_log *get_log_from_minor(int minor)
{
- if (log_main.misc.minor == minor)
- return &log_main;
- if (log_events.misc.minor == minor)
- return &log_events;
- if (log_radio.misc.minor == minor)
- return &log_radio;
- if (log_system.misc.minor == minor)
- return &log_system;
+ int i;
+
+ for (i = 0; i < MAX_LOGS; i++) {
+ if (logs_array[i]->misc.minor == minor)
+ return logs_array[i];
+ }
return NULL;
}
-static int __init init_log(struct logger_log *log)
+static int __init add_log(struct logger_log *log)
{
- int ret;
+ int i;
+ for (i = 0; i < MAX_LOGS; i++) {
+ if (logs_array[i] == 0) {
+ logs_array[i] = log;
+ return 0;
+ }
+ }
+ return -1;
+}
+
+/*
+ * Log size must be a power of two, greater than LOGGER_ENTRY_MAX_LEN,
+ * and less than LONG_MAX minus LOGGER_ENTRY_MAX_LEN.
+ */
+static int __init create_log(char *log_name, int size)
+{
+ int ret = 0;
+ struct logger_log *log;
+ unsigned char *buffer;
+
+ buffer = vmalloc(size);
+ if (buffer == NULL)
+ return -1;
+
+ log = kzalloc(sizeof(struct logger_log), GFP_KERNEL);
+ if (log == NULL) {
+ ret = -1;
+ goto out_free_buffer;
+ }
+ log->buffer = buffer;
+
+ log->misc.minor = MISC_DYNAMIC_MINOR;
+ log->misc.name = kstrdup(log_name, GFP_KERNEL);
+ if (log->misc.name == NULL) {
+ ret = -1;
+ goto out_free_log;
+ }
+
+ log->misc.fops = &logger_fops;
+ log->misc.parent = NULL;
+
+ init_waitqueue_head(&log->wq);
+ INIT_LIST_HEAD(&log->readers);
+ mutex_init(&log->mutex);
+ log->w_off = 0;
+ log->head = 0;
+ log->size = size;
+
+ ret = add_log(log);
+ if (unlikely(ret))
+ goto out_free_log;
+
+ /* finally, initialize the misc device for this log */
ret = misc_register(&log->misc);
if (unlikely(ret)) {
printk(KERN_ERR "logger: failed to register misc "
"device for log '%s'!\n", log->misc.name);
- return ret;
+ goto out_free_log;
}
printk(KERN_INFO "logger: created %luK log '%s'\n",
(unsigned long) log->size >> 10, log->misc.name);
return 0;
+
+out_free_log:
+ kfree(log);
+
+out_free_buffer:
+ vfree(buffer);
+ return ret;
}
static int __init logger_init(void)
{
int ret;
- ret = init_log(&log_main);
+ ret = create_log(LOGGER_LOG_MAIN, 256*1024);
if (unlikely(ret))
goto out;
- ret = init_log(&log_events);
+ ret = create_log(LOGGER_LOG_EVENTS, 256*1024);
if (unlikely(ret))
goto out;
- ret = init_log(&log_radio);
+ ret = create_log(LOGGER_LOG_RADIO, 256*1024);
if (unlikely(ret))
goto out;
- ret = init_log(&log_system);
+ ret = create_log(LOGGER_LOG_SYSTEM, 256*1024);
if (unlikely(ret))
goto out;
--
1.7.9.5
On Fri, May 04, 2012 at 04:33:16PM -0700, Tim Bird wrote:
> +#define MAX_LOGS 5
> +struct logger_log *logs_array[MAX_LOGS];
You are going to make this a list and not a static array in the
future, right?
> -static int __init init_log(struct logger_log *log)
> +static int __init add_log(struct logger_log *log)
> {
> - int ret;
> + int i;
>
> + for (i = 0; i < MAX_LOGS; i++) {
> + if (logs_array[i] == 0) {
> + logs_array[i] = log;
> + return 0;
> + }
> + }
> + return -1;
> +}
I see you didn't run your patch through sparse :(
Care to fix up the sparse warnings and resend?
thanks,
greg k-h
On 05/04/2012 04:37 PM, Greg KH wrote:
> On Fri, May 04, 2012 at 04:33:16PM -0700, Tim Bird wrote:
>> +#define MAX_LOGS 5
>> +struct logger_log *logs_array[MAX_LOGS];
>
> You are going to make this a list and not a static array in the
> future, right?
Would that be better? The for-loop is IMHO simpler than a
list walk for finding matches. I anticipate that the size of
this array should never (famous last words) be bigger than about
20 entries, even in the dynamic-allocation-per-application
case. And that's a ways off in implementation.
I'll be happy to switch to a linked list once it looks like we're
going to have more than 5 entries. Or I can switch to a linked
list now if you think it's better form to code for the longer-term
anticipated features.
Not a big deal to me either way.
>
>> -static int __init init_log(struct logger_log *log)
>> +static int __init add_log(struct logger_log *log)
>> {
>> - int ret;
>> + int i;
>>
>> + for (i = 0; i < MAX_LOGS; i++) {
>> + if (logs_array[i] == 0) {
>> + logs_array[i] = log;
>> + return 0;
>> + }
>> + }
>> + return -1;
>> +}
>
> I see you didn't run your patch through sparse :(
Indeed - shame on me. :-(
> Care to fix up the sparse warnings and resend?
OK - this patch has 2 sparse issues, and there were
2 already in the code. Do you want me to send the
other 2 sparse fixes as an independent patch, or could
I cheat and throw them in this one?
Below is what the fixes look like. (BTW - these are
formatted for human review but not for mainline submission.)
Thanks for your help.
-- Tim
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index fa5fd71..9f4ed84 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -61,7 +61,7 @@ struct logger_reader {
};
/* logger_offset - returns index 'n' into the log via (optimized) modulus */
-size_t logger_offset(struct logger_log *log, size_t n)
+static size_t logger_offset(struct logger_log *log, size_t n)
{
return n & (log->size-1);
}
@@ -349,7 +349,7 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
* writev(), and aio_write(). Writes are our fast path, and we try to optimize
* them above all else.
*/
-ssize_t logger_aio_write(struct kiocb *iocb, const struct iovec *iov,
+static ssize_t logger_aio_write(struct kiocb *iocb, const struct iovec *iov,
unsigned long nr_segs, loff_t ppos)
{
struct logger_log *log = file_get_log(iocb->ki_filp);
@@ -566,7 +566,7 @@ static const struct file_operations logger_fops = {
};
#define MAX_LOGS 5
-struct logger_log *logs_array[MAX_LOGS];
+static struct logger_log *logs_array[MAX_LOGS];
static struct logger_log *get_log_from_minor(int minor)
{
@@ -584,7 +584,7 @@ static int __init add_log(struct logger_log *log)
int i;
for (i = 0; i < MAX_LOGS; i++) {
- if (logs_array[i] == 0) {
+ if (logs_array[i] == NULL) {
logs_array[i] = log;
return 0;
}
=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================
On Mon, May 07, 2012 at 11:50:06AM -0700, Tim Bird wrote:
> On 05/04/2012 04:37 PM, Greg KH wrote:
> > On Fri, May 04, 2012 at 04:33:16PM -0700, Tim Bird wrote:
> >> +#define MAX_LOGS 5
> >> +struct logger_log *logs_array[MAX_LOGS];
> >
> > You are going to make this a list and not a static array in the
> > future, right?
>
> Would that be better? The for-loop is IMHO simpler than a
> list walk for finding matches. I anticipate that the size of
> this array should never (famous last words) be bigger than about
> 20 entries, even in the dynamic-allocation-per-application
> case. And that's a ways off in implementation.
>
> I'll be happy to switch to a linked list once it looks like we're
> going to have more than 5 entries. Or I can switch to a linked
> list now if you think it's better form to code for the longer-term
> anticipated features.
>
> Not a big deal to me either way.
For now, it's fine, just in the future I imagine, if you are going to
continue down the patch of this type of api and interface, you will want
to make it dynamic.
> >> -static int __init init_log(struct logger_log *log)
> >> +static int __init add_log(struct logger_log *log)
> >> {
> >> - int ret;
> >> + int i;
> >>
> >> + for (i = 0; i < MAX_LOGS; i++) {
> >> + if (logs_array[i] == 0) {
> >> + logs_array[i] = log;
> >> + return 0;
> >> + }
> >> + }
> >> + return -1;
> >> +}
> >
> > I see you didn't run your patch through sparse :(
>
> Indeed - shame on me. :-(
>
> > Care to fix up the sparse warnings and resend?
> OK - this patch has 2 sparse issues, and there were
> 2 already in the code. Do you want me to send the
> other 2 sparse fixes as an independent patch, or could
> I cheat and throw them in this one?
Fix your patch first and resend it. You can send the 'fix up sparse
warnings' patch after it if you want to.
I don't want to add new patches that add new checkpatch and sparse
problems, the goal of having the code in the staging tree is that it
gets better over time, not worse :)
thanks,
greg k-h
On 05/07/2012 11:58 AM, Greg KH wrote:
> On Mon, May 07, 2012 at 11:50:06AM -0700, Tim Bird wrote:
>> On 05/04/2012 04:37 PM, Greg KH wrote:
>>> On Fri, May 04, 2012 at 04:33:16PM -0700, Tim Bird wrote:
>>>> +#define MAX_LOGS 5
>>>> +struct logger_log *logs_array[MAX_LOGS];
>>>
>>> You are going to make this a list and not a static array in the
>>> future, right?
>> Would that be better? The for-loop is IMHO simpler than a
>> list walk for finding matches. I anticipate that the size of
>> this array should never (famous last words) be bigger than about
>> 20 entries, even in the dynamic-allocation-per-application
>> case. And that's a ways off in implementation.
>>
>> I'll be happy to switch to a linked list once it looks like we're
>> going to have more than 5 entries. Or I can switch to a linked
>> list now if you think it's better form to code for the longer-term
>> anticipated features.
>>
>> Not a big deal to me either way.
>
> For now, it's fine, just in the future I imagine, if you are going to
> continue down the patch of this type of api and interface, you will want
> to make it dynamic.
I'm still working out how this might integrate into the syslog API
(and with Kay's recent work). It's unclear if this will be the
ultimate data structure holding these entries or not - or if something
different will take its place. In any event, I'll keep
this in mind if we stay with this data structure.
>
>>>> -static int __init init_log(struct logger_log *log)
>>>> +static int __init add_log(struct logger_log *log)
>>>> {
>>>> - int ret;
>>>> + int i;
>>>>
>>>> + for (i = 0; i < MAX_LOGS; i++) {
>>>> + if (logs_array[i] == 0) {
>>>> + logs_array[i] = log;
>>>> + return 0;
>>>> + }
>>>> + }
>>>> + return -1;
>>>> +}
>>>
>>> I see you didn't run your patch through sparse :(
>>
>> Indeed - shame on me. :-(
>>
>>> Care to fix up the sparse warnings and resend?
>> OK - this patch has 2 sparse issues, and there were
>> 2 already in the code. Do you want me to send the
>> other 2 sparse fixes as an independent patch, or could
>> I cheat and throw them in this one?
>
> Fix your patch first and resend it. You can send the 'fix up sparse
> warnings' patch after it if you want to.
OK - will do.
> I don't want to add new patches that add new checkpatch and sparse
> problems, the goal of having the code in the staging tree is that it
> gets better over time, not worse :)
hehe. Yes. :-)
BTW, I have a whitespace cleanup for one of my previous patches. I'm unsure
if I should send this in, or wait until the code in question is affected
by another patch. I thought the standard kernel policy was to not do
standalone whitespace cleanup patches (especially trivial ones). But
is that the same policy for staging?
I'm happy to submit something now, or wait for a more appropriate time
in the future, and just watch for the right opportunity.
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================
On Mon, May 07, 2012 at 12:14:04PM -0700, Tim Bird wrote:
> BTW, I have a whitespace cleanup for one of my previous patches. I'm unsure
> if I should send this in, or wait until the code in question is affected
> by another patch. I thought the standard kernel policy was to not do
> standalone whitespace cleanup patches (especially trivial ones). But
> is that the same policy for staging?
No, the policy is to do stand-alone whitespace cleanup patches,
especially trivial ones :)
Well, for portions of the kernel I maintain at least, I don't mind them
at all, send them on when you have them.
thanks,
greg k-h
On Mon, May 7, 2012 at 11:50 AM, Tim Bird <[email protected]> wrote:
> On 05/04/2012 04:37 PM, Greg KH wrote:
>> On Fri, May 04, 2012 at 04:33:16PM -0700, Tim Bird wrote:
>>> +#define MAX_LOGS 5
>>> +struct logger_log *logs_array[MAX_LOGS];
>>
>> You are going to make this a list and not a static array in the
>> future, right?
>
> Would that be better? The for-loop is IMHO simpler than a
> list walk for finding matches. I anticipate that the size of
> this array should never (famous last words) be bigger than about
> 20 entries, even in the dynamic-allocation-per-application
> case. And that's a ways off in implementation.
>
> I'll be happy to switch to a linked list once it looks like we're
> going to have more than 5 entries. Or I can switch to a linked
> list now if you think it's better form to code for the longer-term
> anticipated features.
>
> Not a big deal to me either way.
I'd also favor a list approach. It won't break unexpectedly when
somebody inevitably adds that 6th log or whatever, and the list
traversal cost only occurs during open(), which is not a frequent
operation -- most users keep the log driver open for the process
lifespan (both readers and writers).
Brian
On 05/07/2012 03:13 PM, Brian Swetland wrote:
> On Mon, May 7, 2012 at 11:50 AM, Tim Bird <[email protected]> wrote:
>> On 05/04/2012 04:37 PM, Greg KH wrote:
>>> On Fri, May 04, 2012 at 04:33:16PM -0700, Tim Bird wrote:
>>>> +#define MAX_LOGS 5
>>>> +struct logger_log *logs_array[MAX_LOGS];
>>>
>>> You are going to make this a list and not a static array in the
>>> future, right?
>>
>> Would that be better? The for-loop is IMHO simpler than a
>> list walk for finding matches. I anticipate that the size of
>> this array should never (famous last words) be bigger than about
>> 20 entries, even in the dynamic-allocation-per-application
>> case. And that's a ways off in implementation.
>>
>> I'll be happy to switch to a linked list once it looks like we're
>> going to have more than 5 entries. Or I can switch to a linked
>> list now if you think it's better form to code for the longer-term
>> anticipated features.
>>
>> Not a big deal to me either way.
>
> I'd also favor a list approach. It won't break unexpectedly when
> somebody inevitably adds that 6th log or whatever, and the list
> traversal cost only occurs during open(), which is not a frequent
> operation -- most users keep the log driver open for the process
> lifespan (both readers and writers).
OK - I'll convert to a list and send an updated patch soon.
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================