[U-Boot] Refactoring eth_write_hwaddr() and friends (was: Re: [PATCH v2] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back)

[Dirk Behme]

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=b4b9b4341fdecb25869a07cc8dbe9deefd6452bd;hb=HEAD#l172

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