On Thu, Nov 18, 2021 at 08:08:14AM +0100, Wolfgang Denk wrote:
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.
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... https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-send... https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-send...
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.