[PATCH 1/2] mtd: gpmi: fix the NULL pointer

[PATCH 1/2] mtd: gpmi: fix the NULL pointer

[Huang Shijie]

The imx23 board will check the fingerprint, so it will call the
mx23_check_transcription_stamp. This function will use @chip->buffers->databuf
as its buffer which is allocated in the nand_scan_tail().

Unfortunately, the mx23_check_transcription_stamp is called before the
nand_scan_tail(). So we will meet a NULL pointer bug:

--------------------------------------------------------------------
[    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
[    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
[    1.170000] pgd = c0004000
[    1.170000] [000005d0] *pgd=00000000
[    1.180000] Internal error: Oops: 5 [#1] ARM
[    1.180000] Modules linked in:
[    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
[    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
[    1.180000] PC is at memcmp+0x10/0x54
[    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
[    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
[    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
[    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
[    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
[    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
[    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
[    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
[    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)
--------------------------------------------------------------------

This patch uses the local array(whose size is 4 bytes) to replace
the referencing of the @chip->buffers->databuf.

Use the macro FINGERPRINT_LEN to avoid the hardcode.

Cc: stable@vger.kernel.org 
Reported-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 7ac2280..d34cc54 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1343,6 +1343,7 @@ static int nand_boot_set_geometry(struct gpmi_nand_data *this)
 }
 
 static const char  *fingerprint = "STMP";
+#define FINGERPRINT_LEN		4
 static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
 {
 	struct boot_rom_geometry *rom_geo = &this->rom_geometry;
@@ -1352,7 +1353,7 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
 	unsigned int search_area_size_in_strides;
 	unsigned int stride;
 	unsigned int page;
-	uint8_t *buffer = chip->buffers->databuf;
+	uint8_t buffer[FINGERPRINT_LEN + 1];
 	int saved_chip_number;
 	int found_an_ncb_fingerprint = false;
 
-- 
1.7.2.rc3

[PATCH 2/2] mtd: gpmi: only scan two chips for imx6

[Huang Shijie]

We cannot scan two chips for imx23 and imx28:
  imx23: the Ready-Busy1 line is not connected for some board.
  imx28: we do not set the pinctrl for Ready-Busy1

So we only scan two chips for imx6.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index d34cc54..00ea9c4 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1675,7 +1675,7 @@ static int gpmi_nfc_init(struct gpmi_nand_data *this)
 	if (ret)
 		goto err_out;
 
-	ret = nand_scan_ident(mtd, 2, NULL);
+	ret = nand_scan_ident(mtd, GPMI_IS_MX6Q(this) ? 2 : 1, NULL);
 	if (ret)
 		goto err_out;
 
-- 
1.7.2.rc3

Re: [PATCH 1/2] mtd: gpmi: fix the NULL pointer

[Fabio Estevam]

Hi Huang,

On Thu, Nov 7, 2013 at 7:46 AM, Huang Shijie <b32955@freescale.com> wrote:
> The imx23 board will check the fingerprint, so it will call the
> mx23_check_transcription_stamp. This function will use @chip->buffers->databuf
> as its buffer which is allocated in the nand_scan_tail().
>
> Unfortunately, the mx23_check_transcription_stamp is called before the
> nand_scan_tail(). So we will meet a NULL pointer bug:
>
> --------------------------------------------------------------------
> [    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
> [    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
> [    1.170000] pgd = c0004000
> [    1.170000] [000005d0] *pgd=00000000
> [    1.180000] Internal error: Oops: 5 [#1] ARM
> [    1.180000] Modules linked in:
> [    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
> [    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
> [    1.180000] PC is at memcmp+0x10/0x54
> [    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
> [    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
> [    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
> [    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
> [    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
> [    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
> [    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> [    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
> [    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)
> --------------------------------------------------------------------
>
> This patch uses the local array(whose size is 4 bytes) to replace
> the referencing of the @chip->buffers->databuf.
>
> Use the macro FINGERPRINT_LEN to avoid the hardcode.
>
> Cc: stable@vger.kernel.org
> Reported-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 7ac2280..d34cc54 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1343,6 +1343,7 @@ static int nand_boot_set_geometry(struct gpmi_nand_data *this)
>  }
>
>  static const char  *fingerprint = "STMP";
> +#define FINGERPRINT_LEN                4
>  static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
>  {
>         struct boot_rom_geometry *rom_geo = &this->rom_geometry;
> @@ -1352,7 +1353,7 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
>         unsigned int search_area_size_in_strides;
>         unsigned int stride;
>         unsigned int page;
> -       uint8_t *buffer = chip->buffers->databuf;
> +       uint8_t buffer[FINGERPRINT_LEN + 1];

Why buffer[FINGERPRINT_LEN + 1] instead of buffer[FINGERPRINT_LEN] ?

Regards,

Fabio Estevam

Re: [PATCH 2/2] mtd: gpmi: only scan two chips for imx6

[Fabio Estevam]

On Thu, Nov 7, 2013 at 7:46 AM, Huang Shijie <b32955@freescale.com> wrote:
> We cannot scan two chips for imx23 and imx28:
>   imx23: the Ready-Busy1 line is not connected for some board.
>   imx28: we do not set the pinctrl for Ready-Busy1
>
> So we only scan two chips for imx6.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>

You missed to Cc stable on this one.

If only 1/2 gets into stable we still see the crash.

Regards,

Fabio Estevam

Re: [PATCH 2/2] mtd: gpmi: only scan two chips for imx6

[Fabio Estevam]

On Thu, Nov 7, 2013 at 7:46 AM, Huang Shijie <b32955@freescale.com> wrote:
> We cannot scan two chips for imx23 and imx28:
>   imx23: the Ready-Busy1 line is not connected for some board.
>   imx28: we do not set the pinctrl for Ready-Busy1
>
> So we only scan two chips for imx6.

What happens if Ready-Busy1 line is not connected on some mx6 board?
You cannot assume that all boards
have this line connected and configured in pinctrl just like you
mentioned for mx23 and mx28.

Regards,

Fabio Estevam

Re: [PATCH 1/2] mtd: gpmi: fix the NULL pointer

[Huang Shijie]

于 2013年11月07日 21:07, Fabio Estevam 写道:
> Why buffer[FINGERPRINT_LEN + 1] instead of buffer[FINGERPRINT_LEN] ?
just for the safe guard. "+1" is to simulate the terminal '\0' of the 
string.
Of course, it's ok to use the buffer[FINGERPINGT_LEN].

thanks
Huang Shijie

Re: [PATCH 2/2] mtd: gpmi: only scan two chips for imx6

[Huang Shijie]

于 2013年11月07日 21:08, Fabio Estevam 写道:
> You missed to Cc stable on this one.
>
this patch has not been merged by linus's tree.
so it is not need to CC stable.

thanks
Huang Shijie

Re: [PATCH 2/2] mtd: gpmi: only scan two chips for imx6

[Huang Shijie]

于 2013年11月08日 11:11, Huang Shijie 写道:
> this patch has not been merged by linus's tree. 
I meant the patch:
"cdef39d mtd: gpmi: scan two nand chips"

Huang Shijie

Re: [PATCH 2/2] mtd: gpmi: only scan two chips for imx6

[Huang Shijie]

于 2013年11月07日 21:22, Fabio Estevam 写道:
> On Thu, Nov 7, 2013 at 7:46 AM, Huang Shijie<b32955@freescale.com>  wrote:
>> We cannot scan two chips for imx23 and imx28:
>>    imx23: the Ready-Busy1 line is not connected for some board.
>>    imx28: we do not set the pinctrl for Ready-Busy1
>>
>> So we only scan two chips for imx6.
> What happens if Ready-Busy1 line is not connected on some mx6 board?
> You cannot assume that all boards
we will meet a DMA timeout.

All the imx6's boards supporting the gpmi have the RB1 now.
the customer's boards will follow our boards, so their boards also 
should have the RB1 line too.

> have this line connected and configured in pinctrl just like you
> mentioned for mx23 and mx28.
Please check the schematics for imx23/imx28/imx6x.
For imx6, the line is OR-combined with the RB0.
For imx23-evk,imx28-evk, the RB1 line is a seperate line, not OR with 
the RB0.
     and the pinctrls does not have RB1.

thanks
Huang Shijie

Re: [PATCH 1/2] mtd: gpmi: fix the NULL pointer

[Fabio Estevam]

On Fri, Nov 8, 2013 at 1:10 AM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年11月07日 21:07, Fabio Estevam 写道:
>
>> Why buffer[FINGERPRINT_LEN + 1] instead of buffer[FINGERPRINT_LEN] ?
>
> just for the safe guard. "+1" is to simulate the terminal '\0' of the
> string.
> Of course, it's ok to use the buffer[FINGERPINGT_LEN].

Then please send a v2 without the +1. Let's save one byte ;-)

Regards,

Fabio Estevam

Re: [PATCH 1/2] mtd: gpmi: fix the NULL pointer

[Brian Norris]

On Thu, Nov 07, 2013 at 05:46:36PM +0800, Huang Shijie wrote:
> The imx23 board will check the fingerprint, so it will call the
> mx23_check_transcription_stamp. This function will use @chip->buffers->databuf
> as its buffer which is allocated in the nand_scan_tail().
> 
> Unfortunately, the mx23_check_transcription_stamp is called before the
> nand_scan_tail(). So we will meet a NULL pointer bug:
> 
> --------------------------------------------------------------------
> [    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
> [    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
> [    1.170000] pgd = c0004000
> [    1.170000] [000005d0] *pgd=00000000
> [    1.180000] Internal error: Oops: 5 [#1] ARM
> [    1.180000] Modules linked in:
> [    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
> [    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
> [    1.180000] PC is at memcmp+0x10/0x54
> [    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
> [    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
> [    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
> [    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
> [    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
> [    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
> [    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> [    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
> [    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)
> --------------------------------------------------------------------
> 
> This patch uses the local array(whose size is 4 bytes) to replace
> the referencing of the @chip->buffers->databuf.
> 
> Use the macro FINGERPRINT_LEN to avoid the hardcode.
> 
> Cc: stable@vger.kernel.org 

This is only for 3.12+, right, since we changed around the
nand_scan_tail() behavior? (commit
f720e7ce510bf79f029be45ce200ccfce5551280)

> Reported-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 7ac2280..d34cc54 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1343,6 +1343,7 @@ static int nand_boot_set_geometry(struct gpmi_nand_data *this)
>  }
>  
>  static const char  *fingerprint = "STMP";
> +#define FINGERPRINT_LEN		4
>  static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
>  {
>  	struct boot_rom_geometry *rom_geo = &this->rom_geometry;
> @@ -1352,7 +1353,7 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
>  	unsigned int search_area_size_in_strides;
>  	unsigned int stride;
>  	unsigned int page;
> -	uint8_t *buffer = chip->buffers->databuf;
> +	uint8_t buffer[FINGERPRINT_LEN + 1];

I agree with Fabio; we don't actually need the +1. (Of course, saving 1
byte is not that much of a problem. But making the code more
straightforward is.) If you "ack" I'll just drop the +1 myself.

>  	int saved_chip_number;
>  	int found_an_ncb_fingerprint = false;
>  

Thanks for looking into this quickly, BTW.

Brian

Re: [PATCH 2/2] mtd: gpmi: only scan two chips for imx6

[Brian Norris]

On Thu, Nov 07, 2013 at 05:46:37PM +0800, Huang Shijie wrote:
> We cannot scan two chips for imx23 and imx28:
>   imx23: the Ready-Busy1 line is not connected for some board.
>   imx28: we do not set the pinctrl for Ready-Busy1
> 
> So we only scan two chips for imx6.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Pushed to l2-mtd.git. Thanks!

This is a major bugfix (only for the queued-up code for 3.13), right? Do
we want to include a stronger bug description from Fabio?

Brian

Re: [PATCH 1/2] mtd: gpmi: fix the NULL pointer

[Fabio Estevam]

On Fri, Nov 8, 2013 at 3:51 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

> I agree with Fabio; we don't actually need the +1. (Of course, saving 1
> byte is not that much of a problem. But making the code more
> straightforward is.) If you "ack" I'll just drop the +1 myself.

You can also add my:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

Thanks,

Fabio Estevam

Re: [PATCH 1/2] mtd: gpmi: fix the NULL pointer

[Huang Shijie]

On Fri, Nov 08, 2013 at 09:51:19AM -0800, Brian Norris wrote:
> 
> I agree with Fabio; we don't actually need the +1. (Of course, saving 1
> byte is not that much of a problem. But making the code more
> straightforward is.) If you "ack" I'll just drop the +1 myself.
it is ok to me.

You can drop the +1 yourself.

thanks
Huang Shijie

Re: [PATCH 1/2] mtd: gpmi: fix the NULL pointer

[Huang Shijie]

于 2013年11月09日 01:51, Brian Norris 写道:
> Thanks for looking into this quickly, BTW.
this patch is not good, it uses the data in the stack to do the DMA 
transfer.

I will send a new patch to fix this NULL issue.

thanks
Huang Shijie

[PATCH V2] mtd: gpmi: fix the NULL pointer

[Huang Shijie]

The imx23 board will check the fingerprint, so it will call the
mx23_check_transcription_stamp. This function will use @chip->buffers->databuf
as its buffer which is allocated in the nand_scan_tail().

Unfortunately, the mx23_check_transcription_stamp is called before the
nand_scan_tail(). So we will meet a NULL pointer bug:

--------------------------------------------------------------------
[    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
[    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
[    1.170000] pgd = c0004000
[    1.170000] [000005d0] *pgd=00000000
[    1.180000] Internal error: Oops: 5 [#1] ARM
[    1.180000] Modules linked in:
[    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
[    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
[    1.180000] PC is at memcmp+0x10/0x54
[    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
[    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
[    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
[    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
[    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
[    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
[    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
[    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
[    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)
--------------------------------------------------------------------

This patch does two things:
 1.) Allocates the @chip->buffers itself, and set the NAND_OWN_BUFFERS
     for chip->options.
 2.) Also initialize the @chip->oob_poi which is used by the
     mx23_write_transcription_stamp(). We replace the @chip->ecc.write_page_raw
     with chip->write_buf, since the the @chip->ecc.write_page_raw is
     initialize in the nand_scan_tail() too. If we not do so, we will meet
     a NULL pointer too.

Cc: stable@vger.kernel.org
Reported-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Huang Shijie <b32955@freescale.com>
---
v1 --> v2: do not use a local array any more.
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 6e74917..a2531e7 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1465,7 +1465,7 @@ static int mx23_write_transcription_stamp(struct gpmi_nand_data *this)
 		/* Write the first page of the current stride. */
 		dev_dbg(dev, "Writing an NCB fingerprint in page 0x%x\n", page);
 		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
-		chip->ecc.write_page_raw(mtd, chip, buffer, 0);
+		chip->write_buf(mtd, buffer, mtd->writesize);
 		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
 
 		/* Wait for the write to finish. */
@@ -1609,6 +1609,12 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
 	struct bch_geometry *bch_geo = &this->bch_geometry;
 	int ret;
 
+	chip->buffers = kzalloc(sizeof(*chip->buffers), GFP_KERNEL);
+	if (!chip->buffers)
+		return -ENOMEM;
+	chip->options |= NAND_OWN_BUFFERS;
+	chip->oob_poi = chip->buffers->databuf + mtd->writesize;
+
 	/* Prepare for the BBT scan. */
 	ret = gpmi_pre_bbt_scan(this);
 	if (ret)
-- 
1.7.2.rc3

Re: [PATCH V2] mtd: gpmi: fix the NULL pointer

[Huang Shijie]

于 2013年11月11日 18:40, Huang Shijie 写道:
> The imx23 board will check the fingerprint, so it will call the
> mx23_check_transcription_stamp. This function will use @chip->buffers->databuf
> as its buffer which is allocated in the nand_scan_tail().
>
> Unfortunately, the mx23_check_transcription_stamp is called before the
> nand_scan_tail(). So we will meet a NULL pointer bug:
>
> --------------------------------------------------------------------
> [    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
> [    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
> [    1.170000] pgd = c0004000
> [    1.170000] [000005d0] *pgd=00000000
> [    1.180000] Internal error: Oops: 5 [#1] ARM
> [    1.180000] Modules linked in:
> [    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
> [    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
> [    1.180000] PC is at memcmp+0x10/0x54
> [    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
> [    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
> [    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
> [    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
> [    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
> [    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
> [    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> [    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
> [    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)
> --------------------------------------------------------------------
>
> This patch does two things:
>  1.) Allocates the @chip->buffers itself, and set the NAND_OWN_BUFFERS
>      for chip->options.
>  2.) Also initialize the @chip->oob_poi which is used by the
>      mx23_write_transcription_stamp(). We replace the @chip->ecc.write_page_raw
>      with chip->write_buf, since the the @chip->ecc.write_page_raw is
>      initialize in the nand_scan_tail() too. If we not do so, we will meet
>      a NULL pointer too.
>
> Cc: stable@vger.kernel.org
> Reported-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
> v1 --> v2: do not use a local array any more.
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 6e74917..a2531e7 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1465,7 +1465,7 @@ static int mx23_write_transcription_stamp(struct gpmi_nand_data *this)
>  		/* Write the first page of the current stride. */
>  		dev_dbg(dev, "Writing an NCB fingerprint in page 0x%x\n", page);
>  		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> -		chip->ecc.write_page_raw(mtd, chip, buffer, 0);
> +		chip->write_buf(mtd, buffer, mtd->writesize);
>  		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>  
>  		/* Wait for the write to finish. */
> @@ -1609,6 +1609,12 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
>  	struct bch_geometry *bch_geo = &this->bch_geometry;
>  	int ret;
>  
> +	chip->buffers = kzalloc(sizeof(*chip->buffers), GFP_KERNEL);
> +	if (!chip->buffers)
> +		return -ENOMEM;
> +	chip->options |= NAND_OWN_BUFFERS;
> +	chip->oob_poi = chip->buffers->databuf + mtd->writesize;
> +
>  	/* Prepare for the BBT scan. */
>  	ret = gpmi_pre_bbt_scan(this);
>  	if (ret)
Hi Fabio:
could you test this patch too?

thanks
Huang Shijie

Re: [PATCH V2] mtd: gpmi: fix the NULL pointer

[Fabio Estevam]

Hi Huang,

On Mon, Nov 11, 2013 at 9:07 AM, Huang Shijie <b32955@freescale.com> wrote:

> Hi Fabio:
> could you test this patch too?

Will test it later today and will let you know the result.

Regards,

Fabio Estevam

Re: [PATCH V2] mtd: gpmi: fix the NULL pointer

[Fabio Estevam]

On Mon, Nov 11, 2013 at 8:40 AM, Huang Shijie <b32955@freescale.com> wrote:
> The imx23 board will check the fingerprint, so it will call the
> mx23_check_transcription_stamp. This function will use @chip->buffers->databuf
> as its buffer which is allocated in the nand_scan_tail().
>
> Unfortunately, the mx23_check_transcription_stamp is called before the
> nand_scan_tail(). So we will meet a NULL pointer bug:
>
> --------------------------------------------------------------------
> [    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
> [    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
> [    1.170000] pgd = c0004000
> [    1.170000] [000005d0] *pgd=00000000
> [    1.180000] Internal error: Oops: 5 [#1] ARM
> [    1.180000] Modules linked in:
> [    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
> [    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
> [    1.180000] PC is at memcmp+0x10/0x54
> [    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
> [    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
> [    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
> [    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
> [    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
> [    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
> [    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> [    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
> [    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)
> --------------------------------------------------------------------
>
> This patch does two things:
>  1.) Allocates the @chip->buffers itself, and set the NAND_OWN_BUFFERS
>      for chip->options.
>  2.) Also initialize the @chip->oob_poi which is used by the
>      mx23_write_transcription_stamp(). We replace the @chip->ecc.write_page_raw
>      with chip->write_buf, since the the @chip->ecc.write_page_raw is
>      initialize in the nand_scan_tail() too. If we not do so, we will meet
>      a NULL pointer too.
>
> Cc: stable@vger.kernel.org
> Reported-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
> v1 --> v2: do not use a local array any more.
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 6e74917..a2531e7 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1465,7 +1465,7 @@ static int mx23_write_transcription_stamp(struct gpmi_nand_data *this)
>                 /* Write the first page of the current stride. */
>                 dev_dbg(dev, "Writing an NCB fingerprint in page 0x%x\n", page);
>                 chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> -               chip->ecc.write_page_raw(mtd, chip, buffer, 0);
> +               chip->write_buf(mtd, buffer, mtd->writesize);
>                 chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>
>                 /* Wait for the write to finish. */
> @@ -1609,6 +1609,12 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
>         struct bch_geometry *bch_geo = &this->bch_geometry;
>         int ret;
>
> +       chip->buffers = kzalloc(sizeof(*chip->buffers), GFP_KERNEL);
> +       if (!chip->buffers)
> +               return -ENOMEM;
> +       chip->options |= NAND_OWN_BUFFERS;
> +       chip->oob_poi = chip->buffers->databuf + mtd->writesize;

It seems that this patch is making more changes than the required to
fix the NULL dereference bug.

I think that a simpler fix would be just to allocate the buffer in
mx23_check_transcription_stamp() with kzalloc.

Will submit a proposal now.

Regards,

Fabio Estevam

Re: [PATCH V2] mtd: gpmi: fix the NULL pointer

[Brian Norris]

Hi Huang,

On Mon, Nov 11, 2013 at 06:40:19PM +0800, Huang Shijie wrote:
> The imx23 board will check the fingerprint, so it will call the
> mx23_check_transcription_stamp. This function will use @chip->buffers->databuf
> as its buffer which is allocated in the nand_scan_tail().
> 
> Unfortunately, the mx23_check_transcription_stamp is called before the
> nand_scan_tail(). So we will meet a NULL pointer bug:
> 
> --------------------------------------------------------------------
> [    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
> [    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
> [    1.170000] pgd = c0004000
> [    1.170000] [000005d0] *pgd=00000000
> [    1.180000] Internal error: Oops: 5 [#1] ARM
> [    1.180000] Modules linked in:
> [    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
> [    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
> [    1.180000] PC is at memcmp+0x10/0x54
> [    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
> [    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
> [    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
> [    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
> [    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
> [    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
> [    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> [    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
> [    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)
> --------------------------------------------------------------------
> 
> This patch does two things:
>  1.) Allocates the @chip->buffers itself, and set the NAND_OWN_BUFFERS
>      for chip->options.
>  2.) Also initialize the @chip->oob_poi which is used by the
>      mx23_write_transcription_stamp(). We replace the @chip->ecc.write_page_raw
>      with chip->write_buf, since the the @chip->ecc.write_page_raw is
>      initialize in the nand_scan_tail() too. If we not do so, we will meet
>      a NULL pointer too.

It seems like your GPMI init structure is yet again trying to circumvent
nand_base. I think there is a better solution that can help you avoid
working around things that *should* be initialized in nand_scan_tail.
I'll explain.

Your current init sequence consists of the following:

 |_ nand_scan_ident
 |_ nand_init_last
   |_ gpmi_init_last
     |_ gpmi_pre_bbt_scan
       |_ gpmi_set_geometry
       |_ nand_boot_init
         |_ mx23_boot_init
           |_ mx23_check_transcription_stamp (*)
           |_ mx23_write_transcription_stamp (*)
 |_ nand_scan_tail

The (*) portions are incorrect right now. But they also seem to be
related to bad block scanning, not the initial configuration of the
device. Furthermore, they depend on some functionality of
nand_scan_tail.

So I would recommend the following: set the NAND_SKIP_BBTSCAN, then
rearrange to the following call structure:

 |_ nand_scan_ident
 |_ chip->options |= NAND_SKIP_BBTSCAN;
 |_ nand_init_last
   |_ gpmi_init_last [1]
     |_ gpmi_set_geometry
 |_ nand_scan_tail
 |_ gpmi_pre_bbt_scan [2]
   |_ nand_boot_init
     |_ mx23_boot_init
       |_ mx23_check_transcription_stamp
       |_ mx23_write_transcription_stamp
 |_ chip->scan_bbt() [3]

Regarding [1] and [2]: we can split apart the code from gpmi_init_last()
so that have the stuff that is *only* necessary to do before [3] placed
under [2]. That way, nand_scan_tail() will initialize remaining pieces,
and all you have to do is configure and run your BBT scan. No more
dancing around uninitialized callback functions or buffers.

What do you think?

Brian

Re: [PATCH V2] mtd: gpmi: fix the NULL pointer

[Huang Shijie]

于 2013年11月11日 18:40, Huang Shijie 写道:
> +	chip->buffers = kzalloc(sizeof(*chip->buffers), GFP_KERNEL);
> +	if (!chip->buffers)
> +		return -ENOMEM;
> +	chip->options |= NAND_OWN_BUFFERS;
It has a memory leak for the NAND_OWN_BUFFERS. i should free this buffer
myself too.

thanks
Huang Shijie

Re: [PATCH V2] mtd: gpmi: fix the NULL pointer

[Brian Norris]

+ Fabio, in case he missed this (it was also in HTML, so it might not
have gotten through the filters)

On Tue, Nov 12, 2013 at 11:28:24AM +0800, Huang Shijie wrote:
> 于 2013年11月12日 11:18, Brian Norris 写道:
> 
>     So I would recommend the following: set the NAND_SKIP_BBTSCAN, then
>     rearrange to the following call structure:
> 
>      |_ nand_scan_ident
>      |_ chip->options |= NAND_SKIP_BBTSCAN;
>      |_ nand_init_last
>        |_ gpmi_init_last [1]
>          |_ gpmi_set_geometry
>      |_ nand_scan_tail
>      |_ gpmi_pre_bbt_scan [2]
>        |_ nand_boot_init
>          |_ mx23_boot_init
>            |_ mx23_check_transcription_stamp
>            |_ mx23_write_transcription_stamp
>      |_ chip->scan_bbt() [3]
> 
>     Regarding [1] and [2]: we can split apart the code from gpmi_init_last()
>     so that have the stuff that is *only* necessary to do before [3] placed
>     under [2]. That way, nand_scan_tail() will initialize remaining pieces,
>     and all you have to do is configure and run your BBT scan. No more
>     dancing around uninitialized callback functions or buffers.
> 
>     What do you think?
> 
> it's ok to me. I will code the patch right now, and send out it after i test
> it.

Sounds good.

Brian