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

login
register
mail settings
Submitter Phil Sutter
Date June 26, 2013, 6:25 p.m.
Message ID <1372271126-2642-3-git-send-email-phil.sutter@viprinet.com>
Download mbox | patch
Permalink /patch/254835/
State Superseded
Delegated to: Scott Wood
Headers show

Comments

Phil Sutter - June 26, 2013, 6:25 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 | 105 ++++++++++++++++++++++++------------------------------
 1 file changed, 46 insertions(+), 59 deletions(-)
Phil Sutter - July 19, 2013, 10:09 a.m.
Hi,

On Wed, Jul 17, 2013 at 05:25:31PM -0500, Scott Wood wrote:
> On 06/26/2013 01:25:26 PM, 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 | 105  
> > ++++++++++++++++++++++++------------------------------
> >  1 file changed, 46 insertions(+), 59 deletions(-)
> 
> Missing description of changes since v2
> 
> > -#else /* ! CONFIG_ENV_OFFSET_REDUND */
> > +
> > +static unsigned char env_flags;
> 
> env_nand.c:193:22: warning: 'env_flags' defined but not used
> [-Wunused-variable]
> 
> (when CONFIG_ENV_OFFSET_REDUND is not defined)
> 
> >  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;
> > +	static const struct env_location 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
> > +	};
> > +
> 
> env_nand.c:206:4: error: initializer element is not constant
> env_nand.c:206:4: error: (near initialization for  
> 'location[0].erase_opts')
> 
> You could make nand_erase_options static, or you could use code to  
> assign
> that field.
> 
> Is this code untested, or did you accidentally send an old version?

Yes, indeed. My apologies for not having tested this, seems like a
combination of too much belief in your builtin parser and failure of
mine.

Corrected version which incorporates your suggestions and tries to solve
the issue above in a cleaner way follows. And yes, this time it has even
been tested.

Greetings, Phil

Patch

diff --git a/common/env_nand.c b/common/env_nand.c
index b745822..06af5ce 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -168,72 +168,56 @@  int writeenv(size_t offset, u_char *buf)
 	return 0;
 }
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-static unsigned char env_flags;
+struct env_location {
+	const char *name;
+	nand_erase_options_t *erase_opts;
+	loff_t offset;
+};
 
-int saveenv(void)
+static int erase_and_write_env(struct env_location *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;
-
-	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
-		return 1;
+	int ret = 0;
 
-	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);
+	printf("Erasing %s...\n", location->name);
+	location->erase_opts->offset = location->offset;
+	if (nand_erase_opts(&nand_info[0], location->erase_opts))
 		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) {
-		puts("FAILED!\n");
-		return 1;
-	}
-
-	puts("done\n");
-
-	gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+	printf("Writing to %s... ", location->name);
+	ret = writeenv(location->offset, env_new);
+	puts(ret ? "FAILED!\n" : "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;
+	static const struct env_location 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 +228,25 @@  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)