u-boot: remove driver lookup loop from env_save()

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 <[email protected]>

Reviewed-by Simon Goldschmidt <[email protected]>

---

Changes in v5:
- Correction to 'Reviewed-by' tag

Changes in v4:
- Remove env_load_location from gd_t since it is not used anymore

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                         | 34 ++++++++++++++++------------------
  include/asm-generic/global_data.h |  2 +-
  2 files changed, 17 insertions(+), 19 deletions(-)

env/env.c +16 -18

index 5c0842a..e033b46 100644
@@ -119,21 +119,12 @@ 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_prio = prio;
 
-	return ENVL_UNKNOWN;
+	return env_locations[prio];
  }
 
 

@@ -205,22 +196,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();

include/asm-generic/global_data.h +1 -1

index 0fd4900..c83fc01 100644
@@ -50,7 +50,7 @@ typedef struct global_data {
  	unsigned long env_addr;		/* Address  of Environment struct */
  	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;		/* Priority of the loaded environment */
 
  	unsigned long ram_base;		/* Base address of RAM used by U-Boot */
  	unsigned long ram_top;		/* Top address of RAM used by U-Boot */

Simon, Maxime,

any input on this? Via which tree would this be pushed? Should we CC Tom?

It would be nice if 2018.09 had this bug fixed (endless loop in env_save() on error).

Quoted 6 lines · Simon Goldschmidt

Simon, Maxime,

any input on this? Via which tree would this be pushed? Should we CC Tom?

It would be nice if 2018.09 had this bug fixed (endless loop in env_save() on error).

This looks good to me but Tom will probably end up this patch, so you definitely want to cc him.

Maxime

+ Tom:

I don't know via which tree this would go in. I think you took the last env changes directly?

v1..v4 are detached threads, I can send you links if you need them.

Quoted 8 lines · Goldschmidt Simon

+ Tom:

I don't know via which tree this would go in. I think you took the last env
changes directly?

v1..v4 are detached threads, I can send you links if you need them.

It's on my TODO list to pick up, thanks!

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 <[email protected]>

Reviewed-by Simon Goldschmidt <[email protected]>