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

2 Feb 2021

On 02.02.21 10:30, Stefan Roese wrote:
...
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
JFYI:
No need to re-send this patch with this added RB tag, as I already did
send a new RB to the last mail as reply. Patchwork collects these tags
when sent to the list. So you only need to include them, if you send
a new patch version.
thanks for this hint, obviously I step into it all the time ...
Ok, lesson learnt, let's see what I can do wrong next time ... ;-/
Back on topic: I guess that was all I needed to do so that the patch will get merged when its time comes.
If not, please let me know.
Thanks,
Harry
...
Thanks,
Stefan
...
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];
Viele Grüße,
Stefan
-- 
Harry Waschkeit - Software Engineer
duagon Germany GmbH
Neuwieder Straße 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott - Handelsregister AG Nuernberg HRB 5540


    