[PATCH 4/4] MTD: drivers return max_bitflips, mtd returns -EUCLEAN
computersforpeace at gmail.com
Thu Mar 29 13:30:40 EDT 2012
On Sun, Mar 11, 2012 at 2:21 PM, Mike Dunn <mikedunn at newsguy.com> wrote:
> The drivers' read methods, absent an error, return a non-negative integer
> indicating the maximum number of bit errors that were corrected in any one
> writesize region. MTD returns -EUCLEAN if this is >= euclean_threshold, 0
> otherwise. Note that this is a subtle change to the driver interface.
> This and the preceeding patches in this set were tested ubi on top of the
> nandsim and docg4 devices, running the ubi test io_basic from mtd-utils.
Did you test any non-NAND devices? I'm getting the feeling that some
of this is misguided or at least needs some fixing. See below.
> We may want to also change the name of the macro mtd_is_bitflip(), since this is
> not really correctly named anymore.
What sort of name change? Shouldn't EUCLEAN still mean bitflip? We are
just masking the bitflips below a threshold. Anyway, if you have a
better name, bring it up! I think I changed the name from my original
choice based on Artem's suggestions, so I'm open to anything with more
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 5bbd717..6871658 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -789,12 +789,24 @@ EXPORT_SYMBOL_GPL(mtd_get_unmapped_area);
> int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
> u_char *buf)
> + int ret_code;
> *retlen = 0;
> if (from < 0 || from > mtd->size || len > mtd->size - from)
> return -EINVAL;
> if (!len)
> return 0;
> - 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);
> + if (unlikely(ret_code < 0))
> + return ret_code;
> + return ret_code >= mtd->euclean_threshold ? -EUCLEAN : 0;
This seems wrong in the case that mtd->euclean_threshold == 0. This
could be the case for any of the following:
(1) NAND that uses ECC_NONE
(2) NAND drivers with HW_ECC that don't initialize chip->ecc.strength
(3) MTD without ECC (e.g., NOR)
(4) Other drivers that might have missed initializing mtd->ecc_strength
Regarding (1): although ECC_NONE is not recommended, it's possible. I
don't think this deserves an EUCLEAN message.
Regarding (2): I notice that your nand_base.c code (patch 2) leaves
ecc.strength initialization up to the driver. I think a sanity check
could be provided in those cases. For instance, in the NAND_ECC_HW*
cases, you could warn or error out on 'chip->ecc.strength == 0'.
If I'm correct, (3) is quite significant, since non-ECC'd MTDs would
produce EUCLEAN statuses on every read.
More information about the linux-mtd