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

[Tom Rini]

On Wed, Nov 17, 2021 at 04:56:27PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211117123548.GX24579 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20211117/38a3b645/attachment.sig>