onenand_wait and onenane_read

onenand_wait and onenane_read

[Adrian Hunter]

Hi

onenand_wait does not return an error if there is an ecc failure i.e.

ecc & ONENAND_ECC_2BIT_ALL

but onenand_read still marks the bufferRAM as valid i.e.

ret = this->wait(mtd, FL_READING);
/* First copy data and check return value for ECC handling */
onenand_update_bufferram(mtd, from, !ret);

This looks wrong to me.  I think onenand_wait should return an error
if there is an ecc failure.

Can you comment on this?

Regards
Adrian

onenand_wait and onenane_read

[Kyungmin Park]

Hi,

> onenand_wait does not return an error if there is an ecc failure i.e.
> 
> ecc & ONENAND_ECC_2BIT_ALL
> 
> but onenand_read still marks the bufferRAM as valid i.e.
> 
> ret = this->wait(mtd, FL_READING);
> /* First copy data and check return value for ECC handling */
> onenand_update_bufferram(mtd, from, !ret);
> 
> This looks wrong to me.  I think onenand_wait should return an error
> if there is an ecc failure.

Yes you're right.

Even though upper layer (JFFS2) receive the -EBADMSG or -EUCLEAN, OneNAND driver can think it is valid data.

Thank you,
Kyungmin Park

From 9235da2851242b96a938d43583bf88427ed14044 Mon Sep 17 00:00:00 2001
From: Kyungmin Park <kyungmin.park@samsung.com>
Date: Fri, 29 Dec 2006 11:51:40 +0900
Subject: [PATCH] [MTD] OneNAND: fix onenand_wait bug in read ecc error

Even though there is ECC error. OneNAND driver updates the bufferram as valid

Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mtd/onenand/onenand_base.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index 0037cee..e80857b 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -298,7 +298,7 @@ static int onenand_wait(struct mtd_info *mtd, int state)
 	unsigned long timeout;
 	unsigned int flags = ONENAND_INT_MASTER;
 	unsigned int interrupt = 0;
-	unsigned int ctrl, ecc;
+	unsigned int ctrl;
 
 	/* The 20 msec is enough */
 	timeout = jiffies + msecs_to_jiffies(20);
@@ -324,7 +324,7 @@ static int onenand_wait(struct mtd_info *mtd, int state)
 	}
 
 	if (interrupt & ONENAND_INT_READ) {
-		ecc = this->read_word(this->base + ONENAND_REG_ECC_STATUS);
+		int ecc = this->read_word(this->base + ONENAND_REG_ECC_STATUS);
 		if (ecc) {
 			DEBUG(MTD_DEBUG_LEVEL0, "onenand_wait: ECC error = 0x%04x\n", ecc);
 			if (ecc & ONENAND_ECC_2BIT_ALL)
@@ -332,6 +332,7 @@ static int onenand_wait(struct mtd_info *mtd, int state)
 			else if (ecc & ONENAND_ECC_1BIT_ALL)
 				mtd->ecc_stats.corrected++;
 		}
+		return ecc;
 	}
 
 	return 0;
-- 
1.4.4.2

Re: onenand_wait and onenane_read

[Adrian Hunter]

Thanks for looking at this.

Note that after the patch, onenane_wait seems to return non-zero even
when the error is corrected by ecc.  Shouldn't it return zero in that
case?

onenand_wait and onenane_read

[Kyungmin Park]

Hi,

> Note that after the patch, onenane_wait seems to return non-zero even
> when the error is corrected by ecc.  Shouldn't it return zero in that
> case?

After ecc patch, I think is it really need to return error code when 1 bit ecc.
Even though there's 1-bit ecc the bufferram has valid data. since ecc logic already corrects it.

I think we don't need to return ecc error when 1-bit ecc. We only return error code when 2-bit ecc error

@@ -327,12 +327,12 @@ static int onenand_wait(struct mtd_info *mtd, int state)
                int ecc = this->read_word(this->base + ONENAND_REG_ECC_STATUS);
                if (ecc) {
                        DEBUG(MTD_DEBUG_LEVEL0, "onenand_wait: ECC error = 0x%04x\n", ecc);
-                       if (ecc & ONENAND_ECC_2BIT_ALL)
+                       if (ecc & ONENAND_ECC_2BIT_ALL) {
                                mtd->ecc_stats.failed++;
-                       else if (ecc & ONENAND_ECC_1BIT_ALL)
+                               return ecc;
+                       } else if (ecc & ONENAND_ECC_1BIT_ALL)
                                mtd->ecc_stats.corrected++;
                }
-               return ecc;
        }

        return 0;

Thank you,
Kyungmin Park