[PATCH 0/3] mtd-utils: fix nandwrite virtual block handling

[PATCH 0/3] mtd-utils: fix nandwrite virtual block handling

[David Oberhollenzer]

This patch series mainly fixes how nandwrite checks for bad blocks
when using jffs2 virtual erase blocks and tries to cleanup the
nandwrite main loop by using new utility functions.

David Oberhollenzer (3):
      nandwrite: add stricter sanity checking for blockalign
      nandwrite: replace erase loop with mtd_erase_multi
      nandwrite: fix/cleanup bad block skipping

 nand-utils/nandwrite.c | 82 +++++++++++++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 38 deletions(-)

[PATCH 1/3] nandwrite: add stricter sanity checking for blockalign

[David Oberhollenzer]

This patch makes sure that a virtual erase block is always
composed of a postivie number of erase blocks (i.e. 1 or more)
and enforces the block alignment to be a power of two as
suggested by the help text and assumed throughout the program.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 nand-utils/nandwrite.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
index 9602a6e..998c68c 100644
--- a/nand-utils/nandwrite.c
+++ b/nand-utils/nandwrite.c
@@ -191,9 +191,13 @@ static void process_options(int argc, char * const argv[])
 		errmsg_die("Can't specify negative device offset with option"
 				" -s: %lld", mtdoffset);
 
-	if (blockalign < 0)
-		errmsg_die("Can't specify negative blockalign with option -b:"
-				" %d", blockalign);
+	if (blockalign <= 0)
+		errmsg_die("Can't specify negative or zero blockalign with "
+				"option -b: %d", blockalign);
+
+	if (!is_power_of_2(blockalign))
+		errmsg_die("Can't specify a non-power-of-two blockalign with "
+				"option -b: %d", blockalign);
 
 	if (autoplace && noecc)
 		errmsg_die("Autoplacement and no-ECC are mutually exclusive");
-- 
2.10.2

[PATCH 2/3] nandwrite: replace erase loop with mtd_erase_multi

[David Oberhollenzer]

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 nand-utils/nandwrite.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
index 998c68c..22c741d 100644
--- a/nand-utils/nandwrite.c
+++ b/nand-utils/nandwrite.c
@@ -540,7 +540,6 @@ int main(int argc, char * const argv[])
 		}
 
 		if (ret) {
-			long long i;
 			if (errno != EIO) {
 				sys_errmsg("%s: MTD write failure", mtd_device);
 				goto closeall;
@@ -551,13 +550,13 @@ int main(int argc, char * const argv[])
 
 			fprintf(stderr, "Erasing failed write from %#08llx to %#08llx\n",
 				blockstart, blockstart + ebsize_aligned - 1);
-			for (i = blockstart; i < blockstart + ebsize_aligned; i += mtd.eb_size) {
-				if (mtd_erase(mtd_desc, &mtd, fd, i / mtd.eb_size)) {
-					int errno_tmp = errno;
-					sys_errmsg("%s: MTD Erase failure", mtd_device);
-					if (errno_tmp != EIO)
-						goto closeall;
-				}
+
+			if (mtd_erase_multi(mtd_desc, &mtd, fd,
+					blockstart / mtd.eb_size, blockalign)) {
+				int errno_tmp = errno;
+				sys_errmsg("%s: MTD Erase failure", mtd_device);
+				if (errno_tmp != EIO)
+					goto closeall;
 			}
 
 			if (markbad) {
-- 
2.10.2

[PATCH 3/3] nandwrite: fix/cleanup bad block skipping

[David Oberhollenzer]

JFFS2 supports clustering erase blocks to virtual erase blocks.
nandwrite supports this, but previously mixed up virtual and
physical erase block numbers when checking for bad blocks.

This patch adds a function for checking if a virtual erase block
is bad and replaces the broken mtd_is_bad loop.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 nand-utils/nandwrite.c | 57 ++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
index 22c741d..c7a53d1 100644
--- a/nand-utils/nandwrite.c
+++ b/nand-utils/nandwrite.c
@@ -235,6 +235,20 @@ static void erase_buffer(void *buffer, size_t size)
 		memset(buffer, kEraseByte, size);
 }
 
+static int is_virt_block_bad(struct mtd_dev_info *mtd, int fd,
+				long long offset)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < blockalign; ++i) {
+		ret = mtd_is_bad(mtd, fd, offset / mtd->eb_size + i);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
 /*
  * Main program
  */
@@ -246,10 +260,8 @@ int main(int argc, char * const argv[])
 	int ifd = -1;
 	int pagelen;
 	long long imglen = 0;
-	bool baderaseblock = false;
 	long long blockstart = -1;
 	struct mtd_dev_info mtd;
-	long long offs;
 	int ret;
 	bool failed = true;
 	/* contains all the data read from the file so far for the current eraseblock */
@@ -391,7 +403,6 @@ int main(int argc, char * const argv[])
 		 */
 		while (blockstart != (mtdoffset & (~ebsize_aligned + 1))) {
 			blockstart = mtdoffset & (~ebsize_aligned + 1);
-			offs = blockstart;
 
 			/*
 			 * if writebuf == filebuf, we are rewinding so we must
@@ -403,40 +414,32 @@ int main(int argc, char * const argv[])
 				writebuf = filebuf;
 			}
 
-			baderaseblock = false;
 			if (!quiet)
 				fprintf(stdout, "Writing data to block %lld at offset 0x%llx\n",
 						 blockstart / ebsize_aligned, blockstart);
 
-			/* Check all the blocks in an erase block for bad blocks */
 			if (noskipbad)
 				continue;
 
-			do {
-				ret = mtd_is_bad(&mtd, fd, offs / ebsize_aligned);
-				if (ret < 0) {
-					sys_errmsg("%s: MTD get bad block failed", mtd_device);
-					goto closeall;
-				} else if (ret == 1) {
-					baderaseblock = true;
-					if (!quiet)
-						fprintf(stderr, "Bad block at %llx, %u block(s) "
-								"from %llx will be skipped\n",
-								offs, blockalign, blockstart);
-				}
+			ret = is_virt_block_bad(&mtd, fd, blockstart);
 
-				if (baderaseblock) {
-					mtdoffset = blockstart + ebsize_aligned;
-
-					if (mtdoffset > mtd.size) {
-						errmsg("too many bad blocks, cannot complete request");
-						goto closeall;
-					}
-				}
+			if (ret < 0) {
+				sys_errmsg("%s: MTD get bad block failed", mtd_device);
+				goto closeall;
+			} else if (ret == 1) {
+				if (!quiet)
+					fprintf(stderr,
+						"Bad block at %llx, %u block(s) "
+						"will be skipped\n",
+						blockstart, blockalign);
 
-				offs +=  ebsize_aligned / blockalign;
-			} while (offs < blockstart + ebsize_aligned);
+				mtdoffset = blockstart + ebsize_aligned;
 
+				if (mtdoffset > mtd.size) {
+					errmsg("too many bad blocks, cannot complete request");
+					goto closeall;
+				}
+			}
 		}
 
 		/* Read more data from the input if there isn't enough in the buffer */
-- 
2.10.2

Re: [PATCH 3/3] nandwrite: fix/cleanup bad block skipping

[Mike Crowe]

On Thursday 12 January 2017 at 11:28:28 +0100, David Oberhollenzer wrote:
> JFFS2 supports clustering erase blocks to virtual erase blocks.
> nandwrite supports this, but previously mixed up virtual and
> physical erase block numbers when checking for bad blocks.
> 
> This patch adds a function for checking if a virtual erase block
> is bad and replaces the broken mtd_is_bad loop.
> 
> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> ---
>  nand-utils/nandwrite.c | 57 ++++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
> index 22c741d..c7a53d1 100644
> --- a/nand-utils/nandwrite.c
> +++ b/nand-utils/nandwrite.c
> @@ -235,6 +235,20 @@ static void erase_buffer(void *buffer, size_t size)
>  		memset(buffer, kEraseByte, size);
>  }
>  
> +static int is_virt_block_bad(struct mtd_dev_info *mtd, int fd,
> +				long long offset)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < blockalign; ++i) {
> +		ret = mtd_is_bad(mtd, fd, offset / mtd->eb_size + i);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * Main program
>   */
> @@ -246,10 +260,8 @@ int main(int argc, char * const argv[])
>  	int ifd = -1;
>  	int pagelen;
>  	long long imglen = 0;
> -	bool baderaseblock = false;
>  	long long blockstart = -1;
>  	struct mtd_dev_info mtd;
> -	long long offs;
>  	int ret;
>  	bool failed = true;
>  	/* contains all the data read from the file so far for the current eraseblock */
> @@ -391,7 +403,6 @@ int main(int argc, char * const argv[])
>  		 */
>  		while (blockstart != (mtdoffset & (~ebsize_aligned + 1))) {
>  			blockstart = mtdoffset & (~ebsize_aligned + 1);
> -			offs = blockstart;
>  
>  			/*
>  			 * if writebuf == filebuf, we are rewinding so we must
> @@ -403,40 +414,32 @@ int main(int argc, char * const argv[])
>  				writebuf = filebuf;
>  			}
>  
> -			baderaseblock = false;
>  			if (!quiet)
>  				fprintf(stdout, "Writing data to block %lld at offset 0x%llx\n",
>  						 blockstart / ebsize_aligned, blockstart);
>  
> -			/* Check all the blocks in an erase block for bad blocks */
>  			if (noskipbad)
>  				continue;
>  
> -			do {
> -				ret = mtd_is_bad(&mtd, fd, offs / ebsize_aligned);
> -				if (ret < 0) {
> -					sys_errmsg("%s: MTD get bad block failed", mtd_device);
> -					goto closeall;
> -				} else if (ret == 1) {
> -					baderaseblock = true;
> -					if (!quiet)
> -						fprintf(stderr, "Bad block at %llx, %u block(s) "
> -								"from %llx will be skipped\n",
> -								offs, blockalign, blockstart);
> -				}
> +			ret = is_virt_block_bad(&mtd, fd, blockstart);
>  
> -				if (baderaseblock) {
> -					mtdoffset = blockstart + ebsize_aligned;
> -
> -					if (mtdoffset > mtd.size) {
> -						errmsg("too many bad blocks, cannot complete request");
> -						goto closeall;
> -					}
> -				}
> +			if (ret < 0) {
> +				sys_errmsg("%s: MTD get bad block failed", mtd_device);
> +				goto closeall;
> +			} else if (ret == 1) {
> +				if (!quiet)
> +					fprintf(stderr,
> +						"Bad block at %llx, %u block(s) "
> +						"will be skipped\n",
> +						blockstart, blockalign);
>  
> -				offs +=  ebsize_aligned / blockalign;
> -			} while (offs < blockstart + ebsize_aligned);
> +				mtdoffset = blockstart + ebsize_aligned;
>  
> +				if (mtdoffset > mtd.size) {
> +					errmsg("too many bad blocks, cannot complete request");
> +					goto closeall;
> +				}
> +			}
>  		}
>  
>  		/* Read more data from the input if there isn't enough in the buffer */
> -- 
> 2.10.2

I think that this change does probably fix the problems I raised in
http://lists.infradead.org/pipermail/linux-mtd/2017-January/071516.html and
its parents. I can't test it since we don't use virtual erase blocks.

It might be worth is_virt_block_bad checking that it has been passed a
multiple of ebsize_aligned. The caller in this patch is obviously aligned
(so hopefully the compiler would throw away such a check when it inlines
the static function) but that may not always be the case.

I will rebase my --skip-bad-blocks-to-start patch to make use of the new
function.

Thanks.

Mike.