| Message ID | 20201217231918.40164-1-brandon.maier@rockwellcollins.com |
|---|---|
| State | Accepted |
| Commit | b9c3052fbb25bff26702e6c16abfd0a5ec92040c |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: increment redund flag on read fail | expand |
On Thu, Dec 17, 2020 at 05:19:18PM -0600, Brandon Maier wrote: > If one of the reads fails when importing redundant environments (a > single read failure), the env_flags wouldn't get initialized in > env_import_redund(). If a user then calls saveenv, the new environment > will have the wrong flags value. So on the next load the new environment > will be ignored. > > While debugging this, I also noticed that env/sf.c was not correctly > handling a single read failure, as it would not check the crc before > assigning it to gd->env_addr. > > Having a special error path for when there is a single read failure > seems unnecessary and may lead to future bugs. Instead collapse the > 'single read failure' error to be the same as a 'single crc failure'. > That way env_check_redund() either passes or fails, and if it passes we > are guaranteed to have checked the CRC. > > Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> > CC: Joe Hershberger <joe.hershberger@ni.com> > CC: Wolfgang Denk <wd@denx.de> > CC: Heiko Schocher <hs@denx.de> Sorry for the delay. Looking this over, yes, I think this is correct so: Reviewed-by: Tom Rini <trini@konsulko.com> And I'm going to give Joe or Wolfgang or Heiko a little more time to chime in before I apply this in time for v2021.04-rc2.
On Thu, Dec 17, 2020 at 05:19:18PM -0600, Brandon Maier wrote: > If one of the reads fails when importing redundant environments (a > single read failure), the env_flags wouldn't get initialized in > env_import_redund(). If a user then calls saveenv, the new environment > will have the wrong flags value. So on the next load the new environment > will be ignored. > > While debugging this, I also noticed that env/sf.c was not correctly > handling a single read failure, as it would not check the crc before > assigning it to gd->env_addr. > > Having a special error path for when there is a single read failure > seems unnecessary and may lead to future bugs. Instead collapse the > 'single read failure' error to be the same as a 'single crc failure'. > That way env_check_redund() either passes or fails, and if it passes we > are guaranteed to have checked the CRC. > > Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> > CC: Joe Hershberger <joe.hershberger@ni.com> > CC: Wolfgang Denk <wd@denx.de> > CC: Heiko Schocher <hs@denx.de> > Reviewed-by: Tom Rini <trini@konsulko.com> Applied to u-boot/master, thanks!
diff --git a/env/common.c b/env/common.c index 6c32a9b479..ca22885fa4 100644 --- a/env/common.c +++ b/env/common.c @@ -144,7 +144,7 @@ static unsigned char env_flags; int env_check_redund(const char *buf1, int buf1_read_fail, const char *buf2, int buf2_read_fail) { - int crc1_ok, crc2_ok; + int crc1_ok = 0, crc2_ok = 0; env_t *tmp_env1, *tmp_env2; tmp_env1 = (env_t *)buf1; @@ -152,25 +152,18 @@ int env_check_redund(const char *buf1, int buf1_read_fail, if (buf1_read_fail && buf2_read_fail) { puts("*** Error - No Valid Environment Area found\n"); + return -EIO; } else if (buf1_read_fail || buf2_read_fail) { puts("*** Warning - some problems detected "); puts("reading environment; recovered successfully\n"); } - if (buf1_read_fail && buf2_read_fail) { - return -EIO; - } else if (!buf1_read_fail && buf2_read_fail) { - gd->env_valid = ENV_VALID; - return -EINVAL; - } else if (buf1_read_fail && !buf2_read_fail) { - gd->env_valid = ENV_REDUND; - return -ENOENT; - } - - crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == - tmp_env1->crc; - crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == - tmp_env2->crc; + if (!buf1_read_fail) + crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == + tmp_env1->crc; + if (!buf2_read_fail) + crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == + tmp_env2->crc; if (!crc1_ok && !crc2_ok) { return -ENOMSG; /* needed for env_load() */ @@ -207,10 +200,6 @@ int env_import_redund(const char *buf1, int buf1_read_fail, if (ret == -EIO) { env_set_default("bad env area", 0); return -EIO; - } else if (ret == -EINVAL) { - return env_import((char *)buf1, 1, flags); - } else if (ret == -ENOENT) { - return env_import((char *)buf2, 1, flags); } else if (ret == -ENOMSG) { env_set_default("bad CRC", 0); return -ENOMSG; diff --git a/env/sf.c b/env/sf.c index 91ed2860ed..1102b42804 100644 --- a/env/sf.c +++ b/env/sf.c @@ -350,7 +350,7 @@ static int env_sf_init_early(void) ret = env_check_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2, read2_fail); - if (ret == -EIO || ret == -ENOMSG) + if (ret < 0) goto err_read; if (gd->env_valid == ENV_VALID) diff --git a/include/env.h b/include/env.h index c15339a93f..b5731e4b9a 100644 --- a/include/env.h +++ b/include/env.h @@ -328,8 +328,6 @@ int env_export(struct environment_s *env_out); * @buf2_read_fail: 0 if buf2 is valid, non-zero if invalid * @return 0 if OK, * -EIO if no environment is valid, - * -EINVAL if read of second entry is good - * -ENOENT if read of first entry is good * -ENOMSG if the CRC was bad */
If one of the reads fails when importing redundant environments (a single read failure), the env_flags wouldn't get initialized in env_import_redund(). If a user then calls saveenv, the new environment will have the wrong flags value. So on the next load the new environment will be ignored. While debugging this, I also noticed that env/sf.c was not correctly handling a single read failure, as it would not check the crc before assigning it to gd->env_addr. Having a special error path for when there is a single read failure seems unnecessary and may lead to future bugs. Instead collapse the 'single read failure' error to be the same as a 'single crc failure'. That way env_check_redund() either passes or fails, and if it passes we are guaranteed to have checked the CRC. Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> CC: Joe Hershberger <joe.hershberger@ni.com> CC: Wolfgang Denk <wd@denx.de> CC: Heiko Schocher <hs@denx.de> --- env/common.c | 27 ++++++++------------------- env/sf.c | 2 +- include/env.h | 2 -- 3 files changed, 9 insertions(+), 22 deletions(-)