[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[Peter Tyser]

From: John Schmoller <jschmoller@xes-inc.com>

The incrememt/decrement test has an off-by-one error which results in an
extra 4 bytes being tested past the specified end address.  For
instance, when running "mtest 0x1000 0x2000", the bytes 0x2000-0x2003
would be tested which is counterintuitive and at odds with the end
address calculation of other U-Boot memory tests.

Example of original behavior:
=> md 0x2000 10
00002000: deadbeef deadbeef deadbeef deadbeef    ................
00002010: deadbeef deadbeef deadbeef deadbeef    ................
00002020: deadbeef deadbeef deadbeef deadbeef    ................
00002030: deadbeef deadbeef deadbeef deadbeef    ................
=> mtest 0x1000 0x2000 1 1
Testing 00001000 ... 00002000:
Tested 1 iteration(s) with 0 errors.
=> md 0x2000 10
00002000: 00000000 deadbeef deadbeef deadbeef    ................
00002010: deadbeef deadbeef deadbeef deadbeef    ................
00002020: deadbeef deadbeef deadbeef deadbeef    ................
00002030: deadbeef deadbeef deadbeef deadbeef    ................
=>

This change results in the memory at 0x2000 not being modified by mtest
in the example above.

Signed-off-by: John Schmoller <jschmoller@xes-inc.com>
Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
 common/cmd_mem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index 1839330..0bc3c70 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -862,7 +862,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 		 *
 		 * Returns:     0 if the test succeeds, 1 if the test fails.
 		 */
-		num_words = ((ulong)end - (ulong)start)/sizeof(vu_long) + 1;
+		num_words = ((ulong)end - (ulong)start)/sizeof(vu_long);
 
 		/*
 		 * Fill memory with a known pattern.
-- 
1.7.0.4

[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[Wolfgang Denk]

Dear Peter Tyser,

In message <1274375283-13004-1-git-send-email-ptyser@xes-inc.com> you wrote:
> 
> The incrememt/decrement test has an off-by-one error which results in an
> extra 4 bytes being tested past the specified end address.  For
> instance, when running "mtest 0x1000 0x2000", the bytes 0x2000-0x2003
> would be tested which is counterintuitive and at odds with the end
> address calculation of other U-Boot memory tests.

I disagree. I understand your reasoning, but actually it has always
been the case that commands that take an address reagion specify as
end address the last address to be used, not oni behind the range.
You may not like this, but that's how it has been implemented > 10
years ago, and many people are trained on this behaviour. See for
example the flash erase command, wehre you will type something like

	=> erase 40040000 4007FFFF

Here, like in other places, we really use the end address.

So for the sake of consisteny I tend to reject your patch.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A Perl script is correct if it's halfway readable and  gets  the  job
done before your boss fires you.
                       - L. Wall & R. L. Schwartz, _Programming Perl_

[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[Peter Tyser]

Hi Wolfgang,

> In message <1274375283-13004-1-git-send-email-ptyser@xes-inc.com> you wrote:
> > 
> > The incrememt/decrement test has an off-by-one error which results in an
> > extra 4 bytes being tested past the specified end address.  For
> > instance, when running "mtest 0x1000 0x2000", the bytes 0x2000-0x2003
> > would be tested which is counterintuitive and at odds with the end
> > address calculation of other U-Boot memory tests.
> 
> I disagree. I understand your reasoning, but actually it has always
> been the case that commands that take an address reagion specify as
> end address the last address to be used, not oni behind the range.
> You may not like this, but that's how it has been implemented > 10
> years ago, and many people are trained on this behaviour. See for
> example the flash erase command, wehre you will type something like
> 
> 	=> erase 40040000 4007FFFF
> 
> Here, like in other places, we really use the end address.
> 
> So for the sake of consisteny I tend to reject your patch.

I can see your point but the current memtest code is not consistent with
your description.
- Every other test other than the increment/decrement tests the region <
end address.  Eg in the start:0x1000 end:0x2000 example, the *only* test
that touches 0x2000-0x2003 region is the increment/decrement test.
Either its broken, or the other memory test functions are.

- The output of 'mtest' is misleading:
=> mtest 0x1000 0x2000 1 1
Testing 00001000 ... 00002000:

That should be "00001000 ... 00002003" then, correct?  (I know it should
be "00001000 ... 00001fff" to be consistent with this patch's
implementation, so this argument is weak...)

How would you like this cleaned up?  Bring the address coverage of the
other tests inline with the increment/decrement test?  Improve the mtest
output so its obvious what exactly is being tested?

Best,
Peter

[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[Wolfgang Denk]

Dear Peter Tyser,

In message <1274382441.18152.37.camel@petert> you wrote:
> 
> I can see your point but the current memtest code is not consistent with
> your description.
> - Every other test other than the increment/decrement tests the region <
> end address.  Eg in the start:0x1000 end:0x2000 example, the *only* test
> that touches 0x2000-0x2003 region is the increment/decrement test.
> Either its broken, or the other memory test functions are.

I think this might indeed be the case. IIRC I originally wrote only
the simple increment/decrement test, and the other tests got added
later by others, probably with nobody noticing the differing
behaviour.

> - The output of 'mtest' is misleading:
> => mtest 0x1000 0x2000 1 1
> Testing 00001000 ... 00002000:
> 
> That should be "00001000 ... 00002003" then, correct?  (I know it should

No, it should not. The output shows the addresses where data is
written to. If you write a 32 bit word to address 00002000, this
writes to the byte addresses 00002000, 00002001, 00002002 and
00002003 (assuming a big endian system). So the output actually is
correct.

> be "00001000 ... 00001fff" to be consistent with this patch's
> implementation, so this argument is weak...)

No, because we do not actually write to this address (which would also
be misaligned for a word write).

> How would you like this cleaned up?  Bring the address coverage of the
> other tests inline with the increment/decrement test?  Improve the mtest
> output so its obvious what exactly is being tested?

Both, of course :-)  Although I think the output is even correct, it
just leaves room for misinterpretation.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
ADVISORY:  There is  an  Extremely Small  but  Nonzero  Chance  That,
Through a Process Know as "Tunneling," This Product May Spontaneously
Disappear  from Its Present Location and Reappear at Any Random Place
in the Universe, Including Your Neighbor's Domicile. The Manufacturer
Will Not Be Responsible for Any Damages  or  Inconvenience  That  May
Result.

[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[Peter Tyser]

<snip>

> > - The output of 'mtest' is misleading:
> > => mtest 0x1000 0x2000 1 1
> > Testing 00001000 ... 00002000:
> > 
> > That should be "00001000 ... 00002003" then, correct?  (I know it should
> 
> No, it should not. The output shows the addresses where data is
> written to. If you write a 32 bit word to address 00002000, this
> writes to the byte addresses 00002000, 00002001, 00002002 and
> 00002003 (assuming a big endian system). So the output actually is
> correct.

The current output leaves a lot of interpretation up to the user.
Without digging into the code they won't know that 32-bits is written at
0x2000.  They may think its a byte at 0x2000.  Or maybe a 64-bit
quantity, they'd have no idea.

> > be "00001000 ... 00001fff" to be consistent with this patch's
> > implementation, so this argument is weak...)
> 
> No, because we do not actually write to this address (which would also
> be misaligned for a word write).

It depends on interpretation.  eg:
=> mtest 0x1000 0x1ffd 1 1       
Testing 00001000 ... 00001ffd:

This is *really* testing bytes 0x1000-0x1fff.  It's impossible for Joe
User to figure that out though...

> > How would you like this cleaned up?  Bring the address coverage of the
> > other tests inline with the increment/decrement test?  Improve the mtest
> > output so its obvious what exactly is being tested?
> 
> Both, of course :-)  Although I think the output is even correct, it
> just leaves room for misinterpretation.

As far as the output, my vote would be to align the end address to a
32-bit address and add 3.  eg assuming a starting address of 1000 and
ending addresses of:
0x1ffc - output: Testing 00001000 ... 00001fff
0x1ffd - output: Testing 00001000 ... 00001fff
0x1ffe - output: Testing 00001000 ... 00001fff
0x1fff - output: Testing 00001000 ... 00001fff
0x2000 - output: Testing 00001000 ... 00002003
0x2001 - output: Testing 00001000 ... 00002003
...

This would tell the user explicitly what bytes have been tested and they
wouldn't have to calculate alignments or know word sizes.  

Another possibility would be to replace the "end address" argument with
a "size" argument.  So "mtest 0x1000 0x1000" would test 0x1000 bytes at
address 0x1000, from 0x1000-0x1fff.  I think that would be clearer to
most people, but the downside is you'd have to do some math to calculate
the size parameter in some cases (eg testing the region after exception
vectors, but before the U-Boot image in RAM).

Let me know what you think about how to display the tested memory
region.  I'm fine with leaving the "end addr" vs "size" debate for the
next release, if at all.

Best,
Peter

[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[Wolfgang Denk]

Dear Peter Tyser,

In message <1274386292.18152.119.camel@petert> you wrote:
> 
> The current output leaves a lot of interpretation up to the user.

Agreed. This is one of the typical commands that where never well
designed or even intnded for normal users, but serrved a purpose, and
found useful, so it stuck.  No surprise it has sharp edges ;-)

> It depends on interpretation.  eg:
> => mtest 0x1000 0x1ffd 1 1       
> Testing 00001000 ... 00001ffd:

To be honest - I wasn't even aware we support such a notation ;-)

> This is *really* testing bytes 0x1000-0x1fff.  It's impossible for Joe
> User to figure that out though...

...not without reading the source code, indeed. But then, this is
always a good excercise :-)

> As far as the output, my vote would be to align the end address to a
> 32-bit address and add 3.  eg assuming a starting address of 1000 and
> ending addresses of:
> 0x1ffc - output: Testing 00001000 ... 00001fff
> 0x1ffd - output: Testing 00001000 ... 00001fff
> 0x1ffe - output: Testing 00001000 ... 00001fff
> 0x1fff - output: Testing 00001000 ... 00001fff
> 0x2000 - output: Testing 00001000 ... 00002003
> 0x2001 - output: Testing 00001000 ... 00002003

No, please do not implement such automatic alignment; it may be useful
for some cases, but it may as well hurt, for example if you
intentionally want to run mtest with misalignment, like giving both
odd start and end addresses.

> Another possibility would be to replace the "end address" argument with
> a "size" argument.  So "mtest 0x1000 0x1000" would test 0x1000 bytes at
> address 0x1000, from 0x1000-0x1fff.  I think that would be clearer to
> most people, but the downside is you'd have to do some math to calculate
> the size parameter in some cases (eg testing the region after exception
> vectors, but before the U-Boot image in RAM).

I think it is more difficult to calculate the sizes instead of the end
addresses.

> Let me know what you think about how to display the tested memory
> region.  I'm fine with leaving the "end addr" vs "size" debate for the
> next release, if at all.

I always appreciate is someone is thorough and willing enough to clean
up such old mess.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
For every complex problem, there is a solution that is simple,  neat,
and wrong.                                           -- H. L. Mencken

[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[Peter Tyser]

Hi Wolfgang,

> > As far as the output, my vote would be to align the end address to a
> > 32-bit address and add 3.  eg assuming a starting address of 1000 and
> > ending addresses of:
> > 0x1ffc - output: Testing 00001000 ... 00001fff
> > 0x1ffd - output: Testing 00001000 ... 00001fff
> > 0x1ffe - output: Testing 00001000 ... 00001fff
> > 0x1fff - output: Testing 00001000 ... 00001fff
> > 0x2000 - output: Testing 00001000 ... 00002003
> > 0x2001 - output: Testing 00001000 ... 00002003
> 
> No, please do not implement such automatic alignment; it may be useful
> for some cases, but it may as well hurt, for example if you
> intentionally want to run mtest with misalignment, like giving both
> odd start and end addresses.

I didn't express it well, but what I was getting at was that the
"Testing X .. Y" would ideally state exactly what is being tested.
Unaligned addresses would still be allowed.  I think right now the end
address is always automatically aligned to the same alignment as the
start address though, so the current output is very misleading.

eg:
mw.l 0x1000 0x12345678 0x1000
mtest 0x1003 0x1ffc 1 1
Testing 00001003 ... 00001ffc:
...
md 0x1000 10; md 0x1ff0 10
00001000: 12345600 00000000 00000000 00000000    .4V.............
00001010: 00000000 00000000 00000000 00000000    ................
00001020: 00000000 00000000 00000000 00000000    ................
00001030: 00000000 00000000 00000000 00000000    ................
00001ff0: 00000000 00000000 00000000 00000078    ...............x
00002000: 12345678 12345678 12345678 12345678    .4Vx.4Vx.4Vx.4Vx
00002010: 12345678 12345678 12345678 12345678    .4Vx.4Vx.4Vx.4Vx
00002020: 12345678 12345678 12345678 12345678    .4Vx.4Vx.4Vx.4Vx

You can see the starting alignment was respected, but the ending
alignment was truncated to be 32-bit aligned to the starting address.
In the above example, I think it would be nice to see "Testing
00001003 ... 00001ffe".  Or some other way such that the user knows that
their input wasn't executed to a T; their end address was truncated.

Best,
Peter

[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test

[Wolfgang Denk]

Dear Peter Tyser,

In message <1274392850.18152.253.camel@petert> you wrote:
> 
> I didn't express it well, but what I was getting at was that the
> "Testing X .. Y" would ideally state exactly what is being tested.

We have full agreement here.

> Unaligned addresses would still be allowed.  I think right now the end
> address is always automatically aligned to the same alignment as the
> start address though, so the current output is very misleading.

Agreed, too.

> You can see the starting alignment was respected, but the ending
> alignment was truncated to be 32-bit aligned to the starting address.
> In the above example, I think it would be nice to see "Testing
> 00001003 ... 00001ffe".  Or some other way such that the user knows that
> their input wasn't executed to a T; their end address was truncated.

Yep. We are in sync.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Who alone has reason to *lie  himself  out*  of  actuality?  He  who
*suffers* from it."                             - Friedrich Nietzsche