[2/2] env: Fix saving environment to "bad CRC" location

Message ID 20190118191904.634-3-semen.protsenko@linaro.org
State Accepted
Commit 293a172b67230e14c43f2b7a8b6f302b1d03ca4e
Headers show
Series
  • env: Fix "env save" to malformed environment
Related show

Commit Message

Sam Protsenko Jan. 18, 2019, 7:19 p.m.
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.

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(-)

Comments

Simon Goldschmidt Jan. 18, 2019, 8:46 p.m. | #1
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.

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'.

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

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@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>
Sam Protsenko Jan. 19, 2019, 1:28 p.m. | #2
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@lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
Simon Goldschmidt Jan. 21, 2019, 9:18 a.m. | #3
Am 19.01.2019 um 14:28 schrieb Sam Protsenko:
> 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.

Well, I don't know if that's another matter. This multi-env code is 
pretty new, and it seems to me it hasn't fully worked, yet, for corner 
cases. Being like that, I'm not totally sure your change is OK for 
everyone using more than one env source. What if the first device 
returning an error code was an unexpected error? You'll then save your 
environment to some other storage without noticing... Is that expected 
behaviour for multi-env support? Or shouldn't we rather return an error 
and let the user supply the target device instead?

I just fear your change might break things for others.

I'm not saying your patch is totally wrong, just thinking. I did use 
multi-env support, but never checked error paths back then...

Regards,
Simon

> 
>> 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@lists.denx.de
>>> https://lists.denx.de/listinfo/u-boot
Simon Goldschmidt Jan. 21, 2019, 1:36 p.m. | #4
Am 21.01.2019 um 10:18 schrieb Simon Goldschmidt:
> Am 19.01.2019 um 14:28 schrieb Sam Protsenko:
>> 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.
> 
> Well, I don't know if that's another matter. This multi-env code is
> pretty new, and it seems to me it hasn't fully worked, yet, for corner
> cases. Being like that, I'm not totally sure your change is OK for
> everyone using more than one env source. What if the first device
> returning an error code was an unexpected error? You'll then save your
> environment to some other storage without noticing... Is that expected
> behaviour for multi-env support? Or shouldn't we rather return an error
> and let the user supply the target device instead?
> 
> I just fear your change might break things for others.
> 
> I'm not saying your patch is totally wrong, just thinking. I did use
> multi-env support, but never checked error paths back then...

Thinking about this again, we're probably better off with this patch 
than without, so:

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

> 
> Regards,
> Simon
> 
>>
>>> 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@lists.denx.de
>>>> https://lists.denx.de/listinfo/u-boot
>

Patch

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;
 }