[PATCH] env: sf: single function env_sf_save()
Stefan Roese
sr at denx.de
Thu Jan 28 11:11:31 CET 2021
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?
>>
>>> 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.
> 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.
Perhaps somebody else has a quick idea on how to handle this without
introduncing #ifdef's here?
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
--
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