Patchwork [7/7] mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

login
register
mail settings
Submitter Mike Dunn
Date April 24, 2012, 7:18 p.m.
Message ID <1335295105-7981-8-git-send-email-mikedunn@newsguy.com>
Download mbox | patch
Permalink /patch/154752/
State New
Headers show

Comments

Mike Dunn - April 24, 2012, 7:18 p.m.
The drivers _read() method, absent an error, return a non-negative integer
indicating the maximum number of bit errors that were corrected in any one
region comprising an ecc step.  MTD returns -EUCLEAN if this is >=
bitflip_threshold, 0 otherwise.  If bitflip_threshold is zero, the comparison is
not made since these devices lack ECC and always return zero in the non-error
case (thanks Brian) [1].  Note that this is a subtle change to the driver
interface.

This and the preceeding patches in this set were tested with ubi on top of the
nandsim and docg4 devices, running the ubi test io_basic from mtd-utils.

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040468.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/devices/docg3.c        |    4 +++-
 drivers/mtd/mtdcore.c              |   14 +++++++++++++-
 drivers/mtd/mtdpart.c              |   12 ++++++------
 drivers/mtd/nand/alauda.c          |    4 ++--
 drivers/mtd/nand/nand_base.c       |   18 ++++++++++++++----
 drivers/mtd/onenand/onenand_base.c |    6 ++++--
 include/linux/mtd/nand.h           |    3 +++
 7 files changed, 45 insertions(+), 16 deletions(-)
Shmulik Ladkani - April 25, 2012, 10:14 a.m.
Hi Mike,

Reviewed mtdcore.c, mtdpart.c and nand_base.c.
Looks very good.

Minor comments below.

On Tue, 24 Apr 2012 12:18:25 -0700 Mike Dunn <mikedunn@newsguy.com> wrote:
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 9651c06..d6321f6 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -67,12 +67,12 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	stats = part->master->ecc_stats;
>  	res = part->master->_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;
>  }

Probably need {} around the statements within the conditions.

Also I think there's no reason to execute "corrected +=" in case 'res'
is zero (although no harm).

Regards,
Shmulik
Robert Jarzmik - April 25, 2012, 6:27 p.m.
Mike Dunn <mikedunn@newsguy.com> writes:

One little trick with docg3 patch part:
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 3414031..7644d59 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -948,7 +949,8 @@ 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, ret);
> +				ret = max_bitflips;
>  			}
>  		}

If you do set ret here, the next loop you'll do in the following statement
"	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
0.

I think you should change :
>- 	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
into
>- 	while (ret >= 0 && (len > 0 || ooblen > 0)) {".

With that change, please add my:
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.
Mike Dunn - April 25, 2012, 7:13 p.m.
Thanks for the review Robert.

On 04/25/2012 11:27 AM, Robert Jarzmik wrote:
> 
> I think you should change :
>> - 	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
> into
>> - 	while (ret >= 0 && (len > 0 || ooblen > 0)) {".
> 
> With that change, please add my:
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>


In my exuberance, I prematurely sent the next version of the whole set.  Artem,
Robert's requested change is in patch 7/7.  If no other problems come to light,
maybe you could consider merging the first 6?  Then I only need to prepare a
corrected version of patch 7.

Thanks,
Mike
Artem Bityutskiy - April 29, 2012, 7:24 p.m.
On Wed, 2012-04-25 at 12:13 -0700, Mike Dunn wrote:
> Thanks for the review Robert.
> 
> On 04/25/2012 11:27 AM, Robert Jarzmik wrote:
> > 
> > I think you should change :
> >> - 	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
> > into
> >> - 	while (ret >= 0 && (len > 0 || ooblen > 0)) {".
> > 
> > With that change, please add my:
> > Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> 
> In my exuberance, I prematurely sent the next version of the whole set.  Artem,
> Robert's requested change is in patch 7/7.  If no other problems come to light,
> maybe you could consider merging the first 6?  Then I only need to prepare a
> corrected version of patch 7.

Pushed the series to l2-mtd.git, thanks! You can send the newer version
and I'll put it instead of the older one. Do not forget to add
Acked-by's when you re-send, please.
Mike Dunn - April 30, 2012, 7:55 p.m.
On 04/29/2012 12:24 PM, Artem Bityutskiy wrote:
> On Wed, 2012-04-25 at 12:13 -0700, Mike Dunn wrote:
>> Thanks for the review Robert.
>>
>> On 04/25/2012 11:27 AM, Robert Jarzmik wrote:
>>>
>>> I think you should change :
>>>> - 	while (!ret && (len > 0 || ooblen > 0)) {" will exit because ret is not
>>> into
>>>> - 	while (ret >= 0 && (len > 0 || ooblen > 0)) {".
>>>
>>> With that change, please add my:
>>> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
>>
>>
>> In my exuberance, I prematurely sent the next version of the whole set.  Artem,
>> Robert's requested change is in patch 7/7.  If no other problems come to light,
>> maybe you could consider merging the first 6?  Then I only need to prepare a
>> corrected version of patch 7.
> 
> Pushed the series to l2-mtd.git, thanks! You can send the newer version
> and I'll put it instead of the older one. Do not forget to add
> Acked-by's when you re-send, please.
> 


Thanks Artem.  Since you merged all 7, the only thing remaining is the fix to
docg3 that Robert describes above.  Robert, if you would rather prepare that
patch, I defer to you.  Otherwise, I'll submit it no later than tomorrow.

Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.

Mike
Robert Jarzmik - April 30, 2012, 8:31 p.m.
Mike Dunn <mikedunn@newsguy.com> writes:

> Thanks Artem.  Since you merged all 7, the only thing remaining is the fix to
> docg3 that Robert describes above.  Robert, if you would rather prepare that
> patch, I defer to you.  Otherwise, I'll submit it no later than tomorrow.
Oh no, go ahead, you have my blessing.

> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just
> merged.
That's your opportunity to add it :)

Cheers.
Artem Bityutskiy - May 1, 2012, 12:20 p.m.
On Mon, 2012-04-30 at 12:55 -0700, Mike Dunn wrote:
> Thanks Artem.  Since you merged all 7, the only thing remaining is the fix to
> docg3 that Robert describes above.  Robert, if you would rather prepare that
> patch, I defer to you.  Otherwise, I'll submit it no later than tomorrow.
> 
> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.

If you know that an Acked-by or Reviewed-by was missed, let me know and
I'll add the tag(s) to the patch(es). I think it is important to be
careful about the tags because people spent their time helping to
improve the patches.
Mike Dunn - May 1, 2012, 5:27 p.m.
On 05/01/2012 05:20 AM, Artem Bityutskiy wrote:
> On Mon, 2012-04-30 at 12:55 -0700, Mike Dunn wrote:
>>
>> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.
> 
> If you know that an Acked-by or Reviewed-by was missed, let me know and
> I'll add the tag(s) to the patch(es). I think it is important to be
> careful about the tags because people spent their time helping to
> improve the patches.


Well, I owe a debt to several reviewers, among them Brian Norris (caught the bug
that would cause all non-ecc devices to return -EUCLEAN), Shmulik Ladkani, Ivan
Djelic (reviews and tests), Scott Wood, Robert Jarzmik, yourself, ...

As far as formal tags... maybe these, if they don't object:

mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN
5568eb3c3bb2816281b6a7c04db92434b72b1495
Tested-by: Ivan Djelic <ivan.djelic@parrot.com>

mtd: nand: read_page() returns max_bitflips
6bf87bf989bfdfc78fb2c5cd55de4bab9b572992
Acked-by (freescale changes): Scott Wood <scottwood@freescale.com>

If the others feel a tag credit is appropriate, I certainly don't mind.

Thanks,
Mike
Shmulik Ladkani - May 1, 2012, 6:51 p.m.
On Tue, 01 May 2012 10:27:14 -0700 Mike Dunn <mikedunn@newsguy.com> wrote:
> On 05/01/2012 05:20 AM, Artem Bityutskiy wrote:
> > On Mon, 2012-04-30 at 12:55 -0700, Mike Dunn wrote:
> >>
> >> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.
> > 
> > If you know that an Acked-by or Reviewed-by was missed, let me know and
> > I'll add the tag(s) to the patch(es). I think it is important to be
> > careful about the tags because people spent their time helping to
> > improve the patches.
> 
> 
> Well, I owe a debt to several reviewers, among them Brian Norris (caught the bug
> that would cause all non-ecc devices to return -EUCLEAN), Shmulik Ladkani, Ivan
> Djelic (reviews and tests), Scott Wood, Robert Jarzmik, yourself, ...
> 
> As far as formal tags... maybe these, if they don't object:
> 
> mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN
> 5568eb3c3bb2816281b6a7c04db92434b72b1495
> Tested-by: Ivan Djelic <ivan.djelic@parrot.com>
> 
> mtd: nand: read_page() returns max_bitflips
> 6bf87bf989bfdfc78fb2c5cd55de4bab9b572992
> Acked-by (freescale changes): Scott Wood <scottwood@freescale.com>
> 
> If the others feel a tag credit is appropriate, I certainly don't mind.

On a side note (sorry for the off-topic post):

It seems the use of the formal tags is somewhat limited.

AFAIU, Acked-by is supposed to be specified by the maintainer or major
contributor, the one in charge of the code affected, and it could
be specified to just a part of the patch.

OTOH Reviewed-by can be specified by any interested reviewer, as an
indication that the entire patch has been reviewed.

However when patches get partially reviewed by interested parties, no
formal tag is suitable.

Regards,
Shmulik
Artem Bityutskiy - May 2, 2012, 3:59 a.m.
On Tue, 2012-05-01 at 10:27 -0700, Mike Dunn wrote:
> On 05/01/2012 05:20 AM, Artem Bityutskiy wrote:
> > On Mon, 2012-04-30 at 12:55 -0700, Mike Dunn wrote:
> >>
> >> Yeah, sorry Artem, I neglected to add the Acked-by's in the patches you just merged.
> > 
> > If you know that an Acked-by or Reviewed-by was missed, let me know and
> > I'll add the tag(s) to the patch(es). I think it is important to be
> > careful about the tags because people spent their time helping to
> > improve the patches.
> 
> 
> Well, I owe a debt to several reviewers, among them Brian Norris (caught the bug
> that would cause all non-ecc devices to return -EUCLEAN), Shmulik Ladkani, Ivan
> Djelic (reviews and tests), Scott Wood, Robert Jarzmik, yourself, ...
> 
> As far as formal tags... maybe these, if they don't object:

How it usually goes - if the person sends a "formal" acked-by or some
other tag which can be copy-pasted, then he cares and wants the tag to
go in with the patch. But if the person just comments on the patch,
reviews, but does not send a "formal" tag, he does not care. In this
case you may give the person a credit in free form in the commit message
optionally, if you feel very grateful.

IOW, if we missed someone's "formal" tag - let's add it - I shamelessly
rebase my tree anyway, under excuse of "I am not the maintainer" :-)

> mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN
> 5568eb3c3bb2816281b6a7c04db92434b72b1495
> Tested-by: Ivan Djelic <ivan.djelic@parrot.com>
> 
> mtd: nand: read_page() returns max_bitflips
> 6bf87bf989bfdfc78fb2c5cd55de4bab9b572992
> Acked-by (freescale changes): Scott Wood <scottwood@freescale.com>

OK, thanks.

Patch

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 3414031..7644d59 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -862,6 +862,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;
+	int max_bitflips = 0;
 
 	if (buf)
 		len = ops->len;
@@ -948,7 +949,8 @@  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, ret);
+				ret = max_bitflips;
 			}
 		}
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 6a7cba1..b5bb628 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -800,12 +800,24 @@  EXPORT_SYMBOL_GPL(mtd_get_unmapped_area);
 int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
 	     u_char *buf)
 {
+	int ret_code;
 	*retlen = 0;
 	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->bitflip_threshold == 0)
+		return 0;	/* device lacks ecc */
+	return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
 }
 EXPORT_SYMBOL_GPL(mtd_read);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 9651c06..d6321f6 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -67,12 +67,12 @@  static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
 	stats = part->master->ecc_stats;
 	res = part->master->_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/alauda.c b/drivers/mtd/nand/alauda.c
index 4f20e1d..60a0dfd 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 ecc step */
 	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 ecc step */
 	if (uncorrected)
 		err = -EBADMSG;
 	return err;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 275c43f..9f1b202 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1486,6 +1486,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;
 
@@ -1513,7 +1514,10 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				sndcmd = 0;
 			}
 
-			/* 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, page);
@@ -1530,15 +1534,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_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);
 			}
 
@@ -1571,6 +1579,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;
@@ -1609,7 +1619,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 b3ce12e..7153e0d 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 bitflips per ecc step; ONENANDs correct 1 bit only */
+	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 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 1482340..2829e8b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -459,6 +459,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.
@@ -519,6 +521,7 @@  struct nand_chip {
 	uint64_t chipsize;
 	int pagemask;
 	int pagebuf;
+	unsigned int pagebuf_bitflips;
 	int subpagesize;
 	uint8_t cellinfo;
 	int badblockpos;