[PATCH] mtd/nand:Fix wrong usage of is_blank() in fsl_ifc_run_command

[PATCH] mtd/nand:Fix wrong usage of is_blank() in fsl_ifc_run_command

[Prabhakar Kushwaha]

Freescale IFC NAND Machine calculates ECC on 512byte sector and same is used in
fsl_ifc_run_command() during ECC status verification. Also this sector is passed
to is_blank() for blank checking. It is wrong at first place because
is_blank()'s implementation checks for Page size and OOB area size. 
is_blank() should be called per page for main and OOB area verification.

Variables name are redefined to avoid confusion between buffer and ecc sector.

Signed-off-by: Poonam Aggrwal <poonam.aggrwal@freescale.com>
Signed-off-by: Scott Wood <scottwood@freescale.com>
Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com>
---
 git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git (branch next)

 Tested on P1010RDB

 This patch is created on top of IFC driver patch (already floated in mailing
 list). Please find their link:
 http://patchwork.ozlabs.org/patch/133315/
 http://patchwork.ozlabs.org/patch/133316/

 This patch is replacement of my earlier patch
  "mtd/nand:Fix wrong address read in is_blank()"

 drivers/mtd/nand/fsl_ifc_nand.c |   52 +++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 854fe95..33b55d2 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -1,7 +1,7 @@
 /*
  * Freescale Integrated Flash Controller NAND driver
  *
- * Copyright 2011 Freescale Semiconductor, Inc
+ * Copyright 2011,2012 Freescale Semiconductor, Inc
  *
  * Author: Dipen Dudhat <Dipen.Dudhat@freescale.com>
  *
@@ -216,24 +216,11 @@ static int check_read_ecc(struct mtd_info *mtd, struct fsl_ifc_ctrl *ctrl,
 			  u32 *eccstat, unsigned int bufnum)
 {
 	u32 reg = eccstat[bufnum / 4];
-	int errors = (reg >> ((3 - bufnum % 4) * 8)) & 15;
+	int errors;
 
-	if (errors == 15) { /* uncorrectable */
-		/* Blank pages fail hw ECC checks */
-		if (is_blank(mtd, bufnum))
-			return 1;
+	errors = (reg >> ((3 - bufnum % 4) * 8)) & 15;
 
-		/*
-		 * We disable ECCER reporting in hardware due to
-		 * erratum IFC-A002770 -- so report it now if we
-		 * see an uncorrectable error in ECCSTAT.
-		 */
-		ctrl->nand_stat |= IFC_NAND_EVTER_STAT_ECCER;
-	} else if (errors > 0) {
-		mtd->ecc_stats.corrected += errors;
-	}
-
-	return 0;
+	return errors;
 }
 
 /*
@@ -273,16 +260,33 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
 		dev_err(priv->dev, "NAND Flash Write Protect Error\n");
 
 	if (nctrl->eccread) {
-		int bufperpage = mtd->writesize / 512;
-		int bufnum = (nctrl->page & priv->bufnum_mask) * bufperpage;
-		int bufnum_end = bufnum + bufperpage - 1;
+		int errors;
+		int bufnum = nctrl->page & priv->bufnum_mask;
+		int sector = bufnum * chip->ecc.steps;
+		int sector_end = sector + chip->ecc.steps - 1;
 
-		for (i = bufnum / 4; i <= bufnum_end / 4; i++)
+		for (i = sector / 4; i <= sector_end / 4; i++)
 			eccstat[i] = in_be32(&ifc->ifc_nand.nand_eccstat[i]);
 
-		for (i = bufnum; i <= bufnum_end; i++) {
-			if (check_read_ecc(mtd, ctrl, eccstat, i))
+		for (i = sector; i <= sector_end; i++) {
+			errors = check_read_ecc(mtd, ctrl, eccstat, i);
+
+			if (errors == 15) {
+				/*
+				 * Uncorrectable error.
+				 * OK only if the whole page is blank.
+				 *
+				 * We disable ECCER reporting due to...
+				 * erratum IFC-A002770 -- so report it now if we
+				 * see an uncorrectable error in ECCSTAT.
+				 */
+				if (!is_blank(mtd, bufnum))
+					ctrl->nand_stat |=
+						IFC_NAND_EVTER_STAT_ECCER;
 				break;
+			}
+
+			mtd->ecc_stats.corrected += errors;
 		}
 
 		nctrl->eccread = 0;
-- 
1.7.5.4

Re: [PATCH] mtd/nand:Fix wrong usage of is_blank() in fsl_ifc_run_command

[Scott Wood]

On 01/17/2012 10:13 PM, Prabhakar Kushwaha wrote:
> +		for (i = sector; i <= sector_end; i++) {
> +			errors = check_read_ecc(mtd, ctrl, eccstat, i);
> +
> +			if (errors == 15) {
> +				/*
> +				 * Uncorrectable error.
> +				 * OK only if the whole page is blank.
> +				 *
> +				 * We disable ECCER reporting due to...

s/due to.../due to/

Otherwise looks good.

-Scott

Re: [PATCH] mtd/nand:Fix wrong usage of is_blank() in fsl_ifc_run_command

[Artem Bityutskiy]

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

On Wed, 2012-01-18 at 09:43 +0530, Prabhakar Kushwaha wrote:
> Freescale IFC NAND Machine calculates ECC on 512byte sector and same is used in
> fsl_ifc_run_command() during ECC status verification. Also this sector is passed
> to is_blank() for blank checking. It is wrong at first place because
> is_blank()'s implementation checks for Page size and OOB area size. 
> is_blank() should be called per page for main and OOB area verification.
> 
> Variables name are redefined to avoid confusion between buffer and ecc sector.
> 
> Signed-off-by: Poonam Aggrwal <poonam.aggrwal@freescale.com>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com>

The driver is not in 3.3-rc1, so I skip this patch.

-- 
Best Regards,
Artem Bityutskiy

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

RE: [PATCH] mtd/nand:Fix wrong usage of is_blank() in fsl_ifc_run_command

[Kushwaha Prabhakar-B32579]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQXJ0ZW0gQml0eXV0c2tp
eSBbbWFpbHRvOmRlZGVraW5kMUBnbWFpbC5jb21dDQo+IFNlbnQ6IEZyaWRheSwgSmFudWFyeSAy
NywgMjAxMiA2OjQ2IFBNDQo+IFRvOiBLdXNod2FoYSBQcmFiaGFrYXItQjMyNTc5DQo+IENjOiBs
aW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZzsgbGludXgtbXRkQGxpc3RzLmluZnJhZGVhZC5v
cmc7IFdvb2QNCj4gU2NvdHQtQjA3NDIxOyBBZ2dyd2FsIFBvb25hbS1CMTA4MTINCj4gU3ViamVj
dDogUmU6IFtQQVRDSF0gbXRkL25hbmQ6Rml4IHdyb25nIHVzYWdlIG9mIGlzX2JsYW5rKCkgaW4N
Cj4gZnNsX2lmY19ydW5fY29tbWFuZA0KPiANCj4gT24gV2VkLCAyMDEyLTAxLTE4IGF0IDA5OjQz
ICswNTMwLCBQcmFiaGFrYXIgS3VzaHdhaGEgd3JvdGU6DQo+ID4gRnJlZXNjYWxlIElGQyBOQU5E
IE1hY2hpbmUgY2FsY3VsYXRlcyBFQ0Mgb24gNTEyYnl0ZSBzZWN0b3IgYW5kIHNhbWUNCj4gPiBp
cyB1c2VkIGluDQo+ID4gZnNsX2lmY19ydW5fY29tbWFuZCgpIGR1cmluZyBFQ0Mgc3RhdHVzIHZl
cmlmaWNhdGlvbi4gQWxzbyB0aGlzIHNlY3Rvcg0KPiA+IGlzIHBhc3NlZCB0byBpc19ibGFuaygp
IGZvciBibGFuayBjaGVja2luZy4gSXQgaXMgd3JvbmcgYXQgZmlyc3QgcGxhY2UNCj4gPiBiZWNh
dXNlIGlzX2JsYW5rKCkncyBpbXBsZW1lbnRhdGlvbiBjaGVja3MgZm9yIFBhZ2Ugc2l6ZSBhbmQg
T09CIGFyZWENCj4gc2l6ZS4NCj4gPiBpc19ibGFuaygpIHNob3VsZCBiZSBjYWxsZWQgcGVyIHBh
Z2UgZm9yIG1haW4gYW5kIE9PQiBhcmVhDQo+IHZlcmlmaWNhdGlvbi4NCj4gPg0KPiA+IFZhcmlh
YmxlcyBuYW1lIGFyZSByZWRlZmluZWQgdG8gYXZvaWQgY29uZnVzaW9uIGJldHdlZW4gYnVmZmVy
IGFuZCBlY2MNCj4gc2VjdG9yLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogUG9vbmFtIEFnZ3J3
YWwgPHBvb25hbS5hZ2dyd2FsQGZyZWVzY2FsZS5jb20+DQo+ID4gU2lnbmVkLW9mZi1ieTogU2Nv
dHQgV29vZCA8c2NvdHR3b29kQGZyZWVzY2FsZS5jb20+DQo+ID4gU2lnbmVkLW9mZi1ieTogUHJh
Ymhha2FyIEt1c2h3YWhhIDxwcmFiaGFrYXJAZnJlZXNjYWxlLmNvbT4NCj4gDQo+IFRoZSBkcml2
ZXIgaXMgbm90IGluIDMuMy1yYzEsIHNvIEkgc2tpcCB0aGlzIHBhdGNoLg0KPiANCg0KDQpUaGlz
IHBhdGNoIGlzIHNxdWFzaGVkIGluIExpbnV4IE5BTkQgZHJpdmVyIHBhdGNoLiBJdCB3aWxsIGJl
IHByb3ZpZGVkIGJ5IGdhbGFrJ3MgdHJlZQ0KDQoiTkFORCBNYWNoaW5lIHN1cHBvcnQgZm9yIElu
dGVncmF0ZWQgRmxhc2ggQ29udHJvbGxlciINCmh0dHA6Ly9wYXRjaHdvcmsub3psYWJzLm9yZy9w
YXRjaC8xMzcwMjQvDQoNCi0tUHJhYmhha2FyDQoNCg==