[PATCH 0/5] Android: Small documentation changes and a bug fix

[PATCH 0/5] Android: Small documentation changes and a bug fix

[Cruz Julian Bishop]

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

[PATCH 1/5] Fix comment/license formatting in android/ashmem.c

[Cruz Julian Bishop]

Signed-off-by: Cruz Julian Bishop <cruzjbishop@gmail.com>
---
 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 <rlove@google.com>
-**
-** 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 <rlove@google.com>
+ *
+ * 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

[PATCH 2/5] Complete documentation of logger_entry in android/logger.h

[Cruz Julian Bishop]

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 <cruzjbishop@gmail.com>
---
 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

[PATCH 3/5] Finish documentation of two structs in android/logger.c

[Cruz Julian Bishop]

Signed-off-by: Cruz Julian Bishop <cruzjbishop@gmail.com>
---
 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

[PATCH 4/5] Redocument some functions in android/logger.c

[Cruz Julian Bishop]

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 <cruzjbishop@gmail.com>
---
 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

[PATCH 5/5] Fixes a potential bug in android/logger.c

[Cruz Julian Bishop]

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 <cruzjbishop@gmail.com>
---
 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

Re: [PATCH 0/5] Android: Small documentation changes and a bug fix

[Cruz Julian Bishop]

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

Re: [PATCH 5/5] Fixes a potential bug in android/logger.c

[Ryan Mallon]

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 <cruzjbishop@gmail.com>
> ---
>  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

Re: [PATCH 4/5] Redocument some functions in android/logger.c

[Greg KH]

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 <cruzjbishop@gmail.com>
> ---
>  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

Re: [PATCH 5/5] Fixes a potential bug in android/logger.c

[Greg KH]

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

Re: [PATCH 0/5] Android: Small documentation changes and a bug fix

[Greg KH]

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

Re: [PATCH 5/5] Fixes a potential bug in android/logger.c

[Cruz Julian Bishop]

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