Re: [PATCH 2/3] devmem: introduce size_inside_page()
From: Andrew Morton
Date: Fri Sep 11 2009 - 19:57:31 EST
On 2009年9月11日 10:23:35 +0800
Wu Fengguang <fengguang.wu@xxxxxxxxx> wrote:
>
Introduce size_inside_page() to replace duplicate /dev/mem code.
>
>
Also apply it to /dev/kmem, whose alignment logic was buggy.
>
>
>
CC: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
>
CC: Greg Kroah-Hartman <gregkh@xxxxxxx>
>
CC: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
>
CC: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
>
CC: Avi Kivity <avi@xxxxxxxxxxxx>
>
Signed-off-by: Wu Fengguang <fengguang.wu@xxxxxxxxx>
>
---
>
drivers/char/mem.c | 60 +++++++++++++------------------------------
>
1 file changed, 19 insertions(+), 41 deletions(-)
>
>
--- linux.orig/drivers/char/mem.c
>
+++ linux/drivers/char/mem.c
>
@@ -35,6 +35,19 @@
>
# include <linux/efi.h>
>
#endif
>
>
+static inline unsigned long size_inside_page(unsigned long start,
>
+ unsigned long size)
>
+{
>
+ unsigned long sz;
>
+
>
+ if (-start & (PAGE_SIZE - 1))
>
+ sz = -start & (PAGE_SIZE - 1);
What on earth is this doing? Negating an unsigned number?
Can we get rid of these party tricks and use something more
conventional here? In a separate patch I guess.
>
+ else
>
+ sz = PAGE_SIZE;
>
+
>
+ return min_t(unsigned long, sz, size);
Can use min() here.
>
+}
Please have a think about the types. Should we be using unsigned long,
or size_t? Which makes more sense? Which maps better onto reality?
I suspect that the min_t which you inherited was added somewhere
because someone didn't get the types right: int-vs-size_t or something.
If we actually get the types right, this sort of thing goes away.
>
@@ -462,10 +451,8 @@ static ssize_t read_kmem(struct file *fi
>
if (!kbuf)
>
return -ENOMEM;
>
while (count > 0) {
>
- int len = count;
>
+ int len = size_inside_page(p, count);
int?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
http://www.tux.org/lkml/