Introduce variables whitelisting in environment
cover Introduce variables whitelisting in environment
This patch series is based on this[1] patch series from Maxime.
This is an RFC. It's been only tested in a specific use case on a custom i.MX6 board. It's known to break compilation on a few boards.
I have a use case where we want some variables from a first environment to be overriden by variables from a second environment. For example, we want to load variables from the default env (ENV_IS_NOWHERE) and then load only a handful of other variables from, e.g., NAND.
In our use case, we basically can be sure that the default env in the U-Boot binary is secure but we want only a few variables to be modified, thus keeping control over the overall behaviour of U-Boot in secure mode.
It works in that way:
- from highest to lowest priority, the first environment that can be loaded (that has successfully init and whose load function has returned no errors) will be the main environment,
- then, all the following environment that could be successfully loaded (same conditions as the main environment) are secondary environment. The env variables that are defined both in CONFIG_ENV_VAR_WHITELIST_LIST and in the secondary environments override the ones in the main environment,
- for saving, we save the whole environment to all environments available, be they main or secondary (it does not matter to save the whole environment on secondary environments as only the whitelisted variables will be overriden in the loading process,
I have also a few questions that could help me to get the whole thing to work.
1) I can't really get my head around the use of gd->env_addr, what is it used for? It is set in a bunch of different places but only once is it explicitly used (basically to alternate the env_addr between the one associated to main and redundant environment (in NAND for example)).
2) Why do we consider ENV_IS_NOWHERE an invalid environment? The only place I found a use for it was to just say that if the environment is invalid, we should set to default environment (in env_relocate in env/common.c). With my patch series I guess that we could remove this fallback and force ENV_IS_NOWHERE to be always there.
3) There are a few (20) boards that set gd->env_addr and gd->env_valid in their board file. What is the reason to do such a thing? Isn't those overriden anyway by the environment driver?
I'm looking forward to getting your feedback on this patch series.
Thanks,
Quentin
[1] https://patchwork.ozlabs.org/cover/842057/
Quentin Schulz (11):
env: fix ret not being set and fails when no env could have been init
lib: hashtable: support whitelisting env variables
env: add support for whitelisting variables from secondary environments
env: make nowhere an env medium like the others
cmd: saveenv: enable the saveenv command when ENV_IS_NOWHERE is defined but another env medium is enabled too
env: add env_driver to env_driver functions' arguments
env: gd flags without ENV_READY is enough to discriminate in env_get_default
env: add env_driver parameter to env_import_redund
env: make env_locations a global variable
env: introducing env_info struct for storing info per env
env: store information about each environment in gd
board/sunxi/board.c | 2 +- cmd/nvedit.c | 16 ++- common/board_r.c | 8 +- env/Kconfig | 29 +++--- env/common.c | 45 ++++++---- env/eeprom.c | 40 ++++----- env/env.c | 142 +++++++++++++++++++++++++------ env/ext4.c | 4 +- env/fat.c | 4 +- env/flash.c | 58 ++++++------- env/mmc.c | 14 +-- env/nand.c | 46 +++++----- env/nowhere.c | 12 ++- env/nvram.c | 18 ++-- env/onenand.c | 6 +- env/remote.c | 10 +- env/sata.c | 4 +- env/sf.c | 34 +++---- env/ubi.c | 14 +-- include/asm-generic/global_data.h | 5 +- include/environment.h | 59 ++++++++----- include/search.h | 2 +- lib/hashtable.c | 17 +++- 23 files changed, 379 insertions(+), 210 deletions(-)
base-commit: 5d41e28058e7b378c9fa5c61ecc074a682ba2db4
Signature · 1 line
git-series 0.9.1
PATCH 1/11 env: fix ret not being set and fails when no env could have been init
This patch is fixing things that most likely will be done by Maxime in the v2 of his patch series for multiple environments support, one way or the other.
--- env/env.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
env/env.c +11 -10
@@ -182,24 +182,25 @@ int env_init(void)struct env_driver *drv;int ret = -ENOENT ;int prio; +bool init = false;for (prio =0 ; (drv =env_driver_lookup (ENVO_INIT , prio)); prio++) { -if (!drv->init || !drv->init ()) +if (drv->init) + ret = drv->init (drv); +else + ret =0 ; + +if (!ret) { gd->env_has_init |=BIT (drv->location); + init = true; + }debug ("%s: Environment %s init done (ret=%d)\n" , __func__, drv->name, ret); } -if (!prio) +if (!prio || !init)return -ENODEV ; -if (ret == -ENOENT ) { - gd->env_addr = (ulong)&default_environment[0 ]; - gd->env_valid =ENV_VALID ; - -return 0 ; - } - -return ret; +return 0 ; }
Signature · 1 line
git-series 0.9.1
PATCH 2/11 lib: hashtable: support whitelisting env variables
In reply to Quentin SchulzWhen passing the vars array to himport_r, variables in the array that are not in the loaded environment are removed from the current environment.
Of course, this isn't suitable for whitelisting some variables. Let's introduce a whitelisting boolean that will not remove array variables that are not in the loaded environment from the current environment.
The remaining of the code will rightfully override any variable that already exists.
--- board/sunxi/board.c | 2 +- cmd/nvedit.c | 2 +- env/common.c | 6 +++--- include/search.h | 2 +- lib/hashtable.c | 17 ++++++++++++++++- 5 files changed, 22 insertions(+), 7 deletions(-)
board/sunxi/board.c +1 -1
@@ -647,7 +647,7 @@ static void parse_spl_header(const uint32_t spl_addr)* import -t" the string(s) at fel_script_address right away. */ himport_r (&env_htab, (char *)(uintptr_t )spl->fel_script_address, - spl->fel_uEnv_length,'\n' ,H_NOCLEAR ,0 ,0 ,NULL ); + spl->fel_uEnv_length,'\n' ,H_NOCLEAR ,0 ,0 ,NULL ,0 );return ; }/* otherwise assume .scr format (mkimage-type script) */
cmd/nvedit.c +1 -1
@@ -1079,7 +1079,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, }if (himport_r (&env_htab, ptr, size, sep, del ?0 :H_NOCLEAR , - crlf_is_lf,0 ,NULL ) ==0 ) { + crlf_is_lf,0 ,NULL , false) ==0 ) {pr_err ("Environment import failed: errno = %d\n" , errno);return 1 ; }
env/common.c +3 -3
@@ -83,7 +83,7 @@ void set_default_env(const char *s)if (himport_r (&env_htab, (char *)default_environment,sizeof (default_environment),'\0' , flags,0 , -0 ,NULL ) ==0 ) +0 ,NULL , false) ==0 )pr_err ("Environment import failed: errno = %d\n" , errno); gd->flags |=GD_FLG_ENV_READY ;
@@ -100,7 +100,7 @@ int set_default_vars(int nvars, char * const vars[])*/ return himport_r (&env_htab, (const char *)default_environment,sizeof (default_environment),'\0' , -H_NOCLEAR |H_INTERACTIVE ,0 , nvars, vars); +H_NOCLEAR |H_INTERACTIVE ,0 , nvars, vars,0 ); }#ifdef CONFIG_ENV_AES
@@ -178,7 +178,7 @@ int env_import(const char *buf, int check) }if (himport_r (&env_htab, (char *)ep->data,ENV_SIZE ,'\0' ,0 ,0 , -0 ,NULL )) { +0 ,NULL , false)) { gd->flags |=GD_FLG_ENV_READY ;return 1 ; }
include/search.h +1 -1
@@ -101,7 +101,7 @@ extern ssize_t hexport_r(struct hsearch_data *__htab,extern int himport_r (struct hsearch_data *__htab,const char *__env,size_t __size,const char __sep,int __flag,int __crlf_is_lf,int nvars, -char *const vars[]); +char *const vars[],bool whitelisting);/* Walk the whole table calling the callback on each element */ extern int hwalk_r (struct hsearch_data *__htab,int (*callback)(ENTRY *));
lib/hashtable.c +16 -1
@@ -772,11 +772,17 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[])* * In theory, arbitrary separator characters can be used, but only * '\0' and '\n' have really been tested. +* +* "whitelisting" makes the function to not remove variables from the +* env that were not found in "vars". +* if "whitelisting" is false, the function remove variables from env +* that were not found in "vars". */ int himport_r (struct hsearch_data *htab,const char *env,size_t size,const char sep,int flag, -int crlf_is_lf,int nvars,char *const vars[]) +int crlf_is_lf,int nvars,char *const vars[], +bool whitelisting) {char *data, *sp, *dp, *name, *value;char *localvars[nvars];
@@ -935,6 +941,14 @@ int himport_r(struct hsearch_data *htab,free (data);/* process variables which were not considered */ +/* +* If we are importing variables from a second env and checking they're +* whitelisted, we don't want to delete the variables in current env +* because it was not in the whitelist. +*/ +if (whitelisting) + goto out; +for (i =0 ; i < nvars; i++) {if (localvars[i] ==NULL )continue ;
@@ -952,6 +966,7 @@ int himport_r(struct hsearch_data *htab,printf ("WARNING: '%s' not in imported env, deleting it!\n" , localvars[i]); } +out:debug ("INSERT: done\n" );return 1 ;/* everything OK */ }
Signature · 1 line
git-series 0.9.1
PATCH 3/11 env: add support for whitelisting variables from secondary environments
In reply to Quentin SchulzThis patch makes it possible to have a first environment that will be the base environment, secondary environment that will be pre-loaded and have its variables passing through the whitelist "mask" to override variables in the base environment.
The base environment will be the environment with the highest priority (0 is highest) that can load and the next loadable environment will be the secondary environment (with a lowest priority than the base environment of course).
The whitelist "mask" is built with the content of space-separated list of environment variables in ENV_VAR_WHITELIST_LIST in Kconfig.
--- env/Kconfig | 17 +++++++- env/common.c | 7 +++- env/env.c | 100 +++++++++++++++++++++++++++++++++++++++---- include/environment.h | 6 +++- 4 files changed, 123 insertions(+), 7 deletions(-)
env/Kconfig +17 -0
@@ -438,4 +438,21 @@ config ENV_UBI_VOLUME endif +config ENV_VAR_WHITELIST + bool "Enable overriding some variables from secondary environments" + help + This allows overriding some variables from secondary environments. + After the first valid environment is loaded, a secondary environment + is pre-loaded and its variables that are present in the whitelist will + be added to the current environment or will override existing + variables. + +config ENV_VAR_WHITELIST_LIST + depends on ENV_VAR_WHITELIST + string "Whitelist of variables from secondary environments" + help + This defines the variables that are allowed to be overriden by + ones from secondary environments. + This is a list of environment variables that are space-separated. + endmenu
env/common.c +7 -0
@@ -177,8 +177,15 @@ int env_import(const char *buf, int check)return ret; } +#ifdef CONFIG_ENV_VAR_WHITELIST +if (himport_r (&env_htab, (char *)ep->data,ENV_SIZE ,'\0' , + whitelisting ?H_NOCLEAR :0 ,0 , + whitelisting ? whitelist_nvars :0 , + whitelisting ? whitelist :NULL , whitelisting)) { +#else if (himport_r (&env_htab, (char *)ep->data,ENV_SIZE ,'\0' ,0 ,0 ,0 ,NULL , false)) { +#endif gd->flags |=GD_FLG_ENV_READY ;return 1 ; }
env/env.c +93 -7
154 lines not on this page · raw message
include/environment.h +6 -0
@@ -267,6 +267,12 @@ struct env_driver {int (*init)(void ); }; +#ifdef CONFIG_ENV_VAR_WHITELIST +extern char *const *whitelist; +extern unsigned int whitelist_nvars; +extern bool whitelisting; +#endif +/* Declare a new environment location driver */ #define U_BOOT_ENV_LOCATION (__name) \ ll_entry_declare(struct env_driver, __name, env_driver)
Signature · 1 line
git-series 0.9.1
PATCH 3/11 env: add support for whitelisting variables from secondary environments
Hi Quentin,
env/Kconfig config ENV_UBI_VOLUME · line 448
Quoted 13 lines
(quoted diff) +2
-0+config ENV_VAR_WHITELIST + bool "Enable overriding some variables from secondaryenvironments"
(quoted diff) +2
-0+ help + This allows overriding some variables from secondaryenvironments.
(quoted diff) +1
-0+ After the first valid environment is loaded, a secondaryenvironment
(quoted diff) +1
-0+ is pre-loaded and its variables that are present in thewhitelist will
(quoted diff) +1
-0+ be added to the current environment or will overrideexisting
env/Kconfig +1
-0+ variables.
This description seems to do what is already done in the "env import" command.
It also allows for setting env variables from HUSH prompt.
Maybe it would be possible to re-use or extend it to suits your needs?
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de -------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171226/96eb08db/attachment.sig>
PATCH 3/11 env: add support for whitelisting variables from secondary environments
Hi Lukasz,
Quoted 19 lines · Lukasz Majewski
Hi Quentin,
env/Kconfig config ENV_UBI_VOLUME · line 448
Quoted 13 lines
(quoted diff) +2
-0+config ENV_VAR_WHITELIST + bool "Enable overriding some variables from secondaryenvironments"
(quoted diff) +2
-0+ help + This allows overriding some variables from secondaryenvironments.
(quoted diff) +1
-0+ After the first valid environment is loaded, a secondaryenvironment
(quoted diff) +1
-0+ is pre-loaded and its variables that are present in thewhitelist will
(quoted diff) +1
-0+ be added to the current environment or will overrideexisting
env/Kconfig +1
-0+ variables.This description seems to do what is already done in the "env import" command.
Didn't know that one, thanks.
Quoted 4 lines
It also allows for setting env variables from HUSH prompt.
Maybe it would be possible to re-use or extend it to suits your needs?
It's pretty much everything I needed except the whitelisting of variables.
While it's possible for env export to take a list of variables to export, for what I've seen, env import does not take a list of variables to import.
It's needed since the env I'll import is unsafe. I'll have a look at that and make an other RFC :)
Thanks for the review, very helpful,
Quentin
Signature · 11 lines
Quentin Schulz, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: OpenPGP digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180103/c0b3f77b/attachment.sig>
PATCH 4/11 env: make nowhere an env medium like the others
In reply to Quentin SchulzSince we now allow the loading of different environment media by priority, we can mimic the current fallback system with nowhere medium by adding a load function to the nowhere driver (and make it the medium with the lowest priority).
Nowhere then becomes a valid environment medium.
This also makes it possible to either enforce the value of some variables by using nowhere medium when using it as secondary environment or to completely enforce all the environment variables except a few from a secondary environment when using it as base environment.
--- env/Kconfig | 12 ++---------- env/common.c | 2 +- env/nowhere.c | 8 +++++++- 3 files changed, 10 insertions(+), 12 deletions(-)
env/Kconfig +2 -10
@@ -2,16 +2,6 @@ menu "Environment" config ENV_IS_NOWHERE bool "Environment is not stored" - depends on !ENV_IS_IN_EEPROM - depends on !ENV_IS_IN_FAT - depends on !ENV_IS_IN_FLASH - depends on !ENV_IS_IN_MMC - depends on !ENV_IS_IN_NAND - depends on !ENV_IS_IN_NVRAM - depends on !ENV_IS_IN_ONENAND - depends on !ENV_IS_IN_REMOTE - depends on !ENV_IS_IN_SPI_FLASH - depends on !ENV_IS_IN_UBI default y help Define this if you don't want to or can't have an environment stored
@@ -19,6 +9,8 @@ config ENV_IS_NOWHERE while U-Boot is running, but once U-Boot exits it will not be stored. U-Boot will therefore always start up with a default environment. + When whitelisting is enabled, define this to be able to use the + default environment as either base or secondary environment. config ENV_IS_IN_EEPROM bool "Environment in EEPROM"
env/common.c +1 -1
@@ -279,7 +279,7 @@ void env_relocate(void) env_htab.change_ok += gd->reloc_off;#endif if (gd->env_valid ==ENV_INVALID ) { -#if defined(CONFIG_ENV_IS_NOWHERE ) || defined(CONFIG_SPL_BUILD ) +#if defined(CONFIG_SPL_BUILD )/* Environment not changable */ set_default_env (NULL );#else
env/nowhere.c +7 -1
@@ -15,6 +15,11 @@DECLARE_GLOBAL_DATA_PTR ; +static int env_nowhere_load (void ) +{ +return !env_import ((char *)default_environment,0 ); +} +/* * Because we only ever have the default environment available we must mark * it as invalid.
@@ -22,7 +27,7 @@ DECLARE_GLOBAL_DATA_PTR;static int env_nowhere_init (void ) { gd->env_addr = (ulong)&default_environment[0 ]; - gd->env_valid =ENV_INVALID ; + gd->env_valid =ENV_VALID ;return 0 ; }
@@ -31,4 +36,5 @@ U_BOOT_ENV_LOCATION(nowhere) = { .location = ENVL_NOWHERE, .init = env_nowhere_init, ENV_NAME("nowhere") + .load = env_nowhere_load, };
Signature · 1 line
git-series 0.9.1
PATCH 5/11 cmd: saveenv: enable the saveenv command when ENV_IS_NOWHERE is defined
In reply to Quentin Schulzbut another env medium is enabled too
Now that it is possible to have multiple environments at the same time, nowhere included, enable the saveenv command when nowhere medium is enabled but accompanied by another medium on which the saveenv command is possible.
--- cmd/nvedit.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
cmd/nvedit.c +13 -1
@@ -704,7 +704,19 @@ ulong env_get_ulong(const char *name, int base, ulong default_val) } #ifndef CONFIG_SPL_BUILD -#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE) +#if defined(CONFIG_CMD_SAVEENV) && (!defined(CONFIG_ENV_IS_NOWHERE) || (\ + defined(CONFIG_ENV_IS_IN_EEPROM) || \ + defined(CONFIG_ENV_IS_IN_FLASH) || \ + defined(CONFIG_ENV_IS_IN_MMC) || \ + defined(CONFIG_ENV_IS_IN_FAT) || \ + defined(CONFIG_ENV_IS_IN_EXT4) || \ + defined(CONFIG_ENV_IS_IN_NAND) || \ + defined(CONFIG_ENV_IS_IN_NVRAM) || \ + defined(CONFIG_ENV_IS_IN_ONENAND) || \ + defined(CONFIG_ENV_IS_IN_SATA) || \ + defined(CONFIG_ENV_IS_IN_SPI_FLASH) || \ + defined(CONFIG_ENV_IS_IN_REMOTE) || \ + defined(CONFIG_ENV_IS_IN_UBI))) static int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {
Signature · 1 line
git-series 0.9.1
PATCH 6/11 env: add env_driver to env_driver functions' arguments
In reply to Quentin SchulzMight be interesting to get some infos about the driver (e.g. its location) when inside one of its functions.
--- env/eeprom.c | 6 +++--- env/env.c | 8 ++++---- env/ext4.c | 4 ++-- env/fat.c | 4 ++-- env/flash.c | 10 +++++----- env/mmc.c | 6 +++--- env/nand.c | 8 ++++---- env/nowhere.c | 4 ++-- env/nvram.c | 8 ++++---- env/onenand.c | 4 ++-- env/remote.c | 6 +++--- env/sata.c | 4 ++-- env/sf.c | 8 ++++---- env/ubi.c | 8 ++++---- include/environment.h | 14 ++++++++++---- 15 files changed, 54 insertions(+), 48 deletions(-)
env/eeprom.c +3 -3
@@ -61,7 +61,7 @@ static int eeprom_bus_write(unsigned dev_addr, unsigned offset,return rcode; } -static int env_eeprom_get_char (int index) +static int env_eeprom_get_char (struct env_driver *drv,int index) {uchar c;unsigned int off =CONFIG_ENV_OFFSET ;
@@ -76,7 +76,7 @@ static int env_eeprom_get_char(int index)return c; } -static int env_eeprom_load (void ) +static int env_eeprom_load (struct env_driver *drv) {char buf_env[CONFIG_ENV_SIZE ];unsigned int off =CONFIG_ENV_OFFSET ;
@@ -186,7 +186,7 @@ static int env_eeprom_load(void)return 0 ; } -static int env_eeprom_save (void ) +static int env_eeprom_save (struct env_driver *drv) {env_t env_new;int rc;
env/env.c +4 -4
@@ -153,7 +153,7 @@ int env_get_char(int index)if (!(gd->env_has_init &BIT (drv->location)))continue ; - ret = drv->get_char (index); + ret = drv->get_char (drv, index);if (!ret)return 0 ;
@@ -181,7 +181,7 @@ int env_load(void)continue ;printf ("Loading Environment from %s... " , drv->name); - ret = drv->load (); + ret = drv->load (drv);printf ("%s\n" , ret ?"Failed" :"OK" );if (!ret)break ;
@@ -205,7 +205,7 @@ int env_load(void)printf ("Overriding env variables with ones from %s env..." , __func__, drv->name); - ret = drv->load (); + ret = drv->load (drv);printf ("%s\n" , ret ?"Failed" :"OK" );if (!ret) { found = true;
@@ -241,7 +241,7 @@ int env_save(void)continue ;printf ("Saving Environment to %s... " , drv->name); - ret = drv->save (); + ret = drv->save (drv);printf ("%s\n" , ret ?"Failed" :"OK" );#ifdef CONFIG_ENV_VAR_WHITELIST /* When whitelisting, we want to save to all media available */
env/ext4.c +2 -2
@@ -34,7 +34,7 @@DECLARE_GLOBAL_DATA_PTR ;#ifdef CONFIG_CMD_SAVEENV -static int env_ext4_save (void ) +static int env_ext4_save (struct env_driver *drv) {env_t env_new;struct blk_desc *dev_desc =NULL ;
@@ -75,7 +75,7 @@ static int env_ext4_save(void) }#endif /* CONFIG_CMD_SAVEENV */ -static int env_ext4_load (void ) +static int env_ext4_load (struct env_driver *drv) {ALLOC_CACHE_ALIGN_BUFFER (char, buf,CONFIG_ENV_SIZE );struct blk_desc *dev_desc =NULL ;
env/fat.c +2 -2
@@ -34,7 +34,7 @@DECLARE_GLOBAL_DATA_PTR ;#ifdef CMD_SAVEENV -static int env_fat_save (void ) +static int env_fat_save (struct env_driver *drv) {env_t env_new;struct blk_desc *dev_desc =NULL ;
@@ -73,7 +73,7 @@ static int env_fat_save(void)#endif /* CMD_SAVEENV */ #ifdef LOADENV -static int env_fat_load (void ) +static int env_fat_load (struct env_driver *drv) {ALLOC_CACHE_ALIGN_BUFFER (char, buf,CONFIG_ENV_SIZE );struct blk_desc *dev_desc =NULL ;
env/flash.c +5 -5
@@ -71,7 +71,7 @@ static ulong __maybe_unused end_addr_new = #ifdef CONFIG_ENV_ADDR_REDUND #ifdef INITENV -static int env_flash_init(void) +static int env_flash_init(struct env_driver *drv) { int crc1_ok = 0, crc2_ok = 0;
@@ -117,7 +117,7 @@ static int env_flash_init(void)#endif #ifdef CMD_SAVEENV -static int env_flash_save (void ) +static int env_flash_save (struct env_driver *drv) {env_t env_new;char *saved_data =NULL ;
@@ -222,7 +222,7 @@ done: #else /* ! CONFIG_ENV_ADDR_REDUND */ #ifdef INITENV -static int env_flash_init(void) +static int env_flash_init(struct env_driver *drv) { if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { gd->env_addr = (ulong)&(env_ptr->data);
@@ -237,7 +237,7 @@ static int env_flash_init(void)#endif #ifdef CMD_SAVEENV -static int env_flash_save (void ) +static int env_flash_save (struct env_driver *drv) {env_t env_new;int rc =1 ;
@@ -308,7 +308,7 @@ done:#endif /* CONFIG_ENV_ADDR_REDUND */ #ifdef LOADENV -static int env_flash_load (void ) +static int env_flash_load (struct env_driver *drv) {#ifdef CONFIG_ENV_ADDR_REDUND if (gd->env_addr != (ulong)&(flash_addr->data)) {
env/mmc.c +3 -3
@@ -143,7 +143,7 @@ static inline int write_env(struct mmc *mmc, unsigned long size,return (n == blk_cnt) ?0 :-1 ; } -static int env_mmc_save (void ) +static int env_mmc_save (struct env_driver *drv) {ALLOC_CACHE_ALIGN_BUFFER (env_t, env_new,1 );int dev =mmc_get_env_dev ();
@@ -206,7 +206,7 @@ static inline int read_env(struct mmc *mmc, unsigned long size, }#ifdef CONFIG_ENV_OFFSET_REDUND -static int env_mmc_load (void ) +static int env_mmc_load (struct env_driver *drv) {#if !defined(ENV_IS_EMBEDDED )struct mmc *mmc;
@@ -268,7 +268,7 @@ err: return ret; }#else /* ! CONFIG_ENV_OFFSET_REDUND */ -static int env_mmc_load (void ) +static int env_mmc_load (struct env_driver *drv) {#if !defined(ENV_IS_EMBEDDED )ALLOC_CACHE_ALIGN_BUFFER (char, buf,CONFIG_ENV_SIZE );
env/nand.c +4 -4
@@ -62,7 +62,7 @@ DECLARE_GLOBAL_DATA_PTR;* This way the SPL loads not only the U-Boot image from NAND but * also the environment. */ -static int env_nand_init (void ) +static int env_nand_init (struct env_driver *drv) {#if defined(ENV_IS_EMBEDDED ) || defined(CONFIG_NAND_ENV_DST )int crc1_ok =0 , crc2_ok =0 ;
@@ -183,7 +183,7 @@ static int erase_and_write_env(const struct nand_env_location *location,return ret; } -static int env_nand_save (void ) +static int env_nand_save (struct env_driver *drv) {int ret =0 ;ALLOC_CACHE_ALIGN_BUFFER (env_t, env_new,1 );
@@ -315,7 +315,7 @@ int get_nand_env_oob(struct mtd_info *mtd, unsigned long *result)#endif #ifdef CONFIG_ENV_OFFSET_REDUND -static int env_nand_load (void ) +static int env_nand_load (struct env_driver *drv) {#if defined(ENV_IS_EMBEDDED )return 0 ;
@@ -368,7 +368,7 @@ done:* device i.e., nand_dev_desc + 0. This is also the behaviour using * the new NAND code. */ -static int env_nand_load (void ) +static int env_nand_load (struct env_driver *drv) {#if !defined(ENV_IS_EMBEDDED )int ret;
env/nowhere.c +2 -2
@@ -15,7 +15,7 @@DECLARE_GLOBAL_DATA_PTR ; -static int env_nowhere_load (void ) +static int env_nowhere_load (struct env_driver *drv) {return !env_import ((char *)default_environment,0 ); }
@@ -24,7 +24,7 @@ static int env_nowhere_load(void)* Because we only ever have the default environment available we must mark * it as invalid. */ -static int env_nowhere_init (void ) +static int env_nowhere_init (struct env_driver *drv) { gd->env_addr = (ulong)&default_environment[0 ]; gd->env_valid =ENV_VALID ;
env/nvram.c +4 -4
@@ -41,7 +41,7 @@ env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;#endif #ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE -static int env_nvram_get_char (int index) +static int env_nvram_get_char (struct env_driver *drv,int index) {uchar c;
@@ -51,7 +51,7 @@ static int env_nvram_get_char(int index) }#endif -static int env_nvram_load (void ) +static int env_nvram_load (struct env_driver *drv) {char buf[CONFIG_ENV_SIZE ];
@@ -65,7 +65,7 @@ static int env_nvram_load(void)return 0 ; } -static int env_nvram_save (void ) +static int env_nvram_save (struct env_driver *drv) {env_t env_new;int rcode =0 ;
@@ -88,7 +88,7 @@ static int env_nvram_save(void)* * We are still running from ROM, so data use is limited */ -static int env_nvram_init (void ) +static int env_nvram_init (struct env_driver *drv) {#if defined(CONFIG_SYS_NVRAM_ACCESS_ROUTINE )ulong crc;
env/onenand.c +2 -2
@@ -26,7 +26,7 @@DECLARE_GLOBAL_DATA_PTR ; -static int env_onenand_load (void ) +static int env_onenand_load (struct env_driver *drv) {struct mtd_info *mtd = &onenand_mtd;#ifdef CONFIG_ENV_ADDR_FLEX
@@ -63,7 +63,7 @@ static int env_onenand_load(void)return rc ?0 : -EIO ; } -static int env_onenand_save (void ) +static int env_onenand_save (struct env_driver *drv) {env_t env_new;int ret;
env/remote.c +3 -3
@@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR;#define CONFIG_ENV_OFFSET 0#endif -static int env_remote_init (void ) +static int env_remote_init (struct env_driver *drv) {if (crc32 (0 , env_ptr->data,ENV_SIZE ) == env_ptr->crc) { gd->env_addr = (ulong)&(env_ptr->data);
@@ -35,7 +35,7 @@ static int env_remote_init(void) }#ifdef CONFIG_CMD_SAVEENV -static int env_remote_save (void ) +static int env_remote_save (struct env_driver *drv) {#ifdef CONFIG_SRIO_PCIE_BOOT_SLAVE printf ("Can not support the 'saveenv' when boot from SRIO or PCIE!\n" );
@@ -46,7 +46,7 @@ static int env_remote_save(void) }#endif /* CONFIG_CMD_SAVEENV */ -static int env_remote_load (void ) +static int env_remote_load (struct env_driver *drv) {#ifndef ENV_IS_EMBEDDED env_import ((char *)env_ptr,1 );
env/sata.c +2 -2
@@ -45,7 +45,7 @@ static inline int write_env(struct blk_desc *sata, unsigned long size,return (n == blk_cnt) ?0 :-1 ; } -static int env_sata_save (void ) +static int env_sata_save (struct env_driver *drv) {ALLOC_CACHE_ALIGN_BUFFER (env_t, env_new,1 );struct blk_desc *sata =NULL ;
@@ -91,7 +91,7 @@ static inline int read_env(struct blk_desc *sata, unsigned long size,return (n == blk_cnt) ?0 :-1 ; } -static void env_sata_load (void ) +static void env_sata_load (struct env_driver *drv) {ALLOC_CACHE_ALIGN_BUFFER (char, buf,CONFIG_ENV_SIZE );struct blk_desc *sata =NULL ;
env/sf.c +4 -4
@@ -82,7 +82,7 @@ static int setup_flash_device(void) #if defined(CONFIG_ENV_OFFSET_REDUND) #ifdef CMD_SAVEENV -static int env_sf_save(void) +static int env_sf_save(struct env_driver *drv) { env_t env_new; char *saved_buffer = NULL, flag = OBSOLETE_FLAG;
@@ -162,7 +162,7 @@ static int env_sf_save(void) }#endif /* CMD_SAVEENV */ -static int env_sf_load (void ) +static int env_sf_load (struct env_driver *drv) {int ret;int crc1_ok =0 , crc2_ok =0 ;
@@ -251,7 +251,7 @@ out: } #else #ifdef CMD_SAVEENV -static int env_sf_save(void) +static int env_sf_save(struct env_driver *drv) { u32 saved_size, saved_offset, sector; char *saved_buffer = NULL;
@@ -312,7 +312,7 @@ static int env_sf_save(void) }#endif /* CMD_SAVEENV */ -static int env_sf_load (void ) +static int env_sf_load (struct env_driver *drv) {int ret;char *buf =NULL ;
env/ubi.c +4 -4
@@ -20,7 +20,7 @@ DECLARE_GLOBAL_DATA_PTR; #ifdef CONFIG_CMD_SAVEENV #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT -static int env_ubi_save(void) +static int env_ubi_save(struct env_driver *drv) { ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1); int ret;
@@ -62,7 +62,7 @@ static int env_ubi_save(void) return 0; }#else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */ -static int env_ubi_save (void ) +static int env_ubi_save (struct env_driver *drv) {ALLOC_CACHE_ALIGN_BUFFER (env_t, env_new,1 );int ret;
@@ -91,7 +91,7 @@ static int env_ubi_save(void)#endif /* CONFIG_CMD_SAVEENV */ #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT -static int env_ubi_load (void ) +static int env_ubi_load (struct env_driver *drv) {ALLOC_CACHE_ALIGN_BUFFER (char, env1_buf,CONFIG_ENV_SIZE );ALLOC_CACHE_ALIGN_BUFFER (char, env2_buf,CONFIG_ENV_SIZE );
@@ -135,7 +135,7 @@ static int env_ubi_load(void) return 0; }#else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */ -static int env_ubi_load (void ) +static int env_ubi_load (struct env_driver *drv) {ALLOC_CACHE_ALIGN_BUFFER (char, buf,CONFIG_ENV_SIZE );
include/environment.h +10 -4
@@ -222,6 +222,8 @@ enum env_operation { ENVO_SAVE, }; +struct env_driver ; +struct env_driver {const char *name;enum env_location location;
@@ -232,10 +234,11 @@ struct env_driver {* This method is optional. If not provided, a default implementation * will read from gd->env_addr. * +* @drv: the driver whose get_char function is about to be called * @index: Index of character to read (0=first) * @return character read, or -ve on error */ -int (*get_char)(int index); +int (*get_char)(struct env_driver *drv,int index);/** * load() - Load the environment from storage
@@ -243,28 +246,31 @@ struct env_driver {* This method is optional. If not provided, no environment will be * loaded. * +* @drv: the driver whose load function is about to be called * @return 0 if OK, -ve on error */ -int (*load)(void ); +int (*load)(struct env_driver *drv);/** * save() - Save the environment to storage * * This method is required for 'saveenv' to work. * +* @drv: the driver whose save function is about to be called * @return 0 if OK, -ve on error */ -int (*save)(void ); +int (*save)(struct env_driver *drv);/** * init() - Set up the initial pre-relocation environment * * This method is optional. * +* @drv: the driver whose init function is about to be called * @return 0 if OK, -ENOENT if no initial environment could be found, * other -ve on error */ -int (*init)(void ); +int (*init)(struct env_driver *drv); };#ifdef CONFIG_ENV_VAR_WHITELIST
Signature · 1 line
git-series 0.9.1
PATCH 7/11 env: gd flags without ENV_READY is enough to discriminate in
In reply to Quentin Schulzenv_get_default
To prepare the removal of env_valid in an upcoming patch, and since the ENV_READY flag in gd is enough to discriminate in env_get_char, let's update env_get_char to return the variable in the default environment when ENV_READY flag is not set and remove env_valid setting and getting from env_get_default.
--- env/common.c | 3 --- env/env.c | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-)
env/common.c +0 -3
@@ -47,14 +47,11 @@ int env_get_yesno(const char *var)char *env_get_default (const char *name) {char *ret_val; -unsigned long really_valid = gd->env_valid;unsigned long real_gd_flags = gd->flags;/* Pretend that the image is bad. */ gd->flags &= ~GD_FLG_ENV_READY ; - gd->env_valid =ENV_INVALID ; ret_val =env_get (name); - gd->env_valid = really_valid; gd->flags = real_gd_flags;return ret_val; }
env/env.c +1 -1
@@ -141,7 +141,7 @@ int env_get_char(int index)struct env_driver *drv;int prio; -if (gd->env_valid ==ENV_INVALID ) +if (!(gd->flags &GD_FLG_ENV_READY ))return default_environment[index];for (prio =0 ; (drv =env_driver_lookup (ENVO_GET_CHAR , prio)); prio++) {
Signature · 1 line
git-series 0.9.1
PATCH 8/11 env: add env_driver parameter to env_import_redund
In reply to Quentin SchulzTo prepare for an upcoming patch that uses the location of the driver used when doing an env_import_redund, add env_driver as an argument of env_import_redund so that it can be used within.
Update all calls to this function and the prototype.
--- env/common.c | 2 +- env/nand.c | 2 +- env/ubi.c | 2 +- include/environment.h | 3 ++- 4 files changed, 5 insertions(+), 4 deletions(-)
env/common.c +1 -1
@@ -197,7 +197,7 @@ int env_import(const char *buf, int check)#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT static unsigned char env_flags; -int env_import_redund (const char *buf1,const char *buf2) +int env_import_redund (struct env_driver *drv,const char *buf1,const char *buf2) {int crc1_ok, crc2_ok;env_t *ep, *tmp_env1, *tmp_env2;
env/nand.c +1 -1
@@ -352,7 +352,7 @@ static int env_nand_load(struct env_driver *drv) gd->env_valid =ENV_REDUND ;env_import ((char *)tmp_env2,1 ); }else { -env_import_redund ((char *)tmp_env1, (char *)tmp_env2); +env_import_redund (drv, (char *)tmp_env1, (char *)tmp_env2); } done:
env/ubi.c +1 -1
@@ -130,7 +130,7 @@ static int env_ubi_load(struct env_driver *drv) CONFIG_ENV_UBI_PART, CONFIG_ENV_UBI_VOLUME_REDUND); } -env_import_redund ((char *)tmp_env1, (char *)tmp_env2); +env_import_redund (drv, (char *)tmp_env1, (char *)tmp_env2);return 0 ; }
include/environment.h +2 -1
@@ -318,7 +318,8 @@ int env_export(env_t *env_out);#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT /* Select and import one of two redundant environments */ -int env_import_redund (const char *buf1,const char *buf2); +int env_import_redund (struct env_driver *drv,const char *buf1, +const char *buf2);#endif /**
Signature · 1 line
git-series 0.9.1
PATCH 9/11 env: make env_locations a global variable
In reply to Quentin SchulzMake env_locations a global variable so that it can be used in env drivers.
--- env/env.c | 2 +- include/environment.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)
env/env.c +1 -1
@@ -35,7 +35,7 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)return NULL ; } -static enum env_location env_locations[] = { +enum env_location env_locations[] = {#ifdef CONFIG_ENV_IS_IN_EEPROM ENVL_EEPROM,#endif
include/environment.h +2 -0
@@ -215,6 +215,8 @@ enum env_location { ENVL_UNKNOWN, }; +extern enum env_location env_locations[]; +enum env_operation {ENVO_GET_CHAR ,ENVO_INIT ,
Signature · 1 line
git-series 0.9.1
PATCH 10/11 env: introducing env_info struct for storing info per env
In reply to Quentin SchulzTo introduce the ability to use multiple environments at the same time, it is required to store a few information about each of the environment.
Each environment has a different env_addr, "validity" (redund or main) and status of init (failed or succeeded).
This structure is meant to be used in include/asm-generic/global_data.h and thus needs to be put out of the #ifndef DO_DEPS_ONLY.
--- include/environment.h | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)
include/environment.h +20 -14
@@ -183,20 +183,6 @@ extern uint mmc_get_env_part(struct mmc *mmc);# endif #endif -#ifndef DO_DEPS_ONLY - -#include <env_attr.h> -#include <env_callback.h> -#include <env_flags.h> -#include <search.h> - -/* Value for environment validity */ -enum env_valid { -ENV_INVALID ,/* No valid environment */ -ENV_VALID ,/* First or only environment is valid */ -ENV_REDUND ,/* Redundant environment is valid */ -}; -enum env_location {ENVL_EEPROM ,ENVL_EXT4 ,
@@ -215,6 +201,26 @@ enum env_location { ENVL_UNKNOWN, }; +/* Value for environment validity */ +enum env_valid { +ENV_INVALID ,/* No valid environment */ +ENV_VALID ,/* First or only environment is valid */ +ENV_REDUND ,/* Redundant environment is valid */ +}; + +struct env_info { +unsigned long env_addr; +enum env_valid env_valid; +bool has_init; +}; + +#ifndef DO_DEPS_ONLY + +#include <env_attr.h> +#include <env_callback.h> +#include <env_flags.h> +#include <search.h> +extern enum env_location env_locations[]; enum env_operation {
Signature · 1 line
git-series 0.9.1
PATCH 11/11 env: store information about each environment in gd
In reply to Quentin SchulzTo store the "validity" (redund or main), the env_addr (different when redund or main env is used) and the init status of the environment for each environment, an array of env_info structure is created in the global data pointer.
Move all calls to env_addr, env_valid and env_has_init to the new model in the different env drivers.
Note that env_addr and env_valid defined in board file are intentionally left untouched and thus, will break compilation for a few boards. The question is: why do we need to set env_addr and env_valid in board files? In addition, env_addr is never used (only when using redundant env and it's set before being used).
--- common/board_r.c | 8 ++++- env/common.c | 25 ++++++++++------- env/eeprom.c | 34 +++++++++++------------ env/env.c | 13 +++++---- env/flash.c | 48 ++++++++++++++++---------------- env/mmc.c | 8 ++--- env/nand.c | 36 ++++++++++++------------ env/nowhere.c | 4 +-- env/nvram.c | 10 +++---- env/onenand.c | 2 +- env/remote.c | 4 +-- env/sf.c | 26 ++++++++--------- env/ubi.c | 4 +-- include/asm-generic/global_data.h | 5 +-- 14 files changed, 121 insertions(+), 106 deletions(-)
common/board_r.c +7 -1
@@ -125,6 +125,10 @@ __weak int fixup_cpu(void)static int initr_reloc_global_data (void ) { +#ifdef CONFIG_SYS_EXTRA_ENV_RELOC +int i; +#endif +#ifdef __ARM__ monitor_flash_len = _end - __image_copy_start;#elif defined(CONFIG_NDS32 )
@@ -154,7 +158,9 @@ static int initr_reloc_global_data(void)* in SRAM mode and initialize that cache from SRAM mode back to being * a cache in cpu_init_r. */ - gd->env_addr += gd->relocaddr -CONFIG_SYS_MONITOR_BASE ; +for (i =0 ; i <ENVL_COUNT ; i++) + gd->env_infos[i].env_addr += gd->relocaddr - +CONFIG_SYS_MONITOR_BASE ;#endif #ifdef CONFIG_OF_EMBED /*
env/common.c +16 -9
@@ -214,24 +214,24 @@ int env_import_redund(struct env_driver *drv, const char *buf1, const char *buf2set_default_env ("!bad CRC" );return 0 ; }else if (crc1_ok && !crc2_ok) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }else if (!crc1_ok && crc2_ok) { - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ; }else {/* both ok - check serial */ if (tmp_env1->flags ==255 && tmp_env2->flags ==0 ) - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ;else if (tmp_env2->flags ==255 && tmp_env1->flags ==0 ) - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;else if (tmp_env1->flags > tmp_env2->flags) - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;else if (tmp_env2->flags > tmp_env1->flags) - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ;else /* flags are equal - almost impossible */ - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; } -if (gd->env_valid ==ENV_VALID ) +if (gd->env_infos[drv->location].env_valid ==ENV_VALID ) ep = tmp_env1;else ep = tmp_env2;
@@ -271,11 +271,18 @@ int env_export(env_t *env_out)void env_relocate (void ) { +int i; +#if defined(CONFIG_NEEDS_MANUAL_RELOC )env_reloc (); env_htab.change_ok += gd->reloc_off; +#endif -if (gd->env_valid ==ENV_INVALID ) { +for (i =0 ; i <ENVL_COUNT ; i++) +if (gd->env_infos[i].has_init) +break ; + +if (i ==ENVL_COUNT ) {#if defined(CONFIG_SPL_BUILD )/* Environment not changable */ set_default_env (NULL );
env/eeprom.c +17 -17
@@ -67,7 +67,7 @@ static int env_eeprom_get_char(struct env_driver *drv, int index)unsigned int off =CONFIG_ENV_OFFSET ;#ifdef CONFIG_ENV_OFFSET_REDUND -if (gd->env_valid ==ENV_REDUND ) +if (gd->env_infos[drv->location].env_valid ==ENV_REDUND ) off =CONFIG_ENV_OFFSET_REDUND ;#endif eeprom_bus_read(CONFIG_SYS_DEF_EEPROM_ADDR,
@@ -121,24 +121,24 @@ static int env_eeprom_load(struct env_driver *drv) }if (!crc_ok[0 ] && !crc_ok[1 ]) { - gd->env_addr =0 ; - gd->env_valid =ENV_INVALID ; + gd->env_infos[drv->location].env_addr =0 ; + gd->env_infos[drv->location].env_valid =ENV_INVALID ; }else if (crc_ok[0 ] && !crc_ok[1 ]) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }else if (!crc_ok[0 ] && crc_ok[1 ]) { - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ; }else {/* both ok - check serial */ if (flags[0 ] ==ACTIVE_FLAG && flags[1 ] ==OBSOLETE_FLAG ) - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;else if (flags[0 ] ==OBSOLETE_FLAG && flags[1 ] ==ACTIVE_FLAG ) - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ;else if (flags[0 ] ==0xFF && flags[1 ] ==0 ) - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ;else if (flags[1 ] ==0xFF && flags[0 ] ==0 ) - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;else /* flags are equal - almost impossible */ - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }#else /* CONFIG_ENV_OFFSET_REDUND */
@@ -166,15 +166,15 @@ static int env_eeprom_load(struct env_driver *drv) }if (crc == new) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }else { - gd->env_valid =ENV_INVALID ; + gd->env_infos[drv->location].env_valid =ENV_INVALID ; }#endif /* CONFIG_ENV_OFFSET_REDUND */ off =CONFIG_ENV_OFFSET ;#ifdef CONFIG_ENV_OFFSET_REDUND -if (gd->env_valid ==ENV_REDUND ) +if (gd->env_infos[drv->location].env_valid ==ENV_REDUND ) off =CONFIG_ENV_OFFSET_REDUND ;#endif
@@ -201,7 +201,7 @@ static int env_eeprom_save(struct env_driver *drv)return rc;#ifdef CONFIG_ENV_OFFSET_REDUND -if (gd->env_valid ==ENV_VALID ) { +if (gd->env_infos[drv->location].env_valid ==ENV_VALID ) { off =CONFIG_ENV_OFFSET_REDUND ; off_red =CONFIG_ENV_OFFSET ; }
@@ -218,10 +218,10 @@ static int env_eeprom_save(struct env_driver *drv) off_red + offsetof(env_t , flags), (uchar *)&flag_obsolete, 1); -if (gd->env_valid ==ENV_VALID ) - gd->env_valid =ENV_REDUND ; +if (gd->env_infos[drv->location].env_valid ==ENV_VALID ) + gd->env_infos[drv->location].env_valid =ENV_REDUND ;else - gd->env_valid = ENV_VALID; + gd->env_infos[drv->location].env_valid = ENV_VALID; }#endif return rc;
env/env.c +8 -5
@@ -150,7 +150,7 @@ int env_get_char(int index)if (!drv->get_char)continue ; -if (!(gd->env_has_init &BIT (drv->location))) +if (!gd->env_infos[drv->location].has_init)continue ; ret = drv->get_char (drv, index);
@@ -177,7 +177,7 @@ int env_load(void)if (!drv->load)continue ; -if (!(gd->env_has_init &BIT (drv->location))) +if (!gd->env_infos[drv->location].has_init)continue ;printf ("Loading Environment from %s... " , drv->name);
@@ -203,7 +203,10 @@ int env_load(void)if (!drv->load)continue ; -printf ("Overriding env variables with ones from %s env..." , +if (!gd->env_infos[drv->location].has_init) +continue ; + +printf ("%s: Overriding env variables with ones from %s env..." , __func__, drv->name); ret = drv->load (drv);printf ("%s\n" , ret ?"Failed" :"OK" );
@@ -237,7 +240,7 @@ int env_save(void)if (!drv->save)continue ; -if (!(gd->env_has_init &BIT (drv->location))) +if (!gd->env_infos[drv->location].has_init)continue ;printf ("Saving Environment to %s... " , drv->name);
@@ -277,7 +280,7 @@ int env_init(void) ret =0 ;if (!ret) { - gd->env_has_init |=BIT (drv->location); + gd->env_infos[drv->location].has_init = true; init = true; }
env/flash.c +24 -24
94 lines not on this page · raw message
env/mmc.c +4 -4
@@ -163,7 +163,7 @@ static int env_mmc_save(struct env_driver *drv) goto fini;#ifdef CONFIG_ENV_OFFSET_REDUND -if (gd->env_valid ==ENV_VALID ) +if (gd->env_infos[drv->location].env_valid ==ENV_VALID ) copy =1 ;#endif
@@ -182,7 +182,7 @@ static int env_mmc_save(struct env_driver *drv) ret =0 ;#ifdef CONFIG_ENV_OFFSET_REDUND - gd->env_valid = gd->env_valid ==ENV_REDUND ?ENV_VALID :ENV_REDUND ; + gd->env_infos[drv->location].env_valid = gd->env_infos[drv->location].env_valid ==ENV_REDUND ?ENV_VALID :ENV_REDUND ;#endif fini:
@@ -247,10 +247,10 @@ static int env_mmc_load(struct env_driver *drv) ret = -EIO ; goto fini; }else if (!read1_fail && read2_fail) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;env_import ((char *)tmp_env1,1 ); }else if (read1_fail && !read2_fail) { - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ;env_import ((char *)tmp_env2,1 ); }else {env_import_redund ((char *)tmp_env1, (char *)tmp_env2);
env/nand.c +18 -18
@@ -78,42 +78,42 @@ static int env_nand_init(struct env_driver *drv) crc1_ok =crc32 (0 , tmp_env1->data,ENV_SIZE ) == tmp_env1->crc;if (!crc1_ok && !crc2_ok) { - gd->env_addr =0 ; - gd->env_valid =ENV_INVALID ; + gd->env_infos[drv->location].env_addr =0 ; + gd->env_infos[drv->location].env_valid =ENV_INVALID ;return 0 ; }else if (crc1_ok && !crc2_ok) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }#ifdef CONFIG_ENV_OFFSET_REDUND else if (!crc1_ok && crc2_ok) { - gd->env_valid = ENV_REDUND; + gd->env_infos[drv->location].env_valid = ENV_REDUND; } else {/* both ok - check serial */ if (tmp_env1->flags ==255 && tmp_env2->flags ==0 ) - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ;else if (tmp_env2->flags ==255 && tmp_env1->flags ==0 ) - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;else if (tmp_env1->flags > tmp_env2->flags) - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;else if (tmp_env2->flags > tmp_env1->flags) - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ;else /* flags are equal - almost impossible */ - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; } -if (gd->env_valid ==ENV_REDUND ) +if (gd->env_infos[drv->location].env_valid ==ENV_REDUND ) env_ptr = tmp_env2; else#endif -if (gd->env_valid ==ENV_VALID ) +if (gd->env_infos[drv->location].env_valid ==ENV_VALID ) env_ptr = tmp_env1; - gd->env_addr = (ulong )env_ptr->data; + gd->env_infos[drv->location].env_addr = (ulong )env_ptr->data;#else /* ENV_IS_EMBEDDED || CONFIG_NAND_ENV_DST */ - gd->env_addr = (ulong)&default_environment[0 ]; - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_addr = (ulong)&default_environment[0 ]; + gd->env_infos[drv->location].env_valid =ENV_VALID ;#endif /* ENV_IS_EMBEDDED || CONFIG_NAND_ENV_DST */ return 0 ;
@@ -216,14 +216,14 @@ static int env_nand_save(struct env_driver *drv)return ret;#ifdef CONFIG_ENV_OFFSET_REDUND - env_idx = (gd->env_valid ==ENV_VALID ); + env_idx = (gd->env_infos[drv->location].env_valid ==ENV_VALID );#endif ret =erase_and_write_env (&location[env_idx], (u_char *)env_new);#ifdef CONFIG_ENV_OFFSET_REDUND if (!ret) {/* preset other copy for next write */ - gd->env_valid = gd->env_valid ==ENV_REDUND ?ENV_VALID : + gd->env_infos[drv->location].env_valid = gd->env_infos[drv->location].env_valid ==ENV_REDUND ?ENV_VALID :ENV_REDUND ;return ret; }
@@ -346,10 +346,10 @@ static int env_nand_load(struct env_driver *drv)set_default_env ("!bad env area" ); goto done; }else if (!read1_fail && read2_fail) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;env_import ((char *)tmp_env1,1 ); }else if (read1_fail && !read2_fail) { - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ;env_import ((char *)tmp_env2,1 ); }else {env_import_redund (drv, (char *)tmp_env1, (char *)tmp_env2);
env/nowhere.c +2 -2
@@ -26,8 +26,8 @@ static int env_nowhere_load(struct env_driver *drv)*/ static int env_nowhere_init (struct env_driver *drv) { - gd->env_addr = (ulong)&default_environment[0 ]; - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_addr = (ulong)&default_environment[0 ]; + gd->env_infos[drv->location].env_valid =ENV_VALID ;return 0 ; }
env/nvram.c +5 -5
@@ -98,15 +98,15 @@ static int env_nvram_init(struct env_driver *drv)nvram_read (data,CONFIG_ENV_ADDR +sizeof (ulong),ENV_SIZE );if (crc32 (0 , data,ENV_SIZE ) == crc) { - gd->env_addr = (ulong )CONFIG_ENV_ADDR +sizeof (long ); + gd->env_infos[drv->location].env_addr = (ulong )CONFIG_ENV_ADDR +sizeof (long );#else if (crc32 (0 , env_ptr->data,ENV_SIZE ) == env_ptr->crc) { - gd->env_addr = (ulong)&env_ptr->data; + gd->env_infos[drv->location].env_addr = (ulong)&env_ptr->data;#endif - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }else { - gd->env_addr = (ulong)&default_environment[0 ]; - gd->env_valid =ENV_INVALID ; + gd->env_infos[drv->location].env_addr = (ulong)&default_environment[0 ]; + gd->env_infos[drv->location].env_valid =ENV_INVALID ; }return 0 ;
env/onenand.c +1 -1
@@ -58,7 +58,7 @@ static int env_onenand_load(struct env_driver *drv) rc =env_import (buf,1 );if (rc) - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ;return rc ?0 : -EIO ; }
env/remote.c +2 -2
@@ -26,8 +26,8 @@ DECLARE_GLOBAL_DATA_PTR;static int env_remote_init (struct env_driver *drv) {if (crc32 (0 , env_ptr->data,ENV_SIZE ) == env_ptr->crc) { - gd->env_addr = (ulong)&(env_ptr->data); - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_addr = (ulong)&(env_ptr->data); + gd->env_infos[drv->location].env_valid =ENV_VALID ;return 0 ; }
env/sf.c +13 -13
@@ -98,7 +98,7 @@ static int env_sf_save(struct env_driver *drv)return -EIO ; env_new.flags =ACTIVE_FLAG ; -if (gd->env_valid ==ENV_VALID ) { +if (gd->env_infos[drv->location].env_valid ==ENV_VALID ) { env_new_offset =CONFIG_ENV_OFFSET_REDUND ; env_offset =CONFIG_ENV_OFFSET ; }else {
@@ -150,9 +150,9 @@ static int env_sf_save(struct env_driver *drv)puts ("done\n" ); - gd->env_valid = gd->env_valid ==ENV_REDUND ?ENV_VALID :ENV_REDUND ; + gd->env_infos[drv->location].env_valid = gd->env_infos[drv->location].env_valid ==ENV_REDUND ?ENV_VALID :ENV_REDUND ; -printf ("Valid environment: %d\n" , (int )gd->env_valid); +printf ("Valid environment: %d\n" , (int )gd->env_infos[drv->location].env_valid); done:if (saved_buffer)
@@ -206,30 +206,30 @@ static int env_sf_load(struct env_driver *drv) ret = -EIO ; goto err_read; }else if (crc1_ok && !crc2_ok) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }else if (!crc1_ok && crc2_ok) { - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ; }else if (tmp_env1->flags ==ACTIVE_FLAG && tmp_env2->flags ==OBSOLETE_FLAG ) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }else if (tmp_env1->flags ==OBSOLETE_FLAG && tmp_env2->flags ==ACTIVE_FLAG ) { - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ; }else if (tmp_env1->flags == tmp_env2->flags) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }else if (tmp_env1->flags ==0xFF ) { - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; }else if (tmp_env2->flags ==0xFF ) { - gd->env_valid =ENV_REDUND ; + gd->env_infos[drv->location].env_valid =ENV_REDUND ; }else {/* * this differs from code in env_flash.c, but I think a sane * default path is desirable. */ - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; } -if (gd->env_valid ==ENV_VALID ) +if (gd->env_infos[drv->location].env_valid ==ENV_VALID ) ep = tmp_env1;else ep = tmp_env2;
@@ -336,7 +336,7 @@ static int env_sf_load(struct env_driver *drv) ret =env_import (buf,1 );if (ret) - gd->env_valid =ENV_VALID ; + gd->env_infos[drv->location].env_valid =ENV_VALID ; err_read:spi_flash_free (env_flash);
env/ubi.c +2 -2
@@ -35,7 +35,7 @@ static int env_ubi_save(struct env_driver *drv)return 1 ; } -if (gd->env_valid ==ENV_VALID ) { +if (gd->env_infos[drv->location].env_valid ==ENV_VALID ) {puts ("Writing to redundant UBI... " );if (ubi_volume_write (CONFIG_ENV_UBI_VOLUME_REDUND , (void *)env_new,CONFIG_ENV_SIZE )) {
@@ -57,7 +57,7 @@ static int env_ubi_save(struct env_driver *drv)puts ("done\n" ); - gd->env_valid = gd->env_valid ==ENV_REDUND ?ENV_VALID :ENV_REDUND ; + gd->env_infos[drv->location].env_valid = gd->env_infos[drv->location].env_valid ==ENV_REDUND ?ENV_VALID :ENV_REDUND ;return 0 ; }
include/asm-generic/global_data.h +2 -3
@@ -23,6 +23,7 @@ #ifndef __ASSEMBLY__ #include <membuff.h> #include <linux/list.h> +#include <environment.h> typedef struct global_data { bd_t *bd;
@@ -48,9 +49,7 @@ typedef struct global_data {#if CONFIG_IS_ENABLED (PRE_CONSOLE_BUFFER )unsigned long precon_buf_idx;/* Pre-Console buffer index */ #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_info env_infos[ENVL_COUNT ];unsigned long ram_top;/* Top address of RAM used by U-Boot */ unsigned long relocaddr;/* Start address of U-Boot in RAM */
Signature · 1 line
git-series 0.9.1
cover Introduce variables whitelisting in environment
In reply to Quentin SchulzHi Quentin,
Quoted 5 lines
2) Why do we consider
ENV_IS_NOWHEREan invalid environment? The only place I found a use for it was to just say that if the environment is invalid, we should set to default environment (inenv_relocateinenv/common.c). With my patch series I guess that we could remove this fallback and forceENV_IS_NOWHEREto be always there.
It is perfectly valid to use ENV_IS_NOWHERE to get envs from hardcoded board file (for example for production u-boot).
However, some time ago we had extension in the envs to get the support for envs from different mediums (with set priorities).
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de -------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171226/1b8894d9/attachment.sig>
cover Introduce variables whitelisting in environment
In reply to Quentin SchulzHi Quentin,
Quoted 41 lines · Quentin Schulz
This patch series is based on this[1] patch series from Maxime.
This is an RFC. It's been only tested in a specific use case on a custom i.MX6 board. It's known to break compilation on a few boards.
I have a use case where we want some variables from a first environment to be overriden by variables from a second environment. For example, we want to load variables from the default env (
ENV_IS_NOWHERE) and then load only a handful of other variables from, e.g., NAND.In our use case, we basically can be sure that the default env in the U-Boot binary is secure but we want only a few variables to be modified, thus keeping control over the overall behaviour of U-Boot in secure mode.
It works in that way:
- from highest to lowest priority, the first environment that can be loaded (that has successfully init and whose load function has returned no errors) will be the main environment,
- then, all the following environment that could be successfully loaded (same conditions as the main environment) are secondary environment. The env variables that are defined both inCONFIG_ENV_VAR_WHITELIST_LISTand in the secondary environments override the ones in the main environment,
- for saving, we save the whole environment to all environments available, be they main or secondary (it does not matter to save the whole environment on secondary environments as only the whitelisted variables will be overriden in the loading process,I have also a few questions that could help me to get the whole thing to work.
1) I can't really get my head around the use of
gd->env_addr, what is it used for? It is set in a bunch of different places but only once is it explicitly used (basically to alternate theenv_addrbetween the one associated to main and redundant environment (in NAND for example)).2) Why do we consider
ENV_IS_NOWHEREan invalid environment? The only place I found a use for it was to just say that if the environment is invalid, we should set to default environment (inenv_relocateinenv/common.c). With my patch series I guess that we could remove this fallback and forceENV_IS_NOWHEREto be always there.
Quoted 6 lines
3) There are a few (20) boards that set
gd->env_addrandgd->env_validin their board file. What is the reason to do such a thing? Isn't those overriden anyway by the environment driver?I'm looking forward to getting your feedback on this patch series.
I certainly understand the goal and it seems generally useful.
But I wonder whether this is the best way to implement it.
We could have a UCLASS_ENV uclass, with driver-model drivers which load the environment (i.e. have load() and save() methods). Config for the drivers would come from the device tree. Useful drivers would be:
- one that loads the env from a single location
- one that loads multiple envs from different locations in priority order
- one that does what you want
Then people could set their own policy by adding a driver.
I worry that what we have here is quite heavyweight, and will be imposed on all users, e.g. the size increase of gd, the new logic. Where does it end? I think splitting things up into different use cases makes sense.
When I did the env refactor I envisaged using driver-model for the post-reloc env load/save at some point in the future. It solves the problem of getting the config (can use device tree) and different boards wanting to do different things (boards can provide an env driver).
Regards,
Simon
Quoted 46 lines
Thanks,
Quentin[1] https://patchwork.ozlabs.org/cover/842057/
Quentin Schulz (11):
env: fix ret not being set and fails when no env could have been init
lib: hashtable: support whitelisting env variables
env: add support for whitelisting variables from secondary environments
env: make nowhere an env medium like the others
cmd: saveenv: enable the saveenv command whenENV_IS_NOWHEREis defined but another env medium is enabled too
env: addenv_drivertoenv_driverfunctions' arguments
env: gd flags withoutENV_READYis enough to discriminate inenv_get_default
env: addenv_driverparameter toenv_import_redund
env: makeenv_locationsa global variable
env: introducingenv_infostruct for storing info per env
env: store information about each environment in gdboard/sunxi/board.c | 2 +- cmd/nvedit.c | 16 ++- common/board_r.c | 8 +- env/Kconfig | 29 +++--- env/common.c | 45 ++++++---- env/eeprom.c | 40 ++++----- env/env.c | 142 +++++++++++++++++++++++++------ env/ext4.c | 4 +- env/fat.c | 4 +- env/flash.c | 58 ++++++------- env/mmc.c | 14 +-- env/nand.c | 46 +++++----- env/nowhere.c | 12 ++- env/nvram.c | 18 ++-- env/onenand.c | 6 +- env/remote.c | 10 +- env/sata.c | 4 +- env/sf.c | 34 +++---- env/ubi.c | 14 +-- include/asm-generic/global_data.h | 5 +- include/environment.h | 59 ++++++++----- include/search.h | 2 +- lib/hashtable.c | 17 +++- 23 files changed, 379 insertions(+), 210 deletions(-)base-commit:
5d41e28058e7b378c9fa5c61ecc074a682ba2db4--
git-series 0.9.1
cover Introduce variables whitelisting in environment
Hi Simon,
Quoted 52 lines · Simon Glass
Hi Quentin,
Quoted 41 lines · Quentin Schulz
This patch series is based on this[1] patch series from Maxime.
This is an RFC. It's been only tested in a specific use case on a custom i.MX6 board. It's known to break compilation on a few boards.
I have a use case where we want some variables from a first environment to be overriden by variables from a second environment. For example, we want to load variables from the default env (
ENV_IS_NOWHERE) and then load only a handful of other variables from, e.g., NAND.In our use case, we basically can be sure that the default env in the U-Boot binary is secure but we want only a few variables to be modified, thus keeping control over the overall behaviour of U-Boot in secure mode.
It works in that way:
- from highest to lowest priority, the first environment that can be loaded (that has successfully init and whose load function has returned no errors) will be the main environment,
- then, all the following environment that could be successfully loaded (same conditions as the main environment) are secondary environment. The env variables that are defined both inCONFIG_ENV_VAR_WHITELIST_LISTand in the secondary environments override the ones in the main environment,
- for saving, we save the whole environment to all environments available, be they main or secondary (it does not matter to save the whole environment on secondary environments as only the whitelisted variables will be overriden in the loading process,I have also a few questions that could help me to get the whole thing to work.
1) I can't really get my head around the use of
gd->env_addr, what is it used for? It is set in a bunch of different places but only once is it explicitly used (basically to alternate theenv_addrbetween the one associated to main and redundant environment (in NAND for example)).2) Why do we consider
ENV_IS_NOWHEREan invalid environment? The only place I found a use for it was to just say that if the environment is invalid, we should set to default environment (inenv_relocateinenv/common.c). With my patch series I guess that we could remove this fallback and forceENV_IS_NOWHEREto be always there.Quoted 6 lines
3) There are a few (20) boards that set
gd->env_addrandgd->env_validin their board file. What is the reason to do such a thing? Isn't those overriden anyway by the environment driver?I'm looking forward to getting your feedback on this patch series.
Quoted 8 lines
I certainly understand the goal and it seems generally useful.
But I wonder whether this is the best way to implement it.
We could have a
UCLASS_ENVuclass, with driver-model drivers which load the environment (i.e. haveload()andsave()methods). Config for the drivers would come from the device tree. Useful drivers would be:
I'm not sure the device tree is the place to set/get such info. That has nothing to do with hardware, it's only logic for the bootloader.
Quoted 6 lines
- one that loads the env from a single location
- one that loads multiple envs from different locations in priority order
- one that does what you wantThen people could set their own policy by adding a driver.
I'll have to document myself on driver-model and how U-Boot implement it then :)
Quoted 5 lines
I worry that what we have here is quite heavyweight, and will be imposed on all users, e.g. the size increase of gd, the new logic. Where does it end? I think splitting things up into different use cases makes sense.
Agree on that.
Quoted 6 lines
When I did the env refactor I envisaged using driver-model for the post-reloc env load/save at some point in the future. It solves the problem of getting the config (can use device tree) and different boards wanting to do different things (boards can provide an env driver).
Overall, I prefer Lukasz's suggestion as it's quicker and easier to implement and require (I think) less changes in the code.
Thanks for the review,
Quentin
Signature · 3 lines
Quentin Schulz, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
cover Introduce variables whitelisting in environment
Hi Quentin,
Quoted 92 lines · Quentin Schulz
Hi Simon,
Quoted 52 lines · Simon Glass
Hi Quentin,
Quoted 41 lines · Quentin Schulz
This patch series is based on this[1] patch series from Maxime.
This is an RFC. It's been only tested in a specific use case on a custom i.MX6 board. It's known to break compilation on a few boards.
I have a use case where we want some variables from a first environment to be overriden by variables from a second environment. For example, we want to load variables from the default env (
ENV_IS_NOWHERE) and then load only a handful of other variables from, e.g., NAND.In our use case, we basically can be sure that the default env in the U-Boot binary is secure but we want only a few variables to be modified, thus keeping control over the overall behaviour of U-Boot in secure mode.
It works in that way:
- from highest to lowest priority, the first environment that can be loaded (that has successfully init and whose load function has returned no errors) will be the main environment,
- then, all the following environment that could be successfully loaded (same conditions as the main environment) are secondary environment. The env variables that are defined both inCONFIG_ENV_VAR_WHITELIST_LISTand in the secondary environments override the ones in the main environment,
- for saving, we save the whole environment to all environments available, be they main or secondary (it does not matter to save the whole environment on secondary environments as only the whitelisted variables will be overriden in the loading process,I have also a few questions that could help me to get the whole thing to work.
1) I can't really get my head around the use of
gd->env_addr, what is it used for? It is set in a bunch of different places but only once is it explicitly used (basically to alternate theenv_addrbetween the one associated to main and redundant environment (in NAND for example)).2) Why do we consider
ENV_IS_NOWHEREan invalid environment? The only place I found a use for it was to just say that if the environment is invalid, we should set to default environment (inenv_relocateinenv/common.c). With my patch series I guess that we could remove this fallback and forceENV_IS_NOWHEREto be always there.Quoted 6 lines
3) There are a few (20) boards that set
gd->env_addrandgd->env_validin their board file. What is the reason to do such a thing? Isn't those overriden anyway by the environment driver?I'm looking forward to getting your feedback on this patch series.
Quoted 8 lines
I certainly understand the goal and it seems generally useful.
But I wonder whether this is the best way to implement it.
We could have a
UCLASS_ENVuclass, with driver-model drivers which load the environment (i.e. haveload()andsave()methods). Config for the drivers would come from the device tree. Useful drivers would be:I'm not sure the device tree is the place to set/get such info. That has nothing to do with hardware, it's only logic for the bootloader.
Quoted 6 lines
- one that loads the env from a single location
- one that loads multiple envs from different locations in priority order
- one that does what you wantThen people could set their own policy by adding a driver.
I'll have to document myself on driver-model and how U-Boot implement it then :)
Quoted 5 lines
I worry that what we have here is quite heavyweight, and will be imposed on all users, e.g. the size increase of gd, the new logic. Where does it end? I think splitting things up into different use cases makes sense.
Agree on that.
Quoted 6 lines
When I did the env refactor I envisaged using driver-model for the post-reloc env load/save at some point in the future. It solves the problem of getting the config (can use device tree) and different boards wanting to do different things (boards can provide an env driver).
Quoted 2 lines
Overall, I prefer Lukasz's suggestion as it's quicker and easier to implement and require (I think) less changes in the code.
Do you mean selecting from different locations? Yes that is a good thing, but is orthogonal to this series.
Here you are trying to add functionality which will apply to any env location, or to them as a whole. So we should think of your change as implementing a new policy rather than a new env location.
Regards,
Simon