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

[Quentin Schulz <quentin.schulz@cherry.de>]

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