linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Masahiro Yamada <yamada.masahiro@socionext.com>
To: linux-mtd@lists.infradead.org
Cc: laurent.monat@idquantique.com,
	thorsten.christiansson@idquantique.com,
	Enrico Jorns <ejo@pengutronix.de>,
	Jason Roberts <jason.e.roberts@intel.com>,
	Artem Bityutskiy <artem.bityutskiy@linux.intel.com>,
	Dinh Nguyen <dinguyen@kernel.org>,
	Boris Brezillon <boris.brezillon@free-electrons.com>,
	Marek Vasut <marek.vasut@gmail.com>,
	Brian Norris <computersforpeace@gmail.com>,
	Graham Moore <grmoore@opensource.altera.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Chuanxiao Dong <chuanxiao.dong@intel.com>,
	Jassi Brar <jaswinder.singh@linaro.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	linux-kernel@vger.kernel.org, Richard Weinberger <richard@nod.at>,
	Cyrille Pitchen <cyrille.pitchen@atmel.com>
Subject: [PATCH v2 10/53] mtd: nand: denali: fix erased page checking
Date: Wed, 22 Mar 2017 23:07:17 +0900	[thread overview]
Message-ID: <1490191680-14481-11-git-send-email-yamada.masahiro@socionext.com> (raw)
In-Reply-To: <1490191680-14481-1-git-send-email-yamada.masahiro@socionext.com>

This part is wrong in multiple ways:

[1] is_erased() is called against "buf" twice, so the second one is
meaningless.  The second call should check chip->oob_poi.

[2] This code block is nested by double "if (check_erase_page)".
The inner one is redundant.

[3] Erased page checking without threshold is false-positive.
Basically, there are two ways for erased page checking:
- read the whole of page + oob in raw transfer, then check if all
  the data are 0xFF.
- read the ECC-corrected page + oob, then check if *almost* all the
  data are 0xFF (bit-flips less than ecc.strength are allowed)
While here, it checks if all data in ECC-corrected page are 0xFF.
This is too strong because not all of the data are 0xFF after they
are manipulated by the ECC engine.  Proper threshold must be taken
into account to avoid false-positive ecc_stats.failed increments.

[4] positive return value for uncorrectable bitflips

The comment of ecc->read_page() says it should return "0 if bitflips
uncorrectable", but the current code could return a positive value
in the case.

This commit solves the problems above.  The nand framework provides
a helper nand_check_erased_ecc_chunk() for erased page check with
threshold.  The driver's own helper is unneeded.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Squash some patches into one.
  - Use nand_check_erased_ecc_chunk() with threshold

 drivers/mtd/nand/denali.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 2c59eb3..86381ac 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -883,19 +883,6 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
 	}
 }
 
-/*
- * this function examines buffers to see if they contain data that
- * indicate that the buffer is part of an erased region of flash.
- */
-static bool is_erased(uint8_t *buf, int len)
-{
-	int i;
-
-	for (i = 0; i < len; i++)
-		if (buf[i] != 0xFF)
-			return false;
-	return true;
-}
 #define ECC_SECTOR_SIZE 512
 
 #define ECC_SECTOR(x)	(((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
@@ -1119,6 +1106,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	uint32_t irq_status;
 	uint32_t irq_mask = INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR;
 	bool check_erased_page = false;
+	int stat;
 
 	if (page != denali->page) {
 		dev_err(denali->dev,
@@ -1148,12 +1136,15 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	if (check_erased_page) {
 		read_oob_data(mtd, chip->oob_poi, denali->page);
 
-		/* check ECC failures that may have occurred on erased pages */
-		if (check_erased_page) {
-			if (!is_erased(buf, mtd->writesize))
-				mtd->ecc_stats.failed++;
-			if (!is_erased(buf, mtd->oobsize))
-				mtd->ecc_stats.failed++;
+		stat = nand_check_erased_ecc_chunk(
+					buf, mtd->writesize,
+					chip->oob_poi, mtd->oobsize,
+					NULL, 0,
+					chip->ecc.strength * chip->ecc.steps);
+		if (stat < 0) {
+			mtd->ecc_stats.failed++;
+			/* return 0 for uncorrectable bitflips */
+			max_bitflips = 0;
 		}
 	}
 	return max_bitflips;
-- 
2.7.4

  parent reply	other threads:[~2017-03-22 14:11 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-22 14:07 [PATCH v2 00/53] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 01/53] mtd: nand: allow to set only one of ECC size and ECC strength from DT Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 02/53] mtd: nand: use read_oob() instead of cmdfunc() for bad block check Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 03/53] mtd: nand: denali: remove unused CONFIG option and macros Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 04/53] mtd: nand: denali: remove redundant define of BANK(x) Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 05/53] mtd: nand: denali: remove more unused struct members Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 06/53] mtd: nand: denali: fix comment of denali_nand_info::flash_mem Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 07/53] mtd: nand: denali: consolidate INTR_STATUS__* and INTR_EN__* macros Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 08/53] mtd: nand: denali: introduce capability flag Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 09/53] mtd: nand: denali: use int where no reason to use fixed width variable Masahiro Yamada
2017-03-22 14:07 ` Masahiro Yamada [this message]
2017-03-22 20:36   ` [PATCH v2 10/53] mtd: nand: denali: fix erased page checking Boris Brezillon
2017-03-23  5:15     ` Masahiro Yamada
2017-03-23  8:03       ` Boris Brezillon
2017-03-22 20:56   ` Boris Brezillon
2017-03-23  5:04     ` Masahiro Yamada
2017-03-23  7:56       ` Boris Brezillon
2017-03-24  2:43         ` Masahiro Yamada
2017-03-24  8:06           ` Boris Brezillon
2017-03-22 14:07 ` [PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc() Masahiro Yamada
2017-03-22 20:57   ` Boris Brezillon
2017-03-23  7:02     ` Masahiro Yamada
2017-03-23  8:12       ` Boris Brezillon
2017-03-22 14:07 ` [PATCH v2 12/53] mtd: nand: denali: support HW_ECC_FIXUP capability Masahiro Yamada
2017-03-22 21:09   ` Boris Brezillon
2017-03-23  7:06     ` Masahiro Yamada
2017-03-23  8:16       ` Boris Brezillon
2017-03-22 21:12   ` Boris Brezillon
2017-03-23  7:05     ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 13/53] mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 14/53] mtd: nand: denali: support 64bit capable DMA engine Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 15/53] mtd: nand: denali_dt: remove dma-mask DT property Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 16/53] mtd: nand: denali_dt: use pdev instead of ofdev for platform_device Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 17/53] mtd: nand: denali: allow to override revision number Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 18/53] mtd: nand: denali: use nand_chip to hold frequently accessed data Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 19/53] mtd: nand: denali: call nand_set_flash_node() to set DT node Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 20/53] mtd: nand: denali: do not set mtd->name Masahiro Yamada
2017-03-27 15:31   ` Boris Brezillon
2017-03-28 21:32     ` Masahiro Yamada
2017-03-28 21:40       ` Boris Brezillon
2017-03-29  1:19         ` Masahiro Yamada
2017-03-29  7:19           ` Boris Brezillon
2017-03-29 11:30             ` Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 21/53] mtd: nand: denali: move multi device fixup code to a helper function Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 22/53] mtd: nand: denali: simplify multi device fixup code Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 23/53] mtd: nand: denali: set DEVICES_CONNECTED 1 if not set Masahiro Yamada
2017-03-22 14:07 ` [PATCH v2 24/53] mtd: nand: denali: remove meaningless writes to read-only registers Masahiro Yamada

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1490191680-14481-11-git-send-email-yamada.masahiro@socionext.com \
    --to=yamada.masahiro@socionext.com \
    --cc=artem.bityutskiy@linux.intel.com \
    --cc=boris.brezillon@free-electrons.com \
    --cc=chuanxiao.dong@intel.com \
    --cc=computersforpeace@gmail.com \
    --cc=cyrille.pitchen@atmel.com \
    --cc=dinguyen@kernel.org \
    --cc=dwmw2@infradead.org \
    --cc=ejo@pengutronix.de \
    --cc=grmoore@opensource.altera.com \
    --cc=jason.e.roberts@intel.com \
    --cc=jaswinder.singh@linaro.org \
    --cc=laurent.monat@idquantique.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=marek.vasut@gmail.com \
    --cc=mhiramat@kernel.org \
    --cc=richard@nod.at \
    --cc=thorsten.christiansson@idquantique.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).