[U-Boot] [PATCH v3 03/15] env: Pass additional parameters to the env lookup function
Simon Goldschmidt
sgoldschmidt at de.pepperl-fuchs.com
Thu Apr 5 10:48:08 UTC 2018
Sorry to warm up this old thread, but I think the implementation of
env_save does not really work on error:
On my board, I have the environment stored in QSPI. If saving fails,
env_save tries to continue to the next environment driver, but
env_driver_lookup/env_get_location always returns the same driver for
ENVOP_SAVE (gd->env_load_location).
The result is that if the env driver returns an error from its save
function, env_save hangs in an infinite loop.
Sorry I don't have a patch for this right now...
Simon
On 23.01.2018 21:16, Maxime Ripard wrote:
> In preparation for the multiple environment support, let's introduce two
> new parameters to the environment driver lookup function: the priority and
> operation.
>
> The operation parameter is meant to identify, obviously, the operation you
> might want to perform on the environment.
>
> The priority is a number passed to identify the environment priority you
> want to retrieve. The lowest priority parameter (0) will be the primary
> source.
>
> Combining the two parameters allow you to support multiple environments
> through different priorities, and to change those priorities between read
> and writes operations.
>
> This is especially useful to implement migration mechanisms where you want
> to always use the same environment first, be it to read or write, while the
> common case is more likely to use the same environment it has read from to
> write it to.
>
> Signed-off-by: Maxime Ripard <maxime.ripard at free-electrons.com>
> ---
> env/env.c | 157 +++++++++++++++++++++++++++++--------------
> include/environment.h | 8 ++-
> 2 files changed, 116 insertions(+), 49 deletions(-)
>
> diff --git a/env/env.c b/env/env.c
> index 97ada5b5a6fd..73da149fd8ca 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -26,8 +26,33 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> return NULL;
> }
>
> -static enum env_location env_get_location(void)
> +/**
> + * 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.
> + *
> + * 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
> + */
> +static enum env_location env_get_location(enum env_operation op, int prio)
> {
> + /*
> + * We support a single environment, so any environment asked
> + * with a priority that is not zero is out of our supported
> + * bounds.
> + */
> + if (prio >= 1)
> + return ENVL_UNKNOWN;
> +
> if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> return ENVL_EEPROM;
> else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> @@ -52,11 +77,27 @@ static enum env_location env_get_location(void)
> return ENVL_UNKNOWN;
> }
>
> -static struct env_driver *env_driver_lookup(void)
> +
> +/**
> + * env_driver_lookup() - Finds the most suited environment location
> + * @op: operations performed on the environment
> + * @prio: priority between the multiple environments, 0 being the
> + * highest priority
> + *
> + * This will try to find the available environment with the highest
> + * priority in the system.
> + *
> + * Returns:
> + * NULL on error, a pointer to a struct env_driver otherwise
> + */
> +static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
> {
> - enum env_location loc = env_get_location();
> + enum env_location loc = env_get_location(op, prio);
> struct env_driver *drv;
>
> + if (loc == ENVL_UNKNOWN)
> + return NULL;
> +
> drv = _env_driver_lookup(loc);
> if (!drv) {
> debug("%s: No environment driver for location %d\n", __func__,
> @@ -69,83 +110,101 @@ static struct env_driver *env_driver_lookup(void)
>
> int env_get_char(int index)
> {
> - struct env_driver *drv = env_driver_lookup();
> - int ret;
> + struct env_driver *drv;
> + int prio;
>
> if (gd->env_valid == ENV_INVALID)
> return default_environment[index];
> - if (!drv)
> - return -ENODEV;
> - if (!drv->get_char)
> - return *(uchar *)(gd->env_addr + index);
> - ret = drv->get_char(index);
> - if (ret < 0) {
> - debug("%s: Environment failed to load (err=%d)\n",
> - __func__, ret);
> +
> + for (prio = 0; (drv = env_driver_lookup(ENVOP_GET_CHAR, prio)); prio++) {
> + int ret;
> +
> + if (!drv->get_char)
> + continue;
> +
> + ret = drv->get_char(index);
> + if (!ret)
> + return 0;
> +
> + debug("%s: Environment %s failed to load (err=%d)\n", __func__,
> + drv->name, ret);
> }
>
> - return ret;
> + return -ENODEV;
> }
>
> int env_load(void)
> {
> - struct env_driver *drv = env_driver_lookup();
> - int ret = 0;
> + struct env_driver *drv;
> + int prio;
>
> - if (!drv)
> - return -ENODEV;
> - if (!drv->load)
> - return 0;
> - ret = drv->load();
> - if (ret) {
> - debug("%s: Environment failed to load (err=%d)\n", __func__,
> - ret);
> - return ret;
> + for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {
> + int ret;
> +
> + if (!drv->load)
> + continue;
> +
> + ret = drv->load();
> + if (!ret)
> + return 0;
> +
> + debug("%s: Environment %s failed to load (err=%d)\n", __func__,
> + drv->name, ret);
> }
>
> - return 0;
> + return -ENODEV;
> }
>
> int env_save(void)
> {
> - struct env_driver *drv = env_driver_lookup();
> - int ret;
> + struct env_driver *drv;
> + int prio;
>
> - if (!drv)
> - return -ENODEV;
> - if (!drv->save)
> - return -ENOSYS;
> -
> - printf("Saving Environment to %s...\n", drv->name);
> - ret = drv->save();
> - if (ret) {
> - debug("%s: Environment failed to save (err=%d)\n", __func__,
> - ret);
> - return ret;
> + for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
> + int ret;
> +
> + if (!drv->save)
> + continue;
> +
> + printf("Saving Environment to %s...\n", drv->name);
> + ret = drv->save();
> + if (!ret)
> + return 0;
> +
> + debug("%s: Environment %s failed to save (err=%d)\n", __func__,
> + drv->name, ret);
> }
>
> - return 0;
> + return -ENODEV;
> }
>
> int env_init(void)
> {
> - struct env_driver *drv = env_driver_lookup();
> + struct env_driver *drv;
> int ret = -ENOENT;
> + int prio;
> +
> + for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
> + if (!drv->init)
> + continue;
>
> - if (!drv)
> - return -ENODEV;
> - if (drv->init)
> ret = drv->init();
> + if (!ret)
> + return 0;
> +
> + debug("%s: Environment %s failed to init (err=%d)\n", __func__,
> + drv->name, ret);
> + }
> +
> + if (!prio)
> + return -ENODEV;
> +
> if (ret == -ENOENT) {
> gd->env_addr = (ulong)&default_environment[0];
> gd->env_valid = ENV_VALID;
>
> return 0;
> - } else if (ret) {
> - debug("%s: Environment failed to init (err=%d)\n", __func__,
> - ret);
> - return ret;
> }
>
> - return 0;
> + return ret;
> }
> diff --git a/include/environment.h b/include/environment.h
> index a2015c299aa9..a4060506fabb 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -205,6 +205,14 @@ enum env_location {
> ENVL_UNKNOWN,
> };
>
> +/* value for the various operations we want to perform on the env */
> +enum env_operation {
> + ENVOP_GET_CHAR, /* we want to call the get_char function */
> + ENVOP_INIT, /* we want to call the init function */
> + ENVOP_LOAD, /* we want to call the load function */
> + ENVOP_SAVE, /* we want to call the save function */
> +};
> +
> struct env_driver {
> const char *name;
> enum env_location location;
More information about the U-Boot
mailing list