[U-Boot] [PATCH] NetLoop initialization bug

Michael Zaidman
2009-03-30T13:07:11Z

Hello Heiko,

Please see my comments and updated patch below.

On Mon, Mar 30, 2009 at 7:12 AM, Heiko Schocher <hs@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.zaidman@gmail.com>
>> Date: Sun, Mar 22, 2009 at 8:35 PM
>> Subject: [U-Boot] [PATCH] NetLoop initialization bug
>> To: u-boot at 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.zaidman at 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