diff mbox series

[V3,2/2] fw_env: simplify logic & code paths in the fw_env_open()

Message ID 20220112114706.12269-3-zajec5@gmail.com
State Accepted
Commit 07c79dd5fdeaefb39c9e7a97f3b66de63109a18d
Delegated to: Tom Rini
Headers show
Series fw_env: two minor code cleanups | expand

Commit Message

Rafał Miłecki Jan. 12, 2022, 11:47 a.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>

Environment variables can be stored in two formats:
1. Single entry with header containing CRC32
2. Two entries with extra flags field in each entry header

For that reason fw_env_open() has two main code paths and there are
pointers for CRC32/flags/data.

Previous implementation was a bit hard to follow:
1. It was checking for used format twice (in reversed order each time)
2. It was setting "environment" global struct fields to some temporary
   values that required extra comments explaining it

This change simplifies that code:
1. It introduces two clear code paths
2. It sets "environment" global struct fields values only once it really
   knows them

To be fair there are *two* crc32() calls now and an extra pointer
variable but that should be cheap enough and worth it.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Add missing "environment.image = addr0;"
---
 tools/env/fw_env.c | 77 +++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 46 deletions(-)

Comments

Tom Rini Feb. 11, 2022, 5:06 p.m. UTC | #1 
On Wed, Jan 12, 2022 at 12:47:06PM +0100, Rafał Miłecki wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Environment variables can be stored in two formats:
> 1. Single entry with header containing CRC32
> 2. Two entries with extra flags field in each entry header
> 
> For that reason fw_env_open() has two main code paths and there are
> pointers for CRC32/flags/data.
> 
> Previous implementation was a bit hard to follow:
> 1. It was checking for used format twice (in reversed order each time)
> 2. It was setting "environment" global struct fields to some temporary
>    values that required extra comments explaining it
> 
> This change simplifies that code:
> 1. It introduces two clear code paths
> 2. It sets "environment" global struct fields values only once it really
>    knows them
> 
> To be fair there are *two* crc32() calls now and an extra pointer
> variable but that should be cheap enough and worth it.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index a738b910c2..31afef6f3b 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1421,9 +1421,6 @@  int fw_env_open(struct env_opts *opts)
 
 	int ret;
 
-	struct env_image_single *single;
-	struct env_image_redundant *redundant;
-
 	if (!opts)
 		opts = &default_opts;
 
@@ -1439,40 +1436,37 @@  int fw_env_open(struct env_opts *opts)
 		goto open_cleanup;
 	}
 
-	/* read environment from FLASH to local buffer */
-	environment.image = addr0;
-
-	if (have_redund_env) {
-		redundant = addr0;
-		environment.crc = &redundant->crc;
-		environment.flags = &redundant->flags;
-		environment.data = redundant->data;
-	} else {
-		single = addr0;
-		environment.crc = &single->crc;
-		environment.flags = NULL;
-		environment.data = single->data;
-	}
-
 	dev_current = 0;
-	if (flash_io(O_RDONLY, environment.image, CUR_ENVSIZE)) {
+	if (flash_io(O_RDONLY, addr0, CUR_ENVSIZE)) {
 		ret = -EIO;
 		goto open_cleanup;
 	}
 
-	crc0 = crc32(0, (uint8_t *)environment.data, ENV_SIZE);
-
-	crc0_ok = (crc0 == *environment.crc);
 	if (!have_redund_env) {
+		struct env_image_single *single = addr0;
+
+		crc0 = crc32(0, (uint8_t *)single->data, ENV_SIZE);
+		crc0_ok = (crc0 == single->crc);
 		if (!crc0_ok) {
 			fprintf(stderr,
 				"Warning: Bad CRC, using default environment\n");
-			memcpy(environment.data, default_environment,
+			memcpy(single->data, default_environment,
 			       sizeof(default_environment));
 			environment.dirty = 1;
 		}
+
+		environment.image = addr0;
+		environment.crc = &single->crc;
+		environment.flags = NULL;
+		environment.data = single->data;
 	} else {
-		flag0 = *environment.flags;
+		struct env_image_redundant *redundant0 = addr0;
+		struct env_image_redundant *redundant1;
+
+		crc0 = crc32(0, (uint8_t *)redundant0->data, ENV_SIZE);
+		crc0_ok = (crc0 == redundant0->crc);
+
+		flag0 = redundant0->flags;
 
 		dev_current = 1;
 		addr1 = calloc(1, CUR_ENVSIZE);
@@ -1483,14 +1477,9 @@  int fw_env_open(struct env_opts *opts)
 			ret = -ENOMEM;
 			goto open_cleanup;
 		}
-		redundant = addr1;
+		redundant1 = addr1;
 
-		/*
-		 * have to set environment.image for flash_read(), careful -
-		 * other pointers in environment still point inside addr0
-		 */
-		environment.image = addr1;
-		if (flash_io(O_RDONLY, environment.image, CUR_ENVSIZE)) {
+		if (flash_io(O_RDONLY, addr1, CUR_ENVSIZE)) {
 			ret = -EIO;
 			goto open_cleanup;
 		}
@@ -1518,18 +1507,12 @@  int fw_env_open(struct env_opts *opts)
 			goto open_cleanup;
 		}
 
-		crc1 = crc32(0, (uint8_t *)redundant->data, ENV_SIZE);
+		crc1 = crc32(0, (uint8_t *)redundant1->data, ENV_SIZE);
 
-		crc1_ok = (crc1 == redundant->crc);
-		flag1 = redundant->flags;
+		crc1_ok = (crc1 == redundant1->crc);
+		flag1 = redundant1->flags;
 
-		/*
-		 * environment.data still points to ((struct
-		 * env_image_redundant *)addr0)->data. If the two
-		 * environments differ, or one has bad crc, force a
-		 * write-out by marking the environment dirty.
-		 */
-		if (memcmp(environment.data, redundant->data, ENV_SIZE) ||
+		if (memcmp(redundant0->data, redundant1->data, ENV_SIZE) ||
 		    !crc0_ok || !crc1_ok)
 			environment.dirty = 1;
 
@@ -1540,7 +1523,7 @@  int fw_env_open(struct env_opts *opts)
 		} else if (!crc0_ok && !crc1_ok) {
 			fprintf(stderr,
 				"Warning: Bad CRC, using default environment\n");
-			memcpy(environment.data, default_environment,
+			memcpy(redundant0->data, default_environment,
 			       sizeof(default_environment));
 			environment.dirty = 1;
 			dev_current = 0;
@@ -1586,13 +1569,15 @@  int fw_env_open(struct env_opts *opts)
 		 */
 		if (dev_current) {
 			environment.image = addr1;
-			environment.crc = &redundant->crc;
-			environment.flags = &redundant->flags;
-			environment.data = redundant->data;
+			environment.crc = &redundant1->crc;
+			environment.flags = &redundant1->flags;
+			environment.data = redundant1->data;
 			free(addr0);
 		} else {
 			environment.image = addr0;
-			/* Other pointers are already set */
+			environment.crc = &redundant0->crc;
+			environment.flags = &redundant0->flags;
+			environment.data = redundant0->data;
 			free(addr1);
 		}
 #ifdef DEBUG