[U-Boot] env_flash.c:saveenv() broken when env is smaller than a sector

Mike Frysinger vapier at gentoo.org
Thu Dec 30 02:53:17 CET 2010


On Saturday, July 17, 2010 15:45:48 Wolfgang Denk wrote:
> --- a/common/env_flash.c
> +++ b/common/env_flash.c
>  #ifdef CMD_SAVEENV
>  int saveenv(void)
>  {
> -	char *saved_data = NULL;
> -	int rc = 1;
> -	char flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG;
> +	env_t	env_new;
> +	ssize_t	len;
> +	char	*saved_data = NULL;
> +	char	*res;
> +	int	rc = 1;
> +	char	flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG;
>  #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> -	ulong up_data = 0;
> +	ulong	up_data = 0;
>  #endif
> 
> -	debug ("Protect off %08lX ... %08lX\n",
> +	debug("Protect off %08lX ... %08lX\n",
>  		(ulong)flash_addr, end_addr);
> 
> -	if (flash_sect_protect (0, (ulong)flash_addr, end_addr)) {
> -		goto Done;
> +	if (flash_sect_protect(0, (ulong)flash_addr, end_addr)) {
> +		goto done;
>  	}
> 
> -	debug ("Protect off %08lX ... %08lX\n",
> +	debug("Protect off %08lX ... %08lX\n",
>  		(ulong)flash_addr_new, end_addr_new);
> 
> -	if (flash_sect_protect (0, (ulong)flash_addr_new, end_addr_new)) {
> -		goto Done;
> +	if (flash_sect_protect(0, (ulong)flash_addr_new, end_addr_new)) {
> +		goto done;
> +	}
> +
> +	res = (char *)&env_new.data;
> +	len = hexport('\0', &res, ENV_SIZE);
> +	if (len < 0) {
> +		error("Cannot export environment: errno = %d\n", errno);
> +		goto done;
>  	}
> +	env_new.crc   = crc32(0, env_new.data, ENV_SIZE);
> +	env_new.flags = new_flag;
> 
>  #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
>  	up_data = (end_addr_new + 1 - ((long)flash_addr_new + CONFIG_ENV_SIZE));
> -	debug ("Data to save 0x%x\n", up_data);
> +	debug("Data to save 0x%lX\n", up_data);
>  	if (up_data) {
>  		if ((saved_data = malloc(up_data)) == NULL) {
>  			printf("Unable to save the rest of sector (%ld)\n",
>  				up_data);
> -			goto Done;
> +			goto done;
>  		}
>  		memcpy(saved_data,
>  			(void *)((long)flash_addr_new + CONFIG_ENV_SIZE), up_data);
> -		debug ("Data (start 0x%x, len 0x%x) saved at 0x%x\n",
> -			   (long)flash_addr_new + CONFIG_ENV_SIZE,
> -				up_data, saved_data);
> +		debug("Data (start 0x%lX, len 0x%lX) saved at 0x%p\n",
> +			(long)flash_addr_new + CONFIG_ENV_SIZE,
> +			up_data, saved_data);
>  	}
>  #endif
> -	puts ("Erasing Flash...");
> -	debug (" %08lX ... %08lX ...",
> +	puts("Erasing Flash...");
> +	debug(" %08lX ... %08lX ...",
>  		(ulong)flash_addr_new, end_addr_new);
> 
> -	if (flash_sect_erase ((ulong)flash_addr_new, end_addr_new)) {
> -		goto Done;
> +	if (flash_sect_erase((ulong)flash_addr_new, end_addr_new)) {
> +		goto done;
>  	}
> 
> -	puts ("Writing to Flash... ");
> -	debug (" %08lX ... %08lX ...",
> +	puts("Writing to Flash... ");
> +	debug(" %08lX ... %08lX ...",
>  		(ulong)&(flash_addr_new->data),
>  		sizeof(env_ptr->data)+(ulong)&(flash_addr_new->data));
> -	if ((rc = flash_write((char *)env_ptr->data,
> -			(ulong)&(flash_addr_new->data),
> -			sizeof(env_ptr->data))) ||
> -	    (rc = flash_write((char *)&(env_ptr->crc),
> -			(ulong)&(flash_addr_new->crc),
> -			sizeof(env_ptr->crc))) ||
> +	if ((rc = flash_write((char *)&env_new,
> +			(ulong)flash_addr_new,
> +			sizeof(env_new))) ||
>  	    (rc = flash_write(&flag,
>  			(ulong)&(flash_addr->flags),
> -			sizeof(flash_addr->flags))) ||
> -	    (rc = flash_write(&new_flag,
> -			(ulong)&(flash_addr_new->flags),
> -			sizeof(flash_addr_new->flags))))
> -	{
> -		flash_perror (rc);
> -		goto Done;
> +			sizeof(flash_addr->flags))) ) {
> +		flash_perror(rc);
> +		goto done;
>  	}
> -	puts ("done\n");
> 
>  #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
>  	if (up_data) { /* restore the rest of sector */
> -		debug ("Restoring the rest of data to 0x%x len 0x%x\n",
> -			   (long)flash_addr_new + CONFIG_ENV_SIZE, up_data);
> +		debug("Restoring the rest of data to 0x%lX len 0x%lX\n",
> +			(long)flash_addr_new + CONFIG_ENV_SIZE, up_data);
>  		if (flash_write(saved_data,
>  				(long)flash_addr_new + CONFIG_ENV_SIZE,
>  				up_data)) {
>  			flash_perror(rc);
> -			goto Done;
> +			goto done;
>  		}
>  	}
>  #endif
> +	puts("done\n");
> +
>  	{
>  		env_t * etmp = flash_addr;
>  		ulong ltmp = end_addr;
> @@ -220,13 +230,12 @@ int saveenv(void)
>  	}
> 
>  	rc = 0;
> -Done:
> -
> +done:
>  	if (saved_data)
> -		free (saved_data);
> +		free(saved_data);
>  	/* try to re-protect */
> -	(void) flash_sect_protect (1, (ulong)flash_addr, end_addr);
> -	(void) flash_sect_protect (1, (ulong)flash_addr_new, end_addr_new);
> +	(void) flash_sect_protect(1, (ulong)flash_addr, end_addr);
> +	(void) flash_sect_protect(1, (ulong)flash_addr_new, end_addr_new);
> 
>  	return rc;
>  }
> @@ -244,83 +253,93 @@ int  env_init(void)
> 
>  	gd->env_addr  = (ulong)&default_environment[0];
>  	gd->env_valid = 0;
> -	return (0);
> +	return 0;
>  }
> 
>  #ifdef CMD_SAVEENV
> 
>  int saveenv(void)
>  {
> -	int	len, rc;
> -	ulong	end_addr;
> -	ulong	flash_sect_addr;
> -#if defined(CONFIG_ENV_SECT_SIZE) && (CONFIG_ENV_SECT_SIZE >
> CONFIG_ENV_SIZE) -	ulong	flash_offset;
> -	uchar	env_buffer[CONFIG_ENV_SECT_SIZE];
> -#else
> -	uchar *env_buffer = (uchar *)env_ptr;
> -#endif	/* CONFIG_ENV_SECT_SIZE */
> -	int rcode = 0;
> -
> -#if defined(CONFIG_ENV_SECT_SIZE) && (CONFIG_ENV_SECT_SIZE >
> CONFIG_ENV_SIZE) -
> -	flash_offset    = ((ulong)flash_addr) & (CONFIG_ENV_SECT_SIZE-1);
> -	flash_sect_addr = ((ulong)flash_addr) & ~(CONFIG_ENV_SECT_SIZE-1);
> -
> -	debug ( "copy old content: "
> -		"sect_addr: %08lX  env_addr: %08lX  offset: %08lX\n",
> -		flash_sect_addr, (ulong)flash_addr, flash_offset);
> -
> -	/* copy old contents to temporary buffer */
> -	memcpy (env_buffer, (void *)flash_sect_addr, CONFIG_ENV_SECT_SIZE);
> -
> -	/* copy current environment to temporary buffer */
> -	memcpy ((uchar *)((unsigned long)env_buffer + flash_offset),
> -		env_ptr,
> -		CONFIG_ENV_SIZE);
> +	env_t	env_new;
> +	ssize_t	len;
> +	int	rc = 1;
> +	char	*res;
> +	char	*saved_data = NULL;
> +#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> +	ulong	up_data = 0;
> 
> -	len	 = CONFIG_ENV_SECT_SIZE;
> -#else
> -	flash_sect_addr = (ulong)flash_addr;
> -	len	 = CONFIG_ENV_SIZE;
> +	up_data = (end_addr + 1 - ((long)flash_addr + CONFIG_ENV_SIZE));
> +	debug("Data to save 0x%lx\n", up_data);
> +	if (up_data) {
> +		if ((saved_data = malloc(up_data)) == NULL) {
> +			printf("Unable to save the rest of sector (%ld)\n",
> +				up_data);
> +			goto done;
> +		}
> +		memcpy(saved_data,
> +			(void *)((long)flash_addr + CONFIG_ENV_SIZE), up_data);
> +		debug("Data (start 0x%lx, len 0x%lx) saved at 0x%lx\n",
> +			(ulong)flash_addr + CONFIG_ENV_SIZE,
> +			up_data,
> +			(ulong)saved_data);
> +	}
>  #endif	/* CONFIG_ENV_SECT_SIZE */
> 
> -	end_addr = flash_sect_addr + len - 1;
> +	debug("Protect off %08lX ... %08lX\n",
> +		(ulong)flash_addr, end_addr);
> 
> -	debug ("Protect off %08lX ... %08lX\n",
> -		(ulong)flash_sect_addr, end_addr);
> +	if (flash_sect_protect(0, (long)flash_addr, end_addr))
> +		goto done;
> 
> -	if (flash_sect_protect (0, flash_sect_addr, end_addr))
> -		return 1;
> +	res = (char *)&env_new.data;
> +	len = hexport('\0', &res, ENV_SIZE);
> +	if (len < 0) {
> +		error("Cannot export environment: errno = %d\n", errno);
> +		goto done;
> +	}
> +	env_new.crc = crc32(0, env_new.data, ENV_SIZE);
> 
> -	puts ("Erasing Flash...");
> -	if (flash_sect_erase (flash_sect_addr, end_addr))
> -		return 1;
> +	puts("Erasing Flash...");
> +	if (flash_sect_erase((long)flash_addr, end_addr))
> +		goto done;
> 
> -	puts ("Writing to Flash... ");
> -	rc = flash_write((char *)env_buffer, flash_sect_addr, len);
> +	puts("Writing to Flash... ");
> +	rc = flash_write((char *)&env_new, (long)flash_addr, CONFIG_ENV_SIZE);
>  	if (rc != 0) {
> -		flash_perror (rc);
> -		rcode = 1;
> -	} else {
> -		puts ("done\n");
> +		flash_perror(rc);
> +		goto done;
>  	}
> -
> +#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> +	if (up_data) {	/* restore the rest of sector */
> +		debug("Restoring the rest of data to 0x%lx len 0x%lx\n",
> +			(ulong)flash_addr + CONFIG_ENV_SIZE, up_data);
> +		if (flash_write(saved_data,
> +				(long)flash_addr + CONFIG_ENV_SIZE,
> +				up_data)) {
> +			flash_perror(rc);
> +			goto done;
> +		}
> +	}
> +#endif
> +	puts("done\n");
> +	rc = 0;
> +done:
> +	if (saved_data)
> +		free(saved_data);
>  	/* try to re-protect */
> -	(void) flash_sect_protect (1, flash_sect_addr, end_addr);
> -	return rcode;
> +	(void) flash_sect_protect(1, (long)flash_addr, end_addr);
> +	return rc;
>  }

it would seem that somewhere nestled in this rewrite, support for embedded env 
was broken (CONFIG_ENV_SIZE < CONFIG_ENV_SECT_SIZE).  on a bf548-ezkit for 
example, i see this behavior:

(v2010.09 / before this commit):
bfin> save
Saving Environment to Flash...
. done
Un-Protected 1 sectors
Erasing Flash...
. done
Erased 1 sectors
Writing to Flash... done
. done
Protected 1 sectors

(after this commit / v2010.12):
bfin> save
Saving Environment to Flash...
Error: start address not on sector boundary
Error: start address not on sector boundary

not sure if anyone has noticed/fixed this yet and would save me from the 
trouble of finding/fixing the problem ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101229/14330289/attachment.pgp 


More information about the U-Boot mailing list