[PATCH u-boot 0/5] Only call aspeednic_init when preparing to use network

[PATCH u-boot 0/5] Only call aspeednic_init when preparing to use network

[OpenBMC Patches]

The aspeednic driver was calling init from its initialization function and
calling halt later.  This left the DMA engine running and writing to
memory when Ethernet traffic arrived when control was handed off
to the operating system.

Remove this call to init and rely on the framework to call
write_hwaddr with the assigned MAC address.

In addition upstream discourages setting the random address in
drivers.  Later upstream created a config for this behavior and I
back-ported that patch.

In my testing, the random mac address is about 90% 1 value after a
u-boot reset command but after a kernel reboot it does not repeat.

The code uses rand with timer as the seed source; the network stack
then uses the mac as the seed for random delays.  We may prefer
to only configure u-boot setting a random mac only after a better
source of entropy, letting the kernel use its sources to set it when
we do not net-boot.

milton

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Review on Reviewable"/>](https://reviewable.io/reviews/openbmc/u-boot/4)
<!-- Reviewable:end -->


https://github.com/openbmc/u-boot/pull/4

Joe Hershberger (1):
  net: Implement random ethaddr fallback in eth.c

Milton D. Miller II (4):
  net: aspeednic: Create aspeed_write_hwaddr from set_mac_address
  net: aspeednic: Do not start hardware in initialize
  net: aspeednic: Remove extra spaces before assignments
  net: aspeednic: Do not fill in a random MAC address

 README                  |  3 ++-
 doc/README.enetaddr     |  2 ++
 drivers/net/aspeednic.c | 31 ++++++++++++++-----------------
 net/eth.c               | 13 ++++++++++++-
 4 files changed, 30 insertions(+), 19 deletions(-)

-- 
2.7.1

[PATCH u-boot 1/5] net: aspeednic: Create aspeed_write_hwaddr from set_mac_address

[OpenBMC Patches]

From: "Milton D. Miller II" <miltonm@us.ibm.com>

Tell the ethernet framework how to set the MAC address on the
aspeed nic so it can be set at the expected points in the code.

Rename set_mac_address to aspeed_write_hwaddr.  Drop the unused
argument, change the prototype to int, and return 0.

Assign the device write_hwaddr method to this new function.

Signed-off-by: Milton Miller <miltonm@us.ibm.com>
---
 drivers/net/aspeednic.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/aspeednic.c b/drivers/net/aspeednic.c
index d70c7ab..b8ce24c 100644
--- a/drivers/net/aspeednic.c
+++ b/drivers/net/aspeednic.c
@@ -411,7 +411,7 @@ static int   aspeednic_init(struct eth_device* dev, bd_t* bis);
 static int   aspeednic_send(struct eth_device* dev, volatile void *packet, int length);
 static int   aspeednic_recv(struct eth_device* dev);
 static void  aspeednic_halt(struct eth_device* dev);
-static void  set_mac_address (struct eth_device* dev, bd_t* bis);
+static int   aspeednic_write_hwaddr(struct eth_device* dev);
 static void  phy_write_register (struct eth_device* dev, u8 PHY_Register, u8 PHY_Address, u16 PHY_Data);
 static u16   phy_read_register (struct eth_device* dev, u8 PHY_Register, u8 PHY_Address);
 #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
@@ -564,6 +564,7 @@ int aspeednic_initialize(bd_t *bis)
   dev->halt   = aspeednic_halt;
   dev->send   = aspeednic_send;
   dev->recv   = aspeednic_recv;
+  dev->write_hwaddr = aspeednic_write_hwaddr;
 
   /* Ensure we're not sleeping. */
   if (CONFIG_ASPEED_MAC_PHY_SETTING >= 1) {
@@ -1157,7 +1158,7 @@ static int aspeednic_init(struct eth_device* dev, bd_t* bis)
 
   aspeednic_probe_phy(dev);
 
-  set_mac_address (dev, bis);
+  aspeednic_write_hwaddr(dev);
   set_mac_control_register (dev);
 
   for (i = 0; i < NUM_RX_DESC; i++) {
@@ -1362,7 +1363,7 @@ static void aspeednic_halt(struct eth_device* dev)
   STOP_MAC(dev);
 }
 
-static void set_mac_address (struct eth_device* dev, bd_t* bis)
+static int aspeednic_write_hwaddr(struct eth_device* dev)
 {
   if (!eth_getenv_enetaddr_by_index("eth", 0, dev->enetaddr)) {
     eth_random_enetaddr(dev->enetaddr);
@@ -1374,6 +1375,8 @@ static void set_mac_address (struct eth_device* dev, bd_t* bis)
   if (CONFIG_ASPEED_MAC_PHY_SETTING >= 1) {
     memcpy(NCSI_Request.SA, dev->enetaddr, 6);
   }
+
+  return 0;
 }
 
 static u16 phy_read_register (struct eth_device* dev, u8 PHY_Register, u8 PHY_Address)
-- 
2.7.1

Re: [PATCH u-boot 1/5] net: aspeednic: Create aspeed_write_hwaddr from set_mac_address

[Cyril Bur]

On Fri, 11 Mar 2016 12:40:40 -0600
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

> From: "Milton D. Miller II" <miltonm@us.ibm.com>
> 
> Tell the ethernet framework how to set the MAC address on the
> aspeed nic so it can be set at the expected points in the code.
> 
> Rename set_mac_address to aspeed_write_hwaddr.  Drop the unused
> argument, change the prototype to int, and return 0.
> 
> Assign the device write_hwaddr method to this new function.
> 
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>

Reviewed-by: Cyril Bur <cyrilbur@gmail.com>

> ---
>  drivers/net/aspeednic.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/aspeednic.c b/drivers/net/aspeednic.c
> index d70c7ab..b8ce24c 100644
> --- a/drivers/net/aspeednic.c
> +++ b/drivers/net/aspeednic.c
> @@ -411,7 +411,7 @@ static int   aspeednic_init(struct eth_device* dev, bd_t* bis);
>  static int   aspeednic_send(struct eth_device* dev, volatile void *packet, int length);
>  static int   aspeednic_recv(struct eth_device* dev);
>  static void  aspeednic_halt(struct eth_device* dev);
> -static void  set_mac_address (struct eth_device* dev, bd_t* bis);
> +static int   aspeednic_write_hwaddr(struct eth_device* dev);
>  static void  phy_write_register (struct eth_device* dev, u8 PHY_Register, u8 PHY_Address, u16 PHY_Data);
>  static u16   phy_read_register (struct eth_device* dev, u8 PHY_Register, u8 PHY_Address);
>  #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
> @@ -564,6 +564,7 @@ int aspeednic_initialize(bd_t *bis)
>    dev->halt   = aspeednic_halt;
>    dev->send   = aspeednic_send;
>    dev->recv   = aspeednic_recv;
> +  dev->write_hwaddr = aspeednic_write_hwaddr;
>  
>    /* Ensure we're not sleeping. */
>    if (CONFIG_ASPEED_MAC_PHY_SETTING >= 1) {
> @@ -1157,7 +1158,7 @@ static int aspeednic_init(struct eth_device* dev, bd_t* bis)
>  
>    aspeednic_probe_phy(dev);
>  
> -  set_mac_address (dev, bis);
> +  aspeednic_write_hwaddr(dev);
>    set_mac_control_register (dev);
>  
>    for (i = 0; i < NUM_RX_DESC; i++) {
> @@ -1362,7 +1363,7 @@ static void aspeednic_halt(struct eth_device* dev)
>    STOP_MAC(dev);
>  }
>  
> -static void set_mac_address (struct eth_device* dev, bd_t* bis)
> +static int aspeednic_write_hwaddr(struct eth_device* dev)
>  {
>    if (!eth_getenv_enetaddr_by_index("eth", 0, dev->enetaddr)) {
>      eth_random_enetaddr(dev->enetaddr);
> @@ -1374,6 +1375,8 @@ static void set_mac_address (struct eth_device* dev, bd_t* bis)
>    if (CONFIG_ASPEED_MAC_PHY_SETTING >= 1) {
>      memcpy(NCSI_Request.SA, dev->enetaddr, 6);
>    }
> +
> +  return 0;
>  }
>  
>  static u16 phy_read_register (struct eth_device* dev, u8 PHY_Register, u8 PHY_Address)

[PATCH u-boot 2/5] net: aspeednic: Do not start hardware in initialize

[OpenBMC Patches]

From: "Milton D. Miller II" <miltonm@us.ibm.com>

Delete the call to aspeenic_init from the aspeenic_initialize
function.  Instead rely on the framework to call the write_hwaddr
method to set the MAC address for the operating system to use.

The aspeednic driver was calling init before registering the
hardware, causing the dma engine to be left running writing
incoming ethernet packets to memory when handing control to the
operating system.

The initialize function is specifically documented to not touch
the hardware or probe in doc/README.drivers.net.

Also, the init method can take a significant amount of time
especially when NC-SI and retries are involved.  In addition
extra work is being performed to find the MAC address which the
framework will do again later.

Signed-off-by: Milton Miller <miltonm@us.ibm.com>
---
 drivers/net/aspeednic.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/aspeednic.c b/drivers/net/aspeednic.c
index b8ce24c..90c0f0d 100644
--- a/drivers/net/aspeednic.c
+++ b/drivers/net/aspeednic.c
@@ -574,8 +574,6 @@ int aspeednic_initialize(bd_t *bis)
     udelay(10 * 1000);
   }
 
-  dev->init(dev, bis);
-
   eth_register(dev);
 
 #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
-- 
2.7.1

Re: [PATCH u-boot 2/5] net: aspeednic: Do not start hardware in initialize

[Cyril Bur]

On Fri, 11 Mar 2016 12:40:41 -0600
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

> From: "Milton D. Miller II" <miltonm@us.ibm.com>
> 
> Delete the call to aspeenic_init from the aspeenic_initialize
> function.  Instead rely on the framework to call the write_hwaddr
> method to set the MAC address for the operating system to use.
> 
> The aspeednic driver was calling init before registering the
> hardware, causing the dma engine to be left running writing
> incoming ethernet packets to memory when handing control to the
> operating system.
> 
> The initialize function is specifically documented to not touch
> the hardware or probe in doc/README.drivers.net.
> 
> Also, the init method can take a significant amount of time
> especially when NC-SI and retries are involved.  In addition
> extra work is being performed to find the MAC address which the
> framework will do again later.
> 
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>

Reviewed-by: Cyril Bur <cyrilbur@gmail.com>

> ---
>  drivers/net/aspeednic.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/aspeednic.c b/drivers/net/aspeednic.c
> index b8ce24c..90c0f0d 100644
> --- a/drivers/net/aspeednic.c
> +++ b/drivers/net/aspeednic.c
> @@ -574,8 +574,6 @@ int aspeednic_initialize(bd_t *bis)
>      udelay(10 * 1000);
>    }
>  
> -  dev->init(dev, bis);
> -
>    eth_register(dev);
>  
>  #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)

[PATCH u-boot 3/5] net: aspeednic: Remove extra spaces before assignments

[OpenBMC Patches]

From: "Milton D. Miller II" <miltonm@us.ibm.com>

The device method assignments had extra spaces before the =
signs but all prior methods were 4 characters and so they
were aligned.  Since write_hwaddr is significantly longer,
the resulting spacing is not pleasing.  Since only a few other
sites had simiar whitespace replace all multi-space sequences
before an assignemnt with one space.

Signed-off-by: Milton Miller <miltonm@us.ibm.com>
---
 drivers/net/aspeednic.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/aspeednic.c b/drivers/net/aspeednic.c
index 90c0f0d..841df8b 100644
--- a/drivers/net/aspeednic.c
+++ b/drivers/net/aspeednic.c
@@ -560,10 +560,10 @@ int aspeednic_initialize(bd_t *bis)
   *(volatile u_long *)(SCU_BASE + SCU_SCRATCH_REGISTER) = cpu_to_le32((SCURegister & ~(0x3000)) | (CONFIG_MAC2_PHY_SETTING << 12));
 
 
-  dev->init   = aspeednic_init;
-  dev->halt   = aspeednic_halt;
-  dev->send   = aspeednic_send;
-  dev->recv   = aspeednic_recv;
+  dev->init = aspeednic_init;
+  dev->halt = aspeednic_halt;
+  dev->send = aspeednic_send;
+  dev->recv = aspeednic_recv;
   dev->write_hwaddr = aspeednic_write_hwaddr;
 
   /* Ensure we're not sleeping. */
@@ -1267,9 +1267,9 @@ static int aspeednic_send(struct eth_device* dev, volatile void *packet, int len
 //            memset ((void *)cpu_to_le32((u32) (packet + length)), 0, 60 - length);
     length = 60;
   }
-  tx_ring[tx_new].buf    = cpu_to_le32(((u32) packet));
-  tx_ring[tx_new].status   &= (~(0x3FFF));
-  tx_ring[tx_new].status   |= cpu_to_le32(LTS | FTS | length);
+  tx_ring[tx_new].buf = cpu_to_le32(((u32) packet));
+  tx_ring[tx_new].status &= (~(0x3FFF));
+  tx_ring[tx_new].status |= cpu_to_le32(LTS | FTS | length);
   tx_ring[tx_new].status |= cpu_to_le32(TXDMA_OWN);
 
   OUTL(dev, POLL_DEMAND, TXPD_REG);
@@ -1297,7 +1297,7 @@ static int aspeednic_send(struct eth_device* dev, volatile void *packet, int len
 static int aspeednic_recv(struct eth_device* dev)
 {
   s32   status;
-  int   length    = 0;
+  int   length = 0;
 
   for ( ; ; )
   {
-- 
2.7.1

Re: [PATCH u-boot 3/5] net: aspeednic: Remove extra spaces before assignments

[Cyril Bur]

On Fri, 11 Mar 2016 12:40:42 -0600
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

> From: "Milton D. Miller II" <miltonm@us.ibm.com>
> 
> The device method assignments had extra spaces before the =
> signs but all prior methods were 4 characters and so they
> were aligned.  Since write_hwaddr is significantly longer,
> the resulting spacing is not pleasing.  Since only a few other
> sites had simiar whitespace replace all multi-space sequences
> before an assignemnt with one space.
> 
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>

Reviewed-by: Cyril Bur <cyrilbur@gmail.com>

> ---
>  drivers/net/aspeednic.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/aspeednic.c b/drivers/net/aspeednic.c
> index 90c0f0d..841df8b 100644
> --- a/drivers/net/aspeednic.c
> +++ b/drivers/net/aspeednic.c
> @@ -560,10 +560,10 @@ int aspeednic_initialize(bd_t *bis)
>    *(volatile u_long *)(SCU_BASE + SCU_SCRATCH_REGISTER) = cpu_to_le32((SCURegister & ~(0x3000)) | (CONFIG_MAC2_PHY_SETTING << 12));
>  
>  
> -  dev->init   = aspeednic_init;
> -  dev->halt   = aspeednic_halt;
> -  dev->send   = aspeednic_send;
> -  dev->recv   = aspeednic_recv;
> +  dev->init = aspeednic_init;
> +  dev->halt = aspeednic_halt;
> +  dev->send = aspeednic_send;
> +  dev->recv = aspeednic_recv;
>    dev->write_hwaddr = aspeednic_write_hwaddr;
>  
>    /* Ensure we're not sleeping. */
> @@ -1267,9 +1267,9 @@ static int aspeednic_send(struct eth_device* dev, volatile void *packet, int len
>  //            memset ((void *)cpu_to_le32((u32) (packet + length)), 0, 60 - length);
>      length = 60;
>    }
> -  tx_ring[tx_new].buf    = cpu_to_le32(((u32) packet));
> -  tx_ring[tx_new].status   &= (~(0x3FFF));
> -  tx_ring[tx_new].status   |= cpu_to_le32(LTS | FTS | length);
> +  tx_ring[tx_new].buf = cpu_to_le32(((u32) packet));
> +  tx_ring[tx_new].status &= (~(0x3FFF));
> +  tx_ring[tx_new].status |= cpu_to_le32(LTS | FTS | length);
>    tx_ring[tx_new].status |= cpu_to_le32(TXDMA_OWN);
>  
>    OUTL(dev, POLL_DEMAND, TXPD_REG);
> @@ -1297,7 +1297,7 @@ static int aspeednic_send(struct eth_device* dev, volatile void *packet, int len
>  static int aspeednic_recv(struct eth_device* dev)
>  {
>    s32   status;
> -  int   length    = 0;
> +  int   length = 0;
>  
>    for ( ; ; )
>    {

[PATCH u-boot 4/5] net: Implement random ethaddr fallback in eth.c

[OpenBMC Patches]

From: Joe Hershberger <joe.hershberger@ni.com>

Commit bef1014b31c5b33052bcaa865ba3618d73e906f0 upstream.

Backport changes CONFIG_NET_RANDOM_ETHADDR to CONFIG_RANDOM_MACADDR,
net_random_ethaddr to eth_random_enetaddr, and is_zero_ethaddr to
is_zero_ether_addr, and keeps existing function order.

Upstream log:

Implement the random ethaddr fallback in eth.c so it is in a common
place and not reimplemented in each board or driver that wants this
behavior.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

Signed-off-by: Milton D. Miller II <miltonm@us.ibm.com>
---
 README              |  3 ++-
 doc/README.enetaddr |  2 ++
 net/eth.c           | 13 ++++++++++++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/README b/README
index bcfffc3..e06dc8a 100644
--- a/README
+++ b/README
@@ -4764,7 +4764,8 @@ o If both the SROM and the environment contain a MAC address, and the
   warning is printed.
 
 o If neither SROM nor the environment contain a MAC address, an error
-  is raised.
+  is raised. If CONFIG_NET_RANDOM_ETHADDR is defined, then in this case
+  a random, locally-assigned MAC is used.
 
 If Ethernet drivers implement the 'write_hwaddr' function, valid MAC addresses
 will be programmed into hardware as part of the initialization process.	 This
diff --git a/doc/README.enetaddr b/doc/README.enetaddr
index 1eaeaf9..611d5a4 100644
--- a/doc/README.enetaddr
+++ b/doc/README.enetaddr
@@ -37,6 +37,8 @@ Correct flow of setting up the MAC address (summarized):
    environment variable will be used unchanged.
    If the environment variable is not set, it will be initialized from
    eth_device->enetaddr, and a warning will be printed.
+   If both are invalid and CONFIG_NET_RANDOM_ETHADDR is defined, a random,
+   locally-assigned MAC is written to eth_device->enetaddr.
 4. Program the address into hardware if the following conditions are met:
 	a) The relevant driver has a 'write_addr' function
 	b) The user hasn't set an 'ethmacskip' environment variable
diff --git a/net/eth.c b/net/eth.c
index 321d5b1..22d5adb 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -109,6 +109,7 @@ static int __def_eth_init(bd_t *bis)
 {
 	return -1;
 }
+
 int cpu_eth_init(bd_t *bis) __attribute__((weak, alias("__def_eth_init")));
 int board_eth_init(bd_t *bis) __attribute__((weak, alias("__def_eth_init")));
 
@@ -214,7 +215,17 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 		eth_setenv_enetaddr_by_index(base_name, eth_number,
 					     dev->enetaddr);
 		printf("\nWarning: %s using MAC address from net device\n",
-			dev->name);
+		       dev->name);
+	} else if (is_zero_ether_addr(dev->enetaddr)) {
+#ifdef CONFIG_RANDOM_MACADDR
+		eth_random_enetaddr(dev->enetaddr);
+		printf("\nWarning: %s (eth%d) using random MAC address - %pM\n",
+		       dev->name, eth_number, dev->enetaddr);
+#else
+		printf("\nError: %s address not set.\n",
+		       dev->name);
+		return -EINVAL;
+#endif
 	}
 
 	if (dev->write_hwaddr &&
-- 
2.7.1

Re: [PATCH u-boot 4/5] net: Implement random ethaddr fallback in eth.c

[Cyril Bur]

On Fri, 11 Mar 2016 12:40:43 -0600
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

> From: Joe Hershberger <joe.hershberger@ni.com>
> 
> Commit bef1014b31c5b33052bcaa865ba3618d73e906f0 upstream.
> 
> Backport changes CONFIG_NET_RANDOM_ETHADDR to CONFIG_RANDOM_MACADDR,
> net_random_ethaddr to eth_random_enetaddr, and is_zero_ethaddr to
> is_zero_ether_addr, and keeps existing function order.
> 

Should s/CONFIG_NET_RANDOM_ETHADDR/CONFIG_RANDOM_MACADDR/ have been done
everywhere? It would be best that the README files still reflect what the code
actually does no?

Cyril

> Upstream log:
> 
> Implement the random ethaddr fallback in eth.c so it is in a common
> place and not reimplemented in each board or driver that wants this
> behavior.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Signed-off-by: Milton D. Miller II <miltonm@us.ibm.com>
> ---
>  README              |  3 ++-
>  doc/README.enetaddr |  2 ++
>  net/eth.c           | 13 ++++++++++++-
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/README b/README
> index bcfffc3..e06dc8a 100644
> --- a/README
> +++ b/README
> @@ -4764,7 +4764,8 @@ o If both the SROM and the environment contain a MAC address, and the
>    warning is printed.
>  
>  o If neither SROM nor the environment contain a MAC address, an error
> -  is raised.
> +  is raised. If CONFIG_NET_RANDOM_ETHADDR is defined, then in this case
> +  a random, locally-assigned MAC is used.
>  
>  If Ethernet drivers implement the 'write_hwaddr' function, valid MAC addresses
>  will be programmed into hardware as part of the initialization process.	 This
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index 1eaeaf9..611d5a4 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -37,6 +37,8 @@ Correct flow of setting up the MAC address (summarized):
>     environment variable will be used unchanged.
>     If the environment variable is not set, it will be initialized from
>     eth_device->enetaddr, and a warning will be printed.
> +   If both are invalid and CONFIG_NET_RANDOM_ETHADDR is defined, a random,
> +   locally-assigned MAC is written to eth_device->enetaddr.
>  4. Program the address into hardware if the following conditions are met:
>  	a) The relevant driver has a 'write_addr' function
>  	b) The user hasn't set an 'ethmacskip' environment variable
> diff --git a/net/eth.c b/net/eth.c
> index 321d5b1..22d5adb 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -109,6 +109,7 @@ static int __def_eth_init(bd_t *bis)
>  {
>  	return -1;
>  }
> +
>  int cpu_eth_init(bd_t *bis) __attribute__((weak, alias("__def_eth_init")));
>  int board_eth_init(bd_t *bis) __attribute__((weak, alias("__def_eth_init")));
>  
> @@ -214,7 +215,17 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>  		eth_setenv_enetaddr_by_index(base_name, eth_number,
>  					     dev->enetaddr);
>  		printf("\nWarning: %s using MAC address from net device\n",
> -			dev->name);
> +		       dev->name);
> +	} else if (is_zero_ether_addr(dev->enetaddr)) {
> +#ifdef CONFIG_RANDOM_MACADDR
> +		eth_random_enetaddr(dev->enetaddr);
> +		printf("\nWarning: %s (eth%d) using random MAC address - %pM\n",
> +		       dev->name, eth_number, dev->enetaddr);
> +#else
> +		printf("\nError: %s address not set.\n",
> +		       dev->name);
> +		return -EINVAL;
> +#endif
>  	}
>  
>  	if (dev->write_hwaddr &&

[PATCH u-boot 5/5] net: aspeednic: Do not fill in a random MAC address

[OpenBMC Patches]

From: "Milton D. Miller II" <miltonm@us.ibm.com>

Do not check the u-boot environemnt or fill in a random address
in the write_hwaddr hook, instead rely on the framework to do so.

The doc/README.ethaddr specifically states that random addresses
are only to be assigned as part of a emergency such as a netboot
recovery command.

The upstream commit created a config variable to assign a
random mac when none is set leaving it zero and that has now
been backported.

Note: The hardware address is reset to 0 as part of the ethernet
reset performed at boot.  If no valid MAC address is found in
the environment the hardware will contain zeros and the operating
system will assign a valid random MAC address if u-boot is
configured not to.

The net effect is an attempt to use the network will result in
the ethernet address not set warning being printed if the ethaddr
variable is not set, and a warning iwth the random mac address
if the config is set.  If a valid ethernet address is set
in the environment it will be programmed in the hardware and
used by the operating system.

Signed-off-by: Milton Miller <miltonm@us.ibm.com>
---
 drivers/net/aspeednic.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/aspeednic.c b/drivers/net/aspeednic.c
index 841df8b..e80561f 100644
--- a/drivers/net/aspeednic.c
+++ b/drivers/net/aspeednic.c
@@ -1363,10 +1363,6 @@ static void aspeednic_halt(struct eth_device* dev)
 
 static int aspeednic_write_hwaddr(struct eth_device* dev)
 {
-  if (!eth_getenv_enetaddr_by_index("eth", 0, dev->enetaddr)) {
-    eth_random_enetaddr(dev->enetaddr);
-  }
-
   OUTL(dev, ((dev->enetaddr[2] << 24) | (dev->enetaddr[3] << 16)
              | (dev->enetaddr[4] << 8) | dev->enetaddr[5]), MAC_LADR_REG);
   OUTL(dev, ((dev->enetaddr[0] << 8) | dev->enetaddr[1]), MAC_MADR_REG);
-- 
2.7.1

Re: [PATCH u-boot 5/5] net: aspeednic: Do not fill in a random MAC address

[Cyril Bur]

On Fri, 11 Mar 2016 12:40:44 -0600
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

> From: "Milton D. Miller II" <miltonm@us.ibm.com>
> 

Hi Milton,

Series looks nice.

One question about this patch, should you also have removed the call to
aspeednick_write_hwaddr() from aspeednic_initialize() since the eth layer will
do it for you? Not a bit deal at all, the eth layer will also detect that it's
been done and just skip it?



> Do not check the u-boot environemnt or fill in a random address
> in the write_hwaddr hook, instead rely on the framework to do so.
> 
> The doc/README.ethaddr specifically states that random addresses
> are only to be assigned as part of a emergency such as a netboot
> recovery command.
> 
> The upstream commit created a config variable to assign a
> random mac when none is set leaving it zero and that has now
> been backported.
> 
> Note: The hardware address is reset to 0 as part of the ethernet
> reset performed at boot.  If no valid MAC address is found in
> the environment the hardware will contain zeros and the operating
> system will assign a valid random MAC address if u-boot is
> configured not to.
> 
> The net effect is an attempt to use the network will result in
> the ethernet address not set warning being printed if the ethaddr
> variable is not set, and a warning iwth the random mac address
> if the config is set.  If a valid ethernet address is set
> in the environment it will be programmed in the hardware and
> used by the operating system.
> 
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>
> ---
>  drivers/net/aspeednic.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/net/aspeednic.c b/drivers/net/aspeednic.c
> index 841df8b..e80561f 100644
> --- a/drivers/net/aspeednic.c
> +++ b/drivers/net/aspeednic.c
> @@ -1363,10 +1363,6 @@ static void aspeednic_halt(struct eth_device* dev)
>  
>  static int aspeednic_write_hwaddr(struct eth_device* dev)
>  {
> -  if (!eth_getenv_enetaddr_by_index("eth", 0, dev->enetaddr)) {
> -    eth_random_enetaddr(dev->enetaddr);
> -  }
> -
>    OUTL(dev, ((dev->enetaddr[2] << 24) | (dev->enetaddr[3] << 16)
>               | (dev->enetaddr[4] << 8) | dev->enetaddr[5]), MAC_LADR_REG);
>    OUTL(dev, ((dev->enetaddr[0] << 8) | dev->enetaddr[1]), MAC_MADR_REG);

Re: [PATCH u-boot 5/5] net: aspeednic: Do not fill in a random MAC address

[Milton Miller II]

[-- Attachment #1: Type: text/html, Size: 1398 bytes --]