[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
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.
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
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
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
<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
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
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
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
participants (2)
-
Peter Tyser -
Wolfgang Denk