Hi Simon,
On 9/2/24 12:26 AM, Simon Glass wrote:
This unmaps a different address from what was mapped. Fix it.
Signed-off-by: Simon Glass <s...@chromium.org>
---
(no changes since v1)
cmd/mem.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/cmd/mem.c b/cmd/mem.c
index 274348068c2..4d6fde28531 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -245,7 +245,7 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int
argc,
int size;
int rcode = 0;
const char *type;
- const void *buf1, *buf2, *base;
+ const void *buf1, *buf2, *base, *ptr1, *ptr2;
ulong word1, word2; /* 64-bit if MEM_SUPPORT_64BIT_DATA */
if (argc != 4)
@@ -270,22 +270,22 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag,
int argc,
bytes = size * count;
base = buf1 = map_sysmem(addr1, bytes);
"base" isn't changed in the rest of the code, so we could just reuse it
instead of declaring yet another variable.
buf2 = map_sysmem(addr2, bytes);
We could also set ptr2 here... Allowing to avoid the diff from here to....
- for (ngood = 0; ngood < count; ++ngood) {
+ for (ngood = 0, ptr1 = buf1, ptr2 = buf2; ngood < count; ++ngood) { >
if (size == 4) {
- word1 = *(u32 *)buf1;
- word2 = *(u32 *)buf2;
+ word1 = *(u32 *)ptr1;
+ word2 = *(u32 *)ptr2;
} else if (MEM_SUPPORT_64BIT_DATA && size == 8) {
- word1 = *(ulong *)buf1;
- word2 = *(ulong *)buf2;
+ word1 = *(ulong *)ptr1;
+ word2 = *(ulong *)ptr2;
} else if (size == 2) {
- word1 = *(u16 *)buf1;
- word2 = *(u16 *)buf2;
+ word1 = *(u16 *)ptr1;
+ word2 = *(u16 *)ptr2;
} else {
- word1 = *(u8 *)buf1;
- word2 = *(u8 *)buf2;
+ word1 = *(u8 *)ptr1;
+ word2 = *(u8 *)ptr2;
}
if (word1 != word2) {
- ulong offset = buf1 - base;
+ ulong offset = ptr1 - base;
printf("%s at 0x%08lx (%#0*lx) != %s at 0x%08lx
(%#0*lx)\n",
type, (ulong)(addr1 + offset), size, word1,
type, (ulong)(addr2 + offset), size, word2);
@@ -293,8 +293,8 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int
argc,
break;
}
- buf1 += size;
- buf2 += size;
+ ptr1 += size;
+ ptr2 += size;
here - making the commit all the more explicit (for me this commit is
basically only renaming a variable, since unmap_system doesn't appear in
the git context) - by only changing:
unmap_system(buf1);
unmap_system(buf2);
to
unmap_system(base);
unmap_system(ptr2);
I believe?
Additionally, my linter tells me that:
buf1 += size;
buf2 += size;
is undefined behavior:
arithOperationsOnVoidPointer: 'buf1' is of type 'const void *'. When
using void pointers in calculations, the behaviour is undefined.
I suggest the following:
buf1 = ((u8 *)buf1) + size;
buf2 = ((u8 *)buf2) + size;
since size seems to be size in bytes?
What do you think?
We already have test/cmd/mem.c is this something we can augment to test
the unmapping is proper too?
Cheers,
Quentin
