Re: [PATCH v3 1/1] env: sf: single function env_sf_save()

On 3/1/21 11:39 AM, Harry Waschkeit wrote:

Hi again,

gentle ping for that patch, also in view of subsequently sent patch ...

https://lists.denx.de/pipermail/u-boot/2021-February/439797.html

... which saw a competing patch by Patrick Delaunay a week later:

https://lists.denx.de/pipermail/u-boot/2021-February/440769.html

However, the latter doesn't seem to take embedded environments into account.

Can you give an example of where your patch would work while Patrick's wouldn't?

Thanks.

--Sean

Best regards, Harry

On 02.02.21 09:21, Harry Waschkeit wrote:

...

Instead of implementing redundant environments in two very similar functions env_sf_save(), handle redundancy in one function, placing the few differences in appropriate pre-compiler sections depending on config option CONFIG_ENV_OFFSET_REDUND.

Additionally, several checkpatch complaints were addressed.

This patch is in preparation for adding support for env erase.

Signed-off-by: Harry Waschkeit Harry.Waschkeit@men.de Reviewed-by: Stefan Roese sr@denx.de

---

Change in v3:

• no change in patch, only added "reviewed-by" to commit log

Change in v2:

• remove one more #ifdef, instead take advantage of compiler attribute __maybe_unused for one variable used only in case of redundant environments

env/sf.c | 130 ++++++++++++++++++------------------------------------- 1 file changed, 41 insertions(+), 89 deletions(-)

diff --git a/env/sf.c b/env/sf.c index 937778aa37..199114fd3b 100644 --- a/env/sf.c +++ b/env/sf.c @@ -66,13 +66,14 @@ static int setup_flash_device(void) return 0; } -#if defined(CONFIG_ENV_OFFSET_REDUND) static int env_sf_save(void) { env_t env_new;

• char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;

• char *saved_buffer = NULL; u32 saved_size, saved_offset, sector;
• ulong offset; int ret;
• __maybe_unused char flag = ENV_REDUND_OBSOLETE; ret = setup_flash_device(); if (ret)

@@ -81,27 +82,33 @@ static int env_sf_save(void) ret = env_export(&env_new); if (ret) return -EIO;

• env_new.flags = ENV_REDUND_ACTIVE;
• if (gd->env_valid == ENV_VALID) {

•    env_new_offset = CONFIG_ENV_OFFSET_REDUND;
  

•    env_offset = CONFIG_ENV_OFFSET;
  

• } else {

•    env_new_offset = CONFIG_ENV_OFFSET;
  

•    env_offset = CONFIG_ENV_OFFSET_REDUND;
  

• }

+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)

•    env_new.flags    = ENV_REDUND_ACTIVE;
  

•    if (gd->env_valid == ENV_VALID) {
  

•        env_new_offset = CONFIG_ENV_OFFSET_REDUND;
  

•        env_offset = CONFIG_ENV_OFFSET;
  

•    } else {
  

•        env_new_offset = CONFIG_ENV_OFFSET;
  

•        env_offset = CONFIG_ENV_OFFSET_REDUND;
  

•    }
  

•    offset = env_new_offset;
  

+#else

•    offset = CONFIG_ENV_OFFSET;
  

+#endif /* Is the sector larger than the env (i.e. embedded) */ if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;

•    saved_offset = env_new_offset + CONFIG_ENV_SIZE;
  

•    saved_offset = offset + CONFIG_ENV_SIZE;
      saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
      if (!saved_buffer) {
          ret = -ENOMEM;
          goto done;
      }
  

•    ret = spi_flash_read(env_flash, saved_offset,
  

•                saved_size, saved_buffer);
  

•    ret = spi_flash_read(env_flash, saved_offset, saved_size,
  

•                 saved_buffer);
      if (ret)
          goto done;
  }
  

@@ -109,35 +116,39 @@ static int env_sf_save(void) sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); puts("Erasing SPI flash...");

• ret = spi_flash_erase(env_flash, env_new_offset,

•            sector * CONFIG_ENV_SECT_SIZE);
  

• ret = spi_flash_erase(env_flash, offset,

•              sector * CONFIG_ENV_SECT_SIZE);
  if (ret)
      goto done;
  puts("Writing to SPI flash...");
  

• ret = spi_flash_write(env_flash, env_new_offset,

•    CONFIG_ENV_SIZE, &env_new);
  

• ret = spi_flash_write(env_flash, offset,

•              CONFIG_ENV_SIZE, &env_new);
  if (ret)
      goto done;
  if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
  

•    ret = spi_flash_write(env_flash, saved_offset,
  

•                saved_size, saved_buffer);
  

•    ret = spi_flash_write(env_flash, saved_offset, saved_size,
  

•                  saved_buffer);
      if (ret)
          goto done;
  }
  

• ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),

•            sizeof(env_new.flags), &flag);
  

• if (ret)

•    goto done;
  

+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)

•    ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
  

•                  sizeof(env_new.flags), &flag);
  

•    if (ret)
  

•        goto done;
  

• puts("done\n");

•    puts("done\n");
  

• gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;

•    gd->env_valid = gd->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_valid);
  

+#else

•    puts("done\n");
  

+#endif done: if (saved_buffer) @@ -146,6 +157,7 @@ static int env_sf_save(void) return ret; } +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) static int env_sf_load(void) { int ret; @@ -183,66 +195,6 @@ out: return ret; } #else -static int env_sf_save(void) -{

• u32 saved_size, saved_offset, sector;
• char *saved_buffer = NULL;
• int ret = 1;
• env_t env_new;
• ret = setup_flash_device();
• if (ret)

•    return ret;
  

• /* Is the sector larger than the env (i.e. embedded) */
• if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {

•    saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
  

•    saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
  

•    saved_buffer = malloc(saved_size);
  

•    if (!saved_buffer)
  

•        goto done;
  

•    ret = spi_flash_read(env_flash, saved_offset,
  

•        saved_size, saved_buffer);
  

•    if (ret)
  

•        goto done;
  

• }
• ret = env_export(&env_new);
• if (ret)

•    goto done;
  

• sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
• puts("Erasing SPI flash...");
• ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,

•    sector * CONFIG_ENV_SECT_SIZE);
  

• if (ret)

•    goto done;
  

• puts("Writing to SPI flash...");
• ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,

•    CONFIG_ENV_SIZE, &env_new);
  

• if (ret)

•    goto done;
  

• if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {

•    ret = spi_flash_write(env_flash, saved_offset,
  

•        saved_size, saved_buffer);
  

•    if (ret)
  

•        goto done;
  

• }
• ret = 0;
• puts("done\n");
• done:
• if (saved_buffer)

•    free(saved_buffer);
  

• return ret;

-}

• static int env_sf_load(void) { int ret;

@@ -258,8 +210,8 @@ static int env_sf_load(void) if (ret) goto out;

• ret = spi_flash_read(env_flash,

•    CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
  

• ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,

•             buf);
  if (ret) {
      env_set_default("spi_flash_read() failed", 0);
      goto err_read;
  

@@ -292,7 +244,7 @@ static int env_sf_init(void) env_t *env_ptr = (env_t *)env_sf_get_env_addr(); if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {

•    gd->env_addr    = (ulong)&(env_ptr->data);
  

•    gd->env_addr    = (ulong)&env_ptr->data;
      gd->env_valid    = 1;
  } else {
      gd->env_addr = (ulong)&default_environment[0];