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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

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.

> 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

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?

>  (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@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");

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]