[U-Boot] Inefficient code in NetLoop() ?

[U-Boot] Inefficient code in NetLoop() ?

[Wolfgang Denk]

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

-- 
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
All easy problems have already been solved.

[U-Boot] Inefficient code in NetLoop() ?

[Ben Warren]

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

[U-Boot] Inefficient code in NetLoop() ?

[Wolfgang Denk]

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

-- 
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: Some say the learning curve is steep,  but  you  only  have  to
climb it once.                                      - Karl Lehenbauer

[U-Boot] Inefficient code in NetLoop() ?

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot] Inefficient code in NetLoop() ?

[Rafal Jaworowski]

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

[U-Boot] Inefficient code in NetLoop() ?

[Wolfgang Denk]

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

-- 
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
You cannot propel yourself forward by patting yourself on the back.