| Message ID | 1344700231-12624-1-git-send-email-shijie8@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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@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: <stable@vger.kernel.org> > Signed-off-by: Huang Shijie <shijie8@gmail.com> > --- > 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(-) > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 8c0d2f0..e1e4c51 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > @@ -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); devm_ will work here just fine. > + 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. > + 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; > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h index 1547a60..8ddf115 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h > @@ -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
On Sat, Aug 11, 2012 at 3:13 PM, Marek Vasut <marex@denx.de> wrote: > 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. > 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 > postponed, yet this fixes a serious problem with the driver. > >> 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@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: <stable@vger.kernel.org> >> Signed-off-by: Huang Shijie <shijie8@gmail.com> >> --- >> 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(-) >> >> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 8c0d2f0..e1e4c51 100644 >> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >> @@ -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); > > devm_ will work here just fine. > >> + 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. > >> + 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; >> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h >> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h index 1547a60..8ddf115 100644 >> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h >> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h >> @@ -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
Dear Huang Shijie, > On Sat, Aug 11, 2012 at 3:13 PM, Marek Vasut <marex@denx.de> wrote: > > 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. > 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. > 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. [...] > > Best regards, > > Marek Vasut
On Sat, Aug 11, 2012 at 8:47 AM, Marek Vasut <marex@denx.de> wrote: > Dear Huang Shijie, > >> On Sat, Aug 11, 2012 at 3:13 PM, Marek Vasut <marex@denx.de> wrote: >> > 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. > >> 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 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
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 8c0d2f0..e1e4c51 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c @@ -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; diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h index 1547a60..8ddf115 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h @@ -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;
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@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: <stable@vger.kernel.org> Signed-off-by: Huang Shijie <shijie8@gmail.com> --- 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(-)