[PATCH 0/3] /dev/mem cleanups and bug fix

[PATCH 0/3] /dev/mem cleanups and bug fix

[Wu Fengguang]

Hi Andrew,

Here are three patches for /dev/[k]mem.
They are mostly code cleanups, plus a minor bug fix:
if someone read high mem via /dev/kmem starting from
an unaligned address, it's calculation of 'len' will
go wrong.

I would recommend them for .32.

Thanks,
Fengguang
-- 

[PATCH 1/3] devmem: remove redundant test on len

[Wu Fengguang]

[-- Attachment #1: kmem-cleanup.patch --]
[-- Type: text/plain, Size: 1100 bytes --]

The len test in write_kmem() is always true, so can be reduced.

CC: Marcelo Tosatti <mtosatti@redhat.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
CC: Johannes Berg <johannes@sipsolutions.net>
CC: Avi Kivity <avi@qumranet.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 drivers/char/mem.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

--- linux.orig/drivers/char/mem.c
+++ linux/drivers/char/mem.c
@@ -580,18 +580,16 @@ static ssize_t write_kmem(struct file * 
 		while (count > 0) {
 			int len = count;
 
 			if (len > PAGE_SIZE)
 				len = PAGE_SIZE;
-			if (len) {
-				written = copy_from_user(kbuf, buf, len);
-				if (written) {
-					if (wrote + virtr)
-						break;
-					free_page((unsigned long)kbuf);
-					return -EFAULT;
-				}
+			written = copy_from_user(kbuf, buf, len);
+			if (written) {
+				if (wrote + virtr)
+					break;
+				free_page((unsigned long)kbuf);
+				return -EFAULT;
 			}
 			len = vwrite(kbuf, (char *)p, len);
 			count -= len;
 			buf += len;
 			virtr += len;

-- 

[PATCH 2/3] devmem: introduce size_inside_page()

[Wu Fengguang]

[-- Attachment #1: kmem-round-to-page-size.patch --]
[-- Type: text/plain, Size: 3236 bytes --]

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@redhat.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
CC: Johannes Berg <johannes@sipsolutions.net>
CC: Avi Kivity <avi@qumranet.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 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);
+	else
+		sz = PAGE_SIZE;
+
+	return min_t(unsigned long, sz, size);
+}
+
 /*
  * Architectures vary in how they handle caching for addresses
  * outside of main memory.
@@ -142,15 +155,7 @@ static ssize_t read_mem(struct file * fi
 #endif
 
 	while (count > 0) {
-		/*
-		 * Handle first page in case it's not aligned
-		 */
-		if (-p & (PAGE_SIZE - 1))
-			sz = -p & (PAGE_SIZE - 1);
-		else
-			sz = PAGE_SIZE;
-
-		sz = min_t(unsigned long, sz, count);
+		sz = size_inside_page(p, count);
 
 		if (!range_is_allowed(p >> PAGE_SHIFT, count))
 			return -EPERM;
@@ -209,15 +214,7 @@ static ssize_t write_mem(struct file * f
 #endif
 
 	while (count > 0) {
-		/*
-		 * Handle first page in case it's not aligned
-		 */
-		if (-p & (PAGE_SIZE - 1))
-			sz = -p & (PAGE_SIZE - 1);
-		else
-			sz = PAGE_SIZE;
-
-		sz = min_t(unsigned long, sz, count);
+		sz = size_inside_page(p, count);
 
 		if (!range_is_allowed(p >> PAGE_SHIFT, sz))
 			return -EPERM;
@@ -430,15 +427,7 @@ static ssize_t read_kmem(struct file *fi
 		}
 #endif
 		while (low_count > 0) {
-			/*
-			 * Handle first page in case it's not aligned
-			 */
-			if (-p & (PAGE_SIZE - 1))
-				sz = -p & (PAGE_SIZE - 1);
-			else
-				sz = PAGE_SIZE;
-
-			sz = min_t(unsigned long, sz, low_count);
+			sz = size_inside_page(p, low_count);
 
 			/*
 			 * On ia64 if a page has been mapped somewhere as
@@ -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);
 
-			if (len > PAGE_SIZE)
-				len = PAGE_SIZE;
 			len = vread(kbuf, (char *)p, len);
 			if (!len)
 				break;
@@ -510,15 +497,8 @@ do_write_kmem(void *p, unsigned long rea
 
 	while (count > 0) {
 		char *ptr;
-		/*
-		 * Handle first page in case it's not aligned
-		 */
-		if (-realp & (PAGE_SIZE - 1))
-			sz = -realp & (PAGE_SIZE - 1);
-		else
-			sz = PAGE_SIZE;
 
-		sz = min_t(unsigned long, sz, count);
+		sz = size_inside_page(realp, count);
 
 		/*
 		 * On ia64 if a page has been mapped somewhere as
@@ -578,10 +558,8 @@ static ssize_t write_kmem(struct file * 
 		if (!kbuf)
 			return wrote ? wrote : -ENOMEM;
 		while (count > 0) {
-			int len = count;
+			int len = size_inside_page(p, count);
 
-			if (len > PAGE_SIZE)
-				len = PAGE_SIZE;
 			written = copy_from_user(kbuf, buf, len);
 			if (written) {
 				if (wrote + virtr)

-- 

[PATCH 3/3] devmem: cleanup unxlate_dev_mem_ptr() calls

[Wu Fengguang]

[-- Attachment #1: kmem-dev-mem-cleanup.patch --]
[-- Type: text/plain, Size: 1388 bytes --]

No behavior change.

CC: Marcelo Tosatti <mtosatti@redhat.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
CC: Johannes Berg <johannes@sipsolutions.net>
CC: Avi Kivity <avi@qumranet.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 drivers/char/mem.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

--- linux-mm.orig/drivers/char/mem.c	2009-09-10 21:59:39.000000000 +0800
+++ linux-mm/drivers/char/mem.c	2009-09-10 22:00:12.000000000 +0800
@@ -131,6 +131,7 @@ static ssize_t read_mem(struct file * fi
 			size_t count, loff_t *ppos)
 {
 	unsigned long p = *ppos;
+	unsigned long ret;
 	ssize_t read, sz;
 	char *ptr;
 
@@ -169,12 +170,10 @@ static ssize_t read_mem(struct file * fi
 		if (!ptr)
 			return -EFAULT;
 
-		if (copy_to_user(buf, ptr, sz)) {
-			unxlate_dev_mem_ptr(p, ptr);
-			return -EFAULT;
-		}
-
+		ret = copy_to_user(buf, ptr, sz);
 		unxlate_dev_mem_ptr(p, ptr);
+		if (ret)
+			return -EFAULT;
 
 		buf += sz;
 		p += sz;
@@ -232,16 +231,14 @@ static ssize_t write_mem(struct file * f
 		}
 
 		copied = copy_from_user(ptr, buf, sz);
+		unxlate_dev_mem_ptr(p, ptr);
 		if (copied) {
 			written += sz - copied;
-			unxlate_dev_mem_ptr(p, ptr);
 			if (written)
 				break;
 			return -EFAULT;
 		}
 
-		unxlate_dev_mem_ptr(p, ptr);
-
 		buf += sz;
 		p += sz;
 		count -= sz;

-- 

Re: [PATCH 0/3] /dev/mem cleanups and bug fix

[Andi Kleen]

On Fri, Sep 11, 2009 at 10:23:33AM +0800, Wu Fengguang wrote:
> Hi Andrew,
> 
> Here are three patches for /dev/[k]mem.
> They are mostly code cleanups, plus a minor bug fix:
> if someone read high mem via /dev/kmem starting from
> an unaligned address, it's calculation of 'len' will
> go wrong.
> 
> I would recommend them for .32.

I reviewed the three patches and they all look good to me.

Acked-by: Andi Kleen <ak@linux.intel.com>

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 2/3] devmem: introduce size_inside_page()

[Andrew Morton]

On Fri, 11 Sep 2009 10:23:35 +0800
Wu Fengguang <fengguang.wu@intel.com> 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@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@suse.de>
> CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
> CC: Johannes Berg <johannes@sipsolutions.net>
> CC: Avi Kivity <avi@qumranet.com>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
>  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?

Re: [PATCH 1/3] devmem: remove redundant test on len

[Andrew Morton]

On Fri, 11 Sep 2009 10:23:34 +0800
Wu Fengguang <fengguang.wu@intel.com> wrote:

> The len test in write_kmem() is always true, so can be reduced.
> 
> CC: Marcelo Tosatti <mtosatti@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@suse.de>
> CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
> CC: Johannes Berg <johannes@sipsolutions.net>
> CC: Avi Kivity <avi@qumranet.com>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
>  drivers/char/mem.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> --- linux.orig/drivers/char/mem.c
> +++ linux/drivers/char/mem.c
> @@ -580,18 +580,16 @@ static ssize_t write_kmem(struct file * 
>  		while (count > 0) {
>  			int len = count;
>  
>  			if (len > PAGE_SIZE)
>  				len = PAGE_SIZE;
> -			if (len) {
> -				written = copy_from_user(kbuf, buf, len);
> -				if (written) {
> -					if (wrote + virtr)
> -						break;
> -					free_page((unsigned long)kbuf);
> -					return -EFAULT;
> -				}
> +			written = copy_from_user(kbuf, buf, len);
> +			if (written) {
> +				if (wrote + virtr)
> +					break;
> +				free_page((unsigned long)kbuf);
> +				return -EFAULT;
>  			}
>  			len = vwrite(kbuf, (char *)p, len);
>  			count -= len;
>  			buf += len;
>  			virtr += len;

humpf.  But take a closer look at what remains.


Local var `written' is unneeded here.


Which makes us look at what `written' _does_ do:

		if (written != wrote)
			return written;
		wrote = written;

lolwhowrotethat?

local var `written' can at least be made local to the first loop.

write_kmem() has a lot of typecasts which indicates that the choices of
types were inappropriate.

Re: [PATCH 3/3] devmem: cleanup unxlate_dev_mem_ptr() calls

[Andrew Morton]

On Fri, 11 Sep 2009 10:23:36 +0800
Wu Fengguang <fengguang.wu@intel.com> wrote:

> No behavior change.
> 
> CC: Marcelo Tosatti <mtosatti@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@suse.de>
> CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
> CC: Johannes Berg <johannes@sipsolutions.net>
> CC: Avi Kivity <avi@qumranet.com>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
>  drivers/char/mem.c |   13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> --- linux-mm.orig/drivers/char/mem.c	2009-09-10 21:59:39.000000000 +0800
> +++ linux-mm/drivers/char/mem.c	2009-09-10 22:00:12.000000000 +0800
> @@ -131,6 +131,7 @@ static ssize_t read_mem(struct file * fi
>  			size_t count, loff_t *ppos)
>  {
>  	unsigned long p = *ppos;
> +	unsigned long ret;
>  	ssize_t read, sz;
>  	char *ptr;
>  
> @@ -169,12 +170,10 @@ static ssize_t read_mem(struct file * fi
>  		if (!ptr)
>  			return -EFAULT;
>  
> -		if (copy_to_user(buf, ptr, sz)) {
> -			unxlate_dev_mem_ptr(p, ptr);
> -			return -EFAULT;
> -		}
> -
> +		ret = copy_to_user(buf, ptr, sz);
>  		unxlate_dev_mem_ptr(p, ptr);
> +		if (ret)
> +			return -EFAULT;
>  
>  		buf += sz;
>  		p += sz;

- local var `ret' didn't need function-wide scope.  I think it's
  better to reduce its scope if poss.

- conventionally the identifier `ret' refers to "the value which this
  function will return".  Ditto `retval' and `rc'.

  But that's not what `ret' does here so let's call it something
  else?  `remaining' is rather verbose and formal, but accurate.


--- a/drivers/char/mem.c~dev-mem-cleanup-unxlate_dev_mem_ptr-calls-fix
+++ a/drivers/char/mem.c
@@ -131,7 +131,6 @@ static ssize_t read_mem(struct file * fi
 			size_t count, loff_t *ppos)
 {
 	unsigned long p = *ppos;
-	unsigned long ret;
 	ssize_t read, sz;
 	char *ptr;
 
@@ -156,6 +155,8 @@ static ssize_t read_mem(struct file * fi
 #endif
 
 	while (count > 0) {
+		unsigned long remaining;
+
 		sz = size_inside_page(p, count);
 
 		if (!range_is_allowed(p >> PAGE_SHIFT, count))
@@ -170,9 +171,9 @@ static ssize_t read_mem(struct file * fi
 		if (!ptr)
 			return -EFAULT;
 
-		ret = copy_to_user(buf, ptr, sz);
+		remaining = copy_to_user(buf, ptr, sz);
 		unxlate_dev_mem_ptr(p, ptr);
-		if (ret)
+		if (remaining)
 			return -EFAULT;
 
 		buf += sz;
_

Re: [PATCH 1/3] devmem: remove redundant test on len

[Wu Fengguang]

On Sat, Sep 12, 2009 at 08:00:47AM +0800, Andrew Morton wrote:
> On Fri, 11 Sep 2009 10:23:34 +0800
> Wu Fengguang <fengguang.wu@intel.com> wrote:
> 
> > The len test in write_kmem() is always true, so can be reduced.
> > 
> > CC: Marcelo Tosatti <mtosatti@redhat.com>
> > CC: Greg Kroah-Hartman <gregkh@suse.de>
> > CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > CC: Johannes Berg <johannes@sipsolutions.net>
> > CC: Avi Kivity <avi@qumranet.com>
> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > ---
> >  drivers/char/mem.c |   14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > --- linux.orig/drivers/char/mem.c
> > +++ linux/drivers/char/mem.c
> > @@ -580,18 +580,16 @@ static ssize_t write_kmem(struct file * 
> >  		while (count > 0) {
> >  			int len = count;
> >  
> >  			if (len > PAGE_SIZE)
> >  				len = PAGE_SIZE;
> > -			if (len) {
> > -				written = copy_from_user(kbuf, buf, len);
> > -				if (written) {
> > -					if (wrote + virtr)
> > -						break;
> > -					free_page((unsigned long)kbuf);
> > -					return -EFAULT;
> > -				}
> > +			written = copy_from_user(kbuf, buf, len);
> > +			if (written) {
> > +				if (wrote + virtr)
> > +					break;
> > +				free_page((unsigned long)kbuf);
> > +				return -EFAULT;
> >  			}
> >  			len = vwrite(kbuf, (char *)p, len);
> >  			count -= len;
> >  			buf += len;
> >  			virtr += len;
> 
> humpf.  But take a closer look at what remains.

Yeah, it asks for more cleanup patches..

> 
> Local var `written' is unneeded here.
> 
> 
> Which makes us look at what `written' _does_ do:
> 
> 		if (written != wrote)
> 			return written;
> 		wrote = written;
> 
> lolwhowrotethat?
> 
> local var `written' can at least be made local to the first loop.

Right, I'll make this straight.

> write_kmem() has a lot of typecasts which indicates that the choices of
> types were inappropriate.

It seems hard to improve because of a fundamental issue: the same
value is taken as both virtual and physical address, and also be
compared and calculated as numbers..

Thanks,
Fengguang

Re: [PATCH 2/3] devmem: introduce size_inside_page()

[Wu Fengguang]

On Sat, Sep 12, 2009 at 07:55:43AM +0800, Andrew Morton wrote:
> On Fri, 11 Sep 2009 10:23:35 +0800
> Wu Fengguang <fengguang.wu@intel.com> 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@redhat.com>
> > CC: Greg Kroah-Hartman <gregkh@suse.de>
> > CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > CC: Johannes Berg <johannes@sipsolutions.net>
> > CC: Avi Kivity <avi@qumranet.com>
> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > ---
> >  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.

OK. See the followed patches.

> > +	else
> > +		sz = PAGE_SIZE;
> > +
> > +	return min_t(unsigned long, sz, size);
> 
> Can use min() here.

Done.

> > +}
> 
> 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.

I tend to just use unsigned long because even though the value itself
is small, it will be elevated to unsigned long in majority use cases.

Does that make sense?

> 
> > @@ -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?

Err, changed it to unsigned long sz.

Thanks,
Fengguang

Re: [PATCH 3/3] devmem: cleanup unxlate_dev_mem_ptr() calls

[Wu Fengguang]

On Sat, Sep 12, 2009 at 08:05:00AM +0800, Andrew Morton wrote:
> On Fri, 11 Sep 2009 10:23:36 +0800
> Wu Fengguang <fengguang.wu@intel.com> wrote:
> 
> > No behavior change.
> > 
> > CC: Marcelo Tosatti <mtosatti@redhat.com>
> > CC: Greg Kroah-Hartman <gregkh@suse.de>
> > CC: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > CC: Johannes Berg <johannes@sipsolutions.net>
> > CC: Avi Kivity <avi@qumranet.com>
> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > ---
> >  drivers/char/mem.c |   13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > --- linux-mm.orig/drivers/char/mem.c	2009-09-10 21:59:39.000000000 +0800
> > +++ linux-mm/drivers/char/mem.c	2009-09-10 22:00:12.000000000 +0800
> > @@ -131,6 +131,7 @@ static ssize_t read_mem(struct file * fi
> >  			size_t count, loff_t *ppos)
> >  {
> >  	unsigned long p = *ppos;
> > +	unsigned long ret;
> >  	ssize_t read, sz;
> >  	char *ptr;
> >  
> > @@ -169,12 +170,10 @@ static ssize_t read_mem(struct file * fi
> >  		if (!ptr)
> >  			return -EFAULT;
> >  
> > -		if (copy_to_user(buf, ptr, sz)) {
> > -			unxlate_dev_mem_ptr(p, ptr);
> > -			return -EFAULT;
> > -		}
> > -
> > +		ret = copy_to_user(buf, ptr, sz);
> >  		unxlate_dev_mem_ptr(p, ptr);
> > +		if (ret)
> > +			return -EFAULT;
> >  
> >  		buf += sz;
> >  		p += sz;
> 
> - local var `ret' didn't need function-wide scope.  I think it's
>   better to reduce its scope if poss.

OK.

> - conventionally the identifier `ret' refers to "the value which this
>   function will return".  Ditto `retval' and `rc'.

Good to know that, thanks!

>   But that's not what `ret' does here so let's call it something
>   else?  `remaining' is rather verbose and formal, but accurate.
> 
> 
> --- a/drivers/char/mem.c~dev-mem-cleanup-unxlate_dev_mem_ptr-calls-fix
> +++ a/drivers/char/mem.c
> @@ -131,7 +131,6 @@ static ssize_t read_mem(struct file * fi
>  			size_t count, loff_t *ppos)
>  {
>  	unsigned long p = *ppos;
> -	unsigned long ret;

I got compile error because another place in read_mem() used ret.
I'll send you an updated fix.

Thanks,
Fengguang

>  	ssize_t read, sz;
>  	char *ptr;
>  
> @@ -156,6 +155,8 @@ static ssize_t read_mem(struct file * fi
>  #endif
>  
>  	while (count > 0) {
> +		unsigned long remaining;
> +
>  		sz = size_inside_page(p, count);
>  
>  		if (!range_is_allowed(p >> PAGE_SHIFT, count))
> @@ -170,9 +171,9 @@ static ssize_t read_mem(struct file * fi
>  		if (!ptr)
>  			return -EFAULT;
>  
> -		ret = copy_to_user(buf, ptr, sz);
> +		remaining = copy_to_user(buf, ptr, sz);
>  		unxlate_dev_mem_ptr(p, ptr);
> -		if (ret)
> +		if (remaining)
>  			return -EFAULT;
>  
>  		buf += sz;
> _