Hi,
This set of patches completes more documentation in android/logger.c, as well as fixing a bug there and a comment formatting issue in android/ashmem.c.
Sorry if kernel-doc was not supposed to be applied to driver files - If it isn't, I'll be sure to remember that for next time. :)
Cruz Julian Bishop (5):
Fix comment/license formatting in android/ashmem.c
Complete documentation of logger_entry in android/logger.h
Finish documentation of two structs in android/logger.c
Redocument some functions in android/logger.c
Fixes a potential bug in android/logger.c
drivers/staging/android/ashmem.c | 32 ++++-----
drivers/staging/android/logger.c | 134 +++++++++++++++++++++++++-------------
drivers/staging/android/logger.h | 24 +++++--
3 files changed, 121 insertions(+), 69 deletions(-)
--
1.7.9.5
Signed-off-by: Cruz Julian Bishop <[email protected]>
---
drivers/staging/android/logger.c | 40 +++++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index f7b8237..1d5ed47 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -32,38 +32,50 @@
#include <asm/ioctls.h>
-/*
+/**
* struct logger_log - represents a specific log, such as 'main' or 'radio'
+ * @buffer: The actual ring buffer
+ * @misc: The "misc" device representing the log
+ * @wq: The wait queue for @readers
+ * @readers: This log's readers
+ * @mutex: The mutex that protects the @buffer
+ * @w_off: The current write head offset
+ * @head: The head, or location that readers start reading at.
+ * @size: The size of the log
+ * @logs: The list of log channels
*
* This structure lives from module insertion until module removal, so it does
* not need additional reference counting. The structure is protected by the
* mutex 'mutex'.
*/
struct logger_log {
- unsigned char *buffer;/* the ring buffer itself */
- struct miscdevice misc; /* misc device representing the log */
- wait_queue_head_t wq; /* wait queue for readers */
- struct list_head readers; /* this log's readers */
- struct mutex mutex; /* mutex protecting buffer */
- 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)*/
+ unsigned char *buffer;
+ struct miscdevice misc;
+ wait_queue_head_t wq;
+ struct list_head readers;
+ struct mutex mutex;
+ size_t w_off;
+ size_t head;
+ size_t size;
+ struct list_head logs;
};
static LIST_HEAD(log_list);
-/*
+/**
* struct logger_reader - a logging device open for reading
+ * @log: The associated log
+ * @list: The associated entry in @logger_log's list
+ * @r_off: The current read head offset.
*
* This object lives from open to release, so we don't need additional
* reference counting. The structure is protected by log->mutex.
*/
struct logger_reader {
- struct logger_log *log; /* associated log */
- struct list_head list; /* entry in logger_log's list */
- size_t r_off; /* current read head offset */
+ struct logger_log *log;
+ struct list_head list;
+ size_t r_off;
};
/* logger_offset - returns index 'n' into the log via (optimized) modulus */
--
1.7.9.5
Signed-off-by: Cruz Julian Bishop <[email protected]>
---
drivers/staging/android/ashmem.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 69cf2db..94a740d 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -1,20 +1,20 @@
/* mm/ashmem.c
-**
-** Anonymous Shared Memory Subsystem, ashmem
-**
-** Copyright (C) 2008 Google, Inc.
-**
-** Robert Love <[email protected]>
-**
-** This software is licensed under the terms of the GNU General Public
-** License version 2, as published by the Free Software Foundation, and
-** may be copied, distributed, and modified under those terms.
-**
-** This program is distributed in the hope that it will be useful,
-** but WITHOUT ANY WARRANTY; without even the implied warranty of
-** MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
-** GNU General Public License for more details.
-*/
+ *
+ * Anonymous Shared Memory Subsystem, ashmem
+ *
+ * Copyright (C) 2008 Google, Inc.
+ *
+ * Robert Love <[email protected]>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
#define pr_fmt(fmt) "ashmem: " fmt
--
1.7.9.5
Previously, there were simply comments after each part - Now, it is completed properly according to "Kernel doc"
Sorry in advance if I made any mistakes.
Signed-off-by: Cruz Julian Bishop <[email protected]>
---
drivers/staging/android/logger.h | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/logger.h b/drivers/staging/android/logger.h
index 2cb06e9..9b929a8 100644
--- a/drivers/staging/android/logger.h
+++ b/drivers/staging/android/logger.h
@@ -20,14 +20,24 @@
#include <linux/types.h>
#include <linux/ioctl.h>
+/**
+ * struct logger_entry - defines a single entry that is given to a logger
+ * @len: The length of the payload
+ * @__pad: Two bytes of padding that appear to be required
+ * @pid: The generating process' process ID
+ * @tid: The generating process' thread ID
+ * @sec: The number of seconds that have elapsed since the Epoch
+ * @nsec: The number of nanoseconds that have elapsed since @sec
+ * @msg: The message that is to be logged
+ */
struct logger_entry {
- __u16 len; /* length of the payload */
- __u16 __pad; /* no matter what, we get 2 bytes of padding */
- __s32 pid; /* generating process's pid */
- __s32 tid; /* generating process's tid */
- __s32 sec; /* seconds since Epoch */
- __s32 nsec; /* nanoseconds */
- char msg[0]; /* the entry's payload */
+ __u16 len;
+ __u16 __pad;
+ __s32 pid;
+ __s32 tid;
+ __s32 sec;
+ __s32 nsec;
+ char msg[0];
};
#define LOGGER_LOG_RADIO "log_radio" /* radio-related messages */
--
1.7.9.5
Previously, when calling is_between(a, b, c), the calculation was wrong.
It counted C as between A and B if C was equal to B, but not A.
Example of this are:
is_between(1, 10, 10) = 1 (Expected: 0)
is_between(1, 10, 1) = 0 (Expected: 0)
is_between(20, 10, 10) = 1 (Expected: 0)
And so on and so forth.
Obviously, ten is not a number between one and ten - only two to eight are, so I made this patch :)
Signed-off-by: Cruz Julian Bishop <[email protected]>
---
drivers/staging/android/logger.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 226d8b5..925df5c 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -298,11 +298,11 @@ static inline int is_between(size_t a, size_t b, size_t c)
{
if (a < b) {
/* is c between a and b? */
- if (a < c && c <= b)
+ if (a < c && c < b)
return 1;
} else {
/* is c outside of b through a? */
- if (c <= b || a < c)
+ if (c < b || a < c)
return 1;
}
--
1.7.9.5
I will document the rest later if they remain unchanged
Normally, I would do them all at once, but I don't have the chance to do them all at the moment
Signed-off-by: Cruz Julian Bishop <[email protected]>
---
drivers/staging/android/logger.c | 90 +++++++++++++++++++++++++-------------
1 file changed, 60 insertions(+), 30 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 1d5ed47..226d8b5 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -78,15 +78,20 @@ struct logger_reader {
size_t r_off;
};
-/* logger_offset - returns index 'n' into the log via (optimized) modulus */
+/**
+ * logger_offset() - returns index 'n' into the log via (optimized) modulus
+ * @log: The log being referenced
+ * @n: The index number being referenced
+ */
static size_t logger_offset(struct logger_log *log, size_t n)
{
return n & (log->size - 1);
}
-/*
- * file_get_log - Given a file structure, return the associated log
+/**
+ * file_get_log() - Given a file, return the associated log
+ * @file: The file being referenced
*
* This isn't aesthetic. We have several goals:
*
@@ -108,9 +113,11 @@ static inline struct logger_log *file_get_log(struct file *file)
return file->private_data;
}
-/*
- * get_entry_len - Grabs the length of the payload of the next entry starting
- * from 'off'.
+/**
+ * get_entry_len() - Grabs the length of the payload of the entry starting
+ * at @off
+ * @log: The log being referenced
+ * @off: The offset to start counting at
*
* An entry length is 2 bytes (16 bits) in host endian order.
* In the log, the length does not include the size of the log entry structure.
@@ -134,9 +141,13 @@ static __u32 get_entry_len(struct logger_log *log, size_t off)
return sizeof(struct logger_entry) + val;
}
-/*
- * do_read_log_to_user - reads exactly 'count' bytes from 'log' into the
- * user-space buffer 'buf'. Returns 'count' on success.
+/**
+ * do_read_log_to_user() - reads exactly @count bytes from @log into the
+ * user-space buffer @buf. Returns @count on success.
+ * @log: The log being read from
+ * @reader: The logger reader that reads from @log
+ * @buf: The user-space buffer being written into
+ * @count: The number of bytes being read
*
* Caller must hold log->mutex.
*/
@@ -169,8 +180,12 @@ static ssize_t do_read_log_to_user(struct logger_log *log,
return count;
}
-/*
- * logger_read - our log's read() method
+/**
+ * logger_read() - our log's read() method
+ * @file: The file being read from
+ * @buf: The user-space buffer being written into
+ * @count: The minimum number of bytes to be read
+ * @pos: Unused, posssibly the write position or offset in @buf
*
* Behavior:
*
@@ -241,11 +256,14 @@ out:
return ret;
}
-/*
- * get_next_entry - return the offset of the first valid entry at least 'len'
- * bytes after 'off'.
+/**
+ * get_next_entry() - return the offset of the first valid entry at least @len
+ * bytes after @off.
+ * @log: The log being read from
+ * @off: The offset / number of bytes to skip
+ * @len: The minimum number of bytes to read
*
- * Caller must hold log->mutex.
+ * Caller must hold @log->mutex.
*/
static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
{
@@ -260,19 +278,21 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
return off;
}
-/*
- * is_between - is a < c < b, accounting for wrapping of a, b, and c
+/**
+ * is_between() - is @a < @c < @b, accounting for wrapping of @a, @b, and @c
* positions in the buffer
+ * @a: The starting position
+ * @b: The finishing position
+ * @c: The position being searched for
*
- * That is, if a<b, check for c between a and b
- * and if a>b, check for c outside (not between) a and b
+ * That is, if @a < @b, check for @c between @a and @b
+ * and if @a > @b, check for @c outside (not between) @a and @b
*
* |------- a xxxxxxxx b --------|
* c^
*
* |xxxxx b --------- a xxxxxxxxx|
- * c^
- * or c^
+ * c^ or c^
*/
static inline int is_between(size_t a, size_t b, size_t c)
{
@@ -289,13 +309,17 @@ static inline int is_between(size_t a, size_t b, size_t c)
return 0;
}
-/*
- * fix_up_readers - walk the list of all readers and "fix up" any who were
- * lapped by the writer; also do the same for the default "start head".
+/**
+ * fix_up_readers() - walk the list of all readers and "fix up" any who were
+ * lapped by the writer.
+ * @log: The log being referenced
+ * @len: The number of bytes to "pull" the reader forward by
+ *
+ * Also does the same for the default "start head".
* We do this by "pulling forward" the readers and start head to the first
* entry after the new write head.
*
- * The caller needs to hold log->mutex.
+ * The caller needs to hold @log->mutex.
*/
static void fix_up_readers(struct logger_log *log, size_t len)
{
@@ -311,8 +335,11 @@ static void fix_up_readers(struct logger_log *log, size_t len)
reader->r_off = get_next_entry(log, reader->r_off, len);
}
-/*
- * do_write_log - writes 'len' bytes from 'buf' to 'log'
+/**
+ * do_write_log() - writes 'len' bytes from @buf to @log
+ * @log: The log being written into
+ * @buf: The buffer being read from
+ * @count: The number of bytes to write
*
* The caller needs to hold log->mutex.
*/
@@ -330,9 +357,12 @@ static void do_write_log(struct logger_log *log, const void *buf, size_t count)
}
-/*
- * do_write_log_user - writes 'len' bytes from the user-space buffer 'buf' to
- * the log 'log'
+/**
+ * do_write_log_user() - writes 'len' bytes from the user-space buffer @buf
+ * to @log
+ * @log: The log being written into
+ * @buf: The user-space buffer being read from
+ * @count: The number of bytes to write
*
* The caller needs to hold log->mutex.
*
--
1.7.9.5
Sorry, I didn't realize that text would look so ugly in LKML.
I'll be sure to keep the lines nice and short next time.
~Cruz
On 01/08/12 14:54, Cruz Julian Bishop wrote:
> Previously, when calling is_between(a, b, c), the calculation was wrong.
> It counted C as between A and B if C was equal to B, but not A.
>
> Example of this are:
>
> is_between(1, 10, 10) = 1 (Expected: 0)
> is_between(1, 10, 1) = 0 (Expected: 0)
> is_between(20, 10, 10) = 1 (Expected: 0)
>
> And so on and so forth.
>
> Obviously, ten is not a number between one and ten - only two to eight are, so I made this patch :)
Is nine not a number between one and ten? :-p.
The question with a patch like this is whether the function's
documentation, which says it returns 1 if a < c < b is wrong, or whether
the implementation, which does a < c <= b is wrong. If the documentation
is wrong, and something is relying on the current implementation, then
this patch might actually break things.
> Signed-off-by: Cruz Julian Bishop <[email protected]>
> ---
> drivers/staging/android/logger.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 226d8b5..925df5c 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -298,11 +298,11 @@ static inline int is_between(size_t a, size_t b, size_t c)
> {
> if (a < b) {
> /* is c between a and b? */
> - if (a < c && c <= b)
> + if (a < c && c < b)
> return 1;
> } else {
> /* is c outside of b through a? */
> - if (c <= b || a < c)
> + if (c < b || a < c)
> return 1;
> }
A couple of other improvements could be done here. The function should
really return bool, inline is unnecessary (the compiler is smart enough
to do that for us), and we can simplify the logic a bit too:
static bool is_between(size_t a, size_t b, size_t c)
{
if (a < b)
swap(a, b);
return (a < c && c < b);
}
~Ryan
On Wed, Aug 01, 2012 at 02:54:19PM +1000, Cruz Julian Bishop wrote:
> I will document the rest later if they remain unchanged
> Normally, I would do them all at once, but I don't have the chance to do them all at the moment
>
> Signed-off-by: Cruz Julian Bishop <[email protected]>
> ---
> drivers/staging/android/logger.c | 90 +++++++++++++++++++++++++-------------
> 1 file changed, 60 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 1d5ed47..226d8b5 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -78,15 +78,20 @@ struct logger_reader {
> size_t r_off;
> };
>
> -/* logger_offset - returns index 'n' into the log via (optimized) modulus */
> +/**
> + * logger_offset() - returns index 'n' into the log via (optimized) modulus
> + * @log: The log being referenced
> + * @n: The index number being referenced
> + */
> static size_t logger_offset(struct logger_log *log, size_t n)
There is no need to document static functions in this style, unless you
really feel it is needed.
For simple things like this, it isn't needed at all, so I'm not going to
apply this patch, sorry.
greg k-h
On Thu, Aug 02, 2012 at 09:50:44AM +1000, Ryan Mallon wrote:
> On 01/08/12 14:54, Cruz Julian Bishop wrote:
> > Previously, when calling is_between(a, b, c), the calculation was wrong.
> > It counted C as between A and B if C was equal to B, but not A.
> >
> > Example of this are:
> >
> > is_between(1, 10, 10) = 1 (Expected: 0)
> > is_between(1, 10, 1) = 0 (Expected: 0)
> > is_between(20, 10, 10) = 1 (Expected: 0)
> >
> > And so on and so forth.
> >
> > Obviously, ten is not a number between one and ten - only two to eight are, so I made this patch :)
>
> Is nine not a number between one and ten? :-p.
>
> The question with a patch like this is whether the function's
> documentation, which says it returns 1 if a < c < b is wrong, or whether
> the implementation, which does a < c <= b is wrong. If the documentation
> is wrong, and something is relying on the current implementation, then
> this patch might actually break things.
I agree, which is correct? I'd stick with the code for now, care to fix
the comment instead?
greg k-h
On Wed, Aug 01, 2012 at 02:54:15PM +1000, Cruz Julian Bishop wrote:
> Hi,
>
> This set of patches completes more documentation in android/logger.c, as well as fixing a bug there and a comment formatting issue in android/ashmem.c.
>
> Sorry if kernel-doc was not supposed to be applied to driver files - If it isn't, I'll be sure to remember that for next time. :)
>
> Cruz Julian Bishop (5):
> Fix comment/license formatting in android/ashmem.c
> Complete documentation of logger_entry in android/logger.h
> Finish documentation of two structs in android/logger.c
> Redocument some functions in android/logger.c
> Fixes a potential bug in android/logger.c
In the future, please use a better subject line. Something like:
staging: android: logger: fix finish documentation of structure foo
As it is, I had to go and edit your subjects, and wrap your changelog
comments (72 columns please). In the future, I might not.
thanks,
greg k-h
On Mon, 2012-08-13 at 19:01 -0700, Greg KH wrote:
> On Thu, Aug 02, 2012 at 09:50:44AM +1000, Ryan Mallon wrote:
> > On 01/08/12 14:54, Cruz Julian Bishop wrote:
> > > Previously, when calling is_between(a, b, c), the calculation was wrong.
> > > It counted C as between A and B if C was equal to B, but not A.
> > >
> > > Example of this are:
> > >
> > > is_between(1, 10, 10) = 1 (Expected: 0)
> > > is_between(1, 10, 1) = 0 (Expected: 0)
> > > is_between(20, 10, 10) = 1 (Expected: 0)
> > >
> > > And so on and so forth.
> > >
> > > Obviously, ten is not a number between one and ten - only two to eight are, so I made this patch :)
> >
> > Is nine not a number between one and ten? :-p.
> >
> > The question with a patch like this is whether the function's
> > documentation, which says it returns 1 if a < c < b is wrong, or whether
> > the implementation, which does a < c <= b is wrong. If the documentation
> > is wrong, and something is relying on the current implementation, then
> > this patch might actually break things.
>
> I agree, which is correct? I'd stick with the code for now, care to fix
> the comment instead?
>
> greg k-h
Sure, I'll send a new patch for it soon