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

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

[Nicholas Faustini]

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.

The env_save() function should not loop through the environment
location list but it should use the previously discovered
environment driver once.

Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com>
---

 env/env.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/env/env.c b/env/env.c
index 5c0842a..897d197 100644
--- a/env/env.c
+++ b/env/env.c
@@ -211,16 +211,16 @@ int env_load(void)
 int env_save(void)
 {
 	struct env_driver *drv;
-	int prio;
 
-	for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
+	drv = env_driver_lookup(ENVOP_SAVE, 0);
+	if (drv) {
 		int ret;
 
 		if (!drv->save)
-			continue;
+			return -ENODEV;
 
 		if (!env_has_inited(drv->location))
-			continue;
+			return -ENODEV;
 
 		printf("Saving Environment to %s... ", drv->name);
 		ret = drv->save();
-- 
2.7.4

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

[Simon Glass]

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?

>
> The env_save() function should not loop through the environment
> location list but it should use the previously discovered
> environment driver once.

What is that? It should be possible to write the env to multiple
places. Also what is the previously discovered environment?

>
> Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com>
> ---
>
>  env/env.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/env/env.c b/env/env.c
> index 5c0842a..897d197 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -211,16 +211,16 @@ int env_load(void)
>  int env_save(void)
>  {
>         struct env_driver *drv;
> -       int prio;
>
> -       for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
> +       drv = env_driver_lookup(ENVOP_SAVE, 0);
> +       if (drv) {
>                 int ret;
>
>                 if (!drv->save)
> -                       continue;
> +                       return -ENODEV;
>
>                 if (!env_has_inited(drv->location))
> -                       continue;
> +                       return -ENODEV;
>
>                 printf("Saving Environment to %s... ", drv->name);
>                 ret = drv->save();
> --
> 2.7.4
>

Regards,
Simon

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

[Simon Goldschmidt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8-sig", Size: 3751 bytes --]

Hi,


Pepperl+Fuchs GmbH, Mannheim
Geschaeftsfuehrer/Managing Directors: Dr.-Ing. Gunther Kegel (Vors./CEO), Werner Guthier, Mehmet Hatiboglu
Vorsitzender des Aufsichtsrats/Chairman of the supervisory board: Michael Fuchs sen.
Registergericht/Register Court: AG Mannheim HRB 4713
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.

Maybe the 'env save' command might need a parameter the tells it where 
to save?

Regards,
Simon (Goldschmidt)

> 
>>
>> The env_save() function should not loop through the environment
>> location list but it should use the previously discovered
>> environment driver once.
> 
> What is that? It should be possible to write the env to multiple
> places. Also what is the previously discovered environment?
> 
>>
>> Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com>
>> ---
>>
>>   env/env.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/env/env.c b/env/env.c
>> index 5c0842a..897d197 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -211,16 +211,16 @@ int env_load(void)
>>   int env_save(void)
>>   {
>>          struct env_driver *drv;
>> -       int prio;
>>
>> -       for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
>> +       drv = env_driver_lookup(ENVOP_SAVE, 0);
>> +       if (drv) {
>>                  int ret;
>>
>>                  if (!drv->save)
>> -                       continue;
>> +                       return -ENODEV;
>>
>>                  if (!env_has_inited(drv->location))
>> -                       continue;
>> +                       return -ENODEV;
>>
>>                  printf("Saving Environment to %s... ", drv->name);
>>                  ret = drv->save();
>> --
>> 2.7.4
>>
> 
> Regards,
> Simon
> 

Wichtiger Hinweis:
Diese E-Mail einschliesslich ihrer Anhaenge enthaelt vertrauliche und rechtlich geschuetzte Informationen, die nur fuer den Adressaten bestimmt sind. 
Sollten Sie nicht der bezeichnete Adressat sein, so teilen Sie dies bitte dem Absender umgehend mit und loeschen Sie diese Nachricht und ihre Anhaenge. Die unbefugte Weitergabe, das Anfertigen von Kopien und jede Veraenderung der E-Mail ist untersagt. Der Absender haftet nicht fuer Inhalte von veraenderten E-Mails.


Important Information:
This e-mail message including its attachments contains confidential and legally protected information solely intended for the addressee. If you are not the intended addressee of this message, please contact the addresser immediately and delete this message including its attachments. The unauthorized dissemination, copying and change of this e-mail are strictly forbidden. The addresser shall not be liable for the content of such changed e-mails.

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

[Simon Goldschmidt]

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.

Maybe the 'env save' command might need a parameter the tells it where 
to save?

Regards,
Simon (Goldschmidt)

> 
>>
>> The env_save() function should not loop through the environment
>> location list but it should use the previously discovered
>> environment driver once.
> 
> What is that? It should be possible to write the env to multiple
> places. Also what is the previously discovered environment?
> 
>>
>> Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com>
>> ---
>>
>>   env/env.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/env/env.c b/env/env.c
>> index 5c0842a..897d197 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -211,16 +211,16 @@ int env_load(void)
>>   int env_save(void)
>>   {
>>          struct env_driver *drv;
>> -       int prio;
>>
>> -       for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
>> +       drv = env_driver_lookup(ENVOP_SAVE, 0);
>> +       if (drv) {
>>                  int ret;
>>
>>                  if (!drv->save)
>> -                       continue;
>> +                       return -ENODEV;
>>
>>                  if (!env_has_inited(drv->location))
>> -                       continue;
>> +                       return -ENODEV;
>>
>>                  printf("Saving Environment to %s... ", drv->name);
>>                  ret = drv->save();
>> --
>> 2.7.4
>>
> 
> Regards,
> Simon
> 

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

[Lukasz Majewski]

Hi Simon,

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

I can confirm that there is a problem as described here - when you fail
to write to eMMC device (like e.g. 2, not 0), then we loop forever.

I do see such situation with Odroid XU3 when switching to BLK.

> Do we need a limit on
> prio?
> 
> >
> > The env_save() function should not loop through the environment
> > location list but it should use the previously discovered
> > environment driver once.  
> 
> What is that? It should be possible to write the env to multiple
> places. Also what is the previously discovered environment?
> 
> >
> > Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com>
> > ---
> >
> >  env/env.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/env/env.c b/env/env.c
> > index 5c0842a..897d197 100644
> > --- a/env/env.c
> > +++ b/env/env.c
> > @@ -211,16 +211,16 @@ int env_load(void)
> >  int env_save(void)
> >  {
> >         struct env_driver *drv;
> > -       int prio;
> >
> > -       for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio));
> > prio++) {
> > +       drv = env_driver_lookup(ENVOP_SAVE, 0);
> > +       if (drv) {
> >                 int ret;
> >
> >                 if (!drv->save)
> > -                       continue;
> > +                       return -ENODEV;
> >
> >                 if (!env_has_inited(drv->location))
> > -                       continue;
> > +                       return -ENODEV;
> >
> >                 printf("Saving Environment to %s... ", drv->name);
> >                 ret = drv->save();
> > --
> > 2.7.4
> >  
> 
> Regards,
> Simon




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180711/028ac2f8/attachment.sig>

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

[Nicholas]

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.

> Maybe the 'env save' command might need a parameter the tells it
> where 
> to save?
> 
> Regards,
> Simon (Goldschmidt)
> 

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.

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.

> > 
> > 
> > > 
> > > 
> > > The env_save() function should not loop through the environment
> > > location list but it should use the previously discovered
> > > environment driver once.
> > What is that? It should be possible to write the env to multiple
> > places. Also what is the previously discovered environment?
> > 
> > > 
> > > 
> > > Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com
> > > >
> > > ---
> > > 
> > >   env/env.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/env/env.c b/env/env.c
> > > index 5c0842a..897d197 100644
> > > --- a/env/env.c
> > > +++ b/env/env.c
> > > @@ -211,16 +211,16 @@ int env_load(void)
> > >   int env_save(void)
> > >   {
> > >          struct env_driver *drv;
> > > -       int prio;
> > > 
> > > -       for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE,
> > > prio)); prio++) {
> > > +       drv = env_driver_lookup(ENVOP_SAVE, 0);
> > > +       if (drv) {
> > >                  int ret;
> > > 
> > >                  if (!drv->save)
> > > -                       continue;
> > > +                       return -ENODEV;
> > > 
> > >                  if (!env_has_inited(drv->location))
> > > -                       continue;
> > > +                       return -ENODEV;
> > > 
> > >                  printf("Saving Environment to %s... ", drv-
> > > >name);
> > >                  ret = drv->save();
> > > --
> > > 2.7.4
> > > 
> > Regards,
> > Simon
> > 

[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@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>

[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@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

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

[Nicholas]

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

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

[Wolfgang Denk]

Dear Maxime,

In message <20180711094838.zyczks6iksp772ir@flea> you wrote:
> 
> 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.

If the feature is available, use cases will spring into mind.

Assume you have a eMMC based system which can also boot from other
storage (say, SDCard), and you want to write the current content of
the eMMC environment to the alternative boot device.

Yes, there are usually alternative approaches to perform similar
actions, but an easy way to save lsewhere might be handy, especially
if it comes at low cost.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
grep me no patterns and I'll tell you no lines.

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

[Maxime Ripard]

On Wed, Jul 11, 2018 at 12:49:50PM +0200, Wolfgang Denk wrote:
> Dear Maxime,
> 
> In message <20180711094838.zyczks6iksp772ir@flea> you wrote:
> > 
> > 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.
> 
> If the feature is available, use cases will spring into mind.
> 
> Assume you have a eMMC based system which can also boot from other
> storage (say, SDCard), and you want to write the current content of
> the eMMC environment to the alternative boot device.
> 
> Yes, there are usually alternative approaches to perform similar
> actions, but an easy way to save lsewhere might be handy, especially
> if it comes at low cost.

Right, but that would bring a much more significant rework as to how
the environments are handled than just the changes that are being
discussed here. At the moment, the environment can only be stored on
one device for each "types" (one raw MMC, one MTD partition, etc). The
redundancy allows you to duplicate it, but you won't be able to store
on just one instance of your choosing at the moment.

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/67b7e1f8/attachment.sig>

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

[Maxime Ripard]

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.

-- 
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/57501e79/attachment.sig>

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

[Wolfgang Denk]

Dear Maxime,

In message <20180711134735.vhhc7quuzcp42bo7@flea> you wrote:
> 
> > If the feature is available, use cases will spring into mind.
...
>
> Right, but that would bring a much more significant rework as to how
> the environments are handled than just the changes that are being
> discussed here. At the moment, the environment can only be stored on
> one device for each "types" (one raw MMC, one MTD partition, etc). The
> redundancy allows you to duplicate it, but you won't be able to store
> on just one instance of your choosing at the moment.

I am aware of this.  However, if we can pave the way for this to be
added later at little or no additional cost, should we not try that?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The only person who always got his work done by Friday
                                                 was Robinson Crusoe.

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

[Simon Goldschmidt]

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.


Simon

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

[Maxime Ripard]

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.

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/20180712/4ca759e2/attachment.sig>

[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