[U-Boot] [PATCH v2] memsize: Fix for bug in memory sizing code
The original memory sizing code in get_ram_size clobbers the word at the base address, but forgets to restore it.
Unlike the (reverted) commit b8496cced856ff411f1eb2e4eff20f5abe7080b0 we'll go save and restore the base address value at the start and then end of the function. It needs to stay cleared until the detection is done, otherwise we'll fail to detect the same piece of memory being mapped multiple times into the address space.
Cc: Wolfgang Denk wd@denx.de Cc: Iwo Mergler Iwo.Mergler@netcommwireless.com Signed-off-by: Gerd Hoffmann kraxel@redhat.com --- common/memsize.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/common/memsize.c b/common/memsize.c index 589400d..72d0156 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -27,12 +27,15 @@ DECLARE_GLOBAL_DATA_PTR; long get_ram_size(long *base, long maxsize) { volatile long *addr; - long save[32]; + long save[32], save_base; long cnt; long val; long size; int i = 0;
+ save_base = base[0]; + sync (); + for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ sync (); @@ -43,8 +46,6 @@ long get_ram_size(long *base, long maxsize)
addr = base; sync (); - save[i] = *addr; - sync (); *addr = 0;
sync (); @@ -52,12 +53,12 @@ long get_ram_size(long *base, long maxsize) /* Restore the original data before leaving the function. */ sync (); - *addr = save[i]; for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) { addr = base + cnt; sync (); *addr = save[--i]; } + base[0] = save_base; return (0); }
@@ -73,10 +74,12 @@ long get_ram_size(long *base, long maxsize) addr = base + cnt; *addr = save[--i]; } + base[0] = save_base; return (size); } }
+ base[0] = save_base; return (maxsize); }
Prepare code to make later modifications checkpatch-clean.
Signed-off-by: Wolfgang Denk wd@denx.de --- common/memsize.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/common/memsize.c b/common/memsize.c index 589400d..0fb9ba5 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -33,43 +33,46 @@ long get_ram_size(long *base, long maxsize) long size; int i = 0;
- for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) { + for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ - sync (); + sync(); save[i++] = *addr; - sync (); + sync(); *addr = ~cnt; }
addr = base; - sync (); + sync(); save[i] = *addr; - sync (); + sync(); *addr = 0;
- sync (); + sync(); if ((val = *addr) != 0) { - /* Restore the original data before leaving the function. - */ - sync (); + /* Restore the original data before leaving the function. */ + sync(); *addr = save[i]; for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) { addr = base + cnt; - sync (); + sync(); *addr = save[--i]; } return (0); }
- for (cnt = 1; cnt < maxsize / sizeof (long); cnt <<= 1) { + for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) { addr = base + cnt; /* pointer arith! */ val = *addr; *addr = save[--i]; if (val != ~cnt) { - size = cnt * sizeof (long); - /* Restore the original data before leaving the function. + size = cnt * sizeof(long); + /* + * Restore the original data + * before leaving the function. */ - for (cnt <<= 1; cnt < maxsize / sizeof (long); cnt <<= 1) { + for (cnt <<= 1; + cnt < maxsize / sizeof(long); + cnt <<= 1) { addr = base + cnt; *addr = save[--i]; }
The original memory sizing code in get_ram_size clobbers the word at the base address, but forgets to restore it.
Unlike the (reverted) commit b8496cced856ff411f1eb2e4eff20f5abe7080b0 we save and restore the base address value at the start resp. at the end of the function. It needs to stay cleared until the detection is done, otherwise it will fail to detect the same piece of memory being mapped multiple times into the address space.
Signed-off-by: Gerd Hoffmann kraxel@redhat.com Signed-off-by: Wolfgang Denk wd@denx.de Cc: Iwo Mergler Iwo.Mergler@netcommwireless.com --- common/memsize.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/common/memsize.c b/common/memsize.c index 0fb9ba5..d5827dd 100644 --- a/common/memsize.c +++ b/common/memsize.c @@ -1,5 +1,5 @@ /* - * (C) Copyright 2004 + * (C) Copyright 2004-2014 * Wolfgang Denk, DENX Software Engineering, wd@denx.de. * * SPDX-License-Identifier: GPL-2.0+ @@ -31,7 +31,7 @@ long get_ram_size(long *base, long maxsize) long cnt; long val; long size; - int i = 0; + int i = 0, last;
for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ @@ -44,6 +44,7 @@ long get_ram_size(long *base, long maxsize) addr = base; sync(); save[i] = *addr; + last = i; sync(); *addr = 0;
@@ -51,7 +52,7 @@ long get_ram_size(long *base, long maxsize) if ((val = *addr) != 0) { /* Restore the original data before leaving the function. */ sync(); - *addr = save[i]; + *addr = save[last]; for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) { addr = base + cnt; sync(); @@ -62,7 +63,9 @@ long get_ram_size(long *base, long maxsize)
for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) { addr = base + cnt; /* pointer arith! */ + sync(); val = *addr; + sync(); *addr = save[--i]; if (val != ~cnt) { size = cnt * sizeof(long); @@ -74,12 +77,19 @@ long get_ram_size(long *base, long maxsize) cnt < maxsize / sizeof(long); cnt <<= 1) { addr = base + cnt; + sync(); *addr = save[--i]; } + addr = base; + sync(); + *addr = save[last]; return (size); } }
+ addr = base; + sync(); + *addr = save[last]; return (maxsize); }
Note: there is no patch 3/3 in his series.
It was originally
[PATCH] powerpc: TQM5200: convert to generic board
but this is actually not related to these changes here, so I decided to post it independently.
Sorry for the confusion.
Best regards,
Wolfgang Denk
Hi,
Signed-off-by: Gerd Hoffmann kraxel@redhat.com
No, I didn't signed this patch off. And I will not because it is broken.
for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ @@ -44,6 +44,7 @@ long get_ram_size(long *base, long maxsize) addr = base; sync(); save[i] = *addr;
base[0] might not be the original value any more at this point ...
- addr = base;
- sync();
- *addr = save[last]; return (maxsize);
... so this may corrupt memory.
cheers, Gerd
On Tue, Oct 21, 2014 at 10:14:10PM +0200, Wolfgang Denk wrote:
Prepare code to make later modifications checkpatch-clean.
Signed-off-by: Wolfgang Denk wd@denx.de
Applied to u-boot/master, thanks!
Dear Gerd Hoffmann,
In message 1413910153-5907-1-git-send-email-kraxel@redhat.com you wrote:
The original memory sizing code in get_ram_size clobbers the word at the base address, but forgets to restore it.
The funny thing is that it avtually works on some boards. Do you have an explanation for that?
- long save[32];
- long save[32], save_base;
Why do you need another variable? The original code stores the value as last entry in save[] - what's wrong with that?
- save_base = base[0];
- sync ();
You add this code here, but...
for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ sync (); @@ -43,8 +46,6 @@ long get_ram_size(long *base, long maxsize)
addr = base; sync ();
- save[i] = *addr;
- sync ();
...remove it's equivalent here. Why would your code be any better than the existing one?
*addr = save[i];
base[0] = save_base;
Same here - the line you removed above does the very same as the one you add here. In which way is the new code supposed to be different or even better?
@@ -73,10 +74,12 @@ long get_ram_size(long *base, long maxsize) addr = base + cnt; *addr = save[--i]; }
base[0] = save_base; return (size);} }
base[0] = save_base; return (maxsize);
Agreed here. These two make sense to me. I still wonder why it works on the boards I used for testing, while it's failing on yours.
Which exit path are you taking? The one at the end of the function, i. e. "return (maxsize)" ? What happens if you double the memory size to be checked?
I'll resend a slightly reworked patch in a minute; could you please test if this works for you, too?
Thanks.
Best regards,
Wolfgang Denk
On Di, 2014-10-21 at 22:14 +0200, Wolfgang Denk wrote:
Dear Gerd Hoffmann,
In message 1413910153-5907-1-git-send-email-kraxel@redhat.com you wrote:
The original memory sizing code in get_ram_size clobbers the word at the base address, but forgets to restore it.
The funny thing is that it avtually works on some boards. Do you have an explanation for that?
Sure. If the same piece of memory appears multiple times in the address space the base address value is saved and restored multiple times too.
- long save[32];
- long save[32], save_base;
Why do you need another variable? The original code stores the value as last entry in save[] - what's wrong with that?
last entry index isn't known at the places where I save/restore the value, so I did it this way. Alternative approach would be to use the first index instead and shift all the other values by one. Matter of taste, both will get the job done.
- save_base = base[0];
- sync ();
You add this code here, but...
for (cnt = (maxsize / sizeof (long)) >> 1; cnt > 0; cnt >>= 1) { addr = base + cnt; /* pointer arith! */ sync (); @@ -43,8 +46,6 @@ long get_ram_size(long *base, long maxsize)
addr = base; sync ();
- save[i] = *addr;
- sync ();
...remove it's equivalent here. Why would your code be any better than the existing one?
There can be multiple saves/restores for the base memory location (see above). Therefore the ordering matters, restore must use the reverse order of save. No exception for base[0], so when we restore it last we must save it first.
*addr = save[i];
base[0] = save_base;Same here - the line you removed above does the very same as the one you add here. In which way is the new code supposed to be different or even better?
... and we must restore it last everywhere.
@@ -73,10 +74,12 @@ long get_ram_size(long *base, long maxsize) addr = base + cnt; *addr = save[--i]; }
base[0] = save_base; return (size);} }
base[0] = save_base; return (maxsize);
Agreed here. These two make sense to me. I still wonder why it works on the boards I used for testing, while it's failing on yours.
Which exit path are you taking? The one at the end of the function, i. e. "return (maxsize)" ? What happens if you double the memory size to be checked?
Both cases can happen, depending on how much memory I configure for qemu, and both are working correctly for me.
cheers, Gerd
participants (3)
-
Gerd Hoffmann -
Tom Rini -
Wolfgang Denk