Re: [U-Boot] [PATCH] bootp: Fix bug in auto_load function

Hi Michal,

On Wed, Aug 31, 2011 at 3:36 AM, Michal Simek monstr@monstr.eu wrote:

Patch: "Put common autoload code into auto_load() function" (sha1: 093498669e77597635a24f326f11efeab213d394) is not simple code cleanup but code change which introduce new bug.

If autoload variable is not setup it worked as autoload=yes.

Currently if autoload is not setup dhcp sends request in forever loop.

Thanks for bring this up - there was some discussion at the time I remember. Also please excuse the verbatim code in this email but I want it to be clear.

The old code in v2011.03 is:

/* Obey the 'autoload' setting */ if ((s = getenv("autoload")) != NULL) { if (*s == 'n') { /* * Just use BOOTP to configure system; * Do not use TFTP to load the bootfile. */ NetState = NETLOOP_SUCCESS; return; #if defined(CONFIG_CMD_NFS) } else if (strcmp(s, "NFS") == 0) { /* * Use NFS to load the bootfile. */ NfsStart(); return; #endif } } TftpStart(); return;

and

if ((s = getenv("autoload")) != NULL) { if (*s == 'n') { /* * Just use BOOTP to configure system; * Do not use TFTP to load the bootfile. */ NetState = NETLOOP_SUCCESS; return; #if defined(CONFIG_CMD_NFS) } else if (strcmp(s, "NFS") == 0) { /* * Use NFS to load the bootfile. */ NfsStart(); return; #endif } }

TftpStart();

These two sites were changed to a call to auto_load:

static void auto_load(void) { const char *s = getenv("autoload");

if (s != NULL) { if (*s == 'n') { /* * Just use BOOTP to configure system; * Do not use TFTP to load the bootfile. */ NetState = NETLOOP_SUCCESS; return; } #if defined(CONFIG_CMD_NFS) if (strcmp(s, "NFS") == 0) { /* * Use NFS to load the bootfile. */ NfsStart(); return; } #endif TftpStart(); } }

I don't see a change in the functionality here - I may be missing something so please can you explain?

From my experience U-Boot has always performed an autoload unless you

'setenv autoload n', even if the variable is not set. From the README:

autoload - if set to "no" (any string beginning with 'n'), "bootp" will just load perform a lookup of the configuration from the BOOTP server, but not try to load any image using TFTP

which suggests that this is the correct behavior (i.e. it defaults to autoload=yes)

There are two options how to fix it:

1. Move TftpStart() which is in this patch
2. Change functionality if autoload is not setup, set NetSate and ends.

@@ -165,7 +165,8 @@ static void auto_load(void)                }  #endif        TftpStart();

• }

• } else
• NetState = NETLOOP_SUCCESS;

}

CC: Eric Bénard eric@eukrea.com CC: Simon Glass sjg@chromium.org Signed-off-by: Michal Simek monstr@monstr.eu

---

net/bootp.c |    2 +-  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bootp.c b/net/bootp.c index 3db08ea..a003c42 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -164,8 +164,8 @@ static void auto_load(void)                        return;                }  #endif

• TftpStart();

}

• TftpStart();

}

Isn't this the opposite of the patch you want? I think you want to move TftpStart() inside the loop, like this:

static void auto_load(void) { const char *s = getenv("autoload");

if (s != NULL) { if (*s == 'n') { /* * Just use BOOTP to configure system; * Do not use TFTP to load the bootfile. */ NetState = NETLOOP_SUCCESS; return; } #if defined(CONFIG_CMD_NFS) if (strcmp(s, "NFS") == 0) { /* * Use NFS to load the bootfile. */ NfsStart(); return; TftpStart(); } #endif } }

but we should first agree that this is the expected behavior, and change the README.

Regards, Simon

#if !defined(CONFIG_CMD_DHCP)

1.5.5.6