| Message ID | 20190717082525.891-5-takahiro.akashi@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: non-volatile variables support | expand |
Dear AKASHI Takahiro, In message <20190717082525.891-5-takahiro.akashi@linaro.org> you wrote: > This patch shows how environment storage drivers should be modified > at the minimum to support contexts. This commit message is misleading. No part of this patch is related to contexts. What it acually does is adding support for variable sized environment blocks. The commit message should be fixed. Also this commit makes me wonder if you have tested your patches for bisectability. it seems changes that belong to together (like making the envionment size variable) are split across patches. This needs probably refacturing / resorting. Best regards, Wolfgang Denk
On Fri, Jul 19, 2019 at 10:14:06AM +0200, Wolfgang Denk wrote: > Dear AKASHI Takahiro, > > In message <20190717082525.891-5-takahiro.akashi@linaro.org> you wrote: > > This patch shows how environment storage drivers should be modified > > at the minimum to support contexts. > > This commit message is misleading. No part of this patch is related > to contexts. What it acually does is adding support for variable > sized environment blocks. The commit message should be fixed. Please note that this patch (or even all the patches in this set) are not intended to be merged as they are. This is an RFC in order for me to determine if you agree with the approach I take here or not. This is the primary goal. The code itself is nothing but a help for your understandings of my basic ideas. > > Also this commit makes me wonder if you have tested your patches for > bisectability. Again, not. Thanks, -Takahiro Akashi > it seems changes that belong to together (like > making the envionment size variable) are split across patches. This > needs probably refacturing / resorting. > > Best regards, > > Wolfgang Denk > > -- > 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@denx.de > What can it profit a man to gain the whole world and to come to his > property with a gastric ulcer, a blown prostate, and bifocals? > -- John Steinbeck, _Cannery Row_
Dear Takahiro, In message <20190719083010.GT21948@linaro.org> you wrote: > On Fri, Jul 19, 2019 at 10:14:06AM +0200, Wolfgang Denk wrote: > > Dear AKASHI Takahiro, > > > > In message <20190717082525.891-5-takahiro.akashi@linaro.org> you wrote: > > > This patch shows how environment storage drivers should be modified > > > at the minimum to support contexts. > > > > This commit message is misleading. No part of this patch is related > > to contexts. What it acually does is adding support for variable > > sized environment blocks. The commit message should be fixed. > > Please note that this patch (or even all the patches in this set) > are not intended to be merged as they are. > This is an RFC in order for me to determine if you agree with > the approach I take here or not. This is the primary goal. > The code itself is nothing but a help for your understandings > of my basic ideas. Agreed. Please consider my comment as help for restructuring this patrch set and improving commit messages. > > Also this commit makes me wonder if you have tested your patches for > > bisectability. > > Again, not. I already mentioned that I think the whole patch set should be restructured. For example, adding support for volatile vaariables is one logical thing; adding support for auto-save another one; extending the env (and eventually called driver) functions by a dummy context parameter (which is initially 0 everywhere, and unused in the code) is a single step before you actually introduce contexts, etc. For each step, code size changes (especially for SPL) and bisectability shpuld be checked. But these are actually general rules for all patches ;-) Best regards, Wolfgang Denk
diff --git a/env/flash.c b/env/flash.c index dca6567a097d..e1afd8358f30 100644 --- a/env/flash.c +++ b/env/flash.c @@ -47,13 +47,13 @@ DECLARE_GLOBAL_DATA_PTR; #if defined(CONFIG_ENV_ADDR_REDUND) && defined(CMD_SAVEENV) || \ !defined(CONFIG_ENV_ADDR_REDUND) && defined(INITENV) #ifdef ENV_IS_EMBEDDED -static env_t *env_ptr = &environment; +static env_hdr_t *env_ptr = &environment; #else /* ! ENV_IS_EMBEDDED */ -static env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR; +static env_hdr_t *env_ptr = (env_hdr_t *)CONFIG_ENV_ADDR; #endif /* ENV_IS_EMBEDDED */ #endif -static __maybe_unused env_t *flash_addr = (env_t *)CONFIG_ENV_ADDR; +static __maybe_unused env_hdr_t *flash_addr = (env_hdr_t *)CONFIG_ENV_ADDR; /* CONFIG_ENV_ADDR is supposed to be on sector boundary */ static ulong __maybe_unused end_addr = @@ -61,7 +61,8 @@ static ulong __maybe_unused end_addr = #ifdef CONFIG_ENV_ADDR_REDUND -static env_t __maybe_unused *flash_addr_new = (env_t *)CONFIG_ENV_ADDR_REDUND; +static env_hdr_t __maybe_unused *flash_addr_new = + (env_hdr_t *)CONFIG_ENV_ADDR_REDUND; /* CONFIG_ENV_ADDR_REDUND is supposed to be on sector boundary */ static ulong __maybe_unused end_addr_new = @@ -81,9 +82,10 @@ static int env_flash_init(void) ulong addr1 = (ulong)&(flash_addr->data); ulong addr2 = (ulong)&(flash_addr_new->data); - crc1_ok = crc32(0, flash_addr->data, ENV_SIZE) == flash_addr->crc; - crc2_ok = - crc32(0, flash_addr_new->data, ENV_SIZE) == flash_addr_new->crc; + crc1_ok = crc32(0, flash_addr->data, flash_addr->data_size) + == flash_addr->crc; + crc2_ok = crc32(0, flash_addr_new->data, flash_addr->data_size) + == flash_addr_new->crc; if (crc1_ok && !crc2_ok) { gd->env_addr = addr1; @@ -118,7 +120,7 @@ static int env_flash_init(void) #ifdef CMD_SAVEENV static int env_flash_save(void) { - env_t env_new; + env_hdr_t env_new; char *saved_data = NULL; char flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG; int rc = 1; @@ -193,7 +195,7 @@ static int env_flash_save(void) puts("done\n"); { - env_t *etmp = flash_addr; + env_hdr_t *etmp = flash_addr; ulong ltmp = end_addr; flash_addr = flash_addr_new; @@ -223,7 +225,7 @@ done: #ifdef INITENV static int env_flash_init(void) { - if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { + if (crc32(0, env_ptr->data, env_ptr->data_size) == env_ptr->crc) { gd->env_addr = (ulong)&(env_ptr->data); gd->env_valid = ENV_VALID; return 0; @@ -267,6 +269,7 @@ static int env_flash_save(void) if (flash_sect_protect(0, (long)flash_addr, end_addr)) goto done; + env_new.data_size = ENV_SIZE; rc = env_export(&env_new); if (rc) goto done; @@ -311,7 +314,7 @@ static int env_flash_load(void) { #ifdef CONFIG_ENV_ADDR_REDUND if (gd->env_addr != (ulong)&(flash_addr->data)) { - env_t *etmp = flash_addr; + env_hdr_t *etmp = flash_addr; ulong ltmp = end_addr; flash_addr = flash_addr_new; @@ -322,7 +325,8 @@ static int env_flash_load(void) } if (flash_addr_new->flags != OBSOLETE_FLAG && - crc32(0, flash_addr_new->data, ENV_SIZE) == flash_addr_new->crc) { + crc32(0, flash_addr_new->data, flash_addr_new->data_size) + == flash_addr_new->crc) { char flag = OBSOLETE_FLAG; gd->env_valid = ENV_REDUND;
This patch shows how environment storage drivers should be modified at the minimum to support contexts. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- env/flash.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)