[PATCH] env: sf: add support for env erase
Harry Waschkeit
harry.waschkeit at men.de
Wed Oct 14 18:10:21 CEST 2020
On 10/9/20 7:00 PM, Sean Anderson wrote:
> On 10/9/20 12:43 PM, Harry Waschkeit wrote:
>> Hi Sean,
>>
>> thanks for your try and sorry for the inconvenience my beginner's mistakes have
>> caused :-(
>>
>> It is definitely no good idea to use copy&paste with patch data, I should have
>> guessed that beforehand ...
>
> You *can* do it, you just have to configure your mail client correctly.
> However, it gets pretty tedious when you have a lot of patches :)
Yeah, guess so ... ;-/
> I suggest configuring git send-email. If you are going to be making more
> patch series, also check out tools/patman.
I'll definitely have a look at that, sooner or later.
>>
>> Anyway, concerning the complaints from checkpatch.pl: I indeed ran checkpatch.pl
>> on my patch prior to sending it - the real one, not the one as pasted into the
>> mail ;-/
>>
>> The alignment warnings simply result from the fact that I adhered to the style
>> used in that file already, you can easily verify that by running checkpatch.pl
>> on the complete file.
>
> Please keep new code in the correct style. For example, you have
>
>> + ret = spi_flash_read(env_flash, saved_offset,
>> + saved_size, saved_buffer);
>
> which is aligned properly, but later on you have
>
>>>> + ret = spi_flash_read(env_flash, saved_offset,
>>>> + saved_size, saved_buffer);
>
> which is not.
Ok, got it.
>>
>> For the warnings with "IS_ENABLED(CONFIG...)" I don't see what I could do to
>> get around them: all three occurrences are about compiling functions into the
>> code depending on CONFIG_CMD_ERASEENV.
>> Two times it is the new function env_sf_erase(), one variant for normal and the
>> other for redundand environment handling.
>> The third time it is used to define this new method into the structure
>> U_BOOT_ENV_LOCATION(sf).
>
> The macro IS_ENABLED can be used both in C code and in preprocessor
> directives. See include/linux/kconfig.h for details.
Hmm, that's strange, I tried that one but the complaints remained.
Chances are I did something wrong so I will have a look at kconfig.h to get also
around that.
>>
>> The sign-off problem I guess is probably caused by the check not accepting name
>> in reverse order, isn't it?
>
> Yes. The format is "First Last <email at address>".
This will be the easiest part then :-)
>> Anyway, I will change my user.name to match the order in the mail address so
>> next patch is hopefully correct.
>>
>> So please let me know what else I should do beside sending a properly formatted
>> patch ;-/
>
> See below.
>
>> I will take care of that before resending my patch (v2 then, right?).
>
> Yes.
>
>>
>> On 10/9/20 3:55 PM, Sean Anderson wrote:
>>> On 10/8/20 1:27 PM, Harry Waschkeit wrote:
>>>> Command "env erase" didn't work even though CONFIG_CMD_ERASEENV was
>>>> defined, because serial flash environment routines didn't implement
>>>> erase method.
>>>>
>>>> Signed-off-by: Waschkeit, Harry <Harry.Waschkeit at men.de>
>>>> ---
>>>
>>> Hi Harry,
>>>
>>> I wanted to test out your patch, but I couldn't get it to apply. It
>>> appears that your mail program has replaced the tabs with spaces, so git
>>> can't figure out how to apply it. I tried to fix it by performing the
>>> substitutions
>>>
>>> s/^\(.\) /\1\t/g
>>> s/ /\t/g
>>>
>>> but it still wouldn't apply. In addition, checkpatch has a few warnings:
>>>
>>>> $ scripts/checkpatch.pl env-sf-add-support-for-env-erase.patch
>>>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>>>> #152: FILE: env/sf.c:149:
>>>> +#ifdef CONFIG_CMD_ERASEENV
>>>>
>>>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>>>> #240: FILE: env/sf.c:318:
>>>> +#ifdef CONFIG_CMD_ERASEENV
>>>>
>>>> CHECK: Alignment should match open parenthesis
>>>> #260: FILE: env/sf.c:338:
>>>> + ret = spi_flash_read(env_flash, saved_offset,
>>>> + saved_size, saved_buffer);
>>>>
>>>> CHECK: Alignment should match open parenthesis
>>>> #269: FILE: env/sf.c:347:
>>>> + ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>> + sector * CONFIG_ENV_SECT_SIZE);
>>>>
>>>> CHECK: Alignment should match open parenthesis
>>>> #276: FILE: env/sf.c:354:
>>>> + ret = spi_flash_write(env_flash, saved_offset,
>>>> + saved_size, saved_buffer);
>>>>
>>>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>>>> #307: FILE: env/sf.c:437:
>>>> +#if defined(CONFIG_CMD_ERASEENV) && defined(CONFIG_ENV_ADDR)
>>>>
>>>> WARNING: Missing Signed-off-by: line by nominal patch author 'Harry Waschkeit <harry.waschkeit at men.de>'
>>>>
>>>> total: 0 errors, 4 warnings, 3 checks, 158 lines checked
>>>>
>>>> NOTE: For some of the reported defects, checkpatch may be able to
>>>> mechanically convert to the typical style using --fix or --fix-inplace.
>>>>
>>>> env-sf-add-support-for-env-erase.patch has style problems, please review.
>>>>
>>>> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PREFER_ETHER_ADDR_COPY USLEEP_RANGE
>>>>
>>>> NOTE: If any of the errors are false positives, please report
>>>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>>
>>> Please fix these issues and resend, thanks!
>>>
>>> --Sean
>>>
>>>> env/sf.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 128 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/env/sf.c b/env/sf.c
>>>> index 937778aa37..9cda192a73 100644
>>>> --- a/env/sf.c
>>>> +++ b/env/sf.c
>>>> @@ -146,6 +146,78 @@ static int env_sf_save(void)
>>>> return ret;
>>>> }
>>>> +#ifdef CONFIG_CMD_ERASEENV
>>>> +static int env_sf_erase(void)
>>>> +{
>>>> + char *saved_buffer = NULL;
>>>> + u32 saved_size, saved_offset, sector;
>>>> + ulong offset;
>>>> + ulong offsets[2] = { CONFIG_ENV_OFFSET, CONFIG_ENV_OFFSET_REDUND };
>>>> + int i;
>>>> + int ret;
>>>> +
>>>> + ret = setup_flash_device();
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + /* get temporary storage if sector is larger than env (i.e. embedded) */
>>>> + if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> + saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>> + saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>> + if (!saved_buffer) {
>>>> + ret = -ENOMEM;
>>>> + goto done;
>>>> + }
>>>> + }
>
> Can this logic be split out into a separate function, since it is shared
> with env_sf_save? Perhaps make a function like env_sf_do_erase() and
> call it from env_sf_save and env_sf_erase?
Funnily enough, I did think about that for a short moment but cowardly
didn't dare restructuring such a central file with my first U-Boot patch
ever ...
But yeah, I fully agree, code replication of non-trivial things deserve
appropriate refactoring, I'll give that a try in my next patch.
>>>> +
>>>> + sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>> +
>>>> + /* simply erase both environments, retaining non-env data (if any) */
>>>> + for (i = 0; i < ARRAY_SIZE(offsets); i++) {
>>>> + offset = offsets[i];
>>>> +
>>>> + if (saved_buffer) {
>>>> + saved_offset = offset + CONFIG_ENV_SIZE;
>>>> + ret = spi_flash_read(env_flash, saved_offset,
>>>> + saved_size, saved_buffer);
>>>> + if (ret)
>>>> + goto done;
>>>> + }
>>>> +
>>>> + if (i)
>>>> + puts("Redund:");
>>>> +
>>>> + puts("Erasing SPI flash...");
>>>> + ret = spi_flash_erase(env_flash, offset,
>>>> + sector * CONFIG_ENV_SECT_SIZE);
>>>> + if (ret)
>>>> + goto done;
>>>> +
>>>> + if (saved_buffer) {
>>>> + puts("Writing non-environment data to SPI flash...");
>>>> + ret = spi_flash_write(env_flash, saved_offset,
>>>> + saved_size, saved_buffer);
>>>> + if (ret)
>>>> + goto done;
>>>> + }
>>>> +
>>>> + puts("done\n");
>>>> + }
>>>> +
>>>> + /* here we know that both env sections are cleared */
>>>> + env_new_offset = CONFIG_ENV_OFFSET;
>>>> + env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> +
>>>> + gd->env_valid = ENV_INVALID;
>>>> +
>>>> + done:
>>>> + if (saved_buffer)
>>>> + free(saved_buffer);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +#endif /* CONFIG_CMD_ERASEENV */
>>>> +
>>>> static int env_sf_load(void)
>>>> {
>>>> int ret;
>>>> @@ -182,7 +254,7 @@ out:
>>>> return ret;
>>>> }
>>>> -#else
>>>> +#else /* #if defined(CONFIG_ENV_OFFSET_REDUND) */
>>>> static int env_sf_save(void)
>>>> {
>>>> u32 saved_size, saved_offset, sector;
>>>> @@ -243,6 +315,57 @@ static int env_sf_save(void)
>>>> return ret;
>>>> }
>>>> +#ifdef CONFIG_CMD_ERASEENV
>>>> +static int env_sf_erase(void)
>>>> +{
>>>> + u32 saved_size, saved_offset, sector;
>>>> + char *saved_buffer = NULL;
>>>> + int ret = 1;
>>>> +
>>>> + 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;
>>>> + }
>
> Same thing here; can this be a separate function?
Sure, when I am at it anyway for the other path :-)
Actually, the two paths (w/ and w/o redundancy) look to me as if
they probably could be cleaned up even more so that no separate
implementations for these functions would be necessary, but I am
not sure how welcome that would be ...
I don't want to give the impression being a know-it-all, you know,
just want to help a little bit improving things ;-)
>>>> +
>>>> + 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;
>>>> +
>>>> + if (saved_buffer) {
>>>> + puts("Writing non-environment data to SPI flash...");
>>>> + ret = spi_flash_write(env_flash, saved_offset,
>>>> + saved_size, saved_buffer);
>>>> + if (ret)
>>>> + goto done;
>>>> + }
>>>> +
>>>> + puts("done\n");
>>>> +
>>>> + done:
>>>> + if (saved_buffer)
>>>> + free(saved_buffer);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +#endif /* CONFIG_CMD_ERASEENV */
>>>> +
>>>> static int env_sf_load(void)
>>>> {
>>>> int ret;
>>>> @@ -277,7 +400,7 @@ out:
>>>> return ret;
>>>> }
>>>> -#endif
>>>> +#endif /* #if defined(CONFIG_ENV_OFFSET_REDUND) #else */
>>>> #if CONFIG_ENV_ADDR != 0x0
>>>> __weak void *env_sf_get_env_addr(void)
>>>> @@ -311,4 +434,7 @@ U_BOOT_ENV_LOCATION(sf) = {
>>>> #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>>>> .init = env_sf_init,
>>>> #endif
>>>> +#if defined(CONFIG_CMD_ERASEENV) && defined(CONFIG_ENV_ADDR)
>
> Why does this depend on ENV_ADDR, when above we only depend on
> CMD_ERASEENV?
Good catch, will have another thorough look at that.
>>>> + .erase = env_sf_erase,
>>>> +#endif
>>>> };
>>>
>>
>>
>
> --Sean
>
--
Harry Waschkeit - Software Engineer
duagon Germany GmbH
Neuwieder Straße 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott - Handelsregister AG Nuernberg HRB 5540
More information about the U-Boot
mailing list