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

From: Wolfgang Denk <wd@denx.de>
To: Tom Rini <trini@konsulko.com>
Cc: Ramon Fried <rfried.dev@gmail.com>,
	Michael Walle <michael@walle.cc>,
	Michal Simek <michal.simek@xilinx.com>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	git <git@xilinx.com>, Joe Hershberger <joe.hershberger@ni.com>,
	Simon Glass <sjg@chromium.org>,
	U-Boot Mailing List <u-boot@lists.denx.de>
Subject: Re: [PATCH] net: uclass: Save ethernet MAC address when generated
Date: Thu, 18 Nov 2021 08:08:14 +0100	[thread overview]
Message-ID: <1889944.1637219294@gemini.denx.de> (raw)
In-Reply-To: <20211117161545.GA24579@bill-the-cat>

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