Patchwork [U-Boot,PATCHv2,2/4] env_nand.c: support falling back to redundant env when writing

login
register
mail settings
Submitter Phil Sutter
Date Feb. 21, 2013, 5:21 p.m.
Message ID <1361467316-29044-3-git-send-email-phil.sutter@viprinet.com>
Download mbox | patch
Permalink /patch/222379/
State Changes Requested
Delegated to: Scott Wood
Headers show

Comments

Phil Sutter - Feb. 21, 2013, 5:21 p.m.
Without this patch, when the currently chosen environment to be written
has bad blocks, saveenv fails completely. Instead, when there is
redundant environment fall back to the other copy. Environment reading
needs no adjustment, as the fallback logic for incomplete writes applies
to this case as well.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c |  103 ++++++++++++++++++++++------------------------------
 1 files changed, 44 insertions(+), 59 deletions(-)
Scott Wood - Feb. 23, 2013, 1:32 a.m.
On Thu, Feb 21, 2013 at 06:21:54PM +0100, Phil Sutter wrote:
> Without this patch, when the currently chosen environment to be written
> has bad blocks, saveenv fails completely. Instead, when there is
> redundant environment fall back to the other copy. Environment reading
> needs no adjustment, as the fallback logic for incomplete writes applies
> to this case as well.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> ---
>  common/env_nand.c |  103 ++++++++++++++++++++++------------------------------
>  1 files changed, 44 insertions(+), 59 deletions(-)
> 
> diff --git a/common/env_nand.c b/common/env_nand.c
> index 22e72a2..c0c985c 100644
> --- a/common/env_nand.c
> +++ b/common/env_nand.c
> @@ -168,72 +168,55 @@ int writeenv(size_t offset, u_char *buf)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_ENV_OFFSET_REDUND
> -static unsigned char env_flags;
> +typedef struct env_location {
> +	const char *name;
> +	nand_erase_options_t *erase_opts;
> +	loff_t offset;
> +} env_location_t;

WARNING: do not add new typedefs
#286: FILE: common/env_nand.c:171:
+typedef struct env_location {

> +	printf("Writing to %s... ", location->name);
> +	if ((ret = writeenv(location->offset, env_new)))
>  		puts("FAILED!\n");
> -		return 1;
> -	}

ERROR: do not use assignment in if condition
#339: FILE: common/env_nand.c:187:
+	if ((ret = writeenv(location->offset, env_new)))


> -
> -	puts("done\n");
> -
> -	gd->env_valid = gd->env_valid == 2 ? 1 : 2;
> +	else
> +		puts("OK\n");
>  
>  	return ret;
>  }
> -#else /* ! CONFIG_ENV_OFFSET_REDUND */
> +
> +static unsigned char env_flags;
> +
>  int saveenv(void)
>  {
>  	int	ret = 0;
>  	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
>  	ssize_t	len;
>  	char	*res;
> +	int	env_idx;
>  	nand_erase_options_t nand_erase_options;
> +	env_location_t location[] = {{
> +		.name = "NAND",
> +		.erase_opts = &nand_erase_options,
> +		.offset = CONFIG_ENV_OFFSET,
> +#ifdef CONFIG_ENV_OFFSET_REDUND
> +	}, {
> +		.name = "redundant NAND",
> +		.erase_opts = &nand_erase_options,
> +		.offset = CONFIG_ENV_OFFSET_REDUND,
> +#endif
> +	}};
> +

ERROR: space required after that close brace '}'
#374: FILE: common/env_nand.c:215:
+	}};

...or rather, it should be like this:

	static const struct env_location location[] = {
		{
			.name = "NAND";
			...
		},
#ifdef CONFIG_ENV_OFFSET_REDUND
		{
			.name = "redundant NAND",
			...
		},
#endif
	};

Note that without the "static" GCC will emit code to dynamically
initialize the array, which will be larger.

> +#ifdef CONFIG_ENV_OFFSET_REDUND
> +	if (ret) {
> +		env_idx = (env_idx + 1) & 1;
> +		ret = erase_and_write_env(&location[env_idx],
> +				(u_char *)env_new);
> +	} else
> +		gd->env_valid = gd->env_valid == 2 ? 1 : 2;

Braces around both halves of the if/else if one side has them.

-Scott

Patch

diff --git a/common/env_nand.c b/common/env_nand.c
index 22e72a2..c0c985c 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -168,72 +168,55 @@  int writeenv(size_t offset, u_char *buf)
 	return 0;
 }
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-static unsigned char env_flags;
+typedef struct env_location {
+	const char *name;
+	nand_erase_options_t *erase_opts;
+	loff_t offset;
+} env_location_t;
 
-int saveenv(void)
+static int erase_and_write_env(env_location_t *location, u_char *env_new)
 {
-	env_t	env_new;
-	ssize_t	len;
-	char	*res;
-	int	ret = 0;
-	nand_erase_options_t nand_erase_options;
-
-	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
-	nand_erase_options.length = CONFIG_ENV_RANGE;
+	int ret = 0;
 
-	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
+	printf("Erasing %s...\n", location->name);
+	location->erase_opts->offset = location->offset;
+	if (nand_erase_opts(&nand_info[0], location->erase_opts))
 		return 1;
 
-	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
-	if (len < 0) {
-		error("Cannot export environment: errno = %d\n", errno);
-		return 1;
-	}
-	env_new.crc	= crc32(0, env_new.data, ENV_SIZE);
-	env_new.flags	= ++env_flags; /* increase the serial */
-
-	if (gd->env_valid == 1) {
-		puts("Erasing redundant NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET_REDUND;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to redundant NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)&env_new);
-	} else {
-		puts("Erasing NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET, (u_char *)&env_new);
-	}
-	if (ret) {
+	printf("Writing to %s... ", location->name);
+	if ((ret = writeenv(location->offset, env_new)))
 		puts("FAILED!\n");
-		return 1;
-	}
-
-	puts("done\n");
-
-	gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+	else
+		puts("OK\n");
 
 	return ret;
 }
-#else /* ! CONFIG_ENV_OFFSET_REDUND */
+
+static unsigned char env_flags;
+
 int saveenv(void)
 {
 	int	ret = 0;
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	ssize_t	len;
 	char	*res;
+	int	env_idx;
 	nand_erase_options_t nand_erase_options;
+	env_location_t location[] = {{
+		.name = "NAND",
+		.erase_opts = &nand_erase_options,
+		.offset = CONFIG_ENV_OFFSET,
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	}, {
+		.name = "redundant NAND",
+		.erase_opts = &nand_erase_options,
+		.offset = CONFIG_ENV_OFFSET_REDUND,
+#endif
+	}};
+
 
 	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
 	nand_erase_options.length = CONFIG_ENV_RANGE;
-	nand_erase_options.offset = CONFIG_ENV_OFFSET;
 
 	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
 		return 1;
@@ -244,22 +227,24 @@  int saveenv(void)
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;
 	}
-	env_new->crc = crc32(0, env_new->data, ENV_SIZE);
-
-	puts("Erasing Nand...\n");
-	if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-		return 1;
+	env_new->crc   = crc32(0, env_new->data, ENV_SIZE);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	env_new->flags = ++env_flags; /* increase the serial */
+	env_idx = (gd->env_valid == 1);
+#endif
 
-	puts("Writing to Nand... ");
-	if (writeenv(CONFIG_ENV_OFFSET, (u_char *)env_new)) {
-		puts("FAILED!\n");
-		return 1;
-	}
+	ret = erase_and_write_env(&location[env_idx], (u_char *)env_new);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	if (ret) {
+		env_idx = (env_idx + 1) & 1;
+		ret = erase_and_write_env(&location[env_idx],
+				(u_char *)env_new);
+	} else
+		gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+#endif
 
-	puts("done\n");
 	return ret;
 }
-#endif /* CONFIG_ENV_OFFSET_REDUND */
 #endif /* CMD_SAVEENV */
 
 int readenv(size_t offset, u_char *buf)