[PATCH 1/4] cmd: mem: Correctly count the errors in mtest

[PATCH 1/4] cmd: mem: Correctly count the errors in mtest

[Stefan Roese]

This patch changes mtest to correctly count the overall errors and
print them even in the abort (Ctrl-C) case.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 cmd/mem.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/cmd/mem.c b/cmd/mem.c
index 6d54f19527..9367278aa8 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -871,7 +871,7 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 	ulong start, end;
 	vu_long *buf, *dummy;
 	ulong iteration_limit = 0;
-	int ret;
+	ulong count = 0;
 	ulong errs = 0;	/* number of errors, or -1 if interrupted */
 	ulong pattern = 0;
 	int iteration;
@@ -929,6 +929,7 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 		}
 		if (errs == -1UL)
 			break;
+		count += errs;
 	}
 
 	/*
@@ -947,14 +948,10 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (errs == -1UL) {
 		/* Memory test was aborted - write a newline to finish off */
 		putc('\n');
-		ret = 1;
-	} else {
-		printf("Tested %d iteration(s) with %lu errors.\n",
-			iteration, errs);
-		ret = errs != 0;
 	}
+	printf("Tested %d iteration(s) with %lu errors.\n", iteration, count);
 
-	return ret;
+	return errs != 0;
 }
 #endif	/* CONFIG_CMD_MEMTEST */
 
-- 
2.25.1

[PATCH 2/4] cmd: mem: Drop eldk-4.2 workaround and use cast in unmap_sysmem()

[Stefan Roese]

Use a cast instead of the "eldk-4.2" workaround for unmap_sysmem().

Signed-off-by: Stefan Roese <sr@denx.de>
---
 cmd/mem.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/cmd/mem.c b/cmd/mem.c
index 9367278aa8..f519adaee2 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -932,18 +932,8 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 		count += errs;
 	}
 
-	/*
-	 * Work-around for eldk-4.2 which gives this warning if we try to
-	 * case in the unmap_sysmem() call:
-	 * warning: initialization discards qualifiers from pointer target type
-	 */
-	{
-		void *vbuf = (void *)buf;
-		void *vdummy = (void *)dummy;
-
-		unmap_sysmem(vbuf);
-		unmap_sysmem(vdummy);
-	}
+	unmap_sysmem((void *)buf);
+	unmap_sysmem((void *)dummy);
 
 	if (errs == -1UL) {
 		/* Memory test was aborted - write a newline to finish off */
-- 
2.25.1

[PATCH 3/4] cmd: mem: Use IS_ENABLED instead of alt_test variable

[Stefan Roese]

This patch uses the IS_ENABLED() macro to check, which mtest variant
is enabled.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 cmd/mem.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/cmd/mem.c b/cmd/mem.c
index f519adaee2..2ccc7032ad 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -875,11 +875,6 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 	ulong errs = 0;	/* number of errors, or -1 if interrupted */
 	ulong pattern = 0;
 	int iteration;
-#if defined(CONFIG_SYS_ALT_MEMTEST)
-	const int alt_test = 1;
-#else
-	const int alt_test = 0;
-#endif
 
 	start = CONFIG_SYS_MEMTEST_START;
 	end = CONFIG_SYS_MEMTEST_END;
@@ -921,7 +916,7 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 
 		printf("Iteration: %6d\r", iteration + 1);
 		debug("\n");
-		if (alt_test) {
+		if (IS_ENABLED(CONFIG_SYS_ALT_MEMTEST)) {
 			errs = mem_test_alt(buf, start, end, dummy);
 		} else {
 			errs = mem_test_quick(buf, start, end, pattern,
-- 
2.25.1

[PATCH 4/4] cmd: mem: Add bitflip memory test to alternate mtest

[Stefan Roese]

This additional bitflip memory test is inspired by the bitflip test
in memtester v4.3.0. It show some errors on some problematic GARDENA
MT7688 based boards. The other memory tests usually don't show any
errors here.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 cmd/mem.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/cmd/mem.c b/cmd/mem.c
index 2ccc7032ad..0bfb6081e7 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -801,6 +801,59 @@ static ulong mem_test_alt(vu_long *buf, ulong start_addr, ulong end_addr,
 	return errs;
 }
 
+static int compare_regions(volatile unsigned long *bufa,
+			   volatile unsigned long *bufb, size_t count)
+{
+	volatile unsigned long  *p1 = bufa;
+	volatile unsigned long  *p2 = bufb;
+	int errs = 0;
+	size_t i;
+
+	for (i = 0; i < count; i++, p1++, p2++) {
+		if (*p1 != *p2) {
+			printf("FAILURE: 0x%08lx != 0x%08lx (delta=0x%08lx -> bit %ld) at offset 0x%08lx\n",
+			       (unsigned long)*p1, (unsigned long)*p2,
+			       *p1 ^ *p2, __ffs(*p1 ^ *p2),
+				(unsigned long)(i * sizeof(unsigned long)));
+			errs++;
+		}
+	}
+
+	return errs;
+}
+
+static ulong test_bitflip_comparison(volatile unsigned long *bufa,
+				     volatile unsigned long *bufb, size_t count)
+{
+	volatile unsigned long *p1 = bufa;
+	volatile unsigned long *p2 = bufb;
+	unsigned int j, k;
+	unsigned long q;
+	size_t i;
+	int max;
+	int errs = 0;
+
+	max = sizeof(unsigned long) * 8;
+	for (k = 0; k < max; k++) {
+		q = 0x00000001L << k;
+		for (j = 0; j < 8; j++) {
+			WATCHDOG_RESET();
+			q = ~q;
+			p1 = (volatile unsigned long *)bufa;
+			p2 = (volatile unsigned long *)bufb;
+			for (i = 0; i < count; i++)
+				*p1++ = *p2++ = (i % 2) == 0 ? q : ~q;
+
+			errs += compare_regions(bufa, bufb, count);
+		}
+
+		if (ctrlc())
+			return -1UL;
+	}
+
+	return errs;
+}
+
 static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
 			    vu_long pattern, int iteration)
 {
@@ -918,6 +971,13 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 		debug("\n");
 		if (IS_ENABLED(CONFIG_SYS_ALT_MEMTEST)) {
 			errs = mem_test_alt(buf, start, end, dummy);
+			if (errs == -1UL)
+				break;
+			count += errs;
+			errs = test_bitflip_comparison(buf,
+						       buf + (end - start) / 2,
+						       (end - start) /
+						       sizeof(unsigned long));
 		} else {
 			errs = mem_test_quick(buf, start, end, pattern,
 					      iteration);
-- 
2.25.1

[PATCH 1/4] cmd: mem: Correctly count the errors in mtest

[Tom Rini]

On Thu, Mar 05, 2020 at 07:21:29AM +0100, Stefan Roese wrote:

> This patch changes mtest to correctly count the overall errors and
> print them even in the abort (Ctrl-C) case.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200421/4055d4f1/attachment.sig>

[PATCH 2/4] cmd: mem: Drop eldk-4.2 workaround and use cast in unmap_sysmem()

[Tom Rini]

On Thu, Mar 05, 2020 at 07:21:30AM +0100, Stefan Roese wrote:

> Use a cast instead of the "eldk-4.2" workaround for unmap_sysmem().
> 
> Signed-off-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200421/d74e83ee/attachment.sig>

[PATCH 3/4] cmd: mem: Use IS_ENABLED instead of alt_test variable

[Tom Rini]

On Thu, Mar 05, 2020 at 07:21:31AM +0100, Stefan Roese wrote:

> This patch uses the IS_ENABLED() macro to check, which mtest variant
> is enabled.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200421/0f30af70/attachment.sig>

[PATCH 4/4] cmd: mem: Add bitflip memory test to alternate mtest

[Tom Rini]

On Thu, Mar 05, 2020 at 07:21:32AM +0100, Stefan Roese wrote:

> This additional bitflip memory test is inspired by the bitflip test
> in memtester v4.3.0. It show some errors on some problematic GARDENA
> MT7688 based boards. The other memory tests usually don't show any
> errors here.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200421/aa4b0424/attachment.sig>

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

[Ralph Siemsen]

The bitflip test uses two equal sized memory buffers. This is achived
by splitting the range of memory into two pieces. The address of the
second buffer was not correctly calulated, thus the bitflip test would
accessing memory beyond the end of the specified range.

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 in
turn misses the last word.

Fixes: 8e434cb705d463bc8cff935160e4fb4c77cb99ab ("cmd: mem: Add bitflip
memory test to alternate mtest")

Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org>
--
TODO/FIXME: maybe the ending address should be automatically aligned?

Change-Id: Ie641d04e731fc5bc6a3bbef914bf7fad136cdc94
---
 cmd/mem.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cmd/mem.c b/cmd/mem.c
index 9b97f7bf69..88e15b2d61 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -988,8 +988,9 @@ static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
 				break;
 			count += errs;
 			errs = test_bitflip_comparison(buf,
-						       buf + (end - start) / 2,
-						       (end - start) /
+						       buf + (end - start + 1) / 2 /
+						       sizeof(unsigned long),
+						       (end - start + 1) / 2 /
 						       sizeof(unsigned long));
 		} else {
 			errs = mem_test_quick(buf, start, end, pattern,
-- 
2.17.1

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

[Stefan Roese]

Hi Ralph,

On 09.09.20 03:33, Ralph Siemsen wrote:
> The bitflip test uses two equal sized memory buffers. This is achived
> by splitting the range of memory into two pieces. The address of the
> second buffer was not correctly calulated, thus the bitflip test would

calculated

> accessing memory beyond the end of the specified range.
> 
> 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 in
> turn misses the last word.
> 
> Fixes: 8e434cb705d463bc8cff935160e4fb4c77cb99ab ("cmd: mem: Add bitflip
> memory test to alternate mtest")
> 
> Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org>
> --
> TODO/FIXME: maybe the ending address should be automatically aligned?
> 
> Change-Id: Ie641d04e731fc5bc6a3bbef914bf7fad136cdc94
> ---
>   cmd/mem.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/mem.c b/cmd/mem.c
> index 9b97f7bf69..88e15b2d61 100644
> --- a/cmd/mem.c
> +++ b/cmd/mem.c
> @@ -988,8 +988,9 @@ static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
>   				break;
>   			count += errs;
>   			errs = test_bitflip_comparison(buf,
> -						       buf + (end - start) / 2,
> -						       (end - start) /
> +						       buf + (end - start + 1) / 2 /
> +						       sizeof(unsigned long),
> +						       (end - start + 1) / 2 /
>   						       sizeof(unsigned long));

Thanks for finding and fixing this:

Reviewed-by: Stefan Roese <sr@denx.de>

Perhaps you could assign a variable to make the lines a bit shorter:

			half_size = (end - start + 1) / 2 /
				sizeof(unsigned long);
    			errs = test_bitflip_comparison(buf,
						       buf + half_size,
						       half_size);

?

Thanks,
Stefan

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

[Ralph Siemsen]

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).

Fixes: 8e434cb705d463bc8cff935160e4fb4c77cb99ab ("cmd: mem: Add bitflip
memory test to alternate mtest")

Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org>
--

Changes in v2:
- Minor refactor to reduce line length
- Spellcheck and cleanup commit message

Change-Id: Ie641d04e731fc5bc6a3bbef914bf7fad136cdc94
---
 cmd/mem.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/cmd/mem.c b/cmd/mem.c
index 9b97f7bf69..7bfa93d2a0 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -934,7 +934,7 @@ static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
 static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
 			char *const argv[])
 {
-	ulong start, end;
+	ulong start, end, half_size;
 	vu_long scratch_space;
 	vu_long *buf, *dummy = &scratch_space;
 	ulong iteration_limit = 0;
@@ -987,10 +987,11 @@ static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
 			if (errs == -1UL)
 				break;
 			count += errs;
+			half_size = (end - start + 1) / 2 /
+				    sizeof(unsigned long);
 			errs = test_bitflip_comparison(buf,
-						       buf + (end - start) / 2,
-						       (end - start) /
-						       sizeof(unsigned long));
+						       buf + half_size,
+						       half_size);
 		} else {
 			errs = mem_test_quick(buf, start, end, pattern,
 					      iteration);
-- 
2.17.1

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

[Ralph Siemsen]

Hi Stefan,

On Wed, Sep 09, 2020 at 10:49:29AM +0200, Stefan Roese wrote:
>Hi Ralph,
>
>Thanks for finding and fixing this:

I've sent a v2 with the suggested changes.

Have also noticed that mtest takes considerably longer when doing the 
bitflip test. To the point where using it for manufacturing test is not 
really feasible anymore. Do you think it makes sense to add a Kconfig 
option for enabling the bitflip test?

Regards,
Ralph

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

[Stefan Roese]

Hi Ralph,

On 09.09.20 15:06, Ralph Siemsen wrote:
> Hi Stefan,
> 
> On Wed, Sep 09, 2020 at 10:49:29AM +0200, Stefan Roese wrote:
>> Hi Ralph,
>>
>> Thanks for finding and fixing this:
> 
> I've sent a v2 with the suggested changes.

Yes, thanks.

> Have also noticed that mtest takes considerably longer when doing the 
> bitflip test. To the point where using it for manufacturing test is not 
> really feasible anymore. Do you think it makes sense to add a Kconfig 
> option for enabling the bitflip test?

I agree that it's too time consuming (usually) for a manufacturing test.
Either you are okay with disabling CONFIG_SYS_ALT_MEMTEST on your board,
which will also disable this bitflip test. Or please continue adding a
new Kconfig option for it - which is probable the better option.

Thanks,
Stefan

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

[Stefan Roese]

On 09.09.20 15:02, 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).
> 
> Fixes: 8e434cb705d463bc8cff935160e4fb4c77cb99ab ("cmd: mem: Add bitflip
> memory test to alternate mtest")
> 
> Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> --
> 
> Changes in v2:
> - Minor refactor to reduce line length
> - Spellcheck and cleanup commit message
> 
> Change-Id: Ie641d04e731fc5bc6a3bbef914bf7fad136cdc94
> ---
>   cmd/mem.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/mem.c b/cmd/mem.c
> index 9b97f7bf69..7bfa93d2a0 100644
> --- a/cmd/mem.c
> +++ b/cmd/mem.c
> @@ -934,7 +934,7 @@ static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
>   static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
>   			char *const argv[])
>   {
> -	ulong start, end;
> +	ulong start, end, half_size;
>   	vu_long scratch_space;
>   	vu_long *buf, *dummy = &scratch_space;
>   	ulong iteration_limit = 0;
> @@ -987,10 +987,11 @@ static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
>   			if (errs == -1UL)
>   				break;
>   			count += errs;
> +			half_size = (end - start + 1) / 2 /
> +				    sizeof(unsigned long);
>   			errs = test_bitflip_comparison(buf,
> -						       buf + (end - start) / 2,
> -						       (end - start) /
> -						       sizeof(unsigned long));
> +						       buf + half_size,
> +						       half_size);
>   		} else {
>   			errs = mem_test_quick(buf, start, end, pattern,
>   					      iteration);
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

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

[Ralph Siemsen]

Hi Stefan,

On Wed, Sep 09, 2020 at 03:34:35PM +0200, Stefan Roese wrote:
>
>I agree that it's too time consuming (usually) for a manufacturing test.
>Either you are okay with disabling CONFIG_SYS_ALT_MEMTEST on your board,
>which will also disable this bitflip test. Or please continue adding a
>new Kconfig option for it - which is probable the better option.

Very good, I will send a separate patch that adds a Kconfig option.

Small procedural question: Patchwork is showing state=new for both 
versions of the mtest fix [1]. Is this normal, or did I miss some step 
when posting the v2 patch?

Thanks,
Ralph

[1] http://patchwork.ozlabs.org/project/uboot/list/?submitter=76946

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

[Stefan Roese]

Hi Ralph,

On 09.09.20 15:49, Ralph Siemsen wrote:
> Hi Stefan,
> 
> On Wed, Sep 09, 2020 at 03:34:35PM +0200, Stefan Roese wrote:
>>
>> I agree that it's too time consuming (usually) for a manufacturing test.
>> Either you are okay with disabling CONFIG_SYS_ALT_MEMTEST on your board,
>> which will also disable this bitflip test. Or please continue adding a
>> new Kconfig option for it - which is probable the better option.
> 
> Very good, I will send a separate patch that adds a Kconfig option.
> 
> Small procedural question: Patchwork is showing state=new for both 
> versions of the mtest fix [1]. Is this normal, or did I miss some step 
> when posting the v2 patch?

The only thing you missed, was adding my RB tag to v2. I did send a new
RB again to this mail, so patchwork will collect it automatically.

You could mark your v1 as superseded in patchwork though.

Thanks,
Stefan

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

[Ralph Siemsen]

Hi Stefan,

On Wed, Sep 09, 2020 at 03:53:08PM +0200, Stefan Roese wrote:
>Hi Ralph,
>
>On 09.09.20 15:49, Ralph Siemsen wrote:
>>
>>Very good, I will send a separate patch that adds a Kconfig option.

As it turns out, doing a separate patch for this gets messy, and also 
would introduce a dependency between the patches. Will do v3 instead.

>>Small procedural question: Patchwork is showing state=new for both 
>>versions of the mtest fix [1]. Is this normal, or did I miss some step 
>>when posting the v2 patch?
>
>The only thing you missed, was adding my RB tag to v2. I did send a new
>RB again to this mail, so patchwork will collect it automatically.

Thanks, I was not entirely clear what the policy was for this. 
Presumably there is a way for a reviewer to 'revoke' the RB in case they 
disagree with subsequent changes?

>You could mark your v1 as superseded in patchwork though.

Seems somebody beat me to it, v1 is now marked as superseded.

Ralph

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

[Stefan Roese]

Hi Ralph,

On 09.09.20 17:07, Ralph Siemsen wrote:
> Hi Stefan,
> 
> On Wed, Sep 09, 2020 at 03:53:08PM +0200, Stefan Roese wrote:
>> Hi Ralph,
>>
>> On 09.09.20 15:49, Ralph Siemsen wrote:
>>>
>>> Very good, I will send a separate patch that adds a Kconfig option.
> 
> As it turns out, doing a separate patch for this gets messy, and also 
> would introduce a dependency between the patches. Will do v3 instead.

Okay.

>>> Small procedural question: Patchwork is showing state=new for both 
>>> versions of the mtest fix [1]. Is this normal, or did I miss some 
>>> step when posting the v2 patch?
>>
>> The only thing you missed, was adding my RB tag to v2. I did send a new
>> RB again to this mail, so patchwork will collect it automatically.
> 
> Thanks, I was not entirely clear what the policy was for this. 
> Presumably there is a way for a reviewer to 'revoke' the RB in case they 
> disagree with subsequent changes?

Usually the RB is only added, when not too many further changes are
made by the committer. There is no strict rule here AFAIK.

>> You could mark your v1 as superseded in patchwork though.
> 
> Seems somebody beat me to it, v1 is now marked as superseded.

Tom is quite active here. ;)

Thanks,
Stefan

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

[Ralph Siemsen]

Hi Stefan,

On Wed, Sep 09, 2020 at 05:13:49PM +0200, Stefan Roese wrote:
>
>Usually the RB is only added, when not too many further changes are
>made by the committer. There is no strict rule here AFAIK.

In that case I'll omit your RB on v3 as the change is somewhat larger, 
and includes the "unrelated" Kconfig change also.

>>Seems somebody beat me to it, v1 is now marked as superseded.
>
>Tom is quite active here. ;)

He is not the only one ;)

Ralph

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

[Ralph Siemsen]

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@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
+
 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);
 		} else {
 			errs = mem_test_quick(buf, start, end, pattern,
 					      iteration);
-- 
2.17.1

[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@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

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

[Ralph Siemsen]

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@linaro.org>
--

Changes in v4:
- Avoid #ifdef in the code

Change-Id: Ie641d04e731fc5bc6a3bbef914bf7fad136cdc94
---
 cmd/Kconfig | 12 ++++++++++++
 cmd/mem.c   | 18 ++++++++++++++----
 2 files changed, 26 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..63d69ce632 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -867,6 +867,18 @@ 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);
+}
+
 static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
 			    vu_long pattern, int iteration)
 {
@@ -987,10 +999,8 @@ 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));
+			if (IS_ENABLED(CONFIG_SYS_ALT_MEMTEST_BITFLIP))
+				errs = mem_test_bitflip(buf, start, end);
 		} else {
 			errs = mem_test_quick(buf, start, end, pattern,
 					      iteration);
-- 
2.17.1

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

[Ralph Siemsen]

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@linaro.org>
--

Changes in v5:
- Correct logic for updating error count

Changes in v4:
- Avoid #ifdef in the code

Change-Id: Ie641d04e731fc5bc6a3bbef914bf7fad136cdc94
---
 cmd/Kconfig | 12 ++++++++++++
 cmd/mem.c   | 21 ++++++++++++++++-----
 2 files changed, 28 insertions(+), 5 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..dc4a0ffab3 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -867,6 +867,18 @@ 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);
+}
+
 static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
 			    vu_long pattern, int iteration)
 {
@@ -986,11 +998,10 @@ static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
 			errs = mem_test_alt(buf, start, end, dummy);
 			if (errs == -1UL)
 				break;
-			count += errs;
-			errs = test_bitflip_comparison(buf,
-						       buf + (end - start) / 2,
-						       (end - start) /
-						       sizeof(unsigned long));
+			if (IS_ENABLED(CONFIG_SYS_ALT_MEMTEST_BITFLIP)) {
+				count += errs;
+				errs = mem_test_bitflip(buf, start, end);
+			}
 		} else {
 			errs = mem_test_quick(buf, start, end, pattern,
 					      iteration);
-- 
2.17.1

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

[Stefan Roese]

On 09.09.20 18:10, 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@linaro.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> --
> 
> Changes in v5:
> - Correct logic for updating error count
> 
> Changes in v4:
> - Avoid #ifdef in the code
> 
> Change-Id: Ie641d04e731fc5bc6a3bbef914bf7fad136cdc94
> ---
>   cmd/Kconfig | 12 ++++++++++++
>   cmd/mem.c   | 21 ++++++++++++++++-----
>   2 files changed, 28 insertions(+), 5 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..dc4a0ffab3 100644
> --- a/cmd/mem.c
> +++ b/cmd/mem.c
> @@ -867,6 +867,18 @@ 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);
> +}
> +
>   static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
>   			    vu_long pattern, int iteration)
>   {
> @@ -986,11 +998,10 @@ static int do_mem_mtest(struct cmd_tbl *cmdtp, int flag, int argc,
>   			errs = mem_test_alt(buf, start, end, dummy);
>   			if (errs == -1UL)
>   				break;
> -			count += errs;
> -			errs = test_bitflip_comparison(buf,
> -						       buf + (end - start) / 2,
> -						       (end - start) /
> -						       sizeof(unsigned long));
> +			if (IS_ENABLED(CONFIG_SYS_ALT_MEMTEST_BITFLIP)) {
> +				count += errs;
> +				errs = mem_test_bitflip(buf, start, end);
> +			}
>   		} else {
>   			errs = mem_test_quick(buf, start, end, pattern,
>   					      iteration);
> 

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

[Tom Rini]

On Wed, Sep 09, 2020 at 12:10:00PM -0400, 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@linaro.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200919/01a788d1/attachment.sig>