All of lore.kernel.org
 help / color / mirror / Atom feed
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

  reply	other threads:[~2021-11-18  7:08 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-29 11:14 [PATCH] net: uclass: Save ethernet MAC address when generated Michal Simek
2021-11-01 20:25 ` Ramon Fried
2021-11-02  9:00   ` Michael Walle
2021-11-02 10:27     ` Michal Simek
2021-11-03 16:57       ` Tom Rini
2021-11-04 11:18         ` Michal Simek
2021-11-04 11:37           ` Tom Rini
2021-11-04 11:43             ` Michal Simek
2021-11-04 12:59               ` Tom Rini
2021-11-04 13:06                 ` Michal Simek
2021-11-04  2:09       ` Grygorii Strashko
2021-11-04 11:16         ` Michal Simek
2021-11-04 12:27           ` Michael Walle
2021-11-04 13:15             ` Michal Simek
2021-11-04 13:40               ` Michael Walle
2021-11-04 21:00                 ` Ramon Fried
2021-11-09 13:55                   ` Michael Walle
2021-11-11  9:10                     ` Michael Walle
2021-11-16 14:18                       ` Tom Rini
2021-11-16 14:56                         ` Wolfgang Denk
2021-11-16 18:41                           ` Tom Rini
2021-11-17  7:44                             ` Wolfgang Denk
2021-11-17 11:50                               ` Tom Rini
2021-11-17 12:24                                 ` Wolfgang Denk
2021-11-17 12:35                                   ` Tom Rini
2021-11-17 15:56                                     ` Wolfgang Denk
2021-11-17 16:15                                       ` Tom Rini
2021-11-18  7:08                                         ` Wolfgang Denk [this message]
2021-11-18  9:46                                           ` Wolfgang Denk
2021-11-18 14:51                                             ` Tom Rini
2021-11-18 16:29                                           ` Tom Rini
2021-11-18 19:04                                             ` Wolfgang Denk
2021-11-18 19:54                                               ` Tom Rini
2021-11-19 12:30                                                 ` Michal Simek
2021-11-20 15:56                                                   ` Tom Rini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1889944.1637219294@gemini.denx.de \
    --to=wd@denx.de \
    --cc=git@xilinx.com \
    --cc=grygorii.strashko@ti.com \
    --cc=joe.hershberger@ni.com \
    --cc=michael@walle.cc \
    --cc=michal.simek@xilinx.com \
    --cc=rfried.dev@gmail.com \
    --cc=sjg@chromium.org \
    --cc=trini@konsulko.com \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.