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

Stefan Roese sr at denx.de
Fri Jan 29 08:16:23 CET 2021


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.

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de


More information about the U-Boot mailing list