From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sam Protsenko Date: Sat, 19 Jan 2019 15:28:23 +0200 Subject: [U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location In-Reply-To: References: <20190118191904.634-1-semen.protsenko@linaro.org> <20190118191904.634-3-semen.protsenko@linaro.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On Fri, Jan 18, 2019 at 10:46 PM Simon Goldschmidt wrote: > > > > Am Fr., 18. Jan. 2019, 20:20 hat Sam Protsenko 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 >> --- >> 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