Re: [PATCH] mtd: core: only increment ecc_stats.badblocks on confirmed good->bad transition

[Miquel Raynal]

Hello Wang,

> @@ -2349,17 +2351,35 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  		return -EROFS;
>  
>  	if (mtd->flags & MTD_SLC_ON_MLC_EMULATION)
>  		ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize;
>  
> -	ret = master->_block_markbad(master, mtd_get_master_ofs(mtd, ofs));
> +	moffs = mtd_get_master_ofs(mtd, ofs);
> +
> +	/* Pre-check: remember current state if available. */
> +	if (master->_block_isbad)
> +		oldbad = master->_block_isbad(master, moffs);
> +
> +	ret = master->_block_markbad(master, moffs);
>  	if (ret)
>  		return ret;
>  
> -	while (mtd->parent) {
> -		mtd->ecc_stats.badblocks++;
> -		mtd = mtd->parent;
> +	/*
> +	 * Post-check and bump stats only on a confirmed good->bad transition.
> +	 * If _block_isbad is not implemented, be conservative and do not bump.
> +	 */
> +	if (master->_block_isbad) {
> +		/* If it was already bad, nothing to do. */
> +		if (oldbad > 0)
> +			return 0;
> +
> +		if (master->_block_isbad(master, moffs) > 0) {
> +			while (mtd->parent) {
> +				mtd->ecc_stats.badblocks++;
> +				mtd = mtd->parent;
> +			}
> +		}

I don't think you can assume the block was already bad and must not be
accounted as a new bad block if you cannot verify that. In this case we
must remain conservative and tell userspace a new block was marked bad,
I believe.

Said otherwise, the { while () badblocks++ } block shall remain outside
of the if (_block_isbad) condition and remain untouched. Just bail out
early if you are sure this is not needed.

Thanks,
Miquèl