Re: [PATCH] powerpc: fix large nvram access

Re: [PATCH] powerpc: fix large nvram access

[Olaf Hering]

 On Mon, Jan 09, Linux Kernel Mailing List wrote:

> tree da8f883f72d08f9534e9eca3bba662413b9bd865
> parent d52771fce4e774fa786097d34412a057d487c697
> author Arnd Bergmann <arnd@arndb.de> Fri, 09 Dec 2005 19:21:44 +0100
> committer Paul Mackerras <paulus@samba.org> Mon, 09 Jan 2006 14:53:31 +1100
> 
> [PATCH] powerpc: fix large nvram access
> 
> /dev/nvram uses the user-provided read/write size
> for kmalloc, which fails, if a large number is passed.
> This will always use a single page at most, which
> can be expected to succeed.
> 
> Signed-off-by: Arnd Bergmann <arndb@de.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> 
>  arch/powerpc/kernel/nvram_64.c |  114 +++++++++++++++++++----------------------

this change breaks my nvsetenv binary on JS20.

(none):/# mount proc ; nvsetenv
Error reading /dev/nvram: Success
(none):/# strace -f nvsetenv
execve("/sbin/nvsetenv", ["nvsetenv"], [/* 61 vars */]) = 0
brk(0)                                  = 0x10014000
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf7fe0000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY)      = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=171714, ...}) = 0
mmap(NULL, 171714, PROT_READ, MAP_PRIVATE, 3, 0) = 0xf7fb6000
close(3)                                = 0
open("/lib/libc.so.6", O_RDONLY)        = 3
read(3, "\177ELF\1\2\1\0\0\0\0\0\0\0\0\0\0\3\0\24\0\0\0\1\0\1\325"..., 512) = 512
fstat64(3, {st_mode=S_IFREG|0755, st_size=1569515, ...}) = 0
mmap(0xfe99000, 1401836, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0xfe99000
madvise(0xfe99000, 1401836, MADV_SEQUENTIAL|0x1) = 0
mprotect(0xffd8000, 65536, PROT_NONE)   = 0
mmap(0xffe8000, 20480, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x13f000) = 0xffe8000
mmap(0xffed000, 9196, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0xffed000
close(3)                                = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf7fb5000
mprotect(0xffe8000, 4096, PROT_READ)    = 0
munmap(0xf7fb6000, 171714)              = 0
brk(0)                                  = 0x10014000
brk(0x10035000)                         = 0x10035000
open("/proc/device-tree/compatible", O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0444, st_size=13, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf7fdf000
read(3, "IBM,1234-123\0", 1024)         = 13
read(3, "", 1024)                       = 0
close(3)                                = 0
munmap(0xf7fdf000, 4096)                = 0
open("/dev/nvram", O_RDONLY)            = 3
read(3, "P\347\0\34of-config\0\0\0", 16) = 16
lseek(3, 432, SEEK_CUR)                 = 448
read(3, "p\375\2\0common\0\0\0\0\0\0", 16) = 16
read(3, "ibm,fw-phandle-enable?=false\0lit"..., 8176) = 4096
dup(2)                                  = 4
fcntl64(4, F_GETFL)                     = 0x2 (flags O_RDWR)
fstat64(4, {st_mode=S_IFCHR|0600, st_rdev=makedev(5, 1), ...}) = 0
ioctl(4, TCGETS, {B38400 opost isig icanon echo ...}) = 0
mmap(NULL, 131072, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf7fc0000
_llseek(4, 0, 0xffd63fd8, SEEK_CUR)     = -1 ESPIPE (Illegal seek)
write(4, "Error reading /dev/nvram: Succes"..., 34Error reading /dev/nvram: Success
) = 34
close(4)                                = 0
munmap(0xf7fc0000, 131072)              = 0
exit_group(1)                           = ?
(none):/# 


-- 
short story of a lazy sysadmin:
 alias appserv=wotan

Re: [PATCH] powerpc: fix large nvram access

[Andreas Schwab]

Olaf Hering <olh@suse.de> writes:

> this change breaks my nvsetenv binary on JS20.
>
> (none):/# mount proc ; nvsetenv
> Error reading /dev/nvram: Success

It was broken to begin with.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] powerpc: fix large nvram access

[Arnd Bergmann]

Am Dienstag, 10. Januar 2006 23:13 schrieb Olaf Hering:
>  On Mon, Jan 09, Linux Kernel Mailing List wrote:
> > tree da8f883f72d08f9534e9eca3bba662413b9bd865
> > parent d52771fce4e774fa786097d34412a057d487c697
> > author Arnd Bergmann <arnd@arndb.de> Fri, 09 Dec 2005 19:21:44 +0100
> > committer Paul Mackerras <paulus@samba.org> Mon, 09 Jan 2006 14:53:31
> > +1100
> >
> > [PATCH] powerpc: fix large nvram access
> >
> > /dev/nvram uses the user-provided read/write size
> > for kmalloc, which fails, if a large number is passed.
> > This will always use a single page at most, which
> > can be expected to succeed.
> >
> > Signed-off-by: Arnd Bergmann <arndb@de.ibm.com>
> > Signed-off-by: Paul Mackerras <paulus@samba.org>
> >
> >  arch/powerpc/kernel/nvram_64.c |  114
> > +++++++++++++++++++----------------------
>
> this change breaks my nvsetenv binary on JS20.
>

I think this has uncovered a bug that has been in nvsetenv for a long time.

> open("/dev/nvram", O_RDONLY)            = 3
> read(3, "P\347\0\34of-config\0\0\0", 16) = 16
> lseek(3, 432, SEEK_CUR)                 = 448
> read(3, "p\375\2\0common\0\0\0\0\0\0", 16) = 16
> read(3, "ibm,fw-phandle-enable?=false\0lit"..., 8176) = 4096

This is where my patch changed the behavior of the driver. It used to return all the data in one chunk, now it returns 4096 bytes at most.

> dup(2)                                  = 4
> fcntl64(4, F_GETFL)                     = 0x2 (flags O_RDWR)
> fstat64(4, {st_mode=S_IFCHR|0600, st_rdev=makedev(5, 1), ...}) = 0
> ioctl(4, TCGETS, {B38400 opost isig icanon echo ...}) = 0
> mmap(NULL, 131072, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xf7fc0000 
> _llseek(4, 0, 0xffd63fd8, SEEK_CUR)     = -1 ESPIPE 
> (Illegal seek) 
> write(4, "Error reading /dev/nvram: Succes"..., 34Error reading  /dev/nvram: Success ) = 34
> close(4)                                = 0
> munmap(0xf7fc0000, 131072)              = 0
> exit_group(1)                           = ?
> (none):/#

And then it got here:

|    if (lseek(nvfd, NVSTART, 0) < 0
|        || read(nvfd, &nvbuf, NVSIZE) != NVSIZE) {
|        perror("Error reading /dev/nvram");
|        exit(EXIT_FAILURE);
|    }

I'm pretty sure that the failing _llseek is part of the perror 
implementation and not the real problem, as we were first thinking.

The problem is that nvsetenv bails out on a short read, while according 
to the man page, "It is not an error if this number is smaller than the
number of bytes requested". Of course, character devices often differ
in their behavior from what is in the man pages for file I/O.

I'd suggest we change both ends: nvsetenv should really be able to deal
with a short read, and we can double the size of the kmalloc buffer
for the kernel implementation. Please test the patch below.

---

Subject: powerpc: fix nvsetenv regression with fixed nvram read/write

reading from /dev/nvram was recently fixed to not try to allocate the
user-defined amount of kernel memory. This broke the nvsetenv tool
which relied on never getting short reads from /dev/nvram.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index fd7db8d..8da6e57 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -34,6 +34,15 @@
 
 #undef DEBUG_NVRAM
 
+/*
+ * Old nvsetenv versions expect us to be able to return 8kb
+ * of data in a single read, so use that as a limit for short
+ * reads.
+ * We can't simply use the requested size because large
+ * kmalloc allocations often fail.
+ */
+#define NVRAM_ALLOC_SIZE 8192
+
 static int nvram_scan_partitions(void);
 static int nvram_setup_partition(void);
 static int nvram_create_os_partition(void);
@@ -94,7 +103,7 @@ static ssize_t dev_nvram_read(struct fil
 		goto out;
 
 	count = min_t(size_t, count, size - *ppos);
-	count = min(count, PAGE_SIZE);
+	count = min(count, NVRAM_ALLOC_SIZE);
 
 	ret = -ENOMEM;
 	tmp = kmalloc(count, GFP_KERNEL);
@@ -131,7 +140,7 @@ static ssize_t dev_nvram_write(struct fi
 		goto out;
 
 	count = min_t(size_t, count, size - *ppos);
-	count = min(count, PAGE_SIZE);
+	count = min(count, NVRAM_ALLOC_SIZE);
 
 	ret = -ENOMEM;
 	tmp = kmalloc(count, GFP_KERNEL);

Re: [PATCH] powerpc: fix large nvram access

[Olaf Hering]

 On Wed, Jan 11, Arnd Bergmann wrote:

> Am Dienstag, 10. Januar 2006 23:13 schrieb Olaf Hering:
> >  On Mon, Jan 09, Linux Kernel Mailing List wrote:
> > > tree da8f883f72d08f9534e9eca3bba662413b9bd865
> > > parent d52771fce4e774fa786097d34412a057d487c697
> > > author Arnd Bergmann <arnd@arndb.de> Fri, 09 Dec 2005 19:21:44 +0100
> > > committer Paul Mackerras <paulus@samba.org> Mon, 09 Jan 2006 14:53:31
> > > +1100
> > >
> > > [PATCH] powerpc: fix large nvram access
> > >
> > > /dev/nvram uses the user-provided read/write size
> > > for kmalloc, which fails, if a large number is passed.
> > > This will always use a single page at most, which
> > > can be expected to succeed.
> > >
> > > Signed-off-by: Arnd Bergmann <arndb@de.ibm.com>
> > > Signed-off-by: Paul Mackerras <paulus@samba.org>
> > >
> > >  arch/powerpc/kernel/nvram_64.c |  114
> > > +++++++++++++++++++----------------------
> >
> > this change breaks my nvsetenv binary on JS20.
> >
> 
> I think this has uncovered a bug that has been in nvsetenv for a long time.

Yeah. Dont bother, I will fix that tool.

-- 
short story of a lazy sysadmin:
 alias appserv=wotan

Re: [PATCH] powerpc: fix large nvram access

[Olaf Hering]

 On Wed, Jan 11, Olaf Hering wrote:

> > I think this has uncovered a bug that has been in nvsetenv for a long time.

I made this change.

Index: pmac-utils-2.1/nvsetenv.c
===================================================================
--- pmac-utils-2.1.orig/nvsetenv.c
+++ pmac-utils-2.1/nvsetenv.c
@@ -110,12 +110,21 @@ int nvcsum( void )
 static void nvload( int nvfd )
 {
     int s;
+    ssize_t r, toread = 0;
+    char *buf = (char *)&nvbuf;
 
-    if (lseek(nvfd, NVSTART, 0) < 0
-	|| read(nvfd, &nvbuf, NVSIZE) != NVSIZE) {
-	perror("Error reading /dev/nvram");
+    if ((off_t)-1 == lseek(nvfd, NVSTART, 0)) {
+	perror("Error seeking /dev/nvram");
 	exit(EXIT_FAILURE);
     }
+    do {
+	r = read(nvfd, buf + toread, NVSIZE - toread);
+	if (r < 0) {
+		perror("Error reading /dev/nvram");
+		exit(EXIT_FAILURE);
+	}
+	toread += r;
+    } while (toread < NVSIZE && r);
     if (nvbuf.nv.magic != NVMAGIC)
 	(void) fprintf(stderr, "Warning: Bad magic number %x\n",
 			 nvbuf.nv.magic);
@@ -127,11 +136,21 @@ static void nvload( int nvfd )
 
 static void nvstore(int nvfd)
 {
-    if (lseek(nvfd, NVSTART, 0) < 0
-	|| write(nvfd, &nvbuf, NVSIZE) != NVSIZE) {
-	perror("Error writing /dev/nvram");
+    char *p = (char *)&nvbuf;
+    size_t count = 0;
+    off_t w;
+    if (lseek(nvfd, NVSTART, 0) < 0) {
+	perror("Error seeking /dev/nvram");
 	exit(EXIT_FAILURE);
     }
+    do {
+	w = write(nvfd, p + count, NVSIZE - count);
+	if (w < 0) {
+		perror("Error writing /dev/nvram");
+		exit(EXIT_FAILURE);
+	}
+	count += w;
+   } while (count < NVSIZE);
 }
 
 void nvunpack( void )
Index: pmac-utils-2.1/nwnvsetenv.c
===================================================================
--- pmac-utils-2.1.orig/nwnvsetenv.c
+++ pmac-utils-2.1/nwnvsetenv.c
@@ -88,15 +88,20 @@ int nvscan(int nvfd, chrp_header* chrph,
 static char* nvload( int nvfd , int nvsize)
 {
     char* nvbuf = malloc(nvsize);
+    ssize_t r, toread = 0;
 
     if (!nvbuf) {
 	perror("Error allocating buffer");
 	exit(EXIT_FAILURE);
     }
-    if (read(nvfd, nvbuf, nvsize) != nvsize) {
-	perror("Error reading /dev/nvram");
-	exit(EXIT_FAILURE);
-    }
+    do {
+	r = read(nvfd, nvbuf + toread, nvsize - toread);
+	if (r < 0) {
+		perror("Error reading /dev/nvram");
+		exit(EXIT_FAILURE);
+	}
+	toread += r;
+    } while (toread < nvsize && r);
     return nvbuf;
 }
 
@@ -182,6 +187,7 @@ print_var(char* nvbuf, int nvsize, char*
 	printf("%s\n",buf);
 }
 
+#if 0
 /* This fucntion is not used here, it is left
    her for the curious */
 
@@ -197,26 +203,28 @@ unsigned short chrp_checksum(chrp_header
 	sum = (sum & 0xFF) + (sum>>8);
     return sum;
 }                                                                                                 
+#endif
 
 static void
 nvstore(int nvfd, chrp_header* chrph, char* nvbuf, int nvstart, int nvsize)
 {
     // mmh, the checksum is calculated for the header only
     // since we did not modify the header we can just ignore it.
-    ssize_t written;
-    ssize_t seek =  nvstart + sizeof(chrp_header);
-    written = lseek(nvfd, seek, SEEK_SET);
-    if (written != seek)
-    {
+    size_t count = 0;
+    off_t w, seek =  nvstart + sizeof(chrp_header);
+    w = lseek(nvfd, seek, SEEK_SET);
+    if (w != seek) {
     	fprintf(stderr,"Error seeking /dev/nvram\n");
 	exit(EXIT_FAILURE);
     }
-    written = write(nvfd, nvbuf, nvsize);
-    if (written != nvsize)
-    {
-    	fprintf(stderr,"Error writing /dev/nvram %x %x\n", nvsize, seek);
-	exit(EXIT_FAILURE);
-    }
+    do {
+	w = write(nvfd, nvbuf + count, nvsize - count);
+	if (w < 0) {
+		perror("Error writing /dev/nvram");
+		exit(EXIT_FAILURE);
+	}
+	count += w;
+   } while (count < (size_t)nvsize);
 }
 
 /* print / set the New World NVRAM */

-- 
short story of a lazy sysadmin:
 alias appserv=wotan