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 allows saving the environment into the same location that has been previously loaded.

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 use the previously discovered environment driver once.

Signed-off-by Nicholas Faustini <[email protected]>

---

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

env/env.c +4 -5

index 5c0842a..897d197 100644
@@ -211,16 +211,16 @@ int env_load(void)
 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);
+       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();
--

2.7.4

Hi Nicholas,

Quoted 5 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 allows saving the environment into the same location that has been previously loaded.

Quoted 2 lines

This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.

Why is this? Should it not stop eventually? Do we need a limit on prio?

Quoted 5 lines

The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.

What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?

Quoted 30 lines

Signed-off-by Nicholas Faustini <[email protected]>

---

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

env/env.c +4 -4

index 5c0842a..897d197 100644
@@ -211,16 +211,16 @@ int env_load(void)
  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);
+       if (drv) {
                 int ret;
 
                 if (!drv->save)
-                       continue;
+                       return -ENODEV;
 
                 if (!env_has_inited(drv->location))
-                       continue;
+                       return -ENODEV;
 

Quoted 5 lines

env/env.c +0 -1

                 printf("Saving Environment to %s... ", drv->name);
                 ret = drv->save();
--

2.7.4

Regards,
Simon

Hi Nicholas,

Quoted 5 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 allows saving the environment into the same location that has been previously loaded.

Quoted 2 lines

This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.

Why is this? Should it not stop eventually? Do we need a limit on prio?

Quoted 5 lines

The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.

What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?

Quoted 33 lines

Signed-off-by Nicholas Faustini <[email protected]>

---

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

env/env.c +4 -5

index 5c0842a..897d197 100644
@@ -211,16 +211,16 @@ int env_load(void)
  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);
+       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 2 lines

2.7.4

Regards,
Simon

Hi,

sorry for the disclaimer in the last mail. Still don't know why this is
the default here :-(

Resending without the disclaimer to make possible follow-ups cleaner:

Quoted 12 lines · Simon Glass

Hi Nicholas,

Quoted 7 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 allows saving the environment into the same location that has been previously loaded.

Quoted 5 lines

Quoted 3 lines

This behaviour causes env_save() to fall into an infinite loop when
the low-level drv->save() call fails.

Why is this? Should it not stop eventually? Do we need a limit on prio?

See my description in this mail:

https://lists.denx.de/pipermail/u-boot/2018-April/324728.html

Unfortunately, I got diverted at that time and could not follow up on
this. Essentially, the question is raised what 'env save' is supposed to
do with multiple environments.

Currently, env_save() effectively stores only to the location where the
env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.

Maybe the 'env save' command might need a parameter the tells it where
to save?

Regards,
Simon (Goldschmidt)

env_load_location.

Quoted 52 lines

Quoted 7 lines

The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.

What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?

Quoted 34 lines

Signed-off-by Nicholas Faustini <[email protected]

Quoted 1 line

---

  env/env.c | 8 ++++----

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

env/env.c +4 -4

index 5c0842a..897d197 100644
@@ -211,16 +211,16 @@ int env_load(void)
   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);
+       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 5 lines

Quoted 1 line

name);

                 ret = drv->save();
--
2.7.4

Regards,
Simon

Hi Simon,

thanks for your comments and clarifications. I realize that I was not super clear when describing the problem.

Quoted 36 lines · Simon Goldschmidt

Hi,

sorry for the disclaimer in the last mail. Still don't know why this is
the default here :-(

Resending without the disclaimer to make possible follow-ups cleaner:

Quoted 12 lines · Simon Glass

Hi Nicholas,

Quoted 7 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 allows saving the environment into the same location that has been previously loaded.

Quoted 5 lines

Quoted 3 lines

This behaviour causes env_save() to fall into an infinite loop when
the low-level drv->save() call fails.

Why is this? Should it not stop eventually? Do we need a limit on prio?

See my description in this mail:

https://lists.denx.de/pipermail/u-boot/2018-April/324728.html

Unfortunately, I got diverted at that time and could not follow up on
this. Essentially, the question is raised what 'env save' is supposed to
do with multiple environments.

Quoted 4 lines

Currently, env_save() effectively stores only to the location where the
env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.

I have the same feeling about env_save(). Loading in multiple environments is straightforward. Saving is not.

I personally think that it makes more sense that env_save() saves into the same location from which the enviroment has been previously loaded (that is, current implementation). Otherwise, the logic becomes error- prone since an user may env_load() from one location and env_save() into another, resulting in misalignments which requires a lot of extra logic in order to avoid such misalignments.

Quoted 3 lines

Maybe the 'env save' command might need a parameter the tells it where
to save?

Having a parameter to env_save() might be a solution but in my opinion it would break the priority logic: an user follows the priority logic when loading but she can work around that when saving. Moreover, having the location embedded into env_*() functions is a great nice to have imho.

Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-

Quoted 1 line

env_load_location.

Hi,

Quoted 50 lines · Nicholas

Hi Simon,

thanks for your comments and clarifications. I realize that I was not super clear when describing the problem.

Quoted 36 lines · Simon Goldschmidt

Hi,

sorry for the disclaimer in the last mail. Still don't know why this is
the default here :-(

Resending without the disclaimer to make possible follow-ups cleaner:

Quoted 12 lines · Simon Glass

Hi Nicholas,

Quoted 7 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 allows saving the environment into the same location that has been previously loaded.

Quoted 5 lines

Quoted 3 lines

This behaviour causes env_save() to fall into an infinite loop when
the low-level drv->save() call fails.

Why is this? Should it not stop eventually? Do we need a limit on prio?

See my description in this mail:

https://lists.denx.de/pipermail/u-boot/2018-April/324728.html

Unfortunately, I got diverted at that time and could not follow up on
this. Essentially, the question is raised what 'env save' is supposed to
do with multiple environments.

Quoted 4 lines

Currently, env_save() effectively stores only to the location where the
env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.

I have the same feeling about env_save(). Loading in multiple environments is straightforward. Saving is not.

Quoted 6 lines

I personally think that it makes more sense that env_save() saves into the same location from which the enviroment has been previously loaded (that is, current implementation). Otherwise, the logic becomes error- prone since an user may env_load() from one location and env_save() into another, resulting in misalignments which requires a lot of extra logic in order to avoid such misalignments.

Or worse, you might end up erasing something that shouldn't be erased on your board.

Quoted 9 lines

Quoted 3 lines

Maybe the 'env save' command might need a parameter the tells it where
to save?

Having a parameter to env_save() might be a solution but in my opinion it would break the priority logic: an user follows the priority logic when loading but she can work around that when saving. Moreover, having the location embedded into env_*() functions is a great nice to have imho.

I agree.

Quoted 4 lines

Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-

Quoted 1 line

Quoted 1 line

env_load_location.

I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.

Quoted 109 lines · Maxime Ripard

Hi,

Quoted 65 lines · Nicholas

Hi Simon,

thanks for your comments and clarifications. I realize that I was not
super clear when describing the problem.

Quoted 48 lines · Simon Goldschmidt

Hi,

sorry for the disclaimer in the last mail. Still don't know why this
is
the default here :-(

Resending without the disclaimer to make possible follow-ups cleaner:

Quoted 17 lines · Simon Glass

Hi Nicholas,

Quoted 11 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 allows saving the
environment into the same location that has been previously loaded.

Quoted 7 lines

Quoted 4 lines

This behaviour causes env_save() to fall into an infinite loop
when
the low-level drv->save() call fails.

Why is this? Should it not stop eventually? Do we need a limit on
prio?

See my description in this mail:

https://lists.denx.de/pipermail/u-boot/2018-April/324728.html

Unfortunately, I got diverted at that time and could not follow up
on
this. Essentially, the question is raised what 'env save' is supposed
to
do with multiple environments.

Quoted 6 lines

Currently, env_save() effectively stores only to the location where
the
env has been loaded from, which is questionable. But storing to all
locations or the default location might not be correct, either.

I have the same feeling about env_save(). Loading in multiple environments is straightforward. Saving is not.

Quoted 11 lines

I personally think that it makes more sense that env_save() saves into
the same location from which the enviroment has been previously loaded
(that is, current implementation). Otherwise, the logic becomes error-
prone since an user may env_load() from one location and env_save()
into another, resulting in misalignments which requires a lot of extra
logic in order to avoid such misalignments.

Or worse, you might end up erasing something that shouldn't be erased
on your board.

Quoted 6 lines

Quoted 6 lines

Maybe the 'env save' command might need a parameter the tells it
where
to save?

Quoted 9 lines

Having a parameter to env_save() might be a solution but in my opinion
it would break the priority logic: an user follows the priority logic
when loading but she can work around that when saving. Moreover, having
the location embedded into env_*() functions is a great nice to have
imho.

I agree.

Quoted 8 lines

Maybe a solution could be to have an env_save() function which acts in
a similar way as proposed in my patch and an env_save_prio() function,
which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-

Quoted 1 line

Quoted 5 lines

Quoted 1 line

Quoted 1 line

env_load_location.

I don't really see a use-case for overriding wherever the environment
at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.

Well, the use case I saw was that I wanted to test redundant environment
storage. I admit this is not an end user use case but a developer use
case, so I guess you're right.

So after fixing this endless loops I see two questions:
- to which environment do we store if the one in 'env_load_location' fails to store?

- to which environment do we store if all environments failed to load
(currently, it seems we store to the lowest prio in this case?)

Simon

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-> env_load_location.

I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.

Quoted 7 lines

Well, the use case I saw was that I wanted to test redundant environment  storage. I admit this is not an end user use case but a developer use  case, so I guess you're right.

So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location'  fails to store?

Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?

If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.

If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.

Quoted 24 lines · Nicholas

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-> env_load_location.

I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.

Quoted 7 lines

Well, the use case I saw was that I wanted to test redundant environment  storage. I admit this is not an end user use case but a developer use  case, so I guess you're right.

So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location'  fails to store?

Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?

Quoted 8 lines

If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.

If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.

In that case, we don't want to store to a lower priority, but to a higher one. Otherwise, the environment will be saved just fine, but will not be read next time, which will be very confusing to the user.

And with a higher priority, you might end up overwriting something you weren't expecting to overwrite, so I'd vote 1.

Quoted 34 lines · Maxime Ripard

Quoted 24 lines · Nicholas

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-> env_load_location.

I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.

Quoted 7 lines

Well, the use case I saw was that I wanted to test redundant environment  storage. I admit this is not an end user use case but a developer use  case, so I guess you're right.

So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location'  fails to store?

Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?

Quoted 8 lines

If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.

If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.

Quoted 6 lines

In that case, we don't want to store to a lower priority, but to a higher one. Otherwise, the environment will be saved just fine, but will not be read next time, which will be very confusing to the user.

And with a higher priority, you might end up overwriting something you weren't expecting to overwrite, so I'd vote 1.

I agree that 1 would be best. But from reading the code, unless I'm totally wrong, it seems that the patch sent by Nicholas does not suffice:

If no location contained a valid environment (e.g. no environment written yet), the lowest priority will be written as 'gd->env_load_location' is set to the lowest priority from iterating locations in 'env_load()'.

So we might have to reset 'gd->env_load_location' to highest prio if loading the environment fails.

Quoted 47 lines · Simon Goldschmidt

Quoted 34 lines · Maxime Ripard

Quoted 24 lines · Nicholas

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-> env_load_location.

I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.

Quoted 7 lines

Well, the use case I saw was that I wanted to test redundant environment  storage. I admit this is not an end user use case but a developer use  case, so I guess you're right.

So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location'  fails to store?

Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?

Quoted 8 lines

If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.

If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.

Quoted 6 lines

In that case, we don't want to store to a lower priority, but to a higher one. Otherwise, the environment will be saved just fine, but will not be read next time, which will be very confusing to the user.

And with a higher priority, you might end up overwriting something you weren't expecting to overwrite, so I'd vote 1.

I agree that 1 would be best. But from reading the code, unless I'm totally wrong, it seems that the patch sent by Nicholas does not suffice:

Quoted 6 lines

If no location contained a valid environment (e.g. no environment written yet), the lowest priority will be written as 'gd->env_load_location' is set to the lowest priority from iterating locations in 'env_load()'.

So we might have to reset 'gd->env_load_location' to highest prio if loading the environment fails.

That would make sense yes.

I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.

Dear Maxime,

Quoted 4 lines · In message <20180711094838.zyczks6iksp772ir@flea> you wrote:

I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.

If the feature is available, use cases will spring into mind.

Assume you have a eMMC based system which can also boot from other storage (say, SDCard), and you want to write the current content of the eMMC environment to the alternative boot device.

Yes, there are usually alternative approaches to perform similar actions, but an easy way to save lsewhere might be handy, especially if it comes at low cost.

Quoted 2 lines

If the feature is available, use cases will spring into mind.

Right, but that would bring a much more significant rework as to how the environments are handled than just the changes that are being discussed here. At the moment, the environment can only be stored on one device for each "types" (one raw MMC, one MTD partition, etc). The redundancy allows you to duplicate it, but you won't be able to store on just one instance of your choosing at the moment.

Hi Nicholas,

Quoted 5 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 allows saving the environment into the same location that has been previously loaded.

Quoted 2 lines

This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.

Why is this? Should it not stop eventually?

Do we need a limit on
prio?

Quoted 4 lines

The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.

What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?

Quoted 31 lines

Signed-off-by Nicholas Faustini <[email protected]>

---

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

env/env.c +4 -4

index 5c0842a..897d197 100644
@@ -211,16 +211,16 @@ int env_load(void)
 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);
+       if (drv) {
                int ret;
 
                if (!drv->save)
-                       continue;
+                       return -ENODEV;
 
                if (!env_has_inited(drv->location))
-                       continue;
+                       return -ENODEV;
 

Quoted 5 lines

env/env.c +0 -0

                printf("Saving Environment to %s... ", drv->name);

                ret = drv->save();

--
2.7.4

Regards,
Simon