On 23.01.2012 08:31, Simon Glass wrote: ...
Note: This resend is based on my understanding from
http://lists.denx.de/pipermail/u-boot/2012-January/116118.html Please let Eric and me know if I missed anything there.I don't think you have missed anything and I have already acked this. But I want to start a related discussion.
The code structure does bug me a bit - I think it is too confusing. eth_getenv_enetaddr() returns an error if there is no environment variable set or if the address it gets from the environment variable is invalid. We should probably not conflate those two. The first is ok here, but the second isn't, I think.
What if the driver has no write_hwaddr method? Do we silently ignore the environment variable value?
Why use memcmp() against env_enetaddr when the function we just called returns an error that tells us whether it is supposed to be valid (the error return your patch squashes)?
We set the hwaddr by writing directly into the dev->enet_addr field and then calling write_hwaddr() if it exists. Maybe that is ok - is the lack of write_hwaddr() an indication that the driver does MAC address handling on the fly, or just that it can't set the MAC address at all?
Overall I feel that eth_write_hwaddr() should return success or failure, confident in its determination that there is either a valid MAC address or there is not. The message you are seeing is I suppose an indication that it thinks there is a problem, when in fact none exists in this case. At the moment it feels fragile.
I wonder whether a little refactor here would be best?
While discussing about [1] we found that this is only a short term fix (which should go into 2012.03 [2]) and that we should discuss about a more general clean up of eth_write_hwaddr() and friends:
http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=net/eth.c;h=b4b9...
See Simon's ideas above.
Comments? Opinions?
Best regards
Dirk
[1] http://lists.denx.de/pipermail/u-boot/2012-January/116224.html
[2] http://lists.denx.de/pipermail/u-boot/2012-January/116436.html