Patchwork [U-Boot] net/eth: Don't issue warnings for offboard ethernet chips

login
register
mail settings
Submitter Kyle Moffett
Date Dec. 16, 2011, 2:17 a.m.
Message ID <1324001821-15337-1-git-send-email-Kyle.D.Moffett@boeing.com>
Download mbox | patch
Permalink /patch/131772/
State Changes Requested
Headers show

Comments

Kyle Moffett - Dec. 16, 2011, 2:17 a.m.
When using an offboard ethernet chip such as e1000, it is highly likely
that the driver has already read a valid MAC address from the onboard
EEPROM.  In that case, U-Boot should not issue a warning about the
absence of an "eth*addr" value in the environment.

Since the calling code in drivers/usb/eth/usb_ether.c and net/eth.c
cannot tell what the error really was, move the log messages down into
the eth_write_hwaddr() code.

The code is further improved by the use of is_valid_ether_addr(), etc.

A properly configured HWW-1U-1A board is fixed from this output:

  Net:   e1000: 00:50:93:81:ff:8a
         e1000: 00:50:93:81:ff:8b
         owt0, owt1, peer, e1000#0
  Warning: failed to set MAC address
  , e1000#1
  Warning: failed to set MAC address

To this:

  Net:   e1000: 00:50:93:81:ff:8a
         e1000: 00:50:93:81:ff:8b
         owt0, owt1, peer, e1000#0, e1000#1

Furthermore, the log messages should avoid screwing up the "Net:" output
formatting provided by the calling code, EG:

  Net:   eth0, eth1 [could not set MAC: 00:50:93:81:ff:8a], eth2

Finally, the code in the NE-2000 driver which was working around the
spurious error messages from the core code can be removed.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
Cc: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/ne2000_base.c   |   17 +-----------
 drivers/usb/eth/usb_ether.c |    4 +--
 net/eth.c                   |   56 ++++++++++++++++++++++++------------------
 3 files changed, 35 insertions(+), 42 deletions(-)
Wolfgang Denk - Dec. 17, 2011, 8:16 p.m.
Dear Kyle Moffett,

In message <1324001821-15337-1-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> When using an offboard ethernet chip such as e1000, it is highly likely
> that the driver has already read a valid MAC address from the onboard
> EEPROM.  In that case, U-Boot should not issue a warning about the
> absence of an "eth*addr" value in the environment.

Yes, it should.  The rule is that then environment settings always
have precedence, and if they are missing or contain different data
than other sources for this inofmration, a waning shall be printed.


> A properly configured HWW-1U-1A board is fixed from this output:
> 
>   Net:   e1000: 00:50:93:81:ff:8a
>          e1000: 00:50:93:81:ff:8b
>          owt0, owt1, peer, e1000#0
>   Warning: failed to set MAC address
>   , e1000#1
>   Warning: failed to set MAC address
> 
> To this:
> 
>   Net:   e1000: 00:50:93:81:ff:8a
>          e1000: 00:50:93:81:ff:8b
>          owt0, owt1, peer, e1000#0, e1000#1

This is also not correct.  There should never be any printing of the
MAC addresses here.

The messages should be:

	Net:   owt0, owt1, peer, e1000#0, e1000#1

> Furthermore, the log messages should avoid screwing up the "Net:" output
> formatting provided by the calling code, EG:
> 
>   Net:   eth0, eth1 [could not set MAC: 00:50:93:81:ff:8a], eth2

No.  "could not set" is an error message, and deserves a separate
line.


Best regards,

Wolfgang Denk
Kyle Moffett - Dec. 19, 2011, 4:41 p.m.
On Dec 17, 2011, at 15:16, Wolfgang Denk wrote:
> Dear Kyle Moffett,
> In message <1324001821-15337-1-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
>> When using an offboard ethernet chip such as e1000, it is highly likely
>> that the driver has already read a valid MAC address from the onboard
>> EEPROM.  In that case, U-Boot should not issue a warning about the
>> absence of an "eth*addr" value in the environment.
> 
> Yes, it should.  The rule is that then environment settings always
> have precedence, and if they are missing or contain different data
> than other sources for this information, a waning shall be printed.

That is a problem for devices which are typically add-in PCI cards.
In that case U-Boot can't be expected to have knowledge of what the
MAC addresses should be, and it should just use the ROM attached to
the card instead.

In our use case the e1000 chips are on a separate board attached via
CompactPCI.  U-Boot should not spontaneously start throwing errors
just because the board was stuck into a different slot or replaced
due to hardware failure.

I was careful to maintain the behavior that U-Boot will issue a
warning if there is no usable MAC address or if the environment has
a MAC address that does not match the board.

However, in the case that the board itself has a valid external MAC
address and U-Boot does not even have an environment variable, it
should not cause extra messages.  Think about hot-pluggable USB net
adapters where the detection order is nondeterministic.


>> A properly configured HWW-1U-1A board is fixed from this output:
>> 
>>  Net:   e1000: 00:50:93:81:ff:8a
>>         e1000: 00:50:93:81:ff:8b
>>         owt0, owt1, peer, e1000#0
>>  Warning: failed to set MAC address
>>  , e1000#1
>>  Warning: failed to set MAC address
>> 
>> To this:
>> 
>>  Net:   e1000: 00:50:93:81:ff:8a
>>         e1000: 00:50:93:81:ff:8b
>>         owt0, owt1, peer, e1000#0, e1000#1
> 
> This is also not correct.  There should never be any printing of the
> MAC addresses here.
> 

> The messages should be:
> 
> 	Net:   owt0, owt1, peer, e1000#0, e1000#1

The "e1000" driver has always done that.  I can submit a separate
patch to fix that if you would like.


>> Furthermore, the log messages should avoid screwing up the "Net:" output
>> formatting provided by the calling code, EG:
>> 
>>  Net:   eth0, eth1 [could not set MAC: 00:50:93:81:ff:8a], eth2
> 
> No.  "could not set" is an error message, and deserves a separate
> line.

Ok, I will respin the patch so that errors show up like this:

  Net:   eth0, eth1,
         ERROR: Could not set MAC address: 00:50:93:81:ff:8a
         eth2

Is that OK?

Cheers,
Kyle Moffett
Wolfgang Denk - Dec. 19, 2011, 5:55 p.m.
Dear "Moffett, Kyle D",

In message <58A08F2D-4743-4634-A909-466EB853570B@boeing.com> you wrote:
>
> > Yes, it should.  The rule is that then environment settings always
> > have precedence, and if they are missing or contain different data
> > than other sources for this information, a waning shall be printed.
> 
> That is a problem for devices which are typically add-in PCI cards.
> In that case U-Boot can't be expected to have knowledge of what the
> MAC addresses should be, and it should just use the ROM attached to
> the card instead.

The user can be expected to read the boot messages and adjust the
environment if he wants to use the card's settinge.  In any case, the
user shall have the authority to overwrite the card's settings by
defining any settings in the environment he wants.

> In our use case the e1000 chips are on a separate board attached via
> CompactPCI.  U-Boot should not spontaneously start throwing errors
> just because the board was stuck into a different slot or replaced
> due to hardware failure.

U-Boot does not throw errors if you have appropriate settings in the
environment.  The worst to happen is a warning thatt he MAC settings
in U-Boot and on the card don't match.

> However, in the case that the board itself has a valid external MAC
> address and U-Boot does not even have an environment variable, it
> should not cause extra messages.  Think about hot-pluggable USB net
> adapters where the detection order is nondeterministic.

Yes, it should, because a mandatory environment variable is not set
correctly.

> The "e1000" driver has always done that.  I can submit a separate
> patch to fix that if you would like.

Thats would be welcome, thanks.

> Ok, I will respin the patch so that errors show up like this:
> 
>   Net:   eth0, eth1,
>          ERROR: Could not set MAC address: 00:50:93:81:ff:8a
>          eth2
> 
> Is that OK?

Yes, thanks.

Best regards,

Wolfgang Denk

Patch

diff --git a/drivers/net/ne2000_base.c b/drivers/net/ne2000_base.c
index 8275091..20fa70e 100644
--- a/drivers/net/ne2000_base.c
+++ b/drivers/net/ne2000_base.c
@@ -710,21 +710,8 @@  static int ne2k_setup_driver(struct eth_device *dev)
 	nic.rx_buf_start = RX_START;
 	nic.rx_buf_end = RX_END;
 
-	/*
-	 * According to doc/README.enetaddr, drivers shall give priority
-	 * to the MAC address value in the environment, so we do not read
-	 * it from the prom or eeprom if it is specified in the environment.
-	 */
-	if (!eth_getenv_enetaddr("ethaddr", dev->enetaddr)) {
-		/* If the MAC address is not in the environment, get it: */
-		if (!get_prom(dev->enetaddr, nic.base)) /* get MAC from prom */
-			dp83902a_init(dev->enetaddr);   /* fallback: seeprom */
-		/* And write it into the environment otherwise eth_write_hwaddr
-		 * returns -1 due to eth_getenv_enetaddr_by_index() failing,
-		 * and this causes "Warning: failed to set MAC address", and
-		 * cmd_bdinfo has no ethaddr value which it can show: */
-		eth_setenv_enetaddr("ethaddr", dev->enetaddr);
-	}
+	if (!get_prom(dev->enetaddr, nic.base)) /* get MAC from prom */
+		dp83902a_init(dev->enetaddr);   /* fallback: seeprom */
 	return 0;
 }
 
diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
index 6565ea5..bb8be5a 100644
--- a/drivers/usb/eth/usb_ether.c
+++ b/drivers/usb/eth/usb_ether.c
@@ -103,9 +103,7 @@  static void probe_valid_drivers(struct usb_device *dev)
 			 * relies on it
 			 */
 			eth_register(eth);
-			if (eth_write_hwaddr(eth, "usbeth",
-					usb_max_eth_dev - 1))
-				puts("Warning: failed to set MAC address\n");
+			eth_write_hwaddr(eth, "usbeth", usb_max_eth_dev - 1);
 			break;
 			}
 		}
diff --git a/net/eth.c b/net/eth.c
index 4280d6d..1b6ca86 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -187,33 +187,42 @@  static void eth_current_changed(void)
 int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 		   int eth_number)
 {
-	unsigned char env_enetaddr[6];
-	int ret = 0;
+	unsigned char envaddr[6];
 
-	if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
-		return -1;
+	/*
+	 * If the environment does not have a valid MAC address, then just
+	 * try to use whatever we were able to load from the chipset.
+	 */
+	if (!eth_getenv_enetaddr_by_index(base_name, eth_number, envaddr))
+		goto write_addr;
+	if (!is_valid_ether_addr(envaddr)) {
+		printf(" [bad env MAC: %pM]", envaddr);
+		goto write_addr;
+	}
 
-	if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
-		if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
-			memcmp(dev->enetaddr, env_enetaddr, 6)) {
-			printf("\nWarning: %s MAC addresses don't match:\n",
-				dev->name);
-			printf("Address in SROM is         %pM\n",
-				dev->enetaddr);
-			printf("Address in environment is  %pM\n",
-				env_enetaddr);
-		}
+	/* If the ROM and environment match, we should just program it */
+	if (!memcmp(dev->enetaddr, envaddr, 6))
+		goto write_addr;
 
-		memcpy(dev->enetaddr, env_enetaddr, 6);
-	}
+	/* Log the discrepancy and let the environment override the ROM */
+	if (is_valid_ether_addr(dev->enetaddr))
+		printf(" [MAC mismatch: ROM=%pM, env=%pM]",
+				dev->enetaddr, envaddr);
+	memcpy(dev->enetaddr, envaddr, 6);
 
-	if (dev->write_hwaddr &&
-		!eth_mac_skip(eth_number) &&
-		is_valid_ether_addr(dev->enetaddr)) {
-		ret = dev->write_hwaddr(dev);
+write_addr:
+	if (!is_valid_ether_addr(dev->enetaddr)) {
+		printf(" [invalid MAC: %pM]", dev->enetaddr);
+		return -1;
 	}
-
-	return ret;
+	if (dev->write_hwaddr && !eth_mac_skip(eth_number)) {
+		int ret = dev->write_hwaddr(dev);
+		if (ret) {
+			printf(" [could not set MAC: %pM]", dev->enetaddr);
+			return ret;
+		}
+	}
+	return 0;
 }
 
 int eth_register(struct eth_device *dev)
@@ -294,8 +303,7 @@  int eth_initialize(bd_t *bis)
 			if (strchr(dev->name, ' '))
 				puts("\nWarning: eth device name has a space!\n");
 
-			if (eth_write_hwaddr(dev, "eth", eth_number))
-				puts("\nWarning: failed to set MAC address\n");
+			eth_write_hwaddr(dev, "eth", eth_number);
 
 			eth_number++;
 			dev = dev->next;