[PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter]

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

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mtd/onenand/onenand_base.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index dd28355..616747c 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -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

Re: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Jörn Engel]

On Fri, 2 November 2007 13:30:04 +0200, Adrian Hunter wrote:
> 
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index dd28355..616747c 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -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;

Why is -EBADMSG dropped here?

> @@ -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;
> +		}

What about -EUCLEAN?

Jörn

-- 
It is better to die of hunger having lived without grief and fear,
than to live with a troubled spirit amid abundance.
-- Epictetus

Re: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter]

ext Jörn Engel wrote:
> On Fri, 2 November 2007 13:30:04 +0200, Adrian Hunter wrote:
>> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
>> index dd28355..616747c 100644
>> --- a/drivers/mtd/onenand/onenand_base.c
>> +++ b/drivers/mtd/onenand/onenand_base.c
>> @@ -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;
> 
> Why is -EBADMSG dropped here?

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

> 
>> @@ -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;
>> +		}
> 
> What about -EUCLEAN?

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.

> 
> Jörn
> 

Re: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Jörn Engel]

On Mon, 5 November 2007 16:46:13 +0200, Adrian Hunter wrote:
> 
> -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.

Hmm.

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

Those two conditionals should have reversed order, imo.

> >What about -EUCLEAN?
> 
> 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.

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

Re: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter]

ext Jörn Engel wrote:
> On Mon, 5 November 2007 16:46:13 +0200, Adrian Hunter wrote:
>> -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.
> 
> Hmm.
> 
>> 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;
> 
> Those two conditionals should have reversed order, imo.

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

> 
>>> What about -EUCLEAN?
>> 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.
> 
> Such an interface sure looks strange.  But since read_oob is strange
> anyway I don't care very much either way.
> 
> Jörn
> 

[PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter]

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

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mtd/onenand/onenand_base.c |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index dd28355..cedc154 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -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

[PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter]

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

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mtd/onenand/onenand_base.c |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index dd28355..cedc154 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -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

RE: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Kyungmin Park]

> Subject: [PATCH] [MTD] [OneNAND] 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.
> 
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>

Re: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter]

ext Kyungmin Park wrote:
>> Subject: [PATCH] [MTD] [OneNAND] 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.
>>
>> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> 

Please apply :-)

Re: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter]

ext Adrian Hunter wrote:
> ext Kyungmin Park wrote:
>>> Subject: [PATCH] [MTD] [OneNAND] 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.
>>>
>>> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>
> 
> Please apply :-)

...

Re: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter]

ext Adrian Hunter wrote:
> ext Adrian Hunter wrote:
>> ext Kyungmin Park wrote:
>>>> Subject: [PATCH] [MTD] [OneNAND] 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.
>>>>
>>>> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>
>> Please apply :-)
> 
> ...
Please apply