[PATCH v3 1/1] env: sf: single function env_sf_save()

[Harry Waschkeit <harry.waschkeit@men.de>]

On 02.02.21 15:54, Stefan Roese wrote:
> On 02.02.21 15:43, Harry Waschkeit wrote:
>> ?
>> On 02.02.21 10:30, Stefan Roese wrote:
>>> On 02.02.21 09: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@men.de>
>>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>>> ---
>>>> Change in v3:
>>>> ? - no change in patch, only added "reviewed-by" to commit log
>>>
>>> JFYI:
>>> No need to re-send this patch with this added RB tag, as I already did
>>> send a new RB to the last mail as reply. Patchwork collects these tags
>>> when sent to the list. So you only need to include them, if you send
>>> a new patch version.
>>
>> thanks for this hint, obviously I step into it all the time ...
>>
>> Ok, lesson learnt, let's see what I can do wrong next time ... ;-/
>>
>> Back on topic: I guess that was all I needed to do so that the patch will get merged when its time comes.
> 
> Yes, now we (you) need a bit of patience, so that other people might
> review this patch as well. And usually it will get handled after some
> time (depending on the development stage of U-Boot, merge window open
> or shortly before release etc).

Ok, no problem with that.

That also means that I should wait with submission of the other patch (adding "env erase" support) until this one is merged.
Otherwise the patch would need to be significantly different ...

Hmm, I guess it would have been better to not send that patch standalone but instead as a first one in a two-patch series where the second one adds the new functionality on top of the clean-up.
Anyway, something to keep in mind for next time :-)

> It does not hurt of course to check this from time to time and
> "trigger" the maintainer of the subsystem or the custodian this
> patch is delegated to nothing is happening here for a too long
> time (like more than 1 month).

Alright, good to know :-)

Thanks,
Harry

> Thanks,
> Stefan
> 
>> If not, please let me know.
>>
>> Thanks,
>> Harry
>>
>>> Thanks,
>>> Stefan
>>>
>>>> Change in v2:
>>>> ? - remove one more #ifdef, instead take advantage of compiler attribute
>>>> ??? __maybe_unused for one variable used only in case of redundant
>>>> ??? environments
>>>>
>>>> ? env/sf.c | 130 ++++++++++++++++++-------------------------------------
>>>> ? 1 file changed, 41 insertions(+), 89 deletions(-)
>>>>
>>>> diff --git a/env/sf.c b/env/sf.c
>>>> index 937778aa37..199114fd3b 100644
>>>> --- a/env/sf.c
>>>> +++ b/env/sf.c
>>>> @@ -66,13 +66,14 @@ 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;
>>>> +??? __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>>>> ????? ret = setup_flash_device();
>>>> ????? if (ret)
>>>> @@ -81,27 +82,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
>>>> ????? /* 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 +116,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 +157,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 +195,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 +210,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 +244,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