On Mon, Aug 6, 2012 at 8:40 PM, Mike Frysinger vapier@gentoo.org wrote:

there was a patch floating around for setting up ethaddr env var automatically in the net core during init if it wasn't already set

I guess you are referring to this one: http://patchwork.ozlabs.org/patch/136789/

Regards,

Fabio Estevam

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