[U-Boot] [RFC PATCH 04/10] env: Pass additional parameters to the env lookup function

[Quentin Schulz]

Hi Maxime,

On 16/11/2017 10:22, 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             | 122 ++++++++++++++++++++++++++++++--------------------
>  include/environment.h |   7 +++
>  2 files changed, 80 insertions(+), 49 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 97ada5b5a6fd..673bfa6ba41b 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -26,8 +26,11 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
>  	return NULL;
>  }
>  
> -static enum env_location env_get_location(void)
> +static enum env_location env_get_location(enum env_operation op, int prio)
>  {
> +	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 +55,14 @@ static enum env_location env_get_location(void)
>  		return ENVL_UNKNOWN;
>  }
>  
> -static struct env_driver *env_driver_lookup(void)
> +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 +75,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(ENVO_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(ENVO_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(ENVO_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(ENVO_INIT, prio)); prio++) {
> +		if (!drv->init)
> +			continue;
>  
> -	if (!drv)
> -		return -ENODEV;
> -	if (drv->init)
>  		ret = drv->init();
> +		if (!ret)
> +			return 0;
> +

Shouldn't we init all drivers that can be found?

I think it is perfectly plausible that an env that you could init could
fail to save/load its environment. Then we would try to save/load from
the next one but with the current code, the next one wouldn't be
initialized.

Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com