On Fri, Jun 19, 2020 at 02:14:00PM +0000, Patrick DELAUNAY wrote:
Hi Tom and Marek,
From: Tom Rini trini@konsulko.com Sent: jeudi 18 juin 2020 21:16
On Tue, Jun 16, 2020 at 09:40:42AM +0200, Patrick Delaunay wrote:
Don't return error with ret=-ENOENT when the optional ops drv->init is absent but only if env_driver_lookup doesn't found driver.
This patch correct an issue for the code if (!env_init()) env_load() When only ext4 is supported (CONFIG_ENV_IS_IN_EXT4), as the backend env/ext4.c doesn't define an ops .init
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com
(no changes since v1)
env/env.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/env/env.c b/env/env.c index dcc25c030b..819c88f729 100644 --- a/env/env.c +++ b/env/env.c @@ -295,7 +295,10 @@ int env_init(void) int prio;
for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
if (!drv->init || !(ret = drv->init()))
ret = 0;if (drv->init)ret = drv->init();if (!ret) env_set_inited(drv->location);debug("%s: Environment %s init done (ret=%d)\n", __func__,
I'm adding in Marek here because this reminds me of similar questions / concerns I had looking in to his series. At root, I think we're not being consistent in each of our env backing implementations about where flags such as ENV_VALID are set, and return values / checks of functions.
Just outside of the start of the patch context here, we set ret to -ENOENT and just past this, if still -ENOENT we say ENV_VALID and point at the default environment.
But, I don't follow the patch commit message here. If we don't have drv->init we call env_set_inited(drv->location) but we won't have change ret to 0, which means that later on down the function we go back to default environment.
The cause of the issue is because the init() ops is optional in "struct env_driver".
Right.
When this opt is absent, I assume that the initialization is not mandatory but this inititialization need to be tagged in gd->env_has_init with the call of env_set_inited() function
So when drv->init isn't set we are already calling env_set_inited(...). If that's not the case, what's going on?
And the ENV backend is FOUND (don't return -ENOENT)
else the next call of env_has_inited(drv->location) always failed : in env_load()
I see the error in EXT4 env backend,.all the other backend as a env_init() function
But some othe backend don't define the .init operation and have the issue
eeprom.c:235:U_BOOT_ENV_LOCATION(eeprom) = { ext4.c:135:U_BOOT_ENV_LOCATION(ext4) = { fat.c:128:U_BOOT_ENV_LOCATION(fat) = { mmc.c:393:U_BOOT_ENV_LOCATION(mmc) = { onenand.c:108:U_BOOT_ENV_LOCATION(onenand) = { sata.c:117:U_BOOT_ENV_LOCATION(sata) = { ubi.c:179:U_BOOT_ENV_LOCATION(ubi) = {
The other don't have issue:
flash.c:358:U_BOOT_ENV_LOCATION(flash) = { flash.c:368: .init = env_flash_init, nand.c:382:U_BOOT_ENV_LOCATION(nand) = { nand.c:389: .init = env_nand_init, nowhere.c:30:U_BOOT_ENV_LOCATION(nowhere) = { nowhere.c:32: .init = env_nowhere_init, nvram.c:117:U_BOOT_ENV_LOCATION(nvram) = { nvram.c:122: .init = env_nvram_init, remote.c:54:U_BOOT_ENV_LOCATION(remote) = { remote.c:59: .init = env_remote_init, sf.c:306:U_BOOT_ENV_LOCATION(sf) = { sf.c:312: .init = env_sf_init,
Right, there should be a problem showing up on a ton of locations, not just ext4 which is why I'm concerned / confused here. While ext4 isn't as widely used yet as I would expect, FAT/MMC are.
So isn't this a problem in most environment cases then? Thanks!
I don't sure which environment configuration can case issue (only one ENV without drc->init() function) But no issue if at least one CONFIG_ENV_IS_ is activated with driver implementing init ops
But I see the issue in SANDBOX when I activate EXT4 only target. (CONFIG_ENV_IS_IN_EXT4), And no more issue when I add CONFIG_ENV_IS_NOWHERE.
PS: no direct issue if env_init result is not checked but I check this result in the sandbox tests in next patches: if (!env_init()) env_load()
but anyway inconsistent value of gd->env_has_init which can be a problem for any env_has_inited() calls
Right. I think there's some bigger inconsistency going on here that needs to be fixed. I'm also confused / concerned how you're not seeing env_set_inite(..) being called. if (!NULL) is true. Thanks!