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

[Simon Goldschmidt]

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

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