[U-Boot] Inefficient code in NetLoop() ?
Dear Ben,
I just ran over this piece of code in NetLoop() [see "net/net.c"]:
286 int 287 NetLoop(proto_t protocol) 288 { ... 322 eth_halt(); 323 #ifdef CONFIG_NET_MULTI 324 eth_set_current(); 325 #endif 326 if (eth_init(bd) < 0) { 327 eth_halt(); 328 return(-1); 329 }
Am I reading this correctly that we eth_halt() and eth_init() the network interface for each and every call to NetLoop?
This looks terribly inefficient to me - is there any rationale behind this?
Also, the eth_set_current() checking should IMHO be done just once, before we start a network transfer, or when we actually switch interfaces, but not for each and every call to NetLoop() ?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Ben,
I just ran over this piece of code in NetLoop() [see "net/net.c"]:
286 int 287 NetLoop(proto_t protocol) 288 { ... 322 eth_halt(); 323 #ifdef CONFIG_NET_MULTI 324 eth_set_current(); 325 #endif 326 if (eth_init(bd) < 0) { 327 eth_halt(); 328 return(-1); 329 }
Am I reading this correctly that we eth_halt() and eth_init() the network interface for each and every call to NetLoop?
Yes, it looks that way. Ripe for gutting.
This looks terribly inefficient to me - is there any rationale behind this?
Probably, but it escapes me. It most certainly predates my involvement in this project.
Also, the eth_set_current() checking should IMHO be done just once, before we start a network transfer, or when we actually switch interfaces, but not for each and every call to NetLoop() ?
Maybe, but eth_set_current() is pretty lightweight. NetLoop is only called when we start a network transfer, so this doesn't seem too egregious. It could definitely stand to be refactored.
Best regards,
Wolfgang Denk
regards, Ben
Dear Ben Warren,
In message 48DC7CAD.9040502@gmail.com you wrote:
Am I reading this correctly that we eth_halt() and eth_init() the network interface for each and every call to NetLoop?
Yes, it looks that way. Ripe for gutting.
I didn't have much time to look into the code, so I'm just speculating - maybe this is needed for switching interfaces in case of errors?
This looks terribly inefficient to me - is there any rationale behind this?
Probably, but it escapes me. It most certainly predates my involvement in this project.
I should know, but if I ever understood that part of the code, I have completely forgotten about it ;-)
Also, the eth_set_current() checking should IMHO be done just once, before we start a network transfer, or when we actually switch interfaces, but not for each and every call to NetLoop() ?
Maybe, but eth_set_current() is pretty lightweight. NetLoop is only called when we start a network transfer, so this doesn't seem too egregious. It could definitely stand to be refactored.
Do you plan to have a closer look on this in some near future?
Best regards,
Wolfgang Denk
On Friday 26 September 2008, Wolfgang Denk wrote:
Am I reading this correctly that we eth_halt() and eth_init() the network interface for each and every call to NetLoop?
Yes, it looks that way. Ripe for gutting.
I didn't have much time to look into the code, so I'm just speculating
- maybe this is needed for switching interfaces in case of errors?
Some ethernet interfaces (e.g. ppc4xx) need to get stopped after the network transaction. Otherwise the interface will continue to DMA data to the buffers and this could break OS booting. So please don't remove the eth_halt() after the transaction is finished.
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@denx.de =====================================================================
Stefan Roese wrote:
On Friday 26 September 2008, Wolfgang Denk wrote:
Am I reading this correctly that we eth_halt() and eth_init() the network interface for each and every call to NetLoop?
Yes, it looks that way. Ripe for gutting.
I didn't have much time to look into the code, so I'm just speculating
- maybe this is needed for switching interfaces in case of errors?
Some ethernet interfaces (e.g. ppc4xx) need to get stopped after the network transaction. Otherwise the interface will continue to DMA data to the buffers and this could break OS booting. So please don't remove the eth_halt() after the transaction is finished.
I share Stefan's concerns. Isn't in general the eth_init()/eth_halt() construct because of polling mode where the network controller needs to make sure to operate in well defined states?
In our deployments of using U-Boot networking facilities in standalone apps, there were problems when not doing the full eth_init()/eth_halt() sequence: without closing eth_halt() after the initial successfull transaction the network interface would choke and not work after some time, and all subsequent transfers would fail.
kind regards, Rafal
Dear Stefan,
in message 200809260955.59674.sr@denx.de you wrote:
Am I reading this correctly that we eth_halt() and eth_init() the network interface for each and every call to NetLoop?
Yes, it looks that way. Ripe for gutting.
I didn't have much time to look into the code, so I'm just speculating
- maybe this is needed for switching interfaces in case of errors?
Some ethernet interfaces (e.g. ppc4xx) need to get stopped after the network transaction. Otherwise the interface will continue to DMA data to the buffers and this could break OS booting. So please don't remove the eth_halt() after the transaction is finished.
Agreed, *after* performing the task, i. e. before the network related command returns to the shell, the network interface should be shut down. But not right in the middle, in each netloop.
Also, please be aware that we're discussing this in the context of netconsole, where it actually happens for each and every character transmitted :-(
Best regards,
Wolfgang Denk
participants (4)
-
Ben Warren -
Rafal Jaworowski -
Stefan Roese -
Wolfgang Denk