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

[Tom Rini <trini@konsulko.com>]

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