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

[Michal Simek <michal.simek@xilinx.com>]

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


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

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

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


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

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.

Thanks,
Michal