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

login
register
mail settings
Submitter Ivan Djelic
Date April 25, 2012, 11:23 a.m.
Message ID <20120425112319.GA23701@parrot.com>
Download mbox | patch
Permalink /patch/154880/
State New
Headers show

Comments

Ivan Djelic - April 25, 2012, 11:23 a.m.
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:

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
Mike Dunn - April 25, 2012, 3:56 p.m.
On 04/25/2012 04:23 AM, Ivan Djelic wrote:
> 
> 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;
> 


Yes, you're right.  Without this it breaks if the driver sets bitflip_threshold.


> 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


I guess I should have performed this test too :)


>  - check if per-partition customization of bitflip_threshold through sysfs works
> Everything worked as expected.


Thanks once again Ivan.  I'll follow up with a new version of the patch set in a
day or too.

Mike

Patch

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;