[PATCH v3] cmd: mem: fix range of bitflip test

[Stefan Roese]

On 09.09.20 17:21, Ralph Siemsen wrote:
> The bitflip test uses two equal sized memory buffers. This is achieved
> by splitting the range of memory into two pieces. The address of the
> second buffer, as well as the length of each buffer, were not correctly
> calculated. This caused bitflip test to access beyond the end of range.
> This patch fixes the pointer arithmetic problem.
> 
> A second problem arises because u-boot "mtest" command expects the
> ending address to be inclusive. When computing (end - start) this
> results in missing 1 byte of the requested length. The bitflip test
> expects a count rather than an "ending" address. Thus it fails to test
> the last word of the requested range. Fixed by using (end - start + 1).
> 
> Added Kconfig option to optionally disable the bitflip test, since it
> does add significantly to the time taken for "mtest".
> 
> Fixes: 8e434cb705d463bc8cff935160e4fb4c77cb99ab ("cmd: mem: Add bitflip
> memory test to alternate mtest")
> 
> Signed-off-by: Ralph Siemsen <ralph.siemsen at linaro.org>
> --
> 
> Changes in v3:
> - Add Kconfig option to disable bitflip test
> - Refactor the fix into a helper function
> 
> Change-Id: Ie641d04e731fc5bc6a3bbef914bf7fad136cdc94
> ---
>   cmd/Kconfig | 12 ++++++++++++
>   cmd/mem.c   | 24 ++++++++++++++++++++----
>   2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index d54acf2cfd..275bf7fbfe 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -779,6 +779,18 @@ config SYS_ALT_MEMTEST
>   	help
>   	  Use a more complete alternative memory test.
>   
> +if SYS_ALT_MEMTEST
> +
> +config SYS_ALT_MEMTEST_BITFLIP
> +	bool "Bitflip test"
> +	default y
> +	help
> +	  The alternative memory test includes bitflip test since 2020.07.
> +	  The bitflip test significantly increases the overall test time.
> +	  Bitflip test can optionally be disabled here.
> +
> +endif
> +
>   config SYS_MEMTEST_START
>   	hex "default start address for mtest"
>   	default 0
> diff --git a/cmd/mem.c b/cmd/mem.c
> index 9b97f7bf69..63c4bedf24 100644
> --- a/cmd/mem.c
> +++ b/cmd/mem.c
> @@ -814,6 +814,7 @@ static ulong mem_test_alt(vu_long *buf, ulong start_addr, ulong end_addr,
>   	return errs;
>   }
>   
> +#ifdef CONFIG_SYS_ALT_MEMTEST_BITFLIP
>   static int compare_regions(volatile unsigned long *bufa,
>   			   volatile unsigned long *bufb, size_t count)
>   {
> @@ -867,6 +868,24 @@ static ulong test_bitflip_comparison(volatile unsigned long *bufa,
>   	return errs;
>   }
>   
> +static ulong mem_test_bitflip(vu_long *buf, ulong start, ulong end)
> +{
> +	/*
> +	 * Split the specified range into two halves.
> +	 * Note that mtest range is inclusive of start,end.
> +	 * Bitflip test instead uses a count (of 32-bit words).
> +	 */
> +	ulong half_size = (end - start + 1) / 2 / sizeof(unsigned long);
> +
> +	return test_bitflip_comparison(buf, buf + half_size, half_size);
> +}
> +#else
> +static ulong mem_test_bitflip(vu_long *buf, ulong start, ulong end)
> +{
> +	return 0;
> +}
> +#endif

We don't want to add #ifdef's to the code if possible. How about this
change:

Add this function without #ifdef and drop the empty implementation
and ...

> +
>   static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
>   			    vu_long pattern, int iteration)
>   {
> @@ -987,10 +1006,7 @@ static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
>   			if (errs == -1UL)
>   				break;
>   			count += errs;
> -			errs = test_bitflip_comparison(buf,
> -						       buf + (end - start) / 2,
> -						       (end - start) /
> -						       sizeof(unsigned long));
> +			errs = mem_test_bitflip(buf, start, end);

Use this here:

			if (IS_ENABLED(CONFIG_SYS_ALT_MEMTEST_BITFLIP))
				errs = mem_test_bitflip(buf, start, end);

Does this work?

Thanks,
Stefan