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

[Harry Waschkeit]

Hi again Stefan,

On 29.01.21 08:16, Stefan Roese wrote:

On 28.01.21 12:21, Harry Waschkeit wrote:

<snip>

Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate to '0' 
without active redundancy in environments, the parser sees the syntax error of the 
non-existing structure element.

So, I don't see a chance for avoiding the #if construct here, too.
Let me know if I miss something :-)

I agree its not easy or perhaps even impossible. One idea would be
to introduce a union in the struct with "flags" also defines in the
non-redundant case but not being used here. But this would result
in very non-intuitive code AFAICT. So better not go this way.

That would feel very strange to me, too.

Perhaps somebody else has a quick idea on how to handle this without
introduncing #ifdef's here?

It would be possible to introduce a macro like env_set_flags(envp, flag) in 
env.h that only evaluates to something in case of active redundancy, sure.

But that's not even half of a solution, because also variables env_offset and 
env_new_offset which are set in that section are only defined if necessary, 
i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set (btw also protected by #if 
while not possible otherwise).

The trade-off here is between code duplication - which I removed by combining 
two very similar separate functions for the two situations w/ and w/o 
redundancy in one - and in-line pre-compiler protected regions within one 
function.

While I fully agree that the latter should be avoided as far as possible, it is 
not avoidable here afaics.

And avoiding code duplication here outweighs the few pre-compiler occurrences 
in my eyes, but that's just my € 0,02 :-)

I also agree (now). If nobody else comes up with a better idea, then
please proceed with the current implementation to remove the code
duplication.

sorry for the newbie question, I'm not familiar (yet) with the normal procedure 
and I don't want to keep that ball from rolling ;-/

As far as I understood you finally agreed on my patch, but you didn't give it a 
"reviewed-by" all the same; so do you still expect me to change my patch in some way or 
are you waiting for a "reviewed-by" from someone else to give that a go?

Why I am also asking: I have another small patch in he pipe that bases on the changed 
sources and that adds "env erase" support on top of that :-)

Thanks,
Harry

Thanks,
Stefan

Viele Grüße,
Harry

Thanks,
Stefan

Cheers,
Harry

Thanks,
Stefan

      /* 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 +118,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 +159,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 +197,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 +212,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 +246,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

Viele Grüße,
Stefan

Viele Grüße,
Stefan

--
Harry Waschkeit - Software Engineer