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

[Simon Goldschmidt]

On 12.07.2018 09:20, Maxime Ripard wrote:
> On Thu, Jul 12, 2018 at 09:02:26AM +0200, Simon Goldschmidt wrote:
>>
>>
>> On 11.07.2018 15:50, Maxime Ripard wrote:
>>> On Wed, Jul 11, 2018 at 12:44:23PM +0200, Nicholas wrote:
>>>>>>> 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.
>>>
>>> In that case, we don't want to store to a lower priority, but to a
>>> higher one. Otherwise, the environment will be saved just fine, but
>>> will not be read next time, which will be very confusing to the user.
>>>
>>> And with a higher priority, you might end up overwriting something you
>>> weren't expecting to overwrite, so I'd vote 1.
>>
>> I agree that 1 would be best. But from reading the code, unless I'm totally
>> wrong, it seems that the patch sent by Nicholas does not suffice:
>>
>> If no location contained a valid environment (e.g. no environment written
>> yet), the lowest priority will be written as 'gd->env_load_location' is set
>> to the lowest priority from iterating locations in 'env_load()'.
>>
>> So we might have to reset 'gd->env_load_location' to highest prio if loading
>> the environment fails.
> 
> That would make sense yes.

Nicholas has sent v4 of this patch meanwhile, but it's not in reply to v1.

Any comments on that?

Simon