[PATCH] Revert "env: Load env when ENV_IS_NOWHERE is only location selected"

[PATCH] Revert "env: Load env when ENV_IS_NOWHERE is only location selected"

[Marek Vasut]

This reverts commit 8d61237edbf6314a701cf78da2c5893a73ff5438.

This commit broke environment on literally every board I have access
to, with this revert in place, environment works as it should again.
The problem I observe with this patch is that saved environment in
either SPI NOR or eMMC is never used, the system always falls back
to default environment. The 'saveenv' command does succeed, but then
after reset, the default env is again used.

Furthermore, the commit introduced duplicate code in env_init(), this:
"
	if (!prio) {
		gd->env_addr = (ulong)&default_environment[0];
		gd->env_valid = ENV_INVALID;

		return 0;
	}

	if (ret == -ENOENT) {
		gd->env_addr = (ulong)&default_environment[0];
		gd->env_valid = ENV_INVALID;

		return 0;
	}
"

Furthermore, the commit is missing DCO SoB line.

Also note that upstream does not support UltraZed EG board, so
this might have been a patch pulled from downstream which did
depend on some other downstream behavior.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Felix.Vietmeyer@jila.colorado.edu <felix.vietmeyer@jila.colorado.edu>
Cc: Tom Rini <trini@konsulko.com>
---
 env/env.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/env/env.c b/env/env.c
index 7168cb9d318..e4dfb92e154 100644
--- a/env/env.c
+++ b/env/env.c
@@ -322,18 +322,17 @@ int env_init(void)
 
 		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
-	}
-
-	if (!prio) {
-		gd->env_addr = (ulong)&default_environment[0];
-		gd->env_valid = ENV_INVALID;
 
-		return 0;
+		if (gd->env_valid == ENV_INVALID)
+			ret = -ENOENT;
 	}
 
+	if (!prio)
+		return -ENODEV;
+
 	if (ret == -ENOENT) {
 		gd->env_addr = (ulong)&default_environment[0];
-		gd->env_valid = ENV_INVALID;
+		gd->env_valid = ENV_VALID;
 
 		return 0;
 	}
-- 
2.35.1

Re: [PATCH] Revert "env: Load env when ENV_IS_NOWHERE is only location selected"

[Felix.Vietmeyer]

From your description it seems like the board I was working on must have
had an environment on NOR or eMMC which could be loaded but did not allow
the board to come up.
Then, the patch forces u-boot to use the default environment despite an
existing env on NOR or eMMC.

> This reverts commit 8d61237edbf6314a701cf78da2c5893a73ff5438.
>
> This commit broke environment on literally every board I have access
> to, with this revert in place, environment works as it should again.
> The problem I observe with this patch is that saved environment in
> either SPI NOR or eMMC is never used, the system always falls back
> to default environment. The 'saveenv' command does succeed, but then
> after reset, the default env is again used.
>
> Furthermore, the commit introduced duplicate code in env_init(), this:
> "
> 	if (!prio) {
> 		gd->env_addr = (ulong)&default_environment[0];
> 		gd->env_valid = ENV_INVALID;
>
> 		return 0;
> 	}
>
> 	if (ret == -ENOENT) {
> 		gd->env_addr = (ulong)&default_environment[0];
> 		gd->env_valid = ENV_INVALID;
>
> 		return 0;
> 	}
> "
>
> Furthermore, the commit is missing DCO SoB line.
>
> Also note that upstream does not support UltraZed EG board, so
> this might have been a patch pulled from downstream which did
> depend on some other downstream behavior.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Felix.Vietmeyer@jila.colorado.edu <felix.vietmeyer@jila.colorado.edu>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  env/env.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/env/env.c b/env/env.c
> index 7168cb9d318..e4dfb92e154 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -322,18 +322,17 @@ int env_init(void)
>
>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>  		      drv->name, ret);
> -	}
> -
> -	if (!prio) {
> -		gd->env_addr = (ulong)&default_environment[0];
> -		gd->env_valid = ENV_INVALID;
>
> -		return 0;
> +		if (gd->env_valid == ENV_INVALID)
> +			ret = -ENOENT;
>  	}
>
> +	if (!prio)
> +		return -ENODEV;
> +
>  	if (ret == -ENOENT) {
>  		gd->env_addr = (ulong)&default_environment[0];
> -		gd->env_valid = ENV_INVALID;
> +		gd->env_valid = ENV_VALID;
>
>  		return 0;
>  	}
> --
> 2.35.1
>

Re: [PATCH] Revert "env: Load env when ENV_IS_NOWHERE is only location selected"

[Marek Vasut]

On 4/10/22 13:54, Felix.Vietmeyer@jila.colorado.edu wrote:
>>From your description it seems like the board I was working on must have
> had an environment on NOR or eMMC which could be loaded but did not allow
> the board to come up.
> Then, the patch forces u-boot to use the default environment despite an
> existing env on NOR or eMMC.

I suspect the board must have some other problem with the environment.
I just cannot tell what it is. This is using some downstream U-Boot 
fork, right ? Can you reproduce the problem in mainline too ? How ?

Re: [PATCH] Revert "env: Load env when ENV_IS_NOWHERE is only location selected"

[Felix.Vietmeyer]

I am not exactly sure to be honest. We were running with this patched
version for a while and it worked from what I remember.

But we have since updated u-boot and the Kernel and the latest version
does not depend on this patch.

Nevertheless, I can try to recreate the problem and see if I can figure it
out now. I feel like I might pick up on a few more things now that went
unnoticed by me when I created the patch.

> On 4/10/22 13:54, Felix.Vietmeyer@jila.colorado.edu wrote:
>>>From your description it seems like the board I was working on must have
>> had an environment on NOR or eMMC which could be loaded but did not
>> allow
>> the board to come up.
>> Then, the patch forces u-boot to use the default environment despite an
>> existing env on NOR or eMMC.
>
> I suspect the board must have some other problem with the environment.
> I just cannot tell what it is. This is using some downstream U-Boot
> fork, right ? Can you reproduce the problem in mainline too ? How ?
>

Re: [PATCH] Revert "env: Load env when ENV_IS_NOWHERE is only location selected"

[Marek Vasut]

On 4/13/22 02:49, Felix.Vietmeyer@jila.colorado.edu wrote:
> I am not exactly sure to be honest. We were running with this patched
> version for a while and it worked from what I remember.
> 
> But we have since updated u-boot and the Kernel and the latest version
> does not depend on this patch.
> 
> Nevertheless, I can try to recreate the problem and see if I can figure it
> out now. I feel like I might pick up on a few more things now that went
> unnoticed by me when I created the patch.

Thanks. Please CC me if you have a V2.

Re: [PATCH] Revert "env: Load env when ENV_IS_NOWHERE is only location selected"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On Sun, Apr 10, 2022 at 06:46:52AM +0200, Marek Vasut wrote:

> This reverts commit 8d61237edbf6314a701cf78da2c5893a73ff5438.
> 
> This commit broke environment on literally every board I have access
> to, with this revert in place, environment works as it should again.
> The problem I observe with this patch is that saved environment in
> either SPI NOR or eMMC is never used, the system always falls back
> to default environment. The 'saveenv' command does succeed, but then
> after reset, the default env is again used.
> 
> Furthermore, the commit introduced duplicate code in env_init(), this:
> "
> 	if (!prio) {
> 		gd->env_addr = (ulong)&default_environment[0];
> 		gd->env_valid = ENV_INVALID;
> 
> 		return 0;
> 	}
> 
> 	if (ret == -ENOENT) {
> 		gd->env_addr = (ulong)&default_environment[0];
> 		gd->env_valid = ENV_INVALID;
> 
> 		return 0;
> 	}
> "
> 
> Furthermore, the commit is missing DCO SoB line.
> 
> Also note that upstream does not support UltraZed EG board, so
> this might have been a patch pulled from downstream which did
> depend on some other downstream behavior.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Felix.Vietmeyer@jila.colorado.edu <felix.vietmeyer@jila.colorado.edu>
> Cc: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]