[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 at 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.