[U-Boot] [RFC PATCH] u-boot: remove driver lookup loop from env_save()

[Nicholas]

Hi Simon,

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

On mer, 2018-07-11 at 07:09 +0200, Simon Goldschmidt wrote:
> 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:
> 
> On 10.07.2018 22:49, Simon Glass wrote:
> > 
> > Hi Nicholas,
> > 
> > On 10 July 2018 at 06:57, Nicholas Faustini
> > <nicholas.faustini at azcomtech.com> 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 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.
> > 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.

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.

> Maybe the 'env save' command might need a parameter the tells it
> where 
> to save?
> 
> Regards,
> Simon (Goldschmidt)
> 

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-
>env_load_location.

> > 
> > 
> > > 
> > > 
> > > 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?
> > 
> > > 
> > > 
> > > Signed-off-by: Nicholas Faustini <nicholas.faustini at azcomtech.com
> > > >
> > > ---
> > > 
> > >   env/env.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/env/env.c b/env/env.c
> > > index 5c0842a..897d197 100644
> > > --- a/env/env.c
> > > +++ b/env/env.c
> > > @@ -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
> > > 
> > Regards,
> > Simon
> >