[U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location

[Sam Protsenko <semen.protsenko@linaro.org>]

Hi Simon,

On Fri, Jan 18, 2019 at 10:46 PM Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
>
>
> Am Fr., 18. Jan. 2019, 20:20 hat Sam Protsenko <semen.protsenko@linaro.org> geschrieben:
>>
>> In case when the environment on some location is malformed (CRC isn't
>> matching), there is a chance we won't be able to save the environment to
>> that location. For example, consider the case when we only have the
>> environment on eMMC, but it's zeroed out. In that case, we won't be able
>> to "env save" to it, because of "bad CRC" error. That's happening
>> because in env_load() function we consider malformed environment as
>> incorrect one, and  defaulting to the location with highest (0)
>> priority, which can be different from one we are dealing with right now
>> (e.g., highest priority can be ENV_FAT on SD card, which is not
>> inserted, but we want to use ENV_MMC on eMMC, where we were booted
>> from).
>>
>> This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove
>> driver lookup loop from env_save()") on BeagleBone Black, but that
>> commit didn't introduce the wrong logic, it just changed the behavior
>> for default location to use, merely revealing this issue.
>
>
> In my opinion, this automatism of selecting a driver to save the env can always produce a behaviour that is unexpected to the user.
>

Completely agree with you. That's my thoughts exactly, when I ran into
this issue.

> I wonder if it wouldn't be better to either let the board define some default env driver for saving or the user should specify the driver as argument to 'env save'.
>

Yeah, if I have some spare time, gonna implement something like this,
at least as RFC patch. We can start full-scale discussion in that
thread, as there could be more possible ways of fixing that, and we
should consider different platforms when deciding on that. But I think
we should take this patch anyway, as "env save" without arguments
should be left too, for compatibility reasons.

> Note that this is not an objection to your oatches, I'm just not content with the current auto-selection...
>

Yeah, but that's another matter, I hope I will get to that soon.
Meanwhile, can you please add your "Reviewed-by" tag, if the code
looks ok to you? With this patch we won't have semi-bricked boards, at
least.

> Regards,
> Simon
>
>>
>> To fix that, let's implement next logic in env_load():
>>   1. Try to find out correct environment; if found -- use it
>>   2. If working environment wasn't found, but we found malformed one
>>      (with bad CRC), let's use it for further "env save". But make sure
>>      to use malformed environment location with highest priority.
>>   3. If neither correct nor malformed environment was found, let's
>>      default to environment location with highest priority (0)
>>
>> Steps to reproduce mentioned issue on BeagleBone Black (fixed in this
>> patch):
>>
>> 1. Boot from SD card and erase eMMC in U-Boot shell:
>>    => mmc dev 1
>>    => mmc erase 0 100000
>>    => gpt write mmc 1 $partitions
>> 2. Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled
>>    with zeroes
>> 3. Boot from eMMC; try to do:
>>    => env save
>> 4. Observe the error (incorrect behavior). Correct behavior: environment
>>    should be stored correctly on eMMC, in spite of it has "bad CRC"
>>
>> Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()")
>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>> ---
>>  env/env.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/env/env.c b/env/env.c
>> index 003509d342..4b417b90a2 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -177,6 +177,7 @@ int env_get_char(int index)
>>  int env_load(void)
>>  {
>>         struct env_driver *drv;
>> +       int best_prio = -1;
>>         int prio;
>>
>>         for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {
>> @@ -195,20 +196,32 @@ int env_load(void)
>>                  * one message.
>>                  */
>>                 ret = drv->load();
>> -               if (ret) {
>> -                       debug("Failed (%d)\n", ret);
>> -               } else {
>> +               if (!ret) {
>>                         printf("OK\n");
>>                         return 0;
>> +               } else if (ret == -ENOMSG) {
>> +                       /* Handle "bad CRC" case */
>> +                       if (best_prio == -1)
>> +                               best_prio = prio;
>> +               } else {
>> +                       debug("Failed (%d)\n", ret);
>>                 }
>>         }
>>
>>         /*
>>          * In case of invalid environment, we set the 'default' env location
>> -        * to the highest priority. In this way, next calls to env_save()
>> -        * will restore the environment at the right place.
>> +        * to the best choice, i.e.:
>> +        *   1. Environment location with bad CRC, if such location was found
>> +        *   2. Otherwise use the location with highest priority
>> +        *
>> +        * This way, next calls to env_save() will restore the environment
>> +        * at the right place.
>>          */
>> -       env_get_location(ENVOP_LOAD, 0);
>> +       if (best_prio >= 0)
>> +               debug("Selecting environment with bad CRC\n");
>> +       else
>> +               best_prio = 0;
>> +       env_get_location(ENVOP_LOAD, best_prio);
>>
>>         return -ENODEV;
>>  }
>> --
>> 2.20.1
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot