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]>

---

Changes in v2:
- Restore gd->env_load_location to the highest priority location when env_load() fails

  env/env.c | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)

env/env.c +0 -0

index 5c0842a..18eb78d 100644

env/env.c +3 -2

+	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();

Quoted 23 lines · Nicholas Faustini

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]>

---

Changes in v2:
- Restore gd->env_load_location to the highest priority location when
   env_load() fails

  env/env.c | 10 ++++++----

  1 file changed, 6 insertions(+), 4 deletions(-)

env/env.c int env_load(void) · line 208

Quoted 9 lines

env/env.c +0 -0

index 5c0842a..18eb78d 100644
@@ -205,22 +205,24 @@ int env_load(void)
   			return 0;
   	}

env/env.c +1 -0

+	env_get_location(ENVOP_LOAD, 0);

A comment why this is required would be good, I guess.

Quoted 13 lines

+
   return -ENODEV;
  }

  int env_save(void)
  {
   struct env_driver *drv;

env/env.c +0 -1

-	int prio;

env/env.c +0 -1

-	for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE,

prio)); prio++) {

env/env.c int env_load(void) · line 217

Quoted 1 line

env/env.c +1 -0

+	drv = env_driver_lookup(ENVOP_SAVE, 0);

Thinking again about this, would it make more sense to store 'env_load_prio' in 'gd' after successful load? That way, 'env_get_location()' would be more straightforward (no special case for
ENVOP_SAVE) and here in 'env_save()' we could just write something like
this:

drv = env_driver_lookup(ENVOP_SAVE, gd->env_load_prio);

save and !env_has_inited(drv->location) (below)... I put -ENODEV but I

Simon

Quoted 16 lines

env/env.c +1 -0

+	if (drv) {

   int ret;

   if (!drv->save)

env/env.c +1 -1

-			continue;
+			return -ENODEV;

   if (!env_has_inited(drv->location))

env/env.c +1 -1

-			continue;
+			return -ENODEV;

   printf("Saving Environment to %s... ", drv-

Quoted 1 line

name);

   ret = drv->save();

Quoted 33 lines · Simon Goldschmidt

Quoted 31 lines · Nicholas Faustini

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]>

---

Changes in v2:
- Restore gd->env_load_location to the highest priority location when
   env_load() fails

   env/env.c | 10 ++++++----
   1 file changed, 6 insertions(+), 4 deletions(-)

env/env.c +0 -0

index 5c0842a..18eb78d 100644
@@ -205,22 +205,24 @@ int env_load(void)
   			return 0;
   	}
 

Quoted 2 lines

env/env.c int env_load(void) · line 208

Quoted 1 line

env/env.c +1 -0

+	env_get_location(ENVOP_LOAD, 0);

A comment why this is required would be good, I guess.

Sure, I thought the same but eventually I didn't put it. Will do in the next version.

Quoted 22 lines

Quoted 4 lines

env/env.c +1 -0

+
   	return -ENODEV;

env/env.c int env_load(void) · line 217

Quoted 10 lines

env/env.c +1 -2

   }
 
   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, 0);

Thinking again about this, would it make more sense to store 'env_load_prio' in 'gd' after successful load? That way, 'env_get_location()' would be more straightforward (no special case for
ENVOP_SAVE) and here in 'env_save()' we could just write something like
this:

Quoted 3 lines

drv = env_driver_lookup(ENVOP_SAVE, gd->env_load_prio);

I really like the 'env_load_prio' idea. But I also like having the special case for ENVOP_SAVE in env_get_location() as it enforces the fact that the location of 'save' is bound to the location of previous 'load'.
I however don't have a strong opinion on this. I could even remove the whole switch() statement if we introduce 'env_load_prio'.

Also, I have some doubts about what it should be returned when !drv-

Quoted 1 line

save and !env_has_inited(drv->location) (below)... I put -ENODEV but I

don't like it so much.

Quoted 19 lines

Simon

Quoted 16 lines

env/env.c +3 -2

+	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-

Quoted 1 line

name);

   ret = drv->save();