[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Moffett, Kyle D]

On Oct 28, 2011, at 01:49, Wolfgang Denk wrote:
> Commit 114d7fc0 "e1000: Rewrite EEPROM checksum error to give more
> information" failed to initialize the checksum variable which should
> result in random results. Fix that.
> [I wonder if that code has _ever_ been tested!!]
> 

> I wonder if you have ever actually build and run this code???
> With the "checksum" variable being random (due to not being
> initialized) you should have seen serious checksum problems.
> How did this escape your testing?

Yes, sorry, that is the correct fix.

I'm running my old GIT checkout unmodified on my hardware right now
with no problems; I must have just gotten lucky with the compiler's
register allocation, perhaps?

*pokes through disassembly*

Yeah, it looks like I just got lucky and that stack slot is always
overwritten with zero by an earlier function call.


> Commit 2326a94d caused a ton of "unused variable 'x'" warnings.
> Fix these.  While we are at it, remove some bogus parens.
> 
> And all these build warnings - have you ever actully compiled that
> code?  What's going on here???  - wd
> 
> #define E1000_WRITE_FLUSH(a) \
> -	do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
> +	E1000_READ_REG(a, STATUS)

The correct E1000_WRITE_FLUSH macro should be:
  #define E1000_WRITE_FLUSH(a) \
          do { uint32_t x = E1000_READ_REG(a, STATUS); (void)x; } while(0)

It shouldn't return a value, it's just ensuring that writes are properly
posted to the PCI bus.

I booted them and ran some standard load-and-performance tests on our
boards.  Unfortunately I was not paying enough attention to the build
log after splitting the e1000 patches to put the SPI code in a separate
file; that was when I removed the apparently-unnecessary "(void)x;".

Is there a good way to turn on "-Werror" for U-Boot builds by default?
I'll make sure to do that in the future to catch these problems first.

Ohh, actually, for some reason "-Wno-unused" was getting propagated in
to CFLAGS from my system environment variables, that's why I didn't see
the error originally.

My apologies for the bugs.

Cheers,
Kyle Moffett

--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/