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

Thu Nov 4 22:00:12 CET 2021

On Thu, Nov 4, 2021 at 3:40 PM Michael Walle <michael at walle.cc> wrote:
>
> 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
What's the verdict ? We're keeping this patch ?