[PATCH] env: sf: single function env_sf_save()
Harry Waschkeit
harry.waschkeit at men.de
Fri Jan 29 18:18:10 CET 2021
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
More information about the U-Boot
mailing list