Hi Benoît,
On Mon, Aug 6, 2012 at 2:02 PM, Benoît Thébaudeau benoit.thebaudeau@advansee.com wrote:
Hi all,
There's a lot of stuff in U-Boot relying on ethaddr being set, e.g. the bdinfo command, or the linklocal command because of seed_mac. If ethaddr is not set, bdinfo will report exactly that, but linklocal will wait indefinitely without displaying anything.
This sounds like a problem to be fixed one way or another.
The issue is that dev->enetaddr may be set even if ethaddr is not, e.g. through imx_get_mac_from_fuse. eth_write_hwaddr uses a valid ethaddr to override an already set dev->enetaddr, but it does not require ethaddr to be set.
You should get a warning from u-boot that it is using the dev->enetaddr since the ethaddr is missing.
Hence, shouldn't the users of ethaddr rather use dev->enetaddr, or is ethaddr really supposed to be required (bug or feature)?
Because of the logic that prevents dev->enetaddr set from hardware to override ethaddr (since ethaddr should always be the source of the MAC in all but exceptional cases), it seems to me that using ethaddr is correct. Perhaps in the case of bdinfo, it could explicitly say that ethaddr is not set, and if dev->enetaddr is in use, it could also print that.
In the case of linklocal, it is difficult to decide. I don't think we want linklocal seeding its IP addresses with a random MAC address. I think we want to fix this bug by warning and giving up or warning and seeding with 0 (the algorithm can still work this way, just not as well).
If ethaddr is required, should it be up to the boards to set if for cases like imx_get_mac_from_fuse, or should eth_write_hwaddr set it automatically if dev->enetaddr is valid but ethaddr is unset or invalid?
If specific boards may have their MAC stored elsewhere (like imx_get_mac_from_fuse()), then it's probably OK for that board's call to update the ethaddr if it isn't set. In fact it seems like fec_probe() in drivers/net/fec_mxc.c should not be changing edev->enetaddr. It should setenv("ethaddr,...), but only if it is not set.
eth_write_hwaddr() should not write it.
Best regards, Benoît