[03/14] env: Pass additional parameters to the env lookup function

Message ID 275d9901f239926baf949612e99bae437fc1836f.1511864667.git-series.maxime.ripard@free-electrons.com
State Superseded
Headers show
Series
  • env: Multiple env support and env transition for sunxi
Related show

Commit Message

Maxime Ripard Nov. 28, 2017, 10:24 a.m.
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@free-electrons.com>
---
 env/env.c             | 122 +++++++++++++++++++++++++------------------
 include/environment.h |   7 ++-
 2 files changed, 80 insertions(+), 49 deletions(-)

Comments

André Przywara Dec. 5, 2017, 10:09 a.m. | #1
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@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;
>
Maxime Ripard Dec. 8, 2017, 8:25 a.m. | #2
Hi Andre,

On Tue, Dec 05, 2017 at 10:09:04AM +0000, Andre Przywara wrote:
> 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.


I'm not really sure what you meant, but I'll try to explain the
various constraints we have:

  - We need to init all the environments, no matter their priority,
    without any particular ordering

  - We need to load the environments by descending order of priority,
    until a valid one is found. I guess get_char is in the same
    situation, but I'm not really sure what it's supposed to be doing.

  - We need to store the environments. Now, this gets tricky. The
    current code only implement two cases: you store the environment
    where you loaded it from, which is the current case, and forcing
    always the same order, like we did on sunxi.

    But you might envision without pulling your hairs too much a
    situation for example that we would have loaded our environment
    from a read-only location (either because it is physically
    read-only, or because you cannot modify it (ie. signed
    environments)). In this case, you will need to implement a
    fallback strategy that is probably going to be by board, probably
    along the line of trying to save in our environments by ascending
    order, starting with the environment we loaded from.

tl; dr: we might need that flexibility for future use cases that are
not clear yet.

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


I couldn't really find a better way to deal with this.

You're right that the current algorithm is that if we don't have a
driver, return ENODEV, and if we don't have a load function, return 0.

When dealing with multiple enviroments, you would obviously have to
return -ENODEV if you didn't find any, but I couldn't find a proper
way to decide when we would return 0: should we return 0 when no
environment has a load function? only the first one we encounter?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Simon Glass Dec. 29, 2017, 3:13 a.m. | #3
Hi Maxime,

On 28 November 2017 at 03:24, Maxime Ripard
<maxime.ripard@free-electrons.com> 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@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)

Can you please document the function args? The operation of prio needs
to be described somewhere.

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

Why is this needed? Shouldn't _env_driver_lookup() return NULL in this
case anyway?

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

So we get no advice of error? I think it should print a message here,
like ...failed or ...done.

Also if no driver succeeded I suggest returning one of the errors
returned from drv-save().

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

comment here

> +enum env_operation {
> +       ENVO_GET_CHAR,

I think ENVOP would be a better prefix.

> +       ENVO_INIT,
> +       ENVO_LOAD,
> +       ENVO_SAVE,
> +};
> +
>  struct env_driver {
>         const char *name;
>         enum env_location location;
> --
> git-series 0.9.1

Regards,
Simon
Maxime Ripard Jan. 5, 2018, 9:27 a.m. | #4
Hi Simon,

On Thu, Dec 28, 2017 at 08:13:23PM -0700, Simon Glass wrote:
> > -static struct env_driver *env_driver_lookup(void)

> > +static struct env_driver *env_driver_lookup(enum env_operation op, int prio)

> 

> Can you please document the function args? The operation of prio needs

> to be described somewhere.


Will do.

> >  {

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

> 

> Why is this needed? Shouldn't _env_driver_lookup() return NULL in this

> case anyway?


I just thought that making it obvious and explicit (especially when
the behaviour of _env_driver_lookup might change) would be a good
thing, but I can remove it if you want.

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

> 

> So we get no advice of error? I think it should print a message here,

> like ...failed or ...done.


This is addressed in the next patch

> Also if no driver succeeded I suggest returning one of the errors

> returned from drv-save().


And this was one of the comments made in this version, so it's going
to be addressed in the next iteration.

> > --- a/include/environment.h

> > +++ b/include/environment.h

> > @@ -215,6 +215,13 @@ enum env_location {

> >         ENVL_UNKNOWN,

> >  };

> >

> 

> comment here

> 

> > +enum env_operation {

> > +       ENVO_GET_CHAR,

> 

> I think ENVOP would be a better prefix.


That works for me. Thanks!
Maxime

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

Patch

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