| Message ID | 20200616074048.7898-4-patrick.delaunay@st.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: ext4: corrections and add test for env in ext4 | expand |
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. So isn't this a problem in most environment cases then? Thanks!
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". 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 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, > 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 Regards Patrick
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!
Hi Tom, > From: Tom Rini <trini@konsulko.com> > Sent: vendredi 19 juin 2020 20:05 > > 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 > > > drv->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! I was confused also... and I check again the code And I make a error in my first analysis. For the case where init ops is not defined in only one backend. ret = -ENOENT And the last test is true if (ret == -ENOENT) { gd->env_addr = (ulong)&default_environment[0]; gd->env_valid = ENV_VALID; return 0; } In fact this function return the LAST result for 'drv->init()' call and that it is strange when several backend are activated. So this function have no issue when only one ENV backend is activated, and it is the configuration today for most of boards... I will change my patch in the V3 serie: env_init() return 0 if, at least, one backend is correctly initialized (when no ops init or the ops init result is OK) But I don't understood 2 thinks in the last test 1/ Why set ENV Is set to VALID: gd->env_valid = ENV_VALID; in nowhere backend, for same case of default env, it is set ENV_INVALID.... 2/ Why flags is not update gd->flags |= GD_FLG_ENV_DEFAULT; But as it s error case, in should never occurs And I will sent a separate patch for this part to review. Patrick
On Tue, Jun 23, 2020 at 01:13:55PM +0000, Patrick DELAUNAY wrote: > Hi Tom, > > > From: Tom Rini <trini@konsulko.com> > > Sent: vendredi 19 juin 2020 20:05 > > > > 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 > > > > drv->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! > > I was confused also... > and I check again the code Thanks. It's a very tricky section of the code and I think I got some part of it wrong too. What got me off-track was unrolling the test isn't required in what you tried, just adding: ret = 0; // We found at least one env exits. before if (!drv->init || !(ret = drv->init())) would do the same. That said... > And I make a error in my first analysis. > > For the case where init ops is not defined in only one backend. > > ret = -ENOENT > > And the last test is true > > if (ret == -ENOENT) { > gd->env_addr = (ulong)&default_environment[0]; > gd->env_valid = ENV_VALID; > > return 0; > } > > In fact this function return the LAST result for 'drv->init()' call and > that it is strange when several backend are activated. Yes. Things are very fragile in the multi-env case. There being underlying bugs would not surprise me. > So this function have no issue when only one ENV backend is activated, > and it is the configuration today for most of boards... Right, the common case is the way things have worked historically, env exists in one defined location and when that fails (device not present), we get the built-in environment and saveenv needs to fail rather than crash the board (due to writing to non-existent HW, etc). > I will change my patch in the V3 serie: > env_init() return 0 if, at least, one backend is correctly initialized > (when no ops init or the ops init result is OK) > > But I don't understood 2 thinks in the last test > > 1/ Why set ENV Is set to VALID: > > gd->env_valid = ENV_VALID; > > in nowhere backend, for same case of default env, it is set ENV_INVALID.... A good question. I see: commit eeba55cb4a8a29a47d0d26692c188b47ba6bf396 Author: Tom Rini <trini@konsulko.com> Date: Sat Aug 19 22:27:57 2017 -0400 env: Correct case of no sub-init function changed that from setting it to 0 to setting it to ENV_VALID, where 0 meant ENV_INVALID at the time, but the comments in that commit say it used to be setting it to valid. Which brings us back to: commit 203e94f6c9ca03e260175ce240f5856507395585 Author: Simon Glass <sjg@chromium.org> Date: Thu Aug 3 12:21:56 2017 -0600 env: Add an enum for environment state as adding a enum for 0 = ENV_INVALID, 1 = ENV_VALID, 2 = redundant. And there's basically part of a longer series to clean up the environment code. In particular: commit 7938822a6b75b69fff9793b6b1769dddf1249525 Author: Simon Glass <sjg@chromium.org> Date: Thu Aug 3 12:22:02 2017 -0600 env: Drop common init() function is the root of this particular question. Before this change, the logic was "For the init function in of an env location, point at the in-memory default environment, declare it valid". With this change the new common env init function (now env_init() then env_init_new() as functionality migrated to it) when we did not have an explicit drv->init() call we need to point at the in-memory default environment and (was wrong at the time, later fixed to...) mark env as valid. So today, ret = -ENOENT is for the case of no explicit drv->init() function so do what those env drivers were doing before of just pointing gd->env_addr to the default environment and mark it valid. What we're doing today works but feels too clever, given the amount of time it takes to dig in to see what is supposed to be going on. In the case of one environment location, this logic works as expected. In the case of multiple environments, I would need to write down all of the possible combinations and walk the code to be sure what happens, especially since it's link-order dependent on the order we try drivers in. > 2/ Why flags is not update > > gd->flags |= GD_FLG_ENV_DEFAULT; > > > But as it s error case, in should never occurs > And I will sent a separate patch for this part to review. With all of the above in mind, it's not an error case, but being clever with variable names and our errno meanings. So we don't set this flag here because the "default" drv->init() that we're making this snippet of code act like never did that. We only set this flag later on when we're at the point where we know that we cannot get at a stored environment. the "default_environment" variable plays the role of both being the literal default environment as well as being the allocated space we use and move around as gd->env_addr in many cases. The env_init() code block for -ENOENT is to set things up for that case. Please let me know if this makes sense, or if you think I misread something (which is quite possible). Thanks!
Hi Tom, > From: Tom Rini <trini@konsulko.com> > Sent: mardi 23 juin 2020 17:17 > > On Tue, Jun 23, 2020 at 01:13:55PM +0000, Patrick DELAUNAY wrote: > > Hi Tom, > > > > > From: Tom Rini <trini@konsulko.com> > > > Sent: vendredi 19 juin 2020 20:05 > > > > > > 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 > > > > > drv->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! > > > > I was confused also... > > and I check again the code > > Thanks. It's a very tricky section of the code and I think I got some part of it > wrong too. What got me off-track was unrolling the test isn't required in what you > tried, just adding: > ret = 0; // We found at least one env exits. > before if (!drv->init || !(ret = drv->init())) would do the same. That said... > > > And I make a error in my first analysis. > > > > For the case where init ops is not defined in only one backend. > > > > ret = -ENOENT > > > > And the last test is true > > > > if (ret == -ENOENT) { > > gd->env_addr = (ulong)&default_environment[0]; > > gd->env_valid = ENV_VALID; > > > > return 0; > > } > > > > In fact this function return the LAST result for 'drv->init()' call > > and that it is strange when several backend are activated. > > Yes. Things are very fragile in the multi-env case. There being underlying bugs > would not surprise me. I dig deeper in the code and I agree: I will just sent a v3 with minimal update. The multi-env cases it could be long story. > > So this function have no issue when only one ENV backend is activated, > > and it is the configuration today for most of boards... > > Right, the common case is the way things have worked historically, env exists in > one defined location and when that fails (device not present), we get the built-in > environment and saveenv needs to fail rather than crash the board (due to writing > to non-existent HW, etc). Yes tested today on sandbox env save failed to avoid crash but without any trace. => I push a patch to add trace is that make sense. [PATCH] env: add failing trace in env_save http://patchwork.ozlabs.org/project/uboot/list/?series=185459 > > I will change my patch in the V3 serie: > > env_init() return 0 if, at least, one backend is correctly initialized > > (when no ops init or the ops init result is OK) > > > > But I don't understood 2 thinks in the last test > > > > 1/ Why set ENV Is set to VALID: > > > > gd->env_valid = ENV_VALID; > > > > in nowhere backend, for same case of default env, it is set > ENV_INVALID.... > > A good question. I see: > commit eeba55cb4a8a29a47d0d26692c188b47ba6bf396 > Author: Tom Rini <trini@konsulko.com> > Date: Sat Aug 19 22:27:57 2017 -0400 > > env: Correct case of no sub-init function > > changed that from setting it to 0 to setting it to ENV_VALID, where 0 meant > ENV_INVALID at the time, but the comments in that commit say it used to be > setting it to valid. Which brings us back to: > commit 203e94f6c9ca03e260175ce240f5856507395585 > Author: Simon Glass <sjg@chromium.org> > Date: Thu Aug 3 12:21:56 2017 -0600 > > env: Add an enum for environment state > > as adding a enum for 0 = ENV_INVALID, 1 = ENV_VALID, 2 = redundant. > > And there's basically part of a longer series to clean up the environment code. In > particular: > commit 7938822a6b75b69fff9793b6b1769dddf1249525 > Author: Simon Glass <sjg@chromium.org> > Date: Thu Aug 3 12:22:02 2017 -0600 > > env: Drop common init() function > > is the root of this particular question. Before this change, the logic was "For the > init function in of an env location, point at the in-memory default environment, > declare it valid". With this change the new common env init function (now > env_init() then env_init_new() as functionality migrated to it) when we did not have > an explicit drv->init() call we need to point at the in-memory default environment > and (was wrong at the time, later fixed to...) mark env as valid. > > So today, ret = -ENOENT is for the case of no explicit drv->init() function so do > what those env drivers were doing before of just pointing > gd->env_addr to the default environment and mark it valid. > > What we're doing today works but feels too clever, given the amount of time it > takes to dig in to see what is supposed to be going on. > > In the case of one environment location, this logic works as expected. > In the case of multiple environments, I would need to write down all of the possible > combinations and walk the code to be sure what happens, especially since it's > link-order dependent on the order we try drivers in. I check the code deeper and I see many incoherency in multi-env support For example sometime the global ENV configuration gd->env_valid = ENV_VALID; is initialiazed in init function of each backend... for me it should be initialzed only on the load ops when the ENV is really valid. and for default ENV configuration we have gd->env_addr = (ulong)&default_environment[0]; gd->env_valid = ENV_INVALID; in this case set gd->env_addr with ENV_INVALID is not necessary or gd->env_addr = (ulong)&default_environment[0]; gd->env_valid = ENV_VALID; it seems incoherent or uneeded with int env_get_char(int index) { if (gd->env_valid == ENV_INVALID) return default_environment[index]; else return env_get_char_spec(index); } I will continue to check the code deeper but it seems out of the scope for this serie. I will sent a v3 soon. > > 2/ Why flags is not update > > > > gd->flags |= GD_FLG_ENV_DEFAULT; > > > > > > But as it s error case, in should never occurs And I will sent a > > separate patch for this part to review. > > With all of the above in mind, it's not an error case, but being clever with variable > names and our errno meanings. So we don't set this flag here because the > "default" drv->init() that we're making this snippet of code act like never did that. > We only set this flag later on when we're at the point where we know that we > cannot get at a stored environment. > the "default_environment" variable plays the role of both being the literal default > environment as well as being the allocated space we use and move around as gd- > >env_addr in many cases. The env_init() code block for -ENOENT is to set > things up for that case. > > Please let me know if this makes sense, or if you think I misread something > (which is quite possible). Thanks! Yes that make sense: it is fallback to default ENV when all .init failed, I will update my patch in V3 with minimal update of this part and with a new comment. The env code use many gloabal tag, sometime managed by common code, sometime managed by backend... it is difficult to know the responsibility of each part in multi-env context. I will continue check of ENV code in background (and perhaps activate and test a other ENV backend in sandbox) Thanks for your time. Patrick
On Wed, Jun 24, 2020 at 11:19:50AM +0000, Patrick DELAUNAY wrote: > Hi Tom, > > > From: Tom Rini <trini@konsulko.com> > > Sent: mardi 23 juin 2020 17:17 > > > > On Tue, Jun 23, 2020 at 01:13:55PM +0000, Patrick DELAUNAY wrote: > > > Hi Tom, > > > > > > > From: Tom Rini <trini@konsulko.com> > > > > Sent: vendredi 19 juin 2020 20:05 > > > > > > > > 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 > > > > > > drv->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! > > > > > > I was confused also... > > > and I check again the code > > > > Thanks. It's a very tricky section of the code and I think I got some part of it > > wrong too. What got me off-track was unrolling the test isn't required in what you > > tried, just adding: > > ret = 0; // We found at least one env exits. > > before if (!drv->init || !(ret = drv->init())) would do the same. That said... > > > > > And I make a error in my first analysis. > > > > > > For the case where init ops is not defined in only one backend. > > > > > > ret = -ENOENT > > > > > > And the last test is true > > > > > > if (ret == -ENOENT) { > > > gd->env_addr = (ulong)&default_environment[0]; > > > gd->env_valid = ENV_VALID; > > > > > > return 0; > > > } > > > > > > In fact this function return the LAST result for 'drv->init()' call > > > and that it is strange when several backend are activated. > > > > Yes. Things are very fragile in the multi-env case. There being underlying bugs > > would not surprise me. > > I dig deeper in the code and I agree: I will just sent a v3 with minimal update. > > The multi-env cases it could be long story. > > > > So this function have no issue when only one ENV backend is activated, > > > and it is the configuration today for most of boards... > > > > Right, the common case is the way things have worked historically, env exists in > > one defined location and when that fails (device not present), we get the built-in > > environment and saveenv needs to fail rather than crash the board (due to writing > > to non-existent HW, etc). > > Yes tested today on sandbox > env save failed to avoid crash but without any trace. > > => I push a patch to add trace is that make sense. > [PATCH] env: add failing trace in env_save > http://patchwork.ozlabs.org/project/uboot/list/?series=185459 > > > > > I will change my patch in the V3 serie: > > > env_init() return 0 if, at least, one backend is correctly initialized > > > (when no ops init or the ops init result is OK) > > > > > > But I don't understood 2 thinks in the last test > > > > > > 1/ Why set ENV Is set to VALID: > > > > > > gd->env_valid = ENV_VALID; > > > > > > in nowhere backend, for same case of default env, it is set > > ENV_INVALID.... > > > > A good question. I see: > > commit eeba55cb4a8a29a47d0d26692c188b47ba6bf396 > > Author: Tom Rini <trini@konsulko.com> > > Date: Sat Aug 19 22:27:57 2017 -0400 > > > > env: Correct case of no sub-init function > > > > changed that from setting it to 0 to setting it to ENV_VALID, where 0 meant > > ENV_INVALID at the time, but the comments in that commit say it used to be > > setting it to valid. Which brings us back to: > > commit 203e94f6c9ca03e260175ce240f5856507395585 > > Author: Simon Glass <sjg@chromium.org> > > Date: Thu Aug 3 12:21:56 2017 -0600 > > > > env: Add an enum for environment state > > > > as adding a enum for 0 = ENV_INVALID, 1 = ENV_VALID, 2 = redundant. > > > > And there's basically part of a longer series to clean up the environment code. In > > particular: > > commit 7938822a6b75b69fff9793b6b1769dddf1249525 > > Author: Simon Glass <sjg@chromium.org> > > Date: Thu Aug 3 12:22:02 2017 -0600 > > > > env: Drop common init() function > > > > is the root of this particular question. Before this change, the logic was "For the > > init function in of an env location, point at the in-memory default environment, > > declare it valid". With this change the new common env init function (now > > env_init() then env_init_new() as functionality migrated to it) when we did not have > > an explicit drv->init() call we need to point at the in-memory default environment > > and (was wrong at the time, later fixed to...) mark env as valid. > > > > So today, ret = -ENOENT is for the case of no explicit drv->init() function so do > > what those env drivers were doing before of just pointing > > gd->env_addr to the default environment and mark it valid. > > > > What we're doing today works but feels too clever, given the amount of time it > > takes to dig in to see what is supposed to be going on. > > > > In the case of one environment location, this logic works as expected. > > In the case of multiple environments, I would need to write down all of the possible > > combinations and walk the code to be sure what happens, especially since it's > > link-order dependent on the order we try drivers in. > > I check the code deeper and > I see many incoherency in multi-env support > > For example sometime the global ENV configuration > gd->env_valid = ENV_VALID; > > is initialiazed in init function of each backend... > for me it should be initialzed only on the load ops when the ENV is really valid. > > and for default ENV configuration > > we have > gd->env_addr = (ulong)&default_environment[0]; > gd->env_valid = ENV_INVALID; > > in this case set gd->env_addr with ENV_INVALID is not necessary > > or > gd->env_addr = (ulong)&default_environment[0]; > gd->env_valid = ENV_VALID; > > it seems incoherent or uneeded with > > int env_get_char(int index) > { > if (gd->env_valid == ENV_INVALID) > return default_environment[index]; > else > return env_get_char_spec(index); > } > > I will continue to check the code deeper but it seems > out of the scope for this serie. > > I will sent a v3 soon. > > > > 2/ Why flags is not update > > > > > > gd->flags |= GD_FLG_ENV_DEFAULT; > > > > > > > > > But as it s error case, in should never occurs And I will sent a > > > separate patch for this part to review. > > > > With all of the above in mind, it's not an error case, but being clever with variable > > names and our errno meanings. So we don't set this flag here because the > > "default" drv->init() that we're making this snippet of code act like never did that. > > We only set this flag later on when we're at the point where we know that we > > cannot get at a stored environment. > > the "default_environment" variable plays the role of both being the literal default > > environment as well as being the allocated space we use and move around as gd- > > >env_addr in many cases. The env_init() code block for -ENOENT is to set > > things up for that case. > > > > Please let me know if this makes sense, or if you think I misread something > > (which is quite possible). Thanks! > > Yes that make sense: it is fallback to default ENV when all .init failed, I will update > my patch in V3 with minimal update of this part and with a new comment. > > The env code use many gloabal tag, sometime managed by common code, > sometime managed by backend... it is difficult to know the responsibility of each > part in multi-env context. > > I will continue check of ENV code in background (and perhaps activate and test a other > ENV backend in sandbox) > > Thanks for your time. Thanks for looking here as well. Any patches you can add to clarify things or make them more consistent are greatly appreciated.
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__,
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(-)