[U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr

[Joe Hershberger]

Hi Benoît,

On Mon, Aug 6, 2012 at 2:02 PM, Benoît Thébaudeau
<benoit.thebaudeau at 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