Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Alexander,

On 13.10.23 13:11, Sascha Hauer wrote:
> On Thu, Oct 12, 2023 at 05:59:45PM +0300, Alexander Shiyan wrote:
>> Hello.
>>
>> Any news on this patch?
> 
> It's Ahmad's turn to integrate the remaining review topic(s) and resend.

I don't mind if you send the v2. It'll take me at least a week until
I get around to respin this.

Thanks,
Ahmad

> 
> Sascha
> 
>>
>> пт, 6 окт. 2023 г. в 14:59, Sascha Hauer <sha@pengutronix.de>:
>>>
>>> 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@a3f.at>
>>>>>>
>>>>>> 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 |
>>
> 

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