[U-Boot] [PATCH 6/6] Support environment in NAND

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0808271748520.6718 at axis700.grange> you wrote:
>
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -44,6 +44,12 @@
>  #define	CMD_GETENV	"fw_printenv"
>  #define	CMD_SETENV	"fw_setenv"
>  
> +#define min(x, y) ({				\
> +	typeof(x) _min1 = (x);			\
> +	typeof(y) _min2 = (y);			\
> +	(void) (&_min1 == &_min2);		\

What does this do?

> +	_min1 < _min2 ? _min1 : _min2; })


>  typedef struct envdev_s {
>  	char devname[16];		/* Device name */
>  	ulong devoff;			/* Device offset */
> @@ -413,179 +419,290 @@ int fw_setenv (int argc, char *argv[])
>  	return 0;
>  }
>  
> +static int flash_bad_block (int dev, int fd, struct mtd_info_user *mtdinfo,
> +			    loff_t *blockstart, size_t blocklen)
> +{
> +	if (mtdinfo->type == MTD_NANDFLASH) {
> +		int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
> +
> +		if (badblock < 0) {
> +			perror ("Cannot read bad block mark");

It would be probably helpful to print the block address.

> +			return badblock;
> +		}
> +
> +		if (badblock) {
> +			fprintf (stderr, "Bad block at 0x%llx, "
> +				 "skipping\n", *blockstart);
> +			*blockstart += blocklen;
> +			return badblock;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * We are called with count == 0 for backing up as much data from the
> + * range as possible
> + */

Backing up?

>  static int flash_read_buf (int dev, int fd, void *buf, size_t count,
> -			   off_t offset)
> +			   off_t offset, size_t range)
>  {
> +	struct mtd_info_user mtdinfo;
> +	size_t blocklen, processed = 0;
> +	size_t readlen = count ? : range;
> +	off_t erase_offset, block_seek;
> +	loff_t blockstart;
>  	int rc;
> +	int backup_mode = !count;

backup_mode ?

I think there should be an explanation what exactly you are trying to
do.

> -	rc = lseek (fd, offset, SEEK_SET);
> -	if (rc == -1) {
> -		fprintf (stderr,
> -			 "seek error on %s: %s\n",
> -			 DEVNAME (dev), strerror (errno));
> +	if (!count)
> +		count = range;
> +
> +	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> +	if (rc < 0) {
> +		perror ("Cannot get MTD information");
>  		return rc;
>  	}

Did you verify that the code still builds when MTD_OLD is set?

>  
> -	rc = read (fd, buf, count);
> -	if (rc != count) {
> -		fprintf (stderr,
> -			 "Read error on %s: %s\n",
> -			 DEVNAME (dev), strerror (errno));
> -		return -1;
> +	/* Erase sector size is always a power of 2 */
> +	erase_offset = offset & ~(mtdinfo.erasesize - 1);

Please explain this logic.

> +	blockstart = erase_offset;
> +	/* Offset inside a block */
> +	block_seek = offset - erase_offset;
> +
> +	if (mtdinfo.type == MTD_NANDFLASH) {
> +		/*
> +		 * NAND: calculate which blocks we are reading. We have
> +		 * to read one block at a time to skip bad blocks.
> +		 */
> +		blocklen = mtdinfo.erasesize;
> +		/* Limit to one block for the first read */
> +		if (readlen > blocklen - block_seek)
> +			readlen = blocklen - block_seek;
> +	} else {
> +		blocklen = 0;
>  	}
>  
> -	return rc;
> +	/* This only runs once for NOR flash */
> +	while (processed < count) {
> +		rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart, blocklen);

But - NOR flash does not have bad block, so all of this is not needed
at all?

> +		if (rc < 0)
> +			return -1;
> +		else if (blockstart + block_seek + readlen > offset + range) {

I do not understand what you are doing here. Comment?

> +			/* End of range is reached */
> +			if (backup_mode) {
> +				return processed;
> +			} else {
> +				fprintf (stderr,
> +					 "Too few good blocks within range\n");
> +				return -1;
> +			}
> +		} else if (rc)
> +			continue;
> +
> +		/*
> +		 * If a block is bad, we retry in the next block
> +		 * at the same offset - see common/env_nand.c::
> +		 * writeenv()
> +		 */
> +		lseek (fd, blockstart + block_seek, SEEK_SET);

I don't see that you remember which blocks were bad. Does that mean
that you will attemopt to write the environment to known bad blocks?
Sonds not like a good idea to me.

> +		rc = read (fd, buf + processed, readlen);
> +		if (rc != readlen) {
> +			fprintf (stderr,
> +				 "Read error on %s: %s\n",
> +				 DEVNAME (dev), strerror (errno));
> +			return -1;
> +		}
> +		processed += readlen;
> +		readlen = min(blocklen, count - processed);
> +		block_seek = 0;
> +		blockstart += blocklen;
> +	}
> +
> +	return processed;
>  }
>  
> -static int flash_write (void)
> +static int flash_write_buf (int dev, int fd, void *buf, size_t count,
> +			    off_t offset)
>  {
> -	int fd_current, fd_target, rc, dev_target;
> -	erase_info_t erase_current = {}, erase_target;
>  	char *data = NULL;
> -	off_t erase_offset;
> -	struct mtd_info_user mtdinfo_target;
> +	erase_info_t erase;
> +	struct mtd_info_user mtdinfo;
> +	size_t blocklen, erase_len, processed = 0;
> +	size_t writelen, write_total = DEVESIZE (dev);
> +	off_t erase_offset, block_seek;
> +	loff_t blockstart;
> +	int rc;
>  
> -	/* dev_current: fd_current, erase_current */
> -	if ((fd_current = open (DEVNAME (dev_current), O_RDWR)) < 0) {
> -		fprintf (stderr,
> -			 "Can't open %s: %s\n",
> -			 DEVNAME (dev_current), strerror (errno));
> +	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> +	if (rc < 0) {
> +		perror ("Cannot get MTD information");
>  		return -1;
>  	}
>  
> -	if (HaveRedundEnv) {
> -		/* switch to next partition for writing */
> -		dev_target = !dev_current;
> -		/* dev_target: fd_target, erase_target */
> -		if ((fd_target = open (DEVNAME (dev_target), O_RDWR)) < 0) {
> -			fprintf (stderr,
> -				 "Can't open %s: %s\n",
> -				 DEVNAME (dev_target),
> -				 strerror (errno));
> -			return -1;
> -		}
> -	} else {
> -		dev_target = dev_current;
> -		fd_target = fd_current;
> -	}
> +	/* Erase sector size is always a power of 2 */
> +	erase_offset = offset & ~(mtdinfo.erasesize - 1);
> +	/* Maximum area we may use */
> +	erase_len = (offset - erase_offset + DEVESIZE (dev) +
> +		     mtdinfo.erasesize - 1) & ~(mtdinfo.erasesize - 1);
> +
> +	blockstart = erase_offset;
> +	/* Offset inside a block */
> +	block_seek = offset - erase_offset;
>  
>  	/*
>  	 * Support environment anywhere within erase sectors: read out the
>  	 * complete area to be erased, replace the environment image, write
>  	 * the whole block back again.
>  	 */
> -	if (DEVESIZE (dev_target) > CFG_ENV_SIZE) {
> -		data = malloc (DEVESIZE (dev_target));
> +	if (erase_len > DEVESIZE (dev)) {
> +		data = malloc (erase_len);
>  		if (!data) {
>  			fprintf (stderr,
> -				 "Cannot malloc %lu bytes: %s\n",
> -				 DEVESIZE (dev_target),
> -				 strerror (errno));
> +				 "Cannot malloc %u bytes: %s\n",
> +				 erase_len, strerror (errno));
>  			return -1;
>  		}
>  
> -		rc = ioctl (fd_target, MEMGETINFO, &mtdinfo_target);
> -		if (rc < 0) {
> -			perror ("Cannot get MTD information");
> +		/*
> +		 * This is different from a normal read. We have to read as much
> +		 * as we can from a certain area, and it should be at least X
> +		 * bytes, instead of having to read a fixed number of bytes as
> +		 * usual. This also tells us how much data "fits" in the good
> +		 * blocks in the area.
> +		 */
> +		write_total = flash_read_buf (dev, fd, data, 0,
> +					      erase_offset, erase_len);
> +		if (write_total < block_seek + CFG_ENV_SIZE)

Ummm...this is flash_write_buf(), and we start reading data?

Please explain your code.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It would seem that evil retreats when forcibly confronted
	-- Yarnek of Excalbia, "The Savage Curtain", stardate 5906.5