Message ID | 1531472624-18616-1-git-send-email-nicholas.faustini@azcomtech.com |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
Series | [U-Boot,v3] u-boot: remove driver lookup loop from env_save() | expand |
On 13.07.2018 11:03, Nicholas Faustini wrote: > When called with ENVOP_SAVE, env_get_location() only returns the > gd->env_load_location variable without actually checking for > the environment location and priority. > > This behaviour causes env_save() to fall into an infinite loop when > the low-level drv->save() call fails. > > The env_save() function should not loop through the environment > location list but it should save the environment into the location > stored in gd->env_load_location by the last env_load() call. > > Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com> > --- > > Changes in v3: > - Add comment when env_load() fails and the env location is restored > - Introduce env_load_prio into gd struct. It stores the current > priority of the environment location. Refactor of env_get_location() > which acts the same on all the 'env_operation' > > Changes in v2: > - Restore gd->env_load_location to the highest priority location when > env_load() fails > > env/env.c | 35 +++++++++++++++++------------------ > include/asm-generic/global_data.h | 1 + > 2 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/env/env.c b/env/env.c > index 5c0842a..ba13ba8 100644 > --- a/env/env.c > +++ b/env/env.c > @@ -119,21 +119,13 @@ static void env_set_inited(enum env_location location) > */ > __weak enum env_location env_get_location(enum env_operation op, int prio) > { > - switch (op) { > - case ENVOP_GET_CHAR: > - case ENVOP_INIT: > - case ENVOP_LOAD: > - if (prio >= ARRAY_SIZE(env_locations)) > - return ENVL_UNKNOWN; > - > - gd->env_load_location = env_locations[prio]; > - return gd->env_load_location; > - > - case ENVOP_SAVE: > - return gd->env_load_location; > - } > + if (prio >= ARRAY_SIZE(env_locations)) > + return ENVL_UNKNOWN; > + > + gd->env_load_location = env_locations[prio]; > + gd->env_load_prio = prio; > > - return ENVL_UNKNOWN; > + return gd->env_load_location; Ehrm, I just saw gd->env_load_location is now unused... Other than that: Reviewed-by: Simon Goldschmidt <goldsimon@gmx.de> > } > > > @@ -205,22 +197,29 @@ int env_load(void) > return 0; > } > > + /* > + * In case of invalid environment, we set the 'default' env location > + * to the highest priority. In this way, next calls to env_save() > + * will restore the environment at the right place. > + */ > + env_get_location(ENVOP_LOAD, 0); > + > return -ENODEV; > } > > int env_save(void) > { > struct env_driver *drv; > - int prio; > > - for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) { > + drv = env_driver_lookup(ENVOP_SAVE, gd->env_load_prio); > + if (drv) { > int ret; > > if (!drv->save) > - continue; > + return -ENODEV; > > if (!env_has_inited(drv->location)) > - continue; > + return -ENODEV; > > printf("Saving Environment to %s... ", drv->name); > ret = drv->save(); > diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h > index 2d451f8..319810a 100644 > --- a/include/asm-generic/global_data.h > +++ b/include/asm-generic/global_data.h > @@ -51,6 +51,7 @@ typedef struct global_data { > unsigned long env_valid; /* Environment valid? enum env_valid */ > unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */ > int env_load_location; > + int env_load_prio; > > unsigned long ram_top; /* Top address of RAM used by U-Boot */ > unsigned long relocaddr; /* Start address of U-Boot in RAM */ >
On ven, 2018-07-13 at 11:15 +0200, Simon Goldschmidt wrote: > > On 13.07.2018 11:03, Nicholas Faustini wrote: > > > > When called with ENVOP_SAVE, env_get_location() only returns the > > gd->env_load_location variable without actually checking for > > the environment location and priority. > > > > This behaviour causes env_save() to fall into an infinite loop when > > the low-level drv->save() call fails. > > > > The env_save() function should not loop through the environment > > location list but it should save the environment into the location > > stored in gd->env_load_location by the last env_load() call. > > > > Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com> > > --- > > > > Changes in v3: > > - Add comment when env_load() fails and the env location is > > restored > > - Introduce env_load_prio into gd struct. It stores the current > > priority of the environment location. Refactor of > > env_get_location() > > which acts the same on all the 'env_operation' > > > > Changes in v2: > > - Restore gd->env_load_location to the highest priority location > > when > > env_load() fails > > > > env/env.c | 35 +++++++++++++++++--------- > > --------- > > include/asm-generic/global_data.h | 1 + > > 2 files changed, 18 insertions(+), 18 deletions(-) > > > > diff --git a/env/env.c b/env/env.c > > index 5c0842a..ba13ba8 100644 > > --- a/env/env.c > > +++ b/env/env.c > > @@ -119,21 +119,13 @@ static void env_set_inited(enum env_location > > location) > > */ > > __weak enum env_location env_get_location(enum env_operation op, > > int prio) > > { > > - switch (op) { > > - case ENVOP_GET_CHAR: > > - case ENVOP_INIT: > > - case ENVOP_LOAD: > > - if (prio >= ARRAY_SIZE(env_locations)) > > - return ENVL_UNKNOWN; > > - > > - gd->env_load_location = env_locations[prio]; > > - return gd->env_load_location; > > - > > - case ENVOP_SAVE: > > - return gd->env_load_location; > > - } > > + if (prio >= ARRAY_SIZE(env_locations)) > > + return ENVL_UNKNOWN; > > + > > + gd->env_load_location = env_locations[prio]; > > + gd->env_load_prio = prio; > > > > - return ENVL_UNKNOWN; > > + return gd->env_load_location; > Ehrm, I just saw gd->env_load_location is now unused... > Other than that: > > Reviewed-by: Simon Goldschmidt <goldsimon@gmx.de> I didn't notice that.. :blush: I remove it from gd. env_load_prio is actually enough to get the information about the location if someone will need it. > > > > > } > > > > > > @@ -205,22 +197,29 @@ int env_load(void) > > return 0; > > } > > > > + /* > > + * In case of invalid environment, we set the 'default' > > env location > > + * to the highest priority. In this way, next calls to > > env_save() > > + * will restore the environment at the right place. > > + */ > > + env_get_location(ENVOP_LOAD, 0); > > + > > return -ENODEV; > > } > > > > int env_save(void) > > { > > struct env_driver *drv; > > - int prio; > > > > - for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, > > prio)); prio++) { > > + drv = env_driver_lookup(ENVOP_SAVE, gd->env_load_prio); > > + if (drv) { > > int ret; > > > > if (!drv->save) > > - continue; > > + return -ENODEV; > > > > if (!env_has_inited(drv->location)) > > - continue; > > + return -ENODEV; > > > > printf("Saving Environment to %s... ", drv- > > >name); > > ret = drv->save(); > > diff --git a/include/asm-generic/global_data.h b/include/asm- > > generic/global_data.h > > index 2d451f8..319810a 100644 > > --- a/include/asm-generic/global_data.h > > +++ b/include/asm-generic/global_data.h > > @@ -51,6 +51,7 @@ typedef struct global_data { > > unsigned long env_valid; /* Environment valid? > > enum env_valid */ > > unsigned long env_has_init; /* Bitmask of boolean > > of struct env_location offsets */ > > int env_load_location; > > + int env_load_prio; > > > > unsigned long ram_top; /* Top address of > > RAM used by U-Boot */ > > unsigned long relocaddr; /* Start address of U- > > Boot in RAM */ > >
diff --git a/env/env.c b/env/env.c index 5c0842a..ba13ba8 100644 --- a/env/env.c +++ b/env/env.c @@ -119,21 +119,13 @@ static void env_set_inited(enum env_location location) */ __weak enum env_location env_get_location(enum env_operation op, int prio) { - switch (op) { - case ENVOP_GET_CHAR: - case ENVOP_INIT: - case ENVOP_LOAD: - if (prio >= ARRAY_SIZE(env_locations)) - return ENVL_UNKNOWN; - - gd->env_load_location = env_locations[prio]; - return gd->env_load_location; - - case ENVOP_SAVE: - return gd->env_load_location; - } + if (prio >= ARRAY_SIZE(env_locations)) + return ENVL_UNKNOWN; + + gd->env_load_location = env_locations[prio]; + gd->env_load_prio = prio; - return ENVL_UNKNOWN; + return gd->env_load_location; } @@ -205,22 +197,29 @@ int env_load(void) return 0; } + /* + * In case of invalid environment, we set the 'default' env location + * to the highest priority. In this way, next calls to env_save() + * will restore the environment at the right place. + */ + env_get_location(ENVOP_LOAD, 0); + return -ENODEV; } int env_save(void) { struct env_driver *drv; - int prio; - for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) { + drv = env_driver_lookup(ENVOP_SAVE, gd->env_load_prio); + if (drv) { int ret; if (!drv->save) - continue; + return -ENODEV; if (!env_has_inited(drv->location)) - continue; + return -ENODEV; printf("Saving Environment to %s... ", drv->name); ret = drv->save(); diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 2d451f8..319810a 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -51,6 +51,7 @@ typedef struct global_data { unsigned long env_valid; /* Environment valid? enum env_valid */ unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */ int env_load_location; + int env_load_prio; unsigned long ram_top; /* Top address of RAM used by U-Boot */ unsigned long relocaddr; /* Start address of U-Boot in RAM */
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails. The env_save() function should not loop through the environment location list but it should save the environment into the location stored in gd->env_load_location by the last env_load() call. Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com> --- Changes in v3: - Add comment when env_load() fails and the env location is restored - Introduce env_load_prio into gd struct. It stores the current priority of the environment location. Refactor of env_get_location() which acts the same on all the 'env_operation' Changes in v2: - Restore gd->env_load_location to the highest priority location when env_load() fails env/env.c | 35 +++++++++++++++++------------------ include/asm-generic/global_data.h | 1 + 2 files changed, 18 insertions(+), 18 deletions(-)