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.faust...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
