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

[Tom Rini <trini@konsulko.com>]

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