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

[Mike Frysinger <vapier@gentoo.org>]

On Wednesday, December 29, 2010 20:53:17 Mike Frysinger wrote:
> On Saturday, July 17, 2010 15:45:48 Wolfgang Denk wrote:
> > --- a/common/env_flash.c
> > +++ b/common/env_flash.c
> >  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 */
> 
> 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 ...

seems the new saveenv code in the non-redund case has been rewritten 
completely and doesn't support the same feature set as it used to.

old behavior:
 - load up the entire sector
 - overlay new env into sector
 - round env start addr down to sector start
 - round env end addr up to sector end
 - protect/erase/save/protect whole sector

new behavior:
 - load up the sector data after the env
 - overlay new env into sector
 - set env end addr to start of env plus one sector
 - protect/erase/save/protect whole sector

so the difference is that the new code no longer supports envs which do not 
start on a sector boundary.  it does this so that it doesn't memcpy() as much 
data out of the flash at the expense of doing more CPU bound math operations 
(mostly bit twiddling).

so the question is whether the old behavior should be restored.  if it not, 
the documentation should be tweaked to note these requirements and some simple 
CPP checks added to environment.h so that this issue turns into a build 
failure.  after all, it's easy to detect an env offset that isnt at the start 
of the sector:
	#if (CONFIG_ENV_OFFSET & (CONFIG_ENV_SECT_SIZE - 1))
	# error env offset not sector aligned
	#endif
-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/e28b1c0f/attachment.pgp