[PATCH] env: sf: single function env_sf_save()
Stefan Roese
sr at denx.de
Sat Jan 30 10:01:42 CET 2021
Hi Harry,
On 29.01.21 18:18, Harry Waschkeit wrote:
> 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 ;-/
No Problem. Please double-check if not 100% sure. ;)
> 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 :-)
I didn't do a thorough review yet. Let me try to do this quickly...
Thanks,
Stefan
> 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
>>
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