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

On 13/03/19 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@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@denx.de

Acked-by: Stefano Babic sbabic@denx.de

In fact, the commit breaks the API between U-Boot and Linux and optimizes a specific use case in U-Boot only. The API foresees that there is an environment of a specific size without bothering (in linux) where it is stored. It is strictly required that the environment has the same format for all underlying storage - it must be possible to simply copy the environment. Commit breaks tools like mkenvimage, too.

Best regards, Stefano Babic

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; }