All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sam Protsenko <semen.protsenko@linaro.org>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location
Date: Fri, 18 Jan 2019 21:19:04 +0200	[thread overview]
Message-ID: <20190118191904.634-3-semen.protsenko@linaro.org> (raw)
In-Reply-To: <20190118191904.634-1-semen.protsenko@linaro.org>

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

  parent reply	other threads:[~2019-01-18 19:19 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-18 19:19 [U-Boot] [PATCH 0/2] env: Fix "env save" to malformed environment Sam Protsenko
2019-01-18 19:19 ` [U-Boot] [PATCH 1/2] env: common: Return specific error code on bad CRC Sam Protsenko
2019-01-21 13:36   ` Simon Goldschmidt
2019-01-27  3:53   ` [U-Boot] [U-Boot, " Tom Rini
2019-01-18 19:19 ` Sam Protsenko [this message]
2019-01-18 20:46   ` [U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location Simon Goldschmidt
2019-01-19 13:28     ` Sam Protsenko
2019-01-21  9:18       ` Simon Goldschmidt
2019-01-21 13:36         ` Simon Goldschmidt
2019-01-24 14:11   ` Frank Wunderlich
2019-01-25 15:03     ` Sam Protsenko
2019-01-27  3:53   ` [U-Boot] [U-Boot, " Tom Rini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190118191904.634-3-semen.protsenko@linaro.org \
    --to=semen.protsenko@linaro.org \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.