| Message ID | 20201101133812.18701-1-michael@walle.cc |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: env_sf: don't set .init op if not needed | expand |
Hello Michael, Am 01.11.2020 um 14:38 schrieb Michael Walle: > Commit 92765f45bb95 ("env: Access Environment in SPI flashes before > relocation") at least breaks the Kontron sl28 board. I guess it also > breaks others which use a (late) SPI environment. > > Unfortunately, we cannot set the .init op and fall back to the same > behavior as it would be unset. Thus guard the .init op by #if's. > > Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation") > Signed-off-by: Michael Walle <michael@walle.cc> > --- > env/sf.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/env/sf.c b/env/sf.c > index 2eb2de1a4e..18d44a7ddc 100644 > --- a/env/sf.c > +++ b/env/sf.c > @@ -385,7 +385,7 @@ out: > } > #endif > > -static int env_sf_init(void) > +static int __maybe_unused env_sf_init(void) > { > #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) > return env_sf_init_addr(); > @@ -393,8 +393,13 @@ static int env_sf_init(void) > return env_sf_init_early(); > #endif > /* > - * we must return with 0 if there is nothing done, > - * else env_set_inited() get not called in env_init() > + * We shouldn't end up here. Unfortunately, there is no > + * way to return a value which yields the same behavior > + * as if the .init op wouldn't be set at all. See > + * env_init(); env_set_inited() is only called if we > + * return 0, but the default environment is only loaded > + * if -ENOENT is returned. Therefore, we need the ugly > + * ifdeferry for the .init op. > */ > return 0; > } > @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = { > ENV_NAME("SPIFlash") > .load = env_sf_load, > .save = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL, > +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY) > .init = env_sf_init, > +#endif > }; > Ok, tested this patch on an imx6 based board with SPI NOR and it works. But.... there is a problem with environment in spi nor and ENV_APPEND enabled, with current implementation (also before my patches applied): I enabled now ENV_APPEND on this board and CONFIG_ENV_IS_NOWHERE CONFIG_ENV_IS_IN_SPI_FLASH and the Environment from SPI NOR never loaded as gd->env_valid is always ENV_INVALID and env_load() never called from env_relocate(). What do you think about following patch: diff --git a/env/sf.c b/env/sf.c index 2eb2de1a4e..7f3491b458 100644 --- a/env/sf.c +++ b/env/sf.c @@ -393,9 +393,13 @@ static int env_sf_init(void) return env_sf_init_early(); #endif /* - * we must return with 0 if there is nothing done, - * else env_set_inited() get not called in env_init() + * We must return with 0 if there is nothing done, + * to get inited bit set in env_init(). + * We need to set env_valid to ENV_VALID, so later + * env_load() loads the Environment from SPI NOR. */ + gd->env_addr = (ulong)&default_environment[0]; + gd->env_valid = ENV_VALID; return 0; } Can you try it? Another option would be to reutrn -ENOENT and set init bit also when a init function returns -ENOENT: diff --git a/env/env.c b/env/env.c index 42c7d8155e..37b4b54cb7 100644 --- a/env/env.c +++ b/env/env.c @@ -329,6 +329,8 @@ int env_init(void) for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { if (!drv->init || !(ret = drv->init())) env_set_inited(drv->location); + if (ret == -ENOENT) + env_set_inited(drv->location); debug("%s: Environment %s init done (ret=%d)\n", __func__, drv->name, ret); diff --git a/env/sf.c b/env/sf.c index 2eb2de1a4e..66279fb4f4 100644 --- a/env/sf.c +++ b/env/sf.c @@ -396,7 +396,7 @@ static int env_sf_init(void) * we must return with 0 if there is nothing done, * else env_set_inited() get not called in env_init() */ - return 0; + return -ENOENT; } But this may has impact on other environment drivers ... but may is the cleaner approach as env_init() later sets the default environment if ret is -ENOENT ... Thanks! bye, Heiko
Dear Heiko, In message <a80c8548-a8a5-fb98-f9e3-fc659c2bdfec@denx.de> you wrote: > > I enabled now ENV_APPEND on this board and > > CONFIG_ENV_IS_NOWHERE > CONFIG_ENV_IS_IN_SPI_FLASH This gives me the creeps. I know this is not cause by anything in your patch, but anyway... Apparently the meaning of CONFIG_ENV_IS_NOWHERE is nowhere documented :-( But common sense says that "IS NOWHERE" means that there is no storage defined for the environment. I would expect, that Kconfig does not even allow to enable any CONFIG_ENV_IS_IN_* when CONFIG_ENV_IS_NOWHERE is selected - these are logically exclusive. May I suggest that: 1) our Kconfig files are changed such that CONFIG_ENV_IS_NOWHERE and CONFIG_ENV_IS_IN_* are indeed exclusive, so that we adhere to the POLA [1] ? 2) for cases like this one, where there actually _is_ some storage defined, but it shall be used in a non-standard way, a new CONFIG_ option gets created that expresses in it's name what it does? [1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment Thanks! Best regards, Wolfgang Denk
Am 2020-11-02 08:00, schrieb Heiko Schocher: > Hello Michael, > > Am 01.11.2020 um 14:38 schrieb Michael Walle: >> Commit 92765f45bb95 ("env: Access Environment in SPI flashes before >> relocation") at least breaks the Kontron sl28 board. I guess it also >> breaks others which use a (late) SPI environment. >> >> Unfortunately, we cannot set the .init op and fall back to the same >> behavior as it would be unset. Thus guard the .init op by #if's. >> >> Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before >> relocation") >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> env/sf.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/env/sf.c b/env/sf.c >> index 2eb2de1a4e..18d44a7ddc 100644 >> --- a/env/sf.c >> +++ b/env/sf.c >> @@ -385,7 +385,7 @@ out: >> } >> #endif >> -static int env_sf_init(void) >> +static int __maybe_unused env_sf_init(void) >> { >> #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) >> return env_sf_init_addr(); >> @@ -393,8 +393,13 @@ static int env_sf_init(void) >> return env_sf_init_early(); >> #endif >> /* >> - * we must return with 0 if there is nothing done, >> - * else env_set_inited() get not called in env_init() >> + * We shouldn't end up here. Unfortunately, there is no >> + * way to return a value which yields the same behavior >> + * as if the .init op wouldn't be set at all. See >> + * env_init(); env_set_inited() is only called if we >> + * return 0, but the default environment is only loaded >> + * if -ENOENT is returned. Therefore, we need the ugly >> + * ifdeferry for the .init op. >> */ >> return 0; >> } >> @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = { >> ENV_NAME("SPIFlash") >> .load = env_sf_load, >> .save = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : >> NULL, >> +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || >> defined(CONFIG_ENV_SPI_EARLY) >> .init = env_sf_init, >> +#endif >> }; >> > > Ok, tested this patch on an imx6 based board with SPI NOR and it works. > > But.... there is a problem with environment in spi nor and ENV_APPEND > enabled, with current implementation (also before my patches applied): > > I enabled now ENV_APPEND on this board and > > CONFIG_ENV_IS_NOWHERE > CONFIG_ENV_IS_IN_SPI_FLASH > > and the Environment from SPI NOR never loaded as gd->env_valid is > always ENV_INVALID and env_load() never called from env_relocate(). > > What do you think about following patch: > > diff --git a/env/sf.c b/env/sf.c > index 2eb2de1a4e..7f3491b458 100644 > --- a/env/sf.c > +++ b/env/sf.c > @@ -393,9 +393,13 @@ static int env_sf_init(void) > return env_sf_init_early(); > #endif > /* > - * we must return with 0 if there is nothing done, > - * else env_set_inited() get not called in env_init() > + * We must return with 0 if there is nothing done, > + * to get inited bit set in env_init(). > + * We need to set env_valid to ENV_VALID, so later > + * env_load() loads the Environment from SPI NOR. > */ > + gd->env_addr = (ulong)&default_environment[0]; > + gd->env_valid = ENV_VALID; > return 0; > } > > Can you try it? This works for me... > Another option would be to reutrn -ENOENT and set init bit also > when a init function returns -ENOENT: > > diff --git a/env/env.c b/env/env.c > index 42c7d8155e..37b4b54cb7 100644 > --- a/env/env.c > +++ b/env/env.c > @@ -329,6 +329,8 @@ int env_init(void) > for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); > prio++) { > if (!drv->init || !(ret = drv->init())) > env_set_inited(drv->location); > + if (ret == -ENOENT) > + env_set_inited(drv->location); > > debug("%s: Environment %s init done (ret=%d)\n", > __func__, > drv->name, ret); > diff --git a/env/sf.c b/env/sf.c > index 2eb2de1a4e..66279fb4f4 100644 > --- a/env/sf.c > +++ b/env/sf.c > @@ -396,7 +396,7 @@ static int env_sf_init(void) > * we must return with 0 if there is nothing done, > * else env_set_inited() get not called in env_init() > */ > - return 0; > + return -ENOENT; > } > > But this may has impact on other environment drivers ... but may is > the cleaner approach as env_init() later sets the default environment > if ret is -ENOENT ... .. and also this. So we have four solutions (1) revert the series (2) apply my patch (3) use the first solution from Heiko (4) use the second solution from Heiko I'm fine with all four. If it will be (3) or (4) will you prepare a patch, Heiko? -michael
Hello Michael, Am 02.11.2020 um 21:15 schrieb Michael Walle: > Am 2020-11-02 08:00, schrieb Heiko Schocher: >> Hello Michael, >> >> Am 01.11.2020 um 14:38 schrieb Michael Walle: >>> Commit 92765f45bb95 ("env: Access Environment in SPI flashes before >>> relocation") at least breaks the Kontron sl28 board. I guess it also >>> breaks others which use a (late) SPI environment. >>> >>> Unfortunately, we cannot set the .init op and fall back to the same >>> behavior as it would be unset. Thus guard the .init op by #if's. >>> >>> Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation") >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> env/sf.c | 13 ++++++++++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/env/sf.c b/env/sf.c >>> index 2eb2de1a4e..18d44a7ddc 100644 >>> --- a/env/sf.c >>> +++ b/env/sf.c >>> @@ -385,7 +385,7 @@ out: >>> } >>> #endif >>> -static int env_sf_init(void) >>> +static int __maybe_unused env_sf_init(void) >>> { >>> #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) >>> return env_sf_init_addr(); >>> @@ -393,8 +393,13 @@ static int env_sf_init(void) >>> return env_sf_init_early(); >>> #endif >>> /* >>> - * we must return with 0 if there is nothing done, >>> - * else env_set_inited() get not called in env_init() >>> + * We shouldn't end up here. Unfortunately, there is no >>> + * way to return a value which yields the same behavior >>> + * as if the .init op wouldn't be set at all. See >>> + * env_init(); env_set_inited() is only called if we >>> + * return 0, but the default environment is only loaded >>> + * if -ENOENT is returned. Therefore, we need the ugly >>> + * ifdeferry for the .init op. >>> */ >>> return 0; >>> } >>> @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = { >>> ENV_NAME("SPIFlash") >>> .load = env_sf_load, >>> .save = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL, >>> +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY) >>> .init = env_sf_init, >>> +#endif >>> }; >>> >> >> Ok, tested this patch on an imx6 based board with SPI NOR and it works. >> >> But.... there is a problem with environment in spi nor and ENV_APPEND >> enabled, with current implementation (also before my patches applied): >> >> I enabled now ENV_APPEND on this board and >> >> CONFIG_ENV_IS_NOWHERE >> CONFIG_ENV_IS_IN_SPI_FLASH >> >> and the Environment from SPI NOR never loaded as gd->env_valid is >> always ENV_INVALID and env_load() never called from env_relocate(). >> >> What do you think about following patch: >> >> diff --git a/env/sf.c b/env/sf.c >> index 2eb2de1a4e..7f3491b458 100644 >> --- a/env/sf.c >> +++ b/env/sf.c >> @@ -393,9 +393,13 @@ static int env_sf_init(void) >> return env_sf_init_early(); >> #endif >> /* >> - * we must return with 0 if there is nothing done, >> - * else env_set_inited() get not called in env_init() >> + * We must return with 0 if there is nothing done, >> + * to get inited bit set in env_init(). >> + * We need to set env_valid to ENV_VALID, so later >> + * env_load() loads the Environment from SPI NOR. >> */ >> + gd->env_addr = (ulong)&default_environment[0]; >> + gd->env_valid = ENV_VALID; >> return 0; >> } >> >> Can you try it? > > This works for me... > >> Another option would be to reutrn -ENOENT and set init bit also >> when a init function returns -ENOENT: >> >> diff --git a/env/env.c b/env/env.c >> index 42c7d8155e..37b4b54cb7 100644 >> --- a/env/env.c >> +++ b/env/env.c >> @@ -329,6 +329,8 @@ int env_init(void) >> for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { >> if (!drv->init || !(ret = drv->init())) >> env_set_inited(drv->location); >> + if (ret == -ENOENT) >> + env_set_inited(drv->location); >> >> debug("%s: Environment %s init done (ret=%d)\n", __func__, >> drv->name, ret); >> diff --git a/env/sf.c b/env/sf.c >> index 2eb2de1a4e..66279fb4f4 100644 >> --- a/env/sf.c >> +++ b/env/sf.c >> @@ -396,7 +396,7 @@ static int env_sf_init(void) >> * we must return with 0 if there is nothing done, >> * else env_set_inited() get not called in env_init() >> */ >> - return 0; >> + return -ENOENT; >> } >> >> But this may has impact on other environment drivers ... but may is >> the cleaner approach as env_init() later sets the default environment >> if ret is -ENOENT ... > > .. and also this. > > So we have four solutions > (1) revert the series > (2) apply my patch > (3) use the first solution from Heiko > (4) use the second solution from Heiko > > I'm fine with all four. If it will be (3) or (4) will you prepare a > patch, Heiko? I tend to implement solution [4] ... I can send a patch... Simon? Tom? Any suggestions? bye, Heiko
Hello Wolfgang, Am 02.11.2020 um 13:51 schrieb Wolfgang Denk: > Dear Heiko, > > In message <a80c8548-a8a5-fb98-f9e3-fc659c2bdfec@denx.de> you wrote: >> >> I enabled now ENV_APPEND on this board and >> >> CONFIG_ENV_IS_NOWHERE >> CONFIG_ENV_IS_IN_SPI_FLASH > > This gives me the creeps. I know this is not cause by anything in > your patch, but anyway... > > Apparently the meaning of CONFIG_ENV_IS_NOWHERE is nowhere > documented :-( env/Kconfig says: config ENV_IS_NOWHERE bool "Environment is not stored" default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \ !ENV_IS_IN_FAT && !ENV_IS_IN_FLASH && \ !ENV_IS_IN_MMC && !ENV_IS_IN_NAND && \ !ENV_IS_IN_NVRAM && !ENV_IS_IN_ONENAND && \ !ENV_IS_IN_REMOTE && !ENV_IS_IN_SPI_FLASH && \ !ENV_IS_IN_UBI help Define this if you don't want to or can't have an environment stored on a storage medium. In this case the environment will still exist while U-Boot is running, but once U-Boot exits it will not be stored. U-Boot will therefore always start up with a default environment. > But common sense says that "IS NOWHERE" means that there is no > storage defined for the environment. I would expect, that Kconfig Yes and use default one ... > does not even allow to enable any CONFIG_ENV_IS_IN_* when > CONFIG_ENV_IS_NOWHERE is selected - these are logically exclusive. Hmm... > May I suggest that: > > 1) our Kconfig files are changed such that CONFIG_ENV_IS_NOWHERE and > CONFIG_ENV_IS_IN_* are indeed exclusive, so that we adhere to the > POLA [1] ? Yes if we really want such a hard setting without having an environment! Currently CONFIG_ENV_IS_NOWHERE is not that hard and means U-Boot has no Environment ... it means use the built in default environment... > 2) for cases like this one, where there actually _is_ some storage > defined, but it shall be used in a non-standard way, a new > CONFIG_ option gets created that expresses in it's name what it > does? Not in a none standard way! Instead you can define more than one environment storage devices and load them in a board specific order (defined thorugh board specfif function env_get_location()) For example: - load first default environment (and you are correct ENV_IS_NOWHERE is here really a misleading name). - load environment from SPI NOR ... May we only should do a simple rename ? ENV_IS_NOWHERE -> ENV_IS_IN_UBOOT (or ENV_IS_IN_DEFAULT) ? If we really want a hard "there is no storage" switch (which really means there is *no* environment ... I do not even know, if U-boot works without!) we should introduce a new ENV_IS_IN_DEFAULT which loads the default environment... (I do not like this, as all? boards have a default environment, so it is enabled for all? boards ... which makes it obsolete...) better suggestions? bye, Heiko > > [1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment > > Thanks! > > Best regards, > > Wolfgang Denk >
Dear Heiko, In message <c32e8504-1f86-8579-3240-e4cf41e847c9@denx.de> you wrote: > > > Apparently the meaning of CONFIG_ENV_IS_NOWHERE is nowhere > > documented :-( > > env/Kconfig says: Ah, I missed that. I was only checkng the README and the doc/ files... > config ENV_IS_NOWHERE > bool "Environment is not stored" OK, but this is a pretty clear statement. > help > Define this if you don't want to or can't have an environment stored > on a storage medium. In this case the environment will still exist > while U-Boot is running, but once U-Boot exits it will not be > stored. U-Boot will therefore always start up with a default > environment. > > > > But common sense says that "IS NOWHERE" means that there is no > > storage defined for the environment. I would expect, that Kconfig > > Yes and use default one ... Correct. So this matches my understanding of the name of the config option: use this if there is no storage defined for the environment. > > does not even allow to enable any CONFIG_ENV_IS_IN_* when > > CONFIG_ENV_IS_NOWHERE is selected - these are logically exclusive. > > Hmm... > > > May I suggest that: > > > > 1) our Kconfig files are changed such that CONFIG_ENV_IS_NOWHERE and > > CONFIG_ENV_IS_IN_* are indeed exclusive, so that we adhere to the > > POLA [1] ? > > Yes if we really want such a hard setting without having an environment! Well, this is exactly what this config option is supposed to do - both according to the name and according to the documentation: "the environment ... will not be stored." > Currently CONFIG_ENV_IS_NOWHERE is not that hard and means U-Boot has > no Environment ... it means use the built in default environment... I would rephrase this: Currently the implemantation does not enforce the correct behaviour, so it has been misused for other (also useful) things. But this is not clean and should be fixed. > > 2) for cases like this one, where there actually _is_ some storage > > defined, but it shall be used in a non-standard way, a new > > CONFIG_ option gets created that expresses in it's name what it > > does? > > Not in a none standard way! Instead you can define more than one > environment storage devices and load them in a board specific order > (defined thorugh board specfif function env_get_location()) Yes, agreed. But this logically impossible if there is no storage at all, which is what CONFIG_ENV_IS_NOWHERE says. For such cases we must not define CONFIG_ENV_IS_NOWHERE, but something else / something new instead. > For example: > - load first default environment (and you are correct ENV_IS_NOWHERE > is here really a misleading name). > > - load environment from SPI NOR ... > > May we only should do a simple rename ? > > ENV_IS_NOWHERE -> ENV_IS_IN_UBOOT (or ENV_IS_IN_DEFAULT) ? In my understanding, ENV_IS_IN_* defines the storage device used to save the persistent copy (the external representation(s)) of the environment, which get written when you use "env save". So both your suggestions are not acceptable, as the the envrionment never gets saved to the default environment - this is just the data that is used to initialize it in the same was as in an emergency when the persistent the environment (more precisely: when all copies of it) cannot be read (I/O or checksum error). > If we really want a hard "there is no storage" switch (which really You miss the fact that we already have this: CONFIG_ENV_IS_NOWHERE It is just not implemented correctly, and I ask for a cleanup. > means there is *no* environment ... I do not even know, if U-boot > works without!) we should introduce a new ENV_IS_IN_DEFAULT which > loads the default environment... You misunderstand. "No storage" means no storage for writing the _persistent_ environment, see above. Int his case there is still the same emergency handling as in cases of corrupted peristent environment - the fallback to the builtin (socalled "default") environment. Best regards, Wolfgang Denk
On 03/11/2020 08.52, Wolfgang Denk wrote: > Dear Heiko, > > In message <c32e8504-1f86-8579-3240-e4cf41e847c9@denx.de> you wrote: >> >>> Apparently the meaning of CONFIG_ENV_IS_NOWHERE is nowhere >>> documented :-( >> >> env/Kconfig says: > > Ah, I missed that. I was only checkng the README and the doc/ > files... > >> config ENV_IS_NOWHERE >> bool "Environment is not stored" > > OK, but this is a pretty clear statement. > >> help >> Define this if you don't want to or can't have an environment stored >> on a storage medium. In this case the environment will still exist >> while U-Boot is running, but once U-Boot exits it will not be >> stored. U-Boot will therefore always start up with a default >> environment. >> >> >>> But common sense says that "IS NOWHERE" means that there is no >>> storage defined for the environment. I would expect, that Kconfig >> >> Yes and use default one ... > > Correct. So this matches my understanding of the name of the config > option: use this if there is no storage defined for the environment. > >>> does not even allow to enable any CONFIG_ENV_IS_IN_* when >>> CONFIG_ENV_IS_NOWHERE is selected - these are logically exclusive. >> >> Hmm... >> >>> May I suggest that: >>> >>> 1) our Kconfig files are changed such that CONFIG_ENV_IS_NOWHERE and >>> CONFIG_ENV_IS_IN_* are indeed exclusive, so that we adhere to the >>> POLA [1] ? >> >> Yes if we really want such a hard setting without having an environment! > > Well, this is exactly what this config option is supposed to do - > both according to the name and according to the documentation: "the > environment ... will not be stored." > >> Currently CONFIG_ENV_IS_NOWHERE is not that hard and means U-Boot has >> no Environment ... it means use the built in default environment... > > I would rephrase this: Currently the implemantation does not enforce > the correct behaviour, so it has been misused for other (also > useful) things. But this is not clean and should be fixed. > >>> 2) for cases like this one, where there actually _is_ some storage >>> defined, but it shall be used in a non-standard way, a new >>> CONFIG_ option gets created that expresses in it's name what it >>> does? >> >> Not in a none standard way! Instead you can define more than one >> environment storage devices and load them in a board specific order >> (defined thorugh board specfif function env_get_location()) > > Yes, agreed. But this logically impossible if there is no storage > at all, which is what CONFIG_ENV_IS_NOWHERE says. Then should all the current config options CONFIG_ENV_IS_ be renamed to CONFIG_ENV_MAY_BE_? Because that's really what they mean. I certainly agree the current naming isn't very accurate, but the ability to choose CONFIG_ENV_IS_NOWHERE along with some "real" storage option is quite useful. We use it so that we can use the exact same U-Boot binary for both development and production, just different .dtbs; there's a board-specific env_get_location() which reads a DT property to decide whether to return ENVL_SPI_FLASH or ENVL_NOWHERE. Whether there actually needs to be a stub nowhere.c driver for that or one could just return ENVL_UNKNOWN I don't know. Rasmus
On Tue, Nov 03, 2020 at 05:40:16AM +0100, Heiko Schocher wrote: > Hello Michael, > > Am 02.11.2020 um 21:15 schrieb Michael Walle: > > Am 2020-11-02 08:00, schrieb Heiko Schocher: > > > Hello Michael, > > > > > > Am 01.11.2020 um 14:38 schrieb Michael Walle: > > > > Commit 92765f45bb95 ("env: Access Environment in SPI flashes before > > > > relocation") at least breaks the Kontron sl28 board. I guess it also > > > > breaks others which use a (late) SPI environment. > > > > > > > > Unfortunately, we cannot set the .init op and fall back to the same > > > > behavior as it would be unset. Thus guard the .init op by #if's. > > > > > > > > Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation") > > > > Signed-off-by: Michael Walle <michael@walle.cc> > > > > --- > > > > env/sf.c | 13 ++++++++++--- > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/env/sf.c b/env/sf.c > > > > index 2eb2de1a4e..18d44a7ddc 100644 > > > > --- a/env/sf.c > > > > +++ b/env/sf.c > > > > @@ -385,7 +385,7 @@ out: > > > > } > > > > #endif > > > > -static int env_sf_init(void) > > > > +static int __maybe_unused env_sf_init(void) > > > > { > > > > #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) > > > > return env_sf_init_addr(); > > > > @@ -393,8 +393,13 @@ static int env_sf_init(void) > > > > return env_sf_init_early(); > > > > #endif > > > > /* > > > > - * we must return with 0 if there is nothing done, > > > > - * else env_set_inited() get not called in env_init() > > > > + * We shouldn't end up here. Unfortunately, there is no > > > > + * way to return a value which yields the same behavior > > > > + * as if the .init op wouldn't be set at all. See > > > > + * env_init(); env_set_inited() is only called if we > > > > + * return 0, but the default environment is only loaded > > > > + * if -ENOENT is returned. Therefore, we need the ugly > > > > + * ifdeferry for the .init op. > > > > */ > > > > return 0; > > > > } > > > > @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = { > > > > ENV_NAME("SPIFlash") > > > > .load = env_sf_load, > > > > .save = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL, > > > > +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY) > > > > .init = env_sf_init, > > > > +#endif > > > > }; > > > > > > > > > > Ok, tested this patch on an imx6 based board with SPI NOR and it works. > > > > > > But.... there is a problem with environment in spi nor and ENV_APPEND > > > enabled, with current implementation (also before my patches applied): > > > > > > I enabled now ENV_APPEND on this board and > > > > > > CONFIG_ENV_IS_NOWHERE > > > CONFIG_ENV_IS_IN_SPI_FLASH > > > > > > and the Environment from SPI NOR never loaded as gd->env_valid is > > > always ENV_INVALID and env_load() never called from env_relocate(). > > > > > > What do you think about following patch: > > > > > > diff --git a/env/sf.c b/env/sf.c > > > index 2eb2de1a4e..7f3491b458 100644 > > > --- a/env/sf.c > > > +++ b/env/sf.c > > > @@ -393,9 +393,13 @@ static int env_sf_init(void) > > > return env_sf_init_early(); > > > #endif > > > /* > > > - * we must return with 0 if there is nothing done, > > > - * else env_set_inited() get not called in env_init() > > > + * We must return with 0 if there is nothing done, > > > + * to get inited bit set in env_init(). > > > + * We need to set env_valid to ENV_VALID, so later > > > + * env_load() loads the Environment from SPI NOR. > > > */ > > > + gd->env_addr = (ulong)&default_environment[0]; > > > + gd->env_valid = ENV_VALID; > > > return 0; > > > } > > > > > > Can you try it? > > > > This works for me... > > > > > Another option would be to reutrn -ENOENT and set init bit also > > > when a init function returns -ENOENT: > > > > > > diff --git a/env/env.c b/env/env.c > > > index 42c7d8155e..37b4b54cb7 100644 > > > --- a/env/env.c > > > +++ b/env/env.c > > > @@ -329,6 +329,8 @@ int env_init(void) > > > for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { > > > if (!drv->init || !(ret = drv->init())) > > > env_set_inited(drv->location); > > > + if (ret == -ENOENT) > > > + env_set_inited(drv->location); > > > > > > debug("%s: Environment %s init done (ret=%d)\n", __func__, > > > drv->name, ret); > > > diff --git a/env/sf.c b/env/sf.c > > > index 2eb2de1a4e..66279fb4f4 100644 > > > --- a/env/sf.c > > > +++ b/env/sf.c > > > @@ -396,7 +396,7 @@ static int env_sf_init(void) > > > * we must return with 0 if there is nothing done, > > > * else env_set_inited() get not called in env_init() > > > */ > > > - return 0; > > > + return -ENOENT; > > > } > > > > > > But this may has impact on other environment drivers ... but may is > > > the cleaner approach as env_init() later sets the default environment > > > if ret is -ENOENT ... > > > > .. and also this. > > > > So we have four solutions > > (1) revert the series > > (2) apply my patch > > (3) use the first solution from Heiko > > (4) use the second solution from Heiko > > > > I'm fine with all four. If it will be (3) or (4) will you prepare a > > patch, Heiko? > > I tend to implement solution [4] ... I can send a patch... > > Simon? Tom? Any suggestions? Lets go with #4 above then please, thanks!
Dear Rasmus, In message <8ff3b8ad-8c4e-fe99-69c8-7c174e997a49@prevas.dk> you wrote: > > >> Not in a none standard way! Instead you can define more than one > >> environment storage devices and load them in a board specific order > >> (defined thorugh board specfif function env_get_location()) > > > > Yes, agreed. But this logically impossible if there is no storage > > at all, which is what CONFIG_ENV_IS_NOWHERE says. > > Then should all the current config options CONFIG_ENV_IS_ be renamed to > CONFIG_ENV_MAY_BE_? Because that's really what they mean. This is not correct. The CONFIG_ENV_IS_FOO means: if you use "env save", then U-Boot will write the config to external storage using the FOO storage device. > I certainly agree the current naming isn't very accurate, but the > ability to choose CONFIG_ENV_IS_NOWHERE along with some "real" storage > option is quite useful. We use it so that we can use the exact same > U-Boot binary for both development and production, just different .dtbs; > there's a board-specific env_get_location() which reads a DT property to > decide whether to return ENVL_SPI_FLASH or ENVL_NOWHERE. Whether there > actually needs to be a stub nowhere.c driver for that or one could just > return ENVL_UNKNOWN I don't know. If it makes sense do have a nulldev, then it should be added as a CONFIG_ENV_IS_ option. This is something fundamental different from NOWHERE. If you define CONFIG_ENV_IS_NOWHERE, I would for example expect that all "env save" related code is omitted as we will never need it. Best regards, Wolfgang Denk
On 05/11/2020 17.40, Wolfgang Denk wrote: > Dear Rasmus, > > In message <8ff3b8ad-8c4e-fe99-69c8-7c174e997a49@prevas.dk> you wrote: >> >>>> Not in a none standard way! Instead you can define more than one >>>> environment storage devices and load them in a board specific order >>>> (defined thorugh board specfif function env_get_location()) >>> >>> Yes, agreed. But this logically impossible if there is no storage >>> at all, which is what CONFIG_ENV_IS_NOWHERE says. >> >> Then should all the current config options CONFIG_ENV_IS_ be renamed to >> CONFIG_ENV_MAY_BE_? Because that's really what they mean. > > This is not correct. > > The CONFIG_ENV_IS_FOO means: if you use "env save", then U-Boot will > write the config to external storage using the FOO storage device. Wolfgang, you're wrong. What you're saying was once true, when the location was a "choice" in Kconfig, but it hasn't been that since fb69464eae. Nowadays one can select multiple possible backends, and only one of them will be used when doing "env save". Later (208bd2b8), _NOWHERE was made non-mutually-exclusive with the real storage targets. > If you define CONFIG_ENV_IS_NOWHERE, I would for example expect that > all "env save" related code is omitted as we will never need it. I haven't checked, but that functionality does seem to exist - not depending on whether CONFIG_ENV_IS_NOWHERE is not selected, but whether any of the CONFIG_ENV_IS_<somehere> is. See the ENV_IS_IN_DEVICE logic in cmd/nvedit.c. So if you select CONFIG_ENV_IS_NOWHERE and not any of the others, I think the build works as you'd expect. Rasmus
On Fri, Nov 06, 2020 at 08:46:27AM +0100, Rasmus Villemoes wrote: > On 05/11/2020 17.40, Wolfgang Denk wrote: > > Dear Rasmus, > > > > In message <8ff3b8ad-8c4e-fe99-69c8-7c174e997a49@prevas.dk> you wrote: > >> > >>>> Not in a none standard way! Instead you can define more than one > >>>> environment storage devices and load them in a board specific order > >>>> (defined thorugh board specfif function env_get_location()) > >>> > >>> Yes, agreed. But this logically impossible if there is no storage > >>> at all, which is what CONFIG_ENV_IS_NOWHERE says. > >> > >> Then should all the current config options CONFIG_ENV_IS_ be renamed to > >> CONFIG_ENV_MAY_BE_? Because that's really what they mean. > > > > This is not correct. > > > > The CONFIG_ENV_IS_FOO means: if you use "env save", then U-Boot will > > write the config to external storage using the FOO storage device. > > Wolfgang, you're wrong. What you're saying was once true, when the > location was a "choice" in Kconfig, but it hasn't been that since > fb69464eae. Nowadays one can select multiple possible backends, and only > one of them will be used when doing "env save". > > Later (208bd2b8), _NOWHERE was made non-mutually-exclusive with the real > storage targets. Yes. It's intentional that "NOWHERE" means that if this is our run-time location for the environment, then you cannot save the environment. But since we finally implemented the ability to have more than one option enabled and pick the correct one at run time, we build both "mmc" and "nowhere". > > If you define CONFIG_ENV_IS_NOWHERE, I would for example expect that > > all "env save" related code is omitted as we will never need it. > > I haven't checked, but that functionality does seem to exist - not > depending on whether CONFIG_ENV_IS_NOWHERE is not selected, but whether > any of the CONFIG_ENV_IS_<somehere> is. See the ENV_IS_IN_DEVICE logic > in cmd/nvedit.c. So if you select CONFIG_ENV_IS_NOWHERE and not any of > the others, I think the build works as you'd expect. So, CMD_SAVEENV exists. If you don't enable that, I would expect save-related code that can be garbage collected to be discarded. It's unclear without further digging if there's code that could be discarded that is not, but checking the map file for a "nowhere" only build would probably be somewhat instructive.
Dear Rasmus, In message <ac9500de-d236-be83-04a8-8f68be1c7672@prevas.dk> you wrote: > > > The CONFIG_ENV_IS_FOO means: if you use "env save", then U-Boot will > > write the config to external storage using the FOO storage device. > > Wolfgang, you're wrong. What you're saying was once true, when the > location was a "choice" in Kconfig, but it hasn't been that since > fb69464eae. Nowadays one can select multiple possible backends, and only > one of them will be used when doing "env save". I know that. But that slection is made from the selected drivers enabled by the CONFIG_ENV_IS_* settings. > Later (208bd2b8), _NOWHERE was made non-mutually-exclusive with the real > storage targets. And this is a buf that should be fixed, as it makes no sense at all. > > If you define CONFIG_ENV_IS_NOWHERE, I would for example expect that > > all "env save" related code is omitted as we will never need it. > > I haven't checked, but that functionality does seem to exist - not > depending on whether CONFIG_ENV_IS_NOWHERE is not selected, but whether > any of the CONFIG_ENV_IS_<somehere> is. See the ENV_IS_IN_DEVICE logic > in cmd/nvedit.c. So if you select CONFIG_ENV_IS_NOWHERE and not any of > the others, I think the build works as you'd expect. This is intransparent and error prone. This check must not be implemented somewhere in the code, but at Kconfig level. Best regards, Wolfgang Denk
Dear Tom, In message <20201106204557.GG5340@bill-the-cat> you wrote: > > > Later (208bd2b8), _NOWHERE was made non-mutually-exclusive with the real > > storage targets. > > Yes. It's intentional that "NOWHERE" means that if this is our run-time > location for the environment, then you cannot save the environment. But > since we finally implemented the ability to have more than one option > enabled and pick the correct one at run time, we build both "mmc" and > "nowhere". But this breaks logic. If you wanna have a nulldev, than implement a /dev/null. CONFIG_ENV_IS_NOWHERE should be the means to be used when no environment storage drivers are needed / selected, not even /dev/null. Best regards, Wolfgang Denk
diff --git a/env/sf.c b/env/sf.c index 2eb2de1a4e..18d44a7ddc 100644 --- a/env/sf.c +++ b/env/sf.c @@ -385,7 +385,7 @@ out: } #endif -static int env_sf_init(void) +static int __maybe_unused env_sf_init(void) { #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) return env_sf_init_addr(); @@ -393,8 +393,13 @@ static int env_sf_init(void) return env_sf_init_early(); #endif /* - * we must return with 0 if there is nothing done, - * else env_set_inited() get not called in env_init() + * We shouldn't end up here. Unfortunately, there is no + * way to return a value which yields the same behavior + * as if the .init op wouldn't be set at all. See + * env_init(); env_set_inited() is only called if we + * return 0, but the default environment is only loaded + * if -ENOENT is returned. Therefore, we need the ugly + * ifdeferry for the .init op. */ return 0; } @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = { ENV_NAME("SPIFlash") .load = env_sf_load, .save = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL, +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY) .init = env_sf_init, +#endif };
Commit 92765f45bb95 ("env: Access Environment in SPI flashes before relocation") at least breaks the Kontron sl28 board. I guess it also breaks others which use a (late) SPI environment. Unfortunately, we cannot set the .init op and fall back to the same behavior as it would be unset. Thus guard the .init op by #if's. Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation") Signed-off-by: Michael Walle <michael@walle.cc> --- env/sf.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)