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

[Michael Walle <michael@walle.cc>]

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.

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

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

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.

-michael