[U-Boot] [PATCH 0/6] tools/env: refactor flash read/write function

[U-Boot] [PATCH 0/6] tools/env: refactor flash read/write function

[Andreas Fenkart]

Take advantage of the environment being block aligned
block alignment is enforced since 183923d3e4

Andreas Fenkart (6):
  tools/env: factor out environment_end function
  tools/env: pass bad block offset by value
  tools/env: lookup dev_type directly from
    flash_read_buf/flash_write_buf
  tools/env: flash_write_buf: enforce offset to be start of environment
  tools/env: flash_write_buf: remove block alignment calculations
  tools/env: flash_read_buf: enforce offset to be start of environment

 tools/env/fw_env.c | 121 ++++++++++++++++++++---------------------------------
 1 file changed, 45 insertions(+), 76 deletions(-)

-- 
2.8.1

[U-Boot] [PATCH 1/6] tools/env: factor out environment_end function

[Andreas Fenkart]

instead of adhoc computation of the environment end,
use a function with a proper name

Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index d27f57e..90eb5fa 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -643,6 +643,18 @@ int fw_parse_script(char *fname, struct env_opts *opts)
 	return ret;
 }
 
+/**
+ * environment_end() - compute offset of first byte right after environemnt
+ * @dev - index of enviroment buffer
+ * Return:
+ *  device offset of first byte right after environemnt
+ */
+off_t environment_end(int dev)
+{
+	/* environment is block aligned */
+	return DEVOFFSET(dev) + ENVSECTORS(dev) * DEVESIZE(dev);
+}
+
 /*
  * Test for bad block on NAND, just returns 0 on NOR, on NAND:
  * 0	- block is good
@@ -683,7 +695,6 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 				   0 on NOR */
 	size_t processed = 0;	/* progress counter */
 	size_t readlen = count;	/* current read length */
-	off_t top_of_range;	/* end of the last block we may use */
 	off_t block_seek;	/* offset inside the current block to the start
 				   of the data */
 	loff_t blockstart;	/* running start of the current block -
@@ -702,19 +713,11 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 		 */
 		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) +
-				ENVSECTORS (dev)) * blocklen;
-
 		/* 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 */
@@ -723,7 +726,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 		if (rc < 0)		/* block test failed */
 			return -1;
 
-		if (blockstart + block_seek + readlen > top_of_range) {
+		if (blockstart + block_seek + readlen > environment_end(dev)) {
 			/* End of range is reached */
 			fprintf (stderr,
 				 "Too few good blocks within range\n");
@@ -783,7 +786,6 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 				   below offset */
 	off_t block_seek;	/* offset inside the erase block to the start
 				   of the data */
-	off_t top_of_range;	/* end of the last block we may use */
 	loff_t blockstart;	/* running start of the current block -
 				   MEMGETBADBLOCK needs 64 bits */
 	int rc;
@@ -793,7 +795,6 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 	 */
 	if (mtd_type == MTD_ABSENT) {
 		blocklen = count;
-		top_of_range = offset + count;
 		erase_len = blocklen;
 		blockstart = offset;
 		block_seek = 0;
@@ -801,13 +802,10 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 	} else {
 		blocklen = DEVESIZE(dev);
 
-		top_of_range = ((DEVOFFSET(dev) / blocklen) +
-					ENVSECTORS(dev)) * blocklen;
-
 		erase_offset = (offset / blocklen) * blocklen;
 
 		/* Maximum area we may use */
-		erase_len = top_of_range - erase_offset;
+		erase_len = environment_end(dev) - erase_offset;
 
 		blockstart = erase_offset;
 		/* Offset inside a block */
@@ -882,7 +880,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 		if (rc < 0)		/* block test failed */
 			return rc;
 
-		if (blockstart + erasesize > top_of_range) {
+		if (blockstart + erasesize > environment_end(dev)) {
 			fprintf (stderr, "End of range reached, aborting\n");
 			return -1;
 		}
-- 
2.8.1

[U-Boot] [PATCH 2/6] tools/env: pass bad block offset by value

[Andreas Fenkart]

the offset is not modified by linux ioctl call
see mtd_ioctl{drivers/mtd/mtdchar.c}
Makes the interface less ambiguous, since the caller can
now exclude a modification of blockstart

Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 90eb5fa..86849b7 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -661,10 +661,10 @@ off_t environment_end(int dev)
  * > 0	- block is bad
  * < 0	- failed to test
  */
-static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart)
+static int flash_bad_block(int fd, uint8_t mtd_type, loff_t blockstart)
 {
 	if (mtd_type == MTD_NANDFLASH) {
-		int badblock = ioctl (fd, MEMGETBADBLOCK, blockstart);
+		int badblock = ioctl(fd, MEMGETBADBLOCK, &blockstart);
 
 		if (badblock < 0) {
 			perror ("Cannot read bad block mark");
@@ -674,7 +674,7 @@ static int flash_bad_block (int fd, uint8_t mtd_type, loff_t *blockstart)
 		if (badblock) {
 #ifdef DEBUG
 			fprintf (stderr, "Bad block@0x%llx, skipping\n",
-				(unsigned long long) *blockstart);
+				(unsigned long long)blockstart);
 #endif
 			return badblock;
 		}
@@ -722,7 +722,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 
 	/* This only runs once on NOR flash */
 	while (processed < count) {
-		rc = flash_bad_block (fd, mtd_type, &blockstart);
+		rc = flash_bad_block(fd, mtd_type, blockstart);
 		if (rc < 0)		/* block test failed */
 			return -1;
 
@@ -876,7 +876,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 
 	/* This only runs once on NOR flash and SPI-dataflash */
 	while (processed < write_total) {
-		rc = flash_bad_block (fd, mtd_type, &blockstart);
+		rc = flash_bad_block(fd, mtd_type, blockstart);
 		if (rc < 0)		/* block test failed */
 			return rc;
 
-- 
2.8.1

[U-Boot] [PATCH 3/6] tools/env: lookup dev_type directly from flash_read_buf/flash_write_buf

[Andreas Fenkart]

flash_write_buf already looks up size/offset/#sector from struct
envdev_s. It can look up mtd_type as well. Same applies to
flash_read_buf. Makes the interface simpler

Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 86849b7..0f0eaa4 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -689,7 +689,7 @@ static int flash_bad_block(int fd, uint8_t mtd_type, loff_t blockstart)
  * 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, uint8_t mtd_type)
+			   off_t offset)
 {
 	size_t blocklen;	/* erase / write length - one block on NAND,
 				   0 on NOR */
@@ -706,7 +706,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 	/* Offset inside a block */
 	block_seek = offset - blockstart;
 
-	if (mtd_type == MTD_NANDFLASH) {
+	if (DEVTYPE(dev) == MTD_NANDFLASH) {
 		/*
 		 * NAND: calculate which blocks we are reading. We have
 		 * to read one block at a time to skip bad blocks.
@@ -722,7 +722,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 
 	/* This only runs once on NOR flash */
 	while (processed < count) {
-		rc = flash_bad_block(fd, mtd_type, blockstart);
+		rc = flash_bad_block(fd, DEVTYPE(dev), blockstart);
 		if (rc < 0)		/* block test failed */
 			return -1;
 
@@ -770,7 +770,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
  * 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, uint8_t mtd_type)
+			    off_t offset)
 {
 	void *data;
 	struct erase_info_user erase;
@@ -793,7 +793,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 	/*
 	 * For mtd devices only offset and size of the environment do matter
 	 */
-	if (mtd_type == MTD_ABSENT) {
+	if (DEVTYPE(dev) == MTD_ABSENT) {
 		blocklen = count;
 		erase_len = blocklen;
 		blockstart = offset;
@@ -834,8 +834,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 			return -1;
 		}
 
-		rc = flash_read_buf (dev, fd, data, write_total, erase_offset,
-				     mtd_type);
+		rc = flash_read_buf(dev, fd, data, write_total, erase_offset);
 		if (write_total != rc)
 			return -1;
 
@@ -862,7 +861,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 		data = buf;
 	}
 
-	if (mtd_type == MTD_NANDFLASH) {
+	if (DEVTYPE(dev) == MTD_NANDFLASH) {
 		/*
 		 * NAND: calculate which blocks we are writing. We have
 		 * to write one block@a time to skip bad blocks.
@@ -876,7 +875,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 
 	/* This only runs once on NOR flash and SPI-dataflash */
 	while (processed < write_total) {
-		rc = flash_bad_block(fd, mtd_type, blockstart);
+		rc = flash_bad_block(fd, DEVTYPE(dev), blockstart);
 		if (rc < 0)		/* block test failed */
 			return rc;
 
@@ -890,11 +889,11 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 			continue;
 		}
 
-		if (mtd_type != MTD_ABSENT) {
+		if (DEVTYPE(dev) != MTD_ABSENT) {
 			erase.start = blockstart;
 			ioctl(fd, MEMUNLOCK, &erase);
 			/* These do not need an explicit erase cycle */
-			if (mtd_type != MTD_DATAFLASH)
+			if (DEVTYPE(dev) != MTD_DATAFLASH)
 				if (ioctl(fd, MEMERASE, &erase) != 0) {
 					fprintf(stderr,
 						"MTD erase error on %s: %s\n",
@@ -921,7 +920,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 			return -1;
 		}
 
-		if (mtd_type != MTD_ABSENT)
+		if (DEVTYPE(dev) != MTD_ABSENT)
 			ioctl(fd, MEMLOCK, &erase);
 
 		processed  += erasesize;
@@ -1008,8 +1007,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
 #endif
 
 	rc = flash_write_buf(dev_target, fd_target, environment.image,
-			      CUR_ENVSIZE, DEVOFFSET(dev_target),
-			      DEVTYPE(dev_target));
+			     CUR_ENVSIZE, DEVOFFSET(dev_target));
 	if (rc < 0)
 		return rc;
 
@@ -1033,7 +1031,7 @@ static int flash_read (int fd)
 	int rc;
 
 	rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE,
-			    DEVOFFSET(dev_current), DEVTYPE(dev_current));
+			    DEVOFFSET(dev_current));
 	if (rc != CUR_ENVSIZE)
 		return -1;
 
-- 
2.8.1

[U-Boot] Suspected Spam: Do not open attachements![PATCH 4/6] tools/env: flash_write_buf: enforce offset to be start of environment

[Andreas Fenkart]

This allows to take advantage of the environment being block aligned.
This is not a new constraint. Writes always start at the begin of the
environment, since the header with CRC/length as there.
Every environment modification requires updating the header

Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 0f0eaa4..3dc0d53 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -765,12 +765,12 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 }
 
 /*
- * Write count bytes at offset, but stay within ENVSECTORS (dev) sectors of
+ * Write count bytes from begin of environment, but stay within
+ * ENVSECTORS(dev) sectors of
  * DEVOFFSET (dev). Similar to the read case above, on NOR and dataflash 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)
+static int flash_write_buf(int dev, int fd, void *buf, size_t count)
 {
 	void *data;
 	struct erase_info_user erase;
@@ -796,20 +796,21 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 	if (DEVTYPE(dev) == MTD_ABSENT) {
 		blocklen = count;
 		erase_len = blocklen;
-		blockstart = offset;
+		blockstart = DEVOFFSET(dev);
 		block_seek = 0;
 		write_total = blocklen;
 	} else {
 		blocklen = DEVESIZE(dev);
 
-		erase_offset = (offset / blocklen) * blocklen;
+		erase_offset = DEVOFFSET(dev);
 
 		/* Maximum area we may use */
 		erase_len = environment_end(dev) - erase_offset;
 
 		blockstart = erase_offset;
+
 		/* Offset inside a block */
-		block_seek = offset - erase_offset;
+		block_seek = DEVOFFSET(dev) - erase_offset;
 
 		/*
 		 * Data size we actually write: from the start of the block
@@ -1007,7 +1008,7 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
 #endif
 
 	rc = flash_write_buf(dev_target, fd_target, environment.image,
-			     CUR_ENVSIZE, DEVOFFSET(dev_target));
+			     CUR_ENVSIZE);
 	if (rc < 0)
 		return rc;
 
-- 
2.8.1

[U-Boot] [U-Boot, 1/6] tools/env: factor out environment_end function

[Tom Rini]

On Mon, Aug 29, 2016 at 11:16:57PM +0200, Andreas Fenkart wrote:

> instead of adhoc computation of the environment end,
> use a function with a proper name
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161008/1af8dc2b/attachment.sig>

[U-Boot] [U-Boot, 2/6] tools/env: pass bad block offset by value

[Tom Rini]

On Mon, Aug 29, 2016 at 11:16:58PM +0200, Andreas Fenkart wrote:

> the offset is not modified by linux ioctl call
> see mtd_ioctl{drivers/mtd/mtdchar.c}
> Makes the interface less ambiguous, since the caller can
> now exclude a modification of blockstart
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161008/427ff005/attachment.sig>

[U-Boot] [U-Boot, 3/6] tools/env: lookup dev_type directly from flash_read_buf/flash_write_buf

[Tom Rini]

On Mon, Aug 29, 2016 at 11:16:59PM +0200, Andreas Fenkart wrote:

> flash_write_buf already looks up size/offset/#sector from struct
> envdev_s. It can look up mtd_type as well. Same applies to
> flash_read_buf. Makes the interface simpler
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161008/d368e063/attachment.sig>

[U-Boot] Suspected Spam: Do not open attachements![PATCH 4/6] tools/env: flash_write_buf: enforce offset to be start of environment

[Tom Rini]

On Mon, Aug 29, 2016 at 11:17:00PM +0200, Andreas Fenkart wrote:

> This allows to take advantage of the environment being block aligned.
> This is not a new constraint. Writes always start at the begin of the
> environment, since the header with CRC/length as there.
> Every environment modification requires updating the header
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161008/e3b5b7d9/attachment.sig>