From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicholas Date: Wed, 11 Jul 2018 10:33:28 +0200 Subject: [U-Boot] [RFC PATCH] u-boot: remove driver lookup loop from env_save() In-Reply-To: <01c13875-84e4-3923-9ca7-15932703df91@de.pepperl-fuchs.com> References: <1531227476-21591-1-git-send-email-nicholas.faustini@azcomtech.com> <01c13875-84e4-3923-9ca7-15932703df91@de.pepperl-fuchs.com> Message-ID: <1531298008.2477.30.camel@azcomtech.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > > 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 > > > > > > --- > > > > > >   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 > >