[U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location
Simon Goldschmidt
simon.k.r.goldschmidt at gmail.com
Mon Jan 21 13:36:09 UTC 2019
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 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.
>
> 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 at 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 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