| Message ID | 20200909152147.19528-1-ralph.siemsen@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] cmd: mem: fix range of bitflip test | expand |
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
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);
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(-) -- 2.17.1