| Message ID | 1331500873-9792-5-git-send-email-mikedunn@newsguy.com |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 11 Mar 2012 14:21:13 -0700 Mike Dunn <mikedunn@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
On Wed, 14 Mar 2012 13:05:43 +0200 Shmulik Ladkani <shmulik.ladkani@gmail.com> wrote: > > 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? Yes, I missed something and got it wrong. Further meditation enlightened me. No need to reply on this comment. Sorry. Regards, Shmulik
On Sun, Mar 11, 2012 at 2:21 PM, Mike Dunn <mikedunn@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 merit :) > 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. Brian
On Thu, 2012-03-29 at 10:30 -0700, Brian Norris wrote: > On Sun, Mar 11, 2012 at 2:21 PM, Mike Dunn <mikedunn@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. Brian, you are right, these 2 should be reverted and Mike should come back with a better patch-set. I was not careful enough, and thanks for the review.
On 03/29/2012 10:30 AM, Brian Norris wrote: > On Sun, Mar 11, 2012 at 2:21 PM, Mike Dunn <mikedunn@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. No, not tested on non-nand yet. > >> 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 > merit :) > >> 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. You are correct! Thanks. Devices with no ecc (for which euclean_threshold == 0) will always return 0 (absent an error), which will result in mtd always returning -EUCLEAN. Oops! Thanks again. > 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'. Yes, I'll include sanity checks in next patches. > If I'm correct, (3) is quite significant, since non-ECC'd MTDs would > produce EUCLEAN statuses on every read. Yup. Again, oops! Artem, what are your thoughts on fixing these patches, given that many nand drivers will have to be touched? I'm willing to see it through. Thanks again Brian, Mike
On 03/30/2012 05:13 AM, Artem Bityutskiy wrote: > On Thu, 2012-03-29 at 10:30 -0700, Brian Norris wrote: >> 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. > > you are right, these 2 should be reverted and Mike should come back with > a better patch-set. I was not careful enough, and thanks for the review. At least you were not the author. Yes, thank you Brian. Artem, I don't think you pushed the offending patches (3 and 4). If you want to leave the first two patches in, I'll be happy to fix them (they currently don't do anything) when the whole efort is done. Or else you can pull them and I'll start from scratch. Or slink away in shame :) But I'd prefer to get this done right. Mike
Hi Mike, On Fri, 30 Mar 2012 18:05:55 -0700 Mike Dunn <mikedunn@newsguy.com> wrote: > > 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 > > > > > > If I'm correct, (3) is quite significant, since non-ECC'd MTDs would > > produce EUCLEAN statuses on every read. > > > Yup. Again, oops! > Please re-consider having the 'euclean_threshold' comparison within the NAND infrastructure (nand_base.c and clones), instead of within the generic 'mtd_read()' wrapper, as discussed in [1]. This would inherently fix (3). For code unification, we can have a small inlined function that does the comparison, which may be used by nand_base.c and the clones. [1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040371.html http://lists.infradead.org/pipermail/linux-mtd/2012-March/040343.html Regards, Shmulik
On Fri, 2012-03-30 at 18:17 -0700, Mike Dunn wrote: > Artem, I don't think you pushed the offending patches (3 and 4). If you want to > leave the first two patches in, I'll be happy to fix them (they currently don't > do anything) when the whole efort is done. Or else you can pull them and I'll > start from scratch. Or slink away in shame :) But I'd prefer to get this done > right. Yeah, I won't touch them so far and will just wait for incremental patches.
On 04/02/2012 12:17 AM, Artem Bityutskiy wrote: > On Fri, 2012-03-30 at 18:17 -0700, Mike Dunn wrote: >> Artem, I don't think you pushed the offending patches (3 and 4). If you want to >> leave the first two patches in, I'll be happy to fix them (they currently don't >> do anything) when the whole efort is done. Or else you can pull them and I'll >> start from scratch. Or slink away in shame :) But I'd prefer to get this done >> right. > > Yeah, I won't touch them so far and will just wait for incremental > patches. > OK, thanks Artem. I can start today and should have patches ready soon. Mike
Hi Shmulik, On 03/30/2012 11:37 PM, Shmulik Ladkani wrote: > Please re-consider having the 'euclean_threshold' comparison within the > NAND infrastructure (nand_base.c and clones), instead of within the > generic 'mtd_read()' wrapper, as discussed in [1]. This isn't possible given the intended functionality of the patches. The plan is to allow the user to adjust the bitflip_threshold through sysfs, and do it uniquely for each partition. Since the driver itself receives as its argument the mtd_info for the master, not the partition, it can't compare the bitflip count with the appropriate threshold to determine whether or not to return -EUCLEAN. Here, "driver" refers to the nand imfrastructure code or the drivers for the two nand-with-ecc devices that don't use the nand interface (nand/alauda.c and devices/docg3.c). Thanks, Mike
On Mon, 02 Apr 2012 09:40:08 -0700 Mike Dunn <mikedunn@newsguy.com> wrote: > Hi Shmulik, > > On 03/30/2012 11:37 PM, Shmulik Ladkani wrote: > > Please re-consider having the 'euclean_threshold' comparison within the > > NAND infrastructure (nand_base.c and clones), instead of within the > > generic 'mtd_read()' wrapper, as discussed in [1]. > > > This isn't possible given the intended functionality of the patches. The plan > is to allow the user to adjust the bitflip_threshold through sysfs, and do it > uniquely for each partition. Since the driver itself receives as its argument > the mtd_info for the master, not the partition, it can't compare the bitflip > count with the appropriate threshold to determine whether or not to return > -EUCLEAN. Ok. Now I understand why you submitted [l2-mtd ac5b1dd mtd: fix partition wrapper functions]... You rely on ac5b1dd in order for the comparison to be executed per partition. Reasnoable, however delicate. The reslationship between the 'euclean_threeshold' feature and this patch is implicit and not obvious. It raises a concern that one might later change the behavior of 'part_read' to use the mtd_read wrapper for some reason, without understanding the affects. Anyway, looking on it in a broader perspective, can you please state use-case examples where one might set different thresholds for different partitions of same device? I mean, do we really need that granularity? Regards, Shmulik
On Tue, 2012-04-03 at 11:48 +0300, Shmulik Ladkani wrote: > Anyway, looking on it in a broader perspective, can you please state > use-case examples where one might set different thresholds for > different > partitions of same device? > I mean, do we really need that granularity? Current MTD partitions support is very simple and makes it difficult to distinguished between partitions and not partitions.
Sorry Shmulik, I didn't get your post again. (Why?? I get a blizzard of emails from various lists, but seem to lose the important ones!) On 04/13/2012 08:54 AM, Artem Bityutskiy wrote: > On Tue, 2012-04-03 at 11:48 +0300, Shmulik Ladkani wrote: >> Anyway, looking on it in a broader perspective, can you please state >> use-case examples where one might set different thresholds for >> different >> partitions of same device? >> I mean, do we really need that granularity? > > Current MTD partitions support is very simple and makes it difficult to > distinguished between partitions and not partitions. Yes, the way the mtd code is structured, I think going through the partitions is the correct way for the sake of cleanliness alone. BTW, I haven't forgotten this. I hope to have patches ready in a couple days. Thanks, Mike
diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c index 886ca0f..95ea33e 100644 --- a/drivers/mtd/devices/docg3.c +++ b/drivers/mtd/devices/docg3.c @@ -854,6 +854,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from, u8 *buf = ops->datbuf; size_t len, ooblen, nbdata, nboob; u8 hwecc[DOC_ECC_BCH_SIZE], eccconf1; + int max_bitflips = 0; if (buf) len = ops->len; @@ -938,7 +939,8 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from, } if (ret > 0) { mtd->ecc_stats.corrected += ret; - ret = -EUCLEAN; + max_bitflips = max(max_bitflips, ret); + ret = max_bitflips; } } 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; } EXPORT_SYMBOL_GPL(mtd_read); @@ -835,6 +847,27 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, } EXPORT_SYMBOL_GPL(mtd_panic_write); +int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops) +{ + int ret_code; + ops->retlen = ops->oobretlen = 0; + if (!mtd->_read_oob) + return -EOPNOTSUPP; + + /* + * 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_oob(mtd, from, ops); + + if (unlikely(ret_code < 0)) + return ret_code; + + return ret_code >= mtd->euclean_threshold ? -EUCLEAN : 0; +} +EXPORT_SYMBOL_GPL(mtd_read_oob); + /* * 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; return res; } @@ -108,6 +108,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops) { struct mtd_part *part = PART(mtd); + struct mtd_ecc_stats stats = part->master->ecc_stats; int res; if (from >= mtd->size) @@ -133,12 +134,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, } res = part->master->_read_oob(part->master, from + part->offset, ops); - if (unlikely(res)) { - if (mtd_is_bitflip(res)) - mtd->ecc_stats.corrected++; - if (mtd_is_eccerr(res)) - mtd->ecc_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; return res; } diff --git a/drivers/mtd/nand/alauda.c b/drivers/mtd/nand/alauda.c index 4f20e1d..7e59e83 100644 --- a/drivers/mtd/nand/alauda.c +++ b/drivers/mtd/nand/alauda.c @@ -414,7 +414,7 @@ static int alauda_bounce_read(struct mtd_info *mtd, loff_t from, size_t len, } err = 0; if (corrected) - err = -EUCLEAN; + err = 1; /* return max bitflips per page */ if (uncorrected) err = -EBADMSG; out: @@ -446,7 +446,7 @@ static int alauda_read(struct mtd_info *mtd, loff_t from, size_t len, } err = 0; if (corrected) - err = -EUCLEAN; + err = 1; /* return max bitflips per page */ if (uncorrected) err = -EBADMSG; return err; 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 @@ -1469,6 +1469,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, uint32_t oobreadlen = ops->ooblen; uint32_t max_oobsize = ops->mode == MTD_OPS_AUTO_OOB ? mtd->oobavail : mtd->oobsize; + unsigned int max_bitflips = 0; uint8_t *bufpoi, *oob, *buf; @@ -1491,6 +1492,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, /* Is the current page in the buffer? */ if (realpage != chip->pagebuf || oob) { + unsigned int prev_corrected = mtd->ecc_stats.corrected; + bufpoi = aligned ? buf : chip->buffers->databuf; if (likely(sndcmd)) { @@ -1553,6 +1556,9 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, else nand_wait_ready(mtd); } + max_bitflips = max(max_bitflips, + mtd->ecc_stats.corrected - + prev_corrected); } else { memcpy(buf, chip->buffers->databuf + col, bytes); buf += bytes; @@ -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; } /** @@ -1782,6 +1788,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, int readlen = ops->ooblen; int len; uint8_t *buf = ops->oobbuf; + unsigned int max_bitflips = 0; pr_debug("%s: from = 0x%08Lx, len = %i\n", __func__, (unsigned long long)from, readlen); @@ -1816,6 +1823,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, page = realpage & chip->pagemask; while (1) { + unsigned int prev_corrected = mtd->ecc_stats.corrected; if (ops->mode == MTD_OPS_RAW) sndcmd = chip->ecc.read_oob_raw(mtd, chip, page, sndcmd); else @@ -1836,6 +1844,8 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from, else nand_wait_ready(mtd); } + max_bitflips = max(max_bitflips, + mtd->ecc_stats.corrected - prev_corrected); readlen -= len; if (!readlen) @@ -1865,7 +1875,7 @@ static int nand_do_read_oob(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; } /** diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 3d781b8..177b0a5 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -1201,7 +1201,8 @@ static int onenand_mlc_read_ops_nolock(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 per page; ONENANDs correct 1 bit only */ + return mtd->ecc_stats.corrected != stats.corrected ? 1 : 0; } /** @@ -1333,7 +1334,8 @@ static int onenand_read_ops_nolock(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 per page; ONENANDs correct 1 bit only */ + return mtd->ecc_stats.corrected != stats.corrected ? 1 : 0; } /** diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 20b5c40..b0c78f4 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -262,15 +262,7 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf); int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf); - -static inline int mtd_read_oob(struct mtd_info *mtd, loff_t from, - struct mtd_oob_ops *ops) -{ - ops->retlen = ops->oobretlen = 0; - if (!mtd->_read_oob) - return -EOPNOTSUPP; - return mtd->_read_oob(mtd, from, ops); -} +int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops); static inline int mtd_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops)
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. Signed-off-by: Mike Dunn <mikedunn@newsguy.com> --- We may want to also change the name of the macro mtd_is_bitflip(), since this is not really correctly named anymore. drivers/mtd/devices/docg3.c | 4 +++- drivers/mtd/mtdcore.c | 35 ++++++++++++++++++++++++++++++++++- drivers/mtd/mtdpart.c | 25 +++++++++++++------------ drivers/mtd/nand/alauda.c | 4 ++-- drivers/mtd/nand/nand_base.c | 14 ++++++++++++-- drivers/mtd/onenand/onenand_base.c | 6 ++++-- include/linux/mtd/mtd.h | 10 +--------- 7 files changed, 69 insertions(+), 29 deletions(-)