[U-Boot-Users] What if eth_init() fails?

[U-Boot-Users] What if eth_init() fails?

[Upakul Barkakaty]

Hi all,

I observed that when the Ethernet initialization fails, it is not properly
halting the operation and exiting. On walking through the Networking files,
I saw that the eth_init() in eth.c either returns a 0 or 1. Now, in the
NetLoop() in net.c file, from where the eth_init() gets called, checks the
condition 
if (eth_init(bd) < 0) 
{
     eth_halt();
     return(-1);
}
which is thus never true.

Thus the network operation, never exits gracefully, if Ethernet init fails.

Any of you, have any clues about this??
-- 
View this message in context: http://www.nabble.com/What-if-eth_init%28%29-fails--tf4802433.html#a13740586
Sent from the Uboot - Users mailing list archive@Nabble.com.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20071113/c07f5f6b/attachment.htm 

[U-Boot-Users] What if eth_init() fails?

[Detlev Zundel]

Hi Upakul,

> Hi all, I observed that when the Ethernet initialization fails, it is not
> properly halting the operation and exiting. On walking through the Networking
> files, I saw that the eth_init() in eth.c either returns a 0 or 1. Now, in the
> NetLoop() in net.c file, from where the eth_init() gets called, checks the
> condition if (eth_init(bd) < 0) { eth_halt(); return(-1); } which is thus never
> true. Thus the network operation, never exits gracefully, if Ethernet init
> fails. Any of you, have any clues about this??

I think your analysis is right and thus the code in NetLoop should be
fixed.  Feel free to send a patch and I am pretty sure Ben Warren will
pick it up into the u-boot-net repo.

Cheers
  Detlev

-- 
Two  monks went  fishing in an  electron river.  The first monk drew out his
network, and out flopped a hacker.  The second monk cried, "The poor hacker!
How can  it live outside of the  network?"   The first monk said,  "When you
have learned to live outside the network, then you will know."
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot-Users] What if eth_init() fails?

[Upakul Barkakaty]

Hi Detlev/Ben,

Thanks for the replies. I am attaching herewith, the patch which I suppose
should fix the issue in NetLoop().

Regards,
Upakul Barkakaty
Conexant Systems,Inc

On 11/14/07, Detlev Zundel <dzu@denx.de> wrote:
>
> Hi Upakul,
>
> > Hi all, I observed that when the Ethernet initialization fails, it is
> not
> > properly halting the operation and exiting. On walking through the
> Networking
> > files, I saw that the eth_init() in eth.c either returns a 0 or 1. Now,
> in the
> > NetLoop() in net.c file, from where the eth_init() gets called, checks
> the
> > condition if (eth_init(bd) < 0) { eth_halt(); return(-1); } which is
> thus never
> > true. Thus the network operation, never exits gracefully, if Ethernet
> init
> > fails. Any of you, have any clues about this??
>
> I think your analysis is right and thus the code in NetLoop should be
> fixed.  Feel free to send a patch and I am pretty sure Ben Warren will
> pick it up into the u-boot-net repo.
>
> Cheers
>   Detlev
>
> --
> Two  monks went  fishing in an  electron river.  The first monk drew out
> his
> network, and out flopped a hacker.  The second monk cried, "The poor
> hacker!
> How can  it live outside of the  network?"   The first monk said,  "When
> you
> have learned to live outside the network, then you will know."
> --
> DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20071116/68985e7e/attachment.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: net.c.patch
Type: application/octet-stream
Size: 290 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20071116/68985e7e/attachment.obj 

[U-Boot-Users] What if eth_init() fails?

[Detlev Zundel]

Hi Upakul,

> Thanks for the replies. I am attaching herewith, the patch which I suppose
> should fix the issue in NetLoop().

Thanks, but nowadays we would appreciate if you could send a patch
with a proper commit message and a Signed-Off-By line.  Just look at
recent patches here on the mailing list to see what I mean.  Oh and
by the way, version 1.2.0 will not get any update fixes so please base
your patch on the top of tree of git, thanks.

> --- u-boot-1.2.0_orig/net/net.c	2007-01-07 04:43:11.000000000 +0530
> +++ u-boot-1.2.0/net/net.c	2007-11-14 18:03:03.000000000 +0530
> @@ -305,7 +305,7 @@
>  #ifdef CONFIG_NET_MULTI
>  	eth_set_current();
>  #endif
> -	if (eth_init(bd) < 0) {
> +	if (eth_init(bd) > 0) {
>  		eth_halt();
>  		return(-1);
>  	}

Secondly and more important, did you test this?  I'd say your test is
the wrong way round, i.e. eth_init returns true in the C sense (!=0)
if it was able to initialize an interface.  (This also chimes with the
naming of the function by the way).  So I'd propose to go for
"!eth_init(..)".

Cheers
  Detlev

-- 
Man sei weder unzufrieden mit sich selbst - denn das waere Kleinmut - noch
selbstzufrieden - denn das waere Dummheit.
                                    --- Baltasar Gracian
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot-Users] What if eth_init() fails?

[Detlev Zundel]

Hi,

>> --- u-boot-1.2.0_orig/net/net.c	2007-01-07 04:43:11.000000000 +0530
>> +++ u-boot-1.2.0/net/net.c	2007-11-14 18:03:03.000000000 +0530
>> @@ -305,7 +305,7 @@
>>  #ifdef CONFIG_NET_MULTI
>>  	eth_set_current();
>>  #endif
>> -	if (eth_init(bd) < 0) {
>> +	if (eth_init(bd) > 0) {
>>  		eth_halt();
>>  		return(-1);
>>  	}
>
> Secondly and more important, did you test this?  I'd say your test is
> the wrong way round, i.e. eth_init returns true in the C sense (!=0)
> if it was able to initialize an interface.  (This also chimes with the
> naming of the function by the way).  So I'd propose to go for
> "!eth_init(..)".

Thinking about this somemore, I am now convinced that the problem
should not be fixed at the caller but in eth_init.  Testing for <0 is
a pretty idiomatic test for errors, so we should rather adjust
eth_init to fit this idiom than spreading "aberrant" behaviour.

Cheers
  Detlev

-- 
Computers are useless. They can only give you answers.
             -- Pablo Picasso
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de

[U-Boot-Users] What if eth_init() fails?

[Wolfgang Denk]

Dear Upakul,

in message <bb58ac4d0711152213y4e4520ay44ae0b9d5302b9c2@mail.gmail.com> you wrote:
> 
> Thanks for the replies. I am attaching herewith, the patch which I suppose
> should fix the issue in NetLoop().

I'm sorry, but I think this is actually not a good idea.

Tradition is that a function returns <0 (typical -1) in case of
problems, and a return code >=0 indicates success (eventually
including a useful return value).

It seems that the original call was based on that expectation, too:

--- u-boot-1.2.0_orig/net/net.c	2007-01-07 04:43:11.000000000 +0530
+++ u-boot-1.2.0/net/net.c	2007-11-14 18:03:03.000000000 +0530
@@ -305,7 +305,7 @@
 #ifdef CONFIG_NET_MULTI
 	eth_set_current();
 #endif
-	if (eth_init(bd) < 0) {

The test for  "< 0" reads to me: "if there was an error"...

+	if (eth_init(bd) > 0) {

Now this is completely misleading.


Assuming that eth_init() can only result in sucess or failure, the
test should be written either as

	if (eth_init(bd) != 0) ... /* error handling */

or be left as is:

	if (eth_init(bd) < 0) ... /* error handling */

Actually I strongly prefer the second form which perfectly matches my
internal C parser :-)

So the real fix for this problem is to change the code of eth_init()
instead and to make it return -1 in case of errors (instead of 1).

Note that all places where eth_init() is called should be checked.


And BTW: please stop posting HTML!

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
There is an order of things in this universe.
	-- Apollo, "Who Mourns for Adonais?" stardate 3468.1

[U-Boot-Users] What if eth_init() fails?

[Ben Warren]

Hi Upakul,

Upakul Barkakaty wrote:
> Hi all, I observed that when the Ethernet initialization fails, it is 
> not properly halting the operation and exiting. On walking through the 
> Networking files, I saw that the eth_init() in eth.c either returns a 
> 0 or 1. Now, in the NetLoop() in net.c file, from where the eth_init() 
> gets called, checks the condition if (eth_init(bd) < 0) { eth_halt(); 
> return(-1); } which is thus never true. Thus the network operation, 
> never exits gracefully, if Ethernet init fails. Any of you, have any 
> clues about this??
> ------------------------------------------------------------------------
> View this message in context: What if eth_init() fails? 
> <http://www.nabble.com/What-if-eth_init%28%29-fails--tf4802433.html#a13740586>
> Sent from the Uboot - Users mailing list archive 
> <http://www.nabble.com/Uboot---Users-f553.html> at Nabble.com.
You are correct.  If you send a patch, I'll incorporate it.

regards,
Ben

[U-Boot-Users] What if eth_init() fails?

[Timur Tabi]

Upakul Barkakaty wrote:
> Hi all, I observed that when the Ethernet initialization fails, it is 
> not properly halting the operation and exiting. On walking through the 
> Networking files, I saw that the eth_init() in eth.c either returns a 0 
> or 1. Now, in the NetLoop() in net.c file, from where the eth_init() 
> gets called, checks the condition if (eth_init(bd) < 0) { eth_halt(); 
> return(-1); } which is thus never true. Thus the network operation, 
> never exits gracefully, if Ethernet init fails. Any of you, have any 
> clues about this??

This is a known bug.  The problem is that it's been around for so long, people 
don't realize what's happening.  If you fix it, you might break something else.

I still think it should be fixed.  In fact, I was planning on submitting a patch 
next month for it.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot-Users] What if eth_init() fails?

[Mike Frysinger]

On Wednesday 14 November 2007, Timur Tabi wrote:
> Upakul Barkakaty wrote:
> > Hi all, I observed that when the Ethernet initialization fails, it is
> > not properly halting the operation and exiting. On walking through the
> > Networking files, I saw that the eth_init() in eth.c either returns a 0
> > or 1. Now, in the NetLoop() in net.c file, from where the eth_init()
> > gets called, checks the condition if (eth_init(bd) < 0) { eth_halt();
> > return(-1); } which is thus never true. Thus the network operation,
> > never exits gracefully, if Ethernet init fails. Any of you, have any
> > clues about this??
>
> This is a known bug.  The problem is that it's been around for so long,
> people don't realize what's happening.  If you fix it, you might break
> something else.

so by fixing one bug, you may expose other bugs, and that's a bad thing ?  bad 
code has gotta go !
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20071114/ed3e560a/attachment.pgp 

[U-Boot-Users] What if eth_init() fails?

[Timur Tabi]

Mike Frysinger wrote:

> so by fixing one bug, you may expose other bugs, and that's a bad thing ?  bad 
> code has gotta go !

My point was that if you fix this bug, you should be prepared for other bugs to 
show up, and people to be surprised about it.  Don't just fix the first bug and 
walk away.  If you can't test all the other platforms, then you should at least 
pay attention when other people report the other bugs, and then help them out.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot-Users] What if eth_init() fails?

[Mike Frysinger]

On Wednesday 14 November 2007, Timur Tabi wrote:
> Mike Frysinger wrote:
> > so by fixing one bug, you may expose other bugs, and that's a bad thing ?
> >  bad code has gotta go !
>
> My point was that if you fix this bug, you should be prepared for other
> bugs to show up, and people to be surprised about it.  Don't just fix the
> first bug and walk away.  If you can't test all the other platforms, then
> you should at least pay attention when other people report the other bugs,
> and then help them out.

i'd say it is the fault of those platforms for having bad code in the first 
place ... cant reasonably expect Upakul to audit and fix everyone's mistakes.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20071114/002769c6/attachment.pgp 

[U-Boot-Users] What if eth_init() fails?

[Upakul Barkakaty]

Hi Wolfgang/Detlev,

Thanks for writing in. Yes, I do agree with your suggestion that
changing the code in eth.c would be, logically, the best way to handle
this. 

The reason why I had suggested changing the code in Netloop() in net.c,
was because I had observed the Linux open source file dev.c, where
positive error codes had been handled in the dev_init() function, so I
thought that would not be a bad idea. Here is the code snippet from the
Linux file dev.c

	/* Init, if this function is available */
	if (dev->init) {
		ret = dev->init(dev);
		if (ret) {
			if (ret > 0)
				ret = -EIO;
			goto out_err;
		}
	}


But on second thoughts, yes, logically, it would be best to leave the
Netloop() code as it is:

if (eth_init(bd) < 0) ... /* error handling */

and modify the code in eth.c file to make it return a negative code in
case of errors, to somewhat like this:

int eth_init(bd_t *bis)
{
	struct eth_device* old_current;

	if (!eth_current)
		return -1;

	old_current = eth_current;
	do {
		debug ("Trying %s\n", eth_current->name);

		if (!eth_current->init(eth_current,bis)) 
		{
			eth_current->state = ETH_STATE_ACTIVE;
			return 0;
		}
		debug  ("FAIL\n");

		eth_try_another(0);
	} while (old_current != eth_current);

	return -1;
}


Regards,
 
Upakul Barkakaty
Conexant Systems, Inc.



On 11/16/07, Wolfgang Denk <wd@denx.de> wrote:

    Dear Upakul,

    in message
<bb58ac4d0711152213y4e4520ay44ae0b9d5302b9c2@mail.gmail.com> you wrote:
    >
    > Thanks for the replies. I am attaching herewith, the patch which I
suppose
    > should fix the issue in NetLoop().

    I'm sorry, but I think this is actually not a good idea.

    Tradition is that a function returns <0 (typical -1) in case of
    problems, and a return code >=0 indicates success (eventually
    including a useful return value).

    It seems that the original call was based on that expectation, too:

    --- u-boot-1.2.0_orig/net/net.c 2007-01-07 04:43:11.000000000 +0530
    +++ u-boot-1.2.0/net/net.c      2007-11-14 18:03:03.000000000 +0530
    @@ -305,7 +305,7 @@
    #ifdef CONFIG_NET_MULTI
            eth_set_current();
    #endif
    -       if (eth_init(bd) < 0) {

    The test for  "< 0" reads to me: "if there was an error"...

    +       if (eth_init(bd) > 0) {

    Now this is completely misleading.


    Assuming that eth_init() can only result in sucess or failure, the
    test should be written either as

            if (eth_init(bd) != 0) ... /* error handling */

    or be left as is:

            if (eth_init(bd) < 0) ... /* error handling */

    Actually I strongly prefer the second form which perfectly matches
my
    internal C parser :-)

    So the real fix for this problem is to change the code of eth_init()
    instead and to make it return -1 in case of errors (instead of 1).

    Note that all places where eth_init() is called should be checked.


    And BTW: please stop posting HTML!

    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
    There is an order of things in this universe.
            -- Apollo, "Who Mourns for Adonais?" stardate 3468.1



Conexant E-mail Firewall (Conexant.Com) made the following annotations
---------------------------------------------------------------------
********************** Legal Disclaimer **************************** 

"This email may contain confidential and privileged material for the sole use of the intended recipient. Any unauthorized review, use or distribution by others is strictly prohibited. If you have received the message in error, please advise the sender by reply email and delete the message. Thank you." 

********************************************************************** 

---------------------------------------------------------------------

[U-Boot-Users] What if eth_init() fails?

[Wolfgang Denk]

In message <8FCC6A94CA3DF74CB89E1044DBFB0BF803D8AB@NOIDA-MAIL2.bbnet.ad> you wrote:
> 
...
> But on second thoughts, yes, logically, it would be best to leave the
> Netloop() code as it is:
> 
> if (eth_init(bd) < 0) ... /* error handling */
> 
> and modify the code in eth.c file to make it return a negative code in
> case of errors, to somewhat like this:

OK. Will you submit a patch, please?

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 at denx.de
Unix is supported by IBM, like a hanging man is supported by rope.
		        - Don Libes & Sandy Ressler: _Life With Unix_