typo in amdk8/raminit_f.c

[Ronald Hoogenboom]

Hi,

There is a typo in amdk8/raminit_f.c regarding the preprocessor symbol
QRANK_DIMM_SUPPORT in line 2208, which caused the protected code
fragment never to be included for compilation.
I guessed what the dimm_mask and sz.rank should have been (I'm not
familiar with the functionality of this piece of code).

Please review, anyone who is familiar (yhlu/mjones?).

Signed-off-by: Ronald Hoogenboom

[ron minnich]

That's a fine catch. I guess we'll have to have some testing done on
this never-used piece of code.

I'm puzzled that it did not get an error. Anybody know shy? It was not
an #ifdef, it was a #if.

I think this is one reason the Bell Labs crowd started using:
if (preprocessor_symbol == value)

instead of this:
#if preprocessor_symbol == value

That's the C compilers job, after all ...

The more code I see, the more I agree with them.

ron

[Peter Stuge]

ron minnich wrote:
> I think this is one reason the Bell Labs crowd started using:
> if (preprocessor_symbol == value)
>
> That's the C compilers job, after all ...
>
> The more code I see, the more I agree with them.

I can see the point. But I also think excluding code from compilation
is a big feature, especially in firmware.

If we can do it in a smarter way, that could be good, but on the
other hand things can be too smart as well.

It's a balance..

//Peter

[ron minnich]

On Mon, Apr 13, 2009 at 9:15 AM, Peter Stuge  wrote:
> ron minnich wrote:
> > I think this is one reason the Bell Labs crowd started using:
> > if (preprocessor_symbol == value)
> >
> > That's the C compilers job, after all ...
> >
> > The more code I see, the more I agree with them.
>
> I can see the point. But I also think excluding code from compilation
> is a big feature, especially in firmware.

ah yes, but ... just about any reasonable compiler will do something
smart with this: if (0) { ...} or if (1) { ... }.

If fact, romcc would not work at all without these types of
optimizations ... and this one is not even that aggressive.

So, since:
if (preprocessor_symbol == whatever) { ... }

is equivalent to either
if (0) {...}
or
if (1) {...}

I think it is safe to say there is no loss in code reduction.
> If we can do it in a smarter way, that could be good, but on the
> other hand things can be too smart as well.

What Pike et. al are really saying is "don't have cpp do something
that the C compiler can do better and with more error checking".

With which I agree.

The reason I care? Because we've had dead code for several years in
that fragment Ronald discovered, and that could have been avoided.
Scary!

ron

[Marc Jones]

On Mon, Apr 13, 2009 at 9:12 AM, Ronald Hoogenboom  wrote:
> Hi,
>
> There is a typo in amdk8/raminit_f.c regarding the preprocessor symbol
> QRANK_DIMM_SUPPORT in line 2208, which caused the protected code
> fragment never to be included for compilation.
> I guessed what the dimm_mask and sz.rank should have been (I'm not
> familiar with the functionality of this piece of code).
>
> Please review, anyone who is familiar (yhlu/mjones?).
>
> Signed-off-by: Ronald Hoogenboom

Did you find this on a qrank setup or just visual inspection? You fix
looks correct.

Acked-by: Marc Jones

Marc

[Ronald Hoogenboom]

Marc Jones schreef:
> Did you find this on a qrank setup or just visual inspection? You fix
> looks correct.

I found it using vim's quickfix after compiling for m57sli, I guess 
that's equivalent to visual inspection. I copied the missing pieces from 
other locations in the same source file until it compiled successfully. 
Note that there are MANY other warnings after compiling the v2 code, 
most of them unused variables/static functions. If we got rid of them, 
then maybe we would find issues like this more easily. Personally I 
always strive for warning-free compilation results...
> Acked-by: Marc Jones

Does this mean it /tests/ OK?
> Marc

Ronald.

[Marc Jones]

On Wed, Apr 15, 2009 at 12:37 AM, Ronald Hoogenboom
 wrote:
> Marc Jones schreef:
> > Did you find this on a qrank setup or just visual inspection? You fix
> > looks correct.
>
> I found it using vim's quickfix after compiling for m57sli, I guess that's
> equivalent to visual inspection. I copied the missing pieces from other
> locations in the same source file until it compiled successfully. Note that
> there are MANY other warnings after compiling the v2 code, most of them
> unused variables/static functions. If we got rid of them, then maybe we
> would find issues like this more easily. Personally I always strive for
> warning-free compilation results...

I agree and we have been forcing more warning and clean things up.
> > Acked-by: Marc Jones
>
> Does this mean it /tests/ OK?

The code looks correct but I don't have a machine that can do
quad-rank dimms. I have not seen many people use qrank dimms so we may
not find anyone that can test it for us.

Marc

[Peter Stuge]

Marc Jones wrote:
> we may not find anyone that can test it for us.

Please commit the fix.

//Peter

[Marc Jones]

r4137