Re: [PATCH v2 3/9] env: correctly handle result in env_init
On Wed, Jun 24, 2020 at 11:19:50AM +, Patrick DELAUNAY wrote:
> Hi Tom,
>
> > From: Tom Rini
> > Sent: mardi 23 juin 2020 17:17
> >
> > On Tue, Jun 23, 2020 at 01:13:55PM +, Patrick DELAUNAY wrote:
> > > Hi Tom,
> > >
> > > > From: Tom Rini
> > > > Sent: vendredi 19 juin 2020 20:05
> > > >
> > > > On Fri, Jun 19, 2020 at 02:14:00PM +, Patrick DELAUNAY wrote:
> > > > > Hi Tom and Marek,
> > > > >
> > > > > > From: Tom Rini
> > > > > > 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
> > > > > > > ---
> > > > > > >
> > > > > > > (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
RE: [PATCH v2 3/9] env: correctly handle result in env_init
Hi Tom,
> From: Tom Rini
> Sent: mardi 23 juin 2020 17:17
>
> On Tue, Jun 23, 2020 at 01:13:55PM +, Patrick DELAUNAY wrote:
> > Hi Tom,
> >
> > > From: Tom Rini
> > > Sent: vendredi 19 juin 2020 20:05
> > >
> > > On Fri, Jun 19, 2020 at 02:14:00PM +, Patrick DELAUNAY wrote:
> > > > Hi Tom and Marek,
> > > >
> > > > > From: Tom Rini
> > > > > 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
> > > > > > ---
> > > > > >
> > > > > > (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
Re: [PATCH v2 3/9] env: correctly handle result in env_init
On Tue, Jun 23, 2020 at 01:13:55PM +, Patrick DELAUNAY wrote:
> Hi Tom,
>
> > From: Tom Rini
> > Sent: vendredi 19 juin 2020 20:05
> >
> > On Fri, Jun 19, 2020 at 02:14:00PM +, Patrick DELAUNAY wrote:
> > > Hi Tom and Marek,
> > >
> > > > From: Tom Rini
> > > > 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
> > > > > ---
> > > > >
> > > > > (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),
RE: [PATCH v2 3/9] env: correctly handle result in env_init
Hi Tom,
> From: Tom Rini
> Sent: vendredi 19 juin 2020 20:05
>
> On Fri, Jun 19, 2020 at 02:14:00PM +, Patrick DELAUNAY wrote:
> > Hi Tom and Marek,
> >
> > > From: Tom Rini
> > > 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
> > > > ---
> > > >
> > > > (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
> >
Re: [PATCH v2 3/9] env: correctly handle result in env_init
On Fri, Jun 19, 2020 at 02:14:00PM +, Patrick DELAUNAY wrote:
> Hi Tom and Marek,
>
> > From: Tom Rini
> > 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
> > > ---
> > >
> > > (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
RE: [PATCH v2 3/9] env: correctly handle result in env_init
Hi Tom and Marek,
> From: Tom Rini
> 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
> > ---
> >
> > (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
Re: [PATCH v2 3/9] env: correctly handle result in env_init
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
> ---
>
> (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!
--
Tom
signature.asc
Description: PGP signature
