[PATCH 0/2] Support skipping bad blocks when seeking to start address

[David Oberhollenzer]

On 01/05/2017 04:04 PM, Mike Crowe wrote:
>> I think this request is acceptable (even if I don't like the option
>> name :-)), but I'll let Richard and David decide what to do with this
>> patch.
> 
> I'm not particularly attached to the option name. I chose
> "--skip-bad-blocks-to-start" in the hope of being as clear as possible, but
> I agree that it is a mouthful! The confusion over the meaning of the option
> was also one of the reasons why I originally implemented the feature using
> a separate offset.
As I mentioned in my initial suggestion, this is IMO much clearer than
having two different offset options and avoids the confusing corner case
of having both set.

I would gladly accept this patch series (apart from the missing signed-off-by),
however what actually sparked the discussion above was this snippet here:

On 01/04/2017 03:18 PM, Mike Crowe wrote:
> diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
> index 9602a6e..880990f 100644
> --- a/nand-utils/nandwrite.c
> +++ b/nand-utils/nandwrite.c
> @@ -349,6 +356,25 @@ int main(int argc, char * const argv[])
>  		goto closeall;
>  	}
>  
> +	/* Skip bad blocks on the way to the start address if necessary */
> +	if (skip_bad_blocks_to_start) {
> +		unsigned long long bbs_offset = 0;
> +		while (bbs_offset < mtdoffset) {
> +			ret = mtd_is_bad(&mtd, fd, bbs_offset / ebsize_aligned);
> +			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) "
> +						"from %llx will be skipped\n",
> +						bbs_offset, blockalign, bbs_offset);
> +				mtdoffset += ebsize_aligned;
> +			}
> +			bbs_offset += ebsize_aligned;
> +		}
> +	}
> +
>  	/* Check, if length fits into device */
>  	if ((imglen / pagelen) * mtd.min_io_size > mtd.size - mtdoffset) {
>  		fprintf(stderr, "Image %lld bytes, NAND page %d bytes, OOB area %d"
> 
The nandwrite program supports working on clusters of erase blocks aka
"virtual erase blocks", because JFFS2 supports that.

The variable ebsize_aligned is used all over the place in nandwrite and
can hold a multiple of the actual erase block size.

Your patch skips a virtual block only if the first block is bad. IMO it
would make more sense to check if any of the clustered blocks is bad in
the case where ebsize_alligned is larger than a single erase block.

This prompts the questions of how your bootload would deal with that,
on the one heand, and how JFFS2 deals with bad blocks within a virtual
erase block.


Regards,

David