Re: [PATCH v4 19/23] cmd: Fix memory-mapping in cmp command

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