Skip to contentSkip to conversation

mtd: gpmi: add NAND write verify support

PATCH Superseded Huang Shijie 2 participants 5 messages Aug 11, 2012

Subsystems (2)
Memory technology devices (MTD) NAND flash

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Huang Shijie

author
#0 lore raw

PATCH v3 1/1 mtd: gpmi: add NAND write verify support

Add NAND write verify support in gpmi-nand driver.

This patch fixes a kernel crash when we select CONFIG_MTD_NAND_VERIFY_WRITE=y, and do some write tests:

root at freescale /$ dd if=/dev/zero of=/dev/mtd0 bs=128k count=1

[   41.070000] Unable to handle kernel NULL pointer dereference at virtual addr0
[   41.080000] pgd = c77ac000
[   41.080000] [00000000] *pgd=4773c831, *pte=00000000, *ppte=00000000
[   41.090000] Internal error: Oops: 17 [#1] ARM
[   41.090000] Modules linked in:
[   41.090000] CPU: 0    Not tainted (3.6.0-rc1-next-20120809-00002-ga25d017-d)
[   41.090000] PC is at nand_verify_buf+0x18/0x5c
[   41.090000] LR is at nand_write_page+0xc0/0x140
[   41.090000] pc : [<c023e7d4>]    lr : [<c023fcfc>]    psr: 20000013
[   41.090000] sp : c7779de4  ip : 00000000  fp : c6e60000
[   41.090000] r10: 00000000  r9 : 00020000  r8 : 00000000
[   41.090000] r7 : 00000000  r6 : c6e60000  r5 : c762cb10  r4 : 00000000
[   41.090000] r3 : c762c8a8  r2 : 00000800  r1 : c6e60000  r0 : 00000000
[   41.090000] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[   41.090000] Control: 0005317f  Table: 477ac000  DAC: 00000015
[   41.090000] Process dd (pid: 427, stack limit = 0xc7778270)
[   41.090000] Stack: (0xc7779de4 to 0xc777a000)
	...
Cc <[email protected]>
Signed-off-by Huang Shijie <[email protected]>

---
v2 --> v3:
[1] free the verify_buf when we exit.

v1 --> v2:

    [1] use the mtd->writesize to allocate the verify buffer.
    [2] add new field `verify_buf` in gpmi_nand_data{}

    tested this patch on imx6q-arm2 board.
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   25 +++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +25 -0
index 8c0d2f0..e1e4c51 100644
@@ -1512,12 +1512,34 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data  *this)
 	return nand_boot_init(this);
 }
 
+static int gpmi_verify_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct gpmi_nand_data *this = chip->priv;
+	int ret;
+
+	ret = chip->ecc.read_page(mtd, chip, this->verify_buf, 0, 0);
+	if (ret)
+		return -EFAULT;
+	if (memcmp(buf, this->verify_buf, len))
+		return -EFAULT;
+	return 0;
+}
+
 static int gpmi_scan_bbt(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
 	struct gpmi_nand_data *this = chip->priv;
 	int ret;
 
+	/*
+	 * We have gotten the right page size now, use it to allocate
+	 * the proper verify buffer.
+	 */
+	this->verify_buf = kzalloc(mtd->writesize, GFP_KERNEL);
+	if (!this->verify_buf)
+		return -ENOMEM;
+
 	/* Prepare for the BBT scan. */
 	ret = gpmi_pre_bbt_scan(this);
 	if (ret)

@@ -1531,6 +1553,8 @@ void gpmi_nfc_exit(struct gpmi_nand_data *this)
 {
 	nand_release(&this->mtd);
 	gpmi_free_dma_buffer(this);
+	kfree(this->verify_buf);
+	this->verify_buf = NULL;
 }
 
 static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)

@@ -1556,6 +1580,7 @@ static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)
 	chip->read_byte		= gpmi_read_byte;
 	chip->read_buf		= gpmi_read_buf;
 	chip->write_buf		= gpmi_write_buf;
+	chip->verify_buf	= gpmi_verify_buf;
 	chip->ecc.read_page	= gpmi_ecc_read_page;
 	chip->ecc.write_page	= gpmi_ecc_write_page;
 	chip->ecc.read_oob	= gpmi_ecc_read_oob;
drivers/mtd/nand/gpmi-nand/gpmi-nand.h +1 -0
index 1547a60..8ddf115 100644
@@ -144,6 +144,7 @@ struct gpmi_nand_data {
 	/* MTD / NAND */
 	struct nand_chip	nand;
 	struct mtd_info		mtd;
+	uint8_t			*verify_buf;
 
 	/* General-use Variables */
 	int			current_chip;
Signature · 1 line
1.7.4.4
#1 lore raw

PATCH v3 1/1 mtd: gpmi: add NAND write verify support

Dear Huang Shijie,

Please re-add credit to Fabio. Also, Fabio fixed the subject to be more accurate. Your subject makes this sound like it's adding a feature that can be postponed, yet this fixes a serious problem with the driver.

Quoted 65 lines

Add NAND write verify support in gpmi-nand driver.

This patch fixes a kernel crash when we select CONFIG_MTD_NAND_VERIFY_WRITE=y, and do some write tests:

root at freescale /$ dd if=/dev/zero of=/dev/mtd0 bs=128k count=1

[   41.070000] Unable to handle kernel NULL pointer dereference at virtual
addr0 [   41.080000] pgd = c77ac000
[   41.080000] [00000000] *pgd=4773c831, *pte=00000000, *ppte=00000000
[   41.090000] Internal error: Oops: 17 [#1] ARM
[   41.090000] Modules linked in:
[   41.090000] CPU: 0    Not tainted
(3.6.0-rc1-next-20120809-00002-ga25d017-d) [   41.090000] PC is at
nand_verify_buf+0x18/0x5c
[   41.090000] LR is at nand_write_page+0xc0/0x140
[   41.090000] pc : [<c023e7d4>]    lr : [<c023fcfc>]    psr: 20000013
[   41.090000] sp : c7779de4  ip : 00000000  fp : c6e60000
[   41.090000] r10: 00000000  r9 : 00020000  r8 : 00000000
[   41.090000] r7 : 00000000  r6 : c6e60000  r5 : c762cb10  r4 : 00000000
[   41.090000] r3 : c762c8a8  r2 : 00000800  r1 : c6e60000  r0 : 00000000
[   41.090000] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment
user [   41.090000] Control: 0005317f  Table: 477ac000  DAC: 00000015
[   41.090000] Process dd (pid: 427, stack limit = 0xc7778270)
[   41.090000] Stack: (0xc7779de4 to 0xc777a000)
	...
Cc <[email protected]>
Signed-off-by Huang Shijie <[email protected]>

---
v2 --> v3:
[1] free the verify_buf when we exit.

v1 --> v2:

    [1] use the mtd->writesize to allocate the verify buffer.
    [2] add new field `verify_buf` in gpmi_nand_data{}

    tested this patch on imx6q-arm2 board.
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   25 +++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)
(quoted diff) +0 -0

b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 8c0d2f0..e1e4c51 100644

drivers/mtd/nand/gpmi-nand/gpmi-nand.c +13 -0
@@ -1512,12 +1512,34 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data 
 *this) return nand_boot_init(this);
 }
 
+static int gpmi_verify_buf(struct mtd_info *mtd, const uint8_t *buf, int
 len) +{
+	struct nand_chip *chip = mtd->priv;
+	struct gpmi_nand_data *this = chip->priv;
+	int ret;
+
+	ret = chip->ecc.read_page(mtd, chip, this->verify_buf, 0, 0);
+	if (ret)
+		return -EFAULT;
+	if (memcmp(buf, this->verify_buf, len))
+		return -EFAULT;
+	return 0;
+}
+
 static int gpmi_scan_bbt(struct mtd_info *mtd)

drivers/mtd/nand/gpmi-nand/gpmi-nand.c static int gpmi_pre_bbt_scan(struct gpmi_nand_data · line 1539

Quoted 10 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +5 -0
 {
 	struct nand_chip *chip = mtd->priv;
 	struct gpmi_nand_data *this = chip->priv;
 	int ret;
 
+	/*
+	 * We have gotten the right page size now, use it to allocate
+	 * the proper verify buffer.
+	 */
+	this->verify_buf = kzalloc(mtd->writesize, GFP_KERNEL);

devm_ will work here just fine.

drivers/mtd/nand/gpmi-nand/gpmi-nand.c void gpmi_nfc_exit(struct gpmi_nand_data *this) · line 1556

Quoted 11 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +4 -0
+	if (!this->verify_buf)
+		return -ENOMEM;
+
 	/* Prepare for the BBT scan. */
 	ret = gpmi_pre_bbt_scan(this);
 	if (ret)

@@ -1531,6 +1553,8 @@ void gpmi_nfc_exit(struct gpmi_nand_data *this)
 {
 	nand_release(&this->mtd);
 	gpmi_free_dma_buffer(this);
+	kfree(this->verify_buf);

Then you can drop this altogether.

Quoted 17 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +2 -0
+	this->verify_buf = NULL;
 }
 
 static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)

@@ -1556,6 +1580,7 @@ static int __devinit gpmi_nfc_init(struct
 gpmi_nand_data *this) chip->read_byte		= gpmi_read_byte;
 	chip->read_buf		= gpmi_read_buf;
 	chip->write_buf		= gpmi_write_buf;
+	chip->verify_buf	= gpmi_verify_buf;
 	chip->ecc.read_page	= gpmi_ecc_read_page;
 	chip->ecc.write_page	= gpmi_ecc_write_page;
 	chip->ecc.read_oob	= gpmi_ecc_read_oob;
(quoted diff) +0 -0

b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h index 1547a60..8ddf115 100644

drivers/mtd/nand/gpmi-nand/gpmi-nand.h struct gpmi_nand_data { · line 150

Quoted 7 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.h +1 -0
@@ -144,6 +144,7 @@ struct gpmi_nand_data {
 	/* MTD / NAND */
 	struct nand_chip	nand;
 	struct mtd_info		mtd;
+	uint8_t			*verify_buf;
 
 	/* General-use Variables */
 	int			current_chip;

Best regards,
Marek Vasut

author
#2 lore raw

PATCH v3 1/1 mtd: gpmi: add NAND write verify support

Quoted 3 lines · Marek Vasut

Dear Huang Shijie,

Please re-add credit to Fabio. Also, Fabio fixed the subject to be more

sorry. Should I add "Reported by : Fabio ...." to the patch?

I really appriaciate Fabio's work, but I do not think Fabio's patch is more accurate.
The nand chips can be 2K page, 4K page and 8K page. But most of the time, the nand chip is 4K page.
If we fix the verify_buf to 8K size as Fabio's patch does. It will waste much memory.
This is my concern now. :)

My patch maybe not good. So If Fabio also sends out a new patch which allocates the verify_buf with the mtd->writesize size.
I will ack it.

Quoted 1 line

accurate. Your subject makes this sound like it's adding a feature that can be

Please check the code drivers/mtd/onenand/onenand_base.c: 4008. This driver implements the verify_buf in a more smart way. It does not waste much memory.

thanks
Huang Shijie

Quoted 121 lines

postponed, yet this fixes a serious problem with the driver.

Quoted 65 lines

Add NAND write verify support in gpmi-nand driver.

This patch fixes a kernel crash when we select CONFIG_MTD_NAND_VERIFY_WRITE=y, and do some write tests:

root at freescale /$ dd if=/dev/zero of=/dev/mtd0 bs=128k count=1

[   41.070000] Unable to handle kernel NULL pointer dereference at virtual
addr0 [   41.080000] pgd = c77ac000
[   41.080000] [00000000] *pgd=4773c831, *pte=00000000, *ppte=00000000
[   41.090000] Internal error: Oops: 17 [#1] ARM
[   41.090000] Modules linked in:
[   41.090000] CPU: 0    Not tainted
(3.6.0-rc1-next-20120809-00002-ga25d017-d) [   41.090000] PC is at
nand_verify_buf+0x18/0x5c
[   41.090000] LR is at nand_write_page+0xc0/0x140
[   41.090000] pc : [<c023e7d4>]    lr : [<c023fcfc>]    psr: 20000013
[   41.090000] sp : c7779de4  ip : 00000000  fp : c6e60000
[   41.090000] r10: 00000000  r9 : 00020000  r8 : 00000000
[   41.090000] r7 : 00000000  r6 : c6e60000  r5 : c762cb10  r4 : 00000000
[   41.090000] r3 : c762c8a8  r2 : 00000800  r1 : c6e60000  r0 : 00000000
[   41.090000] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment
user [   41.090000] Control: 0005317f  Table: 477ac000  DAC: 00000015
[   41.090000] Process dd (pid: 427, stack limit = 0xc7778270)
[   41.090000] Stack: (0xc7779de4 to 0xc777a000)
      ...
Cc <[email protected]>
Signed-off-by Huang Shijie <[email protected]>

---
v2 --> v3:
[1] free the verify_buf when we exit.

v1 --> v2:

    [1] use the mtd->writesize to allocate the verify buffer.
    [2] add new field `verify_buf` in gpmi_nand_data{}

    tested this patch on imx6q-arm2 board.
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   25 +++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)
(quoted diff) +0 -0

b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 8c0d2f0..e1e4c51 100644

drivers/mtd/nand/gpmi-nand/gpmi-nand.c +13 -0
@@ -1512,12 +1512,34 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data
 *this) return nand_boot_init(this);
 }
 
+static int gpmi_verify_buf(struct mtd_info *mtd, const uint8_t *buf, int
 len) +{
+     struct nand_chip *chip = mtd->priv;
+     struct gpmi_nand_data *this = chip->priv;
+     int ret;
+
+     ret = chip->ecc.read_page(mtd, chip, this->verify_buf, 0, 0);
+     if (ret)
+             return -EFAULT;
+     if (memcmp(buf, this->verify_buf, len))
+             return -EFAULT;
+     return 0;
+}
+
 static int gpmi_scan_bbt(struct mtd_info *mtd)

drivers/mtd/nand/gpmi-nand/gpmi-nand.c static int gpmi_pre_bbt_scan(struct gpmi_nand_data · line 1539

Quoted 10 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +5 -0
 {
      struct nand_chip *chip = mtd->priv;
      struct gpmi_nand_data *this = chip->priv;
      int ret;
 
+     /*
+      * We have gotten the right page size now, use it to allocate
+      * the proper verify buffer.
+      */
+     this->verify_buf = kzalloc(mtd->writesize, GFP_KERNEL);

devm_ will work here just fine.

drivers/mtd/nand/gpmi-nand/gpmi-nand.c void gpmi_nfc_exit(struct gpmi_nand_data *this) · line 1556

Quoted 11 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +4 -0
+     if (!this->verify_buf)
+             return -ENOMEM;
+
      /* Prepare for the BBT scan. */
      ret = gpmi_pre_bbt_scan(this);
      if (ret)

@@ -1531,6 +1553,8 @@ void gpmi_nfc_exit(struct gpmi_nand_data *this)
 {
      nand_release(&this->mtd);
      gpmi_free_dma_buffer(this);
+     kfree(this->verify_buf);

Then you can drop this altogether.

Quoted 17 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +2 -0
+     this->verify_buf = NULL;
 }
 
 static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)

@@ -1556,6 +1580,7 @@ static int __devinit gpmi_nfc_init(struct
 gpmi_nand_data *this) chip->read_byte         = gpmi_read_byte;
      chip->read_buf          = gpmi_read_buf;
      chip->write_buf         = gpmi_write_buf;
+     chip->verify_buf        = gpmi_verify_buf;
      chip->ecc.read_page     = gpmi_ecc_read_page;
      chip->ecc.write_page    = gpmi_ecc_write_page;
      chip->ecc.read_oob      = gpmi_ecc_read_oob;
(quoted diff) +0 -0

b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h index 1547a60..8ddf115 100644

drivers/mtd/nand/gpmi-nand/gpmi-nand.h struct gpmi_nand_data { · line 150

Quoted 7 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.h +1 -0
@@ -144,6 +144,7 @@ struct gpmi_nand_data {
      /* MTD / NAND */
      struct nand_chip        nand;
      struct mtd_info         mtd;
+     uint8_t                 *verify_buf;
 
      /* General-use Variables */
      int                     current_chip;

Best regards,
Marek Vasut

#3 lore raw

PATCH v3 1/1 mtd: gpmi: add NAND write verify support

Dear Huang Shijie,

Quoted 6 lines · Marek Vasut
Quoted 3 lines

Dear Huang Shijie,

Please re-add credit to Fabio. Also, Fabio fixed the subject to be more

sorry. Should I add "Reported by : Fabio ...." to the patch?

Yes, at least.

Quoted 7 lines

I really appriaciate Fabio's work, but I do not think Fabio's patch is more accurate.
The nand chips can be 2K page, 4K page and 8K page. But most of the time, the nand chip is 4K page.
If we fix the verify_buf to 8K size as Fabio's patch does. It will waste much memory.
This is my concern now. :)

That's all right, but the rest of your patch is Fabio's work, correct me if I'm wrong. Just give the guy some credit, he found the flaw and diag'd it.

Quoted 3 lines

My patch maybe not good. So If Fabio also sends out a new patch which allocates the verify_buf with the mtd->writesize size.
I will ack it.

Either way is OK.

[...]

Quoted 2 lines
Quoted 2 lines

Best regards,
Marek Vasut

author
#4 lore raw

PATCH v3 1/1 mtd: gpmi: add NAND write verify support

Quoted 19 lines · Marek Vasut

Dear Huang Shijie,

Quoted 6 lines · Marek Vasut
Quoted 3 lines

Dear Huang Shijie,

Please re-add credit to Fabio. Also, Fabio fixed the subject to be more

sorry. Should I add "Reported by : Fabio ...." to the patch?

Yes, at least.

Quoted 7 lines

I really appriaciate Fabio's work, but I do not think Fabio's patch is more accurate.
The nand chips can be 2K page, 4K page and 8K page. But most of the time, the nand chip is 4K page.
If we fix the verify_buf to 8K size as Fabio's patch does. It will waste much memory.
This is my concern now. :)

Quoted 1 line

That's all right, but the rest of your patch is Fabio's work, correct me if I'm

Frankly speaking, this patch was finished after I read your comments, and before Fabio sent out his patch.
I did not send it out, just because i did not test it on the imx6q-arm2 board.

thanks

Huang Shijie