[PATCH 10/10] board: sl28: disable random MAC address generation

[Michael Walle]

Am 2021-11-16 22:14, schrieb Tom Rini:
> On Mon, Nov 15, 2021 at 11:45:51PM +0100, Michael Walle wrote:
> 
>> Nowadays, u-boot (when CONFIG_NET_RANDOM_ETHADDR is set) will set
>> enetaddr to a random value if not set and then pass the randomly
>> generated MAC address to linux.
> 
> First, for clarity I'm not nak'ing this.  I kind of would like to see a
> slight reword as I think some things aren't 100% correct, even if the
> "save random MAC to ethaddr environment variable" change goes in.  For
> example, it's quite long standing that (dev|pdata)->enetaddr populates
> "mac-address" and "local-mac-address" and it seems in some older cases
> we only set the "local-mac-address" property.

fdt_fixup_memory() in common/fdt_support.c does a env_get(mac).

I'm not even sure, if there is a connection between what is fixed up
in the kernel DT and the corresponding device in u-boot if
CONFIG_DM_SEQ_ALIAS is not set.

>> This is bad for the following reasons:
>>  (1) it makes it impossible for linux to detect this error
>>  (2) linux won't trigger any fallback mechanism for the case where
>>      it didn't find any valid MAC address
> 
> This feels in some ways to be a limitation of the binding:
> https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-controller.yaml

Agreed. But it doesn't render my argument invalid ;)

> And it reads like we really must be populating "mac-address" with that
> random one and while providing a blank "local-mac-address" would be a
> way to say we don't know the true device one, it seems that wouldn't be
> used / noticed?

I guess it will just use the one populated in mac-address, i.e. the
random one. Btw, the binding says "last used" what if u-boot never
actually used that ethernet controller, should it then be empty or
populated with the random mac address.

>>  (3) a saveenv will store this randomly generated MAC address in the
>>      environment
>> 
>> Probably, the user will also be unaware that something is wrong. He 
>> will
>> just get different MAC addresses on each reboot, asking himself why 
>> this
>> is the case.
>> 
>> As this board usually have a serial port, the user can just fix this 
>> by
>> setting the MAC address manually in the environment. Also disable the
>> netconsole just in case, because it cannot be guaranteed that it will
>> work in any case. After all, this was just a convenience option, 
>> because
>> the bootloader - right now - doesn't have the ability to read the MAC
>> address, which is stored in the OTP. But it is far more important to
>> have a clear view of whats wrong with a board and that means we can no
>> longer use this Kconfig option.
>> 
>> Signed-off-by: Michael Walle <michael at walle.cc>
>> ---
>>  configs/kontron_sl28_defconfig | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/configs/kontron_sl28_defconfig 
>> b/configs/kontron_sl28_defconfig
>> index af907175f1..7fb6bdbe82 100644
>> --- a/configs/kontron_sl28_defconfig
>> +++ b/configs/kontron_sl28_defconfig
>> @@ -59,8 +59,6 @@ CONFIG_OF_LIST=""
>>  CONFIG_ENV_OVERWRITE=y
>>  CONFIG_ENV_IS_IN_SPI_FLASH=y
>>  CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
>> -CONFIG_NET_RANDOM_ETHADDR=y
>> -CONFIG_NETCONSOLE=y
>>  CONFIG_SPL_DM_SEQ_ALIAS=y
>>  CONFIG_SCSI_AHCI=y
>>  CONFIG_SATA_CEVA=y
> 
> Now, an alternate solution here would be to enable these two options
> still and write an ft_board_setup() with:
> ... if ethaddr is in the locally administered pool then
>         fdt_delprop(... "mac-address");
>         fdt_delprop(... "local-mac-address");

Which would also trigger if a user sets a "locally administered"
mac himself. Even if unlikely, I'm opposed to such magic and
unexpected behavior. Sooner or later it will bite you.

> And that should cause the kernel to fall through the cases to find out
> where to get the real MAC from.  I'm not super happy with this at 
> first,
> but I also don't see anything clever in the binding we can do.

If I'll need this one again, then I'll just add the random mac
generation in board_eth_init(), I think.

-michael