| Message ID | 1314786967-23261-1-git-send-email-monstr@monstr.eu |
|---|---|
| State | Accepted |
| Commit | 7aabad28044f031a8ed74af18999e893c1d9d49c |
| Headers | show |
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 > >
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
Dear Michal Simek, In message <1314786967-23261-1-git-send-email-monstr@monstr.eu> you 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. Yes, and this is intentionally, because it's what poople expect when they run the "dhcpboot" command. > Currently if autoload is not setup dhcp sends request in > forever loop. That's not correct. Only until a DHCP server provides the needed configurationinformation so TFTP gets started. > 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. That would change behavious in a way that is not wanted. Best regards, Wolfgang Denk
Hi, Wolfgang Denk wrote: > Dear Michal Simek, > > In message <1314786967-23261-1-git-send-email-monstr@monstr.eu> you 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. > > Yes, and this is intentionally, because it's what poople expect when > they run the "dhcpboot" command. ok. > >> Currently if autoload is not setup dhcp sends request in >> forever loop. > > That's not correct. Only until a DHCP server provides the needed > configurationinformation so TFTP gets started. That's what I get/see on my platforms. > >> 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. > > That would change behavious in a way that is not wanted. Agree that point 2 change behavior. Point 1 just fix c&p error. Regards, Michal
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(); } #if !defined(CONFIG_CMD_DHCP)
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. 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(-)