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

[Maxime Ripard]

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

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.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180711/f20a41ce/attachment.sig>