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.
Change from a fixed set of static log structures, to allocation
at init time into a list.
Signed-off-by: Tim Bird <[email protected]>
---
drivers/staging/android/logger.c | 107 ++++++++++++++++++++++----------------
1 file changed, 62 insertions(+), 45 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index ea69b6a..9ebcf42 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>
@@ -45,8 +46,12 @@ struct logger_log {
size_t w_off; /* current write head offset */
size_t head; /* new readers start here */
size_t size; /* size of the log */
+ struct list_head logs; /* list of log channels (myself)*/
};
+static LIST_HEAD(log_list);
+
+
/*
* struct logger_reader - a logging device open for reading
*
@@ -408,7 +413,15 @@ ssize_t logger_aio_write(struct kiocb *iocb, const struct iovec *iov,
return ret;
}
-static struct logger_log *get_log_from_minor(int);
+static struct logger_log *get_log_from_minor(int minor)
+{
+ struct logger_log *log;
+
+ list_for_each_entry(log, &log_list, logs)
+ if (log->misc.minor == minor)
+ return log;
+ return NULL;
+}
/*
* logger_open - the log's open() file operation
@@ -565,80 +578,84 @@ static const struct file_operations logger_fops = {
};
/*
- * 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.
+ * Log size 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, \
-};
+static int __init create_log(char *log_name, int size)
+{
+ int ret = 0;
+ struct logger_log *log;
+ unsigned char *buffer;
-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)
+ buffer = vmalloc(size);
+ if (buffer == NULL)
+ return -1;
-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;
- return NULL;
-}
+ log = kzalloc(sizeof(struct logger_log), GFP_KERNEL);
+ if (log == NULL) {
+ ret = -1;
+ goto out_free_buffer;
+ }
+ log->buffer = buffer;
-static int __init init_log(struct logger_log *log)
-{
- int ret;
+ 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;
+
+ INIT_LIST_HEAD(&log->logs);
+ list_add_tail(&log->logs, &log_list);
+
+ /* 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 Thu, May 10, 2012 at 02:16:31PM -0700, Tim Bird wrote:
> + 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;
Please return a "real" error number, not just -1. You do this a few
times in this function, -EINVAL perhaps? -1 is -EPERM, which is not
what you want to be saying here, right?
thanks,
greg k-h
Fix a few sparse warnings, and improve whitespace.
Signed-off-by: Tim Bird <[email protected]>
---
drivers/staging/android/logger.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 9ebcf42..3e8865f 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -65,9 +65,9 @@ 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);
+ return n & (log->size - 1);
}
@@ -353,7 +353,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);
--
1.7.9.5
On 05/10/2012 02:21 PM, Greg KH wrote:
> On Thu, May 10, 2012 at 02:16:31PM -0700, Tim Bird wrote:
>> + 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;
>
> Please return a "real" error number, not just -1. You do this a few
> times in this function, -EINVAL perhaps? -1 is -EPERM, which is not
> what you want to be saying here, right?
You're right. This is only called by init code now, which doesn't
look at the return value (only checks for non-zero), but the intent
is to change this going forward, so a real error number is called for.
These are all memory allocation failures, so I'm thinking of using
-ENOMEM. Would that be right?
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================
On Thu, May 10, 2012 at 02:29:26PM -0700, Tim Bird wrote:
> On 05/10/2012 02:21 PM, Greg KH wrote:
> > On Thu, May 10, 2012 at 02:16:31PM -0700, Tim Bird wrote:
> >> + 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;
> >
> > Please return a "real" error number, not just -1. You do this a few
> > times in this function, -EINVAL perhaps? -1 is -EPERM, which is not
> > what you want to be saying here, right?
>
> You're right. This is only called by init code now, which doesn't
> look at the return value (only checks for non-zero), but the intent
> is to change this going forward, so a real error number is called for.
>
> These are all memory allocation failures, so I'm thinking of using
> -ENOMEM. Would that be right?
Yes, sorry, I missed that, -ENOMEM is correct
I was looking at the place where the lookup failed to find the log
specified, you should return a proper error for that as well.
thanks,
greg k-h
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.
Change from a fixed set of static log structures, to allocation
at init time into a list. Return proper error numbers on log
allocation failure.
Signed-off-by: Tim Bird <[email protected]>
---
drivers/staging/android/logger.c | 107 ++++++++++++++++++++++----------------
1 file changed, 62 insertions(+), 45 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index ea69b6a..9ebcf42 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>
@@ -45,8 +46,12 @@ struct logger_log {
size_t w_off; /* current write head offset */
size_t head; /* new readers start here */
size_t size; /* size of the log */
+ struct list_head logs; /* list of log channels (myself)*/
};
+static LIST_HEAD(log_list);
+
+
/*
* struct logger_reader - a logging device open for reading
*
@@ -408,7 +413,15 @@ ssize_t logger_aio_write(struct kiocb *iocb, const struct iovec *iov,
return ret;
}
-static struct logger_log *get_log_from_minor(int);
+static struct logger_log *get_log_from_minor(int minor)
+{
+ struct logger_log *log;
+
+ list_for_each_entry(log, &log_list, logs)
+ if (log->misc.minor == minor)
+ return log;
+ return NULL;
+}
/*
* logger_open - the log's open() file operation
@@ -565,80 +578,84 @@ static const struct file_operations logger_fops = {
};
/*
- * 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.
+ * Log size 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, \
-};
+static int __init create_log(char *log_name, int size)
+{
+ int ret = 0;
+ struct logger_log *log;
+ unsigned char *buffer;
-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)
+ buffer = vmalloc(size);
+ if (buffer == NULL)
+ return -ENOMEM;
-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;
- return NULL;
-}
+ log = kzalloc(sizeof(struct logger_log), GFP_KERNEL);
+ if (log == NULL) {
+ ret = -ENOMEM;
+ goto out_free_buffer;
+ }
+ log->buffer = buffer;
-static int __init init_log(struct logger_log *log)
-{
- int ret;
+ log->misc.minor = MISC_DYNAMIC_MINOR;
+ log->misc.name = kstrdup(log_name, GFP_KERNEL);
+ if (log->misc.name == NULL) {
+ ret = -ENOMEM;
+ 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;
+
+ INIT_LIST_HEAD(&log->logs);
+ list_add_tail(&log->logs, &log_list);
+
+ /* 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