On Fri, Jun 19, 2020 at 02:14:00PM +0000, Patrick DELAUNAY wrote: > Hi Tom and Marek, > > > From: Tom Rini <[email protected]> > > 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 <[email protected]> > > > --- > > > > > > (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!
--
Tom
signature.asc
Description: PGP signature
