[PATCH] MTD: drivers return bitlip info to mtd on read

[PATCH] MTD: drivers return bitlip info to mtd on read

[Mike Dunn]

This patch changes the meaning of the value returned by the read() and
read_oob() mtd driver methods.  Previously, absent a hard error, these functions
returned either -EUCLEAN (one or more bitflips were corrected) or 0 (no
bitflips).  Drivers now return, absent a hard error, the maximum number of
bitflips that were corrected on any one page.  

This change is made possible by the fact that all calls to the driver methods
now go through mtd wrapper functions.  The values returned by those wrapper
functions have not changed, nor have their meaning.  Only the values returned to
the mtd wrappers by the driver have changed.

Tested with nandsim and onenand_sim.  The two drivers that were modified were
compile-tested only.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

I'm working on follow-up patches which change the meaning of the -EUCLEAN return
code (as has been discussed at length), but I wanted to get this patch in right
away due to the merge window.

 drivers/mtd/devices/docg3.c        |    5 ++++-
 drivers/mtd/nand/alauda.c          |    4 ++--
 drivers/mtd/nand/nand_base.c       |   10 ++++++++--
 drivers/mtd/onenand/onenand_base.c |    6 ++++--
 include/linux/mtd/mtd.h            |   22 ++++++++++++++++++++--
 5 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 22d5099..73c7574 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -854,6 +854,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	u8 *buf = ops->datbuf;
 	size_t len, ooblen, nbdata, nboob;
 	u8 hwecc[DOC_ECC_BCH_SIZE], eccconf1;
+	unsigned int max_bitflips = 0;
 
 	if (buf)
 		len = ops->len;
@@ -938,7 +939,9 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 			}
 			if (ret > 0) {
 				mtd->ecc_stats.corrected += ret;
-				ret = -EUCLEAN;
+				max_bitflips = max(max_bitflips,
+						   (unsigned int)ret);
+				ret = max_bitflips;
 			}
 		}
 
diff --git a/drivers/mtd/nand/alauda.c b/drivers/mtd/nand/alauda.c
index eb40ea8..60db9f2 100644
--- a/drivers/mtd/nand/alauda.c
+++ b/drivers/mtd/nand/alauda.c
@@ -414,7 +414,7 @@ static int alauda_bounce_read(struct mtd_info *mtd, loff_t from, size_t len,
 	}
 	err = 0;
 	if (corrected)
-		err = -EUCLEAN;
+		err = 1;	/* return max bitflips per page */
 	if (uncorrected)
 		err = -EBADMSG;
 out:
@@ -446,7 +446,7 @@ static int alauda_read(struct mtd_info *mtd, loff_t from, size_t len,
 	}
 	err = 0;
 	if (corrected)
-		err = -EUCLEAN;
+		err = 1;	/* return max bitflips per page */
 	if (uncorrected)
 		err = -EBADMSG;
 	return err;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 28b49b6..b7ab765 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1441,7 +1441,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	uint32_t oobreadlen = ops->ooblen;
 	uint32_t max_oobsize = ops->mode == MTD_OPS_AUTO_OOB ?
 		mtd->oobavail : mtd->oobsize;
-
+	unsigned int max_bitflips = 0;
 	uint8_t *bufpoi, *oob, *buf;
 
 	stats = mtd->ecc_stats;
@@ -1463,6 +1463,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 
 		/* Is the current page in the buffer? */
 		if (realpage != chip->pagebuf || oob) {
+			unsigned int prev_corrected = mtd->ecc_stats.corrected;
+
 			bufpoi = aligned ? buf : chip->buffers->databuf;
 
 			if (likely(sndcmd)) {
@@ -1525,6 +1527,9 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				else
 					nand_wait_ready(mtd);
 			}
+			max_bitflips = max(max_bitflips,
+					   mtd->ecc_stats.corrected -
+					   prev_corrected);
 		} else {
 			memcpy(buf, chip->buffers->databuf + col, bytes);
 			buf += bytes;
@@ -1566,7 +1571,8 @@ 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 number of corrected bitflips on any one page */
+	return max_bitflips;
 }
 
 /**
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index dd278a2..3dd8bdb 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1201,7 +1201,8 @@ static int onenand_mlc_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 number of corrected bitflips on any one page */
+	return mtd->ecc_stats.corrected - stats.corrected ? 1 : 0;
 }
 
 /**
@@ -1333,7 +1334,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 number of corrected bitflips on any one page */
+	return mtd->ecc_stats.corrected - stats.corrected ? 1 : 0;
 }
 
 /**
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index e8c4409..be286ab 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -279,7 +279,16 @@ static inline unsigned long mtd_get_unmapped_area(struct mtd_info *mtd,
 static inline int mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
 			   size_t *retlen, u_char *buf)
 {
-	return mtd->read(mtd, from, len, retlen, buf);
+	/*
+	 * Absent an error, drivers return max_bitflips.
+	 * MTD returns -EUCLEAN (bitflips corrected) or 0 (no bitflips).
+	 * max_bitflips is maximum number of bitflips corrected on any one page.
+	 */
+	int ret = mtd->read(mtd, from, len, retlen, buf);
+	if (ret > 0)
+		return -EUCLEAN;
+	else
+		return ret;
 }
 
 static inline int mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
@@ -304,7 +313,16 @@ static inline int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
 static inline int mtd_read_oob(struct mtd_info *mtd, loff_t from,
 			       struct mtd_oob_ops *ops)
 {
-	return mtd->read_oob(mtd, from, ops);
+	/*
+	 * Absent an error, drivers return max_bitflips.
+	 * MTD returns -EUCLEAN (bitflips corrected) or 0 (no bitflips).
+	 * max_bitflips is maximum number of bitflips corrected on any one page.
+	 */
+	int ret = mtd->read_oob(mtd, from, ops);
+	if (ret > 0)
+		return -EUCLEAN;
+	else
+		return ret;
 }
 
 static inline int mtd_write_oob(struct mtd_info *mtd, loff_t to,
-- 
1.7.3.4

Re: [PATCH] MTD: drivers return bitlip info to mtd on read

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

On Mon, 2011-12-26 at 11:38 -0800, Mike Dunn wrote:
> This patch changes the meaning of the value returned by the read() and
> read_oob() mtd driver methods.  Previously, absent a hard error, these functions
> returned either -EUCLEAN (one or more bitflips were corrected) or 0 (no
> bitflips).  Drivers now return, absent a hard error, the maximum number of
> bitflips that were corrected on any one page.  
> 
> This change is made possible by the fact that all calls to the driver methods
> now go through mtd wrapper functions.  The values returned by those wrapper
> functions have not changed, nor have their meaning.  Only the values returned to
> the mtd wrappers by the driver have changed.
> 
> Tested with nandsim and onenand_sim.  The two drivers that were modified were
> compile-tested only.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>

Hi, I am not sure using the return code is a good way for this.

But please, proceed with working on your issues - this patch is not
going to hit 3.2 anyway - I think it is the 3.3 material. Linus promised
3.2 release this week, then we have the merge window, and then it is
time for further patches in this area.

I will do the other changes which I described in my cover letter. I
will also make a patch which adds a bitflips parameter to read/read_oob
and then you will implement 

But please, just make something which works for you for now and go on
with other DoC issues you have. Meantime I'll prepare the MTD basis
for you.

I think I'll first do the 3 items I described in my series. Then I'll
add "bitflips" parameter to read/read_oob() functions, but won't
implement or ever check it. After this you'll add your code.

How does this sound to you?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] MTD: drivers return bitlip info to mtd on read

[Mike Dunn]

On 12/27/2011 12:33 AM, Artem Bityutskiy wrote:
> On Mon, 2011-12-26 at 11:38 -0800, Mike Dunn wrote:
>> This patch changes the meaning of the value returned by the read() and
>> read_oob() mtd driver methods.  Previously, absent a hard error, these functions
>> returned either -EUCLEAN (one or more bitflips were corrected) or 0 (no
>> bitflips).  Drivers now return, absent a hard error, the maximum number of
>> bitflips that were corrected on any one page.  
>>
>> This change is made possible by the fact that all calls to the driver methods
>> now go through mtd wrapper functions.  The values returned by those wrapper
>> functions have not changed, nor have their meaning.  Only the values returned to
>> the mtd wrappers by the driver have changed.
>>
>> Tested with nandsim and onenand_sim.  The two drivers that were modified were
>> compile-tested only.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> Hi, I am not sure using the return code is a good way for this.


OK.  It was a little kludgey, but had the advantage of touching very few drivers
and leaving the existing function prototypes unchanged.


> But please, proceed with working on your issues - this patch is not
> going to hit 3.2 anyway - I think it is the 3.3 material. Linus promised
> 3.2 release this week, then we have the merge window, and then it is
> time for further patches in this area.
>
> I will do the other changes which I described in my cover letter. I
> will also make a patch which adds a bitflips parameter to read/read_oob
> and then you will implement 
>
> But please, just make something which works for you for now and go on
> with other DoC issues you have. Meantime I'll prepare the MTD basis
> for you.
>
> I think I'll first do the 3 items I described in my series. Then I'll
> add "bitflips" parameter to read/read_oob() functions, but won't
> implement or ever check it. After this you'll add your code.
>
> How does this sound to you?


Sure, you're the boss.  I appreciate your attention on this.  If you like, I'd
be happy to submit a patch that adds the max_bitflips argument to  read() and
read_oob() after you finish the cleanup and consolidation made possible by the
wrappers.  I've already done this work.  Your choice; let me know.

Thanks again,
Mike

Re: [PATCH] MTD: drivers return bitlip info to mtd on read

[Artem Bityutskiy]

On Tue, 2011-12-27 at 12:26 -0800, Mike Dunn wrote:
> On 12/27/2011 12:33 AM, Artem Bityutskiy wrote:
> > On Mon, 2011-12-26 at 11:38 -0800, Mike Dunn wrote:
> >> This patch changes the meaning of the value returned by the read() and
> >> read_oob() mtd driver methods.  Previously, absent a hard error, these functions
> >> returned either -EUCLEAN (one or more bitflips were corrected) or 0 (no
> >> bitflips).  Drivers now return, absent a hard error, the maximum number of
> >> bitflips that were corrected on any one page.  
> >>
> >> This change is made possible by the fact that all calls to the driver methods
> >> now go through mtd wrapper functions.  The values returned by those wrapper
> >> functions have not changed, nor have their meaning.  Only the values returned to
> >> the mtd wrappers by the driver have changed.
> >>
> >> Tested with nandsim and onenand_sim.  The two drivers that were modified were
> >> compile-tested only.
> >>
> >> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> > Hi, I am not sure using the return code is a good way for this.
> 
> 
> OK.  It was a little kludgey, but had the advantage of touching very few drivers
> and leaving the existing function prototypes unchanged.

May be you are right, I need to think. But this is only true when you
are 100% sure no one uses the function pointers directly. The very
reliable way is to rename them, so the offenders would end up with a
compilation error. I am also thinking about out-of-tree drivers.

But I am planning to add leading "_" symbol to the function pointers
(e.g., mtd->read() => mtd->_read()). So this anyway will touch a lot of
code. And then this argument is probably not too strong anymore.

Anyway, let me emphasize that this should not be a show-stopper for
you - proceed with your DoC work meanwile. I'll try to come up with the
changes this week, but no promises, then I'll create a temporary branch
in my tree and we can use it till the end of the 3.3 merge window.

Artem.