[PATCH 1/2] mtd: nand: remove 'sndcmd' parameter of 'read_oob/read_oob_raw'

[PATCH 1/2] mtd: nand: remove 'sndcmd' parameter of 'read_oob/read_oob_raw'

[Shmulik Ladkani]

As of [mtd: nand: remove autoincrement 'sndcmd' code], the
NAND_CMD_READ0 command is issued unconditionally.

Thus, read_oob/read_oob_raw's 'sndcmd' argument is no longer needed, as
well as their return code.

Remove the 'sndcmd' parameter, and set the return code to 0.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
 drivers/mtd/nand/cafe_nand.c           |    4 ++--
 drivers/mtd/nand/denali.c              |    5 ++---
 drivers/mtd/nand/docg4.c               |    2 +-
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    8 ++------
 drivers/mtd/nand/nand_base.c           |   19 +++++++------------
 drivers/mtd/nand/r852.c                |    9 +++------
 include/linux/mtd/nand.h               |    5 ++---
 7 files changed, 19 insertions(+), 33 deletions(-)

diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
index 3a6c88d..41371ba 100644
--- a/drivers/mtd/nand/cafe_nand.c
+++ b/drivers/mtd/nand/cafe_nand.c
@@ -364,11 +364,11 @@ static int cafe_nand_write_oob(struct mtd_info *mtd,
 
 /* Don't use -- use nand_read_oob_std for now */
 static int cafe_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
-			      int page, int sndcmd)
+			      int page)
 {
 	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
 	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
-	return 1;
+	return 0;
 }
 /**
  * cafe_nand_read_page_syndrome - [REPLACEABLE] hardware ecc syndrome based page read
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index a54c186..0650aaf 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1113,12 +1113,11 @@ static int denali_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
-			   int page, int sndcmd)
+			   int page)
 {
 	read_oob_data(mtd, chip->oob_poi, page);
 
-	return 0; /* notify NAND core to send command to
-			   NAND device. */
+	return 0;
 }
 
 static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c
index 1f8485d..a225e49 100644
--- a/drivers/mtd/nand/docg4.c
+++ b/drivers/mtd/nand/docg4.c
@@ -799,7 +799,7 @@ static int docg4_read_page(struct mtd_info *mtd, struct nand_chip *nand,
 }
 
 static int docg4_read_oob(struct mtd_info *mtd, struct nand_chip *nand,
-			  int page, int sndcmd)
+			  int page)
 {
 	struct docg4_priv *doc = nand->priv;
 	void __iomem *docptr = doc->virtadr;
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index dcf4152..9079ebf 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1070,7 +1070,7 @@ exit_auxiliary:
  * this driver.
  */
 static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
-				int page, int sndcmd)
+				int page)
 {
 	struct gpmi_nand_data *this = chip->priv;
 
@@ -1093,11 +1093,7 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->oob_poi[0] = chip->read_byte(mtd);
 	}
 
-	/*
-	 * Return true, indicating that the next call to this function must send
-	 * a command.
-	 */
-	return true;
+	return 0;
 }
 
 static int
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 14e9b32..4047d7c 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1645,17 +1645,13 @@ static int nand_read(struct mtd_info *mtd, loff_t from, size_t len,
  * @mtd: mtd info structure
  * @chip: nand chip info structure
  * @page: page number to read
- * @sndcmd: flag whether to issue read command or not
  */
 static int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
-			     int page, int sndcmd)
+			     int page)
 {
-	if (sndcmd) {
-		chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
-		sndcmd = 0;
-	}
+	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
 	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
-	return sndcmd;
+	return 0;
 }
 
 /**
@@ -1664,10 +1660,9 @@ static int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
  * @mtd: mtd info structure
  * @chip: nand chip info structure
  * @page: page number to read
- * @sndcmd: flag whether to issue read command or not
  */
 static int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
-				  int page, int sndcmd)
+				  int page)
 {
 	uint8_t *buf = chip->oob_poi;
 	int length = mtd->oobsize;
@@ -1694,7 +1689,7 @@ static int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 	if (length > 0)
 		chip->read_buf(mtd, bufpoi, length);
 
-	return 1;
+	return 0;
 }
 
 /**
@@ -1831,9 +1826,9 @@ 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, 1);
+			chip->ecc.read_oob_raw(mtd, chip, page);
 		else
-			chip->ecc.read_oob(mtd, chip, page, 1);
+			chip->ecc.read_oob(mtd, chip, page);
 
 		len = min(len, readlen);
 		buf = nand_transfer_oob(chip, buf, ops, len);
diff --git a/drivers/mtd/nand/r852.c b/drivers/mtd/nand/r852.c
index 859ef31..8cb6277 100644
--- a/drivers/mtd/nand/r852.c
+++ b/drivers/mtd/nand/r852.c
@@ -539,14 +539,11 @@ exit:
  * nand_read_oob_syndrome assumes we can send column address - we can't
  */
 static int r852_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
-			     int page, int sndcmd)
+			     int page)
 {
-	if (sndcmd) {
-		chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
-		sndcmd = 0;
-	}
+	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
 	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
-	return sndcmd;
+	return 0;
 }
 
 /*
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c7755f45..57977c6 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -372,9 +372,8 @@ struct nand_ecc_ctrl {
 	int (*write_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip,
 			int page);
 	int (*read_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip,
-			int page, int sndcmd);
-	int (*read_oob)(struct mtd_info *mtd, struct nand_chip *chip, int page,
-			int sndcmd);
+			int page);
+	int (*read_oob)(struct mtd_info *mtd, struct nand_chip *chip, int page);
 	int (*write_oob)(struct mtd_info *mtd, struct nand_chip *chip,
 			int page);
 };
-- 
1.7.9

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

[Shmulik Ladkani]

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

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;
-- 
1.7.9

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

[Artem Bityutskiy]

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

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?

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH 1/2] mtd: nand: remove 'sndcmd' parameter of 'read_oob/read_oob_raw'

[Artem Bityutskiy]

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

On Wed, 2012-05-09 at 13:06 +0300, Shmulik Ladkani wrote:
> As of [mtd: nand: remove autoincrement 'sndcmd' code], the
> NAND_CMD_READ0 command is issued unconditionally.
> 
> Thus, read_oob/read_oob_raw's 'sndcmd' argument is no longer needed, as
> well as their return code.
> 
> Remove the 'sndcmd' parameter, and set the return code to 0.
> 
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Pushed this one to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

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

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

[Shmulik Ladkani]

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

Re: [PATCH 1/2] mtd: nand: remove 'sndcmd' parameter of 'read_oob/read_oob_raw'

[Artem Bityutskiy]

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

On Wed, 2012-05-09 at 13:06 +0300, Shmulik Ladkani wrote:
> As of [mtd: nand: remove autoincrement 'sndcmd' code], the
> NAND_CMD_READ0 command is issued unconditionally.
> 
> Thus, read_oob/read_oob_raw's 'sndcmd' argument is no longer needed, as
> well as their return code.
> 
> Remove the 'sndcmd' parameter, and set the return code to 0.
> 
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

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

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

[Artem Bityutskiy]

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

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.

-- 
Best Regards,
Artem Bityutskiy

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

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

[Artem Bityutskiy]

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

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!

-- 
Best Regards,
Artem Bityutskiy

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