[PATCH] env: sf: single function env_sf_save()
Harry Waschkeit
harry.waschkeit at men.de
Thu Jan 28 12:21:59 CET 2021
On 28.01.21 11:11, Stefan Roese wrote:
> Hi Harry,
>
> On 28.01.21 11:00, Harry Waschkeit wrote:
>> 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 :-)
>
> Ah, right. It's probably not possible for this for a struct declaration.
> But the variable above can be included always, right?
Sure it could, but then we would have to live with a compiler warning about an unused variable I think.
>>>
>>>> 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.
>
> Too bad we didn't include "flag" for non-redundant env in the beginning.
> Now its too late.
That would probably have been the best solution to avoid #if's, at least in that file.
But keep in mind: as a last consequence of this approach you would end up in structures bearing variables for all eventualities, however unlikely these are needed, blowing up the memory footprint for no reason.
In my opinion such things need to be thought through thoroughly :-)
>> 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 :-)
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
>
--
Harry Waschkeit - Software Engineer
More information about the U-Boot
mailing list