| Message ID | 20180108211542.11891-2-boris.brezillon@free-electrons.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | mtd: Preparation patches for the SPI NAND framework | expand |
On Mon, 8 Jan 2018 22:15:40 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > part_read() and part_read_oob() where counting ECC failures and > bitflips differently. Adjust part_read_oob() to mimic what is done in > part_read(). This is in needed to use ->_read_oob() as a fallback when > when ->_read() is not implemented. > > Note that bitflips and ECC failure accounting on MTD partitions is > broken by design, because nothing prevents concurrent accesses to the > underlying master MTD device between the moment we save the stats in a > local variable and the moment master->_read[_oob]() returns. It's not > something that can easily be fixed, so leave it like that for now. > > Suggested-by: Brian Norris <computersforpeace@gmail.com> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > Changes in v4: > - new patch > --- > drivers/mtd/mtdpart.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index be088bccd593..283e8526713b 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -105,6 +105,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, > struct mtd_oob_ops *ops) > { > struct mtd_part *part = mtd_to_part(mtd); > + struct mtd_ecc_stats stats; > int res; > > if (from >= mtd->size) > @@ -127,12 +128,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, > } > Missing stats = part->parent->ecc_stats; here. I'll send a new version. Sorry for the inconvenience. > res = part->parent->_read_oob(part->parent, 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->parent->ecc_stats.failed - stats.failed; > + else > + mtd->ecc_stats.corrected += > + part->parent->ecc_stats.corrected - stats.corrected; > return res; > } >
Boris Brezillon <boris.brezillon@free-electrons.com> writes: > part_read() and part_read_oob() where counting ECC failures and Typo: s/where/were. > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index be088bccd593..283e8526713b 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -105,6 +105,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, > struct mtd_oob_ops *ops) > { > struct mtd_part *part = mtd_to_part(mtd); > + struct mtd_ecc_stats stats; > int res; > > if (from >= mtd->size) > @@ -127,12 +128,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, > } > > res = part->parent->_read_oob(part->parent, 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->parent->ecc_stats.failed - stats.failed; stats is used uninitialized, but I suppose you've already caught that. Cheers.
On Wed, 10 Jan 2018 21:27:34 +0100 Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > > > part_read() and part_read_oob() where counting ECC failures and > Typo: s/where/were. I'll fix the typo. > > > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > > index be088bccd593..283e8526713b 100644 > > --- a/drivers/mtd/mtdpart.c > > +++ b/drivers/mtd/mtdpart.c > > @@ -105,6 +105,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, > > struct mtd_oob_ops *ops) > > { > > struct mtd_part *part = mtd_to_part(mtd); > > + struct mtd_ecc_stats stats; > > int res; > > > > if (from >= mtd->size) > > @@ -127,12 +128,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, > > } > > > > res = part->parent->_read_oob(part->parent, 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->parent->ecc_stats.failed - stats.failed; > stats is used uninitialized, but I suppose you've already caught that. Yep, already fixed in v5. Thanks, Boris
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index be088bccd593..283e8526713b 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -105,6 +105,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops) { struct mtd_part *part = mtd_to_part(mtd); + struct mtd_ecc_stats stats; int res; if (from >= mtd->size) @@ -127,12 +128,12 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, } res = part->parent->_read_oob(part->parent, 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->parent->ecc_stats.failed - stats.failed; + else + mtd->ecc_stats.corrected += + part->parent->ecc_stats.corrected - stats.corrected; return res; }
part_read() and part_read_oob() where counting ECC failures and bitflips differently. Adjust part_read_oob() to mimic what is done in part_read(). This is in needed to use ->_read_oob() as a fallback when when ->_read() is not implemented. Note that bitflips and ECC failure accounting on MTD partitions is broken by design, because nothing prevents concurrent accesses to the underlying master MTD device between the moment we save the stats in a local variable and the moment master->_read[_oob]() returns. It's not something that can easily be fixed, so leave it like that for now. Suggested-by: Brian Norris <computersforpeace@gmail.com> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- Changes in v4: - new patch --- drivers/mtd/mtdpart.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)