| Message ID | 1518041832-337-1-git-send-email-york.sun@nxp.com |
|---|---|
| State | Accepted |
| Commit | e1caa5841e8a9bc0ee658bdacae0519fa28e1e6a |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,1/2] env: Fix env_load_location | expand |
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. > > Signed-off-by: York Sun <york.sun@nxp.com> > CC: Maxime Ripard <maxime.ripard@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. Thanks! Maxime
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.sun@nxp.com> >> CC: Maxime Ripard <maxime.ripard@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
On 02/08/2018 02:05 AM, Simon Goldschmidt wrote: > 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. Leaving it as ENVL_UNKNOWN doesn't fix the problem. The env_load_location needs to be valid after relocation. Leaving it as unknown doesn't let you write env. > > 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. Not everything in initial RAM is copied to final RAM. After relocation, we shouldn't be using initial RAM at all. Actually the initial RAM may be used for other purpose. > >>> Signed-off-by: York Sun <york.sun@nxp.com> >>> CC: Maxime Ripard <maxime.ripard@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). Yes. That was my intention. York
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. > > Signed-off-by: York Sun <york.sun@nxp.com> > CC: Maxime Ripard <maxime.ripard@free-electrons.com> Applied to u-boot/master, thanks!
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, }; /* value for the various operations we want to perform on the env */
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. Signed-off-by: York Sun <york.sun@nxp.com> CC: Maxime Ripard <maxime.ripard@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(-)