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

[Tom Rini]

On Thu, Nov 18, 2021 at 08:08:14AM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20211117161545.GA24579 at 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
-------------- 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/20211118/2b23c2fe/attachment.sig>