diff mbox series

[v2,03/15] env: Pass additional parameters to the env lookup function

Message ID 4c663f0b6b4bd6f8c15f23527ff02d7e9eebb2ac.1516094113.git-series.maxime.ripard@free-electrons.com
State Superseded
Headers show
Series env: Multiple env support and env transition for sunxi | expand

Commit Message

Maxime Ripard Jan. 16, 2018, 9:16 a.m. UTC 
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             | 135 +++++++++++++++++++++++++++----------------
 include/environment.h |   8 +++-
 2 files changed, 94 insertions(+), 49 deletions(-)

Comments

Simon Glass Jan. 17, 2018, 10:03 p.m. UTC | #1 
Hi Maxime,

On 16 January 2018 at 01:16, 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             | 135 +++++++++++++++++++++++++++----------------
>  include/environment.h |   8 +++-
>  2 files changed, 94 insertions(+), 49 deletions(-)
>
> diff --git a/env/env.c b/env/env.c
> index 97ada5b5a6fd..4dc39b384c1e 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)

Please add a function comment, including why @op is needed.

>  {
> +       if (prio >= 1)
> +               return ENVL_UNKNOWN;

What is this for? Can you please add a comment?

Regards,
Simon
Maxime Ripard Jan. 18, 2018, 2:54 p.m. UTC | #2 
On Wed, Jan 17, 2018 at 03:03:40PM -0700, Simon Glass wrote:
> Hi Maxime,

> 

> On 16 January 2018 at 01:16, 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             | 135 +++++++++++++++++++++++++++----------------

> >  include/environment.h |   8 +++-

> >  2 files changed, 94 insertions(+), 49 deletions(-)

> >

> > diff --git a/env/env.c b/env/env.c

> > index 97ada5b5a6fd..4dc39b384c1e 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)

> 

> Please add a function comment, including why @op is needed.


This was done in a later patch (the __weak one), but you're right that
it makes more sense for it to be here.

> 

> >  {

> > +       if (prio >= 1)

> > +               return ENVL_UNKNOWN;

> 

> What is this for? Can you please add a comment?


Done, thanks!
Maxime

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

Patch

diff --git a/env/env.c b/env/env.c
index 97ada5b5a6fd..4dc39b384c1e 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,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 +88,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;