Hi,

2011/8/31 Simon Glass sjg@chromium.org

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?

just look at TftpStart(); position in new and old code. In new code is inside if (s != NULL) { } part in old not which change behavior.

Just do simple test run set autoload dhcp

on my system it loops forever.

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)

I am fine with that that if autoload var is not set it means 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 } }

Definitely this one - where TftpStart is not even called.

Michal