[U-Boot-Users] [Q] memtest doubt

[U-Boot-Users] [Q] memtest doubt

[Guennadi Liakhovetski]

Hi all,

looking through the common/cmd_mem.c::do_mem_mtest() function, I couldn't 
understand the following place:

		addr_mask = ((ulong)end - (ulong)start)/sizeof(vu_long);
...
		for (offset = 1; (offset & addr_mask) != 0; offset <<= 1) {
			start[offset] = pattern;
		}

why (offset & addr_mask) != 0 and not just offset < addr_mask? Suppose

	end = 0xbf;
	start = 0;

	addr_mask = 0x2f;

The loop will iterate over offset = 1, 2, 4, 8, and on 0x10 it will abort 
and 0x10 and 0x20 will stay untested. Whereas if we just had "offset < 
addr_mask" it would just function correctly, wouldn't it? Yes, I do 
realise, that it is@least unusual to set the end address to anything 
other than start address + ((1 << x) - 1), but still.

Thanks
Guennadi
---
Guennadi Liakhovetski

[U-Boot-Users] [Q] memtest doubt

[Jerry Van Baren]

Guennadi Liakhovetski wrote:
> Hi all,
> 
> looking through the common/cmd_mem.c::do_mem_mtest() function, I couldn't 
> understand the following place:
> 
> 		addr_mask = ((ulong)end - (ulong)start)/sizeof(vu_long);
> ...
> 		for (offset = 1; (offset & addr_mask) != 0; offset <<= 1) {
> 			start[offset] = pattern;
> 		}
> 
> why (offset & addr_mask) != 0 and not just offset < addr_mask? Suppose
> 
> 	end = 0xbf;
> 	start = 0;
> 
> 	addr_mask = 0x2f;
> 
> The loop will iterate over offset = 1, 2, 4, 8, and on 0x10 it will abort 
> and 0x10 and 0x20 will stay untested. Whereas if we just had "offset < 
> addr_mask" it would just function correctly, wouldn't it? Yes, I do 
> realise, that it is at least unusual to set the end address to anything 
> other than start address + ((1 << x) - 1), but still.
> 
> Thanks
> Guennadi

Hi Guennadi,

The address test is stepping through the address lines
   0x01, 0x02, 0x04, 0x08, 0x10, 0x20
Your end of 0xBF with a mask of 0x2F indicates that the address lines 
0x10, 0x40, and 0x80 are not present (even though address line 0x80 
looks like it is part of the test since 0xBF includes 0x80 - but it 
isn't tested).

This is nonsensical with respect to what the address line test is 
testing (address lines!) and how it is testing them - by stepping 
through the address lines and looking for inadvertent overlapped memory 
accesses.  Address lines are inherently powers of two, skipping certain 
ones of them doesn't make much sense, and ending not on a power of two 
(minus one) doesn't make any sense at all.

I fail to see what your change would benefit as an address line test. 
If there is a benefit, you will have to rewrite the address line test 
because having a contiguous mask and a ((2^n) - 1) end is all 
fundamental to how the address line test works, is fundamental to what 
address lines are, and is based on the symptoms that are observable when 
an address line fails.  Simply using an arbitrary end address and a 
funky mask will cause the current test to fail and likely will cause 
some real failures to be undetected.

Best regards,
gvb

[U-Boot-Users] [Q] memtest doubt

[Guennadi Liakhovetski]

Hi Jerry

On Thu, 7 Feb 2008, Jerry Van Baren wrote:

> Guennadi Liakhovetski wrote:
> > Hi all,
> > 
> > looking through the common/cmd_mem.c::do_mem_mtest() function, I couldn't
> > understand the following place:
> > 
> > 		addr_mask = ((ulong)end - (ulong)start)/sizeof(vu_long);
> > ...
> > 		for (offset = 1; (offset & addr_mask) != 0; offset <<= 1) {
> > 			start[offset] = pattern;
> > 		}
> > 
> > why (offset & addr_mask) != 0 and not just offset < addr_mask? Suppose
> > 
> > 	end = 0xbf;
> > 	start = 0;
> > 
> > 	addr_mask = 0x2f;
> > 
> > The loop will iterate over offset = 1, 2, 4, 8, and on 0x10 it will abort
> > and 0x10 and 0x20 will stay untested. Whereas if we just had "offset <
> > addr_mask" it would just function correctly, wouldn't it? Yes, I do realise,
> > that it is at least unusual to set the end address to anything other than
> > start address + ((1 << x) - 1), but still.
> > 
> > Thanks
> > Guennadi
> 
> Hi Guennadi,
> 
> The address test is stepping through the address lines
>   0x01, 0x02, 0x04, 0x08, 0x10, 0x20
> Your end of 0xBF with a mask of 0x2F indicates that the address lines 0x10,
> 0x40, and 0x80 are not present (even though address line 0x80 looks like it is
> part of the test since 0xBF includes 0x80 - but it isn't tested).

I think, you are making a mistake here. Look above how addr_mask is 
calculated:

> > 		addr_mask = ((ulong)end - (ulong)start)/sizeof(vu_long);

That means, it is just calculated based upon your memory range being 
tested. So if for some reason you want to test a strange address range 
like (0x10000) to (0x10000 + 0xbf) then the 0x40 address line will not be 
tested, although it is needed to cover all addresses in this range:-) Or 
am I still missing anything?

Thanks
Guennadi
---
Guennadi Liakhovetski

[U-Boot-Users] [Q] memtest doubt

[Jerry Van Baren]

Guennadi Liakhovetski wrote:
> Hi Jerry
> 
> On Thu, 7 Feb 2008, Jerry Van Baren wrote:
> 
>> Guennadi Liakhovetski wrote:
>>> Hi all,
>>>
>>> looking through the common/cmd_mem.c::do_mem_mtest() function, I couldn't
>>> understand the following place:
>>>
>>> 		addr_mask = ((ulong)end - (ulong)start)/sizeof(vu_long);
>>> ...
>>> 		for (offset = 1; (offset & addr_mask) != 0; offset <<= 1) {
>>> 			start[offset] = pattern;
>>> 		}
>>>
>>> why (offset & addr_mask) != 0 and not just offset < addr_mask? Suppose
>>>
>>> 	end = 0xbf;
>>> 	start = 0;
>>>
>>> 	addr_mask = 0x2f;
>>>
>>> The loop will iterate over offset = 1, 2, 4, 8, and on 0x10 it will abort
>>> and 0x10 and 0x20 will stay untested. Whereas if we just had "offset <
>>> addr_mask" it would just function correctly, wouldn't it? Yes, I do realise,
>>> that it is at least unusual to set the end address to anything other than
>>> start address + ((1 << x) - 1), but still.
>>>
>>> Thanks
>>> Guennadi
>> Hi Guennadi,
>>
>> The address test is stepping through the address lines
>>   0x01, 0x02, 0x04, 0x08, 0x10, 0x20
>> Your end of 0xBF with a mask of 0x2F indicates that the address lines 0x10,
>> 0x40, and 0x80 are not present (even though address line 0x80 looks like it is
>> part of the test since 0xBF includes 0x80 - but it isn't tested).
> 
> I think, you are making a mistake here. Look above how addr_mask is 
> calculated:
> 
>>> 		addr_mask = ((ulong)end - (ulong)start)/sizeof(vu_long);
> 
> That means, it is just calculated based upon your memory range being 
> tested. So if for some reason you want to test a strange address range 
> like (0x10000) to (0x10000 + 0xbf) then the 0x40 address line will not be 
> tested, although it is needed to cover all addresses in this range:-) Or 
> am I still missing anything?
> 
> Thanks
> Guennadi

Ahh, yes.  In your example the 0x80 line isn't tested either, even 
though it could (and should) be.

The address line test has to be careful to not exceed the end of memory 
because that is wrong and will cause errors.  I think that the problem 
you are pointing out is that, when we reserve memory space at the end of 
memory, our "end" address isn't really the memory end address (and 
rightly so), but the *algorithmic* calculation of the "addr_mask" is not 
really correct if "end" isn't really the end of memory.

The result is an address line that *could be* (and should be) tested is 
missed.  FWIIW, in similar code I've written in the past, I've hardcoded 
(#defined) the mask to a sensible value based on my knowledge of the 
specific hardware.  The problem with hardcoding the mask is that it 
doesn't work with dynamic memory sizing.

Your original question/concept still used the name "addr_mask" which 
caused confusion on my part.  It really should be a length, not a mask. 
  Illustrating with a hand-generated patch, what I am now hearing is the 
following change:

- 		addr_mask = ((ulong)end - (ulong)start)/sizeof(vu_long);
+ 		len = ((ulong)end - (ulong)start)/sizeof(vu_long);
  ...
- 		for (offset = 1; (offset & addr_mask) != 0; offset <<= 1) {
+ 		for (offset = 1; offset < len; offset <<= 1) {
  			start[offset] = pattern;
  		}

That makes sense to me.

Best regards,
gvb

[U-Boot-Users] [Q] memtest doubt

[Guennadi Liakhovetski]

On Thu, 7 Feb 2008, Jerry Van Baren wrote:

> Your original question/concept still used the name "addr_mask" which caused
> confusion on my part.

Ahhh, that's just a variable name:-)

> It really should be a length, not a mask.

Of course.

> Illustrating
> with a hand-generated patch, what I am now hearing is the following change:
> 
> - 		addr_mask = ((ulong)end - (ulong)start)/sizeof(vu_long);
> + 		len = ((ulong)end - (ulong)start)/sizeof(vu_long);
>  ...
> - 		for (offset = 1; (offset & addr_mask) != 0; offset <<= 1) {
> + 		for (offset = 1; offset < len; offset <<= 1) {
>  			start[offset] = pattern;
>  		}
> 
> That makes sense to me.

Exactly. If everyone agrees, I'll try not to forget to cook up a patch.

Thanks
Guennadi
---
Guennadi Liakhovetski