public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Heiko Schocher <hs@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] netloop: speed up NetLoop
Date: Wed, 25 Feb 2009 08:46:36 +0100	[thread overview]
Message-ID: <49A4F75C.4010101@denx.de> (raw)
In-Reply-To: <200902241545.06473.vapier@gentoo.org>

Hello Mike,

Mike Frysinger wrote:
> On Tuesday 10 February 2009 03:38:52 Heiko Schocher wrote:
>> NetLoop polls every cycle with getenv some environment variables.
>> This is horribly slow, especially when the environment is big.
>>
>> This patch reads only the environment variables in NetLoop,
>> when they were changed.
>>
>> Also moved the init part of the NetLoop function in a seperate
>> function.
> 
> a bit late, but oh well ...

better late then never ;-)

>> --- a/common/cmd_nvedit.c
>> +++ b/common/cmd_nvedit.c
>> +static int env_id = 1;
>>
>> +int get_env_id (void)
>> +{
>> +	return env_id;
>> +}
> 
> there's no documentation anywhere in the source about these new functions.  
> people have to read the changelog to figure out what the function is for and 
> how it is used.  at least to me, the usage is not conveyed in any way by the 
> function name.  a simple comment block right above these would go a long way.

Ok, I try to add a comment.

>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -28,6 +28,9 @@
>>
>>  #if defined(CONFIG_CMD_NET) && defined(CONFIG_NET_MULTI)
>>
>> +static char *act = NULL;
>> +static int  env_changed_id = 0;
> 
> since these are only used by eth_set_current(), shouldnt they be in that 
> function instead of file scope ?  nothing else in eth.c uses them.

Yes, you are right.

>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -209,6 +209,8 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
>> +int		env_changed_id = 0;
> 
> why isnt marked static ?  and since only NetInitLoop() uses it, it should be 
> in that function rather than file scope.

Yep.

>> +int
>> +NetInitLoop(proto_t protocol)
>> +{
> 
> this function always returns 0, and the only caller is in this file, and it 
> doesnt actually check the return value.  so this function should be marked 
> static and changed to void.
> -mike

OK, I sent a patch for this.

thanks
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

  reply	other threads:[~2009-02-25  7:46 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-02-10  8:38 [U-Boot] netloop: speed up NetLoop Heiko Schocher
2009-02-21 21:42 ` Wolfgang Denk
2009-02-23  7:53 ` Ben Warren
2009-02-24 20:45 ` Mike Frysinger
2009-02-25  7:46   ` Heiko Schocher [this message]
2009-02-24 20:54 ` Mike Frysinger
2009-02-25  7:46   ` Heiko Schocher

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=49A4F75C.4010101@denx.de \
    --to=hs@denx.de \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox