On Tue, Sep 12, 2023 at 02:34:28PM +0200, Ahmad Fatoum wrote:
> Hi Sascha,
>
> On 12.09.23 12:49, Sascha Hauer wrote:
> > On Mon, Sep 11, 2023 at 05:59:27PM +0200, Ahmad Fatoum wrote:
> >> From: Ahmad Fatoum <ahmad@xxxxxx>
> >>
> >> Especially during development, devices often lack a MAC address. While a
> >> MAC address can be easily added to the environment:
> >>
> >> nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"
> >>
> >> It's easily lost when flashing complete new images, e.g. from CI.
> >> Make the development experience neater by deriving a stable MAC address
> >> if possible.
> >
> > I like this, because my Fritzbox network overview is full of duplicate
> > entries coming from boards with random MAC addresses.
>
> Glad you like it. If you are ok with the first two patches, can you take
> those until I respin this?
>
> >
> >
> >> @@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)
> >
> > This function should be renamed. When reviewing this patch I asked
> > myself why you limit generating a fixed MAC address to the DT case until
> > I realized that you actually don't. I was just confused by the function
> > name.
>
> Ah, didn't notice that. Will rename.
>
> >
> >> {
> >> char str[sizeof("xx:xx:xx:xx:xx:xx")];
> >> struct eth_device *edev;
> >> + bool generated = false;
> >> int ret;
> >>
> >> list_for_each_entry(edev, &netdev_list, list) {
> >> @@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
> >>
> >> ret = of_get_mac_addr_nvmem(edev->parent->of_node,
> >> edev->ethaddr);
> >> + if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {
> >
> > This check doesn't seem to be needed, generate_ether_addr() already
> > returns -ENOSYS when this option is not enabled.
>
> Until we have LTO, I'd like to keep this in, so the function need not be called.
> In this instance, I find it is a less ugly alternative to have #ifdef and static inline..
>
> >> @@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
> >>
> >> if (!is_valid_ether_addr(edev->ethaddr)) {
> >> char str[sizeof("xx:xx:xx:xx:xx:xx")];
> >> - random_ether_addr(edev->ethaddr);
> >> +
> >> + ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
> >
> > For most network devices we won't get here because of_populate_ethaddr()
> > will already have done it for us. It's only used for devices that are
> > probed after postenvironment_initcall(), namely USB devices.
>
> Or those that don't have CONFIG_NET_ETHADDR_FROM_MACHINE_ID enabled
> or don't have a machine ID set.
>
> > I think this deserves a cleanup before we merge this. The original
> > reason to introduce a postenvironment_initcall() for getting the MAC
> > address from nvmem was:
> >
> >> We do this in a very late initcall, because we don't want to enforce a
> >> probe a probe order between nvmem providers and network devices. We
> >> can't do it at randomization time, because we need to fixup Ethernet mac
> >> addresses, even when barebox itself doesn't ifup the netdev.
> >
> > I think we should have one centralized function that retrieves the MAC
> > address for an ethernet device. That should be called when we actually
> > need that MAC address, so once in net_new() and once at of_fixup time.
> >
> > Right now the behaviour is inconsistent with regard to random MAC
> > addresses. Currently we propagate the random MAC address to the Linux
> > device tree when we use ethernet in barebox, but we don't propagate it
> > when we do not use ethernet in barebox. We should decide what the
> > desired behaviour is and do it consistently regardless if we use
> > ethernet in barebox or not. This could be cleaned up along the way.
>
> Randomized MAC addresses are a necessary evil when barebox needs to do
> networking and doesn't have a stable address. If we have a stable address,
> we shouldn't randomize.
>
> Fixing up a randomized MAC address is a lesser evil than having different
> MAC addresses for barebox and Linux. If we don't need a MAC address in barebox,
> we shouldn't generate a random one just to fix it up.
>
> If we have a stable address, we should tell Linux about it.
>
> That's the logic of the current code and it's sensible to me. Why change it?
Ok, makes sense. Let's go for it then.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
