Do not stop reading for ECC errors

When an ECC error occurs, the read should be completed anyway before returning -EBADMSG. Returning -EBADMSG straight away is incorrect.

---
drivers/mtd/onenand/onenand_base.c | 21 +++++++++++++++------

1 files changed, 15 insertions(+), 6 deletions(-)

index dd28355..616747c 100644
@@ -855,6 +855,8 @@ static int onenand_read_ops_nolock(struct mtd_info *mtd, loff_t from,

			this->command(mtd, ONENAND_CMD_READ, from, writesize);

ret = this->wait(mtd, FL_READING); onenand_update_bufferram(mtd, from, !ret);
+ if (ret == -EBADMSG)
+ ret = 0;
}
}

@@ -913,6 +915,8 @@ static int onenand_read_ops_nolock(struct mtd_info *mtd, loff_t from,
/* Now wait for load */
ret = this->wait(mtd, FL_READING); onenand_update_bufferram(mtd, from, !ret);
+ if (ret == -EBADMSG)
+ ret = 0;
}

	/*
@@ -944,6 +948,7 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
			struct mtd_oob_ops *ops)
{
	struct onenand_chip *this = mtd->priv;
+	struct mtd_ecc_stats stats;
	int read = 0, thislen, column, oobsize;
	size_t len = ops->ooblen;
	mtd_oob_mode_t mode = ops->mode;
@@ -977,6 +982,8 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
		return -EINVAL;
	}

+ stats = mtd->ecc_stats;
+

	while (read < len) {
		cond_resched();

@@ -988,18 +995,16 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,

		onenand_update_bufferram(mtd, from, 0);

		ret = this->wait(mtd, FL_READING);

- /* First copy data and check return value for ECC handling */
+ if (ret && ret != -EBADMSG) {
+ printk(KERN_ERR "onenand_read_oob_nolock: read failed = 0x%x\n", ret);
+ break;
+ }

		if (mode == MTD_OOB_AUTO)
			onenand_transfer_auto_oob(mtd, buf, column, thislen);
		else
			this->read_bufferram(mtd, ONENAND_SPARERAM, buf, column, thislen);

- if (ret) {
- printk(KERN_ERR "onenand_read_oob_nolock: read failed = 0x%x\n", ret);
- break;
- }
-

		read += thislen;

		if (read == len)
@@ -1016,6 +1021,10 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
	}

	ops->oobretlen = read;

+
+ if (mtd->ecc_stats.failed - stats.failed)
+ return -EBADMSG;
+

	return ret;
}

1.4.4.2

-EBADMSG is an ECC error.

ret must be non-zero to indicate that the bufferram is invalid. Otherwise if we read from the same location again, the data would be taken from bufferram and no ECC error would be reported. However we need to keep reading (see nand_base.c) so ret is set to zero as though no error occured. Finally mtd->ecc_stats.failed is examined to determine ECC errors. i.e.

static int onenand_read(struct mtd_info *mtd, loff_t from, size_t len,
size_t *retlen, u_char *buf)

{
	...
	stats = mtd->ecc_stats;
	...
	while (!ret) {
		...
		ret = this->wait(mtd, FL_READING);
		onenand_update_bufferram(mtd, from, !ret);
		if (ret == -EBADMSG)
			ret = 0;
	}
	*retlen = read;
	if (mtd->ecc_stats.failed - stats.failed)
		return -EBADMSG;
	if (ret)
		return ret;
	return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
}

The wait function doesn't return -EUCLEAN. It increments mtd->ecc_stats.corrected and returns zero for correctable ECC errors.

File systems (like say JFFS2 for example) don't expect -EUCLEAN from mtd->read_oob and would treat it as a fatal error, so it must not be returned in this case.

Hmm.

Those two conditionals should have reversed order, imo.

Such an interface sure looks strange. But since read_oob is strange anyway I don't care very much either way.

Jörn

There's nothing that will change someone's moral outlook quicker
than cash in large sums.
-- Larry Flynt

Very true. I have added that fix to the patch also - see next mail.

When an ECC error occurs, the read should be completed anyway before returning -EBADMSG. Returning -EBADMSG straight away is incorrect.

---
drivers/mtd/onenand/onenand_base.c | 32 ++++++++++++++++++++++----------

1 files changed, 22 insertions(+), 10 deletions(-)

index dd28355..cedc154 100644
@@ -855,6 +855,8 @@ static int onenand_read_ops_nolock(struct mtd_info *mtd, loff_t from,

			this->command(mtd, ONENAND_CMD_READ, from, writesize);

ret = this->wait(mtd, FL_READING); onenand_update_bufferram(mtd, from, !ret);
+ if (ret == -EBADMSG)
+ ret = 0;
}
}

@@ -913,6 +915,8 @@ static int onenand_read_ops_nolock(struct mtd_info *mtd, loff_t from,
/* Now wait for load */
ret = this->wait(mtd, FL_READING); onenand_update_bufferram(mtd, from, !ret);
+ if (ret == -EBADMSG)
+ ret = 0;
}

	/*
@@ -923,12 +927,12 @@ static int onenand_read_ops_nolock(struct mtd_info *mtd, loff_t from,
	ops->retlen = read;
	ops->oobretlen = oobread;

- if (mtd->ecc_stats.failed - stats.failed)
- return -EBADMSG;
-

	if (ret)
		return ret;

+ if (mtd->ecc_stats.failed - stats.failed)
+ return -EBADMSG;
+

	return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
}

@@ -944,6 +948,7 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
struct mtd_oob_ops *ops)

{
	struct onenand_chip *this = mtd->priv;
+	struct mtd_ecc_stats stats;
	int read = 0, thislen, column, oobsize;
	size_t len = ops->ooblen;
	mtd_oob_mode_t mode = ops->mode;
@@ -977,6 +982,8 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
		return -EINVAL;
	}

+ stats = mtd->ecc_stats;
+

	while (read < len) {
		cond_resched();

@@ -988,18 +995,16 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,

		onenand_update_bufferram(mtd, from, 0);

		ret = this->wait(mtd, FL_READING);

- /* First copy data and check return value for ECC handling */
+ if (ret && ret != -EBADMSG) {
+ printk(KERN_ERR "onenand_read_oob_nolock: read failed = 0x%x\n", ret);
+ break;
+ }

		if (mode == MTD_OOB_AUTO)
			onenand_transfer_auto_oob(mtd, buf, column, thislen);
		else
			this->read_bufferram(mtd, ONENAND_SPARERAM, buf, column, thislen);

- if (ret) {
- printk(KERN_ERR "onenand_read_oob_nolock: read failed = 0x%x\n", ret);
- break;
- }
-

		read += thislen;

		if (read == len)
@@ -1016,7 +1021,14 @@ static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
	}

	ops->oobretlen = read;

- return ret;
+
+ if (ret)
+ return ret;
+
+ if (mtd->ecc_stats.failed - stats.failed)
+ return -EBADMSG;
+
+ return 0;
}

/**

1.4.4.2