[PATCH 0/2] Fix double counting of S/W ECC engines' ECC stat

[PATCH 0/2] Fix double counting of S/W ECC engines' ECC stat

[YouChing Lin]

Hello,

This series fix the double counting of S/W ECC engines' ECC stat 

If you use SPI-NAND with SW-ECC engine, the ECC related statistics
(ecc_stats.failed & ecc_stats.corrected) will be doubled, because
those numbers will be double-counted in ecc-sw-[bch/hamming].c and
drivers/mtd/nand/spi/core.c.
    
This can be found by using nandtest/nandbiterrs validation.

Thanks for your time.

YouChing Lin (2):
  mtd: nand: ecc-bch: Fix the double counting of ECC stat
  mtd: nand: ecc-hamming: Fix the double counting of ECC stat

 drivers/mtd/nand/ecc-sw-bch.c     | 9 +++++----
 drivers/mtd/nand/ecc-sw-hamming.c | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

-- 
1.9.1

[PATCH 1/2] mtd: nand: ecc-bch: Fix the double counting of ECC stat

[YouChing Lin]

If you use SPI-NAND with SW-ECC engine, the ECC related statistics
(ecc_stats.failed & ecc_stats.corrected) will be doubled, because
those numbers will be double-counted in ecc-sw-bch.c and
drivers/mtd/nand/spi/core.c.

This can be found by using nandtest/nandbiterrs validation.

Signed-off-by: YouChing Lin <ycllin@mxic.com.tw>
---
 drivers/mtd/nand/ecc-sw-bch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/ecc-sw-bch.c b/drivers/mtd/nand/ecc-sw-bch.c
index 405552d..aade381 100644
--- a/drivers/mtd/nand/ecc-sw-bch.c
+++ b/drivers/mtd/nand/ecc-sw-bch.c
@@ -372,11 +372,12 @@ static int nand_ecc_sw_bch_finish_io_req(struct nand_device *nand,
 						    &ecccode[i],
 						    &ecccalc[i]);
 		if (stat < 0) {
-			mtd->ecc_stats.failed++;
-		} else {
-			mtd->ecc_stats.corrected += stat;
-			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+			nand_ecc_restore_req(&engine_conf->req_ctx, req);
+
+			return -EBADMSG;
 		}
+
+		max_bitflips = max_t(unsigned int, max_bitflips, stat);
 	}
 
 	nand_ecc_restore_req(&engine_conf->req_ctx, req);
-- 
1.9.1

[PATCH 2/2] mtd: nand: ecc-hamming: Fix the double counting of ECC stat

[YouChing Lin]

If you use SPI-NAND with SW-ECC engine, the ECC related statistics
(ecc_stats.failed & ecc_stats.corrected) will be doubled, because
those numbers will be double-counted in ecc-sw-hamming.c and
drivers/mtd/nand/spi/core.c.

This can be found by using nandtest/nandbiterrs validation.

Signed-off-by: YouChing Lin <ycllin@mxic.com.tw>
---
 drivers/mtd/nand/ecc-sw-hamming.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/ecc-sw-hamming.c b/drivers/mtd/nand/ecc-sw-hamming.c
index a7655b6..e8a30cf 100644
--- a/drivers/mtd/nand/ecc-sw-hamming.c
+++ b/drivers/mtd/nand/ecc-sw-hamming.c
@@ -625,11 +625,12 @@ static int nand_ecc_sw_hamming_finish_io_req(struct nand_device *nand,
 							&ecccode[i],
 							&ecccalc[i]);
 		if (stat < 0) {
-			mtd->ecc_stats.failed++;
-		} else {
-			mtd->ecc_stats.corrected += stat;
-			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+			nand_ecc_restore_req(&engine_conf->req_ctx, req);
+
+			return -EBADMSG;
 		}
+
+		max_bitflips = max_t(unsigned int, max_bitflips, stat);
 	}
 
 	nand_ecc_restore_req(&engine_conf->req_ctx, req);
-- 
1.9.1

Re: [PATCH 0/2] Fix double counting of S/W ECC engines' ECC stat

[Miquel Raynal]

Hi YouChing,

YouChing Lin <ycllin@mxic.com.tw> wrote on Tue, 11 May 2021 09:40:33
+0800:

> Hello,
> 
> This series fix the double counting of S/W ECC engines' ECC stat 
> 
> If you use SPI-NAND with SW-ECC engine, the ECC related statistics
> (ecc_stats.failed & ecc_stats.corrected) will be doubled, because
> those numbers will be double-counted in ecc-sw-[bch/hamming].c and
> drivers/mtd/nand/spi/core.c.
>     
> This can be found by using nandtest/nandbiterrs validation.

Good catch!

However I don't think the current fix is valid because these engines
are meant to be used by the raw NAND core as well, I propose something
like the below, can you please tell me if it works as expected? (not
even build tested)

Thanks,
Miquèl


----8<----

Author: Miquel Raynal <miquel.raynal@bootlin.com>
Date:   Tue May 11 10:41:56 2021 +0200

    mtd: spinand: Fix double counting of ECC stats
    
    In the raw NAND world, ECC engines increment ecc_stats and the final
    caller is responsible for returning -EBADMSG if the verification
    failed.
    
    In the SPI-NAND world it was a bit different until now because there was
    only one possible ECC engine: the on-die one. Indeed, the
    spinand_mtd_read() call was incrementing the ecc_stats counters
    depending on the outcome of spinand_check_ecc_status() directly.
    
    So now let's split the logic like this:
    - spinand_check_ecc_status() is specific to the SPI-NAND on-die engine
      and is kept very simple: it just returns the ECC status (bonus point:
      the content of this helper can be overloaded).
    - spinand_ondie_ecc_finish_io_req() is the caller of
      spinand_check_ecc_status() and will increment the counters and
      eventually return -EBADMSG.
    - spinand_mtd_read() is not tied to the on-die ECC implementation and
      should be able to handle results coming from other ECC engines: it has
      the responsibility of returning the maximum number of bitflips which
      happened during the entire operation as this is the only helper that
      is aware that several pages may be read in a row.
    
    Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 61d932c1b718..df134c61853e 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -290,6 +290,8 @@ static int spinand_ondie_ecc_finish_io_req(struct nand_device *nand,
 {
        struct spinand_ondie_ecc_conf *engine_conf = nand->ecc.ctx.priv;
        struct spinand_device *spinand = nand_to_spinand(nand);
+       struct mtd_info *mtd = spinand_to_mtd(spinand);
+       int ret;
 
        if (req->mode == MTD_OPS_RAW)
                return 0;
@@ -299,7 +301,13 @@ static int spinand_ondie_ecc_finish_io_req(struct nand_device *nand,
                return 0;
 
        /* Finish a page write: check the status, report errors/bitflips */
-       return spinand_check_ecc_status(spinand, engine_conf->status);
+       ret = spinand_check_ecc_status(spinand, engine_conf->status);
+       if (ret == -EBADMSG)
+               mtd->ecc_stats.failed++;
+       else if (ret > 0)
+               mtd->ecc_stats.corrected += ret;
+
+       return ret;
 }
 
 static struct nand_ecc_engine_ops spinand_ondie_ecc_engine_ops = {
@@ -620,13 +628,10 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
                if (ret < 0 && ret != -EBADMSG)
                        break;
 
-               if (ret == -EBADMSG) {
+               if (ret == -EBADMSG)
                        ecc_failed = true;
-                       mtd->ecc_stats.failed++;
-               } else {
-                       mtd->ecc_stats.corrected += ret;
+               else
                        max_bitflips = max_t(unsigned int, max_bitflips, ret);
-               }
 
                ret = 0;
                ops->retlen += iter.req.datalen;

Re: [PATCH 0/2] Fix double counting of S/W ECC engines' ECC stat

[ycllin]

> "Miquel Raynal" <miquel.raynal@bootlin.com> 
<deleted>
> 
> Good catch!
> 
> However I don't think the current fix is valid because these engines
> are meant to be used by the raw NAND core as well, I propose something
> like the below, can you please tell me if it works as expected? (not
> even build tested)
> 
> Thanks,
> Miquèl
> 
> 
<deleted>

Thanks for your work.

I tested the two patches(yours and mine) separately in our environment: 
1) MXIC NFC(&raw NAND),2) MXIC SPI host(&SPI-NAND) with S/W BCH engine. 
Both patches are valid(using nandtest/nandbiterrs, values of ecc_stats are 
normal).

This seems to be because the function(nand_ecc_sw_bch_finish_io_req() 
in ecc-sw-bch.c) that would increase the ecc_stats counter is not used 
in the raw NAND world. Am I misunderstanding or is it platform dependency?

BTW, I think your modification should be more in line with the design 
spirit 
of generic ECC engine framework.

Thanks,
YouChing

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

Re: [PATCH 0/2] Fix double counting of S/W ECC engines' ECC stat

[Miquel Raynal]

Hi YouChing,

ycllin@mxic.com.tw wrote on Thu, 13 May 2021 10:11:02 +0800:

> > "Miquel Raynal" <miquel.raynal@bootlin.com>   
> <deleted>
> > 
> > Good catch!
> > 
> > However I don't think the current fix is valid because these engines
> > are meant to be used by the raw NAND core as well, I propose something
> > like the below, can you please tell me if it works as expected? (not
> > even build tested)
> > 
> > Thanks,
> > Miquèl
> > 
> >   
> <deleted>
> 
> Thanks for your work.
> 
> I tested the two patches(yours and mine) separately in our environment: 
> 1) MXIC NFC(&raw NAND),2) MXIC SPI host(&SPI-NAND) with S/W BCH engine. 
> Both patches are valid(using nandtest/nandbiterrs, values of ecc_stats are 
> normal).
> 
> This seems to be because the function(nand_ecc_sw_bch_finish_io_req() 
> in ecc-sw-bch.c) that would increase the ecc_stats counter is not used 
> in the raw NAND world. Am I misunderstanding or is it platform dependency?

I don't think it can be called a platform dependency, it's more like
legacy from the raw NAND world which makes the use of the generic ECC
framework hard and thus is limited to a given set of functions.

> BTW, I think your modification should be more in line with the design 
> spirit 
> of generic ECC engine framework.

Yes, ideally raw NAND should fully comply to this framework but this
would require a hundred days of work and dozens of available boards to
test. During the past 20 years people assumed NAND controller and ECC
engine were a single entity which makes the use of the generic ECC
framework hard to implemented in the raw NAND. So I decided not to put
all my energy there in order to first get this framework available to
SPI-NAND devices.

Thanks,
Miquèl

Re: [PATCH 0/2] Fix double counting of S/W ECC engines' ECC stat

[ycllin]

Hi Miquèl,

> "Miquel Raynal" <miquel.raynal@bootlin.com> 
> Re: [PATCH 0/2] Fix double counting of S/W ECC engines' ECC stat
> 
> Hi YouChing,
> 
> ycllin@mxic.com.tw wrote on Thu, 13 May 2021 10:11:02 +0800:
> 
> > > "Miquel Raynal" <miquel.raynal@bootlin.com> 
> > <deleted>
> > > 
> > > Good catch!
> > > 
> > > However I don't think the current fix is valid because these engines
> > > are meant to be used by the raw NAND core as well, I propose 
something
> > > like the below, can you please tell me if it works as expected? (not
> > > even build tested)
> > > 
> > > Thanks,
> > > Miquèl
> > > 
> > > 
> > <deleted>
> > 
> > Thanks for your work.
> > 
> > I tested the two patches(yours and mine) separately in our 
environment: 
> > 1) MXIC NFC(&raw NAND),2) MXIC SPI host(&SPI-NAND) with S/W BCH 
engine. 
> > Both patches are valid(using nandtest/nandbiterrs, values of ecc_stats 
are 
> > normal).
> > 
> > This seems to be because the function(nand_ecc_sw_bch_finish_io_req() 
> > in ecc-sw-bch.c) that would increase the ecc_stats counter is not used 

> > in the raw NAND world. Am I misunderstanding or is it platform 
dependency?
> 
> I don't think it can be called a platform dependency, it's more like
> legacy from the raw NAND world which makes the use of the generic ECC
> framework hard and thus is limited to a given set of functions.
> 
> > BTW, I think your modification should be more in line with the design 
> > spirit 
> > of generic ECC engine framework.
> 
> Yes, ideally raw NAND should fully comply to this framework but this
> would require a hundred days of work and dozens of available boards to
> test. During the past 20 years people assumed NAND controller and ECC
> engine were a single entity which makes the use of the generic ECC
> framework hard to implemented in the raw NAND. So I decided not to put
> all my energy there in order to first get this framework available to
> SPI-NAND devices.

Thanks for your detailed explanation. 
I agree with your change and it works fine in SPI-NAND world 
(with on-die or S/W ECC engines) and raw NAND world(with S/W ECC engines).

Tested-by: YouChing Lin <ycllin@mxic.com.tw>


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================