[U-Boot] [PATCH 03/14] env: Pass additional parameters to the env lookup function

[Andre Przywara]

Hi Maxime,

On 28/11/17 10:24, 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.

I wonder if this is a bit too generic. Wouldn't a simple:
	bool is_save_op (or some more clever name)
be sufficient? At least this is actually all we need, if I understand
the code correctly.
But it's up to you, I can see advantages for both approaches.

> 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);

Did you deliberately remove this fallback from the new code below?

> -	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;

Is this change to the algorithm acceptable? Formerly we returned 0 on
finding a drv->load pointer to be NULL, now it's -ENODEV, plus we try to
find other sources first.
Or is that solved later somehow with the env_has_init bitmap?

> +
> +		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;

Same as for env_load().

Cheers,
Andre

> +
> +		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;
> +
> +		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 226e3ef2d23a..7fa8b98cc0db 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -215,6 +215,13 @@ enum env_location {
>  	ENVL_UNKNOWN,
>  };
>  
> +enum env_operation {
> +	ENVO_GET_CHAR,
> +	ENVO_INIT,
> +	ENVO_LOAD,
> +	ENVO_SAVE,
> +};
> +
>  struct env_driver {
>  	const char *name;
>  	enum env_location location;
>