[U-Boot-Users] What if eth_init() fails?
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??
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
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@denx.de
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
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
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
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
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.
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
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.
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
participants (6)
-
Ben Warren -
Detlev Zundel -
Mike Frysinger -
Timur Tabi -
Upakul Barkakaty -
Wolfgang Denk