nand_flash_detect_onfi error

nand_flash_detect_onfi error

[Renaud Barbier]

In the file drivers/mtd/nand/nand_base.c, in function
nand_flash_detect_onfi  reading the ONFI data can be run up to 3 times
when there is a CRC error detected in the onfi data:

The function call chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); is located
outside the first for loop:

chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
for (i = 0; i < 3; i++) {
	for (j = 0; j < sizeof(*p); j++)
		((uint8_t *)p)[j] = chip->read_byte(mtd);
	if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
			le16_to_cpu(p->crc)) {
		break;
	}
}

This results in a read beyond buffer error if the data have to be read
more than once.

This error can also be found in U-boot and barebox.

Re: nand_flash_detect_onfi error

[Ezequiel Garcia]

On 3 November 2015 at 13:27, Renaud Barbier <renaud.barbier@ge.com> wrote:
> In the file drivers/mtd/nand/nand_base.c, in function
> nand_flash_detect_onfi  reading the ONFI data can be run up to 3 times
> when there is a CRC error detected in the onfi data:
>
> The function call chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); is located
> outside the first for loop:
>
> chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> for (i = 0; i < 3; i++) {
>         for (j = 0; j < sizeof(*p); j++)
>                 ((uint8_t *)p)[j] = chip->read_byte(mtd);
>         if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
>                         le16_to_cpu(p->crc)) {
>                 break;
>         }
> }
>
> This results in a read beyond buffer error if the data have to be read
> more than once.
>
> This error can also be found in U-boot and barebox.
>

Hi Renaud,

NAND_CMD_PARAM is supposed to read one parameter page,
plus the two redundant parameter pages as well. That's why the NAND core
code reading sizeof(parameter page) x 3 bytes safely.

If you take a look at the ONFI 3.0 spec you'll see the parameter page
is specified to be 767 bytes long, i.e. three redundant parameter pages
can be read.

If you are seeing this kind of bug with a NAND controller, then you need
to extend your CMD_PARAM read to fetch all the copies of the parameter page.

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: nand_flash_detect_onfi error

[Boris Brezillon]

Ezequiel, Renaud,

On Tue, 3 Nov 2015 13:56:16 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> On 3 November 2015 at 13:27, Renaud Barbier <renaud.barbier@ge.com> wrote:
> > In the file drivers/mtd/nand/nand_base.c, in function
> > nand_flash_detect_onfi  reading the ONFI data can be run up to 3 times
> > when there is a CRC error detected in the onfi data:

Do you mean parameter pages (including redundant ones) should be read
several times in case of failures or are you talking about redundant
parameter pages (there should be at least 2 of them and there can be
more)?
I didn't find any mention to the former statement, so if this is really
what you meant could you point the section where it's described?

If you're talking about the latter, then the current implementation is
correct, because redundant pages are contiguous to the first parameter
page.


> >
> > The function call chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); is located
> > outside the first for loop:
> >
> > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> > for (i = 0; i < 3; i++) {
> >         for (j = 0; j < sizeof(*p); j++)
> >                 ((uint8_t *)p)[j] = chip->read_byte(mtd);
> >         if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> >                         le16_to_cpu(p->crc)) {
> >                 break;
> >         }
> > }
> >
> > This results in a read beyond buffer error if the data have to be read
> > more than once.

I don't think so (but I might be wrong). As explained, the redundant
parameter pages are concatenated to the main one, so it's perfectly
valid to read all of them with a single NAND_CMD_PARAM sequence.

BTW, I wonder why ->read_byte() is used instead of ->read_buf() here.

> >
> > This error can also be found in U-boot and barebox.
> >
> 
> Hi Renaud,
> 
> NAND_CMD_PARAM is supposed to read one parameter page,
> plus the two redundant parameter pages as well. That's why the NAND core
> code reading sizeof(parameter page) x 3 bytes safely.
> 
> If you take a look at the ONFI 3.0 spec you'll see the parameter page
> is specified to be 767 bytes long, i.e. three redundant parameter pages
> can be read.
> 
> If you are seeing this kind of bug with a NAND controller, then you need
> to extend your CMD_PARAM read to fetch all the copies of the parameter page.
> 

Hm, sorry but I don't like this idea. ->cmdfunc() is not supposed to
retrieve any data before ->read_xxx() is called. I know some
controllers retrieve data ahead of time and then provide the previously
stored data when ->read_buf() is called, but that's not a good practice
to assume it will work this way on all controllers (actually I keep
thinking the sane implementations are those waiting for the
->read_buf() call before starting retrieving the data from the NAND
chip).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: nand_flash_detect_onfi error

[Brian Norris]

On Tue, Nov 03, 2015 at 09:25:46PM +0100, Boris Brezillon wrote:
> On Tue, 3 Nov 2015 13:56:16 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> > On 3 November 2015 at 13:27, Renaud Barbier <renaud.barbier@ge.com> wrote:
> > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> > > for (i = 0; i < 3; i++) {
> > >         for (j = 0; j < sizeof(*p); j++)
> > >                 ((uint8_t *)p)[j] = chip->read_byte(mtd);
> > >         if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > >                         le16_to_cpu(p->crc)) {
> > >                 break;
> > >         }
> > > }
> > >
...
> BTW, I wonder why ->read_byte() is used instead of ->read_buf() here.

commit bd9c6e99b58255b9de1982711ac9487c9a2f18be
Author: Brian Norris <computersforpeace@gmail.com>
Date:   Fri Nov 29 22:04:28 2013 -0800

    mtd: nand: don't use read_buf for 8-bit ONFI transfers

Brian

Re: nand_flash_detect_onfi error

[Boris Brezillon]

On Tue, 3 Nov 2015 12:45:11 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> On Tue, Nov 03, 2015 at 09:25:46PM +0100, Boris Brezillon wrote:
> > On Tue, 3 Nov 2015 13:56:16 -0300
> > Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> > > On 3 November 2015 at 13:27, Renaud Barbier <renaud.barbier@ge.com> wrote:
> > > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> > > > for (i = 0; i < 3; i++) {
> > > >         for (j = 0; j < sizeof(*p); j++)
> > > >                 ((uint8_t *)p)[j] = chip->read_byte(mtd);
> > > >         if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > > >                         le16_to_cpu(p->crc)) {
> > > >                 break;
> > > >         }
> > > > }
> > > >
> ...
> > BTW, I wonder why ->read_byte() is used instead of ->read_buf() here.
> 
> commit bd9c6e99b58255b9de1982711ac9487c9a2f18be
> Author: Brian Norris <computersforpeace@gmail.com>
> Date:   Fri Nov 29 22:04:28 2013 -0800
> 
>     mtd: nand: don't use read_buf for 8-bit ONFI transfers

Makes sense. Thanks for pointing this out.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: nand_flash_detect_onfi error

[Brian Norris]

On Tue, Nov 3, 2015 at 12:54 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 3 Nov 2015 12:45:11 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
>
>> On Tue, Nov 03, 2015 at 09:25:46PM +0100, Boris Brezillon wrote:
>> > On Tue, 3 Nov 2015 13:56:16 -0300
>> > Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>> > > On 3 November 2015 at 13:27, Renaud Barbier <renaud.barbier@ge.com> wrote:
>> > > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
>> > > > for (i = 0; i < 3; i++) {
>> > > >         for (j = 0; j < sizeof(*p); j++)
>> > > >                 ((uint8_t *)p)[j] = chip->read_byte(mtd);
>> > > >         if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
>> > > >                         le16_to_cpu(p->crc)) {
>> > > >                 break;
>> > > >         }
>> > > > }
>> > > >
>> ...
>> > BTW, I wonder why ->read_byte() is used instead of ->read_buf() here.
>>
>> commit bd9c6e99b58255b9de1982711ac9487c9a2f18be
>> Author: Brian Norris <computersforpeace@gmail.com>
>> Date:   Fri Nov 29 22:04:28 2013 -0800
>>
>>     mtd: nand: don't use read_buf for 8-bit ONFI transfers
>
> Makes sense. Thanks for pointing this out.

Perhaps it was worth a code comment. It can be confusing.

Re: nand_flash_detect_onfi error

[Ezequiel Garcia]

On 3 November 2015 at 17:25, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
[..]
>
> Hm, sorry but I don't like this idea. ->cmdfunc() is not supposed to
> retrieve any data before ->read_xxx() is called. I know some
> controllers retrieve data ahead of time and then provide the previously
> stored data when ->read_buf() is called, but that's not a good practice
> to assume it will work this way on all controllers (actually I keep
> thinking the sane implementations are those waiting for the
> ->read_buf() call before starting retrieving the data from the NAND
> chip).
>

Right. We've discussed this in the past, and although I don't recall
the details,
I recall you were right.

In any case, I just wanted to mention that if Renaud is getting
a read beyond the buffer, then probably the driver needs fixing.

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: nand_flash_detect_onfi error

[Boris Brezillon]

On Tue, 3 Nov 2015 17:59:01 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> On 3 November 2015 at 17:25, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> [..]
> >
> > Hm, sorry but I don't like this idea. ->cmdfunc() is not supposed to
> > retrieve any data before ->read_xxx() is called. I know some
> > controllers retrieve data ahead of time and then provide the previously
> > stored data when ->read_buf() is called, but that's not a good practice
> > to assume it will work this way on all controllers (actually I keep
> > thinking the sane implementations are those waiting for the
> > ->read_buf() call before starting retrieving the data from the NAND
> > chip).
> >
> 
> Right. We've discussed this in the past, and although I don't recall
> the details,
> I recall you were right.
> 
> In any case, I just wanted to mention that if Renaud is getting
> a read beyond the buffer, then probably the driver needs fixing.
> 

My bad, I thought the "read beyond the buffer" thing mentioned by
Renaud was something generic. Now I get your point, and I agree: it
should be fixed in the NAND controller driver, either by increasing the
read buffer or by reworking the implementation to delay data retrieval
until ->read_buf() is called.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: nand_flash_detect_onfi error

[Renaud Barbier]

On 03/11/2015 21:18, Boris Brezillon wrote:
> On Tue, 3 Nov 2015 17:59:01 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> 
>> On 3 November 2015 at 17:25, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> [..]
>>>
>>> Hm, sorry but I don't like this idea. ->cmdfunc() is not supposed to
>>> retrieve any data before ->read_xxx() is called. I know some
>>> controllers retrieve data ahead of time and then provide the previously
>>> stored data when ->read_buf() is called, but that's not a good practice
>>> to assume it will work this way on all controllers (actually I keep
>>> thinking the sane implementations are those waiting for the
>>> ->read_buf() call before starting retrieving the data from the NAND
>>> chip).
>>>
>>
>> Right. We've discussed this in the past, and although I don't recall
>> the details,
>> I recall you were right.
>>
>> In any case, I just wanted to mention that if Renaud is getting
>> a read beyond the buffer, then probably the driver needs fixing.
>>
> 
> My bad, I thought the "read beyond the buffer" thing mentioned by
> Renaud was something generic. Now I get your point, and I agree: it
> should be fixed in the NAND controller driver, either by increasing the
> read buffer or by reworking the implementation to delay data retrieval
> until ->read_buf() is called.
> 

My CPU is a Freescale P1014 supported by the driver
drivers/mtd/nand/fsl_ifc_nand.c. Based on your information it seems the
easiest path is to set the "read_bytes" count to 3 * 256 and program the
controller to use this byte count when the CMD_PARAM command is used.

Cheers.

Re: nand_flash_detect_onfi error

[Brian Norris]

On Wed, Nov 4, 2015 at 8:24 AM, Renaud Barbier <renaud.barbier@ge.com> wrote:
> My CPU is a Freescale P1014 supported by the driver
> drivers/mtd/nand/fsl_ifc_nand.c. Based on your information it seems the
> easiest path is to set the "read_bytes" count to 3 * 256 and program the
> controller to use this byte count when the CMD_PARAM command is used.

That's still a bit ugly. It'd be nice not to have to assume the read length...

Also, have you looked at NAND_CMD_RNDOUT support? That's required if
you need to read out the extended parameter page. See
nand_flash_detect_ext_param_page().

Brian