Patchwork Kernel crashes when CONFIG_MTD_NAND_VERIFY_WRITE=y

login
register
mail settings
Submitter Fabio Estevam
Date Aug. 10, 2012, 1:36 a.m.
Message ID <CAOMZO5BCjV6iLUiQ74GskkOdgGpdqbo2ZB=7eu6VeY=mNwWf0Q@mail.gmail.com>
Download mbox | patch
Permalink /patch/176307/
State New
Headers show

Comments

Fabio Estevam - Aug. 10, 2012, 1:36 a.m.
Hi Marek,

On Thu, Aug 9, 2012 at 8:53 PM, Marek Vasut <marex@denx.de> wrote:

> This problem is there because the GPMI NAND code doesn't implement verify buffer
> function and defaults to nand_verify_buf() call in nand_base.c:

Yes, you are right.

> Now the chip->IO_ADDR_R is zero, making the kernel access bogus location, and
> therefore crash. So the correct solution is to properly implement the struct
> nand_chip *'s verify_buf function.

Right, the patch below prevents the kernel to happen:


Now we need to come up with a real  gpmi_verify_buf function ;-)

Regards,

Fabio Estevam
Marek Vasut - Aug. 10, 2012, 1:41 a.m.
Dear Fabio Estevam,

> Hi Marek,
> 
> On Thu, Aug 9, 2012 at 8:53 PM, Marek Vasut <marex@denx.de> wrote:
> > This problem is there because the GPMI NAND code doesn't implement verify
> > buffer
> 
> > function and defaults to nand_verify_buf() call in nand_base.c:
> Yes, you are right.
> 
> > Now the chip->IO_ADDR_R is zero, making the kernel access bogus location,
> > and therefore crash. So the correct solution is to properly implement
> > the struct nand_chip *'s verify_buf function.
> 
> Right, the patch below prevents the kernel to happen:
> 
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -857,6 +857,15 @@ static uint8_t gpmi_read_byte(struct mtd_info *mtd)
>         return buf[0];
>  }
> 
> +/* Used by the upper layer to verify the data in NAND Flash
> + * with the data in the buf. */
> +static int gpmi_verify_buf(struct mtd_info *mtd,
> +                               const u_char *buf, int len)
> +{
> +       /* TODO: implement verify_buf mechanism */
> +       return 0;
> +}


NAK! This is only a workaround, proper implementation is needed. If it's not 
implemented now, I'm pretty sure such workaround will be there forever.

>  /*
>   * Handles block mark swapping.
>   * It can be called in swapping the block mark, or swapping it back,
> @@ -1568,6 +1577,7 @@ static int __devinit gpmi_nfc_init(struct
> gpmi_nand_data *this)
>         chip->ecc.size          = 1;
>         chip->ecc.strength      = 8;
>         chip->ecc.layout        = &gpmi_hw_ecclayout;
> +       chip->verify_buf        = gpmi_verify_buf;
>         if (of_get_nand_on_flash_bbt(this->dev->of_node))
>                 chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> 
> Now we need to come up with a real  gpmi_verify_buf function ;-)
> 
> Regards,
> 
> Fabio Estevam

Best regards,
Marek Vasut
Marek Vasut - Aug. 10, 2012, 1:42 a.m.
Dear Marek Vasut,

> Dear Fabio Estevam,
> 
> > Hi Marek,
> > 
> > On Thu, Aug 9, 2012 at 8:53 PM, Marek Vasut <marex@denx.de> wrote:
> > > This problem is there because the GPMI NAND code doesn't implement
> > > verify buffer
> > 
> > > function and defaults to nand_verify_buf() call in nand_base.c:
> > Yes, you are right.
> > 
> > > Now the chip->IO_ADDR_R is zero, making the kernel access bogus
> > > location, and therefore crash. So the correct solution is to properly
> > > implement the struct nand_chip *'s verify_buf function.
> > 
> > Right, the patch below prevents the kernel to happen:
> > 
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -857,6 +857,15 @@ static uint8_t gpmi_read_byte(struct mtd_info *mtd)
> > 
> >         return buf[0];
> >  
> >  }
> > 
> > +/* Used by the upper layer to verify the data in NAND Flash
> > + * with the data in the buf. */
> > +static int gpmi_verify_buf(struct mtd_info *mtd,
> > +                               const u_char *buf, int len)
> > +{
> > +       /* TODO: implement verify_buf mechanism */
> > +       return 0;
> > +}
> 
> NAK! This is only a workaround, proper implementation is needed. If it's
> not implemented now, I'm pretty sure such workaround will be there
> forever.
[...]

btw if you want a workaround, make Kconfig entry so it'd disallow VERIFY to be 
selected for GPMI_NAND, but that's also a wrong way to go.

Best regards,
Marek Vasut
Fabio Estevam - Aug. 10, 2012, 1:49 a.m.
On Thu, Aug 9, 2012 at 10:41 PM, Marek Vasut <marex@denx.de> wrote:

> NAK! This is only a workaround, proper implementation is needed. If it's not
> implemented now, I'm pretty sure such workaround will be there forever.

I know, please see below.

>
>>  /*
>>   * Handles block mark swapping.
>>   * It can be called in swapping the block mark, or swapping it back,
>> @@ -1568,6 +1577,7 @@ static int __devinit gpmi_nfc_init(struct
>> gpmi_nand_data *this)
>>         chip->ecc.size          = 1;
>>         chip->ecc.strength      = 8;
>>         chip->ecc.layout        = &gpmi_hw_ecclayout;
>> +       chip->verify_buf        = gpmi_verify_buf;
>>         if (of_get_nand_on_flash_bbt(this->dev->of_node))
>>                 chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>>
>> Now we need to come up with a real  gpmi_verify_buf function ;-)

As I mentioned, I understand that a proper function needs to be created.

Regards,

Fabio Estevam
Huang Shijie - Aug. 10, 2012, 2:08 a.m.
于 2012年08月10日 09:49, Fabio Estevam 写道:
> On Thu, Aug 9, 2012 at 10:41 PM, Marek Vasut<marex@denx.de>  wrote:
>
>> NAK! This is only a workaround, proper implementation is needed. If it's not
>> implemented now, I'm pretty sure such workaround will be there forever.
> I know, please see below.
>
>>>   /*
>>>    * Handles block mark swapping.
>>>    * It can be called in swapping the block mark, or swapping it back,
>>> @@ -1568,6 +1577,7 @@ static int __devinit gpmi_nfc_init(struct
>>> gpmi_nand_data *this)
>>>          chip->ecc.size          = 1;
>>>          chip->ecc.strength      = 8;
>>>          chip->ecc.layout        =&gpmi_hw_ecclayout;
>>> +       chip->verify_buf        = gpmi_verify_buf;
>>>          if (of_get_nand_on_flash_bbt(this->dev->of_node))
>>>                  chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>>>
>>> Now we need to come up with a real  gpmi_verify_buf function ;-)
I does have a real gpmi_verify_buf function in our BSP code.

I will send it out as soon as possible.

thanks
Huang Shijie




> As I mentioned, I understand that a proper function needs to be created.
>
> Regards,
>
> Fabio Estevam
>
Marek Vasut - Aug. 10, 2012, 2:11 a.m.
Dear Huang Shijie,

> 于 2012年08月10日 09:49, Fabio Estevam 写道:
> > On Thu, Aug 9, 2012 at 10:41 PM, Marek Vasut<marex@denx.de>  wrote:
> >> NAK! This is only a workaround, proper implementation is needed. If it's
> >> not implemented now, I'm pretty sure such workaround will be there
> >> forever.
> > 
> > I know, please see below.
> > 
> >>>   /*
> >>>   
> >>>    * Handles block mark swapping.
> >>>    * It can be called in swapping the block mark, or swapping it back,
> >>> 
> >>> @@ -1568,6 +1577,7 @@ static int __devinit gpmi_nfc_init(struct
> >>> gpmi_nand_data *this)
> >>> 
> >>>          chip->ecc.size          = 1;
> >>>          chip->ecc.strength      = 8;
> >>>          chip->ecc.layout        =&gpmi_hw_ecclayout;
> >>> 
> >>> +       chip->verify_buf        = gpmi_verify_buf;
> >>> 
> >>>          if (of_get_nand_on_flash_bbt(this->dev->of_node))
> >>>          
> >>>                  chip->bbt_options |= NAND_BBT_USE_FLASH |
> >>>                  NAND_BBT_NO_OOB;
> >>> 
> >>> Now we need to come up with a real  gpmi_verify_buf function ;-)
> 
> I does have a real gpmi_verify_buf function in our BSP code.
> 
> I will send it out as soon as possible.

I believe you should roll it out ASAP and also CC stable!

> thanks
> Huang Shijie
> 
> > As I mentioned, I understand that a proper function needs to be created.
> > 
> > Regards,
> > 
> > Fabio Estevam

Best regards,
Marek Vasut
Fabio Estevam - Aug. 10, 2012, 2:29 a.m.
On Thu, Aug 9, 2012 at 11:08 PM, Huang Shijie <b32955@freescale.com> wrote:

> I does have a real gpmi_verify_buf function in our BSP code.

Well, the same crash happens in FSL BSP.

> I will send it out as soon as possible.

Ok, great.

Thanks,

Fabio Estevam

Patch

--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -857,6 +857,15 @@  static uint8_t gpmi_read_byte(struct mtd_info *mtd)
        return buf[0];
 }

+/* Used by the upper layer to verify the data in NAND Flash
+ * with the data in the buf. */
+static int gpmi_verify_buf(struct mtd_info *mtd,
+                               const u_char *buf, int len)
+{
+       /* TODO: implement verify_buf mechanism */
+       return 0;
+}
+
 /*
  * Handles block mark swapping.
  * It can be called in swapping the block mark, or swapping it back,
@@ -1568,6 +1577,7 @@  static int __devinit gpmi_nfc_init(struct
gpmi_nand_data *this)
        chip->ecc.size          = 1;
        chip->ecc.strength      = 8;
        chip->ecc.layout        = &gpmi_hw_ecclayout;
+       chip->verify_buf        = gpmi_verify_buf;
        if (of_get_nand_on_flash_bbt(this->dev->of_node))
                chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;