Hello Heiko,
Please see my comments and updated patch below.
On Mon, Mar 30, 2009 at 7:12 AM, Heiko Schocher <h...@denx.de> wrote:
> Hello Michael,
>
> Michael Zaidman wrote:
>> Hi Heiko,
>>
>> The patch "netloop: speed up NetLoop" you delivered
>> into the u-boot-2009.03 introduced bug I have described
>> below a few days ago. Could you please take a look at
>> the proposed fix?
>>
>
> Sorry, seems I missed it.
>
>> Thanks,
>> Michael
>>
>> ---------- Forwarded message ----------
>> From: Michael Zaidman <michael.zaid...@gmail.com>
>> Date: Sun, Mar 22, 2009 at 8:35 PM
>> Subject: [U-Boot] [PATCH] NetLoop initialization bug
>> To: u-boot@lists.denx.de
>>
>>
>> [U-Boot] [PATCH] NetLoop initialization bug
>>
>
> Thanks for catching this.
> Your patch doesn;t apply, I get "malformed patch". Before
> sending a new one, please have a look at my comments:
>
>>
>>
>> Upon u-boot's start up the first ping command causes a failure of the
>> consequent TFTP command. It happens in the recently added mechanism of
>> the NetLoop initialization where initialization of global network parameters
>> is
>> separated in the NetInitLoop routine which is called per env_id change.
>> Thus, ping request will initialize the network parameters necessary for ping
>> operation only, afterwards the env_changed_id will be set to the env_id
>> that will prevent all following initialization requests from other protocols.
>> The problem is that the initialized by ping subset of network parameters is
>> not
>> sufficient for other protocols and particularly for TFTP
>> which requires the NetServerIp also.
>>
>> Signed-off-by: michael.zaid...@gmail.com
>>
>> diff --git a/net/net.c b/net/net.c
>> index a89f6a0..dc98d0f 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -288,64 +288,13 @@ NetInitLoop(proto_t protocol)
>> if (env_changed_id == env_id)
>> return 0;
>>
>> - switch (protocol) {
>> -#if defined(CONFIG_CMD_NFS)
>> - case NFS:
>> -#endif
>> -#if defined(CONFIG_CMD_PING)
>> - case PING:
>> -#endif
>> -#if defined(CONFIG_CMD_SNTP)
>> - case SNTP:
>> -#endif
>> - case NETCONS:
>> - case TFTP:
>> - NetCopyIP(&NetOurIP, &bd->bi_ip_addr);
>> - NetOurGatewayIP = getenv_IPaddr ("gatewayip");
>> - NetOurSubnetMask= getenv_IPaddr ("netmask");
>> - NetOurVLAN = getenv_VLAN("vlan");
>> - NetOurNativeVLAN = getenv_VLAN("nvlan");
>> + NetCopyIP(&NetOurIP, &bd->bi_ip_addr);
>> + NetOurGatewayIP = getenv_IPaddr ("gatewayip");
>> + NetOurSubnetMask= getenv_IPaddr ("netmask");
>> + NetServerIP = getenv_IPaddr ("serverip");
>>
>
> The following 2 vars are just used, if CONFIG_CMD_CDP
> is used, can we do a "#if defined" around it?
>> + NetOurNativeVLAN = getenv_VLAN("nvlan");
>> + NetOurVLAN = getenv_VLAN("vlan");
These two variables have been initialized in original code for all
protocols supported by u-boot. As I can see, at least the NetOurVLAN
is used by most of them. This was the reason for keeping them in place.
>>
> [...]
>> @@ -443,18 +392,19 @@ restart:
>> /* Start with a clean slate... */
>> BootpTry = 0;
>> NetOurIP = 0;
>> - NetServerIP = getenv_IPaddr ("serverip");
>> DhcpRequest(); /* Basically same as BOOTP */
>> break;
>> #endif
>>
>> case BOOTP:
>> BootpTry = 0;
>> + NetOurIP = 0;
>>
>
> why we need this here?
Generally, for the same reason the DHCP does - "Start with a clean state..."
On the other hand you are right - we do not need to clear the NetOurIP
address for BOOTP, DHCP and RARP because the current implementation
does not check it. So it has been removed in my new patch below.
>
>> BootpRequest ();
>> break;
>>
>> case RARP:
>> RarpTry = 0;
>> + NetOurIP = 0;
>>
> also here?
> Has this to do something with the bugfix?
The same as before.
>
> bye
> Heiko
>
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>
>
Here is the updated patch:
Subject: [U-Boot] [PATCH] NetLoop initialization bug
---
net/net.c | 66 +++++-------------------------------------------------------
1 files changed, 6 insertions(+), 60 deletions(-)
diff --git a/net/net.c b/net/net.c
index a89f6a0..91ed0d7 100644
--- a/net/net.c
+++ b/net/net.c
@@ -288,64 +288,13 @@ NetInitLoop(proto_t protocol)
if (env_changed_id == env_id)
return 0;
- switch (protocol) {
-#if defined(CONFIG_CMD_NFS)
- case NFS:
-#endif
-#if defined(CONFIG_CMD_PING)
- case PING:
-#endif
-#if defined(CONFIG_CMD_SNTP)
- case SNTP:
-#endif
- case NETCONS:
- case TFTP:
- NetCopyIP(&NetOurIP, &bd->bi_ip_addr);
- NetOurGatewayIP = getenv_IPaddr ("gatewayip");
- NetOurSubnetMask= getenv_IPaddr ("netmask");
- NetOurVLAN = getenv_VLAN("vlan");
- NetOurNativeVLAN = getenv_VLAN("nvlan");
+ NetCopyIP(&NetOurIP, &bd->bi_ip_addr);
+ NetOurGatewayIP = getenv_IPaddr ("gatewayip");
+ NetOurSubnetMask= getenv_IPaddr ("netmask");
+ NetServerIP = getenv_IPaddr ("serverip");
+ NetOurNativeVLAN = getenv_VLAN("nvlan");
+ NetOurVLAN = getenv_VLAN("vlan");
- switch (protocol) {
-#if defined(CONFIG_CMD_NFS)
- case NFS:
-#endif
- case NETCONS:
- case TFTP:
- NetServerIP = getenv_IPaddr ("serverip");
- break;
-#if defined(CONFIG_CMD_PING)
- case PING:
- /* nothing */
- break;
-#endif
-#if defined(CONFIG_CMD_SNTP)
- case SNTP:
- /* nothing */
- break;
-#endif
- default:
- break;
- }
-
- break;
- case BOOTP:
- case RARP:
- /*
- * initialize our IP addr to 0 in order to accept ANY
- * IP addr assigned to us by the BOOTP / RARP server
- */
- NetOurIP = 0;
- NetServerIP = getenv_IPaddr ("serverip");
- NetOurVLAN = getenv_VLAN("vlan"); /* VLANs must be read */
- NetOurNativeVLAN = getenv_VLAN("nvlan");
- case CDP:
- NetOurVLAN = getenv_VLAN("vlan"); /* VLANs must be read */
- NetOurNativeVLAN = getenv_VLAN("nvlan");
- break;
- default:
- break;
- }
env_changed_id = env_id;
return 0;
}
@@ -440,10 +389,7 @@ restart:
#if defined(CONFIG_CMD_DHCP)
case DHCP:
- /* Start with a clean slate... */
BootpTry = 0;
- NetOurIP = 0;
- NetServerIP = getenv_IPaddr ("serverip");
DhcpRequest(); /* Basically same as BOOTP */
break;
#endif
--
1.5.6.3
Thanks,
Michael
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
