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

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

[Michal Simek]

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

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

[Ramon Fried]

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>

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

[Michael Walle]

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

-michael

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

[Michal Simek]

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.

I found this via net list command where you can see controllers but you 
can't see their mac addresses which is IMHO wrong.

Thanks,
Michal

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

On Tue, Nov 02, 2021 at 11:27:22AM +0100, 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.
> 
> I found this via net list command where you can see controllers but you
> can't see their mac addresses which is IMHO wrong.

So, this causes dhcp to simply timeout for me on a pine64_plus board
(where yes, I believe we do end up using a random MAC) when running the
pytest suite.  I wonder if this particular change messes up the state
machine?  Checking the dnsmasq logs it looks like U-Boot keeps asking
and getting a reply, over and over.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Grygorii Strashko]

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.
For example, we occasionally copy-pasted wrong mac in DT and detected this only after some time when started connecting
similar boards to each other :(
Also, Linux may have it's own way to retrieve MAC if not provided by u-boot.

Wouldn't be enough to just print MAC console when random is used?

> 
> I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.


-- 
Best regards,
grygorii

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

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


> For example, we occasionally copy-pasted wrong mac in DT and detected 
> this only after some time when started connecting
> similar boards to each other :(

That's not issue with this patch.

> 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.
If fdt_fixup_ethernet() is called to update pass it to next phase is 
different story.
Also in our case when u-boot doesn't record mac to DT but mac remains in 
IP itself.

> 
> Wouldn't be enough to just print MAC console when random is used?

As I said above it is already there but IMHO not enough.
Take a look at behavior with this patch and without.

ZYNQ GEM: ff0b0000, mdio bus ff0c0000, phyaddr 4, interface sgmii
Loading new PMUFW cfg obj (32 bytes)

Warning: ethernet@ff0b0000 (eth0) using random MAC address - 
7a:77:b5:39:e8:12

ZYNQ GEM: ff0c0000, mdio bus ff0c0000, phyaddr 8, interface rgmii-id
Loading new PMUFW cfg obj (32 bytes)

Warning: ethernet@ff0c0000 (eth1) using random MAC address - 
32:77:95:89:df:0b
ethernet@ff0c0000 Waiting for PHY auto negotiation to complete....... done
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
DHCP client bound to address 192.168.0.143 (3260 ms)
gpio: pin resetchip5 (gpio 179) value is 0
gpio: pin resetchip5 (gpio 179) value is 1
ethernet@ff0b0000 Waiting for PHY auto negotiation to complete........ done
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
DHCP client bound to address 192.168.0.163 (3513 ms)
Hit any key to stop autoboot:  0
ZynqMP> net list
eth0 : ethernet@ff0b0000 7a:77:b5:39:e8:12 active
eth1 : ethernet@ff0c0000 32:77:95:89:df:0b
ZynqMP>


compare to
ZynqMP> net list
eth0 : ethernet@ff0b0000 00:00:00:00:00:00 active
eth1 : ethernet@ff0c0000 00:00:00:00:00:00

Thanks,
Michal

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

[Michal Simek]

On 11/3/21 17:57, Tom Rini wrote:
> On Tue, Nov 02, 2021 at 11:27:22AM +0100, 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.
>>
>> I found this via net list command where you can see controllers but you
>> can't see their mac addresses which is IMHO wrong.
> 
> So, this causes dhcp to simply timeout for me on a pine64_plus board
> (where yes, I believe we do end up using a random MAC) when running the
> pytest suite.  I wonder if this particular change messes up the state
> machine?  Checking the dnsmasq logs it looks like U-Boot keeps asking
> and getting a reply, over and over.

It is just saving that values to variables. If you want to propagete 
that values in the stack that's up to your system configuration.

What state machine are you talking about?

Thanks,
Michal

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3256 bytes --]

On Thu, Nov 04, 2021 at 12:18:46PM +0100, Michal Simek wrote:
> 
> 
> On 11/3/21 17:57, Tom Rini wrote:
> > On Tue, Nov 02, 2021 at 11:27:22AM +0100, 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.
> > > 
> > > I found this via net list command where you can see controllers but you
> > > can't see their mac addresses which is IMHO wrong.
> > 
> > So, this causes dhcp to simply timeout for me on a pine64_plus board
> > (where yes, I believe we do end up using a random MAC) when running the
> > pytest suite.  I wonder if this particular change messes up the state
> > machine?  Checking the dnsmasq logs it looks like U-Boot keeps asking
> > and getting a reply, over and over.
> 
> It is just saving that values to variables. If you want to propagete that
> values in the stack that's up to your system configuration.
> 
> What state machine are you talking about?

I was just making a guess about state machine, given what I had recalled
of the network stack before.  To be clearer, I bisected down my failure
of 3 tests that run "setenv autoload no ; dhcp" and then dhcp timing
out, to this commit (in the network PR).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Michal Simek]

On 11/4/21 12:37, Tom Rini wrote:
> On Thu, Nov 04, 2021 at 12:18:46PM +0100, Michal Simek wrote:
>>
>>
>> On 11/3/21 17:57, Tom Rini wrote:
>>> On Tue, Nov 02, 2021 at 11:27:22AM +0100, 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.
>>>>
>>>> I found this via net list command where you can see controllers but you
>>>> can't see their mac addresses which is IMHO wrong.
>>>
>>> So, this causes dhcp to simply timeout for me on a pine64_plus board
>>> (where yes, I believe we do end up using a random MAC) when running the
>>> pytest suite.  I wonder if this particular change messes up the state
>>> machine?  Checking the dnsmasq logs it looks like U-Boot keeps asking
>>> and getting a reply, over and over.
>>
>> It is just saving that values to variables. If you want to propagete that
>> values in the stack that's up to your system configuration.
>>
>> What state machine are you talking about?
> 
> I was just making a guess about state machine, given what I had recalled
> of the network stack before.  To be clearer, I bisected down my failure
> of 3 tests that run "setenv autoload no ; dhcp" and then dhcp timing
> out, to this commit (in the network PR).

First of all I didn't know that it was the part of networking pull 
request because didn't see apply message only reviewed from Ramon.

I would be very surprised this patch to caused your issue. It should be 
covering something else but that should be debugged.

Thanks,
Michal

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

[Michael Walle]

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

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4104 bytes --]

On Thu, Nov 04, 2021 at 12:43:11PM +0100, Michal Simek wrote:
> 
> 
> On 11/4/21 12:37, Tom Rini wrote:
> > On Thu, Nov 04, 2021 at 12:18:46PM +0100, Michal Simek wrote:
> > > 
> > > 
> > > On 11/3/21 17:57, Tom Rini wrote:
> > > > On Tue, Nov 02, 2021 at 11:27:22AM +0100, 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.
> > > > > 
> > > > > I found this via net list command where you can see controllers but you
> > > > > can't see their mac addresses which is IMHO wrong.
> > > > 
> > > > So, this causes dhcp to simply timeout for me on a pine64_plus board
> > > > (where yes, I believe we do end up using a random MAC) when running the
> > > > pytest suite.  I wonder if this particular change messes up the state
> > > > machine?  Checking the dnsmasq logs it looks like U-Boot keeps asking
> > > > and getting a reply, over and over.
> > > 
> > > It is just saving that values to variables. If you want to propagete that
> > > values in the stack that's up to your system configuration.
> > > 
> > > What state machine are you talking about?
> > 
> > I was just making a guess about state machine, given what I had recalled
> > of the network stack before.  To be clearer, I bisected down my failure
> > of 3 tests that run "setenv autoload no ; dhcp" and then dhcp timing
> > out, to this commit (in the network PR).
> 
> First of all I didn't know that it was the part of networking pull request
> because didn't see apply message only reviewed from Ramon.
> 
> I would be very surprised this patch to caused your issue. It should be
> covering something else but that should be debugged.

OK, I don't know why my git bisect run lead to this commit, but I've
manually bisected instead this time and found a much more reasonable
problematic commit.  Sorry for the noise.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Michal Simek]

On 11/4/21 13:59, Tom Rini wrote:
> On Thu, Nov 04, 2021 at 12:43:11PM +0100, Michal Simek wrote:
>>
>>
>> On 11/4/21 12:37, Tom Rini wrote:
>>> On Thu, Nov 04, 2021 at 12:18:46PM +0100, Michal Simek wrote:
>>>>
>>>>
>>>> On 11/3/21 17:57, Tom Rini wrote:
>>>>> On Tue, Nov 02, 2021 at 11:27:22AM +0100, 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.
>>>>>>
>>>>>> I found this via net list command where you can see controllers but you
>>>>>> can't see their mac addresses which is IMHO wrong.
>>>>>
>>>>> So, this causes dhcp to simply timeout for me on a pine64_plus board
>>>>> (where yes, I believe we do end up using a random MAC) when running the
>>>>> pytest suite.  I wonder if this particular change messes up the state
>>>>> machine?  Checking the dnsmasq logs it looks like U-Boot keeps asking
>>>>> and getting a reply, over and over.
>>>>
>>>> It is just saving that values to variables. If you want to propagete that
>>>> values in the stack that's up to your system configuration.
>>>>
>>>> What state machine are you talking about?
>>>
>>> I was just making a guess about state machine, given what I had recalled
>>> of the network stack before.  To be clearer, I bisected down my failure
>>> of 3 tests that run "setenv autoload no ; dhcp" and then dhcp timing
>>> out, to this commit (in the network PR).
>>
>> First of all I didn't know that it was the part of networking pull request
>> because didn't see apply message only reviewed from Ramon.
>>
>> I would be very surprised this patch to caused your issue. It should be
>> covering something else but that should be debugged.
> 
> OK, I don't know why my git bisect run lead to this commit, but I've
> manually bisected instead this time and found a much more reasonable
> problematic commit.  Sorry for the noise.
> 

Not big deal.

M

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

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

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

[Michael Walle]

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

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

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

[Ramon Fried]

On Thu, Nov 4, 2021 at 3:40 PM Michael Walle <michael@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@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.
>
> 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 ?

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

[Michael Walle]

Am 2021-11-04 22:00, schrieb Ramon Fried:

> What's the verdict ? We're keeping this patch ?

There was a good point from Grygorii: linux might deal
in its own way with missing ethernet addresses (and warn
the user about it because its usually an error).

And IMHO that net list can be fixed in a different way.

If this patch is accepted, it should be clearly documented
in the Kconfig help text. And as I said, it changes behavior.

-michael

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

[Michael Walle]

Hi Ramon,

Am 2021-11-09 14:55, schrieb Michael Walle:
> Am 2021-11-04 22:00, schrieb Ramon Fried:
> 
>> What's the verdict ? We're keeping this patch ?
> 
> There was a good point from Grygorii: linux might deal
> in its own way with missing ethernet addresses (and warn
> the user about it because its usually an error).
> 
> And IMHO that net list can be fixed in a different way.
> 
> If this patch is accepted, it should be clearly documented
> in the Kconfig help text. And as I said, it changes behavior.
> 

So this patch now in your pull request. Why are you even asking
then?

-michael

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

On Thu, Nov 11, 2021 at 10:10:57AM +0100, Michael Walle wrote:
> Hi Ramon,
> 
> Am 2021-11-09 14:55, schrieb Michael Walle:
> > Am 2021-11-04 22:00, schrieb Ramon Fried:
> > 
> > > What's the verdict ? We're keeping this patch ?
> > 
> > There was a good point from Grygorii: linux might deal
> > in its own way with missing ethernet addresses (and warn
> > the user about it because its usually an error).
> > 
> > And IMHO that net list can be fixed in a different way.
> > 
> > If this patch is accepted, it should be clearly documented
> > in the Kconfig help text. And as I said, it changes behavior.
> 
> So this patch now in your pull request. Why are you even asking
> then?

So, to quote lib/Kconfig:
config NET_RANDOM_ETHADDR
        bool "Random ethaddr if unset"
        help
          Selecting this will allow the Ethernet interface to function
          even when the ethaddr variable for that interface is unset.
          A new MAC address will be generated on every boot and it will
          not be added to the environment.

We need either a re-spin or follow-up as we're changing the documented
behavior.  And as I mentioned in the other thread related on-going
thread, perhaps "ethmacskip" should play a role in preserving existing
behavior?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Wolfgang Denk]

Dear Tom,

In message <20211116141855.GD24579@bill-the-cat> you wrote:
> 
> So, to quote lib/Kconfig:
> config NET_RANDOM_ETHADDR
>         bool "Random ethaddr if unset"
>         help
>           Selecting this will allow the Ethernet interface to function
>           even when the ethaddr variable for that interface is unset.
>           A new MAC address will be generated on every boot and it will
>           not be added to the environment.

This description is at least incomplete, because it makes no
difference between the persistent copy of the environment and it's
in-memory copy.  For network to function, I think the MAC address
must be stored in the in-memory copy of the environment.

> We need either a re-spin or follow-up as we're changing the documented
> behavior.  And as I mentioned in the other thread related on-going
> thread, perhaps "ethmacskip" should play a role in preserving existing
> behavior?

We have way too many ways to do the same thing - nearly, just a
little different :-(

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"A little knowledge is a dangerous thing."                - Doug Gwyn

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

On Tue, Nov 16, 2021 at 03:56:45PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211116141855.GD24579@bill-the-cat> you wrote:
> > 
> > So, to quote lib/Kconfig:
> > config NET_RANDOM_ETHADDR
> >         bool "Random ethaddr if unset"
> >         help
> >           Selecting this will allow the Ethernet interface to function
> >           even when the ethaddr variable for that interface is unset.
> >           A new MAC address will be generated on every boot and it will
> >           not be added to the environment.
> 
> This description is at least incomplete, because it makes no
> difference between the persistent copy of the environment and it's
> in-memory copy.  For network to function, I think the MAC address
> must be stored in the in-memory copy of the environment.

Well, networking has been working with NET_RANDOM_ETHADDR not updating
the environment, with caveats that user display information that reads
from the environment rather than ->enetaddr.  In so far as the
networking stack cares, ->enetaddr is what matters.  We look at the
environment for a MAC, and if it doesn't match the HW we use environment
MAC, but warn.

Now, I think RFC7042 is the current correct and relevant RFC here and it
doesn't mention persistence.  So I don't see a clear external authority
on if locally administered MAC addresses like this should be persistent
or not in this manner.

> > We need either a re-spin or follow-up as we're changing the documented
> > behavior.  And as I mentioned in the other thread related on-going
> > thread, perhaps "ethmacskip" should play a role in preserving existing
> > behavior?
> 
> We have way too many ways to do the same thing - nearly, just a
> little different :-(

Well, in this case I'm not sure that's the right problem to point at.
We can just set ethmacskip as a bit of corner-case functionality and
move on.

Because honestly, the more I read this, the more I think
https://patchwork.ozlabs.org/project/uboot/patch/20211115121152.3470910-1-michael@walle.cc/
is essentially the right direction.  There's no reason for 'net list' to
be using the environment here when ->enetaddr is what's being used by
the stack.  The use case of "I want to make my locally administered MAC
persist because my USB ethernet adapter lacks a MAC address" is solved
via the environment already.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Wolfgang Denk]

Dear Tom,

In message <20211116184146.GF24579@bill-the-cat> you wrote:
> 
> Because honestly, the more I read this, the more I think
> https://patchwork.ozlabs.org/project/uboot/patch/20211115121152.3470910-1-m=
> ichael@walle.cc/
> is essentially the right direction.  There's no reason for 'net list' to
> be using the environment here when ->enetaddr is what's being used by
> the stack.  The use case of "I want to make my locally administered MAC
> persist because my USB ethernet adapter lacks a MAC address" is solved
> via the environment already.

If the MAC address is not placed in the environment, then how can a
user query the currently used MAC address?  All documentation says
basically: run "printenv ethaddr".

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"Plan to throw one away. You will anyway."
                              - Fred Brooks, "The Mythical Man Month"

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On Wed, Nov 17, 2021 at 08:44:52AM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211116184146.GF24579@bill-the-cat> you wrote:
> > 
> > Because honestly, the more I read this, the more I think
> > https://patchwork.ozlabs.org/project/uboot/patch/20211115121152.3470910-1-m=
> > ichael@walle.cc/
> > is essentially the right direction.  There's no reason for 'net list' to
> > be using the environment here when ->enetaddr is what's being used by
> > the stack.  The use case of "I want to make my locally administered MAC
> > persist because my USB ethernet adapter lacks a MAC address" is solved
> > via the environment already.
> 
> If the MAC address is not placed in the environment, then how can a
> user query the currently used MAC address?  All documentation says
> basically: run "printenv ethaddr".

Update the documentation and say to use "net list" or read the previous
part of console log that says we're using a random mac in this case?
The more I look here the more I see we're changing part of the API with
the OS, and it's not something that should be done without consulting
the consumers too.

I should note first that I got how the "USB ethernet that lacks HW
assigned MAC" case has worked since forever (the beagleboard xM is the
first case that springs to mind and that's over 10 years old).  That's
"run gen_eth_addr, setenv ethaddr $X, saveenv" as yes, our randomly
generated MAC within U-Boot has never been super easy to have persist,
possibly intentionally.  Possibly not!  But it's an API we've had for
over 10 years now.

Next, pulling up
https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-controller.yaml
there's two important things.  First, there's no way to flag "this is a
random MAC, do not use" (if after all, that's what the user wants, such
as in Michael's case).  And second, yes we probably ought to have
enforced "mac-address" being the random one we had been using, all
along.  Then we pull up
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/core/of_net.c#n98
and see that oh the kernel's already made some assumptions about what
might be set where and why because of seemingly arbitrary things we were
doing that may or may not have matched up with the at the time binding.

So no, in sum, I'm not convinced that the best path forward right here
and now is to put the random MAC in the environment, not correct the now
incorrect help text and not even poke the binding maintainer nor
relevant lists to see how exactly they think it would be best to handle
a locally administered MAC being used as there are both valid use cases
for "use this in the kernel" and "disregard this in the kernel".

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Wolfgang Denk]

Dear Tom,

In message <20211117115025.GV24579@bill-the-cat> you wrote:
> 
> > If the MAC address is not placed in the environment, then how can a
> > user query the currently used MAC address?  All documentation says
> > basically: run "printenv ethaddr".
>
> Update the documentation and say to use "net list" or read the previous
> part of console log that says we're using a random mac in this case?
> The more I look here the more I see we're changing part of the API with
> the OS, and it's not something that should be done without consulting
> the consumers too.

Well... "printenv ethaddr" has been here ever since U-Boot (or
PPCBoot, as it was still called at that time) got network support.
i. e. more than 21 years ago.

"net list: has only been added very recently, just 5 months ago in
commit 8a3987f47 "cmd: net: add a 'net list' command to list network
devs".

I bet that an overwhelming majority of users would use "printenv
ethaddr" when asked to check the MAC address of a system.


> Next, pulling up
> https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-c=
> ontroller.yaml
> there's two important things.  First, there's no way to flag "this is a
> random MAC, do not use" (if after all, that's what the user wants, such
> as in Michael's case).  And second, yes we probably ought to have
> enforced "mac-address" being the random one we had been using, all
> along.

Why would you expect any "do not use" note?  Locally adminitered MAC
addresses (even when randomly chosen) have been created for good
reason, so they should be usable.

> So no, in sum, I'm not convinced that the best path forward right here
> and now is to put the random MAC in the environment, not correct the now
> incorrect help text and not even poke the binding maintainer nor
> relevant lists to see how exactly they think it would be best to handle
> a locally administered MAC being used as there are both valid use cases
> for "use this in the kernel" and "disregard this in the kernel".

I'm afraid I do not understand what exactly you are proposing here?

But I object to using a MAC address in U-Boot in a way that makes it
invisible to the user who uses documented APIs ("printenv ethaddr").

If we use some MAC address, it shall be possible to read it using
"printenv ethaddr" and to set it using "setenv ethaddr".  Anything
else is inconsistent crap.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Success in marriage is not so much finding the right person as it  is
being the right person.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3999 bytes --]

On Wed, Nov 17, 2021 at 01:24:58PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211117115025.GV24579@bill-the-cat> you wrote:
> > 
> > > If the MAC address is not placed in the environment, then how can a
> > > user query the currently used MAC address?  All documentation says
> > > basically: run "printenv ethaddr".
> >
> > Update the documentation and say to use "net list" or read the previous
> > part of console log that says we're using a random mac in this case?
> > The more I look here the more I see we're changing part of the API with
> > the OS, and it's not something that should be done without consulting
> > the consumers too.
> 
> Well... "printenv ethaddr" has been here ever since U-Boot (or
> PPCBoot, as it was still called at that time) got network support.
> i. e. more than 21 years ago.
> 
> "net list: has only been added very recently, just 5 months ago in
> commit 8a3987f47 "cmd: net: add a 'net list' command to list network
> devs".

OK, so "net list" hasn't been broken that long, good.

> I bet that an overwhelming majority of users would use "printenv
> ethaddr" when asked to check the MAC address of a system.

And it's not shown the randomly generated one for that long either.

> > Next, pulling up
> > https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-c=
> > ontroller.yaml
> > there's two important things.  First, there's no way to flag "this is a
> > random MAC, do not use" (if after all, that's what the user wants, such
> > as in Michael's case).  And second, yes we probably ought to have
> > enforced "mac-address" being the random one we had been using, all
> > along.
> 
> Why would you expect any "do not use" note?  Locally adminitered MAC
> addresses (even when randomly chosen) have been created for good
> reason, so they should be usable.

Because as been noted in either this thread, or the other long thread,
the user does not want to confuse Linux with this emergency random MAC?

> > So no, in sum, I'm not convinced that the best path forward right here
> > and now is to put the random MAC in the environment, not correct the now
> > incorrect help text and not even poke the binding maintainer nor
> > relevant lists to see how exactly they think it would be best to handle
> > a locally administered MAC being used as there are both valid use cases
> > for "use this in the kernel" and "disregard this in the kernel".
> 
> I'm afraid I do not understand what exactly you are proposing here?

I'm objecting to changing our long standing behavior of NOT
automatically passing the random MAC to the OS.  That has always been
user opt-in by using tools/gen_eth_addr and "setenv ethaddr ... ;
saveenv".  Especially since I don't know how many of those 200+ boards
that enable CONFIG_NET_RANDOM_ETHADDR today are "enabled, but never
used" vs "enabled, random MAC in U-Boot since we don't care/didn't
notice, real MAC in Linux".  So yes, another worry is that we have a
class of boards in U-Boot where a random MAC is fine enough since it's
rarely used/needed, but Linux can get the real MAC and now we'll be
blowing that away.  Or maybe we just have a ton of boards that
copypasta'd that option in and didn't really think why.

> But I object to using a MAC address in U-Boot in a way that makes it
> invisible to the user who uses documented APIs ("printenv ethaddr").

Well, that's the API we've had for over 10 years, and it was a common
problem in those earlier days with lots of SBCs where it was cheap to
toss in a USB eth controller that didn't store a MAC anywhere.  Now
we're I believe hitting this due to FPGA stuff.

> If we use some MAC address, it shall be possible to read it using
> "printenv ethaddr" and to set it using "setenv ethaddr".  Anything
> else is inconsistent crap.

Well, we've been inconsistent about the former for forever and there's
a lot of implications to changing it now.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Wolfgang Denk]

Dear Tom,

In message <20211117123548.GX24579@bill-the-cat> you wrote:
> 
> > Why would you expect any "do not use" note?  Locally adminitered MAC
> > addresses (even when randomly chosen) have been created for good
> > reason, so they should be usable.
>
> Because as been noted in either this thread, or the other long thread,
> the user does not want to confuse Linux with this emergency random MAC?

Classical answer: Don't do it, then.

If the user does not want to pass a MAC address to Linux, he should
simply not do it.  Deleting "ethaddr" from the environment should be
all that's needed.  [And if this does not work, then I consider this
a bug that should be fixed.]

> > I'm afraid I do not understand what exactly you are proposing here?
>
> I'm objecting to changing our long standing behavior of NOT
> automatically passing the random MAC to the OS.  That has always been
> user opt-in by using tools/gen_eth_addr and "setenv ethaddr ... ;
> saveenv".  Especially since I don't know how many of those 200+ boards
> that enable CONFIG_NET_RANDOM_ETHADDR today are "enabled, but never
> used" vs "enabled, random MAC in U-Boot since we don't care/didn't
> notice, real MAC in Linux".  So yes, another worry is that we have a
> class of boards in U-Boot where a random MAC is fine enough since it's
> rarely used/needed, but Linux can get the real MAC and now we'll be
> blowing that away.  Or maybe we just have a ton of boards that
> copypasta'd that option in and didn't really think why.

Unfortunately we can only speculate about that [my guess is that
most of them are just copy/paste while brain in low power mode].

> > But I object to using a MAC address in U-Boot in a way that makes it
> > invisible to the user who uses documented APIs ("printenv ethaddr").
>
> Well, that's the API we've had for over 10 years, and it was a common
> problem in those earlier days with lots of SBCs where it was cheap to
> toss in a USB eth controller that didn't store a MAC anywhere.  Now
> we're I believe hitting this due to FPGA stuff.

CONFIG_NET_RANDOM_ETHADDR has been added in 2015 with commit bef1014
"net: Implement random ethaddr fallback in eth.c"; from the changes
then I'm not sure if this was even USB related.

> > If we use some MAC address, it shall be possible to read it using
> > "printenv ethaddr" and to set it using "setenv ethaddr".  Anything
> > else is inconsistent crap.
>
> Well, we've been inconsistent about the former for forever and there's
> a lot of implications to changing it now.

Yes.  However, being bug-compatible is something we should not rate
too high.

[non-random signature used]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"The net result is a system that is not only binary  compatible  with
4.3  BSD, but is even bug for bug compatible in almost all features."
- Avadit  Tevanian,  Jr.,  "Architecture-Independent  Virtual  Memory
Management  for  Parallel  and  Distributed  Environments:  The  Mach
Approach"

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5377 bytes --]

On Wed, Nov 17, 2021 at 04:56:27PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211117123548.GX24579@bill-the-cat> you wrote:
> > 
> > > Why would you expect any "do not use" note?  Locally adminitered MAC
> > > addresses (even when randomly chosen) have been created for good
> > > reason, so they should be usable.
> >
> > Because as been noted in either this thread, or the other long thread,
> > the user does not want to confuse Linux with this emergency random MAC?
> 
> Classical answer: Don't do it, then.
> 
> If the user does not want to pass a MAC address to Linux, he should
> simply not do it.  Deleting "ethaddr" from the environment should be
> all that's needed.  [And if this does not work, then I consider this
> a bug that should be fixed.]

Yes, you're changing behavior by requiring this change, and fwiw I
suggested a slightly different fix-up here of deleting the device tree
properties if it's a random MAC, in Michael's patch just disabling the
feature on the platforms he cares about.

> > > I'm afraid I do not understand what exactly you are proposing here?
> >
> > I'm objecting to changing our long standing behavior of NOT
> > automatically passing the random MAC to the OS.  That has always been
> > user opt-in by using tools/gen_eth_addr and "setenv ethaddr ... ;
> > saveenv".  Especially since I don't know how many of those 200+ boards
> > that enable CONFIG_NET_RANDOM_ETHADDR today are "enabled, but never
> > used" vs "enabled, random MAC in U-Boot since we don't care/didn't
> > notice, real MAC in Linux".  So yes, another worry is that we have a
> > class of boards in U-Boot where a random MAC is fine enough since it's
> > rarely used/needed, but Linux can get the real MAC and now we'll be
> > blowing that away.  Or maybe we just have a ton of boards that
> > copypasta'd that option in and didn't really think why.
> 
> Unfortunately we can only speculate about that [my guess is that
> most of them are just copy/paste while brain in low power mode].

I've asked Neil to chime in on the amlogic cases after talking on IRC,
but the short answer was for prior to fuse/serial# reading support for a
non-random MAC.  Possibly other SoCs in a similar situation.

> > > But I object to using a MAC address in U-Boot in a way that makes it
> > > invisible to the user who uses documented APIs ("printenv ethaddr").
> >
> > Well, that's the API we've had for over 10 years, and it was a common
> > problem in those earlier days with lots of SBCs where it was cheap to
> > toss in a USB eth controller that didn't store a MAC anywhere.  Now
> > we're I believe hitting this due to FPGA stuff.
> 
> CONFIG_NET_RANDOM_ETHADDR has been added in 2015 with commit bef1014
> "net: Implement random ethaddr fallback in eth.c"; from the changes
> then I'm not sure if this was even USB related.

Well, so that explains how / why we did things even longer ago the way
we did.  I thought for some reason CONFIG_NET_RANDOM_ETHADDR had been
even older.

> > > If we use some MAC address, it shall be possible to read it using
> > > "printenv ethaddr" and to set it using "setenv ethaddr".  Anything
> > > else is inconsistent crap.
> >
> > Well, we've been inconsistent about the former for forever and there's
> > a lot of implications to changing it now.
> 
> Yes.  However, being bug-compatible is something we should not rate
> too high.
> 
> [non-random signature used]
> 
> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "The net result is a system that is not only binary  compatible  with
> 4.3  BSD, but is even bug for bug compatible in almost all features."
> - Avadit  Tevanian,  Jr.,  "Architecture-Independent  Virtual  Memory
> Management  for  Parallel  and  Distributed  Environments:  The  Mach
> Approach"

I don't mean this in a super snarky way, but I'm more concerned about
the implications of changing our documented albeit slightly inconsistent
behavior than I am about some of the myriad of technically feasible
environment variable names we've also in theory supported.  For about
half the time we've supported device trees, the solution to "I need to
use a random MAC" was "run tools/gen_eth_addr, setenv, saveenv".  For
the second half of the time, it was the same, but with a side of "or note
the random MAC from console logs and use that".  We're about to
introduce the 3rd variant.  I'd feel a whole lot better about taking in
a v2 of this patch that correct the help (and maybe updates
doc/README.enetaddr!) if someone could report back on what's going on
with the layerscape, imx* and socfpga platforms.  There's in fact a
number of platforms enabling it that I'm pretty sure DENX has or had the
hardware on, so can we get some spot checking done there?  If we can
drop from 250 platforms to 50 platforms with some level of confidence we
understand why are setting NET_RANDOM_ETHADDR, I'd be a lot less worried
we're about to introduce another change that we won't found out messed
up a bunch of people on until some new work-around is accepted in to
Linux or something.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Wolfgang Denk]

Dear Tom,

In message <20211117161545.GA24579@bill-the-cat> you wrote:
> 
> Yes, you're changing behavior by requiring this change, and fwiw I
> suggested a slightly different fix-up here of deleting the device tree
> properties if it's a random MAC, in Michael's patch just disabling the
> feature on the platforms he cares about.

Of course fixing a bug will change behaviour; that's the intention
of the fix.

Technically there is no difference between the user setting
"ethaddr" manually to a locally administered MAC address or doing
this automatically in some code or script.  In all cases setting
"ethaddr" means: hey, this is my MAC address, please use it.

> I've asked Neil to chime in on the amlogic cases after talking on IRC,
> but the short answer was for prior to fuse/serial# reading support for a
> non-random MAC.  Possibly other SoCs in a similar situation.

It is perfectly OK for U-Boot to start with a random MAC address,
use this for a while, and change it so something else later.  This
is what may happen at production: say the MAC address is stored in
some EEPROM or fuses, which are initially empty, so U-Boot will use
a random MAC address, download it's board specific date (serial#,
MAC address, ...) over network, programm it into the respective
storage devices, and switch to using the new "official" MAC address.

> I don't mean this in a super snarky way, but I'm more concerned about
> the implications of changing our documented albeit slightly inconsistent
> behavior than I am about some of the myriad of technically feasible
> environment variable names we've also in theory supported.  For about
> half the time we've supported device trees, the solution to "I need to
> use a random MAC" was "run tools/gen_eth_addr, setenv, saveenv".

This is indeed what seems a straightforward approach to me.  The
problem here is that this needs to be done manually.
CONFIG_NET_RANDOM_ETHADDR is the attempt to do the same
automatically, except that it falls short of setting the "ethaddr"
environment variable.  I consider this a bug.

> For
> the second half of the time, it was the same, but with a side of "or note
> the random MAC from console logs and use that".

Yes, and it should be clear that this is not a reasonable approach.
It thwarts the original intent of the NET_RANDOM_ETHADDR patch to do
thing in an automated, scriptable way. I see this actually as a
manifestation of the bug that ethaddr does not get set. At this
point the problem was recognized, but instead of properly fixing it,
a poor workaround was documented.

> We're about to
> introduce the 3rd variant.  I'd feel a whole lot better about taking in
> a v2 of this patch that correct the help (and maybe updates
> doc/README.enetaddr!) if someone could report back on what's going on
> with the layerscape, imx* and socfpga platforms.  There's in fact a
> number of platforms enabling it that I'm pretty sure DENX has or had the
> hardware on, so can we get some spot checking done there?

I will check and provide an update later, but from the best of my
knowledge none of the boards we ported actually need or use this
feature.  This is just a copy&paste mess.

> If we can
> drop from 250 platforms to 50 platforms with some level of confidence we
> understand why are setting NET_RANDOM_ETHADDR, I'd be a lot less worried
> we're about to introduce another change that we won't found out messed
> up a bunch of people on until some new work-around is accepted in to
> Linux or something.

Well, this work-around in Linux is because there have been
incnsistent ways how to store the MAC address in the device tree.
This has nothing to do with our problem here - Linux has no way to
know whether a locally administered MAC address has been assigned by
the user for a specific purpose, or if it has been generated
randomly.  Actually it does not even care.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
You know that feeling when you're leaning back  on  a  stool  and  it
starts to tip over? Well, that's how I feel all the time.
- Steven Wright

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

[Wolfgang Denk]

Dear Tom,

In message <1889944.1637219294@gemini.denx.de> I wrote:
>
> > We're about to
> > introduce the 3rd variant.  I'd feel a whole lot better about taking in
> > a v2 of this patch that correct the help (and maybe updates
> > doc/README.enetaddr!) if someone could report back on what's going on
> > with the layerscape, imx* and socfpga platforms.  There's in fact a
> > number of platforms enabling it that I'm pretty sure DENX has or had the
> > hardware on, so can we get some spot checking done there?
>
> I will check and provide an update later, but from the best of my
> knowledge none of the boards we ported actually need or use this
> feature.  This is just a copy&paste mess.

We could not find even a single board maintained or upstreamed
by DENX where NET_RANDOM_ETHADDR was even mentioned in the
specifications, and certainly not a mandatory feature.

That's all copy & paste garbage.

Viele Grüße,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The human mind treats a new idea the way the body  treats  a  strange
protein - it rejects it.                                 - P. Medawar

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

On Thu, Nov 18, 2021 at 10:46:28AM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <1889944.1637219294@gemini.denx.de> I wrote:
> >
> > > We're about to
> > > introduce the 3rd variant.  I'd feel a whole lot better about taking in
> > > a v2 of this patch that correct the help (and maybe updates
> > > doc/README.enetaddr!) if someone could report back on what's going on
> > > with the layerscape, imx* and socfpga platforms.  There's in fact a
> > > number of platforms enabling it that I'm pretty sure DENX has or had the
> > > hardware on, so can we get some spot checking done there?
> >
> > I will check and provide an update later, but from the best of my
> > knowledge none of the boards we ported actually need or use this
> > feature.  This is just a copy&paste mess.
> 
> We could not find even a single board maintained or upstreamed
> by DENX where NET_RANDOM_ETHADDR was even mentioned in the
> specifications, and certainly not a mandatory feature.
> 
> That's all copy & paste garbage.

OK, can we please get some patches going then to remove them, starting
with the platforms you have, and then the reference platforms it was
copypastad over from?  Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6781 bytes --]

On Thu, Nov 18, 2021 at 08:08:14AM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211117161545.GA24579@bill-the-cat> you wrote:
> > 
> > Yes, you're changing behavior by requiring this change, and fwiw I
> > suggested a slightly different fix-up here of deleting the device tree
> > properties if it's a random MAC, in Michael's patch just disabling the
> > feature on the platforms he cares about.
> 
> Of course fixing a bug will change behaviour; that's the intention
> of the fix.
> 
> Technically there is no difference between the user setting
> "ethaddr" manually to a locally administered MAC address or doing
> this automatically in some code or script.  In all cases setting
> "ethaddr" means: hey, this is my MAC address, please use it.

Right, I'm not saying those parts are the behavior change.

> > I've asked Neil to chime in on the amlogic cases after talking on IRC,
> > but the short answer was for prior to fuse/serial# reading support for a
> > non-random MAC.  Possibly other SoCs in a similar situation.
> 
> It is perfectly OK for U-Boot to start with a random MAC address,
> use this for a while, and change it so something else later.  This
> is what may happen at production: say the MAC address is stored in
> some EEPROM or fuses, which are initially empty, so U-Boot will use
> a random MAC address, download it's board specific date (serial#,
> MAC address, ...) over network, programm it into the respective
> storage devices, and switch to using the new "official" MAC address.

Yes.  And up until this patch saying I want to use this random MAC with
Linux required user intervention.  And I dare say that half the time or
more that was probably just not noticed (everything comes up with
dynamic host names/dns these days, noticing the IP changed between
U-Boot and Linux is easy to miss in those cases).

> > I don't mean this in a super snarky way, but I'm more concerned about
> > the implications of changing our documented albeit slightly inconsistent
> > behavior than I am about some of the myriad of technically feasible
> > environment variable names we've also in theory supported.  For about
> > half the time we've supported device trees, the solution to "I need to
> > use a random MAC" was "run tools/gen_eth_addr, setenv, saveenv".
> 
> This is indeed what seems a straightforward approach to me.  The
> problem here is that this needs to be done manually.

It's always needed to be manual, yes.

> CONFIG_NET_RANDOM_ETHADDR is the attempt to do the same
> automatically, except that it falls short of setting the "ethaddr"
> environment variable.  I consider this a bug.

Since the code isn't that old, it shouldn't be hard to pull up the
thread / patch on introducing it.  So, lets:
https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-1-git-send-email-joe.hershberger@ni.com/
https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-send-email-joe.hershberger@ni.com/
https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-send-email-joe.hershberger@ni.com/

And from there we can take away I think two important things:
1: Not setting ethaddr with NET_RANDOM_ETHADDR is intentional
2: It was widely inconsistent before!  Lots of platforms were generating
   a random MAC and then setting ethaddr.  I think in fact most were and
   it's just a few (which coincidentally are some of Michael's other
   platforms) that were not.

> > For
> > the second half of the time, it was the same, but with a side of "or note
> > the random MAC from console logs and use that".
> 
> Yes, and it should be clear that this is not a reasonable approach.
> It thwarts the original intent of the NET_RANDOM_ETHADDR patch to do
> thing in an automated, scriptable way. I see this actually as a
> manifestation of the bug that ethaddr does not get set. At this
> point the problem was recognized, but instead of properly fixing it,
> a poor workaround was documented.

Well, for the record, with the above patch links, NET_RANDOM_ETHADDR
_is_ working as intended.  That said..

> > We're about to
> > introduce the 3rd variant.  I'd feel a whole lot better about taking in
> > a v2 of this patch that correct the help (and maybe updates
> > doc/README.enetaddr!) if someone could report back on what's going on
> > with the layerscape, imx* and socfpga platforms.  There's in fact a
> > number of platforms enabling it that I'm pretty sure DENX has or had the
> > hardware on, so can we get some spot checking done there?
> 
> I will check and provide an update later, but from the best of my
> knowledge none of the boards we ported actually need or use this
> feature.  This is just a copy&paste mess.

Thanks!

> > If we can
> > drop from 250 platforms to 50 platforms with some level of confidence we
> > understand why are setting NET_RANDOM_ETHADDR, I'd be a lot less worried
> > we're about to introduce another change that we won't found out messed
> > up a bunch of people on until some new work-around is accepted in to
> > Linux or something.
> 
> Well, this work-around in Linux is because there have been
> incnsistent ways how to store the MAC address in the device tree.
> This has nothing to do with our problem here - Linux has no way to
> know whether a locally administered MAC address has been assigned by
> the user for a specific purpose, or if it has been generated
> randomly.  Actually it does not even care.

So, looking deeper at the history (and talking with Rob off-list), the
binding is messy since things have been messy here since back when DT
was just for PowerPC days, and was not ABI either.

Now, looking at everything again, and including what looks to be closer
to how the mac-address/local-mac-address fixup in DTs worked prior to
NET_RANDOM_ETHADDR being introduced, I need to change my position
slightly.  The way things worked prior to NET_RANDOM_ETHADDR was that
many/most platforms did make a random MAC if needed and then setenv
ethaddr, so fdt_fixup_ethernet() would populate that to the kernel (IF
those platforms did DT at the time of course).  And the
ethernet-controller binding really does just say it wants the MAC that
was most recently used, so it should be that random MAC in those cases.
So, yes, OK, this is a bug fix, in that NET_RANDOM_ETHADDR was incorrect
at introduction.  That means we do need a v2 of this patch that updates
the Kconfig help text as that currently says it will not update the
environment.

And then yes, if platforms can justify a reason to not populate that
random MAC, they need to implement a board-specific change of some sort
or another for the specific use case.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Wolfgang Denk]

Dear Tom,

In message <20211118162920.GH24579@bill-the-cat> you wrote:
> 
> > It is perfectly OK for U-Boot to start with a random MAC address,
> > use this for a while, and change it so something else later.  This
> > is what may happen at production: say the MAC address is stored in
> > some EEPROM or fuses, which are initially empty, so U-Boot will use
> > a random MAC address, download it's board specific date (serial#,
> > MAC address, ...) over network, programm it into the respective
> > storage devices, and switch to using the new "official" MAC address.
>
> Yes.  And up until this patch saying I want to use this random MAC with
> Linux required user intervention.

Correct - this is a bug in the implementation of thispatch, and
apparently the few people that ever used it did not notice it or
care enough about it to submit fixes.

> And I dare say that half the time or
> more that was probably just not noticed (everything comes up with
> dynamic host names/dns these days, noticing the IP changed between
> U-Boot and Linux is easy to miss in those cases).

Agreed.

> > CONFIG_NET_RANDOM_ETHADDR is the attempt to do the same
> > automatically, except that it falls short of setting the "ethaddr"
> > environment variable.  I consider this a bug.
>
> Since the code isn't that old, it shouldn't be hard to pull up the
> thread / patch on introducing it.  So, lets:
> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-1-git-sen=
> d-email-joe.hershberger@ni.com/
> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen=
> d-email-joe.hershberger@ni.com/
> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen=
> d-email-joe.hershberger@ni.com/
>
> And from there we can take away I think two important things:
> 1: Not setting ethaddr with NET_RANDOM_ETHADDR is intentional

Ummm... From which part of the patches or the comments do you take
this conclusion?  Not a single line of code, or comments in the code
or commit messages, nor any of the review comments refer to the
"ethadddr" environment variable.  I thing it was just an oversight
to set it here, which was not detected during testing as everything
appeared to work.

I actually see the opposite - Joes initial commit messages speaks
about "Implement the random *ethaddr* fallback", and as his
following patches clean up the use of hard codes definitions of the
"ethaddr" environment variable, I see it more likely that he means
the environment variable and not MAC address.

> 2: It was widely inconsistent before!  Lots of platforms were generating
>    a random MAC and then setting ethaddr.  I think in fact most were and
>    it's just a few (which coincidentally are some of Michael's other
>    platforms) that were not.

Agreed.

> > Yes, and it should be clear that this is not a reasonable approach.
> > It thwarts the original intent of the NET_RANDOM_ETHADDR patch to do
> > thing in an automated, scriptable way. I see this actually as a
> > manifestation of the bug that ethaddr does not get set. At this
> > point the problem was recognized, but instead of properly fixing it,
> > a poor workaround was documented.
>
> Well, for the record, with the above patch links, NET_RANDOM_ETHADDR
> _is_ working as intended.  That said..

No, it works only half.  It fails to set the "ethaddr" environment
variable which is mandatory for correct funktion (query by the user,
passing to Linux).

> So, yes, OK, this is a bug fix, in that NET_RANDOM_ETHADDR was incorrect
> at introduction.  That means we do need a v2 of this patch that updates
> the Kconfig help text as that currently says it will not update the
> environment.

This makes no sense to me.  Instead of documenting the bug we should
fix it and add the missing eth_env_set_enetaddr().

If I prepare such patches, will you accept these?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Microsoft Multitasking:
                     several applications can crash at the same time.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]

On Thu, Nov 18, 2021 at 08:04:22PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211118162920.GH24579@bill-the-cat> you wrote:
> > 
> > > It is perfectly OK for U-Boot to start with a random MAC address,
> > > use this for a while, and change it so something else later.  This
> > > is what may happen at production: say the MAC address is stored in
> > > some EEPROM or fuses, which are initially empty, so U-Boot will use
> > > a random MAC address, download it's board specific date (serial#,
> > > MAC address, ...) over network, programm it into the respective
> > > storage devices, and switch to using the new "official" MAC address.
> >
> > Yes.  And up until this patch saying I want to use this random MAC with
> > Linux required user intervention.
> 
> Correct - this is a bug in the implementation of thispatch, and
> apparently the few people that ever used it did not notice it or
> care enough about it to submit fixes.
> 
> > And I dare say that half the time or
> > more that was probably just not noticed (everything comes up with
> > dynamic host names/dns these days, noticing the IP changed between
> > U-Boot and Linux is easy to miss in those cases).
> 
> Agreed.
> 
> > > CONFIG_NET_RANDOM_ETHADDR is the attempt to do the same
> > > automatically, except that it falls short of setting the "ethaddr"
> > > environment variable.  I consider this a bug.
> >
> > Since the code isn't that old, it shouldn't be hard to pull up the
> > thread / patch on introducing it.  So, lets:
> > https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-1-git-sen=
> > d-email-joe.hershberger@ni.com/
> > https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen=
> > d-email-joe.hershberger@ni.com/
> > https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen=
> > d-email-joe.hershberger@ni.com/
> >
> > And from there we can take away I think two important things:
> > 1: Not setting ethaddr with NET_RANDOM_ETHADDR is intentional
> 
> Ummm... From which part of the patches or the comments do you take
> this conclusion?  Not a single line of code, or comments in the code

The same part I keep asking to have fixed in a v2, the help text by the
Kconfig option:
+	  A new MAC address will be generated on every boot and it will
+	  not be added to the environment.

[snip]
> > So, yes, OK, this is a bug fix, in that NET_RANDOM_ETHADDR was incorrect
> > at introduction.  That means we do need a v2 of this patch that updates
> > the Kconfig help text as that currently says it will not update the
> > environment.
> 
> This makes no sense to me.  Instead of documenting the bug we should
> fix it and add the missing eth_env_set_enetaddr().
> 
> If I prepare such patches, will you accept these?

Now I'm confused.  Taking the patch this whole thread is attached to, we
make NET_RANDOM_ETHADDR perform eth_env_set_enetaddr_by_index which is
missing (well, only in the DM case, the non-DM case isn't updated and I
guess should also be?).  My problem is that the Kconfig help text for
NET_RANDOM_ETHADDR still reflects what I pasted from the initial commit
of it, a sentence saying we don't update the environment.  This change
does, so the help is wrong and needs fixing.  I have come around to
agreeing with the concept of this patch, that NET_RANDOM_ETHADDR should
cause the environment to be updated and in turn fdt_fixup_ethernet()
will populate this to Linux.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Michal Simek]

Hi,

On 11/18/21 20:54, Tom Rini wrote:
> On Thu, Nov 18, 2021 at 08:04:22PM +0100, Wolfgang Denk wrote:
>> Dear Tom,
>>
>> In message <20211118162920.GH24579@bill-the-cat> you wrote:
>>>
>>>> It is perfectly OK for U-Boot to start with a random MAC address,
>>>> use this for a while, and change it so something else later.  This
>>>> is what may happen at production: say the MAC address is stored in
>>>> some EEPROM or fuses, which are initially empty, so U-Boot will use
>>>> a random MAC address, download it's board specific date (serial#,
>>>> MAC address, ...) over network, programm it into the respective
>>>> storage devices, and switch to using the new "official" MAC address.
>>>
>>> Yes.  And up until this patch saying I want to use this random MAC with
>>> Linux required user intervention.
>>
>> Correct - this is a bug in the implementation of thispatch, and
>> apparently the few people that ever used it did not notice it or
>> care enough about it to submit fixes.
>>
>>> And I dare say that half the time or
>>> more that was probably just not noticed (everything comes up with
>>> dynamic host names/dns these days, noticing the IP changed between
>>> U-Boot and Linux is easy to miss in those cases).
>>
>> Agreed.
>>
>>>> CONFIG_NET_RANDOM_ETHADDR is the attempt to do the same
>>>> automatically, except that it falls short of setting the "ethaddr"
>>>> environment variable.  I consider this a bug.
>>>
>>> Since the code isn't that old, it shouldn't be hard to pull up the
>>> thread / patch on introducing it.  So, lets:
>>> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-1-git-sen=
>>> d-email-joe.hershberger@ni.com/
>>> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen=
>>> d-email-joe.hershberger@ni.com/
>>> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen=
>>> d-email-joe.hershberger@ni.com/
>>>
>>> And from there we can take away I think two important things:
>>> 1: Not setting ethaddr with NET_RANDOM_ETHADDR is intentional
>>
>> Ummm... From which part of the patches or the comments do you take
>> this conclusion?  Not a single line of code, or comments in the code
> 
> The same part I keep asking to have fixed in a v2, the help text by the
> Kconfig option:
> +	  A new MAC address will be generated on every boot and it will
> +	  not be added to the environment.
> 
> [snip]
>>> So, yes, OK, this is a bug fix, in that NET_RANDOM_ETHADDR was incorrect
>>> at introduction.  That means we do need a v2 of this patch that updates
>>> the Kconfig help text as that currently says it will not update the
>>> environment.
>>
>> This makes no sense to me.  Instead of documenting the bug we should
>> fix it and add the missing eth_env_set_enetaddr().
>>
>> If I prepare such patches, will you accept these?
> 
> Now I'm confused.  Taking the patch this whole thread is attached to, we
> make NET_RANDOM_ETHADDR perform eth_env_set_enetaddr_by_index which is
> missing (well, only in the DM case, the non-DM case isn't updated and I
> guess should also be?).  My problem is that the Kconfig help text for
> NET_RANDOM_ETHADDR still reflects what I pasted from the initial commit
> of it, a sentence saying we don't update the environment.  This change
> does, so the help is wrong and needs fixing.  I have come around to
> agreeing with the concept of this patch, that NET_RANDOM_ETHADDR should
> cause the environment to be updated and in turn fdt_fixup_ethernet()
> will populate this to Linux.
> 

This thread is getting quite long and I think that would be the best if 
you (both) can create patch based on how you see it should work and send 
it over. Based on it it will be super clear how you think things should 
work.
My initial intention was to show mac addresses via net list (Actually I 
was investigating how to probe all ethernet controllers without calling 
networking command for getting IPs out of reset with power domain 
driver) which is solved by Michael's patch when he send v2 version.

Thanks,
Michal

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4578 bytes --]

On Fri, Nov 19, 2021 at 01:30:06PM +0100, Michal Simek wrote:
> Hi,
> 
> On 11/18/21 20:54, Tom Rini wrote:
> > On Thu, Nov 18, 2021 at 08:04:22PM +0100, Wolfgang Denk wrote:
> > > Dear Tom,
> > > 
> > > In message <20211118162920.GH24579@bill-the-cat> you wrote:
> > > > 
> > > > > It is perfectly OK for U-Boot to start with a random MAC address,
> > > > > use this for a while, and change it so something else later.  This
> > > > > is what may happen at production: say the MAC address is stored in
> > > > > some EEPROM or fuses, which are initially empty, so U-Boot will use
> > > > > a random MAC address, download it's board specific date (serial#,
> > > > > MAC address, ...) over network, programm it into the respective
> > > > > storage devices, and switch to using the new "official" MAC address.
> > > > 
> > > > Yes.  And up until this patch saying I want to use this random MAC with
> > > > Linux required user intervention.
> > > 
> > > Correct - this is a bug in the implementation of thispatch, and
> > > apparently the few people that ever used it did not notice it or
> > > care enough about it to submit fixes.
> > > 
> > > > And I dare say that half the time or
> > > > more that was probably just not noticed (everything comes up with
> > > > dynamic host names/dns these days, noticing the IP changed between
> > > > U-Boot and Linux is easy to miss in those cases).
> > > 
> > > Agreed.
> > > 
> > > > > CONFIG_NET_RANDOM_ETHADDR is the attempt to do the same
> > > > > automatically, except that it falls short of setting the "ethaddr"
> > > > > environment variable.  I consider this a bug.
> > > > 
> > > > Since the code isn't that old, it shouldn't be hard to pull up the
> > > > thread / patch on introducing it.  So, lets:
> > > > https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-1-git-sen=
> > > > d-email-joe.hershberger@ni.com/
> > > > https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen=
> > > > d-email-joe.hershberger@ni.com/
> > > > https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen=
> > > > d-email-joe.hershberger@ni.com/
> > > > 
> > > > And from there we can take away I think two important things:
> > > > 1: Not setting ethaddr with NET_RANDOM_ETHADDR is intentional
> > > 
> > > Ummm... From which part of the patches or the comments do you take
> > > this conclusion?  Not a single line of code, or comments in the code
> > 
> > The same part I keep asking to have fixed in a v2, the help text by the
> > Kconfig option:
> > +	  A new MAC address will be generated on every boot and it will
> > +	  not be added to the environment.
> > 
> > [snip]
> > > > So, yes, OK, this is a bug fix, in that NET_RANDOM_ETHADDR was incorrect
> > > > at introduction.  That means we do need a v2 of this patch that updates
> > > > the Kconfig help text as that currently says it will not update the
> > > > environment.
> > > 
> > > This makes no sense to me.  Instead of documenting the bug we should
> > > fix it and add the missing eth_env_set_enetaddr().
> > > 
> > > If I prepare such patches, will you accept these?
> > 
> > Now I'm confused.  Taking the patch this whole thread is attached to, we
> > make NET_RANDOM_ETHADDR perform eth_env_set_enetaddr_by_index which is
> > missing (well, only in the DM case, the non-DM case isn't updated and I
> > guess should also be?).  My problem is that the Kconfig help text for
> > NET_RANDOM_ETHADDR still reflects what I pasted from the initial commit
> > of it, a sentence saying we don't update the environment.  This change
> > does, so the help is wrong and needs fixing.  I have come around to
> > agreeing with the concept of this patch, that NET_RANDOM_ETHADDR should
> > cause the environment to be updated and in turn fdt_fixup_ethernet()
> > will populate this to Linux.
> > 
> 
> This thread is getting quite long and I think that would be the best if you
> (both) can create patch based on how you see it should work and send it
> over. Based on it it will be super clear how you think things should work.
> My initial intention was to show mac addresses via net list (Actually I was
> investigating how to probe all ethernet controllers without calling
> networking command for getting IPs out of reset with power domain driver)
> which is solved by Michael's patch when he send v2 version.

Reasonable request, done:
https://patchwork.ozlabs.org/project/uboot/patch/20211120155358.376540-1-trini@konsulko.com/

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]