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

[Benoît Thébaudeau]

Hi Joe,

On Tue, Aug 7, 2012 at 12:45:58 AM, Joe Hershberger wrote:
> 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.

OK.

> > 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.

Yes, or a fallback ethaddr could be automatically set to dev->enetaddr like Mike
said.

> > 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.

No, Mike is correct.

> 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 automatically set, this solves this issue by the way.

The current code seeds with 0, which results in rand_r returning 0, so that
timeout_ms also gets 0, and the mechanism is broken.

> > 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.

According to point 3 in doc/README.enetaddr, the current behavior of fec_probe()
is correct since it refers to "the driver initialized struct
eth_device->enetaddr". The priority given by eth_write_hwaddr() to ethaddr
against dev->enetaddr would also not make sense if that were not correct.

> eth_write_hwaddr() should not write it.

Why? This is what Mike was talking about. I don't find the patch Mike refers to.
However, I see an issue if eth_write_hwaddr() sets a fallback ethaddr from a
valid dev->enetaddr: depending on the config, ethaddr may be set only once, so
that will be one less move for the end user. But we can also consider that if
the MAC address has been set by any means (fuses, EEPROM, etc.), it is
equivalent to having set ethaddr once, so that could be correct. Mike, what do
you think about that?

Best regards,
Benoît