[PATCH] net: uclass: Save ethernet MAC address when generated
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr); + eth_env_set_enetaddr_by_index("eth", dev_seq(dev), + pdata->enetaddr); #else printf("\nError: %s address not set.\n", dev->name);
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr);
eth_env_set_enetaddr_by_index("eth", dev_seq(dev),pdata->enetaddr);#else printf("\nError: %s address not set.\n", dev->name); -- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr);
eth_env_set_enetaddr_by_index("eth", dev_seq(dev),pdata->enetaddr);#else printf("\nError: %s address not set.\n", dev->name); -- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
-michael
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr);
eth_env_set_enetaddr_by_index("eth", dev_seq(dev),pdata->enetaddr);-- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.
Thanks, Michal
On Tue, Nov 02, 2021 at 11:27:22AM +0100, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr);
eth_env_set_enetaddr_by_index("eth", dev_seq(dev),pdata->enetaddr);-- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.
So, this causes dhcp to simply timeout for me on a pine64_plus board (where yes, I believe we do end up using a random MAC) when running the pytest suite. I wonder if this particular change messes up the state machine? Checking the dnsmasq logs it looks like U-Boot keeps asking and getting a reply, over and over.
On 11/3/21 17:57, Tom Rini wrote:
On Tue, Nov 02, 2021 at 11:27:22AM +0100, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr);
eth_env_set_enetaddr_by_index("eth", dev_seq(dev),pdata->enetaddr);-- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.
So, this causes dhcp to simply timeout for me on a pine64_plus board (where yes, I believe we do end up using a random MAC) when running the pytest suite. I wonder if this particular change messes up the state machine? Checking the dnsmasq logs it looks like U-Boot keeps asking and getting a reply, over and over.
It is just saving that values to variables. If you want to propagete that values in the stack that's up to your system configuration.
What state machine are you talking about?
Thanks, Michal
On Thu, Nov 04, 2021 at 12:18:46PM +0100, Michal Simek wrote:
On 11/3/21 17:57, Tom Rini wrote:
On Tue, Nov 02, 2021 at 11:27:22AM +0100, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr);
eth_env_set_enetaddr_by_index("eth", dev_seq(dev),pdata->enetaddr);-- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.
So, this causes dhcp to simply timeout for me on a pine64_plus board (where yes, I believe we do end up using a random MAC) when running the pytest suite. I wonder if this particular change messes up the state machine? Checking the dnsmasq logs it looks like U-Boot keeps asking and getting a reply, over and over.
It is just saving that values to variables. If you want to propagete that values in the stack that's up to your system configuration.
What state machine are you talking about?
I was just making a guess about state machine, given what I had recalled of the network stack before. To be clearer, I bisected down my failure of 3 tests that run "setenv autoload no ; dhcp" and then dhcp timing out, to this commit (in the network PR).
On 11/4/21 12:37, Tom Rini wrote:
On Thu, Nov 04, 2021 at 12:18:46PM +0100, Michal Simek wrote:
On 11/3/21 17:57, Tom Rini wrote:
On Tue, Nov 02, 2021 at 11:27:22AM +0100, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote: > > When MAC address is randomly generated it should be also saved to > variables. This step is there when MAC address is passed via pdata but not > when it is randomly generated. > > Signed-off-by: Michal Simek michal.simek@xilinx.com > --- > > net/eth-uclass.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/eth-uclass.c b/net/eth-uclass.c > index 0da0e85be031..58c308f33276 100644 > --- a/net/eth-uclass.c > +++ b/net/eth-uclass.c > @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) > net_random_ethaddr(pdata->enetaddr); > printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", > dev->name, dev_seq(dev), pdata->enetaddr); > + eth_env_set_enetaddr_by_index("eth", dev_seq(dev), > + pdata->enetaddr); > #else > printf("\nError: %s address not set.\n", > dev->name); > -- > 2.33.1 > Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.
So, this causes dhcp to simply timeout for me on a pine64_plus board (where yes, I believe we do end up using a random MAC) when running the pytest suite. I wonder if this particular change messes up the state machine? Checking the dnsmasq logs it looks like U-Boot keeps asking and getting a reply, over and over.
It is just saving that values to variables. If you want to propagete that values in the stack that's up to your system configuration.
What state machine are you talking about?
I was just making a guess about state machine, given what I had recalled of the network stack before. To be clearer, I bisected down my failure of 3 tests that run "setenv autoload no ; dhcp" and then dhcp timing out, to this commit (in the network PR).
First of all I didn't know that it was the part of networking pull request because didn't see apply message only reviewed from Ramon.
I would be very surprised this patch to caused your issue. It should be covering something else but that should be debugged.
Thanks, Michal
On Thu, Nov 04, 2021 at 12:43:11PM +0100, Michal Simek wrote:
On 11/4/21 12:37, Tom Rini wrote:
On Thu, Nov 04, 2021 at 12:18:46PM +0100, Michal Simek wrote:
On 11/3/21 17:57, Tom Rini wrote:
On Tue, Nov 02, 2021 at 11:27:22AM +0100, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
> On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote: > > > > When MAC address is randomly generated it should be also saved to > > variables. This step is there when MAC address is passed via pdata but not > > when it is randomly generated. > > > > Signed-off-by: Michal Simek michal.simek@xilinx.com > > --- > > > > net/eth-uclass.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/net/eth-uclass.c b/net/eth-uclass.c > > index 0da0e85be031..58c308f33276 100644 > > --- a/net/eth-uclass.c > > +++ b/net/eth-uclass.c > > @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) > > net_random_ethaddr(pdata->enetaddr); > > printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", > > dev->name, dev_seq(dev), pdata->enetaddr); > > + eth_env_set_enetaddr_by_index("eth", dev_seq(dev), > > + pdata->enetaddr); > > #else > > printf("\nError: %s address not set.\n", > > dev->name); > > -- > > 2.33.1 > > > Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.
So, this causes dhcp to simply timeout for me on a pine64_plus board (where yes, I believe we do end up using a random MAC) when running the pytest suite. I wonder if this particular change messes up the state machine? Checking the dnsmasq logs it looks like U-Boot keeps asking and getting a reply, over and over.
It is just saving that values to variables. If you want to propagete that values in the stack that's up to your system configuration.
What state machine are you talking about?
I was just making a guess about state machine, given what I had recalled of the network stack before. To be clearer, I bisected down my failure of 3 tests that run "setenv autoload no ; dhcp" and then dhcp timing out, to this commit (in the network PR).
First of all I didn't know that it was the part of networking pull request because didn't see apply message only reviewed from Ramon.
I would be very surprised this patch to caused your issue. It should be covering something else but that should be debugged.
OK, I don't know why my git bisect run lead to this commit, but I've manually bisected instead this time and found a much more reasonable problematic commit. Sorry for the noise.
On 11/4/21 13:59, Tom Rini wrote:
On Thu, Nov 04, 2021 at 12:43:11PM +0100, Michal Simek wrote:
On 11/4/21 12:37, Tom Rini wrote:
On Thu, Nov 04, 2021 at 12:18:46PM +0100, Michal Simek wrote:
On 11/3/21 17:57, Tom Rini wrote:
On Tue, Nov 02, 2021 at 11:27:22AM +0100, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote: >> On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote: >>> >>> When MAC address is randomly generated it should be also saved to >>> variables. This step is there when MAC address is passed via pdata but not >>> when it is randomly generated. >>> >>> Signed-off-by: Michal Simek michal.simek@xilinx.com >>> --- >>> >>> net/eth-uclass.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c >>> index 0da0e85be031..58c308f33276 100644 >>> --- a/net/eth-uclass.c >>> +++ b/net/eth-uclass.c >>> @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) >>> net_random_ethaddr(pdata->enetaddr); >>> printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", >>> dev->name, dev_seq(dev), pdata->enetaddr); >>> + eth_env_set_enetaddr_by_index("eth", dev_seq(dev), >>> + pdata->enetaddr); >>> #else >>> printf("\nError: %s address not set.\n", >>> dev->name); >>> -- >>> 2.33.1 >>> >> Reviewed-by: Ramon Fried rfried.dev@gmail.com > > Please note, that this will change behavior. Before this commit, the > random mac address was local to u-boot (at least for most network drivers). > After this commit, it will also be communicated to linux. > > I'm not sure what to think of this. At the very least, this should be > documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.
So, this causes dhcp to simply timeout for me on a pine64_plus board (where yes, I believe we do end up using a random MAC) when running the pytest suite. I wonder if this particular change messes up the state machine? Checking the dnsmasq logs it looks like U-Boot keeps asking and getting a reply, over and over.
It is just saving that values to variables. If you want to propagete that values in the stack that's up to your system configuration.
What state machine are you talking about?
I was just making a guess about state machine, given what I had recalled of the network stack before. To be clearer, I bisected down my failure of 3 tests that run "setenv autoload no ; dhcp" and then dhcp timing out, to this commit (in the network PR).
First of all I didn't know that it was the part of networking pull request because didn't see apply message only reviewed from Ramon.
I would be very surprised this patch to caused your issue. It should be covering something else but that should be debugged.
OK, I don't know why my git bisect run lead to this commit, but I've manually bisected instead this time and found a much more reasonable problematic commit. Sorry for the noise.
Not big deal.
M
On 02/11/2021 12:27, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr); + eth_env_set_enetaddr_by_index("eth", dev_seq(dev), + pdata->enetaddr); #else printf("\nError: %s address not set.\n", dev->name); -- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
Propagating random MAC to Linux might be not a good idea as Linux will silently use it while in many cases it means that smth is wrong, and message like "Driver uses random MAC!" helps narrow down issues earlier. For example, we occasionally copy-pasted wrong mac in DT and detected this only after some time when started connecting similar boards to each other :( Also, Linux may have it's own way to retrieve MAC if not provided by u-boot.
Wouldn't be enough to just print MAC console when random is used?
I found this via net list command where you can see controllers but you can't see their mac addresses which is IMHO wrong.
On 11/4/21 03:09, Grygorii Strashko wrote:
On 02/11/2021 12:27, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr); + eth_env_set_enetaddr_by_index("eth", dev_seq(dev), + pdata->enetaddr); #else printf("\nError: %s address not set.\n", dev->name); -- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
Propagating random MAC to Linux might be not a good idea as Linux will silently use it while in many cases it means that smth is wrong, and message like "Driver uses random MAC!" helps narrow down issues earlier.
Not sure if you have ever tried this feature.
Mac address is shown on the log. Warning: ethernet@ff0c0000 (eth1) using random MAC address - 66:dc:38:d1:24:a9
And if you don't want to use this feature just don't enable it via CONFIG_NET_RANDOM_ETHADDR.
For example, we occasionally copy-pasted wrong mac in DT and detected this only after some time when started connecting similar boards to each other :(
That's not issue with this patch.
Also, Linux may have it's own way to retrieve MAC if not provided by u-boot.
Sure and I am not blocking it. I am just saying that u-boot is generating random mac address which is not exported to variables which is what net list command expects. If fdt_fixup_ethernet() is called to update pass it to next phase is different story. Also in our case when u-boot doesn't record mac to DT but mac remains in IP itself.
Wouldn't be enough to just print MAC console when random is used?
As I said above it is already there but IMHO not enough. Take a look at behavior with this patch and without.
ZYNQ GEM: ff0b0000, mdio bus ff0c0000, phyaddr 4, interface sgmii Loading new PMUFW cfg obj (32 bytes)
Warning: ethernet@ff0b0000 (eth0) using random MAC address - 7a:77:b5:39:e8:12
ZYNQ GEM: ff0c0000, mdio bus ff0c0000, phyaddr 8, interface rgmii-id Loading new PMUFW cfg obj (32 bytes)
Warning: ethernet@ff0c0000 (eth1) using random MAC address - 32:77:95:89:df:0b ethernet@ff0c0000 Waiting for PHY auto negotiation to complete....... done BOOTP broadcast 1 BOOTP broadcast 2 BOOTP broadcast 3 BOOTP broadcast 4 DHCP client bound to address 192.168.0.143 (3260 ms) gpio: pin resetchip5 (gpio 179) value is 0 gpio: pin resetchip5 (gpio 179) value is 1 ethernet@ff0b0000 Waiting for PHY auto negotiation to complete........ done BOOTP broadcast 1 BOOTP broadcast 2 BOOTP broadcast 3 BOOTP broadcast 4 DHCP client bound to address 192.168.0.163 (3513 ms) Hit any key to stop autoboot: 0 ZynqMP> net list eth0 : ethernet@ff0b0000 7a:77:b5:39:e8:12 active eth1 : ethernet@ff0c0000 32:77:95:89:df:0b ZynqMP>
compare to ZynqMP> net list eth0 : ethernet@ff0b0000 00:00:00:00:00:00 active eth1 : ethernet@ff0c0000 00:00:00:00:00:00
Thanks, Michal
Am 2021-11-04 12:16, schrieb Michal Simek:
On 11/4/21 03:09, Grygorii Strashko wrote:
On 02/11/2021 12:27, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote:
When MAC address is randomly generated it should be also saved to variables. This step is there when MAC address is passed via pdata but not when it is randomly generated.
Signed-off-by: Michal Simek michal.simek@xilinx.com
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 0da0e85be031..58c308f33276 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) net_random_ethaddr(pdata->enetaddr); printf("\nWarning: %s (eth%d) using random MAC address - %pM\n", dev->name, dev_seq(dev), pdata->enetaddr); + eth_env_set_enetaddr_by_index("eth", dev_seq(dev), + pdata->enetaddr); #else printf("\nError: %s address not set.\n", dev->name); -- 2.33.1
Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
Propagating random MAC to Linux might be not a good idea as Linux will silently use it while in many cases it means that smth is wrong, and message like "Driver uses random MAC!" helps narrow down issues earlier.
Not sure if you have ever tried this feature.
Mac address is shown on the log. Warning: ethernet@ff0c0000 (eth1) using random MAC address - 66:dc:38:d1:24:a9
And if you don't want to use this feature just don't enable it via CONFIG_NET_RANDOM_ETHADDR.
I usually, enable this feature for my boards, because it helps you getting network connectivity in u-boot in worst-case scenarios.
Also, Linux may have it's own way to retrieve MAC if not provided by u-boot.
Sure and I am not blocking it. I am just saying that u-boot is generating random mac address which is not exported to variables which is what net list command expects.
maybe then the net list command should be fixed.
If fdt_fixup_ethernet() is called to update pass it to next phase is different story.
AFAIK the u-boot environment variable has always been the source for the DT fixup. So I doubt we can change that.
Also in our case when u-boot doesn't record mac to DT but mac remains in IP itself.
Have a look at https://elixir.bootlin.com/linux/v5.15/source/drivers/of/of_net.c#L124
With this patch u-boot will now always fill the mac-address property in the device tree and it will never be fetched from the nvmem storage. When you have that Kconfig for random ethernet address set of course.
Thinking about this, I guess this will break the sl28 board, which has CONFIG_NET_RANDOM_ETHADDR set and will normally rely on the fact that the device tree isn't fixed up, because then linux will take it from the OTP memory in flash.
-michael
On 11/4/21 13:27, Michael Walle wrote:
Am 2021-11-04 12:16, schrieb Michal Simek:
On 11/4/21 03:09, Grygorii Strashko wrote:
On 02/11/2021 12:27, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
On Fri, Oct 29, 2021 at 2:14 PM Michal Simek michal.simek@xilinx.com wrote: > > When MAC address is randomly generated it should be also saved to > variables. This step is there when MAC address is passed via > pdata but not > when it is randomly generated. > > Signed-off-by: Michal Simek michal.simek@xilinx.com > --- > > net/eth-uclass.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/eth-uclass.c b/net/eth-uclass.c > index 0da0e85be031..58c308f33276 100644 > --- a/net/eth-uclass.c > +++ b/net/eth-uclass.c > @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice *dev) > net_random_ethaddr(pdata->enetaddr); > printf("\nWarning: %s (eth%d) using random MAC > address - %pM\n", > dev->name, dev_seq(dev), pdata->enetaddr); > + eth_env_set_enetaddr_by_index("eth", dev_seq(dev), > + pdata->enetaddr); > #else > printf("\nError: %s address not set.\n", > dev->name); > -- 2.33.1 > Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
Propagating random MAC to Linux might be not a good idea as Linux will silently use it while in many cases it means that smth is wrong, and message like "Driver uses random MAC!" helps narrow down issues earlier.
Not sure if you have ever tried this feature.
Mac address is shown on the log. Warning: ethernet@ff0c0000 (eth1) using random MAC address - 66:dc:38:d1:24:a9
And if you don't want to use this feature just don't enable it via CONFIG_NET_RANDOM_ETHADDR.
I usually, enable this feature for my boards, because it helps you getting network connectivity in u-boot in worst-case scenarios.
And that make sense. But then your bootloader should use different mac then your OS and with dhcp server you get two different IP addresses.
Also, Linux may have it's own way to retrieve MAC if not provided by u-boot.
Sure and I am not blocking it. I am just saying that u-boot is generating random mac address which is not exported to variables which is what net list command expects.
maybe then the net list command should be fixed.
If you have any proposal how to do it, I am listening.
If fdt_fixup_ethernet() is called to update pass it to next phase is different story.
AFAIK the u-boot environment variable has always been the source for the DT fixup. So I doubt we can change that.
Not only this. Your DT has to also has ethernet aliases which AFAIK is not used by Linux. It means for us this code does nothing because we are not using it.
Also in our case when u-boot doesn't record mac to DT but mac remains in IP itself.
Have a look at https://elixir.bootlin.com/linux/v5.15/source/drivers/of/of_net.c#L124
With this patch u-boot will now always fill the mac-address property in the device tree and it will never be fetched from the nvmem storage. When you have that Kconfig for random ethernet address set of course.
if you save it likely yes. If not on next reboot you get another random mac.
Thinking about this, I guess this will break the sl28 board, which has CONFIG_NET_RANDOM_ETHADDR set and will normally rely on the fact that the device tree isn't fixed up, because then linux will take it from the OTP memory in flash.
Does that mean that you let bootloader work with random address but OS will have access to memory which u-boot does not have?
And if you want to load mac address from different source it should be handled somehow. I don't think there is any dt description for it but you can use ifconfig -hw ether to fix it in userspace anyway.
Thanks, Michal
Am 2021-11-04 14:15, schrieb Michal Simek:
On 11/4/21 13:27, Michael Walle wrote:
Am 2021-11-04 12:16, schrieb Michal Simek:
On 11/4/21 03:09, Grygorii Strashko wrote:
On 02/11/2021 12:27, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote:
> On Fri, Oct 29, 2021 at 2:14 PM Michal Simek > michal.simek@xilinx.com wrote: >> >> When MAC address is randomly generated it should be also saved >> to >> variables. This step is there when MAC address is passed via >> pdata but not >> when it is randomly generated. >> >> Signed-off-by: Michal Simek michal.simek@xilinx.com >> --- >> >> net/eth-uclass.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/eth-uclass.c b/net/eth-uclass.c >> index 0da0e85be031..58c308f33276 100644 >> --- a/net/eth-uclass.c >> +++ b/net/eth-uclass.c >> @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice >> *dev) >> net_random_ethaddr(pdata->enetaddr); >> printf("\nWarning: %s (eth%d) using random MAC >> address - %pM\n", >> dev->name, dev_seq(dev), >> pdata->enetaddr); >> + eth_env_set_enetaddr_by_index("eth", >> dev_seq(dev), >> + pdata->enetaddr); >> #else >> printf("\nError: %s address not set.\n", >> dev->name); >> -- 2.33.1 >> > Reviewed-by: Ramon Fried rfried.dev@gmail.com
Please note, that this will change behavior. Before this commit, the random mac address was local to u-boot (at least for most network drivers). After this commit, it will also be communicated to linux.
I'm not sure what to think of this. At the very least, this should be documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
Propagating random MAC to Linux might be not a good idea as Linux will silently use it while in many cases it means that smth is wrong, and message like "Driver uses random MAC!" helps narrow down issues earlier.
Not sure if you have ever tried this feature.
Mac address is shown on the log. Warning: ethernet@ff0c0000 (eth1) using random MAC address - 66:dc:38:d1:24:a9
And if you don't want to use this feature just don't enable it via CONFIG_NET_RANDOM_ETHADDR.
I usually, enable this feature for my boards, because it helps you getting network connectivity in u-boot in worst-case scenarios.
And that make sense. But then your bootloader should use different mac then your OS and with dhcp server you get two different IP addresses.
Sure, but thats only for recovery/production purposes. You'll get a different mac address for every reset.
Also, Linux may have it's own way to retrieve MAC if not provided by u-boot.
Sure and I am not blocking it. I am just saying that u-boot is generating random mac address which is not exported to variables which is what net list command expects.
maybe then the net list command should be fixed.
If you have any proposal how to do it, I am listening.
Can't "net list" just fetch the address from the device?
If fdt_fixup_ethernet() is called to update pass it to next phase is different story.
AFAIK the u-boot environment variable has always been the source for the DT fixup. So I doubt we can change that.
Not only this. Your DT has to also has ethernet aliases which AFAIK is not used by Linux. It means for us this code does nothing because we are not using it.
Ah, right. At least this will save the sl28 board because there's no mac-address property which could be changed. But my point remains, that it changes behavior. (FWIW I'm still undecided if this is good or bad).
Also in our case when u-boot doesn't record mac to DT but mac remains in IP itself.
Have a look at https://elixir.bootlin.com/linux/v5.15/source/drivers/of/of_net.c#L124
With this patch u-boot will now always fill the mac-address property in the device tree and it will never be fetched from the nvmem storage. When you have that Kconfig for random ethernet address set of course.
if you save it likely yes. If not on next reboot you get another random mac.
No I mean, if your DT has a mac-address property which is fixed up by the bootloader. Before this patch, if there is a random ethernet address the code will try to read it from nvmem. After this patch, the MAC address will always come from the bootloader.
Thinking about this, I guess this will break the sl28 board, which has CONFIG_NET_RANDOM_ETHADDR set and will normally rely on the fact that the device tree isn't fixed up, because then linux will take it from the OTP memory in flash.
Does that mean that you let bootloader work with random address but OS will have access to memory which u-boot does not have?
correct. Usually, we don't need network in the bootloader. Most of the time this is only for debugging. Or if its needed, someone needs to set it by hand because there is no SPI-NOR OTP support in u-boot for now.
And if you want to load mac address from different source it should be handled somehow. I don't think there is any dt description for it but you can use ifconfig -hw ether to fix it in userspace anyway.
I can't follow. The device tree will supply the nvmem provider. Again, as I don't have the mac-address property, I think the sl28 board is fine.
On Thu, Nov 4, 2021 at 3:40 PM Michael Walle michael@walle.cc wrote:
Am 2021-11-04 14:15, schrieb Michal Simek:
On 11/4/21 13:27, Michael Walle wrote:
Am 2021-11-04 12:16, schrieb Michal Simek:
On 11/4/21 03:09, Grygorii Strashko wrote:
On 02/11/2021 12:27, Michal Simek wrote:
On 11/2/21 10:00, Michael Walle wrote: >> On Fri, Oct 29, 2021 at 2:14 PM Michal Simek >> michal.simek@xilinx.com wrote: >>> >>> When MAC address is randomly generated it should be also saved >>> to >>> variables. This step is there when MAC address is passed via >>> pdata but not >>> when it is randomly generated. >>> >>> Signed-off-by: Michal Simek michal.simek@xilinx.com >>> --- >>> >>> net/eth-uclass.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c >>> index 0da0e85be031..58c308f33276 100644 >>> --- a/net/eth-uclass.c >>> +++ b/net/eth-uclass.c >>> @@ -583,6 +583,8 @@ static int eth_post_probe(struct udevice >>> *dev) >>> net_random_ethaddr(pdata->enetaddr); >>> printf("\nWarning: %s (eth%d) using random MAC >>> address - %pM\n", >>> dev->name, dev_seq(dev), >>> pdata->enetaddr); >>> + eth_env_set_enetaddr_by_index("eth", >>> dev_seq(dev), >>> + pdata->enetaddr); >>> #else >>> printf("\nError: %s address not set.\n", >>> dev->name); >>> -- 2.33.1 >>> >> Reviewed-by: Ramon Fried rfried.dev@gmail.com > > Please note, that this will change behavior. Before this commit, > the > random mac address was local to u-boot (at least for most network > drivers). > After this commit, it will also be communicated to linux. > > I'm not sure what to think of this. At the very least, this should > be > documented in the commit message and in the Kconfig help text.
Thanks for bringing this up. I have no issue that this address is being propagated to Linux but others can feel this as an issue. I can definitely extend commit message to say it.
Propagating random MAC to Linux might be not a good idea as Linux will silently use it while in many cases it means that smth is wrong, and message like "Driver uses random MAC!" helps narrow down issues earlier.
Not sure if you have ever tried this feature.
Mac address is shown on the log. Warning: ethernet@ff0c0000 (eth1) using random MAC address - 66:dc:38:d1:24:a9
And if you don't want to use this feature just don't enable it via CONFIG_NET_RANDOM_ETHADDR.
I usually, enable this feature for my boards, because it helps you getting network connectivity in u-boot in worst-case scenarios.
And that make sense. But then your bootloader should use different mac then your OS and with dhcp server you get two different IP addresses.
Sure, but thats only for recovery/production purposes. You'll get a different mac address for every reset.
Also, Linux may have it's own way to retrieve MAC if not provided by u-boot.
Sure and I am not blocking it. I am just saying that u-boot is generating random mac address which is not exported to variables which is what net list command expects.
maybe then the net list command should be fixed.
If you have any proposal how to do it, I am listening.
Can't "net list" just fetch the address from the device?
If fdt_fixup_ethernet() is called to update pass it to next phase is different story.
AFAIK the u-boot environment variable has always been the source for the DT fixup. So I doubt we can change that.
Not only this. Your DT has to also has ethernet aliases which AFAIK is not used by Linux. It means for us this code does nothing because we are not using it.
Ah, right. At least this will save the sl28 board because there's no mac-address property which could be changed. But my point remains, that it changes behavior. (FWIW I'm still undecided if this is good or bad).
Also in our case when u-boot doesn't record mac to DT but mac remains in IP itself.
Have a look at https://elixir.bootlin.com/linux/v5.15/source/drivers/of/of_net.c#L124
With this patch u-boot will now always fill the mac-address property in the device tree and it will never be fetched from the nvmem storage. When you have that Kconfig for random ethernet address set of course.
if you save it likely yes. If not on next reboot you get another random mac.
No I mean, if your DT has a mac-address property which is fixed up by the bootloader. Before this patch, if there is a random ethernet address the code will try to read it from nvmem. After this patch, the MAC address will always come from the bootloader.
Thinking about this, I guess this will break the sl28 board, which has CONFIG_NET_RANDOM_ETHADDR set and will normally rely on the fact that the device tree isn't fixed up, because then linux will take it from the OTP memory in flash.
Does that mean that you let bootloader work with random address but OS will have access to memory which u-boot does not have?
correct. Usually, we don't need network in the bootloader. Most of the time this is only for debugging. Or if its needed, someone needs to set it by hand because there is no SPI-NOR OTP support in u-boot for now.
And if you want to load mac address from different source it should be handled somehow. I don't think there is any dt description for it but you can use ifconfig -hw ether to fix it in userspace anyway.
I can't follow. The device tree will supply the nvmem provider. Again, as I don't have the mac-address property, I think the sl28 board is fine.
-- -michael
What's the verdict ? We're keeping this patch ?
Am 2021-11-04 22:00, schrieb Ramon Fried:
What's the verdict ? We're keeping this patch ?
There was a good point from Grygorii: linux might deal in its own way with missing ethernet addresses (and warn the user about it because its usually an error).
And IMHO that net list can be fixed in a different way.
If this patch is accepted, it should be clearly documented in the Kconfig help text. And as I said, it changes behavior.
-michael
Hi Ramon,
Am 2021-11-09 14:55, schrieb Michael Walle:
Am 2021-11-04 22:00, schrieb Ramon Fried:
What's the verdict ? We're keeping this patch ?
There was a good point from Grygorii: linux might deal in its own way with missing ethernet addresses (and warn the user about it because its usually an error).
And IMHO that net list can be fixed in a different way.
If this patch is accepted, it should be clearly documented in the Kconfig help text. And as I said, it changes behavior.
So this patch now in your pull request. Why are you even asking then?
-michael
On Thu, Nov 11, 2021 at 10:10:57AM +0100, Michael Walle wrote:
Hi Ramon,
Am 2021-11-09 14:55, schrieb Michael Walle:
Am 2021-11-04 22:00, schrieb Ramon Fried:
What's the verdict ? We're keeping this patch ?
There was a good point from Grygorii: linux might deal in its own way with missing ethernet addresses (and warn the user about it because its usually an error).
And IMHO that net list can be fixed in a different way.
If this patch is accepted, it should be clearly documented in the Kconfig help text. And as I said, it changes behavior.
So this patch now in your pull request. Why are you even asking then?
So, to quote lib/Kconfig: config NET_RANDOM_ETHADDR bool "Random ethaddr if unset" help Selecting this will allow the Ethernet interface to function even when the ethaddr variable for that interface is unset. A new MAC address will be generated on every boot and it will not be added to the environment.
We need either a re-spin or follow-up as we're changing the documented behavior. And as I mentioned in the other thread related on-going thread, perhaps "ethmacskip" should play a role in preserving existing behavior?
Dear Tom,
In message 20211116141855.GD24579@bill-the-cat you wrote:
So, to quote lib/Kconfig: config NET_RANDOM_ETHADDR bool "Random ethaddr if unset" help Selecting this will allow the Ethernet interface to function even when the ethaddr variable for that interface is unset. A new MAC address will be generated on every boot and it will not be added to the environment.
This description is at least incomplete, because it makes no difference between the persistent copy of the environment and it's in-memory copy. For network to function, I think the MAC address must be stored in the in-memory copy of the environment.
We need either a re-spin or follow-up as we're changing the documented behavior. And as I mentioned in the other thread related on-going thread, perhaps "ethmacskip" should play a role in preserving existing behavior?
We have way too many ways to do the same thing - nearly, just a little different :-(
Best regards,
Wolfgang Denk
On Tue, Nov 16, 2021 at 03:56:45PM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 20211116141855.GD24579@bill-the-cat you wrote:
So, to quote lib/Kconfig: config NET_RANDOM_ETHADDR bool "Random ethaddr if unset" help Selecting this will allow the Ethernet interface to function even when the ethaddr variable for that interface is unset. A new MAC address will be generated on every boot and it will not be added to the environment.
This description is at least incomplete, because it makes no difference between the persistent copy of the environment and it's in-memory copy. For network to function, I think the MAC address must be stored in the in-memory copy of the environment.
Well, networking has been working with NET_RANDOM_ETHADDR not updating the environment, with caveats that user display information that reads from the environment rather than ->enetaddr. In so far as the networking stack cares, ->enetaddr is what matters. We look at the environment for a MAC, and if it doesn't match the HW we use environment MAC, but warn.
Now, I think RFC7042 is the current correct and relevant RFC here and it doesn't mention persistence. So I don't see a clear external authority on if locally administered MAC addresses like this should be persistent or not in this manner.
We need either a re-spin or follow-up as we're changing the documented behavior. And as I mentioned in the other thread related on-going thread, perhaps "ethmacskip" should play a role in preserving existing behavior?
We have way too many ways to do the same thing - nearly, just a little different :-(
Well, in this case I'm not sure that's the right problem to point at. We can just set ethmacskip as a bit of corner-case functionality and move on.
Because honestly, the more I read this, the more I think https://patchwork.ozlabs.org/project/uboot/patch/20211115121152.3470910-1-mi... is essentially the right direction. There's no reason for 'net list' to be using the environment here when ->enetaddr is what's being used by the stack. The use case of "I want to make my locally administered MAC persist because my USB ethernet adapter lacks a MAC address" is solved via the environment already.
Dear Tom,
In message 20211116184146.GF24579@bill-the-cat you wrote:
Because honestly, the more I read this, the more I think https://patchwork.ozlabs.org/project/uboot/patch/20211115121152.3470910-1-m= ichael@walle.cc/ is essentially the right direction. There's no reason for 'net list' to be using the environment here when ->enetaddr is what's being used by the stack. The use case of "I want to make my locally administered MAC persist because my USB ethernet adapter lacks a MAC address" is solved via the environment already.
If the MAC address is not placed in the environment, then how can a user query the currently used MAC address? All documentation says basically: run "printenv ethaddr".
Best regards,
Wolfgang Denk
On Wed, Nov 17, 2021 at 08:44:52AM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 20211116184146.GF24579@bill-the-cat you wrote:
Because honestly, the more I read this, the more I think https://patchwork.ozlabs.org/project/uboot/patch/20211115121152.3470910-1-m= ichael@walle.cc/ is essentially the right direction. There's no reason for 'net list' to be using the environment here when ->enetaddr is what's being used by the stack. The use case of "I want to make my locally administered MAC persist because my USB ethernet adapter lacks a MAC address" is solved via the environment already.
If the MAC address is not placed in the environment, then how can a user query the currently used MAC address? All documentation says basically: run "printenv ethaddr".
Update the documentation and say to use "net list" or read the previous part of console log that says we're using a random mac in this case? The more I look here the more I see we're changing part of the API with the OS, and it's not something that should be done without consulting the consumers too.
I should note first that I got how the "USB ethernet that lacks HW assigned MAC" case has worked since forever (the beagleboard xM is the first case that springs to mind and that's over 10 years old). That's "run gen_eth_addr, setenv ethaddr $X, saveenv" as yes, our randomly generated MAC within U-Boot has never been super easy to have persist, possibly intentionally. Possibly not! But it's an API we've had for over 10 years now.
Next, pulling up https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-co... there's two important things. First, there's no way to flag "this is a random MAC, do not use" (if after all, that's what the user wants, such as in Michael's case). And second, yes we probably ought to have enforced "mac-address" being the random one we had been using, all along. Then we pull up https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/... and see that oh the kernel's already made some assumptions about what might be set where and why because of seemingly arbitrary things we were doing that may or may not have matched up with the at the time binding.
So no, in sum, I'm not convinced that the best path forward right here and now is to put the random MAC in the environment, not correct the now incorrect help text and not even poke the binding maintainer nor relevant lists to see how exactly they think it would be best to handle a locally administered MAC being used as there are both valid use cases for "use this in the kernel" and "disregard this in the kernel".
Dear Tom,
In message 20211117115025.GV24579@bill-the-cat you wrote:
If the MAC address is not placed in the environment, then how can a user query the currently used MAC address? All documentation says basically: run "printenv ethaddr".
Update the documentation and say to use "net list" or read the previous part of console log that says we're using a random mac in this case? The more I look here the more I see we're changing part of the API with the OS, and it's not something that should be done without consulting the consumers too.
Well... "printenv ethaddr" has been here ever since U-Boot (or PPCBoot, as it was still called at that time) got network support. i. e. more than 21 years ago.
"net list: has only been added very recently, just 5 months ago in commit 8a3987f47 "cmd: net: add a 'net list' command to list network devs".
I bet that an overwhelming majority of users would use "printenv ethaddr" when asked to check the MAC address of a system.
Next, pulling up https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-c= ontroller.yaml there's two important things. First, there's no way to flag "this is a random MAC, do not use" (if after all, that's what the user wants, such as in Michael's case). And second, yes we probably ought to have enforced "mac-address" being the random one we had been using, all along.
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.
So no, in sum, I'm not convinced that the best path forward right here and now is to put the random MAC in the environment, not correct the now incorrect help text and not even poke the binding maintainer nor relevant lists to see how exactly they think it would be best to handle a locally administered MAC being used as there are both valid use cases for "use this in the kernel" and "disregard this in the kernel".
I'm afraid I do not understand what exactly you are proposing here?
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").
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.
Best regards,
Wolfgang Denk
On Wed, Nov 17, 2021 at 01:24:58PM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 20211117115025.GV24579@bill-the-cat you wrote:
If the MAC address is not placed in the environment, then how can a user query the currently used MAC address? All documentation says basically: run "printenv ethaddr".
Update the documentation and say to use "net list" or read the previous part of console log that says we're using a random mac in this case? The more I look here the more I see we're changing part of the API with the OS, and it's not something that should be done without consulting the consumers too.
Well... "printenv ethaddr" has been here ever since U-Boot (or PPCBoot, as it was still called at that time) got network support. i. e. more than 21 years ago.
"net list: has only been added very recently, just 5 months ago in commit 8a3987f47 "cmd: net: add a 'net list' command to list network devs".
OK, so "net list" hasn't been broken that long, good.
I bet that an overwhelming majority of users would use "printenv ethaddr" when asked to check the MAC address of a system.
And it's not shown the randomly generated one for that long either.
Next, pulling up https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-c= ontroller.yaml there's two important things. First, there's no way to flag "this is a random MAC, do not use" (if after all, that's what the user wants, such as in Michael's case). And second, yes we probably ought to have enforced "mac-address" being the random one we had been using, all along.
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?
So no, in sum, I'm not convinced that the best path forward right here and now is to put the random MAC in the environment, not correct the now incorrect help text and not even poke the binding maintainer nor relevant lists to see how exactly they think it would be best to handle a locally administered MAC being used as there are both valid use cases for "use this in the kernel" and "disregard this in the kernel".
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.
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.
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.
Dear Tom,
In message 20211117123548.GX24579@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.]
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].
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.
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
On Wed, Nov 17, 2021 at 04:56:27PM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 20211117123548.GX24579@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@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.
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
Dear Tom,
In message 1889944.1637219294@gemini.denx.de I wrote:
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.
We could not find even a single board maintained or upstreamed by DENX where NET_RANDOM_ETHADDR was even mentioned in the specifications, and certainly not a mandatory feature.
That's all copy & paste garbage.
Viele Grüße,
Wolfgang Denk
On Thu, Nov 18, 2021 at 10:46:28AM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 1889944.1637219294@gemini.denx.de I wrote:
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.
We could not find even a single board maintained or upstreamed by DENX where NET_RANDOM_ETHADDR was even mentioned in the specifications, and certainly not a mandatory feature.
That's all copy & paste garbage.
OK, can we please get some patches going then to remove them, starting with the platforms you have, and then the reference platforms it was copypastad over from? Thanks!
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.
Dear Tom,
In message 20211118162920.GH24579@bill-the-cat you wrote:
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.
Correct - this is a bug in the implementation of thispatch, and apparently the few people that ever used it did not notice it or care enough about it to submit fixes.
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).
Agreed.
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-sen= d-email-joe.hershberger@ni.com/ https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen= d-email-joe.hershberger@ni.com/ https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen= d-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
Ummm... From which part of the patches or the comments do you take this conclusion? Not a single line of code, or comments in the code or commit messages, nor any of the review comments refer to the "ethadddr" environment variable. I thing it was just an oversight to set it here, which was not detected during testing as everything appeared to work.
I actually see the opposite - Joes initial commit messages speaks about "Implement the random *ethaddr* fallback", and as his following patches clean up the use of hard codes definitions of the "ethaddr" environment variable, I see it more likely that he means the environment variable and not MAC address.
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.
Agreed.
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..
No, it works only half. It fails to set the "ethaddr" environment variable which is mandatory for correct funktion (query by the user, passing to Linux).
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.
This makes no sense to me. Instead of documenting the bug we should fix it and add the missing eth_env_set_enetaddr().
If I prepare such patches, will you accept these?
Best regards,
Wolfgang Denk
On Thu, Nov 18, 2021 at 08:04:22PM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 20211118162920.GH24579@bill-the-cat you wrote:
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.
Correct - this is a bug in the implementation of thispatch, and apparently the few people that ever used it did not notice it or care enough about it to submit fixes.
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).
Agreed.
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-sen= d-email-joe.hershberger@ni.com/ https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen= d-email-joe.hershberger@ni.com/ https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen= d-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
Ummm... From which part of the patches or the comments do you take this conclusion? Not a single line of code, or comments in the code
The same part I keep asking to have fixed in a v2, the help text by the Kconfig option: + A new MAC address will be generated on every boot and it will + not be added to the environment.
[snip]
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.
This makes no sense to me. Instead of documenting the bug we should fix it and add the missing eth_env_set_enetaddr().
If I prepare such patches, will you accept these?
Now I'm confused. Taking the patch this whole thread is attached to, we make NET_RANDOM_ETHADDR perform eth_env_set_enetaddr_by_index which is missing (well, only in the DM case, the non-DM case isn't updated and I guess should also be?). My problem is that the Kconfig help text for NET_RANDOM_ETHADDR still reflects what I pasted from the initial commit of it, a sentence saying we don't update the environment. This change does, so the help is wrong and needs fixing. I have come around to agreeing with the concept of this patch, that NET_RANDOM_ETHADDR should cause the environment to be updated and in turn fdt_fixup_ethernet() will populate this to Linux.
Hi,
On 11/18/21 20:54, Tom Rini wrote:
On Thu, Nov 18, 2021 at 08:04:22PM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 20211118162920.GH24579@bill-the-cat you wrote:
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.
Correct - this is a bug in the implementation of thispatch, and apparently the few people that ever used it did not notice it or care enough about it to submit fixes.
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).
Agreed.
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-sen= d-email-joe.hershberger@ni.com/ https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen= d-email-joe.hershberger@ni.com/ https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen= d-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
Ummm... From which part of the patches or the comments do you take this conclusion? Not a single line of code, or comments in the code
The same part I keep asking to have fixed in a v2, the help text by the Kconfig option:
A new MAC address will be generated on every boot and it willnot be added to the environment.[snip]
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.
This makes no sense to me. Instead of documenting the bug we should fix it and add the missing eth_env_set_enetaddr().
If I prepare such patches, will you accept these?
Now I'm confused. Taking the patch this whole thread is attached to, we make NET_RANDOM_ETHADDR perform eth_env_set_enetaddr_by_index which is missing (well, only in the DM case, the non-DM case isn't updated and I guess should also be?). My problem is that the Kconfig help text for NET_RANDOM_ETHADDR still reflects what I pasted from the initial commit of it, a sentence saying we don't update the environment. This change does, so the help is wrong and needs fixing. I have come around to agreeing with the concept of this patch, that NET_RANDOM_ETHADDR should cause the environment to be updated and in turn fdt_fixup_ethernet() will populate this to Linux.
This thread is getting quite long and I think that would be the best if you (both) can create patch based on how you see it should work and send it over. Based on it it will be super clear how you think things should work. My initial intention was to show mac addresses via net list (Actually I was investigating how to probe all ethernet controllers without calling networking command for getting IPs out of reset with power domain driver) which is solved by Michael's patch when he send v2 version.
Thanks, Michal
On Fri, Nov 19, 2021 at 01:30:06PM +0100, Michal Simek wrote:
Hi,
On 11/18/21 20:54, Tom Rini wrote:
On Thu, Nov 18, 2021 at 08:04:22PM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 20211118162920.GH24579@bill-the-cat you wrote:
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.
Correct - this is a bug in the implementation of thispatch, and apparently the few people that ever used it did not notice it or care enough about it to submit fixes.
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).
Agreed.
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-sen= d-email-joe.hershberger@ni.com/ https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen= d-email-joe.hershberger@ni.com/ https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen= d-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
Ummm... From which part of the patches or the comments do you take this conclusion? Not a single line of code, or comments in the code
The same part I keep asking to have fixed in a v2, the help text by the Kconfig option:
A new MAC address will be generated on every boot and it willnot be added to the environment.[snip]
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.
This makes no sense to me. Instead of documenting the bug we should fix it and add the missing eth_env_set_enetaddr().
If I prepare such patches, will you accept these?
Now I'm confused. Taking the patch this whole thread is attached to, we make NET_RANDOM_ETHADDR perform eth_env_set_enetaddr_by_index which is missing (well, only in the DM case, the non-DM case isn't updated and I guess should also be?). My problem is that the Kconfig help text for NET_RANDOM_ETHADDR still reflects what I pasted from the initial commit of it, a sentence saying we don't update the environment. This change does, so the help is wrong and needs fixing. I have come around to agreeing with the concept of this patch, that NET_RANDOM_ETHADDR should cause the environment to be updated and in turn fdt_fixup_ethernet() will populate this to Linux.
This thread is getting quite long and I think that would be the best if you (both) can create patch based on how you see it should work and send it over. Based on it it will be super clear how you think things should work. My initial intention was to show mac addresses via net list (Actually I was investigating how to probe all ethernet controllers without calling networking command for getting IPs out of reset with power domain driver) which is solved by Michael's patch when he send v2 version.
Reasonable request, done: https://patchwork.ozlabs.org/project/uboot/patch/20211120155358.376540-1-tri...
participants (6)
-
Grygorii Strashko -
Michael Walle -
Michal Simek -
Ramon Fried -
Tom Rini -
Wolfgang Denk