Re: [U-Boot] [PATCH 1/2] env: Fix env_load_location

[Simon Goldschmidt]

On 08.02.2018 09:38, Maxime Ripard wrote:
Hi,
Thanks for your patch

On Wed, Feb 07, 2018 at 02:17:11PM -0800, York Sun wrote:
Commit 7d714a24d725 ("env: Support multiple environments") added
static variable env_load_location. When saving environmental
variables, this variable is presumed to have the value set before.
In case the value was set before relocation and U-Boot runs from a
NOR flash, this variable wasn't writable. This causes failure when
saving the environment. To save this location, global data must be
used instead.

Out of curiosity: which linker sections are affected of this issue? I 
assume only initialized data ('.data') is affected as it is left in 
place. Also, I assume that uninitialized data ('.bss') is not affected 
(as your linker can place this into ram)?
If so, this issue could be fixed by changing ENVL_UNKNOWN to zero (which 
you already do in your patch) and leaving 'env_load_location' 
uninitialized, in which case it is initialized to zero (== ENVL_UNKNOWN) 
as defined by the C standard.

Another possibility to fix this would be to have your linker script put 
'data' into ram but initialize it from rom. Something like this:

.data :
{
  <your section contents here>
} >ram AT>rom

To me, both versions would be better than to add members to struct 
global_dat.

Signed-off-by: York Sun <york....@nxp.com>
CC: Maxime Ripard <maxime.rip...@free-electrons.com>
---
Limited test on LS1043ARDB.

  env/env.c                         | 8 +++-----
  include/asm-generic/global_data.h | 1 +
  include/environment.h             | 2 +-
  3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/env/env.c b/env/env.c
index 9a89832..edfb575 100644
--- a/env/env.c
+++ b/env/env.c
@@ -62,8 +62,6 @@ static enum env_location env_locations[] = {
  #endif
  };
  
-static enum env_location env_load_location = ENVL_UNKNOWN;
-
  static bool env_has_inited(enum env_location location)
  {
        return gd->env_has_init & BIT(location);
@@ -108,11 +106,11 @@ __weak enum env_location env_get_location(enum 
env_operation op, int prio)
                if (prio >= ARRAY_SIZE(env_locations))
                        return ENVL_UNKNOWN;
  
-		env_load_location = env_locations[prio];
-               return env_load_location;
+               gd->env_load_location = env_locations[prio];
+               return gd->env_load_location;
  
  	case ENVOP_SAVE:
-               return env_load_location;
+               return gd->env_load_location;
        }
  
  	return ENVL_UNKNOWN;
diff --git a/include/asm-generic/global_data.h 
b/include/asm-generic/global_data.h
index fd8cd45..10f1441 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -51,6 +51,7 @@ typedef struct global_data {
        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 */
+       int env_load_location;
  
  	unsigned long ram_top;		/* Top address of RAM used by U-Boot */
        unsigned long relocaddr;        /* Start address of U-Boot in RAM */
diff --git a/include/environment.h b/include/environment.h
index a406050..0f339da 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -188,6 +188,7 @@ enum env_valid {
  };
  
  enum env_location {
+       ENVL_UNKNOWN,
        ENVL_EEPROM,
        ENVL_EXT4,
        ENVL_FAT,
@@ -202,7 +203,6 @@ enum env_location {
        ENVL_NOWHERE,
  
  	ENVL_COUNT,
-       ENVL_UNKNOWN,
Why did you need to change this? This looks a bit odd.

The global data struct is initialized to zero. I guess this change is 
meant to ensure 'gd->env_load_location' is initialized to ENV_UNKOWN 
(see my comments above).

Simon


Thanks!
Maxime



_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot