[U-Boot] [PATCH] fw_env: add NAND support

[U-Boot] [PATCH] fw_env: add NAND support

[Guennadi Liakhovetski]

Add support for environment in NAND with automatic NOR / NAND recognition, 
including unaligned environment, bad-block skipping, redundant environment 
copy.

Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
---

This is now a single patch. No union any more, instead a struct with 
pointers into a flat image buffer is used. Still, MTD_VERSION=old doesn't 
work, it looks it is also broken in the current version.

diff --git a/tools/env/README b/tools/env/README
index f8a644e..f32f872 100644
--- a/tools/env/README
+++ b/tools/env/README
@@ -22,9 +22,11 @@ following lines are relevant:
 #define DEVICE1_OFFSET    0x0000
 #define ENV1_SIZE         0x4000
 #define DEVICE1_ESIZE     0x4000
+#define DEVICE1_ENVSECTORS     2
 #define DEVICE2_OFFSET    0x0000
 #define ENV2_SIZE         0x4000
 #define DEVICE2_ESIZE     0x4000
+#define DEVICE2_ENVSECTORS     2
 
 Current configuration matches the environment layout of the TRAB
 board.
@@ -46,3 +48,7 @@ then 1 sector.
 
 DEVICEx_ESIZE defines the size of the first sector in the flash
 partition where the environment resides.
+
+DEVICEx_ENVSECTORS defines the number of sectors that may be used for
+this environment instance. On NAND this is used to limit the range
+within which bad blocks are skipped, on NOR it is unused.
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index b8bca91..7479dbe 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -44,36 +44,57 @@
 #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);		\
+	_min1 < _min2 ? _min1 : _min2; })
+
 typedef struct envdev_s {
 	char devname[16];		/* Device name */
 	ulong devoff;			/* Device offset */
 	ulong env_size;			/* environment size */
 	ulong erase_size;		/* device erase size */
+	ulong env_sectors;		/* number of environment sectors */
 } envdev_t;
 
 static envdev_t envdevices[2];
-static int curdev;
+static int dev_current;
 
 #define DEVNAME(i)    envdevices[(i)].devname
 #define DEVOFFSET(i)  envdevices[(i)].devoff
 #define ENVSIZE(i)    envdevices[(i)].env_size
 #define DEVESIZE(i)   envdevices[(i)].erase_size
+#define ENVSECTORS(i) envdevices[(i)].env_sectors
 
-#define CFG_ENV_SIZE ENVSIZE(curdev)
+#define CFG_ENV_SIZE ENVSIZE(dev_current)
 
 #define ENV_SIZE      getenvsize()
 
-typedef struct environment_s {
-	ulong crc;			/* CRC32 over data bytes    */
-	unsigned char flags;		/* active or obsolete */
-	char *data;
-} env_t;
+struct env_image_single {
+	uint32_t	crc;	/* CRC32 over data bytes    */
+	char		data[];
+};
+
+struct env_image_redundant {
+	uint32_t	crc;	/* CRC32 over data bytes    */
+	unsigned char	flags;	/* active or obsolete */
+	char		data[];
+};
+
+struct environment {
+	void		*image;
+	uint32_t	*crc;
+	unsigned char	*flags;
+	char		*data;
+};
 
-static env_t environment;
+static struct environment environment;
 
 static int HaveRedundEnv = 0;
 
 static unsigned char active_flag = 1;
+/* obsolete_flag must be 0 to efficiently set it on NOR flash without erasing */
 static unsigned char obsolete_flag = 0;
 
 
@@ -156,10 +177,11 @@ static char default_environment[] = {
 #ifdef  CONFIG_EXTRA_ENV_SETTINGS
 	CONFIG_EXTRA_ENV_SETTINGS
 #endif
-	"\0"			/* Termimate env_t data with 2 NULs */
+	"\0"			/* Termimate struct environment data with 2 NULs */
 };
 
-static int flash_io (int mode);
+static int flash_write (void);
+static int flash_read (void);
 static char *envmatch (char * s1, char * s2);
 static int env_init (void);
 static int parse_config (void);
@@ -185,7 +207,7 @@ char *fw_getenv (char *name)
 	char *env, *nxt;
 
 	if (env_init ())
-		return (NULL);
+		return NULL;
 
 	for (env = environment.data; *env; env = nxt + 1) {
 		char *val;
@@ -194,15 +216,15 @@ char *fw_getenv (char *name)
 			if (nxt >= &environment.data[ENV_SIZE]) {
 				fprintf (stderr, "## Error: "
 					"environment not terminated\n");
-				return (NULL);
+				return NULL;
 			}
 		}
 		val = envmatch (name, env);
 		if (!val)
 			continue;
-		return (val);
+		return val;
 	}
-	return (NULL);
+	return NULL;
 }
 
 /*
@@ -216,7 +238,7 @@ int fw_printenv (int argc, char *argv[])
 	int rc = 0;
 
 	if (env_init ())
-		return (-1);
+		return -1;
 
 	if (argc == 1) {		/* Print all env variables  */
 		for (env = environment.data; *env; env = nxt + 1) {
@@ -224,13 +246,13 @@ int fw_printenv (int argc, char *argv[])
 				if (nxt >= &environment.data[ENV_SIZE]) {
 					fprintf (stderr, "## Error: "
 						"environment not terminated\n");
-					return (-1);
+					return -1;
 				}
 			}
 
 			printf ("%s\n", env);
 		}
-		return (0);
+		return 0;
 	}
 
 	if (strcmp (argv[1], "-n") == 0) {
@@ -240,7 +262,7 @@ int fw_printenv (int argc, char *argv[])
 		if (argc != 2) {
 			fprintf (stderr, "## Error: "
 				"`-n' option requires exactly one argument\n");
-			return (-1);
+			return -1;
 		}
 	} else {
 		n_flag = 0;
@@ -256,7 +278,7 @@ int fw_printenv (int argc, char *argv[])
 				if (nxt >= &environment.data[ENV_SIZE]) {
 					fprintf (stderr, "## Error: "
 						"environment not terminated\n");
-					return (-1);
+					return -1;
 				}
 			}
 			val = envmatch (name, env);
@@ -275,11 +297,11 @@ int fw_printenv (int argc, char *argv[])
 		}
 	}
 
-	return (rc);
+	return rc;
 }
 
 /*
- * Deletes or sets environment variables. Returns errno style error codes:
+ * Deletes or sets environment variables. Returns -1 and sets errno error codes:
  * 0	  - OK
  * EINVAL - need at least 1 argument
  * EROFS  - certain variables ("ethaddr", "serial#") cannot be
@@ -294,11 +316,12 @@ int fw_setenv (int argc, char *argv[])
 	char *name;
 
 	if (argc < 2) {
-		return (EINVAL);
+		errno = EINVAL;
+		return -1;
 	}
 
 	if (env_init ())
-		return (errno);
+		return -1;
 
 	name = argv[1];
 
@@ -310,7 +333,8 @@ int fw_setenv (int argc, char *argv[])
 			if (nxt >= &environment.data[ENV_SIZE]) {
 				fprintf (stderr, "## Error: "
 					"environment not terminated\n");
-				return (EINVAL);
+				errno = EINVAL;
+				return -1;
 			}
 		}
 		if ((oldval = envmatch (name, env)) != NULL)
@@ -327,7 +351,8 @@ int fw_setenv (int argc, char *argv[])
 		if ((strcmp (name, "ethaddr") == 0) ||
 			(strcmp (name, "serial#") == 0)) {
 			fprintf (stderr, "Can't overwrite \"%s\"\n", name);
-			return (EROFS);
+			errno = EROFS;
+			return -1;
 		}
 
 		if (*++nxt == '\0') {
@@ -366,7 +391,7 @@ int fw_setenv (int argc, char *argv[])
 		fprintf (stderr,
 			"Error: environment overflow, \"%s\" deleted\n",
 			name);
-		return (-1);
+		return -1;
 	}
 	while ((*env = *name++) != '\0')
 		env++;
@@ -382,195 +407,418 @@ int fw_setenv (int argc, char *argv[])
 
   WRITE_FLASH:
 
-	/* Update CRC */
-	environment.crc = crc32 (0, (uint8_t*) environment.data, ENV_SIZE);
+	/*
+	 * Update CRC
+	 */
+	*environment.crc = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
 
 	/* write environment back to flash */
-	if (flash_io (O_RDWR)) {
+	if (flash_write ()) {
 		fprintf (stderr, "Error: can't write fw_env to flash\n");
-		return (-1);
+		return -1;
 	}
 
-	return (0);
+	return 0;
 }
 
-static int flash_io (int mode)
+/*
+ * Test for bad block on NAND, just returns 0 on NOR, on NAND:
+ * 0	- block is good
+ * > 0	- block is bad
+ * < 0	- failed to test
+ */
+static int flash_bad_block (int dev, int fd, struct mtd_info_user *mtdinfo,
+			    loff_t *blockstart)
 {
-	int fd, fdr, rc, otherdev, len, resid;
-	erase_info_t erase;
-	char *data = NULL;
+	if (mtdinfo->type == MTD_NANDFLASH) {
+		int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
 
-	if ((fd = open (DEVNAME (curdev), mode)) < 0) {
-		fprintf (stderr,
-			"Can't open %s: %s\n",
-			DEVNAME (curdev), strerror (errno));
-		return (-1);
-	}
-
-	len = sizeof (environment.crc);
-	if (HaveRedundEnv) {
-		len += sizeof (environment.flags);
-	}
-
-	if (mode == O_RDWR) {
-		if (HaveRedundEnv) {
-			/* switch to next partition for writing */
-			otherdev = !curdev;
-			if ((fdr = open (DEVNAME (otherdev), mode)) < 0) {
-				fprintf (stderr,
-					"Can't open %s: %s\n",
-					DEVNAME (otherdev),
-					strerror (errno));
-				return (-1);
-			}
-		} else {
-			otherdev = curdev;
-			fdr = fd;
-		}
-		printf ("Unlocking flash...\n");
-		erase.length = DEVESIZE (otherdev);
-		erase.start = DEVOFFSET (otherdev);
-		ioctl (fdr, MEMUNLOCK, &erase);
-
-		if (HaveRedundEnv) {
-			erase.length = DEVESIZE (curdev);
-			erase.start = DEVOFFSET (curdev);
-			ioctl (fd, MEMUNLOCK, &erase);
-			environment.flags = active_flag;
+		if (badblock < 0) {
+			perror ("Cannot read bad block mark");
+			return badblock;
 		}
 
-		printf ("Done\n");
-		resid = DEVESIZE (otherdev) - CFG_ENV_SIZE;
-		if (resid) {
-			if ((data = malloc (resid)) == NULL) {
-				fprintf (stderr,
-					"Cannot malloc %d bytes: %s\n",
-					resid,
-					strerror (errno));
-				return (-1);
-			}
-			if (lseek (fdr, DEVOFFSET (otherdev) + CFG_ENV_SIZE, SEEK_SET)
-				== -1) {
-				fprintf (stderr, "seek error on %s: %s\n",
-					DEVNAME (otherdev),
-					strerror (errno));
-				return (-1);
-			}
-			if ((rc = read (fdr, data, resid)) != resid) {
-				fprintf (stderr,
-					"read error on %s: %s\n",
-					DEVNAME (otherdev),
-					strerror (errno));
-				return (-1);
-			}
+		if (badblock) {
+#ifdef DEBUG
+			fprintf (stderr, "Bad block at 0x%llx, "
+				 "skipping\n", *blockstart);
+#endif
+			return badblock;
 		}
+	}
 
-		printf ("Erasing old environment...\n");
+	return 0;
+}
 
-		erase.length = DEVESIZE (otherdev);
-		erase.start = DEVOFFSET (otherdev);
-		if (ioctl (fdr, MEMERASE, &erase) != 0) {
-			fprintf (stderr, "MTD erase error on %s: %s\n",
-				DEVNAME (otherdev),
-				strerror (errno));
-			return (-1);
-		}
+/*
+ * Read data from flash at an offset into a provided buffer. On NAND it skips
+ * bad blocks but makes sure it stays within ENVSECTORS (dev) starting from
+ * the DEVOFFSET (dev) block. On NOR the loop is only run once.
+ */
+static int flash_read_buf (int dev, int fd, void *buf, size_t count,
+			   off_t offset)
+{
+	struct mtd_info_user mtdinfo;
+	/* erase / write length - one block on NAND, 0 on NOR */
+	size_t blocklen;
+	/* progress counter */
+	size_t processed = 0;
+	/* current read length */
+	size_t readlen = count;
+	/* end of the last block we may use */
+	off_t top_of_range;
+	/* offset inside the current block to the start of the data */
+	off_t block_seek;
+	/* running start of the current block - MEMGETBADBLOCK needs 64 bits */
+	loff_t blockstart;
+	int rc;
+
+	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
+	if (rc < 0) {
+		perror ("Cannot get MTD information");
+		return rc;
+	}
+
+	/*
+	 * Start of the first block to be read, relies on the fact, that
+	 * erase sector size is always a power of 2
+	 */
+	blockstart = offset & ~(DEVESIZE (dev) - 1);
+	/* Offset inside a block */
+	block_seek = offset - blockstart;
 
-		printf ("Done\n");
+	if (mtdinfo.type == MTD_NANDFLASH) {
+		/*
+		 * NAND: calculate which blocks we are reading. We have
+		 * to read one block@a time to skip bad blocks.
+		 */
+		blocklen = DEVESIZE (dev);
+
+		/*
+		 * To calculate the top of the range, we have to use the
+		 * global DEVOFFSET (dev), which can be different from offset
+		 */
+		top_of_range = (DEVOFFSET (dev) & ~(blocklen - 1)) +
+			ENVSECTORS (dev) * blocklen;
 
-		printf ("Writing environment to %s...\n", DEVNAME (otherdev));
-		if (lseek (fdr, DEVOFFSET (otherdev), SEEK_SET) == -1) {
+		/* Limit to one block for the first read */
+		if (readlen > blocklen - block_seek)
+			readlen = blocklen - block_seek;
+	} else {
+		blocklen = 0;
+		top_of_range = offset + count;
+	}
+
+	/* This only runs once on NOR flash */
+	while (processed < count) {
+		rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart);
+		if (rc < 0) {
+			return -1;
+		} else if (blockstart + block_seek + readlen > top_of_range) {
+			/* End of range is reached */
 			fprintf (stderr,
-				"seek error on %s: %s\n",
-				DEVNAME (otherdev), strerror (errno));
-			return (-1);
+				 "Too few good blocks within range\n");
+			return -1;
+		} else if (rc) {
+			blockstart += blocklen;
+			continue;
 		}
-		if (write (fdr, &environment, len) != len) {
-			fprintf (stderr,
-				"CRC write error on %s: %s\n",
-				DEVNAME (otherdev), strerror (errno));
-			return (-1);
+
+		/*
+		 * 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);
+
+		rc = read (fd, buf + processed, readlen);
+		if (rc != readlen) {
+			fprintf (stderr, "Read error on %s: %s\n",
+				 DEVNAME (dev), strerror (errno));
+			return -1;
 		}
-		if (write (fdr, environment.data, ENV_SIZE) != ENV_SIZE) {
+#ifdef DEBUG
+		fprintf (stderr, "Read 0x%x bytes at 0x%llx\n",
+			 rc, blockstart + block_seek);
+#endif
+		processed += readlen;
+		readlen = min (blocklen, count - processed);
+		block_seek = 0;
+		blockstart += blocklen;
+	}
+
+	return processed;
+}
+
+/*
+ * Write count bytes at offset, but stay within ENVSETCORS (dev) sectors of
+ * DEVOFFSET (dev). Similar to the read case above, on NOR we erase and write
+ * the whole data at once.
+ */
+static int __flash_write_buf (int dev, int fd, void *buf, size_t count,
+			      off_t offset, struct mtd_info_user *mtdinfo)
+{
+	char *data = NULL;
+	erase_info_t erase;
+	/* length of NAND block / NOR erase sector */
+	size_t blocklen;
+	/* whole area that can be erased - may include bad blocks */
+	size_t erase_len;
+	/* erase / write length - one block on NAND, whole area on NOR */
+	size_t erasesize;
+	/* progress counter */
+	size_t processed = 0;
+	/* total size to actually write - excludinig bad blocks */
+	size_t write_total;
+	/* offset to the first erase block (aligned) below offset */
+	off_t erase_offset;
+	/* offset inside the erase block to the start of the data */
+	off_t block_seek;
+	/* end of the last block we may use */
+	off_t top_of_range;
+	/* running start of the current block - MEMGETBADBLOCK needs 64 bits */
+	loff_t blockstart;
+	int rc;
+
+	blocklen = DEVESIZE (dev);
+
+	/* Erase sector size is always a power of 2 */
+	top_of_range = (DEVOFFSET (dev) & ~(blocklen - 1)) +
+		ENVSECTORS (dev) * blocklen;
+
+	erase_offset = offset & ~(blocklen - 1);
+
+	/* Maximum area we may use */
+	erase_len = top_of_range - erase_offset;
+
+	blockstart = erase_offset;
+	/* Offset inside a block */
+	block_seek = offset - erase_offset;
+
+	/*
+	 * Data size we actually have to write: from the start of the block
+	 * to the start of the data, then count bytes ob data, and to the
+	 * end of the block
+	 */
+	write_total = (block_seek + count + blocklen - 1) & ~(blocklen - 1);
+
+	/*
+	 * Support data anywhere within erase sectors: read out the complete
+	 * area to be erased, replace the environment image, write the whole
+	 * block back again.
+	 */
+	if (write_total > count) {
+		data = malloc (erase_len);
+		if (!data) {
 			fprintf (stderr,
-				"Write error on %s: %s\n",
-				DEVNAME (otherdev), strerror (errno));
-			return (-1);
+				 "Cannot malloc %u bytes: %s\n",
+				 erase_len, strerror (errno));
+			return -1;
 		}
-		if (resid) {
-			if (write (fdr, data, resid) != resid) {
-				fprintf (stderr,
-					"write error on %s: %s\n",
-					DEVNAME (curdev), strerror (errno));
-				return (-1);
-			}
-			free (data);
-		}
-		if (HaveRedundEnv) {
-			/* change flag on current active env partition */
-			if (lseek (fd, DEVOFFSET (curdev) + sizeof (ulong), SEEK_SET)
-				== -1) {
-				fprintf (stderr, "seek error on %s: %s\n",
-					DEVNAME (curdev), strerror (errno));
-				return (-1);
-			}
-			if (write (fd, &obsolete_flag, sizeof (obsolete_flag)) !=
-				sizeof (obsolete_flag)) {
-				fprintf (stderr,
-					"Write error on %s: %s\n",
-					DEVNAME (curdev), strerror (errno));
-				return (-1);
-			}
+
+		rc = flash_read_buf (dev, fd, data, write_total, erase_offset);
+		if (write_total != rc)
+			return -1;
+
+		/* Overwrite the old environment */
+		memcpy(data + block_seek, buf, count);
+	} else {
+		/* Offset is block-aligned by construction here */
+		data = (char *)environment.image;
+		write_total = count;
+	}
+
+	if (mtdinfo->type == MTD_NANDFLASH)
+		/*
+		 * NAND: calculate which blocks we are writing. We have
+		 * to write one block at a time to skip bad blocks.
+		 */
+		erasesize = blocklen;
+	else
+		erasesize = erase_len;
+
+	erase.length = erasesize;
+
+	/* This only runs once on NOR flash */
+	while (processed < write_total) {
+		rc = flash_bad_block (dev, fd, mtdinfo, &blockstart);
+		if (rc < 0) {
+			return rc;
+		} else if (blockstart + erasesize > top_of_range) {
+			fprintf (stderr, "End of range reached, aborting\n");
+			return -1;
+		} else if (rc) {
+			blockstart += blocklen;
+			continue;
 		}
-		printf ("Done\n");
-		printf ("Locking ...\n");
-		erase.length = DEVESIZE (otherdev);
-		erase.start = DEVOFFSET (otherdev);
-		ioctl (fdr, MEMLOCK, &erase);
-		if (HaveRedundEnv) {
-			erase.length = DEVESIZE (curdev);
-			erase.start = DEVOFFSET (curdev);
-			ioctl (fd, MEMLOCK, &erase);
-			if (close (fdr)) {
-				fprintf (stderr,
-					"I/O error on %s: %s\n",
-					DEVNAME (otherdev),
-					strerror (errno));
-				return (-1);
-			}
+
+		erase.start = blockstart;
+		ioctl (fd, MEMUNLOCK, &erase);
+
+		if (ioctl (fd, MEMERASE, &erase) != 0) {
+			fprintf (stderr, "MTD erase error on %s: %s\n",
+				 DEVNAME (dev),
+				 strerror (errno));
+			return -1;
 		}
-		printf ("Done\n");
-	} else {
 
-		if (lseek (fd, DEVOFFSET (curdev), SEEK_SET) == -1) {
+		if (lseek (fd, blockstart, SEEK_SET) == -1) {
 			fprintf (stderr,
-				"seek error on %s: %s\n",
-				DEVNAME (curdev), strerror (errno));
-			return (-1);
+				 "Seek error on %s: %s\n",
+				 DEVNAME (dev), strerror (errno));
+			return -1;
 		}
-		if (read (fd, &environment, len) != len) {
-			fprintf (stderr,
-				"CRC read error on %s: %s\n",
-				DEVNAME (curdev), strerror (errno));
-			return (-1);
+
+#ifdef DEBUG
+		printf ("Writing 0x%x bytes at 0x%llx\n", erasesize, blockstart);
+#endif
+		if (write (fd, data + processed, erasesize) != erasesize) {
+			fprintf (stderr, "Write error on %s: %s\n",
+				 DEVNAME (dev), strerror (errno));
+			return -1;
 		}
-		if ((rc = read (fd, environment.data, ENV_SIZE)) != ENV_SIZE) {
+
+		ioctl (fd, MEMLOCK, &erase);
+
+		processed += blocklen;
+		block_seek = 0;
+		blockstart += blocklen;
+	}
+
+	if (write_total > count)
+		free (data);
+
+	return processed;
+}
+
+static int flash_write_buf (int dev, int fd, void *buf, size_t count,
+			    off_t offset)
+{
+	struct mtd_info_user mtdinfo;
+	int rc;
+
+	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
+	if (rc < 0) {
+		perror ("Cannot get MTD information");
+		return -1;
+	}
+
+	return __flash_write_buf (dev, fd, buf, count, offset, &mtdinfo);
+}
+
+/*
+ * Set obsolete flag at offset
+ */
+static int flash_flag_obsolete (int dev, int fd, off_t offset)
+{
+	struct mtd_info_user mtdinfo;
+	int rc;
+
+	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
+	if (rc < 0) {
+		perror ("Cannot get MTD information");
+		return -1;
+	}
+
+	if (mtdinfo.type == MTD_NANDFLASH) {
+		/*
+		 * No luck on NAND - we could save the erase, but have to write
+		 * a whole erase block anyway
+		 */
+		rc = __flash_write_buf (dev, fd, &obsolete_flag,
+					sizeof (obsolete_flag), offset,
+					&mtdinfo);
+	} else {
+		/* This relies on the fact, that obsolete_flag == 0 */
+		rc = lseek (fd, offset, SEEK_SET);
+		if (rc < 0) {
+			fprintf (stderr, "Cannot seek to set the flag on %s \n",
+				 DEVNAME (dev));
+			return rc;
+		}
+		rc = write (fd, &obsolete_flag, sizeof (obsolete_flag));
+		if (rc < 0)
+			perror ("Could not set obsolete flag");
+	}
+	return rc;
+}
+
+static int flash_write (void)
+{
+	int fd_current, fd_target, rc, dev_target;
+
+	/* 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));
+		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,
-				"Read error on %s: %s\n",
-				DEVNAME (curdev), strerror (errno));
-			return (-1);
+				 "Can't open %s: %s\n",
+				 DEVNAME (dev_target),
+				 strerror (errno));
+			return -1;
 		}
+		*environment.flags = active_flag;
+	} else {
+		dev_target = dev_current;
+		fd_target = fd_current;
 	}
 
-	if (close (fd)) {
+	printf ("Writing new environment at 0x%lx\n", DEVOFFSET (dev_target));
+	rc = flash_write_buf (dev_target, fd_target, environment.image,
+			      CFG_ENV_SIZE, DEVOFFSET (dev_target));
+	if (rc < 0)
+		return rc;
+
+	if (HaveRedundEnv) {
+		off_t offset = DEVOFFSET (dev_current) +
+			offsetof(struct env_image_redundant, flags);
+		printf ("Setting obsolete flag for environment@0x%lx\n",
+			DEVOFFSET (dev_current));
+		flash_flag_obsolete(dev_current, fd_current, offset);
+	}
+
+	if (close (fd_target)) {
 		fprintf (stderr,
-			"I/O error on %s: %s\n",
-			DEVNAME (curdev), strerror (errno));
-		return (-1);
+			 "I/O error on %s: %s\n",
+			 DEVNAME (dev_target), strerror (errno));
+		return -1;
 	}
 
 	/* everything ok */
-	return (0);
+	return 0;
+}
+
+static int flash_read (void)
+{
+	int fd, rc;
+
+	if ((fd = open (DEVNAME (dev_current), O_RDONLY)) < 0) {
+		fprintf (stderr,
+			 "Can't open %s: %s\n",
+			 DEVNAME (dev_current), strerror (errno));
+		return -1;
+	}
+
+	/* Only try within CFG_ENV_RANGE */
+	rc = flash_read_buf (dev_current, fd, environment.image, CFG_ENV_SIZE,
+			     DEVOFFSET (dev_current));
+
+	if (close (fd)) {
+		fprintf (stderr,
+			 "I/O error on %s: %s\n",
+			 DEVNAME (dev_current), strerror (errno));
+		return -1;
+	}
+
+	return rc < 0 ? rc : 0;
 }
 
 /*
@@ -584,10 +832,10 @@ static char *envmatch (char * s1, char * s2)
 
 	while (*s1 == *s2++)
 		if (*s1++ == '=')
-			return (s2);
+			return s2;
 	if (*s1 == '\0' && *(s2 - 1) == '=')
-		return (s2);
-	return (NULL);
+		return s2;
+	return NULL;
 }
 
 /*
@@ -596,107 +844,120 @@ static char *envmatch (char * s1, char * s2)
 static int env_init (void)
 {
 	int crc1, crc1_ok;
-	char *addr1;
+	char flag1, *addr1;
 
 	int crc2, crc2_ok;
-	char flag1, flag2, *addr2;
+	char flag2, *addr2;
+
+	struct env_image_single *single;
+	struct env_image_redundant *redundant;
 
 	if (parse_config ())		/* should fill envdevices */
-		return 1;
+		return -1;
 
-	if ((addr1 = calloc (1, ENV_SIZE)) == NULL) {
+	if ((addr1 = calloc (1, CFG_ENV_SIZE)) == NULL) {
 		fprintf (stderr,
 			"Not enough memory for environment (%ld bytes)\n",
-			ENV_SIZE);
-		return (errno);
+			CFG_ENV_SIZE);
+		return -1;
 	}
 
 	/* read environment from FLASH to local buffer */
-	environment.data = addr1;
-	curdev = 0;
-	if (flash_io (O_RDONLY)) {
-		return (errno);
+	environment.image = addr1;
+
+	if (HaveRedundEnv) {
+		redundant = (struct env_image_redundant *)addr1;
+		environment.crc		= &redundant->crc;
+		environment.flags	= &redundant->flags;
+		environment.data	= redundant->data;
+	} else {
+		single = (struct env_image_single *)addr1;
+		environment.crc		= &single->crc;
+		environment.flags	= NULL;
+		environment.data	= single->data;
 	}
 
-	crc1_ok = ((crc1 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE))
-			   == environment.crc);
+	dev_current = 0;
+	if (flash_read ())
+		return -1;
+
+	crc1 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
+	crc1_ok = (crc1 == *environment.crc);
 	if (!HaveRedundEnv) {
 		if (!crc1_ok) {
 			fprintf (stderr,
 				"Warning: Bad CRC, using default environment\n");
-			memcpy(environment.data, default_environment, sizeof default_environment);
+			memcpy(environment.data, default_environment,
+			       sizeof default_environment);
 		}
 	} else {
-		flag1 = environment.flags;
+		flag1 = *environment.flags;
 
-		curdev = 1;
-		if ((addr2 = calloc (1, ENV_SIZE)) == NULL) {
+		dev_current = 1;
+		if ((addr2 = calloc (1, CFG_ENV_SIZE)) == NULL) {
 			fprintf (stderr,
 				"Not enough memory for environment (%ld bytes)\n",
-				ENV_SIZE);
-			return (errno);
+				CFG_ENV_SIZE);
+			return -1;
 		}
-		environment.data = addr2;
+		redundant = (struct env_image_redundant *)addr2;
 
-		if (flash_io (O_RDONLY)) {
-			return (errno);
-		}
+		/*
+		 * have to set environment.image for flash_read(), careful -
+		 * other pointers in environment still point inside addr1
+		 */
+		environment.image = addr2;
+		if (flash_read ())
+			return -1;
 
-		crc2_ok = ((crc2 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE))
-				   == environment.crc);
-		flag2 = environment.flags;
+		crc2 = crc32 (0, (uint8_t *) redundant->data, ENV_SIZE);
+		crc2_ok = (crc2 == redundant->crc);
+		flag2 = redundant->flags;
 
 		if (crc1_ok && !crc2_ok) {
-			environment.data = addr1;
-			environment.flags = flag1;
-			environment.crc = crc1;
-			curdev = 0;
-			free (addr2);
+			dev_current = 0;
 		} else if (!crc1_ok && crc2_ok) {
-			environment.data = addr2;
-			environment.flags = flag2;
-			environment.crc = crc2;
-			curdev = 1;
-			free (addr1);
+			dev_current = 1;
 		} else if (!crc1_ok && !crc2_ok) {
 			fprintf (stderr,
 				"Warning: Bad CRC, using default environment\n");
-			memcpy(environment.data, default_environment, sizeof default_environment);
-			curdev = 0;
-			free (addr1);
+			memcpy(environment.data,
+			       default_environment, sizeof default_environment);
+			/* prepare for device 2 */
+			dev_current = 0;
+		/* From here: both CRCs correct */
 		} else if (flag1 == active_flag && flag2 == obsolete_flag) {
-			environment.data = addr1;
-			environment.flags = flag1;
-			environment.crc = crc1;
-			curdev = 0;
-			free (addr2);
+			dev_current = 0;
 		} else if (flag1 == obsolete_flag && flag2 == active_flag) {
-			environment.data = addr2;
-			environment.flags = flag2;
-			environment.crc = crc2;
-			curdev = 1;
-			free (addr1);
+			dev_current = 1;
+		/* From here: invalid flag configuration */
 		} else if (flag1 == flag2) {
-			environment.data = addr1;
-			environment.flags = flag1;
-			environment.crc = crc1;
-			curdev = 0;
-			free (addr2);
+			dev_current = 0;
 		} else if (flag1 == 0xFF) {
-			environment.data = addr1;
-			environment.flags = flag1;
-			environment.crc = crc1;
-			curdev = 0;
-			free (addr2);
+			dev_current = 0;
 		} else if (flag2 == 0xFF) {
-			environment.data = addr2;
-			environment.flags = flag2;
-			environment.crc = crc2;
-			curdev = 1;
+			dev_current = 1;
+		} else {
+			dev_current = 0;
+		}
+
+		/*
+		 * If we are reading, we don't need the flag and the crc any
+		 * more, if we are writing, we will set them before writing out
+		 */
+		if (dev_current) {
+			environment.image	= addr2;
+			environment.crc		= &redundant->crc;
+			environment.flags	= &redundant->flags;
+			environment.data	= redundant->data;
 			free (addr1);
+		} else {
+			environment.image	= addr1;
+			/* Other pointers are already set */
+			free (addr2);
 		}
 	}
-	return (0);
+	return 0;
 }
 
 
@@ -709,18 +970,20 @@ static int parse_config ()
 	if (get_config (CONFIG_FILE)) {
 		fprintf (stderr,
 			"Cannot parse config file: %s\n", strerror (errno));
-		return 1;
+		return -1;
 	}
 #else
 	strcpy (DEVNAME (0), DEVICE1_NAME);
 	DEVOFFSET (0) = DEVICE1_OFFSET;
 	ENVSIZE (0) = ENV1_SIZE;
 	DEVESIZE (0) = DEVICE1_ESIZE;
+	ENVSECTORS (0) = DEVICE1_ENVSECTORS;
 #ifdef HAVE_REDUND
 	strcpy (DEVNAME (1), DEVICE2_NAME);
 	DEVOFFSET (1) = DEVICE2_OFFSET;
 	ENVSIZE (1) = ENV2_SIZE;
 	DEVESIZE (1) = DEVICE2_ESIZE;
+	ENVSECTORS (1) = DEVICE2_ENVSECTORS;
 	HaveRedundEnv = 1;
 #endif
 #endif
@@ -728,14 +991,14 @@ static int parse_config ()
 		fprintf (stderr,
 			"Cannot access MTD device %s: %s\n",
 			DEVNAME (0), strerror (errno));
-		return 1;
+		return -1;
 	}
 
 	if (HaveRedundEnv && stat (DEVNAME (1), &st)) {
 		fprintf (stderr,
 			"Cannot access MTD device %s: %s\n",
 			DEVNAME (1), strerror (errno));
-		return 1;
+		return -1;
 	}
 	return 0;
 }
@@ -748,21 +1011,27 @@ static int get_config (char *fname)
 	int rc;
 	char dump[128];
 
-	if ((fp = fopen (fname, "r")) == NULL) {
-		return 1;
-	}
-
-	while ((i < 2) && ((rc = fscanf (fp, "%s %lx %lx %lx",
-				  DEVNAME (i),
-				  &DEVOFFSET (i),
-				  &ENVSIZE (i),
-				  &DEVESIZE (i)  )) != EOF)) {
+	if ((fp = fopen (fname, "r")) == NULL)
+		return -1;
 
+	while (i < 2 && fgets (dump, sizeof (dump), fp)) {
 		/* Skip incomplete conversions and comment strings */
-		if ((rc < 3) || (*DEVNAME (i) == '#')) {
-			fgets (dump, sizeof (dump), fp);	/* Consume till end */
+		if (dump[0] == '#')
 			continue;
-		}
+
+		rc = sscanf (dump, "%s %lx %lx %lx %lx",
+			     DEVNAME (i),
+			     &DEVOFFSET (i),
+			     &ENVSIZE (i),
+			     &DEVESIZE (i),
+			     &ENVSECTORS (i));
+
+		if (rc < 4)
+			continue;
+
+		if (rc < 5)
+			/* Default - 1 sector */
+			ENVSECTORS (i) = 1;
 
 		i++;
 	}
@@ -771,7 +1040,7 @@ static int get_config (char *fname)
 	HaveRedundEnv = i - 1;
 	if (!i) {			/* No valid entries found */
 		errno = EINVAL;
-		return 1;
+		return -1;
 	} else
 		return 0;
 }
diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config
index 2432bd8..0fe37c9 100644
--- a/tools/env/fw_env.config
+++ b/tools/env/fw_env.config
@@ -1,7 +1,11 @@
 # Configuration file for fw_(printenv/saveenv) utility.
 # Up to two entries are valid, in this case the redundand
 # environment sector is assumed present.
+# Notice, that the "Number of sectors" is ignored on NOR.
 
-# MTD device name	Device offset	Env. size	Flash sector size
+# MTD device name	Device offset	Env. size	Flash sector size	Number of sectors
 /dev/mtd1		0x0000		0x4000		0x4000
 /dev/mtd2		0x0000		0x4000		0x4000
+
+# NAND example
+#/dev/mtd0		0x4000		0x4000		0x20000			2

[U-Boot] [PATCH] fw_env: add NAND support

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0809021840240.9447@axis700.grange> you wrote:
> Add support for environment in NAND with automatic NOR / NAND recognition, 
> including unaligned environment, bad-block skipping, redundant environment 
> copy.
> 
> Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
> ---
> 
> This is now a single patch. No union any more, instead a struct with 
> pointers into a flat image buffer is used. Still, MTD_VERSION=old doesn't 
> work, it looks it is also broken in the current version.

I wonder why you didn't even try to fix it.

See [PATCH] fw_env.c: fix build problems with MTD_VERSION=old

Please rebase your patch and resubmit.


> diff --git a/tools/env/README b/tools/env/README
> index f8a644e..f32f872 100644
> --- a/tools/env/README
> +++ b/tools/env/README
> @@ -22,9 +22,11 @@ following lines are relevant:
>  #define DEVICE1_OFFSET    0x0000
>  #define ENV1_SIZE         0x4000
>  #define DEVICE1_ESIZE     0x4000
> +#define DEVICE1_ENVSECTORS     2
>  #define DEVICE2_OFFSET    0x0000
>  #define ENV2_SIZE         0x4000
>  #define DEVICE2_ESIZE     0x4000
> +#define DEVICE2_ENVSECTORS     2
>  
>  Current configuration matches the environment layout of the TRAB
>  board.
> @@ -46,3 +48,7 @@ then 1 sector.
>  
>  DEVICEx_ESIZE defines the size of the first sector in the flash
>  partition where the environment resides.
> +
> +DEVICEx_ENVSECTORS defines the number of sectors that may be used for
> +this environment instance. On NAND this is used to limit the range
> +within which bad blocks are skipped, on NOR it is unused.

I think "...on NOR it is not used" would be clearer ?

> +static int flash_read_buf (int dev, int fd, void *buf, size_t count,
> +			   off_t offset)
> +{
> +	struct mtd_info_user mtdinfo;
> +	/* erase / write length - one block on NAND, 0 on NOR */
> +	size_t blocklen;
> +	/* progress counter */
> +	size_t processed = 0;
> +	/* current read length */
> +	size_t readlen = count;
> +	/* end of the last block we may use */
> +	off_t top_of_range;
> +	/* offset inside the current block to the start of the data */
> +	off_t block_seek;
> +	/* running start of the current block - MEMGETBADBLOCK needs 64 bits */
> +	loff_t blockstart;
> +	int rc;

That's difficult to read. Please reformat (comments on smae line with
code, if necessary continued on next line).

> +	/*
> +	 * Start of the first block to be read, relies on the fact, that
> +	 * erase sector size is always a power of 2
> +	 */
> +	blockstart = offset & ~(DEVESIZE (dev) - 1);

Insert empty line here.

> +	/* Offset inside a block */
> +	block_seek = offset - blockstart;
...
> +		/* Limit to one block for the first read */

Why?

> +		if (readlen > blocklen - block_seek)
> +			readlen = blocklen - block_seek;
> +	} else {
> +		blocklen = 0;
> +		top_of_range = offset + count;
> +	}
> +
> +	/* This only runs once on NOR flash */
> +	while (processed < count) {
> +		rc = flash_bad_block (dev, fd, &mtdinfo, &blockstart);
> +		if (rc < 0) {
> +			return -1;
> +		} else if (blockstart + block_seek + readlen > top_of_range) {
> +			/* End of range is reached */
>  			fprintf (stderr,
> +				 "Too few good blocks within range\n");
> +			return -1;
> +		} else if (rc) {
> +			blockstart += blocklen;
> +			continue;
>  		}

Please rewrite as:

		if (rc < 0)		/* block test failed */
			return -1;

		if (blockstart + block_seek + readlen > top_of_range) {
			/* End of range is reached */
			fprintf (stderr,
				"Too few good blocks within range\n");
			return -1;
		}

		if (rc) {		/* block is bad */
			blockstart += blocklen;
			continue;
		}

I can much easier follow the flow when written that way.

> +
> +		/*
> +		 * 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);

Hm... there was a "continue" for the badblock case just above, so do
we really try the _next_ block here?

> +static int __flash_write_buf (int dev, int fd, void *buf, size_t count,
> +			      off_t offset, struct mtd_info_user *mtdinfo)
> +{
> +	char *data = NULL;
> +	erase_info_t erase;
> +	/* length of NAND block / NOR erase sector */
> +	size_t blocklen;
> +	/* whole area that can be erased - may include bad blocks */
> +	size_t erase_len;
> +	/* erase / write length - one block on NAND, whole area on NOR */
> +	size_t erasesize;
> +	/* progress counter */
> +	size_t processed = 0;
> +	/* total size to actually write - excludinig bad blocks */
> +	size_t write_total;
> +	/* offset to the first erase block (aligned) below offset */
> +	off_t erase_offset;
> +	/* offset inside the erase block to the start of the data */
> +	off_t block_seek;
> +	/* end of the last block we may use */
> +	off_t top_of_range;
> +	/* running start of the current block - MEMGETBADBLOCK needs 64 bits */
> +	loff_t blockstart;
> +	int rc;

Not readable. Please reformat.

> +	/*
> +	 * Data size we actually have to write: from the start of the block
> +	 * to the start of the data, then count bytes ob data, and to the
-------------------------------------------------------^ f
> +	 * end of the block
> +	 */

Hm... sounds as if this was always exactly one full block, then?

> +	write_total = (block_seek + count + blocklen - 1) & ~(blocklen - 1);
> +
> +	/*
> +	 * Support data anywhere within erase sectors: read out the complete
> +	 * area to be erased, replace the environment image, write the whole
> +	 * block back again.
> +	 */
> +	if (write_total > count) {
> +		data = malloc (erase_len);

My understanding is, that erase_len can be > block size. Is this
correct?

I don't see where the actual size of the environment data is taken
into considration?

> +	} else {
> +		/* Offset is block-aligned by construction here */

What does "block-aligned by construction" mean?

> +	if (mtdinfo->type == MTD_NANDFLASH)
Add ----------------------------------------> {
> +		/*
> +		 * NAND: calculate which blocks we are writing. We have
> +		 * to write one block at a time to skip bad blocks.
> +		 */
> +		erasesize = blocklen;
> +	else
	} else {
> +		erasesize = erase_len;
	}

> +	erase.length = erasesize;
> +
> +	/* This only runs once on NOR flash */

How comes that?

> +	while (processed < write_total) {
> +		rc = flash_bad_block (dev, fd, mtdinfo, &blockstart);
> +		if (rc < 0) {
> +			return rc;
> +		} else if (blockstart + erasesize > top_of_range) {
> +			fprintf (stderr, "End of range reached, aborting\n");
> +			return -1;
> +		} else if (rc) {
> +			blockstart += blocklen;
> +			continue;
>  		}

Please rewrite - see above.

> +		processed += blocklen;
> +		block_seek = 0;
> +		blockstart += blocklen;

		processed  += blocklen;
		blockstart += blocklen;
		block_seek = 0;
	

> +	printf ("Writing new environment at 0x%lx\n", DEVOFFSET (dev_target));

If you really want to keep this printf(), thenplease also print the
device name (i. e try to stick to the format of the output of the old
version and at least provide the same information).

You may also consider the comments posted before, that such  informa-
tion should be printed only when the verbose option (argument -v) was
selected.

> static int flash_write (void)
> {
>         int fd_current, fd_target, rc, dev_target;
>  
>         /* 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));
>                 return -1;
>         }

Please factor out the open()/close() as discussed several times
before.

> +	if (rc < 0)
> +		return rc;
> +
> +	if (HaveRedundEnv) {
> +		off_t offset = DEVOFFSET (dev_current) +
> +			offsetof(struct env_image_redundant, flags);
> +		printf ("Setting obsolete flag for environment at 0x%lx\n",
> +			DEVOFFSET (dev_current));

Please make this a debug print only.

In general, please check printed output.

> +		flash_flag_obsolete(dev_current, fd_current, offset);
> +	}
> +
> +	if (close (fd_target)) {
>  		fprintf (stderr,
> +			 "I/O error on %s: %s\n",
> +			 DEVNAME (dev_target), strerror (errno));
> +		return -1;
>  	}

Please factor out the open()/close() as discussed several times
before.

> +static int flash_read (void)
> +{
> +	int fd, rc;
> +
> +	if ((fd = open (DEVNAME (dev_current), O_RDONLY)) < 0) {
> +		fprintf (stderr,
> +			 "Can't open %s: %s\n",
> +			 DEVNAME (dev_current), strerror (errno));
> +		return -1;
> +	}
...
> +	if (close (fd)) {
> +		fprintf (stderr,
> +			 "I/O error on %s: %s\n",
> +			 DEVNAME (dev_current), strerror (errno));
> +		return -1;
> +	}

Please factor out the open()/close() as discussed several times
before.


> +	return rc < 0 ? rc : 0;

	return (rc < 0) ? rc : 0;

> @@ -596,107 +844,120 @@ static char *envmatch (char * s1, char * s2)
>  static int env_init (void)
>  {
>  	int crc1, crc1_ok;
> +	char flag1, *addr1;
>  
>  	int crc2, crc2_ok;
> +	char flag2, *addr2;

I think you should number these 0 an 1, respectively here.

Because then you can read this code much better:


>  		if (crc1_ok && !crc2_ok) {
> +			dev_current = 0;
>  		} else if (!crc1_ok && crc2_ok) {
> +			dev_current = 1;

would become:
	
		if (crc0_ok && !crc1_ok) { 
			dev_current = 0;
		} else if (!crc0_ok && crc1_ok) {
			dev_current = 1;

If "0" is OK, then use "0"; if "1" is ok then use "1".

Your version: if "1" is OK then use "0", if "2" is OK then use "1"
is more difficult to read.

[Or stick with the old identifiers, i. e. use 1 and  2  consistently,
but don't mix 0/1 here and 1/2 there.]

	
...
> +			/* prepare for device 2 */
> +			dev_current = 0;
> +		/* From here: both CRCs correct */

The comment doesn't fit (indentation is wrong), and it's obvious anyway - omit it.

> +		/* From here: invalid flag configuration */

Ditto.

> +		/*
> +		 * If we are reading, we don't need the flag and the crc any
> +		 * more, if we are writing, we will set them before writing out
> +		 */

Note that this is a serious impairment compared to the  old  version.

The whole logic of writing the envrionment (and  especially  so  when
redundancy  is  used)  is based on separate logic steps that are per-
formed strictly sequentially, and only  when  the  previous  one  was
successfully  completed.  This essential to maintain consistent data.
For the redundant case that means:

1) write the environment data to flash.
   *After* this has completed, make the data valid by
2) writing the CRC checksum.
   *After* this has completed, make this copy of the environment valid by
3) writing the valid flag to this copy.
   *After* this has completed, make the other copy of the environment obsolete by
4) writing the obsolete flag to the other copy.

Your new implementation breaks this by writing out the  whole  buffer
at  once.  This  is  not  a  good  ideas,  because you may end up ith
situations that were impossible before, like having a valid flag byte
in flash  even  though  the  environment  data  were  not  completely
written.

I am aware that you have little options with NAND flash, but for  NOR
this is IMHO a serious change for the worse.

Can we please avoid this, and re-establish the old mode of operation,
which was designed for reliability?


> +	if ((fp = fopen (fname, "r")) == NULL)
> +		return -1;
>  
> +	while (i < 2 && fgets (dump, sizeof (dump), fp)) {
>  		/* Skip incomplete conversions and comment strings */
> +		if (dump[0] == '#')
>  			continue;

I think you must initialize ENVSECTORS(i) here...

> +		rc = sscanf (dump, "%s %lx %lx %lx %lx",
> +			     DEVNAME (i),
> +			     &DEVOFFSET (i),
> +			     &ENVSIZE (i),
> +			     &DEVESIZE (i),
> +			     &ENVSECTORS (i));
> +
> +		if (rc < 4)
> +			continue;
> +
> +		if (rc < 5)
> +			/* Default - 1 sector */
> +			ENVSECTORS (i) = 1;

Because if rc < 4, you already continued and left ENVSECTORS uninitialized.

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@denx.de
It became apparent that one reason why the Ice Giants were  known  as
the  Ice  Giants  was  because they were, well, giants. The other was
that they were made of ice.              -Terry Pratchett, _Sourcery_

[U-Boot] [PATCH] fw_env: add NAND support

[Guennadi Liakhovetski]

On Tue, 2 Sep 2008, Wolfgang Denk wrote:

> > +	/* Offset inside a block */
> > +	block_seek = offset - blockstart;
> ...
> > +		/* Limit to one block for the first read */
> 
> Why?

Because this is on NAND, there we have to perform all IO block at a time.

> > +
> > +		/*
> > +		 * 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);
> 
> Hm... there was a "continue" for the badblock case just above, so do
> we really try the _next_ block here?

I meant, if the first block with non-zero offset (block_seek) was bad, 
then in the _next_ after it block we try at the same offset.

> > +	/*
> > +	 * Data size we actually have to write: from the start of the block
> > +	 * to the start of the data, then count bytes ob data, and to the
> -------------------------------------------------------^ f
> > +	 * end of the block
> > +	 */
> 
> Hm... sounds as if this was always exactly one full block, then?

or two, or three...

> > +	write_total = (block_seek + count + blocklen - 1) & ~(blocklen - 1);
> > +
> > +	/*
> > +	 * Support data anywhere within erase sectors: read out the complete
> > +	 * area to be erased, replace the environment image, write the whole
> > +	 * block back again.
> > +	 */
> > +	if (write_total > count) {
> > +		data = malloc (erase_len);
> 
> My understanding is, that erase_len can be > block size. Is this
> correct?

Yes.

> I don't see where the actual size of the environment data is taken
> into considration?

in the "count" function parameter, which is then used to calculate 
write_total.

> > +	} else {
> > +		/* Offset is block-aligned by construction here */
> 
> What does "block-aligned by construction" mean?

It means, by the way I constructed (calculated) lengths above this place, 
we land here only if offset is block-aligned.

> > +	erase.length = erasesize;
> > +
> > +	/* This only runs once on NOR flash */
> 
> How comes that?

Because on NOR I set "erasesize" to the total length of the data plus 
alignment.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH] fw_env: add NAND support

[Guennadi Liakhovetski]

On Tue, 2 Sep 2008, Wolfgang Denk wrote:

> > +		/*
> > +		 * If we are reading, we don't need the flag and the crc any
> > +		 * more, if we are writing, we will set them before writing out
> > +		 */
> 
> Note that this is a serious impairment compared to the  old  version.
> 
> The whole logic of writing the envrionment (and  especially  so  when
> redundancy  is  used)  is based on separate logic steps that are per-
> formed strictly sequentially, and only  when  the  previous  one  was
> successfully  completed.  This essential to maintain consistent data.
> For the redundant case that means:
> 
> 1) write the environment data to flash.
>    *After* this has completed, make the data valid by
> 2) writing the CRC checksum.
>    *After* this has completed, make this copy of the environment valid by
> 3) writing the valid flag to this copy.
>    *After* this has completed, make the other copy of the environment obsolete by
> 4) writing the obsolete flag to the other copy.
> 
> Your new implementation breaks this by writing out the  whole  buffer
> at  once.  This  is  not  a  good  ideas,  because you may end up ith
> situations that were impossible before, like having a valid flag byte
> in flash  even  though  the  environment  data  were  not  completely
> written.

Sorry, I still do not understand what bad can happen with this 
whole-image-at-once implementation. Yes, it may happen, that crc has been 
written, flag has been written, but the data has not completely been 
written. Then, you get a CRC error on reading. Just as if you were writing 
in portions and an error occurred when writing the data. Yes, the flag 
would have been different, with the previous approach you would still have 
the "redundant" flag set in the corrupted copy that you were trying to 
update and "active" in the other one. Writing the whole image at once you 
end up with two "active" flags. But this seems unimportant, because CRCs 
are checked before - at least in fw_env - so, in both cases you end up 
using the non-damaged copy.

If you were first setting the flag to "invalid, write in progress", then 
wrote the environment, then reset the flag to "valid, write completed 
successfully", then yes, writing per one write would be essentially 
different.

In short, I think, CRC provides sufficient protection of data integrity.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH] fw_env: add NAND support

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0809030014190.12823@axis700.grange> you wrote:
>
> If you were first setting the flag to "invalid, write in progress", then 
> wrote the environment, then reset the flag to "valid, write completed 
> successfully", then yes, writing per one write would be essentially 
> different.

But that's exactly what we are doing. The flag  value  for  "invalid,
write  in progress" is 0xFF and is set as soon as we start to destroy
the valid data by erasing the flash.

> In short, I think, CRC provides sufficient protection of data integrity.

CRC32 is not a perfect test. Yes, it's good enough for most  ordinary
cases,  but  when we can design in additional robustness we should do
this. And we should not give up what we  already  have  without  very
good reasons.

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
The Wright Bothers weren't the first to fly. They were just the first
not to crash.

[U-Boot] [PATCH] fw_env: add NAND support

[Guennadi Liakhovetski]

On Tue, 2 Sep 2008, Wolfgang Denk wrote:

> > @@ -596,107 +844,120 @@ static char *envmatch (char * s1, char * s2)
> >  static int env_init (void)
> >  {
> >  	int crc1, crc1_ok;
> > +	char flag1, *addr1;
> >  
> >  	int crc2, crc2_ok;
> > +	char flag2, *addr2;
> 
> I think you should number these 0 an 1, respectively here.
> 
> Because then you can read this code much better:
> 
> 
> >  		if (crc1_ok && !crc2_ok) {
> > +			dev_current = 0;
> >  		} else if (!crc1_ok && crc2_ok) {
> > +			dev_current = 1;
> 
> would become:
> 	
> 		if (crc0_ok && !crc1_ok) { 
> 			dev_current = 0;
> 		} else if (!crc0_ok && crc1_ok) {
> 			dev_current = 1;
> 
> If "0" is OK, then use "0"; if "1" is ok then use "1".
> 
> Your version: if "1" is OK then use "0", if "2" is OK then use "1"
> is more difficult to read.
> 
> [Or stick with the old identifiers, i. e. use 1 and  2  consistently,
> but don't mix 0/1 here and 1/2 there.]

Sorry, don't understand. This is the original code:

		if (crc1_ok && !crc2_ok) {
			environment.data = addr1;
			environment.flags = flag1;
			environment.crc = crc1;
			curdev = 0;
			free (addr2);

Which looks to me like "if (crc1_ok...) {... curdev = 0;...}", which I 
tried to preserve. So, you would prefer me to _change_ it?

> > +	if ((fp = fopen (fname, "r")) == NULL)
> > +		return -1;
> >  
> > +	while (i < 2 && fgets (dump, sizeof (dump), fp)) {
> >  		/* Skip incomplete conversions and comment strings */
> > +		if (dump[0] == '#')
> >  			continue;
> 
> I think you must initialize ENVSECTORS(i) here...
> 
> > +		rc = sscanf (dump, "%s %lx %lx %lx %lx",
> > +			     DEVNAME (i),
> > +			     &DEVOFFSET (i),
> > +			     &ENVSIZE (i),
> > +			     &DEVESIZE (i),
> > +			     &ENVSECTORS (i));
> > +
> > +		if (rc < 4)
> > +			continue;
> > +
> > +		if (rc < 5)
> > +			/* Default - 1 sector */
> > +			ENVSECTORS (i) = 1;
> 
> Because if rc < 4, you already continued and left ENVSECTORS uninitialized.

As far as I understand, with rc == 3 there is no DEVESIZE in the line, 
which doesn't look like a valid configuration line to me. The orginal code 
however accepted such lines and only dropped lines with rc < 3. I do not 
understand how the original code managed to work with rc == 3. As I 
thought this was a bug, I changed the test to "rc < 4", i.e., now I 
require at least 4 fields, in which case I initialise ENVSECTORS to the 
default value - 1 sector. If rc < 4 the counter "i" is not incremented and 
the line is dropped - in the same way as in the original version. Or am I 
missing something?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

[U-Boot] [PATCH] fw_env: add NAND support

[Guennadi Liakhovetski]

On Tue, 2 Sep 2008, Wolfgang Denk wrote:

> Note that this is a serious impairment compared to the  old  version.
> 
> The whole logic of writing the envrionment (and  especially  so  when
> redundancy  is  used)  is based on separate logic steps that are per-
> formed strictly sequentially, and only  when  the  previous  one  was
> successfully  completed.  This essential to maintain consistent data.
> For the redundant case that means:
> 
> 1) write the environment data to flash.
>    *After* this has completed, make the data valid by
> 2) writing the CRC checksum.
>    *After* this has completed, make this copy of the environment valid by
> 3) writing the valid flag to this copy.
>    *After* this has completed, make the other copy of the environment obsolete by
> 4) writing the obsolete flag to the other copy.

I am afraid, I cannot confirm this in the current version. In it first CRC 
and flag is written with one write, then the data, then optionally any 
residual data. Which seems like not providing any additional robustness. 
Would you like me to replicate this behaviour, in which case we could just 
as well write the whole buffer at once as I did up to now, or change it 
to follow your scheme above? In the latter case I am not sure if this will 
work with all kernel versions with all mtd drivers and all NOR flashes.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH] fw_env: add NAND support

[Markus Klotzbücher]

Dear Guennadi,

On Tue, Sep 02, 2008 at 07:05:29PM +0200, Guennadi Liakhovetski wrote:

> +static int flash_flag_obsolete (int dev, int fd, off_t offset)
> +{
> +	struct mtd_info_user mtdinfo;
> +	int rc;
> +
> +	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> +	if (rc < 0) {
> +		perror ("Cannot get MTD information");
> +		return -1;
> +	}
> +
> +	if (mtdinfo.type == MTD_NANDFLASH) {
> +		/*
> +		 * No luck on NAND - we could save the erase, but have to write
> +		 * a whole erase block anyway
> +		 */
> +		rc = __flash_write_buf (dev, fd, &obsolete_flag,
> +					sizeof (obsolete_flag), offset,
> +					&mtdinfo);

Hmmm, what exactly are you doing here? IIRC environment in NAND was
implemented slightly differently than NOR in that it reused the
obsolete flag as a serial number. This number is incremented for every
write so the env with the higher number is always the current one
(besides the corner case where the number overflows, see
env_nand.c). This way the obsoleting can be completly avoided.

I can't see this in your patch?

Best regards
Markus

P.S.: As mentioned before this mechanism of course could save the
"obsoleting" operation in NOR too...

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de")

[U-Boot] [PATCH] fw_env: add NAND support

[Wolfgang Denk]

Dear Guennadi,

In message <alpine.DEB.1.00.0809031108220.6727@axis700.grange> you wrote:
> 
> > would become:
> > 	
> > 		if (crc0_ok && !crc1_ok) { 
> > 			dev_current = 0;
> > 		} else if (!crc0_ok && crc1_ok) {
> > 			dev_current = 1;
> > 
> > If "0" is OK, then use "0"; if "1" is ok then use "1".
> > 
> > Your version: if "1" is OK then use "0", if "2" is OK then use "1"
> > is more difficult to read.
> > 
> > [Or stick with the old identifiers, i. e. use 1 and  2  consistently,
> > but don't mix 0/1 here and 1/2 there.]
> 
> Sorry, don't understand. This is the original code:
> 
> 		if (crc1_ok && !crc2_ok) {
> 			environment.data = addr1;
> 			environment.flags = flag1;
> 			environment.crc = crc1;
> 			curdev = 0;
> 			free (addr2);
> 
> Which looks to me like "if (crc1_ok...) {... curdev = 0;...}", which I 
> tried to preserve. So, you would prefer me to _change_ it?

...except that that nor "curdev =0" is present anywhere, but instead
the index "1" is used consistently: crc1_ok, addr1, flag1, crc1.

> As far as I understand, with rc == 3 there is no DEVESIZE in the line, 
> which doesn't look like a valid configuration line to me. The orginal code 
> however accepted such lines and only dropped lines with rc < 3. I do not 
> understand how the original code managed to work with rc == 3. As I 
> thought this was a bug, I changed the test to "rc < 4", i.e., now I 

Did you verify that is was a bug?


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@denx.de
Ever try. Ever fail. No matter. Try again. Fail again.  Fail  better.
                                                        -- S. Beckett

[U-Boot] [PATCH] fw_env: add NAND support

[Wolfgang Denk]

Dear Markus =?iso-8859-1?Q?Klotzb=FCcher?=,

In message <20080903115150.GA12440@lisa> you wrote:
> 
> Hmmm, what exactly are you doing here? IIRC environment in NAND was
> implemented slightly differently than NOR in that it reused the
> obsolete flag as a serial number. This number is incremented for every
> write so the env with the higher number is always the current one
> (besides the corner case where the number overflows, see
> env_nand.c). This way the obsoleting can be completly avoided.

Right, that was how you designed and implemented it.

> I can't see this in your patch?

I don't see it either. Thanks for pointing out.


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
Fools ignore complexity. Pragmatists suffer it. Some  can  avoid  it.
Geniuses remove it.
     - Perlis's Programming Proverb #58, SIGPLAN Notices, Sept.  1982

[U-Boot] [PATCH] fw_env: add NAND support

[Guennadi Liakhovetski]

On Wed, 3 Sep 2008, Wolfgang Denk wrote:

> > As far as I understand, with rc == 3 there is no DEVESIZE in the line, 
> > which doesn't look like a valid configuration line to me. The orginal code 
> > however accepted such lines and only dropped lines with rc < 3. I do not 
> > understand how the original code managed to work with rc == 3. As I 
> > thought this was a bug, I changed the test to "rc < 4", i.e., now I 
> 
> Did you verify that is was a bug?

Yes:

Unlocking flash...
Done
Cannot malloc -32768 bytes: Cannot allocate memory
Error: can't write fw_env to flash

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH] fw_env: add NAND support

[Guennadi Liakhovetski]

On Wed, 3 Sep 2008, Markus Klotzb?cher wrote:

> Dear Guennadi,
> 
> On Tue, Sep 02, 2008 at 07:05:29PM +0200, Guennadi Liakhovetski wrote:
> 
> > +static int flash_flag_obsolete (int dev, int fd, off_t offset)
> > +{
> > +	struct mtd_info_user mtdinfo;
> > +	int rc;
> > +
> > +	rc = ioctl (fd, MEMGETINFO, &mtdinfo);
> > +	if (rc < 0) {
> > +		perror ("Cannot get MTD information");
> > +		return -1;
> > +	}
> > +
> > +	if (mtdinfo.type == MTD_NANDFLASH) {
> > +		/*
> > +		 * No luck on NAND - we could save the erase, but have to write
> > +		 * a whole erase block anyway
> > +		 */
> > +		rc = __flash_write_buf (dev, fd, &obsolete_flag,
> > +					sizeof (obsolete_flag), offset,
> > +					&mtdinfo);
> 
> Hmmm, what exactly are you doing here? IIRC environment in NAND was
> implemented slightly differently than NOR in that it reused the
> obsolete flag as a serial number. This number is incremented for every
> write so the env with the higher number is always the current one
> (besides the corner case where the number overflows, see
> env_nand.c). This way the obsoleting can be completly avoided.
> 
> I can't see this in your patch?

No, it is not there, because I had no idea about it:-( Thanks for pointing 
out - will implement in the next version.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de