[U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location
Sam Protsenko
semen.protsenko at linaro.org
Sat Jan 19 13:28:23 UTC 2019
Hi Simon,
On Fri, Jan 18, 2019 at 10:46 PM Simon Goldschmidt
<simon.k.r.goldschmidt at gmail.com> wrote:
>
>
>
> Am Fr., 18. Jan. 2019, 20:20 hat Sam Protsenko <semen.protsenko at 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 at 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
More information about the U-Boot
mailing list