| Message ID | 1335902181-18110-1-git-send-email-mikedunn@newsguy.com |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 1, 2012 at 12:56 PM, Mike Dunn <mikedunn@newsguy.com> wrote: > This patch replaces bitflip_threshold with ecc_strength as the value used to > determine whether the device has ecc capability. I think this is more correct > because bitflip_threshold can be changed via sysfs. This isn't the only reason. More simply, there's the reason that ecc_strength is the natural measure of "do I have ECC", not bitflip_threshold, even if we can often say that ecc_strength != 0 if and only if bitflip_threshold != 0. > Sorry for wanting to make tweaks after reviews and Ivan's testing. It's > optional, low risk, and I did test it. Thanks! Aside from the review and testing, this kind of "fixup" usually makes sense to just squash into the original patch ("mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN"), right? > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index b5bb628..5757307 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -815,7 +815,7 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, > ret_code = mtd->_read(mtd, from, len, retlen, buf); > if (unlikely(ret_code < 0)) > return ret_code; > - if (mtd->bitflip_threshold == 0) > + if (mtd->ecc_strength == 0) > return 0; /* device lacks ecc */ > return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0; > } I had meant to suggest this on the original patch but never got around to it. For this patch (on its own, or in combo with the patch it fixes up): Reviewed-by: Brian Norris <computersforpeace@gmail.com>
On Tue, 2012-05-01 at 12:56 -0700, Mike Dunn wrote: > This is a minor change to the recent EUCLEAN patch set that I think makes it > more correct. Currently, the test in mtd_read() that determines whether or not > to return -EUCLEAN is skipped if bitflip_threshold == 0. This logic was added > (without much thought) to fix a bug in an earlier version of the patch set that > would have caused devices lacking ecc to always return -EUCLEAN [1]. > > This patch replaces bitflip_threshold with ecc_strength as the value used to > determine whether the device has ecc capability. I think this is more correct > because bitflip_threshold can be changed via sysfs. Currently, if the user sets > it to zero, -EUCLEAN is never returned. This is also what happens if the user > sets it to a value exceeding ecc_strength. With this patch, if the user sets it > to zero, -EUCLEAN will always be returned (provided ecc_strength > 0), which > makes more sense, and may even be useful occasionally. > > [1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040468.html > > Signed-off-by: Mike Dunn <mikedunn@newsguy.com> Folded this patch into: mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN and pushed to l2-mtd.git, thanks!
diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd index 7883508..db1ad7e 100644 --- a/Documentation/ABI/testing/sysfs-class-mtd +++ b/Documentation/ABI/testing/sysfs-class-mtd @@ -167,7 +167,10 @@ Description: block degradation, but high enough to avoid the consequences of a persistent return value of -EUCLEAN on devices where sticky bitflips occur. Note that if bitflip_threshold exceeds - ecc_strength, -EUCLEAN is never returned by the read functions. + ecc_strength, -EUCLEAN is never returned by mtd_read(). + Conversely, if bitflip_threshold is zero, -EUCLEAN is always + returned, absent a hard error. This is generally applicable only to NAND flash devices with ECC - capability. It is ignored on devices lacking ECC capability. + capability. It is ignored on devices lacking ECC capability; + i.e., devices for which ecc_strength is zero. diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index b5bb628..5757307 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -815,7 +815,7 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, ret_code = mtd->_read(mtd, from, len, retlen, buf); if (unlikely(ret_code < 0)) return ret_code; - if (mtd->bitflip_threshold == 0) + if (mtd->ecc_strength == 0) return 0; /* device lacks ecc */ return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0; }
This is a minor change to the recent EUCLEAN patch set that I think makes it more correct. Currently, the test in mtd_read() that determines whether or not to return -EUCLEAN is skipped if bitflip_threshold == 0. This logic was added (without much thought) to fix a bug in an earlier version of the patch set that would have caused devices lacking ecc to always return -EUCLEAN [1]. This patch replaces bitflip_threshold with ecc_strength as the value used to determine whether the device has ecc capability. I think this is more correct because bitflip_threshold can be changed via sysfs. Currently, if the user sets it to zero, -EUCLEAN is never returned. This is also what happens if the user sets it to a value exceeding ecc_strength. With this patch, if the user sets it to zero, -EUCLEAN will always be returned (provided ecc_strength > 0), which makes more sense, and may even be useful occasionally. [1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040468.html Signed-off-by: Mike Dunn <mikedunn@newsguy.com> --- Sorry for wanting to make tweaks after reviews and Ivan's testing. It's optional, low risk, and I did test it. Thanks! Documentation/ABI/testing/sysfs-class-mtd | 7 +++++-- drivers/mtd/mtdcore.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-)