From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Wed, 17 Jul 2013 17:25:31 -0500
Subject: [U-Boot] [PATCH v3 2/2] env_nand.c: support falling back to
 redundant env when writing
In-Reply-To: <1372271126-2642-3-git-send-email-phil.sutter@viprinet.com>
	(from phil.sutter@viprinet.com on Wed Jun 26 13:25:26 2013)
Message-ID: <1374099931.8183.370@snotra>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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?

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

env_nand.c:237:2: warning: passing argument 1 of 'erase_and_write_env'
discards 'const' qualifier from pointer target type [enabled by default]
env_nand.c:177:12: note: expected 'struct env_location *' but argument  
is
of type 'const struct env_location *'

> +#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);

Can you print a message here specifically saying that redundancy has  
been
lost?  I realize that the previous erase_and_write_env will have printed
"FAILED", but it'd be nice to be explicit about the consequences.

-Scott