Re: [PATCH 0/7] mtd: Change meaning of -EUCLEAN return code on reads

[Ivan Djelic <ivan.djelic@parrot.com>]

On Tue, Apr 24, 2012 at 08:18:18PM +0100, Mike Dunn wrote:
> 
> This problem was discussed a while back [1][2][3], and a consensus of sorts was
> reached for a solution, which these patches implement.  The recent addition of
> the mtd api entry functions now make this solution practical (thanks Artem!).  A
> quick description of each patch will provide a synopsis of the set:
> 
> (1) Fix how mtd->ecc_strength is calculated from nand->ecc.strength to reflect
>     the fact that granularity is now at ecc step level (the two values are now
>     the same).
> (2) Fix a couple incorrect nand->ecc.strength values.
> (3) Expose mtd->ecc_strength through sysfs (read-only).
> (4) mtd->bitflip_threshold is added and exposed as read/write through sysfs.
>     The drivers can initialize it, otherwise mtd initializes it to the default
>     value of ecc_strength.  Users can change it through sysfs.
> (5) The nand driver method _read_page() returns max_bitflips to the nand
>     infrastructure code.  This is the maximum number of bitflip corrections that
>     were made on any one region comprising an ecc step.
> (6) Sanity checks added to nand_scan_tail() to ensure that drivers needing to
>     set ecc strength do so.  (This includes all drivers using hardware ecc.)
> (7) Absent a hard error, all drivers' mtd->_read() methods now return
>     max_bitflips to mtd, and mtd_read() makes the decision of whether to return
>     -EUCLEAN or 0. If max_bitflips >= bitflip_threshold, -EUCLEAN is returned.
>     If bitflip_threshold == 0 (no ecc), make no comparison and return 0.
> 
> Of course reviews greatly appreciated.  Thanks!

Hi Mike,

Thanks for this new version.
The whole series of patches looks OK to me, except for one small glitch:
'mtd->bitflip_threshold' can be customized by the driver, but in that case it is not
propagated to the slave partition devices (the same way 'ecc_strength' is propagated).
Something like this is missing:

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index d6321f6..d518e4d 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -517,6 +517,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 
        slave->mtd.ecclayout = master->ecclayout;
        slave->mtd.ecc_strength = master->ecc_strength;
+       slave->mtd.bitflip_threshold = master->bitflip_threshold;
+
        if (master->_block_isbad) {
                uint64_t offs = 0;

Apart from that, I was able to run a few tests on a BeagleBoard revC3 with simulated
bitflips, dropping my own error concealment code in favor of your patch. I did the
following checks:
 - when errors_corrected < bitflip_threshold, check that mtd_read() returns 0
 - when errors_corrected >= bitflip_threshold, check that mtd_read() returns -EUCLEAN
 - check if driver customization of bitflip_threshold works
 - check if per-partition customization of bitflip_threshold through sysfs works
Everything worked as expected.

BR,
--
Ivan