All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.