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

PATCH Superseded Dirk Behme 2 participants 7 messages Jan 19, 2012 – Feb 10, 2012 Networking

lore

Share & tools

lore

Dirk Behme

author

2012-01-19 08:56 UTC #0 lore raw

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

From: Eric Miao <[email protected]>

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:

With the current code, introduced with below commit, eth_write_hwaddr() will fail immediately if there is no eth<n>addr in the environment variables.

However, e.g. for an overo based product that uses the SMSC911x ethernet chip (with the MAC address set via EEPROM connected to the SMSC911x chip), the MAC address is still OK.

On mx28 boards that are depending on the OCOTP bits to set the MAC address (like the Denx m28 board), the OCOTP bits should be used instead of failing on the environment variables.

Actually, this was the original behavior, and was later changed by commit 7616e7850804c7c69e0a22c179dfcba9e8f3f587.

Signed-off-by Eric Miao <[email protected]>

Acked-by Simon Glass <[email protected]>

Acked-by Dirk Behme <[email protected]>

CC Stefan Roese <[email protected]>

CC Eric Miao <[email protected]>

CC Wolfgang Denk <[email protected]>

CC Philip Balister <[email protected]>

CC Zach Sadecki <[email protected]>

---
v2: Correct the referenced commit ID and update the commit message.
No functional change at the code itself.

Note: This resend is based on my understanding from

http://lists.denx.de/pipermail/u-boot/2012-January/116118.html

Please let Eric and me know if I missed anything there.

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

net/eth.c +1 -2

index b4b9b43..451568f 100644
@@ -175,8 +175,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.0.4

Simon Glass

2012-01-23 07:31 UTC #1 lore raw

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

Hi,

Quoted 32 lines · Dirk Behme

From: Eric Miao <[email protected]>
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:
With the current code, introduced with below commit, eth_write_hwaddr() will fail immediately if there is no eth<n>addr in the environment variables.
However, e.g. for an overo based product that uses the SMSC911x ethernet chip (with the MAC address set via EEPROM connected to the SMSC911x chip), the MAC address is still OK.
On mx28 boards that are depending on the OCOTP bits to set the MAC address (like the Denx m28 board), the OCOTP bits should be used instead of failing on the environment variables.
Actually, this was the original behavior, and was later changed by commit 7616e7850804c7c69e0a22c179dfcba9e8f3f587.
Signed-off-by Eric Miao <[email protected]>
Acked-by Simon Glass <[email protected]>
Acked-by Dirk Behme <[email protected]>
CC Stefan Roese <[email protected]>
CC Eric Miao <[email protected]>
CC Wolfgang Denk <[email protected]>
CC Philip Balister <[email protected]>
CC Zach Sadecki <[email protected]>
---
v2: Correct the referenced commit ID and update the commit message. ? ?No functional change at the code itself.

Quoted 5 lines

Note: This resend is based on my understanding from
? ? ?http://lists.denx.de/pipermail/u-boot/2012-January/116118.html
? ? ?Please let Eric and me know if I missed anything there.

I don't think you have missed anything and I have already acked this. But I want to start a related discussion.

The code structure does bug me a bit - I think it is too confusing. eth_getenv_enetaddr() returns an error if there is no environment variable set or if the address it gets from the environment variable is invalid. We should probably not conflate those two. The first is ok here, but the second isn't, I think.

What if the driver has no write_hwaddr method? Do we silently ignore the environment variable value?

Why use memcmp() against env_enetaddr when the function we just called returns an error that tells us whether it is supposed to be valid (the error return your patch squashes)?

We set the hwaddr by writing directly into the dev->enet_addr field and then calling write_hwaddr() if it exists. Maybe that is ok - is the lack of write_hwaddr() an indication that the driver does MAC address handling on the fly, or just that it can't set the MAC address at all?

Overall I feel that eth_write_hwaddr() should return success or failure, confident in its determination that there is either a valid MAC address or there is not. The message you are seeing is I suppose an indication that it thinks there is a problem, when in fact none exists in this case. At the moment it feels fragile.

I wonder whether a little refactor here would be best?

That said, your patch restores the original behaviour, hiding the problem which isn't actually a problem in this case, and which we don't want to report. So it is better than the status quo. Perhaps a little comment as to why the return value doesn't matter?

Regards,
Simon

Quoted 25 lines

?net/eth.c | ? ?3 +--
?1 files changed, 1 insertions(+), 2 deletions(-)
net/eth.c +1 -2
index b4b9b43..451568f 100644
@@ -175,8 +175,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) && --
1.7.0.4
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Dirk Behme

author

2012-01-23 08:28 UTC #2 lore raw

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

Quoted 69 lines · Simon Glass

Hi,
Quoted 32 lines · Dirk Behme
From: Eric Miao <[email protected]>
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:
With the current code, introduced with below commit, eth_write_hwaddr() will fail immediately if there is no eth<n>addr in the environment variables.
However, e.g. for an overo based product that uses the SMSC911x ethernet chip (with the MAC address set via EEPROM connected to the SMSC911x chip), the MAC address is still OK.
On mx28 boards that are depending on the OCOTP bits to set the MAC address (like the Denx m28 board), the OCOTP bits should be used instead of failing on the environment variables.
Actually, this was the original behavior, and was later changed by commit 7616e7850804c7c69e0a22c179dfcba9e8f3f587.
Signed-off-by Eric Miao <[email protected]>
Acked-by Simon Glass <[email protected]>
Acked-by Dirk Behme <[email protected]>
CC Stefan Roese <[email protected]>
CC Eric Miao <[email protected]>
CC Wolfgang Denk <[email protected]>
CC Philip Balister <[email protected]>
CC Zach Sadecki <[email protected]>
---
v2: Correct the referenced commit ID and update the commit message.
No functional change at the code itself.
Quoted 5 lines
Note: This resend is based on my understanding from
http://lists.denx.de/pipermail/u-boot/2012-January/116118.html
Please let Eric and me know if I missed anything there.
I don't think you have missed anything and I have already acked this. But I want to start a related discussion.
The code structure does bug me a bit - I think it is too confusing. eth_getenv_enetaddr() returns an error if there is no environment variable set or if the address it gets from the environment variable is invalid. We should probably not conflate those two. The first is ok here, but the second isn't, I think.
What if the driver has no write_hwaddr method? Do we silently ignore the environment variable value?
Why use memcmp() against env_enetaddr when the function we just called returns an error that tells us whether it is supposed to be valid (the error return your patch squashes)?
We set the hwaddr by writing directly into the dev->enet_addr field and then calling write_hwaddr() if it exists. Maybe that is ok - is the lack of write_hwaddr() an indication that the driver does MAC address handling on the fly, or just that it can't set the MAC address at all?
Overall I feel that eth_write_hwaddr() should return success or failure, confident in its determination that there is either a valid MAC address or there is not. The message you are seeing is I suppose an indication that it thinks there is a problem, when in fact none exists in this case. At the moment it feels fragile.

Quoted 5 lines

I wonder whether a little refactor here would be best?
That said, your patch restores the original behaviour, hiding the problem which isn't actually a problem in this case, and which we don't want to report. So it is better than the status quo.

Ok, thanks.

I'm not an expert for this code, nor is the patch from me. It's from Eric ;) I just try to help to mainline all the stuff we have collected for i.MX6.

Therefore I wonder if it would be possible to split this into two steps:

a) Improve the status quo by applying this patch
b) In parallel discuss how to refactor and improve this code as you describe above

It's my feeling that with (a) we still have a chance to improve v2012.03. But I doubt that (b) would make it into v2012.03.

Best regards

Dirk

Simon Glass

2012-01-23 16:17 UTC #3 lore raw

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

Hi Dirk,

Quoted 6 lines · Dirk Behme

Quoted 4 lines · Simon Glass
Hi,
On Thu, Jan 19, 2012 at 12:56 AM, Dirk Behme <[email protected]>

Quoted 9 lines · wrote:

Quoted 9 lines
Quoted 9 lines
From: Eric Miao <[email protected]>
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:
With the current code, introduced with below commit, eth_write_hwaddr() will fail immediately if there is no eth<n>addr in the environment

variables.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
However, e.g. for an overo based product that uses the SMSC911x ethernet chip (with the MAC address set via EEPROM connected to the SMSC911x

chip),

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
the MAC address is still OK.
On mx28 boards that are depending on the OCOTP bits to set the MAC

address

Quoted 55 lines

Quoted 55 lines
Quoted 18 lines
(like the Denx m28 board), the OCOTP bits should be used instead of failing on the environment variables.
Actually, this was the original behavior, and was later changed by commit 7616e7850804c7c69e0a22c179dfcba9e8f3f587.
Signed-off-by Eric Miao <[email protected]>
Acked-by Simon Glass <[email protected]>
Acked-by Dirk Behme <[email protected]>
CC Stefan Roese <[email protected]>
CC Eric Miao <[email protected]>
CC Wolfgang Denk <[email protected]>
CC Philip Balister <[email protected]>
CC Zach Sadecki <[email protected]>
---
v2: Correct the referenced commit ID and update the commit message.
No functional change at the code itself.
Quoted 5 lines
Note: This resend is based on my understanding from
http://lists.denx.de/pipermail/u-boot/2012-January/116118.html
Please let Eric and me know if I missed anything there.
I don't think you have missed anything and I have already acked this. But I want to start a related discussion.
The code structure does bug me a bit - I think it is too confusing. eth_getenv_enetaddr() returns an error if there is no environment variable set or if the address it gets from the environment variable is invalid. We should probably not conflate those two. The first is ok here, but the second isn't, I think.
What if the driver has no write_hwaddr method? Do we silently ignore the environment variable value?
Why use memcmp() against env_enetaddr when the function we just called returns an error that tells us whether it is supposed to be valid (the error return your patch squashes)?
We set the hwaddr by writing directly into the dev->enet_addr field and then calling write_hwaddr() if it exists. Maybe that is ok - is the lack of write_hwaddr() an indication that the driver does MAC address handling on the fly, or just that it can't set the MAC address at all?
Overall I feel that eth_write_hwaddr() should return success or failure, confident in its determination that there is either a valid MAC address or there is not. The message you are seeing is I suppose an indication that it thinks there is a problem, when in fact none exists in this case. At the moment it feels fragile.
I wonder whether a little refactor here would be best?

Quoted 8 lines

Quoted 3 lines
That said, your patch restores the original behaviour, hiding the problem which isn't actually a problem in this case, and which we don't want to report. So it is better than the status quo.
Ok, thanks.
I'm not an expert for this code, nor is the patch from me. It's from Eric

;) I just try to help to mainline all the stuff we have collected for i.MX6.

Quoted 5 lines

Therefore I wonder if it would be possible to split this into two steps:
a) Improve the status quo by applying this patch
b) In parallel discuss how to refactor and improve this code as you

describe above

Quoted 2 lines

It's my feeling that with (a) we still have a chance to improve v2012.03.

But I doubt that (b) would make it into v2012.03.

Yes agreed, it is a separate discussion. I added Wolfgang on cc to see what he thinks.

Regards,
Simon

Quoted 5 lines

Best regards
Dirk

Dirk Behme

author

2012-02-08 07:13 UTC #4 lore raw

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

Quoted 102 lines · Simon Glass

Hi Dirk,
Quoted 6 lines · On Jan 23, 2012 12:30 AM, "Dirk Behme" <[email protected]…
Quoted 4 lines · Simon Glass
Hi,
On Thu, Jan 19, 2012 at 12:56 AM, Dirk Behme
Quoted 9 lines · <dirk.behme at de.bosch.com <mailto:[email protected]>> wrote:
Quoted 9 lines
Quoted 9 lines
From: Eric Miao <eric.miao at linaro.org <mailto:[email protected]>>
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:
With the current code, introduced with below commit, eth_write_hwaddr() will fail immediately if there is no eth<n>addr in the environment
variables.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
However, e.g. for an overo based product that uses the SMSC911x
ethernet
Quoted 1 line
Quoted 1 line
Quoted 1 line
chip (with the MAC address set via EEPROM connected to the SMSC911x
chip),
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
the MAC address is still OK.
On mx28 boards that are depending on the OCOTP bits to set the MAC
address
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
(like the Denx m28 board), the OCOTP bits should be used instead of failing on the environment variables.
Actually, this was the original behavior, and was later changed by commit 7616e7850804c7c69e0a22c179dfcba9e8f3f587.
Signed-off-by Eric Miao <[email protected]
<mailto:[email protected]>>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Acked-by Simon Glass <sjg at chromium.org <mailto:[email protected]>>
Acked-by Dirk Behme <[email protected]
<mailto:[email protected]>>
Quoted 46 lines
Quoted 46 lines
Quoted 9 lines
CC Stefan Roese <sr at denx.de <mailto:[email protected]>>
CC Eric Miao <eric.miao at linaro.org <mailto:[email protected]>>
CC Wolfgang Denk <wd at denx.de <mailto:[email protected]>>
CC Philip Balister <philip at balister.org <mailto:[email protected]>>
CC Zach Sadecki <zach at itwatchdogs.com <mailto:[email protected]>>
---
v2: Correct the referenced commit ID and update the commit message.
No functional change at the code itself.
Quoted 5 lines
Note: This resend is based on my understanding from
http://lists.denx.de/pipermail/u-boot/2012-January/116118.html
Please let Eric and me know if I missed anything there.
I don't think you have missed anything and I have already acked this. But I want to start a related discussion.
The code structure does bug me a bit - I think it is too confusing. eth_getenv_enetaddr() returns an error if there is no environment variable set or if the address it gets from the environment variable is invalid. We should probably not conflate those two. The first is ok here, but the second isn't, I think.
What if the driver has no write_hwaddr method? Do we silently ignore the environment variable value?
Why use memcmp() against env_enetaddr when the function we just called returns an error that tells us whether it is supposed to be valid (the error return your patch squashes)?
We set the hwaddr by writing directly into the dev->enet_addr field and then calling write_hwaddr() if it exists. Maybe that is ok - is the lack of write_hwaddr() an indication that the driver does MAC address handling on the fly, or just that it can't set the MAC address at all?
Overall I feel that eth_write_hwaddr() should return success or failure, confident in its determination that there is either a valid MAC address or there is not. The message you are seeing is I suppose an indication that it thinks there is a problem, when in fact none exists in this case. At the moment it feels fragile.
I wonder whether a little refactor here would be best?
Quoted 8 lines
Quoted 3 lines
That said, your patch restores the original behaviour, hiding the problem which isn't actually a problem in this case, and which we don't want to report. So it is better than the status quo.
Ok, thanks.
I'm not an expert for this code, nor is the patch from me. It's from
Eric ;) I just try to help to mainline all the stuff we have collected for i.MX6.
Quoted 5 lines
Therefore I wonder if it would be possible to split this into two steps:
a) Improve the status quo by applying this patch
b) In parallel discuss how to refactor and improve this code as you

Quoted 7 lines

describe above
Quoted 2 lines
It's my feeling that with (a) we still have a chance to improve
v2012.03. But I doubt that (b) would make it into v2012.03.
Yes agreed, it is a separate discussion. I added Wolfgang on cc to see what he thinks.

Any news to this?

Many thanks

Dirk

Simon Glass

2012-02-09 18:25 UTC #5 lore raw

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

Hi Dirk,

Quoted 115 lines · Dirk Behme

Quoted 114 lines · Simon Glass
Hi Dirk,
On Jan 23, 2012 12:30 AM, "Dirk Behme" <[email protected] <mailto:[email protected]>> wrote:
?>
?> On 23.01.2012 08:31, Simon Glass wrote:
?>>
?>> Hi,
?>>
?>> On Thu, Jan 19, 2012 at 12:56 AM, Dirk Behme <[email protected] <mailto:[email protected]>> wrote:
?>>>
?>>> From: Eric Miao <eric.miao at linaro.org <mailto:[email protected]>>
?>>>
?>>> 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:
?>>>
?>>> With the current code, introduced with below commit, eth_write_hwaddr()
?>>> will fail immediately if there is no eth<n>addr in the environment variables.
?>>>
?>>> However, e.g. for an overo based product that uses the SMSC911x ethernet
?>>> chip (with the MAC address set via EEPROM connected to the SMSC911x chip),
?>>> the MAC address is still OK.
?>>>
?>>> On mx28 boards that are depending on the OCOTP bits to set the MAC address
?>>> (like the Denx m28 board), the OCOTP bits should be used instead of ?>>> failing on the environment variables.
?>>>
?>>> Actually, this was the original behavior, and was later changed by ?>>> commit 7616e7850804c7c69e0a22c179dfcba9e8f3f587.
?>>>
?>>> Signed-off-by: Eric Miao <[email protected] <mailto:[email protected]>>
?>>> Acked-by: Simon Glass <sjg at chromium.org <mailto:[email protected]>> ?>>> Acked-by: Dirk Behme <[email protected] <mailto:[email protected]>>
?>>> CC: Stefan Roese <sr at denx.de <mailto:[email protected]>>
?>>> CC: Eric Miao <eric.miao at linaro.org <mailto:[email protected]>> ?>>> CC: Wolfgang Denk <wd at denx.de <mailto:[email protected]>>
?>>> CC: Philip Balister <[email protected] <mailto:[email protected]>>
?>>> CC: Zach Sadecki <zach at itwatchdogs.com <mailto:[email protected]>>
?>>> ---
?>>> v2: Correct the referenced commit ID and update the commit message. ?>>> ? No functional change at the code itself.
?>>>
?>>> Note: This resend is based on my understanding from
?>>>
?>>> ? ? http://lists.denx.de/pipermail/u-boot/2012-January/116118.html
?>>>
?>>> ? ? Please let Eric and me know if I missed anything there. ?>>
?>>
?>> I don't think you have missed anything and I have already acked this. ?>> But I want to start a related discussion.
?>>
?>> The code structure does bug me a bit - I think it is too confusing. ?>> eth_getenv_enetaddr() returns an error if there is no environment ?>> variable set or if the address it gets from the environment variable ?>> is invalid. We should probably not conflate those two. The first is ok ?>> here, but the second isn't, I think.
?>>
?>> What if the driver has no write_hwaddr method? Do we silently ignore ?>> the environment variable value?
?>>
?>> Why use memcmp() against env_enetaddr when the function we just called ?>> returns an error that tells us whether it is supposed to be valid (the ?>> error return your patch squashes)?
?>>
?>> We set the hwaddr by writing directly into the dev->enet_addr field ?>> and then calling write_hwaddr() if it exists. Maybe that is ok - is ?>> the lack of write_hwaddr() an indication that the driver does MAC ?>> address handling on the fly, or just that it can't set the MAC address ?>> at all?
?>>
?>> Overall I feel that eth_write_hwaddr() should return success or ?>> failure, confident in its determination that there is either a valid ?>> MAC address or there is not. The message you are seeing is I suppose ?>> an indication that it thinks there is a problem, when in fact none ?>> exists in this case. At the moment it feels fragile.
?>>
?>> I wonder whether a little refactor here would be best?
?>>
?>> That said, your patch restores the original behaviour, hiding the ?>> problem which isn't actually a problem in this case, and which we ?>> don't want to report. So it is better than the status quo. ?>
?>
?> Ok, thanks.
?>
?> I'm not an expert for this code, nor is the patch from me. It's from Eric ;) I just try to help to mainline all the stuff we have collected for i.MX6.
?>
?> Therefore I wonder if it would be possible to split this into two steps:
?>
?> a) Improve the status quo by applying this patch
?> b) In parallel discuss how to refactor and improve this code as you describe above
?>
?> It's my feeling that with (a) we still have a chance to improve v2012.03. But I doubt that (b) would make it into v2012.03.

Quoted 5 lines

Quoted 2 lines
Yes agreed, it is a separate discussion. I added Wolfgang on cc to see what he thinks.
Any news to this?

Already acked by me. Are you going to start a separate discussion on how to clean up this code? If so please cc me.

Regards,
Simon

Quoted 5 lines

Many thanks
Dirk

Dirk Behme

author

2012-02-10 07:06 UTC #6 lore raw

[U-Boot] Refactoring eth_write_hwaddr() and friends (was: Re: [PATCH v2] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back)

In reply to Simon Glass

On 23.01.2012 08:31, Simon Glass wrote:
...

Quoted 28 lines

Quoted 5 lines
Note: This resend is based on my understanding from
http://lists.denx.de/pipermail/u-boot/2012-January/116118.html
Please let Eric and me know if I missed anything there.
I don't think you have missed anything and I have already acked this. But I want to start a related discussion.
The code structure does bug me a bit - I think it is too confusing. eth_getenv_enetaddr() returns an error if there is no environment variable set or if the address it gets from the environment variable is invalid. We should probably not conflate those two. The first is ok here, but the second isn't, I think.
What if the driver has no write_hwaddr method? Do we silently ignore the environment variable value?
Why use memcmp() against env_enetaddr when the function we just called returns an error that tells us whether it is supposed to be valid (the error return your patch squashes)?
We set the hwaddr by writing directly into the dev->enet_addr field and then calling write_hwaddr() if it exists. Maybe that is ok - is the lack of write_hwaddr() an indication that the driver does MAC address handling on the fly, or just that it can't set the MAC address at all?

Quoted 7 lines

Overall I feel that eth_write_hwaddr() should return success or failure, confident in its determination that there is either a valid MAC address or there is not. The message you are seeing is I suppose an indication that it thinks there is a problem, when in fact none exists in this case. At the moment it feels fragile.
I wonder whether a little refactor here would be best?

While discussing about [1] we found that this is only a short term fix (which should go into 2012.03 [2]) and that we should discuss about a more general clean up of eth_write_hwaddr() and friends:

http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=net/eth.c;h=b4b9b4341fdecb25869a07cc8dbe9deefd6452bd;hb=HEAD#l172

See Simon's ideas above.

Comments? Opinions?

Best regards

Dirk

[1] http://lists.denx.de/pipermail/u-boot/2012-January/116224.html

[2] http://lists.denx.de/pipermail/u-boot/2012-January/116436.html