[PATCH 3/5] cmd: env: check real location for env info command

[Patrick DELAUNAY]

Hi Wolfgang,

> From: Wolfgang Denk <wd at denx.de>
> Sent: vendredi 24 janvier 2020 14:17
> 
> Dear Patrick Delaunay,
> 
> In message
> <20200124133332.3.I42c79507524e5ad68e85fd60bbd686c4c59523ae at changeid>
> you wrote:
> > Check the current ENV location, dynamically provided by the weak
> > function env_get_location to be sure that the environment can be
> > persistent.
> >
> > The compilation flag ENV_IS_IN_DEVICE is not enough when the board
> > dynamically select the available storage location (according boot
> > device for example).
> >
> > This patch solves issue for stm32mp1 platform, when the boot device is
> > USB.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> >
> >  cmd/nvedit.c           | 13 ++++++++++---
> >  env/env.c              | 18 ------------------
> >  include/env_internal.h | 20 ++++++++++++++++++++
> >  3 files changed, 30 insertions(+), 21 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 3d1054e763..a37b7c094a
> > 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -1269,9 +1269,16 @@ static int do_env_info(cmd_tbl_t *cmdtp, int flag,
> >  	/* evaluate whether environment can be persisted */
> >  	if (eval_flags & ENV_INFO_IS_PERSISTED) {  #if
> > defined(CONFIG_CMD_SAVEENV) && defined(ENV_IS_IN_DEVICE)
> > -		if (!quiet)
> > -			printf("Environment can be persisted\n");
> > -		eval_results |= ENV_INFO_IS_PERSISTED;
> > +		enum env_location loc = env_get_location(ENVOP_SAVE,
> 
> Please do not declare variables right in the middle of the code!

Yes, I will modify it....
I am surprised that this issue pas the check patch.
 
> 
> > +++ b/env/env.c
> > @@ -105,24 +105,6 @@ static void env_set_inited(enum env_location location)
> >  	gd->env_has_init |= BIT(location);
> >  }
> >
> > -/**
> > - * env_get_location() - Returns the best env location for a board
> > - * @op: operations performed on the environment
> > - * @prio: priority between the multiple environments, 0 being the
> > - *        highest priority
> > - *
> > - * This will return the preferred environment for the given priority.
> > - * This is overridable by boards if they need to.
> > - *
> > - * All implementations are free to use the operation, the priority
> > and
> > - * any other data relevant to their choice, but must take into
> > account
> > - * the fact that the lowest prority (0) is the most important
> > location
> > - * in the system. The following locations should be returned by order
> > - * of descending priorities, from the highest to the lowest priority.
> > - *
> > - * Returns:
> > - * an enum env_location value on success, a negative error code
> > otherwise
> > - */
> >  __weak enum env_location env_get_location(enum env_operation op, int
> > prio)
> 
> I think it is a really bad idea to remove the comment from the implementation.
> Please keep it here.

Yes I  agree .
I will come back on this par.
 
> > --- a/include/env_internal.h
> > +++ b/include/env_internal.h
> > @@ -209,6 +209,26 @@ struct env_driver {
> >
> >  extern struct hsearch_data env_htab;
> >
> > +/**
> > + * env_get_location() - Returns the best env location for a board
> > + * @op: operations performed on the environment
> > + * @prio: priority between the multiple environments, 0 being the
> > + *        highest priority
> > + *
> > + * This will return the preferred environment for the given priority.
> > + * This is overridable by boards if they need to.
> > + *
> > + * All implementations are free to use the operation, the priority
> > +and
> > + * any other data relevant to their choice, but must take into
> > +account
> > + * the fact that the lowest prority (0) is the most important
> > +location
> > + * in the system. The following locations should be returned by order
> > + * of descending priorities, from the highest to the lowest priority.
> > + *
> > + * Returns:
> > + * an enum env_location value on success, a negative error code
> > +otherwise  */ enum env_location env_get_location(enum env_operation
> > +op, int prio);
> 
> If absolutely necessary, copuy only what is needed for API documentation.

Ok

> 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 "This
> is a test of the Emergency Broadcast System. If this had been an actual
> emergency, do you really think we'd stick around to tell you?"

Thanks

Patrick