From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 28 Jul 2020 08:39:33 -0400 Subject: [PATCH V2 4/7] env: Fix invalid env handling in env_init() In-Reply-To: <79d0e627-0676-b45e-0649-59ac4c421cb9@denx.de> References: <20200707185139.2225-1-marex@denx.de> <20200707185139.2225-4-marex@denx.de> <20200724145655.GN6227@bill-the-cat> <79d0e627-0676-b45e-0649-59ac4c421cb9@denx.de> Message-ID: <20200728123933.GL6965@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Jul 28, 2020 at 09:28:17AM +0200, Marek Vasut wrote: > On 7/24/20 4:56 PM, Tom Rini wrote: > > On Tue, Jul 07, 2020 at 08:51:36PM +0200, Marek Vasut wrote: > > > >> This fixes the case where there are multiple environment drivers, one of > >> them is the default environment one, and it is followed by an environment > >> driver which does not implement .init() callback. The default environment > >> driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init() > >> callback implementation, which is valid behavior for default environment. > >> > >> Since the subsequent environment driver does not implement .init(), it > >> also does not modify the $ret variable in the loop. Therefore, the loop > >> is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the > >> code further down in env_init() will not reset the environment to the > >> default one, which is incorrect. > >> > >> This patch sets the $ret variable back to -ENOENT in case the env_valid > >> is set to ENV_INVALID by an environment driver, so that the environment > >> would be correctly reset back to default one, unless a subsequent driver > >> loads a valid environment. > >> > >> Signed-off-by: Marek Vasut > >> --- > >> V2: Reword commit message > >> --- > >> env/env.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/env/env.c b/env/env.c > >> index dcc25c030b..024d36fdbe 100644 > >> --- a/env/env.c > >> +++ b/env/env.c > >> @@ -300,6 +300,9 @@ int env_init(void) > >> > >> debug("%s: Environment %s init done (ret=%d)\n", __func__, > >> drv->name, ret); > >> + > >> + if (gd->env_valid == ENV_INVALID) > >> + ret = -ENOENT; > >> } > >> > >> if (!prio) > > > > So, I am not sure about this change. Given the whole thread that ends > > in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this > > particular part of the function is being too clever. I think it's > > intentionally not doing what you're adding right here for some use > > cases. > > Which use-cases ? flash specifically sets env_valid to ENV_INVALID and returns 0, and uses the default environment location, when env is invalid. NAND, when embedded, sets addr to 0 when invalid, ENV_INVALID and returns 0. NOWHERE is the real funny case, it says ENV_INVALID and default_environment and returns 0. Which all brings me back to my "too clever" statement. Only REMOTE ever returns non-zero in it's init function. > > I think that to cleanly achieve the goals of your series we need > > to stop letting drv->init be optional so that we then stop doing the > > particular we're doing with "ENOENT means runs this common code path for > > many envs". We may also need to make sure the link order in > > env/Makefile has nowhere first in all cases, rather than just most cases > > like it does now, with a big comment on why. > > So, would it make sense to apply the rest and revisit this patch > separately (likely with ST on CC) so the writeable list won't miss this > release ? If you're OK with that, yes. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: