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

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

On Thu, Jan 24, 2019 at 4:11 PM Frank Wunderlich
<frank-w@public-files.de> wrote:
>
> Hi,
>
> i ran also in same problem, after resize environment (4k to 8k), because my 4k is no more enough for my environment.
>
> i did not understand my CRC have to be checked if i want to save (and the checked part will be overridden). It makes sense to check the CRC of current environment-block which will be written but not the existing existing on mmc.
>
> imho save should verify generated block before save (memory) and after (on storage). So this case should not happen, that saving is impossible because of CRC on mmc is invalid before save...
>
> currently i look on http://www.denx.de/wiki/view/DULG/UBootCmdGroupMMC to find the right params to clear my environment at 0x100000 byte-offset, i guess there are blocks of 512byte so mmc erase should be at block 0x800 with length 0x10 (8k), am i right?
>
> regards Frank
>

Hi Frank,

As I understand, your case is different from one I'm fixing in my
patch. My patch is fixing the case when CRC is wrong during
environment loading, not saving. So in case when environment was
zeroed on your eMMC, you won't be able to do "env save" anymore, which
in my case as bad as a bricked board. So I still think we need to
merge this as is.

Thanks.

>
> > Gesendet: Freitag, 18. Januar 2019 um 20:19 Uhr
> > Von: "Sam Protsenko" <semen.protsenko@linaro.org>
> > An: u-boot at lists.denx.de
> > Cc: "Tom Rini" <trini@konsulko.com>, "Nicholas Faustini" <nicholas.faustini@azcomtech.com>, "Maxime Ripard" <maxime.ripard@free-electrons.com>
> > Betreff: [U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location
> >
> > 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(-)
> >
> > 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
> >