* [PATCH 2/5] logger: simplify and optimize get_entry_len
2012-02-08 2:21 [PATCH 1/5] logger: Change logger_offset() from macro to function Tim Bird
@ 2012-02-08 2:28 ` Tim Bird
2012-02-08 18:37 ` [PATCH 2/5 v2] " Tim Bird
2012-02-08 2:30 ` [PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock Tim Bird
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2012-02-08 2:28 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Dima Zavin, Andrew Morton
Make this code slightly easier to read, and eliminate calls
to sub-routines. Some of these were previously optimized away
by the compiler, but one memcpy was not.
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92456d7..92cfd94 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -93,19 +93,23 @@ static inline struct logger_log *file_get_log(struct file *file)
* get_entry_len - Grabs the length of the payload of the next entry starting
* from 'off'.
*
+ * 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.
+ * This function returns the size including the log entry structure.
+ *
* Caller needs to hold log->mutex.
*/
static __u32 get_entry_len(struct logger_log *log, size_t off)
{
__u16 val;
- switch (log->size - off) {
- case 1:
- memcpy(&val, log->buffer + off, 1);
- memcpy(((char *) &val) + 1, log->buffer, 1);
- break;
- default:
- memcpy(&val, log->buffer + off, 2);
+ if (unlikely(log->size - off == 1)) {
+ /* at end of buffer, handle wrap */
+ ((unsigned char *)&val)[0] = log->buffer[off];
+ ((unsigned char *)&val)[1] = log->buffer[0];
+ } else {
+ ((unsigned char *)&val)[0] = log->buffer[off];
+ ((unsigned char *)&val)[1] = log->buffer[off+1];
}
return sizeof(struct logger_entry) + val;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/5 v2] logger: simplify and optimize get_entry_len
2012-02-08 2:28 ` [PATCH 2/5] logger: simplify and optimize get_entry_len Tim Bird
@ 2012-02-08 18:37 ` Tim Bird
2012-02-09 5:15 ` Dima Zavin
0 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2012-02-08 18:37 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Dima Zavin, Andrew Morton
Make this code slightly easier to read, and eliminate calls
to sub-routines. Some of these were previously optimized away
by the compiler, but one memcpy was not.
In my testing, this makes the code about 20% smaller, and
has no sub-routine calls and no branches (on ARM).
v2 of this patch is, IMHO, easier to read than v1. Compared to
that patch it uses __u8 instead of unsigned char, for
consistency with the __u16 val data type, simplifies the
conditional expression, adds a another comment, and
moves a common statement out of the if.
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 20 ++++++++++++--------
1 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92456d7..3475cb7 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -93,20 +93,24 @@ static inline struct logger_log *file_get_log(struct file *file)
* get_entry_len - Grabs the length of the payload of the next entry starting
* from 'off'.
*
+ * 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.
+ * This function returns the size including the log entry structure.
+ *
* Caller needs to hold log->mutex.
*/
static __u32 get_entry_len(struct logger_log *log, size_t off)
{
__u16 val;
- switch (log->size - off) {
- case 1:
- memcpy(&val, log->buffer + off, 1);
- memcpy(((char *) &val) + 1, log->buffer, 1);
- break;
- default:
- memcpy(&val, log->buffer + off, 2);
- }
+ /* copy 2 bytes from buffer, in memcpy order, */
+ /* handling possible wrap at end of buffer */
+
+ ((__u8 *)&val)[0] = log->buffer[off];
+ if (likely(off+1 < log->size))
+ ((__u8 *)&val)[1] = log->buffer[off+1];
+ else
+ ((__u8 *)&val)[1] = log->buffer[0];
return sizeof(struct logger_entry) + val;
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/5 v2] logger: simplify and optimize get_entry_len
2012-02-08 18:37 ` [PATCH 2/5 v2] " Tim Bird
@ 2012-02-09 5:15 ` Dima Zavin
2012-02-09 5:58 ` Tim Bird
0 siblings, 1 reply; 13+ messages in thread
From: Dima Zavin @ 2012-02-09 5:15 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland, Andrew Morton
On Wed, Feb 8, 2012 at 10:37 AM, Tim Bird <tim.bird@am.sony.com> wrote:
> Make this code slightly easier to read, and eliminate calls
> to sub-routines. Some of these were previously optimized away
> by the compiler, but one memcpy was not.
>
> In my testing, this makes the code about 20% smaller, and
> has no sub-routine calls and no branches (on ARM).
>
> v2 of this patch is, IMHO, easier to read than v1. Compared to
> that patch it uses __u8 instead of unsigned char, for
> consistency with the __u16 val data type, simplifies the
> conditional expression, adds a another comment, and
> moves a common statement out of the if.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
> drivers/staging/android/logger.c | 20 ++++++++++++--------
> 1 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 92456d7..3475cb7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -93,20 +93,24 @@ static inline struct logger_log *file_get_log(struct file *file)
> * get_entry_len - Grabs the length of the payload of the next entry starting
> * from 'off'.
> *
> + * 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.
> + * This function returns the size including the log entry structure.
> + *
> * Caller needs to hold log->mutex.
> */
> static __u32 get_entry_len(struct logger_log *log, size_t off)
> {
> __u16 val;
Could using a union here make things look a little cleaner in the meat
of the function? Something like
union {
__u16 s;
__u8 b[2];
} val;
>
> - switch (log->size - off) {
> - case 1:
> - memcpy(&val, log->buffer + off, 1);
> - memcpy(((char *) &val) + 1, log->buffer, 1);
> - break;
> - default:
> - memcpy(&val, log->buffer + off, 2);
> - }
> + /* copy 2 bytes from buffer, in memcpy order, */
> + /* handling possible wrap at end of buffer */
> +
> + ((__u8 *)&val)[0] = log->buffer[off];
> + if (likely(off+1 < log->size))
> + ((__u8 *)&val)[1] = log->buffer[off+1];
spaces around the + in 'off+1' in the above two lines.
> + else
> + ((__u8 *)&val)[1] = log->buffer[0];
>
> return sizeof(struct logger_entry) + val;
> }
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/5 v2] logger: simplify and optimize get_entry_len
2012-02-09 5:15 ` Dima Zavin
@ 2012-02-09 5:58 ` Tim Bird
0 siblings, 0 replies; 13+ messages in thread
From: Tim Bird @ 2012-02-09 5:58 UTC (permalink / raw)
To: Dima Zavin
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland, Andrew Morton
On 2/8/2012 9:15 PM, Dima Zavin wrote:
> On Wed, Feb 8, 2012 at 10:37 AM, Tim Bird<tim.bird@am.sony.com> wrote:
>> Make this code slightly easier to read, and eliminate calls
>> to sub-routines. Some of these were previously optimized away
>> by the compiler, but one memcpy was not.
>>
>> In my testing, this makes the code about 20% smaller, and
>> has no sub-routine calls and no branches (on ARM).
>>
>> v2 of this patch is, IMHO, easier to read than v1. Compared to
>> that patch it uses __u8 instead of unsigned char, for
>> consistency with the __u16 val data type, simplifies the
>> conditional expression, adds a another comment, and
>> moves a common statement out of the if.
>>
>> Signed-off-by: Tim Bird<tim.bird@am.sony.com>
>> ---
>> drivers/staging/android/logger.c | 20 ++++++++++++--------
>> 1 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
>> index 92456d7..3475cb7 100644
>> --- a/drivers/staging/android/logger.c
>> +++ b/drivers/staging/android/logger.c
>> @@ -93,20 +93,24 @@ static inline struct logger_log *file_get_log(struct file *file)
>> * get_entry_len - Grabs the length of the payload of the next entry starting
>> * from 'off'.
>> *
>> + * 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.
>> + * This function returns the size including the log entry structure.
>> + *
>> * Caller needs to hold log->mutex.
>> */
>> static __u32 get_entry_len(struct logger_log *log, size_t off)
>> {
>> __u16 val;
> Could using a union here make things look a little cleaner in the meat
> of the function? Something like
>
> union {
> __u16 s;
> __u8 b[2];
> } val;
>
That's a good idea. I was looking for a way to use a byte array that
wouldn't get misaligned if the function declaration changed. I'll
test this out and see what it looks like.
>> - switch (log->size - off) {
>> - case 1:
>> - memcpy(&val, log->buffer + off, 1);
>> - memcpy(((char *)&val) + 1, log->buffer, 1);
>> - break;
>> - default:
>> - memcpy(&val, log->buffer + off, 2);
>> - }
>> + /* copy 2 bytes from buffer, in memcpy order, */
>> + /* handling possible wrap at end of buffer */
>> +
>> + ((__u8 *)&val)[0] = log->buffer[off];
>> + if (likely(off+1< log->size))
>> + ((__u8 *)&val)[1] = log->buffer[off+1];
> spaces around the + in 'off+1' in the above two lines.
Yeah. I keep omitting spaces. I'll fix this, and the ones
mentioned on patch 1/5.
-- Tim
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock
2012-02-08 2:21 [PATCH 1/5] logger: Change logger_offset() from macro to function Tim Bird
2012-02-08 2:28 ` [PATCH 2/5] logger: simplify and optimize get_entry_len Tim Bird
@ 2012-02-08 2:30 ` Tim Bird
2012-02-09 5:56 ` Dima Zavin
2012-02-08 2:32 ` [PATCH 4/5] logger: clarify code in clock_interval Tim Bird
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2012-02-08 2:30 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Dima Zavin, Andrew Morton
If mutex_lock waits, it will return in state TASK_RUNNING,
rubbing out the effect of prepare_to_wait().
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92cfd94..54b7cdf 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -172,9 +172,10 @@ static ssize_t logger_read(struct file *file, char __user *buf,
start:
while (1) {
+ mutex_lock(&log->mutex);
+
prepare_to_wait(&log->wq, &wait, TASK_INTERRUPTIBLE);
- mutex_lock(&log->mutex);
ret = (log->w_off == reader->r_off);
mutex_unlock(&log->mutex);
if (!ret)
--
1.7.2.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock
2012-02-08 2:30 ` [PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock Tim Bird
@ 2012-02-09 5:56 ` Dima Zavin
0 siblings, 0 replies; 13+ messages in thread
From: Dima Zavin @ 2012-02-09 5:56 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland, Andrew Morton
On Tue, Feb 7, 2012 at 6:30 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> If mutex_lock waits, it will return in state TASK_RUNNING,
> rubbing out the effect of prepare_to_wait().
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
Acked-by: Dima Zavin <dima@android.com>
> ---
> drivers/staging/android/logger.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 92cfd94..54b7cdf 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -172,9 +172,10 @@ static ssize_t logger_read(struct file *file, char __user *buf,
>
> start:
> while (1) {
> + mutex_lock(&log->mutex);
> +
> prepare_to_wait(&log->wq, &wait, TASK_INTERRUPTIBLE);
>
> - mutex_lock(&log->mutex);
> ret = (log->w_off == reader->r_off);
> mutex_unlock(&log->mutex);
> if (!ret)
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] logger: clarify code in clock_interval
2012-02-08 2:21 [PATCH 1/5] logger: Change logger_offset() from macro to function Tim Bird
2012-02-08 2:28 ` [PATCH 2/5] logger: simplify and optimize get_entry_len Tim Bird
2012-02-08 2:30 ` [PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock Tim Bird
@ 2012-02-08 2:32 ` Tim Bird
2012-02-09 6:09 ` Dima Zavin
2012-02-08 2:34 ` [PATCH 5/5] logger: clarify non-update of w_off in do_write_log_from_user Tim Bird
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2012-02-08 2:32 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Dima Zavin, Andrew Morton
Add commentary, rename the function and make the code easier to read.
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 28 ++++++++++++++++++++--------
1 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 54b7cdf..8d9d4f1 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -242,16 +242,28 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
}
/*
- * clock_interval - is a < c < b in mod-space? Put another way, does the line
- * from a to b cross c?
+ * is_between - is a < c < b, accounting for wrapping of a, b, and c
+ * positions in the buffer
+ *
+ * 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^
*/
-static inline int clock_interval(size_t a, size_t b, size_t c)
+static inline int is_between(size_t a, size_t b, size_t c)
{
- if (b < a) {
- if (a < c || b >= c)
+ if (a < b) {
+ /* is c between a and b? */
+ if (a < c && c <= b)
return 1;
} else {
- if (a < c && b >= c)
+ /* is c outside of b through a? */
+ if (c <= b || a < c)
return 1;
}
@@ -272,11 +284,11 @@ static void fix_up_readers(struct logger_log *log, size_t len)
size_t new = logger_offset(log, old + len);
struct logger_reader *reader;
- if (clock_interval(old, new, log->head))
+ if (is_between(old, new, log->head))
log->head = get_next_entry(log, log->head, len);
list_for_each_entry(reader, &log->readers, list)
- if (clock_interval(old, new, reader->r_off))
+ if (is_between(old, new, reader->r_off))
reader->r_off = get_next_entry(log, reader->r_off, len);
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/5] logger: clarify code in clock_interval
2012-02-08 2:32 ` [PATCH 4/5] logger: clarify code in clock_interval Tim Bird
@ 2012-02-09 6:09 ` Dima Zavin
0 siblings, 0 replies; 13+ messages in thread
From: Dima Zavin @ 2012-02-09 6:09 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland, Andrew Morton
To be honest, i don't find the new logic code much different or
cleaner than the old code. I think just documenting (as well as the
function rename) as you've done without inverting the logic would be
sufficient. The ascii art helps immensely.
--Dima
On Tue, Feb 7, 2012 at 6:32 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> Add commentary, rename the function and make the code easier to read.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
> drivers/staging/android/logger.c | 28 ++++++++++++++++++++--------
> 1 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 54b7cdf..8d9d4f1 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -242,16 +242,28 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
> }
>
> /*
> - * clock_interval - is a < c < b in mod-space? Put another way, does the line
> - * from a to b cross c?
> + * is_between - is a < c < b, accounting for wrapping of a, b, and c
> + * positions in the buffer
> + *
> + * 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^
> */
> -static inline int clock_interval(size_t a, size_t b, size_t c)
> +static inline int is_between(size_t a, size_t b, size_t c)
> {
> - if (b < a) {
> - if (a < c || b >= c)
> + if (a < b) {
> + /* is c between a and b? */
> + if (a < c && c <= b)
> return 1;
> } else {
> - if (a < c && b >= c)
> + /* is c outside of b through a? */
> + if (c <= b || a < c)
> return 1;
> }
>
> @@ -272,11 +284,11 @@ static void fix_up_readers(struct logger_log *log, size_t len)
> size_t new = logger_offset(log, old + len);
> struct logger_reader *reader;
>
> - if (clock_interval(old, new, log->head))
> + if (is_between(old, new, log->head))
> log->head = get_next_entry(log, log->head, len);
>
> list_for_each_entry(reader, &log->readers, list)
> - if (clock_interval(old, new, reader->r_off))
> + if (is_between(old, new, reader->r_off))
> reader->r_off = get_next_entry(log, reader->r_off, len);
> }
>
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/5] logger: clarify non-update of w_off in do_write_log_from_user
2012-02-08 2:21 [PATCH 1/5] logger: Change logger_offset() from macro to function Tim Bird
` (2 preceding siblings ...)
2012-02-08 2:32 ` [PATCH 4/5] logger: clarify code in clock_interval Tim Bird
@ 2012-02-08 2:34 ` Tim Bird
2012-02-09 6:10 ` Dima Zavin
2012-02-08 3:22 ` [PATCH 1/5] logger: Change logger_offset() from macro to function Frank Rowand
2012-02-09 4:54 ` Dima Zavin
5 siblings, 1 reply; 13+ messages in thread
From: Tim Bird @ 2012-02-08 2:34 UTC (permalink / raw)
To: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Dima Zavin, Andrew Morton
Add comment to explain when w_off is not updated in case of failed second
fragment copy to buffer.
Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
drivers/staging/android/logger.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 8d9d4f1..115d8ed 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -330,6 +330,12 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
if (count != len)
if (copy_from_user(log->buffer, buf + len, count - len))
+ /*
+ * Note that by not updating w_off, this abandons the
+ * portion of the new entry that *was* successfully
+ * copied, just above. This is intentional to avoid
+ * message corruption from missing fragments.
+ */
return -EFAULT;
log->w_off = logger_offset(log, log->w_off + count);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] logger: clarify non-update of w_off in do_write_log_from_user
2012-02-08 2:34 ` [PATCH 5/5] logger: clarify non-update of w_off in do_write_log_from_user Tim Bird
@ 2012-02-09 6:10 ` Dima Zavin
0 siblings, 0 replies; 13+ messages in thread
From: Dima Zavin @ 2012-02-09 6:10 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland, Andrew Morton
On Tue, Feb 7, 2012 at 6:34 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> Add comment to explain when w_off is not updated in case of failed second
> fragment copy to buffer.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
Acked-by: Dima Zavin <dima@android.com>
> ---
> drivers/staging/android/logger.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 8d9d4f1..115d8ed 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -330,6 +330,12 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
>
> if (count != len)
> if (copy_from_user(log->buffer, buf + len, count - len))
> + /*
> + * Note that by not updating w_off, this abandons the
> + * portion of the new entry that *was* successfully
> + * copied, just above. This is intentional to avoid
> + * message corruption from missing fragments.
> + */
> return -EFAULT;
>
> log->w_off = logger_offset(log, log->w_off + count);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] logger: Change logger_offset() from macro to function
2012-02-08 2:21 [PATCH 1/5] logger: Change logger_offset() from macro to function Tim Bird
` (3 preceding siblings ...)
2012-02-08 2:34 ` [PATCH 5/5] logger: clarify non-update of w_off in do_write_log_from_user Tim Bird
@ 2012-02-08 3:22 ` Frank Rowand
2012-02-09 4:54 ` Dima Zavin
5 siblings, 0 replies; 13+ messages in thread
From: Frank Rowand @ 2012-02-08 3:22 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland,
Dima Zavin, Andrew Morton
On 02/07/12 18:21, Tim Bird wrote:
> Convert to function and add log as a parameter, rather than relying
> on log in the context of the macro.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
> drivers/staging/android/logger.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index ffc2d04..92456d7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -60,7 +60,11 @@ struct logger_reader {
> };
>
> /* logger_offset - returns index 'n' into the log via (optimized) modulus */
> -#define logger_offset(n) ((n) & (log->size - 1))
> +size_t logger_offset(struct logger_log *log, size_t n)
> +{
> + return n & (log->size-1);
return n & (log->size - 1);
> +}
> +
Reviewed-by: Frank Rowand <frank.rowand@am.sony.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] logger: Change logger_offset() from macro to function
2012-02-08 2:21 [PATCH 1/5] logger: Change logger_offset() from macro to function Tim Bird
` (4 preceding siblings ...)
2012-02-08 3:22 ` [PATCH 1/5] logger: Change logger_offset() from macro to function Frank Rowand
@ 2012-02-09 4:54 ` Dima Zavin
5 siblings, 0 replies; 13+ messages in thread
From: Dima Zavin @ 2012-02-09 4:54 UTC (permalink / raw)
To: Tim Bird
Cc: Greg KH, linux-embedded, linux kernel, Brian Swetland, Andrew Morton
small style nit otherwise looks ok.
On Tue, Feb 7, 2012 at 6:21 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> Convert to function and add log as a parameter, rather than relying
> on log in the context of the macro.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
> drivers/staging/android/logger.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index ffc2d04..92456d7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -60,7 +60,11 @@ struct logger_reader {
> };
>
> /* logger_offset - returns index 'n' into the log via (optimized) modulus */
> -#define logger_offset(n) ((n) & (log->size - 1))
> +size_t logger_offset(struct logger_log *log, size_t n)
> +{
> + return n & (log->size-1);
spaces around -
> +}
> +
>
> /*
> * file_get_log - Given a file structure, return the associated log
> @@ -137,7 +141,7 @@ static ssize_t do_read_log_to_user(struct logger_log *log,
> if (copy_to_user(buf + len, log->buffer, count - len))
> return -EFAULT;
>
> - reader->r_off = logger_offset(reader->r_off + count);
> + reader->r_off = logger_offset(log, reader->r_off + count);
>
> return count;
> }
> @@ -225,7 +229,7 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
>
> do {
> size_t nr = get_entry_len(log, off);
> - off = logger_offset(off + nr);
> + off = logger_offset(log, off + nr);
> count += nr;
> } while (count < len);
>
> @@ -260,7 +264,7 @@ static inline int clock_interval(size_t a, size_t b, size_t c)
> static void fix_up_readers(struct logger_log *log, size_t len)
> {
> size_t old = log->w_off;
> - size_t new = logger_offset(old + len);
> + size_t new = logger_offset(log, old + len);
> struct logger_reader *reader;
>
> if (clock_interval(old, new, log->head))
> @@ -286,7 +290,7 @@ static void do_write_log(struct logger_log *log, const void *buf, size_t count)
> if (count != len)
> memcpy(log->buffer, buf + len, count - len);
>
> - log->w_off = logger_offset(log->w_off + count);
> + log->w_off = logger_offset(log, log->w_off + count);
>
> }
>
> @@ -311,7 +315,7 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
> if (copy_from_user(log->buffer, buf + len, count - len))
> return -EFAULT;
>
> - log->w_off = logger_offset(log->w_off + count);
> + log->w_off = logger_offset(log, log->w_off + count);
>
> return count;
> }
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 13+ messages in thread