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

[Michael Walle]

Am 2021-11-04 14:15, schrieb Michal Simek:
> On 11/4/21 13:27, Michael Walle wrote:
>> Am 2021-11-04 12:16, schrieb 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.
>> 
>> I usually, enable this feature for my boards, because it helps you
>> getting network connectivity in u-boot in worst-case scenarios.
> 
> And that make sense. But then your bootloader should use different mac
> then your OS and with dhcp server you get two different IP addresses.

Sure, but thats only for recovery/production purposes. You'll get a
different mac address for every reset.


>>>> 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.
>> 
>> maybe then the net list command should be fixed.
> 
> If you have any proposal how to do it, I am listening.

Can't "net list" just fetch the address from the device?

>>> If fdt_fixup_ethernet() is called to update pass it to next phase is
>>> different story.
>> 
>> AFAIK the u-boot environment variable has always been the source
>> for the DT fixup. So I doubt we can change that.
> 
> Not only this. Your DT has to also has ethernet aliases which AFAIK is
> not used by Linux. It means for us this code does nothing because we
> are not using it.

Ah, right. At least this will save the sl28 board because there's no
mac-address property which could be changed. But my point remains,
that it changes behavior. (FWIW I'm still undecided if this is good or
bad).

>>> Also in our case when u-boot doesn't record mac to DT but mac remains
>>> in IP itself.
>> 
>> Have a look at
>> https://elixir.bootlin.com/linux/v5.15/source/drivers/of/of_net.c#L124
>> 
>> With this patch u-boot will now always fill the mac-address property
>> in the device tree and it will never be fetched from the nvmem 
>> storage.
>> When you have that Kconfig for random ethernet address set of course.
> 
> if you save it likely yes. If not on next reboot you get another random 
> mac.

No I mean, if your DT has a mac-address property which is fixed up by
the bootloader. Before this patch, if there is a random ethernet address
the code will try to read it from nvmem. After this patch, the MAC 
address
will always come from the bootloader.

>> Thinking about this, I guess this will break the sl28 board, which
>> has CONFIG_NET_RANDOM_ETHADDR set and will normally rely on the fact
>> that the device tree isn't fixed up, because then linux will take
>> it from the OTP memory in flash.
> 
> Does that mean that you let bootloader work with random address but OS
> will have access to memory which u-boot does not have?

correct. Usually, we don't need network in the bootloader. Most of
the time this is only for debugging. Or if its needed, someone needs
to set it by hand because there is no SPI-NOR OTP support in u-boot
for now.

> And if you want to load mac address from different source it should be
> handled somehow. I don't think there is any dt description for it but
> you can use ifconfig -hw ether to fix it in userspace anyway.

I can't follow. The device tree will supply the nvmem provider. Again,
as I don't have the mac-address property, I think the sl28 board is
fine.

-- 
-michael