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