[PATCH 4/4] MTD: drivers return max_bitflips, mtd returns -EUCLEAN

[Shmulik Ladkani]

On Sun, 11 Mar 2012 14:21:13 -0700 Mike Dunn <mikedunn at newsguy.com> wrote:
> -	return mtd->_read(mtd, from, len, retlen, buf);
> +
> +	/*
> +	 * In the absence of an error, drivers return a non-negative integer
> +	 * representing the maximum number of bitflips that were corrected on
> +	 * any one writesize region (typically a NAND page).
> +	 */
> +	ret_code = mtd->_read(mtd, from, len, retlen, buf);

Maybe better for the remark to reside in mtd.h near '_read' definition?

> +	if (unlikely(ret_code < 0))
> +		return ret_code;
> +
> +	return ret_code >= mtd->euclean_threshold ? -EUCLEAN : 0;

I'm a bit confused here.
From former patches, you state:

> The element 'euclean_threshold' is added to struct mtd_info.  If the driver
> leaves this uninitialized, mtd sets it to ecc_strength when the device or
> partition is registered. 

and:

> Flash device drivers initialize 'ecc_strength' in struct mtd_info, which is the
> maximum number of bit errors that can be corrected in one writesize region.

So I conclude that by default 'euclean_threshold' is equivalent to
"maximum number of bit errors that can be corrected in one writesize".

Now, lets go back to the "ret_code >= mtd->euclean_threshold" condition.

I suspect that in the default case, we'll never reach that condition.

Suppose the read introduced 'euclean_threshold' (well, 'ecc_strength')
bit errors; according to defintion of 'ecc_strength', it actually means
an uncorrectable error.
And as such, it is reported by the driver by incrementing
'ecc_stats.failed', and by the nand infrastructure as -EBADMSG.
So we'll hit the "unlikely(ret_code < 0)" case, and not the
"ret_code >= mtd->euclean_threshold" condition.

Conclusion:
As long as 'euclean_threshold' is kept to its default and not tweaked by
the sysfs knob, MTD system no longer reports -EUCLEAN.

Was that the intention? Did I miss something here?

>  /*
>   * Method to access the protection register area, present in some flash
>   * devices. The user data is one time programmable but the factory data is read
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 9651c06..24022ce 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -67,12 +67,12 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	stats = part->master->ecc_stats;
>  	res = part->master->_read(part->master, from + part->offset, len,
>  				  retlen, buf);
> -	if (unlikely(res)) {
> -		if (mtd_is_bitflip(res))
> -			mtd->ecc_stats.corrected += part->master->ecc_stats.corrected - stats.corrected;
> -		if (mtd_is_eccerr(res))
> -			mtd->ecc_stats.failed += part->master->ecc_stats.failed - stats.failed;
> -	}
> +	if (unlikely(mtd_is_eccerr(res)))
> +		mtd->ecc_stats.failed +=
> +			part->master->ecc_stats.failed - stats.failed;
> +	else
> +		mtd->ecc_stats.corrected +=
> +			part->master->ecc_stats.corrected - stats.corrected;

Is there a reason to execute the "else" part ('corrected' increment) in
case 'res' is negative?

> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 8008853..e2bf003 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1594,7 +1600,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  	if (mtd->ecc_stats.failed - stats.failed)
>  		return -EBADMSG;
>  
> -	return  mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
> +	return max_bitflips;
>  }

Two lines eralier we have:
	if (ret)
		return ret;

where 'ret' is assigned to the return value of
read_page/read_page_raw/read_subpage calls.
Meaning, your new code rely on these calls to never return a positive
value, otherwise the 'read' wrapper might mistakenly identify these
as max bitflips.
This is a reasonable assumption. Would be nice if it can be enforced.
(BTW I looked on many implementations, all return 0...)

Regards,
Shmulik