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

Submitted by Shmulik Ladkani on May 9, 2012, 10:13 a.m.

Details

Message ID 20120509131334.2d0b7160@pixies.home.jungo.com
State Accepted
Commit 1951f2f710a621ae0bc4268617046a6c02c634d0
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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;