| Message ID | 20240901222634.460873-20-sjg@chromium.org |
|---|---|
| State | New |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fix various bugs | expand |
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 <sjg@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
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); buf2 = map_sysmem(addr2, bytes); - 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; /* reset watchdog from time to time */ if ((ngood % (64 << 10)) == 0)
This unmaps a different address from what was mapped. Fix it. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) cmd/mem.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)