Patchwork [U-Boot] net/eth.c: Avoid Warning when ethxaddr is not set

login
register
mail settings
Submitter Gwenhael Goavec-Merou
Date March 3, 2012, 1:10 p.m.
Message ID <1330780211-90427-1-git-send-email-gwenhael.goavec-merou@armadeus.com>
Download mbox | patch
Permalink /patch/144422/
State Rejected
Delegated to: Joe Hershberger
Headers show

Comments

Gwenhael Goavec-Merou - March 3, 2012, 1:10 p.m.
The return value for eth_getenv_enetaddr_by_index produces 
a Warning when ethXaddr is not set (env_enetaddr is equal 
to 00:00:00:00:00:00).

The validity of the mac address is tested later in this 
function to avoid to write an erroneous address.

The test of the function's return is consequently useless 
and adds a warning.

Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@armadeus.com>
---
 net/eth.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Wolfgang Denk - March 3, 2012, 2:44 p.m.
Dear Gwenhael Goavec-Merou,

In message <1330780211-90427-1-git-send-email-gwenhael.goavec-merou@armadeus.com> you wrote:
> The return value for eth_getenv_enetaddr_by_index produces 
> a Warning when ethXaddr is not set (env_enetaddr is equal 
> to 00:00:00:00:00:00).
> 
> The validity of the mac address is tested later in this 
> function to avoid to write an erroneous address.
> 
> The test of the function's return is consequently useless 
> and adds a warning.

Which use case do you have in mind?  Why do you think this warning is
bad?  I tend to consider it helpful instead.

Best regards,

Wolfgang Denk
Gwenhael Goavec-Merou - March 3, 2012, 2:57 p.m.
On Sat, 03 Mar 2012 15:44:48 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Gwenhael Goavec-Merou,
> 
> In message <1330780211-90427-1-git-send-email-gwenhael.goavec-merou@armadeus.com> you wrote:
> > The return value for eth_getenv_enetaddr_by_index produces 
> > a Warning when ethXaddr is not set (env_enetaddr is equal 
> > to 00:00:00:00:00:00).
> > 
> > The validity of the mac address is tested later in this 
> > function to avoid to write an erroneous address.
> > 
> > The test of the function's return is consequently useless 
> > and adds a warning.
> 
> Which use case do you have in mind?  Why do you think this warning is
> bad?  I tend to consider it helpful instead.
> 

For boards with mac address stored on an eeprom for example, the variable
ethaddr is not needed. But with this test, a warning is always shown, without
any reason.

Best regards,
Gwenhael Goavec-Merou

>
>
> Best regards,
> 
> Wolfgang Denk
>
Wolfgang Denk - March 3, 2012, 3:11 p.m.
Dear "gwenhael.goavec",

In message <20120303155719.3ae222cc@trabucayre.com> you wrote:
>
> > Which use case do you have in mind?  Why do you think this warning is
> > bad?  I tend to consider it helpful instead.
> 
> For boards with mac address stored on an eeprom for example, the variable
> ethaddr is not needed. But with this test, a warning is always shown, without
> any reason.

The environment variable is always mandatory.  If it does not exist,
or if it's value is different from the one stored somewhere else, a
warning shall be issued (and the environment value takes precedence).

So I think current behaviour is actually correct.

Best regards,

Wolfgang Denk
Simon Glass - March 3, 2012, 4:19 p.m.
Hi,

On Sat, Mar 3, 2012 at 7:11 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear "gwenhael.goavec",
>
> In message <20120303155719.3ae222cc@trabucayre.com> you wrote:
>>
>> > Which use case do you have in mind?  Why do you think this warning is
>> > bad?  I tend to consider it helpful instead.
>>
>> For boards with mac address stored on an eeprom for example, the variable
>> ethaddr is not needed. But with this test, a warning is always shown, without
>> any reason.
>
> The environment variable is always mandatory.  If it does not exist,
> or if it's value is different from the one stored somewhere else, a
> warning shall be issued (and the environment value takes precedence).
>
> So I think current behaviour is actually correct.
>

The code is not ideal - I suspect with a bit of improvement we can do
the right thing in both cases. See the discussion started here:

http://lists.denx.de/pipermail/u-boot/2012-February/117783.html

I think this might be the third patch to do the same thing, so perhaps
one of these people might like to take on the clean-up?

Regards,
Simon

> Best regards,
>
> Wolfgang Denk
>
> --
> 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@denx.de
> In the beginning, there was nothing, which exploded.
>                                - Terry Pratchett, _Lords and Ladies_

Patch

diff --git a/net/eth.c b/net/eth.c
index b4b9b43..1e61aed 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -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;
+	(void)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) &&