[PATCH] net: uclass: Save ethernet MAC address when generated

[Michal Simek]

On 11/4/21 03:09, Grygorii Strashko wrote:
> 
> 
> On 02/11/2021 12:27, Michal Simek wrote:
>>
>>
>> On 11/2/21 10:00, Michael Walle wrote:
>>>> On Fri, Oct 29, 2021 at 2:14 PM Michal Simek 
>>>> <michal.simek at xilinx.com> wrote:
>>>>>
>>>>> When MAC address is randomly generated it should be also saved to
>>>>> variables. This step is there when MAC address is passed via pdata 
>>>>> but not
>>>>> when it is randomly generated.
>>>>>
>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>>>>> ---
>>>>>
>>>>>   net/eth-uclass.c | 2 ++
>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>>>>> index 0da0e85be031..58c308f33276 100644
>>>>> --- a/net/eth-uclass.c
>>>>> +++ b/net/eth-uclass.c
>>>>> @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev)
>>>>>                  net_random_ethaddr(pdata->enetaddr);
>>>>>                  printf("\nWarning: %s (eth%d) using random MAC 
>>>>> address - %pM\n",
>>>>>                         dev->name, dev_seq(dev), pdata->enetaddr);
>>>>> +               eth_env_set_enetaddr_by_index("eth", dev_seq(dev),
>>>>> +                                             pdata->enetaddr);
>>>>>   #else
>>>>>                  printf("\nError: %s address not set.\n",
>>>>>                         dev->name);
>>>>> -- 
>>>>> 2.33.1
>>>>>
>>>> Reviewed-by: Ramon Fried <rfried.dev at gmail.com>
>>>
>>> Please note, that this will change behavior. Before this commit, the
>>> random mac address was local to u-boot (at least for most network 
>>> drivers).
>>> After this commit, it will also be communicated to linux.
>>>
>>> I'm not sure what to think of this. At the very least, this should be
>>> documented in the commit message and in the Kconfig help text.
>>
>> Thanks for bringing this up. I have no issue that this address is 
>> being propagated to Linux but others can feel this as an issue.
>> I can definitely extend commit message to say it.
> 
> Propagating random MAC to Linux might be not a good idea as Linux will 
> silently use it while in many cases it means that smth is wrong,
> and message like "Driver uses random MAC!" helps narrow down issues 
> earlier.

Not sure if you have ever tried this feature.

Mac address is shown on the log.
Warning: ethernet at ff0c0000 (eth1) using random MAC address - 
66:dc:38:d1:24:a9

And if you don't want to use this feature just don't enable it via 
CONFIG_NET_RANDOM_ETHADDR.


> For example, we occasionally copy-pasted wrong mac in DT and detected 
> this only after some time when started connecting
> similar boards to each other :(

That's not issue with this patch.

> Also, Linux may have it's own way to retrieve MAC if not provided by 
> u-boot.

Sure and I am not blocking it. I am just saying that u-boot is 
generating random mac address which is not exported to variables which 
is what net list command expects.
If fdt_fixup_ethernet() is called to update pass it to next phase is 
different story.
Also in our case when u-boot doesn't record mac to DT but mac remains in 
IP itself.

> 
> Wouldn't be enough to just print MAC console when random is used?

As I said above it is already there but IMHO not enough.
Take a look at behavior with this patch and without.

ZYNQ GEM: ff0b0000, mdio bus ff0c0000, phyaddr 4, interface sgmii
Loading new PMUFW cfg obj (32 bytes)

Warning: ethernet at ff0b0000 (eth0) using random MAC address - 
7a:77:b5:39:e8:12

ZYNQ GEM: ff0c0000, mdio bus ff0c0000, phyaddr 8, interface rgmii-id
Loading new PMUFW cfg obj (32 bytes)

Warning: ethernet at ff0c0000 (eth1) using random MAC address - 
32:77:95:89:df:0b
ethernet at ff0c0000 Waiting for PHY auto negotiation to complete....... done
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
DHCP client bound to address 192.168.0.143 (3260 ms)
gpio: pin resetchip5 (gpio 179) value is 0
gpio: pin resetchip5 (gpio 179) value is 1
ethernet at ff0b0000 Waiting for PHY auto negotiation to complete........ done
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
DHCP client bound to address 192.168.0.163 (3513 ms)
Hit any key to stop autoboot:  0
ZynqMP> net list
eth0 : ethernet at ff0b0000 7a:77:b5:39:e8:12 active
eth1 : ethernet at ff0c0000 32:77:95:89:df:0b
ZynqMP>


compare to
ZynqMP> net list
eth0 : ethernet at ff0b0000 00:00:00:00:00:00 active
eth1 : ethernet at ff0c0000 00:00:00:00:00:00

Thanks,
Michal