[U-Boot] [PATCH] Revert "env: add spi_flash_read_env function"

[Horatiu Vultur]

Hi Heiko,

I managed to reproduce the issue that you described.

Don't you think it is a little bit too harsh to remove the commit
completely?

I am not sure but how many cases are where UBoot doesn't store anything
in flash? And then lets the linux to update the flash.

Or is wouldn't better to update fw_setenv to set the entire flash to
0x0 when it detects that the CRC error? The same way how the saveenv it
is doing in UBoot?

/Horatiu

The 03/13/2019 12:15, Heiko Schocher wrote:
> This reverts commit 9a9d66f5eff0f443de4c2c6ca3e27771ed14b1b4.
> 
> because it breaks fw_setenv and U-Boot interworking, if
> U-Boot environment is stored in a SPI-NOR.
> 
> Reproduce it with:
> boot linux with empty Environment and store a variable
> with fw_setenv into it, the Environment is now filled
> with 0xff:
> 
> root at ckey5e:10:8e:~# hexdump -C /dev/mtd4
> 00000000  e9 e8 07 fa 01 62 6f 6f  74 63 6d 64 3d 72 75 6e  |.....bootcmd=run|
> [...]
> 00000f30  7d 00 75 62 69 62 6f 6f  74 76 6f 6c 3d 32 00 00  |}.ubibootvol=2..|
> 00000f40  00 ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
> 
> Boot now U-Boot prints:
> 
> Loading Environment from SPI Flash... SF: Detected s25fl128l with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> *** Warning - bad CRC, using default environment
> 
> Reason is the above commit, as it only reads until \0\0
> is found, and assumes the rest of the Environment
> space is filled with 0x00, which is not the case when
> saving an Environment under linux with fw_setenv.
> 
> Signed-off-by: Heiko Schocher <hs at denx.de>
> ---
> I already posted a fix:
> https://lists.denx.de/pipermail/u-boot/2019-January/355148.html
> which I withdrawed:
> https://lists.denx.de/pipermail/u-boot/2019-January/355529.html
> 
> because I did not recognised the correlation between fw_setenv
> in linux context and saveenv in U-Boot context.
> 
> I think now, best is to revert this commit.
> 
> Comments?
> 
> This patch drops checkpatch warnings:
> env/sf.c:120: check: Alignment should match open parenthesis
> env/sf.c:224: check: Alignment should match open parenthesis
> env/sf.c:281: check: Alignment should match open parenthesis
> 
> but as it is a revert I did not correct them.
> 
>  env/sf.c | 56 +++++++++++---------------------------------------------
>  1 file changed, 11 insertions(+), 45 deletions(-)
> 
> diff --git a/env/sf.c b/env/sf.c
> index b3dec82c35..23cbad5d88 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -81,40 +81,6 @@ static int setup_flash_device(void)
>  	return 0;
>  }
>  
> -static int is_end(const char *addr, size_t size)
> -{
> -	/* The end of env variables is marked by '\0\0' */
> -	int i = 0;
> -
> -	for (i = 0; i < size - 1; ++i)
> -		if (addr[i] == 0x0 && addr[i + 1] == 0x0)
> -			return 1;
> -	return 0;
> -}
> -
> -static int spi_flash_read_env(struct spi_flash *flash, u32 offset, size_t len,
> -			      void *buf)
> -{
> -	u32 addr = 0;
> -	u32 page_size = flash->page_size;
> -
> -	memset(buf, 0x0, len);
> -	for (int i = 0; i < len / page_size; ++i) {
> -		int ret = spi_flash_read(flash, offset, page_size,
> -					 &((char *)buf)[addr]);
> -
> -		if (ret < 0)
> -			return ret;
> -
> -		if (is_end(&((char *)buf)[addr], page_size))
> -			return 0;
> -
> -		addr += page_size;
> -		offset += page_size;
> -	}
> -	return 0;
> -}
> -
>  #if defined(CONFIG_ENV_OFFSET_REDUND)
>  #ifdef CMD_SAVEENV
>  static int env_sf_save(void)
> @@ -150,8 +116,8 @@ static int env_sf_save(void)
>  			ret = -ENOMEM;
>  			goto done;
>  		}
> -		ret = spi_flash_read_env(env_flash, saved_offset,
> -					 saved_size, saved_buffer);
> +		ret = spi_flash_read(env_flash, saved_offset,
> +					saved_size, saved_buffer);
>  		if (ret)
>  			goto done;
>  	}
> @@ -217,10 +183,10 @@ static int env_sf_load(void)
>  	if (ret)
>  		goto out;
>  
> -	read1_fail = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET,
> -					CONFIG_ENV_SIZE, tmp_env1);
> -	read2_fail = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET_REDUND,
> -					CONFIG_ENV_SIZE, tmp_env2);
> +	read1_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET,
> +				    CONFIG_ENV_SIZE, tmp_env1);
> +	read2_fail = spi_flash_read(env_flash, CONFIG_ENV_OFFSET_REDUND,
> +				    CONFIG_ENV_SIZE, tmp_env2);
>  
>  	ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2,
>  				read2_fail);
> @@ -254,8 +220,8 @@ static int env_sf_save(void)
>  		if (!saved_buffer)
>  			goto done;
>  
> -		ret = spi_flash_read_env(env_flash, saved_offset,
> -					 saved_size, saved_buffer);
> +		ret = spi_flash_read(env_flash, saved_offset,
> +			saved_size, saved_buffer);
>  		if (ret)
>  			goto done;
>  	}
> @@ -311,10 +277,10 @@ static int env_sf_load(void)
>  	if (ret)
>  		goto out;
>  
> -	ret = spi_flash_read_env(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
> -				 buf);
> +	ret = spi_flash_read(env_flash,
> +		CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>  	if (ret) {
> -		set_default_env("spi_flash_read_env() failed", 0);
> +		set_default_env("spi_flash_read() failed", 0);
>  		goto err_read;
>  	}
>  
> -- 
> 2.17.2
> 

-- 
/Horatiu