[U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
From: John Schmoller 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 2000: deadbeef deadbeef deadbeef deadbeef 2010: deadbeef deadbeef deadbeef deadbeef 2020: deadbeef deadbeef deadbeef deadbeef 2030: deadbeef deadbeef deadbeef deadbeef => mtest 0x1000 0x2000 1 1 Testing 1000 ... 2000: Tested 1 iteration(s) with 0 errors. => md 0x2000 10 2000: deadbeef deadbeef deadbeef 2010: deadbeef deadbeef deadbeef deadbeef 2020: deadbeef deadbeef deadbeef deadbeef 2030: 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 Signed-off-by: Peter Tyser --- 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
Dear Peter Tyser, In message <1274375283-13004-1-git-send-email-pty...@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 4004 4007 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: w...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
Hi Wolfgang, > In message <1274375283-13004-1-git-send-email-pty...@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 4004 4007 > > 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 1000 ... 2000: That should be "1000 ... 2003" then, correct? (I know it should be "1000 ... 1fff" 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
Dear Peter Tyser, In message <1274382441.18152.37.ca...@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 1000 ... 2000: > > That should be "1000 ... 2003" 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 2000, this writes to the byte addresses 2000, 2001, 2002 and 2003 (assuming a big endian system). So the output actually is correct. > be "1000 ... 1fff" 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: w...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
> > - The output of 'mtest' is misleading: > > => mtest 0x1000 0x2000 1 1 > > Testing 1000 ... 2000: > > > > That should be "1000 ... 2003" 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 2000, this > writes to the byte addresses 2000, 2001, 2002 and > 2003 (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 "1000 ... 1fff" 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 1000 ... 1ffd: 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 1000 ... 1fff 0x1ffd - output: Testing 1000 ... 1fff 0x1ffe - output: Testing 1000 ... 1fff 0x1fff - output: Testing 1000 ... 1fff 0x2000 - output: Testing 1000 ... 2003 0x2001 - output: Testing 1000 ... 2003 ... 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
Dear Peter Tyser, In message <1274386292.18152.119.ca...@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 1000 ... 1ffd: 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 1000 ... 1fff > 0x1ffd - output: Testing 1000 ... 1fff > 0x1ffe - output: Testing 1000 ... 1fff > 0x1fff - output: Testing 1000 ... 1fff > 0x2000 - output: Testing 1000 ... 2003 > 0x2001 - output: Testing 1000 ... 2003 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: w...@denx.de For every complex problem, there is a solution that is simple, neat, and wrong. -- H. L. Mencken ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
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 1000 ... 1fff > > 0x1ffd - output: Testing 1000 ... 1fff > > 0x1ffe - output: Testing 1000 ... 1fff > > 0x1fff - output: Testing 1000 ... 1fff > > 0x2000 - output: Testing 1000 ... 2003 > > 0x2001 - output: Testing 1000 ... 2003 > > 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 1003 ... 1ffc: ... md 0x1000 10; md 0x1ff0 10 1000: 12345600 .4V. 1010: 1020: 1030: 1ff0: 0078...x 2000: 12345678 12345678 12345678 12345678.4Vx.4Vx.4Vx.4Vx 2010: 12345678 12345678 12345678 12345678.4Vx.4Vx.4Vx.4Vx 2020: 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 1003 ... 1ffe". 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mtest: Fix end address of increment/decrement test
Dear Peter Tyser, In message <1274392850.18152.253.ca...@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 > 1003 ... 1ffe". 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: w...@denx.de "Who alone has reason to *lie himself out* of actuality? He who *suffers* from it." - Friedrich Nietzsche ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
