Re: [PATCH v4 2/3] env: Access Environment in SPI flashes before relocation

Hello Tom, Michael,

Am 31.10.2020 um 01:07 schrieb Tom Rini:

On Sat, Oct 31, 2020 at 12:51:27AM +0100, Michael Walle wrote:

...

Hi Heiko, Hi Tom,

Am 2020-10-10 10:28, schrieb Heiko Schocher:

...

Enable the new Kconfig option ENV_SPI_EARLY if you want to use Environment in SPI flash before relocation. Call env_init() and than you can use env_get_f() for accessing Environment variables.

Signed-off-by: Heiko Schocher hs@denx.de

Changes in v4:

• rebased to current master 5dcf7cc590

Changes in v3:

• env_sf_init_early() always return 0 now. If we do not return 0 in this function, env_set_inited() never get called, which has the consequence that env_load/save/erase never work, because they check if the init bit is set.

• add comment from Simon Glass

  • add missing function comments
  • use if(IS_ENABLED...)
  • drop extra brackets
  • let env_sf_init() decide, which function to call add comment that it is necessary to return env_sf_init() with 0.

  env/Kconfig | 8 +++++ env/sf.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig index c6ba08878d..393b191a30 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -376,6 +376,14 @@ config ENV_SPI_MODE Value of the SPI work mode for environment. See include/spi.h for value.

+config ENV_SPI_EARLY

• bool "Access Environment in SPI flashes before relocation"
• depends on ENV_IS_IN_SPI_FLASH
• help

•  Enable this if you want to use Environment in SPI flash
  

•  before relocation. Call env_init() and than you can use
  

•  env_get_f() for accessing Environment variables.
  

• config ENV_IS_IN_UBI bool "Environment in a UBI volume" depends on !CHAIN_OF_TRUST

diff --git a/env/sf.c b/env/sf.c index 937778aa37..2eb2de1a4e 100644 --- a/env/sf.c +++ b/env/sf.c @@ -287,7 +287,10 @@ __weak void *env_sf_get_env_addr(void) #endif

#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) -static int env_sf_init(void) +/*

• • check if Environment on CONFIG_ENV_ADDR is valid.
• */

+static int env_sf_init_addr(void) { env_t *env_ptr = (env_t *)env_sf_get_env_addr();

@@ -303,12 +306,103 @@ static int env_sf_init(void) } #endif

+#if defined(CONFIG_ENV_SPI_EARLY) +/*

• • early load environment from SPI flash (before relocation)
• • and check if it is valid.
• */

+static int env_sf_init_early(void) +{

• int ret;
• int read1_fail;
• int read2_fail;
• int crc1_ok;
• env_t *tmp_env2 = NULL;
• env_t *tmp_env1;
• /*

• * if malloc is not ready yet, we cannot use
  

• * this part yet.
  

• */
  

• if (!gd->malloc_limit)

• return -ENOENT;
  

• tmp_env1 = (env_t *)memalign(ARCH_DMA_MINALIGN,

• 	CONFIG_ENV_SIZE);
  

• if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))

• tmp_env2 = (env_t *)memalign(ARCH_DMA_MINALIGN,
  

• 			     CONFIG_ENV_SIZE);
  

• if (!tmp_env1 || !tmp_env2)

• goto out;
  

• ret = setup_flash_device();
• if (ret)

• goto out;
  

• read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,

• 		    CONFIG_ENV_SIZE, tmp_env1);
  

• if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT)) {

• read2_fail = spi_flash_read(env_flash,
  

• 			    CONFIG_ENV_OFFSET_REDUND,
  

• 			    CONFIG_ENV_SIZE, tmp_env2);
  

• ret = env_check_redund((char *)tmp_env1, read1_fail,
  

• 		       (char *)tmp_env2, read2_fail);
  

• if (ret == -EIO || ret == -ENOMSG)
  

• 	goto err_read;
  

• if (gd->env_valid == ENV_VALID)
  

• 	gd->env_addr = (unsigned long)&tmp_env1->data;
  

• else
  

• 	gd->env_addr = (unsigned long)&tmp_env2->data;
  

• } else {

• if (read1_fail)
  

• 	goto err_read;
  

• crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
  

• 		tmp_env1->crc;
  

• if (!crc1_ok)
  

• 	goto err_read;
  

• /* if valid -> this is our env */
  

• gd->env_valid = ENV_VALID;
  

• gd->env_addr = (unsigned long)&tmp_env1->data;
  

• }
• return 0;

+err_read:

• spi_flash_free(env_flash);
• env_flash = NULL;
• free(tmp_env1);
• if (IS_ENABLED(CONFIG_SYS_REDUNDAND_ENVIRONMENT))

• free(tmp_env2);
  

+out:

• /* env is not valid. always return 0 */
• gd->env_valid = ENV_INVALID;
• return 0;

+} +#endif

+static int env_sf_init(void) +{ +#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)

• return env_sf_init_addr();

+#elif defined(CONFIG_ENV_SPI_EARLY)

• return env_sf_init_early();

+#endif

• /*

• * we must return with 0 if there is nothing done,
  

• * else env_set_inited() get not called in env_init()
  

• */
  

• return 0;

+}

• U_BOOT_ENV_LOCATION(sf) = { .location = ENVL_SPI_FLASH, ENV_NAME("SPIFlash") .load = env_sf_load, .save = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) :

NULL, -#if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) .init = env_sf_init, -#endif

This will break my board (the new kontron sl28). Before the change, the environment is loaded from SPI, after this patch the environment won't be loaded anymore. If I delete the .init callback, the behavior is the same as before.

The problem here is that returning 0 in env_sf_init() is not enough because if the .init callback is not set, env_init() will have ret defaulting to -ENOENT and in this case will load the default environment and setting env_valid to ENV_VALID. I didn't follow the whole call chain then. But this will then eventually lead to loading the actual environment in a later stage.

Ugh. The games we play in the env area really just need to be rewritten. So at this point I've merged the series, should I revert or do you see an easy fix for your platform? Thanks!

Autsch ... sorry ...

Yes, env code is really ugly ...

env/env.c: 323 int env_init(void) 324 { 325 struct env_driver *drv; 326 int ret = -ENOENT; 327 int prio; 328 329 for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { 330 if (!drv->init || !(ret = drv->init())) 331 env_set_inited(drv->location); 332 333 debug("%s: Environment %s init done (ret=%d)\n", __func__, 334 drv->name, ret); 335 } 336 337 if (!prio) 338 return -ENODEV; 339 340 if (ret == -ENOENT) { 341 gd->env_addr = (ulong)&default_environment[0]; 342 gd->env_valid = ENV_VALID; 343 344 return 0; 345 } 346 347 return ret;

So if now the init function returns 0, env_set_inited() sets the init bit, but

gd->env_valid = ENV_VALID;

never set ... which leads in the error Michael see... as in

env/common.c 230 void env_relocate(void) [...] 237 if (gd->env_valid == ENV_INVALID) { 238 #if defined(CONFIG_ENV_IS_NOWHERE) || defined(CONFIG_SPL_BUILD) 239 /* Environment not changable */ 240 env_set_default(NULL, 0); 241 #else 242 bootstage_error(BOOTSTAGE_ID_NET_CHECKSUM); 243 env_set_default("bad CRC", 0); 244 #endif 245 } else { 246 env_load(); 247 }

env_load() never called ... on the other hand in env_load()

181 int env_load(void) 182 { 183 struct env_driver *drv; 184 int best_prio = -1; 185 int prio; 186 187 for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { 188 int ret; 189 190 if (!env_has_inited(drv->location)) 191 continue;

if the init bit is not set, it never loads from storage device here.

:-(

So I tend to say, we must prevent registering ".init" with some ifdeffery ... as I had it in v1 of this series ...

:-(

bye, Heiko