| Message ID | 20200124133332.3.I42c79507524e5ad68e85fd60bbd686c4c59523ae@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] cmd: env: add option for quiet output on env info | expand |
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! > +++ 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. > --- 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. Best regards, Wolfgang Denk
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
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, + gd->env_load_prio); + if (ENVL_NOWHERE != loc && ENVL_UNKNOWN != loc) { + if (!quiet) + printf("Environment can be persisted\n"); + eval_results |= ENV_INFO_IS_PERSISTED; + } else { + if (!quiet) + printf("Environment cannot be persisted\n"); + } #else if (!quiet) printf("Environment cannot be persisted\n"); diff --git a/env/env.c b/env/env.c index 9237bb9c74..4e7a3c0c48 100644 --- a/env/env.c +++ 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) { if (prio >= ARRAY_SIZE(env_locations)) diff --git a/include/env_internal.h b/include/env_internal.h index 90a4df8a72..af29ff0cd6 100644 --- 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); + #endif /* DO_DEPS_ONLY */ #endif /* _ENV_INTERNAL_H_ */
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(-)