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

[Nicholas <nicholas.faustini@azcomtech.com>]

On mer, 2018-07-11 at 12:01 +0200, Simon Goldschmidt wrote:
> 
> On 11.07.2018 11:48, Maxime Ripard wrote:
> > 
> > Hi,
> > 
> > On Wed, Jul 11, 2018 at 10:33:28AM +0200, Nicholas wrote:
> > > 
> > > 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@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.
> > Or worse, you might end up erasing something that shouldn't be
> > erased
> > on your board.
> > 
> > > 
> > > > 
> > > > 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.
> > 
> > > 
> > > 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.
> 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.

> - to which environment do we store if all environments failed to
> load 
> (currently, it seems we store to the lowest prio in this case?)
> 
This issue exists only in 1) and it results in 'not save' the
environment.
> 
> Simon

Regards,
Nicholas