From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Thu, 5 Apr 2018 12:48:08 +0200 Subject: [U-Boot] [PATCH v3 03/15] env: Pass additional parameters to the env lookup function In-Reply-To: References: Message-ID: <233f2b85-e3e9-1c7b-dc87-311871973754@de.pepperl-fuchs.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > --- > 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;