Patchwork [U-Boot,1/3] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

login
register
mail settings
Submitter Paul Burton
Date Aug. 6, 2013, 10:13 a.m.
Message ID <1375784029-9310-2-git-send-email-paul.burton@imgtec.com>
Download mbox | patch
Permalink /patch/264998/
State Superseded
Delegated to: Stefan Roese
Headers show

Comments

Paul Burton - Aug. 6, 2013, 10:13 a.m.
Linux modified the MTD driver interface in commit edbc4540 (with the
same name as this commit). The effect is that calls to mtd_read will not
return -EUCLEAN if the number of ECC-corrected bit errors is below a
certain threshold, which defaults to the strength of the ECC. This
allows -EUCLEAN to stop indicating "some bits were corrected" and begin
indicating "a large number of bits were corrected, the data held in this
region of flash may be lost soon". UBI makes use of this and when
-EUCLEAN is returned from mtd_read it will move data to another block of
flash. Without adopting this interface change UBI on U-boot attempts to
move data between blocks every time a single bit is corrected using the
ECC, which is a very common occurance on some devices. For some devices
it can be so common that UBI gets stuck constantly moving data around
because each block it attempts to use has a single bit error.  This
patch adopts the interface change as in Linux commit edbc4540 in order
to avoid such situations.

Given that none of the drivers under drivers/mtd return -EUCLEAN, this
should only affect those using software ECC. I have tested that it works
on a board which is currently out of tree, but which I hope to be able
to begin upstreaming soon.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---
 drivers/mtd/mtdcore.c              | 14 +++++++++++++-
 drivers/mtd/mtdpart.c              | 12 ++++++------
 drivers/mtd/nand/nand_base.c       | 18 ++++++++++++++----
 drivers/mtd/onenand/onenand_base.c |  3 ++-
 include/linux/mtd/nand.h           |  3 +++
 5 files changed, 38 insertions(+), 12 deletions(-)
Stefan Roese - Aug. 19, 2013, 8:55 a.m.
On 06.08.2013 12:13, Paul Burton wrote:
> Linux modified the MTD driver interface in commit edbc4540 (with the
> same name as this commit). The effect is that calls to mtd_read will not
> return -EUCLEAN if the number of ECC-corrected bit errors is below a
> certain threshold, which defaults to the strength of the ECC. This
> allows -EUCLEAN to stop indicating "some bits were corrected" and begin
> indicating "a large number of bits were corrected, the data held in this
> region of flash may be lost soon". UBI makes use of this and when
> -EUCLEAN is returned from mtd_read it will move data to another block of
> flash. Without adopting this interface change UBI on U-boot attempts to
> move data between blocks every time a single bit is corrected using the
> ECC, which is a very common occurance on some devices. For some devices
> it can be so common that UBI gets stuck constantly moving data around
> because each block it attempts to use has a single bit error.  This
> patch adopts the interface change as in Linux commit edbc4540 in order
> to avoid such situations.
> 
> Given that none of the drivers under drivers/mtd return -EUCLEAN, this
> should only affect those using software ECC. I have tested that it works
> on a board which is currently out of tree, but which I hope to be able
> to begin upstreaming soon.

Paul, a quick question to clarify this. This patch fixes a regression in
the current mtd->_read implementation (used by UBI) that was introduced
by the MTD sync to v3.7.1? Is this correct?

And what error exactly did occur on your system?

Thanks,
Stefan
Scott Wood - Aug. 19, 2013, 4:22 p.m.
On Mon, 2013-08-19 at 10:55 +0200, Stefan Roese wrote:
> On 06.08.2013 12:13, Paul Burton wrote:
> > Linux modified the MTD driver interface in commit edbc4540 (with the
> > same name as this commit). The effect is that calls to mtd_read will not
> > return -EUCLEAN if the number of ECC-corrected bit errors is below a
> > certain threshold, which defaults to the strength of the ECC. This
> > allows -EUCLEAN to stop indicating "some bits were corrected" and begin
> > indicating "a large number of bits were corrected, the data held in this
> > region of flash may be lost soon". UBI makes use of this and when
> > -EUCLEAN is returned from mtd_read it will move data to another block of
> > flash. Without adopting this interface change UBI on U-boot attempts to
> > move data between blocks every time a single bit is corrected using the
> > ECC, which is a very common occurance on some devices. For some devices
> > it can be so common that UBI gets stuck constantly moving data around
> > because each block it attempts to use has a single bit error.  This
> > patch adopts the interface change as in Linux commit edbc4540 in order
> > to avoid such situations.
> > 
> > Given that none of the drivers under drivers/mtd return -EUCLEAN, this
> > should only affect those using software ECC. I have tested that it works
> > on a board which is currently out of tree, but which I hope to be able
> > to begin upstreaming soon.
> 
> Paul, a quick question to clarify this. This patch fixes a regression in
> the current mtd->_read implementation (used by UBI) that was introduced
> by the MTD sync to v3.7.1? Is this correct?
> 
> And what error exactly did occur on your system?

From the description it sounds like it's not a regression but rather an
improvement that wasn't possible before the sync.

-Scott
Paul Burton - Aug. 20, 2013, 9:53 a.m.
I'm not entirely sure whether it's a regression from the MTD merge or 
not, I only started adding support for my board in the past few months 
so I haven't tried older versions. From a glance at the history I 
suspect it might have always been possible, but since it only affects 
setups using software ECC with UBI nobody hit it before. Indeed I've 
since switched to using hardware ECC for my board so it wouldn't hit it 
any more either.

I can't remember the exact call chain off the top of my head, but it 
essentially led to UBI constantly scrubbing PEBs since they often 
(almost always) had some small number of correctable errors. It happened 
enough that the boot just appeared to hang. Prior to the patch a single 
bit flip caused mtd to return -EUCLEAN signalling to UBI that the data 
is potentially at risk and leading it to start scrubbing. In reality 
since a single bit flip is fine there's no need to.

I can switch to software ECC & without my patch to rediscover the exact 
call chain if you require, but it'll probably be a while before I do - 
busy week!

Thanks,
     Paul

On 19/08/13 09:55, Stefan Roese wrote:
> On 06.08.2013 12:13, Paul Burton wrote:
>> Linux modified the MTD driver interface in commit edbc4540 (with the
>> same name as this commit). The effect is that calls to mtd_read will not
>> return -EUCLEAN if the number of ECC-corrected bit errors is below a
>> certain threshold, which defaults to the strength of the ECC. This
>> allows -EUCLEAN to stop indicating "some bits were corrected" and begin
>> indicating "a large number of bits were corrected, the data held in this
>> region of flash may be lost soon". UBI makes use of this and when
>> -EUCLEAN is returned from mtd_read it will move data to another block of
>> flash. Without adopting this interface change UBI on U-boot attempts to
>> move data between blocks every time a single bit is corrected using the
>> ECC, which is a very common occurance on some devices. For some devices
>> it can be so common that UBI gets stuck constantly moving data around
>> because each block it attempts to use has a single bit error.  This
>> patch adopts the interface change as in Linux commit edbc4540 in order
>> to avoid such situations.
>>
>> Given that none of the drivers under drivers/mtd return -EUCLEAN, this
>> should only affect those using software ECC. I have tested that it works
>> on a board which is currently out of tree, but which I hope to be able
>> to begin upstreaming soon.
> Paul, a quick question to clarify this. This patch fixes a regression in
> the current mtd->_read implementation (used by UBI) that was introduced
> by the MTD sync to v3.7.1? Is this correct?
>
> And what error exactly did occur on your system?
>
> Thanks,
> Stefan
>

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 49c0814..deda5f2 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -217,11 +217,23 @@  int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
 int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
 	     u_char *buf)
 {
+	int ret_code;
 	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 ecc region (if applicable; zero otherwise).
+	 */
+	ret_code = mtd->_read(mtd, from, len, retlen, buf);
+	if (unlikely(ret_code < 0))
+		return ret_code;
+	if (mtd->ecc_strength == 0)
+		return 0;	/* device lacks ecc */
+	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
 }
 
 int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 9dfe7bb..146ce11 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -53,12 +53,12 @@  static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
 
 	stats = part->master->ecc_stats;
 	res = mtd_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;
 }
 
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 9e05cef..d4d586c 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1238,6 +1238,7 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 		mtd->oobavail : mtd->oobsize;
 
 	uint8_t *bufpoi, *oob, *buf;
+	unsigned int max_bitflips = 0;
 
 	stats = mtd->ecc_stats;
 
@@ -1265,7 +1266,10 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 
 			chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
 
-			/* Now read the page into the buffer */
+			/*
+			 * Now read the page into the buffer.  Absent an error,
+			 * the read methods return max bitflips per ecc step.
+			 */
 			if (unlikely(ops->mode == MTD_OPS_RAW))
 				ret = chip->ecc.read_page_raw(mtd, chip, bufpoi,
 							      oob_required,
@@ -1284,15 +1288,19 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				break;
 			}
 
+			max_bitflips = max_t(unsigned int, max_bitflips, ret);
+
 			/* Transfer not aligned data */
 			if (!aligned) {
 				if (!NAND_HAS_SUBPAGE_READ(chip) && !oob &&
 				    !(mtd->ecc_stats.failed - stats.failed) &&
-				    (ops->mode != MTD_OPS_RAW))
+				    (ops->mode != MTD_OPS_RAW)) {
 					chip->pagebuf = realpage;
-				else
+					chip->pagebuf_bitflips = ret;
+				} else {
 					/* Invalidate page cache */
 					chip->pagebuf = -1;
+				}
 				memcpy(buf, chip->buffers->databuf + col, bytes);
 			}
 
@@ -1310,6 +1318,8 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 		} else {
 			memcpy(buf, chip->buffers->databuf + col, bytes);
 			buf += bytes;
+			max_bitflips = max_t(unsigned int, max_bitflips,
+					     chip->pagebuf_bitflips);
 		}
 
 		readlen -= bytes;
@@ -1341,7 +1351,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;
 }
 
 /**
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index ddfe7e7..067f8ef 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -969,7 +969,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 ecc step; ONENANDs correct 1 bit only */
+	return mtd->ecc_stats.corrected != stats.corrected ? 1 : 0;
 }
 
 /**
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 2055584..0546565 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -464,6 +464,8 @@  struct nand_buffers {
  * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
  * @pagebuf:		[INTERN] holds the pagenumber which is currently in
  *			data_buf.
+ * @pagebuf_bitflips:	[INTERN] holds the bitflip count for the page which is
+ *			currently in data_buf.
  * @subpagesize:	[INTERN] holds the subpagesize
  * @onfi_version:	[INTERN] holds the chip ONFI version (BCD encoded),
  *			non 0 if ONFI supported.
@@ -531,6 +533,7 @@  struct nand_chip {
 	uint64_t chipsize;
 	int pagemask;
 	int pagebuf;
+	unsigned int pagebuf_bitflips;
 	int subpagesize;
 	uint8_t cellinfo;
 	int badblockpos;