Skip to contentSkip to conversation

net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

PATCH New Eric Miao 6 participants 11 messages Aug 17, 2011 – Jan 16, 2012 Networking

lore
Share & tools
lore

Eric Miao

author
#0 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

---
 net/eth.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
net/eth.c +1 -2
index a34fe59..c4fbe11 100644
@@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 	unsigned char env_enetaddr[6];
 	int ret = 0;
 
-	if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
-		return -1;
+	eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
 
 	if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
 		if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
Signature · 1 line
1.7.4.1
#1 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

Quoted 8 lines · Eric Miao

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Hi Eric,

I found this in u-boot-usb although the commit hash I saw was 7616e7850.

Quoted 17 lines

---
?net/eth.c | ? ?3 +--
?1 files changed, 1 insertions(+), 2 deletions(-)

net/eth.c +1 -2
index a34fe59..c4fbe11 100644
@@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 ? ? ? ?unsigned char env_enetaddr[6];
 ? ? ? ?int ret = 0;
 
- ? ? ? if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
- ? ? ? ? ? ? ? return -1;
+ ? ? ? eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
 
 ? ? ? ?if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
Quoted 1 line

? ? ? ? ? ? ? ?if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&

Regards,
Simon

Quoted 4 lines

--
1.7.4.1

#2 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

Quoted 26 lines · Simon Glass
Quoted 8 lines · Eric Miao

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Hi Eric,

I found this in u-boot-usb although the commit hash I saw was 7616e7850.

Quoted 12 lines
---
 net/eth.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
net/eth.c +0 -0
index a34fe59..c4fbe11 100644
@@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
       unsigned char env_enetaddr[6];
       int ret = 0;
 
Quoted 8 lines

net/eth.c int eth_write_hwaddr(struct eth_device *dev, const char *base_name, · line 201

Quoted 6 lines
net/eth.c +1 -2
-       if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
-               return -1;
+       eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
 
       if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
               if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&

Ping.

Anybody likes to apply this? Or any new comments?

Many thanks and best regards

Dirk

P.S.: I add Wolfgang as he seems to have done the last commits to net/eth.c.

#3 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

Quoted 46 lines · Dirk Behme
Quoted 36 lines · Simon Glass
Quoted 9 lines · Eric Miao

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Hi Eric,

I found this in u-boot-usb although the commit hash I saw was 7616e7850.

Quoted 19 lines

---
?net/eth.c | ? ?3 +--
?1 files changed, 1 insertions(+), 2 deletions(-)

net/eth.c +1 -2
index a34fe59..c4fbe11 100644
@@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const
 char *base_name,
 ? ? ? unsigned char env_enetaddr[6];
 ? ? ? int ret = 0;
 
- ? ? ? if (!eth_getenv_enetaddr_by_index(base_name, eth_number,
 env_enetaddr))
- ? ? ? ? ? ? ? return -1;
+ ? ? ? eth_getenv_enetaddr_by_index(base_name, eth_number,
 env_enetaddr);
Quoted 5 lines
Quoted 2 lines

? ? ? if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { ? ? ? ? ? ? ? if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&

Ping.

Quoted 7 lines

Anybody likes to apply this? Or any new comments?

Many thanks and best regards

Dirk

P.S.: I add Wolfgang as he seems to have done the last commits to net/eth.c.

Maybe Stefan might pick it up through the staging tree?

Regards,
Simon

#4 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

Quoted 44 lines · On Thursday 12 January 2012 06:12:19 Simon Glass wrote:
Quoted 43 lines · Dirk Behme
Quoted 33 lines · Simon Glass
Quoted 8 lines · Eric Miao

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Hi Eric,

I found this in u-boot-usb although the commit hash I saw was 7616e7850.

Quoted 19 lines
---
 net/eth.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
net/eth.c +1 -2
index a34fe59..c4fbe11 100644
@@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const
 char *base_name,
      unsigned char env_enetaddr[6];
      int ret = 0;
 
-       if (!eth_getenv_enetaddr_by_index(base_name, eth_number,
 env_enetaddr))
-               return -1;
+       eth_getenv_enetaddr_by_index(base_name, eth_number,
 env_enetaddr);
Quoted 4 lines
Quoted 2 lines
      if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
              if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&

Ping.

Anybody likes to apply this? Or any new comments?

Quoted 8 lines
Quoted 6 lines

Many thanks and best regards

Dirk

P.S.: I add Wolfgang as he seems to have done the last commits to net/eth.c.

Maybe Stefan might pick it up through the staging tree?

Okay, let's take a look...

Best regards,
Stefan

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

#5 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

In reply to Eric Miao
Quoted 6 lines · On Wednesday 17 August 2011 07:33:44 Eric Miao wrote:

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Applied to u-boot-staging/sr at denx.de. Thanks.

Best regards,
Stefan

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

#6 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

In reply to Eric Miao

Dear Eric Miao,

Quoted 6 lines · In message <[email protected]> you wrote:

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

I'm not going to pull that patch.

Please explain which actual problem you're trying to fix.

Also, fix the commit ID - the one you list above does not exist in mainline.

Best regards,

Wolfgang Denk

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A direct quote from the Boss: "We passed over a lot of good people to
get the ones we hired."
author
#7 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

Quoted 7 lines · Wolfgang Denk

Dear Eric Miao,

Quoted 4 lines · In message <[email protected]> you wrote:

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Quoted 6 lines
Quoted 2 lines

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

I'm not going to pull that patch.

Please explain which actual problem you're trying to fix.

Hi Wolfgang,

The problem was with the current code, if there is no eth<n>addr in the environment variables, eth_write_hwaddr() will fail immediately, and it's going to use the default reset MAC address in the FEC controller which is likely all 0.

To make it worse, the current kernel driver actually depends on the MAC address registers being programmed correctly.

If traced back to the last commit to this file, which added this eth_getenv_enetaddr_by_index(), that commit actually modified the original behavior of this eth_write_hwaddr() function.

author
#8 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

Quoted 10 lines · Eric Miao
Quoted 9 lines · Wolfgang Denk

Dear Eric Miao,

Quoted 6 lines · In message <[email protected]> you wrote:

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Checked with git.denx.de/u-boot.git, the original commit hash is

7616e7850804c7c69e0a22c179dfcba9e8f3f587

Please note the behavior modification to eth_write_hwaddr() when it's excerpted from the original eth_initialize().

#9 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

In reply to Wolfgang Denk
Quoted 7 lines · Wolfgang Denk

Dear Eric Miao,

Quoted 4 lines · In message <[email protected]> you wrote:

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it fails, fall back to use dev->enetaddr, which could be filled up by the ethernet device driver.

Quoted 7 lines
Quoted 2 lines

Actually, this is the original behavior, and was later changed by commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

I'm not going to pull that patch.

Please explain which actual problem you're trying to fix.

For u-boot for an overo based product that uses the SMSC911x ethernet chip (with MAC address set via EEPROM connected to the SMSC911x chip) I have a local patch to prevent u-boot from printing:

Warning: failed to set MAC address

The MAC address is still OK and we basically removed the code that led to the message to reduce customer confusion :)

Hopefully, the patch people are talking about here is a proper fix to this problem.

Philip

Quoted 7 lines

Also, fix the commit ID - the one you list above does not exist in mainline.

Best regards,

Wolfgang Denk

#10 lore raw

PATCH 1/1 net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

Dear Philip Balister,

Quoted 10 lines · In message <[email protected]> you wrote:
Quoted 4 lines

I'm not going to pull that patch.

Please explain which actual problem you're trying to fix.

For u-boot for an overo based product that uses the SMSC911x ethernet chip (with MAC address set via EEPROM connected to the SMSC911x chip) I have a local patch to prevent u-boot from printing:

Warning: failed to set MAC address

Something went wrong here. Either you should not run the operation that failed, or you should find out why it failed and fixe the cause of that.

Quoted 2 lines

The MAC address is still OK and we basically removed the code that led to the message to reduce customer confusion :)

Papering over errors or warnings is rarely a good thing. We should understand what happened and fix the original cause of the problem instead.

Best regards,

Wolfgang Denk

Signature · 8 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I wrote my name at the top of the page. I wrote down  the  number  of
the  question  ``1''.  After much reflection I put a bracket round it
thus ``(1)''. But thereafter I could not think of anything  connected
with it that was either relevant or true.
                        - Sir Winston Churchill _My Early Life_ ch. 2