[10/14] env: Initialise all the environments

Message ID ea9bdc96998b88069eb95a0f79eec5b338e37a54.1511864667.git-series.maxime.ripard@free-electrons.com
State New
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.
Since we want to have multiple environments, we will need to initialise
all the environments since we don't know at init time what drivers might
fail when calling load.

Let's init all of them, and only consider for further operations the ones
that have not reported any errors at init time.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c                         | 19 ++++++++++++-------
 include/asm-generic/global_data.h |  1 +
 2 files changed, 13 insertions(+), 7 deletions(-)

Comments

Quentin Schulz Nov. 28, 2017, 12:24 p.m. | #1
Hi Maxime,

On 28/11/2017 11:24, Maxime Ripard wrote:
> Since we want to have multiple environments, we will need to initialise
> all the environments since we don't know at init time what drivers might
> fail when calling load.
> 
> Let's init all of them, and only consider for further operations the ones
> that have not reported any errors at init time.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c                         | 19 ++++++++++++-------
>  include/asm-generic/global_data.h |  1 +
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 5176700133d3..b4d8886e7a69 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -112,6 +112,9 @@ int env_get_char(int index)
>  		if (!drv->get_char)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		ret = drv->get_char(index);
>  		if (!ret)
>  			return 0;
> @@ -134,6 +137,9 @@ int env_load(void)
>  		if (!drv->load)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		printf("Loading Environment from %s... ", drv->name);
>  		ret = drv->load();
>  		printf("%s\n", ret ? "Failed" : "OK");
> @@ -155,6 +161,9 @@ int env_save(void)
>  		if (!drv->save)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		printf("Saving Environment to %s... ", drv->name);
>  		ret = drv->save();
>  		printf("%s\n", ret ? "Failed" : "OK");
> @@ -175,14 +184,10 @@ int env_init(void)
>  	int prio;
>  
>  	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
> -		if (!drv->init)
> -			continue;
> -
> -		ret = drv->init();
> -		if (!ret)
> -			return 0;
> +		if (!drv->init || !drv->init())
> +			gd->env_has_init |= BIT(drv->location);
>  
> -		debug("%s: Environment %s failed to init (err=%d)\n", __func__,
> +		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>  		      drv->name, ret);

You're not setting ret.

And you're printing that init env is always successful, and that's not
always the case as drv could have an init and call to drv->init() could
return an error code.

Thanks,
Quentin
Maxime Ripard Nov. 28, 2017, 12:29 p.m. | #2
On Tue, Nov 28, 2017 at 01:24:45PM +0100, Quentin Schulz wrote:
> Hi Maxime,

> 

> On 28/11/2017 11:24, Maxime Ripard wrote:

> > Since we want to have multiple environments, we will need to initialise

> > all the environments since we don't know at init time what drivers might

> > fail when calling load.

> > 

> > Let's init all of them, and only consider for further operations the ones

> > that have not reported any errors at init time.

> > 

> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> > ---

> >  env/env.c                         | 19 ++++++++++++-------

> >  include/asm-generic/global_data.h |  1 +

> >  2 files changed, 13 insertions(+), 7 deletions(-)

> > 

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

> > index 5176700133d3..b4d8886e7a69 100644

> > --- a/env/env.c

> > +++ b/env/env.c

> > @@ -112,6 +112,9 @@ int env_get_char(int index)

> >  		if (!drv->get_char)

> >  			continue;

> >  

> > +		if (!(gd->env_has_init & BIT(drv->location)))

> > +			continue;

> > +

> >  		ret = drv->get_char(index);

> >  		if (!ret)

> >  			return 0;

> > @@ -134,6 +137,9 @@ int env_load(void)

> >  		if (!drv->load)

> >  			continue;

> >  

> > +		if (!(gd->env_has_init & BIT(drv->location)))

> > +			continue;

> > +

> >  		printf("Loading Environment from %s... ", drv->name);

> >  		ret = drv->load();

> >  		printf("%s\n", ret ? "Failed" : "OK");

> > @@ -155,6 +161,9 @@ int env_save(void)

> >  		if (!drv->save)

> >  			continue;

> >  

> > +		if (!(gd->env_has_init & BIT(drv->location)))

> > +			continue;

> > +

> >  		printf("Saving Environment to %s... ", drv->name);

> >  		ret = drv->save();

> >  		printf("%s\n", ret ? "Failed" : "OK");

> > @@ -175,14 +184,10 @@ int env_init(void)

> >  	int prio;

> >  

> >  	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {

> > -		if (!drv->init)

> > -			continue;

> > -

> > -		ret = drv->init();

> > -		if (!ret)

> > -			return 0;

> > +		if (!drv->init || !drv->init())

> > +			gd->env_has_init |= BIT(drv->location);

> >  

> > -		debug("%s: Environment %s failed to init (err=%d)\n", __func__,

> > +		debug("%s: Environment %s init done (ret=%d)\n", __func__,

> >  		      drv->name, ret);

> 

> You're not setting ret.


Ah, right.

> And you're printing that init env is always successful


Not really. The message says that it's been done, not that it's been
done successfully.

> and that's not always the case as drv could have an init and call to

> drv->init() could return an error code.


Right, but I'm not really sure what could be done about that error
code beside printing it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Andre Przywara Dec. 5, 2017, 10:11 a.m. | #3
Hi,

On 28/11/17 10:24, Maxime Ripard wrote:
> Since we want to have multiple environments, we will need to initialise
> all the environments since we don't know at init time what drivers might
> fail when calling load.
> 
> Let's init all of them, and only consider for further operations the ones
> that have not reported any errors at init time.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c                         | 19 ++++++++++++-------
>  include/asm-generic/global_data.h |  1 +
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/env/env.c b/env/env.c
> index 5176700133d3..b4d8886e7a69 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -112,6 +112,9 @@ int env_get_char(int index)
>  		if (!drv->get_char)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		ret = drv->get_char(index);
>  		if (!ret)
>  			return 0;
> @@ -134,6 +137,9 @@ int env_load(void)
>  		if (!drv->load)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		printf("Loading Environment from %s... ", drv->name);
>  		ret = drv->load();
>  		printf("%s\n", ret ? "Failed" : "OK");
> @@ -155,6 +161,9 @@ int env_save(void)
>  		if (!drv->save)
>  			continue;
>  
> +		if (!(gd->env_has_init & BIT(drv->location)))
> +			continue;
> +
>  		printf("Saving Environment to %s... ", drv->name);
>  		ret = drv->save();
>  		printf("%s\n", ret ? "Failed" : "OK");
> @@ -175,14 +184,10 @@ int env_init(void)
>  	int prio;
>  

Becoming a bit desperate now, because I am struggling to find actual
issues in this series ;-), so:

Maybe we should have a build time check should the env_locations array
ever grow beyond 32?
	BUILD_BUG_ON(ARRAY_SIZE(env_locations) > BITS_PER_LONG);

>  	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
> -		if (!drv->init)
> -			continue;
> -
> -		ret = drv->init();
> -		if (!ret)
> -			return 0;
> +		if (!drv->init || !drv->init())
> +			gd->env_has_init |= BIT(drv->location);
>  
> -		debug("%s: Environment %s failed to init (err=%d)\n", __func__,
> +		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>  		      drv->name, ret);
>  	}
>  
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 944f58195caf..1d0611fe9498 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -50,6 +50,7 @@ typedef struct global_data {
>  #endif
>  	unsigned long env_addr;		/* Address  of Environment struct */
>  	unsigned long env_valid;	/* Environment valid? enum env_valid */
> +	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */

struct env_location? Do you mean the env_location array? Or the enum?

Cheers,
Andre.

>  	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
>  	unsigned long relocaddr;	/* Start address of U-Boot in RAM */
>
Simon Glass Dec. 29, 2017, 3:13 a.m. | #4
Hi Maxime,

On 28 November 2017 at 03:24, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Since we want to have multiple environments, we will need to initialise
> all the environments since we don't know at init time what drivers might
> fail when calling load.
>
> Let's init all of them, and only consider for further operations the ones
> that have not reported any errors at init time.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c                         | 19 ++++++++++++-------
>  include/asm-generic/global_data.h |  1 +
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/env/env.c b/env/env.c
> index 5176700133d3..b4d8886e7a69 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -112,6 +112,9 @@ int env_get_char(int index)
>                 if (!drv->get_char)
>                         continue;
>
> +               if (!(gd->env_has_init & BIT(drv->location)))
> +                       continue;
> +

Little nit. Can you please put the access in functions like
env_has_inited() and env_set_inited()? This allows us to change the
implementation later, and also it more self-documenting.

>                 ret = drv->get_char(index);
>                 if (!ret)
>                         return 0;
> @@ -134,6 +137,9 @@ int env_load(void)
>                 if (!drv->load)
>                         continue;
>
> +               if (!(gd->env_has_init & BIT(drv->location)))
> +                       continue;
> +
>                 printf("Loading Environment from %s... ", drv->name);
>                 ret = drv->load();
>                 printf("%s\n", ret ? "Failed" : "OK");
> @@ -155,6 +161,9 @@ int env_save(void)
>                 if (!drv->save)
>                         continue;
>
> +               if (!(gd->env_has_init & BIT(drv->location)))
> +                       continue;
> +
>                 printf("Saving Environment to %s... ", drv->name);
>                 ret = drv->save();
>                 printf("%s\n", ret ? "Failed" : "OK");
> @@ -175,14 +184,10 @@ int env_init(void)
>         int prio;
>
>         for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
> -               if (!drv->init)
> -                       continue;
> -
> -               ret = drv->init();
> -               if (!ret)
> -                       return 0;
> +               if (!drv->init || !drv->init())
> +                       gd->env_has_init |= BIT(drv->location);
>
> -               debug("%s: Environment %s failed to init (err=%d)\n", __func__,
> +               debug("%s: Environment %s init done (ret=%d)\n", __func__,
>                       drv->name, ret);
>         }
>
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 944f58195caf..1d0611fe9498 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -50,6 +50,7 @@ typedef struct global_data {
>  #endif
>         unsigned long env_addr;         /* Address  of Environment struct */
>         unsigned long env_valid;        /* Environment valid? enum env_valid */
> +       unsigned long env_has_init;     /* Bitmask of boolean of struct env_location offsets */
>
>         unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>         unsigned long relocaddr;        /* Start address of U-Boot in RAM */
> --
> git-series 0.9.1
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Regards,
Simon

Patch

diff --git a/env/env.c b/env/env.c
index 5176700133d3..b4d8886e7a69 100644
--- a/env/env.c
+++ b/env/env.c
@@ -112,6 +112,9 @@  int env_get_char(int index)
 		if (!drv->get_char)
 			continue;
 
+		if (!(gd->env_has_init & BIT(drv->location)))
+			continue;
+
 		ret = drv->get_char(index);
 		if (!ret)
 			return 0;
@@ -134,6 +137,9 @@  int env_load(void)
 		if (!drv->load)
 			continue;
 
+		if (!(gd->env_has_init & BIT(drv->location)))
+			continue;
+
 		printf("Loading Environment from %s... ", drv->name);
 		ret = drv->load();
 		printf("%s\n", ret ? "Failed" : "OK");
@@ -155,6 +161,9 @@  int env_save(void)
 		if (!drv->save)
 			continue;
 
+		if (!(gd->env_has_init & BIT(drv->location)))
+			continue;
+
 		printf("Saving Environment to %s... ", drv->name);
 		ret = drv->save();
 		printf("%s\n", ret ? "Failed" : "OK");
@@ -175,14 +184,10 @@  int env_init(void)
 	int prio;
 
 	for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) {
-		if (!drv->init)
-			continue;
-
-		ret = drv->init();
-		if (!ret)
-			return 0;
+		if (!drv->init || !drv->init())
+			gd->env_has_init |= BIT(drv->location);
 
-		debug("%s: Environment %s failed to init (err=%d)\n", __func__,
+		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
 	}
 
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 944f58195caf..1d0611fe9498 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -50,6 +50,7 @@  typedef struct global_data {
 #endif
 	unsigned long env_addr;		/* Address  of Environment struct */
 	unsigned long env_valid;	/* Environment valid? enum env_valid */
+	unsigned long env_has_init;	/* Bitmask of boolean of struct env_location offsets */
 
 	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
 	unsigned long relocaddr;	/* Start address of U-Boot in RAM */