Patchwork [2/2] mtd: nand: check the return code of 'read_oob/read_oob_raw'

login
register
mail settings
Submitter Shmulik Ladkani
Date May 9, 2012, 10:13 a.m.
Message ID <20120509131334.2d0b7160@pixies.home.jungo.com>
Download mbox | patch
Permalink /patch/157920/
State Accepted
Commit 1951f2f710a621ae0bc4268617046a6c02c634d0
Headers show

Comments

Shmulik Ladkani - May 9, 2012, 10:13 a.m.
Apparently, there is an implementor of 'read_oob' which may return an
error inidication (e.g. docg4_read_oob may return -EIO).

Test the return value of 'read_oob/read_oob_raw', and if negative,
propagate the error, so it's returned by the '_read_oob' interface.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
 drivers/mtd/nand/nand_base.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)
Artem Bityutskiy - May 11, 2012, 11:48 a.m.
On Wed, 2012-05-09 at 13:13 +0300, Shmulik Ladkani wrote:

> @@ -1826,9 +1827,12 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
>  
>  	while (1) {
>  		if (ops->mode == MTD_OPS_RAW)
> -			chip->ecc.read_oob_raw(mtd, chip, page);
> +			ret = chip->ecc.read_oob_raw(mtd, chip, page);
>  		else
> -			chip->ecc.read_oob(mtd, chip, page);
> +			ret = chip->ecc.read_oob(mtd, chip, page);
> +
> +		if (ret < 0)
> +			break;

For page reading the convention is that we keep reading and try to read
everything anyway, I guess it is reasonable thing to do for OOB as well?
Shmulik Ladkani - May 11, 2012, 12:54 p.m.
Hi Artem,

On Fri, 11 May 2012 14:48:11 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2012-05-09 at 13:13 +0300, Shmulik Ladkani wrote:
> > @@ -1826,9 +1827,12 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
> >  
> >  	while (1) {
> >  		if (ops->mode == MTD_OPS_RAW)
> > -			chip->ecc.read_oob_raw(mtd, chip, page);
> > +			ret = chip->ecc.read_oob_raw(mtd, chip, page);
> >  		else
> > -			chip->ecc.read_oob(mtd, chip, page);
> > +			ret = chip->ecc.read_oob(mtd, chip, page);
> > +
> > +		if (ret < 0)
> > +			break;
> 
> For page reading the convention is that we keep reading and try to read
> everything anyway, I guess it is reasonable thing to do for OOB as well?

AFAIU, we actually _stop_ reading upon 'ecc.read_page()' error.
And 'ops->retlen' is updated to reflect actual bytes sucessfully read.

See snip from 'nand_do_read_ops':

---------------------------------
	while (1) {

		...
		...

			chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);

			/*
			 * 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,
							      page);
			else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob)
				ret = chip->ecc.read_subpage(mtd, chip,
							col, bytes, bufpoi);
			else
				ret = chip->ecc.read_page(mtd, chip, bufpoi,
							  oob_required, page);
			if (ret < 0) {
				if (!aligned)
					/* Invalidate page cache */
					chip->pagebuf = -1;
				break;
			}

		...
		...

	}

	ops->retlen = ops->len - (size_t) readlen;
	if (oob)
		ops->oobretlen = ops->ooblen - oobreadlen;

	if (ret < 0)
		return ret;
---------------------------------

The error is propagated back to 'nand_read' and to the 'mtd->_read'
user.

Have I misinterpreted your question? Did you mean something else?

BTW, note that the patch does not intend to change existing
behavior of '_read_oob' in case 'ecc.read_oob()' calls were succesful.

The behavior is changed only if 'ecc.read_oob()' fails.
In that case an error indication is returned and 'ops->oobretlen' is set
appropriately.
I havn't tested this, but it seems as the reaonable thing to report back
to the '_read_oob' users.

Regards,
Shmulik
Artem Bityutskiy - May 11, 2012, 1:34 p.m.
On Fri, 2012-05-11 at 15:54 +0300, Shmulik Ladkani wrote:
> Hi Artem,
> 
> On Fri, 11 May 2012 14:48:11 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2012-05-09 at 13:13 +0300, Shmulik Ladkani wrote:
> > > @@ -1826,9 +1827,12 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
> > >  
> > >  	while (1) {
> > >  		if (ops->mode == MTD_OPS_RAW)
> > > -			chip->ecc.read_oob_raw(mtd, chip, page);
> > > +			ret = chip->ecc.read_oob_raw(mtd, chip, page);
> > >  		else
> > > -			chip->ecc.read_oob(mtd, chip, page);
> > > +			ret = chip->ecc.read_oob(mtd, chip, page);
> > > +
> > > +		if (ret < 0)
> > > +			break;
> > 
> > For page reading the convention is that we keep reading and try to read
> > everything anyway, I guess it is reasonable thing to do for OOB as well?
> 
> AFAIU, we actually _stop_ reading upon 'ecc.read_page()' error.
> And 'ops->retlen' is updated to reflect actual bytes sucessfully read.

Yes, you are right. It should keep reading in case of an ECC error, not
in case of any error.
Artem Bityutskiy - May 11, 2012, 1:45 p.m.
On Wed, 2012-05-09 at 13:13 +0300, Shmulik Ladkani wrote:
> Apparently, there is an implementor of 'read_oob' which may return an
> error inidication (e.g. docg4_read_oob may return -EIO).
> 
> Test the return value of 'read_oob/read_oob_raw', and if negative,
> propagate the error, so it's returned by the '_read_oob' interface.
> 
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Pushed to l2-mtd.git, thanks!

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4047d7c..d47586c 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1791,6 +1791,7 @@  static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 	int readlen = ops->ooblen;
 	int len;
 	uint8_t *buf = ops->oobbuf;
+	int ret = 0;
 
 	pr_debug("%s: from = 0x%08Lx, len = %i\n",
 			__func__, (unsigned long long)from, readlen);
@@ -1826,9 +1827,12 @@  static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 
 	while (1) {
 		if (ops->mode == MTD_OPS_RAW)
-			chip->ecc.read_oob_raw(mtd, chip, page);
+			ret = chip->ecc.read_oob_raw(mtd, chip, page);
 		else
-			chip->ecc.read_oob(mtd, chip, page);
+			ret = chip->ecc.read_oob(mtd, chip, page);
+
+		if (ret < 0)
+			break;
 
 		len = min(len, readlen);
 		buf = nand_transfer_oob(chip, buf, ops, len);
@@ -1857,7 +1861,10 @@  static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 		}
 	}
 
-	ops->oobretlen = ops->ooblen;
+	ops->oobretlen = ops->ooblen - readlen;
+
+	if (ret < 0)
+		return ret;
 
 	if (mtd->ecc_stats.failed - stats.failed)
 		return -EBADMSG;