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

Harry Waschkeit harry.waschkeit at men.de
Thu Jan 28 11:00:30 CET 2021


Hi Stefan,

thanks a lot for your prompt reply :-)

And sorry that I didn't manage to continue on that for such a long time ...


On 28.01.21 09:50, Stefan Roese wrote:
> Hi Harry,
> 
> On 28.01.21 08: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 at men.de>
> 
> I like this idea very much, thanks for working on this. One comment
> below..
> 
>> ---
>>   env/sf.c | 132 ++++++++++++++++++-------------------------------------
>>   1 file changed, 43 insertions(+), 89 deletions(-)
>>
>> diff --git a/env/sf.c b/env/sf.c
>> index 937778aa37..c60bd1deed 100644
>> --- a/env/sf.c
>> +++ b/env/sf.c
>> @@ -66,13 +66,16 @@ 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;
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +    char    flag = ENV_REDUND_OBSOLETE;
>> +#endif
> 
> No more #idef's if possible please. See below...

As checkpatch.pl throws warnings for each such #if[def] occurrence, I already tried to get rid of them.
But I can't see how this shall be possible in a structure declaration.
I'm eager to learn how to do that if possible, though :-)

> 
>>       ret = setup_flash_device();
>>       if (ret)
>> @@ -81,27 +84,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
> 
> Can you please try to add this without adding #ifdef's but using
> if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead?

I tried that, too, but it was doomed to fail, because element flags in env_t structure is only defined if redundant environment is enabled.

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 :-)

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
> 


-- 
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



More information about the U-Boot mailing list