mtd: fix the wrong check condition

PATCH New Huang Shijie 4 participants 9 messages Feb 15, 2012 – Feb 18, 2012

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Huang Shijie

author

2012-02-15 10:33 UTC #0 lore raw

PATCH 1/1 mtd: fix the wrong check condition

If we use || check condition, many NAND chips which are not ONFI nands have to do the ONFI detection.

Use && here to detect the ONFI NAND when we can not find any type in the nand_flash_ids.

Signed-off-by Huang Shijie <[email protected]>

---
 drivers/mtd/nand/nand_base.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/mtd/nand/nand_base.c +1 -1

index 8a393f9..90020a5 100644
@@ -2966,7 +2966,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 			break;
 
 	chip->onfi_version = 0;
-	if (!type->name || !type->pagesize) {
+	if (!type->name && !type->pagesize) {
 		/* Check is chip is ONFI compliant */
 		ret = nand_flash_detect_onfi(mtd, chip, &busw);
 		if (ret)

Signature · 1 line

1.7.3.2

Brian Norris

2012-02-16 02:59 UTC #1 lore raw

PATCH 1/1 mtd: fix the wrong check condition

(Add Florian and Matthieu)

Quoted 2 lines · Huang Shijie

If we use || check condition, many NAND chips which are not ONFI nands have to do the ONFI detection.

Running the ONFI detection on non-ONFI NAND should not, ideally, be a problem. They should fail one or both tests included in the routine: the 'O N F I' string check or the CRC calculation.

Quoted 2 lines

Use && here to detect the ONFI NAND when we can not find any type in the nand_flash_ids.

There are many chips whose ID might be in the NAND table but for which it is preferable (or even required) to check by ONFI for one reason or another. For instance, some ONFI chips might use odd-sized OOB that isn't in the ID decoding algorithm.

The current || check is, I think, designed to weed out old small-page NAND only, which define both 'name' and 'pagesize' in the table.

So in short: the current code works as intended.

Brian

Huang Shijie

author

2012-02-16 03:12 UTC #2 lore raw

Hi,

Quoted 7 lines

(Add Florian and Matthieu)
Quoted 2 lines · Huang Shijie
If we use || check condition, many NAND chips which are not ONFI nands have to do the ONFI detection.
Running the ONFI detection on non-ONFI NAND should not, ideally, be a problem. They should fail one or both tests included in the routine: the 'O N F I' string check or the CRC calculation.

NO.

I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448).
It is not an ONFI nand. See the datasheet in the attachment.
But it accidentally can pass the ONFI detection, and get the result : page size 4192, oob:96. This is a wrong result.

Quoted 6 lines

Quoted 3 lines
Use && here to detect the ONFI NAND when we can not find any type in the nand_flash_ids.
There are many chips whose ID might be in the NAND table but for which it is preferable (or even required) to check by ONFI for one reason or another. For instance, some ONFI chips might use odd-sized OOB that isn't in the ID decoding algorithm.

This nand is 32Gb, but we can not parse it out from the id.
I ever want to add a new database which use the all the 8/6 bytes id as key. It seems it's time to change it now.

Huang Shijie

Quoted 6 lines

The current || check is, I think, designed to weed out old small-page NAND only, which define both 'name' and 'pagesize' in the table.
So in short: the current code works as intended.
Brian

Florian Fainelli

2012-02-16 11:04 UTC #3 lore raw

PATCH 1/1 mtd: fix the wrong check condition

Hi,

Quoted 11 lines · Huang Shijie

Hi,
Quoted 9 lines
(Add Florian and Matthieu)
Quoted 2 lines · Huang Shijie
If we use || check condition, many NAND chips which are not ONFI nands have to do the ONFI detection.
Running the ONFI detection on non-ONFI NAND should not, ideally, be a problem. They should fail one or both tests included in the routine:
the 'O N F I' string check or the CRC calculation.
NO.

Can you please post the dump of the ONFI page as read by your controller? Is the CRC check passing? The ONFI crc function is made so that a page full of zeroes or 0xff won't generate a respectively 0 or ff checksum, so we should catch this during the CRC check.

Quoted 5 lines

I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). It is not an ONFI nand. See the datasheet in the attachment. But it accidentally can pass the ONFI detection, and get the result :
page size 4192, oob:96. This is a wrong result.

I have already seen a Hynix chip answering to the READID command too, and this was highly confusing our bootloader, however, I suppose that we should be able to circumvent this issue anyway.

Quoted 13 lines

Quoted 5 lines
Quoted 8 lines
Quoted 2 lines
Use && here to detect the ONFI NAND when we can not find any type in the nand_flash_ids.
There are many chips whose ID might be in the NAND table but for which it is preferable (or even required) to check by ONFI for one reason or another. For instance, some ONFI chips might use odd-sized OOB that isn't in the ID decoding algorithm.

Quoted 4 lines

This nand is 32Gb, but we can not parse it out from the id. I ever want to add a new database which use the all the 8/6 bytes id as key.
It seems it's time to change it now.

You have said that already, but we have yet to see patches for this, I guess if you can post your database patch that will be easier to comment on. --
Florian

Huang Shijie

2012-02-17 03:07 UTC #4 lore raw

PATCH 1/1 mtd: fix the wrong check condition

hi,

Quoted 18 lines · Florian Fainelli

Hi,
Quoted 15 lines · Huang Shijie
Hi,
Quoted 12 lines
(Add Florian and Matthieu)
Quoted 3 lines · Huang Shijie
If we use || check condition, many NAND chips which are not ONFI nands have to do the ONFI detection.
Running the ONFI detection on non-ONFI NAND should not, ideally, be a problem. They should fail one or both tests included in the routine:
the 'O N F I' string check or the CRC calculation.

Quoted 8 lines

Quoted 1 line
NO.
Can you please post the dump of the ONFI page as read by your controller? Is the CRC check passing? The ONFI crc function is made so that a page full of zeroes or 0xff won't generate a respectively 0 or ff checksum, so we should catch this during the CRC check.

Sorry. I did not check the log carefully.

the Hynix nand does _not_ pass the CRC check, but it passed the "ONFI" string check.

The log "ONFI flash detected" made me misunderstood.

BR
Huang Shijie

Quoted 40 lines

Quoted 5 lines
I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). It is not an ONFI nand. See the datasheet in the attachment. But it accidentally can pass the ONFI detection, and get the result :
page size 4192, oob:96. This is a wrong result.
I have already seen a Hynix chip answering to the READID command too, and this was highly confusing our bootloader, however, I suppose that we should be able to circumvent this issue anyway.
Quoted 14 lines
Quoted 5 lines
Quoted 9 lines
Quoted 2 lines
Use && here to detect the ONFI NAND when we can not find any type in the nand_flash_ids.
There are many chips whose ID might be in the NAND table but for which it is preferable (or even required) to check by ONFI for one reason or another. For instance, some ONFI chips might use odd-sized OOB that isn't in the ID decoding algorithm.
Quoted 4 lines
This nand is 32Gb, but we can not parse it out from the id. I ever want to add a new database which use the all the 8/6 bytes id as key.
It seems it's time to change it now.
You have said that already, but we have yet to see patches for this, I guess if you can post your database patch that will be easier to comment on. --
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Florian Fainelli

2012-02-17 08:50 UTC #5 lore raw

PATCH 1/1 mtd: fix the wrong check condition

Le 02/17/12 04:07, Huang Shijie a ?crit :

Quoted 23 lines

hi,
Quoted 13 lines · Florian Fainelli
Hi,
Quoted 10 lines · Huang Shijie
Hi,
Quoted 9 lines
(Add Florian and Matthieu)
Quoted 2 lines · Huang Shijie
If we use || check condition, many NAND chips which are not ONFI nands have to do the ONFI detection.
Running the ONFI detection on non-ONFI NAND should not, ideally, be a problem. They should fail one or both tests included in the routine:
the 'O N F I' string check or the CRC calculation.
Quoted 7 lines
Quoted 1 line
NO.
Can you please post the dump of the ONFI page as read by your controller? Is the CRC check passing? The ONFI crc function is made so that a page full of zeroes or 0xff won't generate a respectively 0 or ff checksum, so we should catch this during the CRC check.

Quoted 4 lines

Sorry. I did not check the log carefully.
the Hynix nand does _not_ pass the CRC check, but it passed the "ONFI" string check.

This is exactly what I saw too with a different Hynix chip, it replied ONFI, but the CRC check failed.

Quoted 2 lines

The log "ONFI flash detected" made me misunderstood.

Yes I agree, the log is there too soon, thanks for your patch.

Thanks.

Quoted 43 lines

BR
Huang Shijie
Quoted 30 lines
Quoted 4 lines
I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). It is not an ONFI nand. See the datasheet in the attachment. But it accidentally can pass the ONFI detection, and get the result :
page size 4192, oob:96. This is a wrong result.
I have already seen a Hynix chip answering to the READID command too, and this was highly confusing our bootloader, however, I suppose that we should be able to circumvent this issue anyway.
Quoted 11 lines
Quoted 11 lines
Quoted 5 lines
Use && here to detect the ONFI NAND when we can not find any type in the nand_flash_ids.
There are many chips whose ID might be in the NAND table but for which it is preferable (or even required) to check by ONFI for one reason or another. For instance, some ONFI chips might use odd-sized OOB that isn't in the ID decoding algorithm.
Quoted 4 lines
This nand is 32Gb, but we can not parse it out from the id. I ever want to add a new database which use the all the 8/6 bytes id as key.
It seems it's time to change it now.
You have said that already, but we have yet to see patches for this, I guess if you can post your database patch that will be easier to comment on. --
Florian
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Brian Norris

2012-02-18 05:43 UTC #6 lore raw

PATCH 1/1 mtd: fix the wrong check condition

Quoted 8 lines · Florian Fainelli

Le 02/17/12 04:07, Huang Shijie a ?crit :
Quoted 4 lines
Sorry. I did not check the log carefully.
the Hynix nand does _not_ pass the CRC check, but it passed the "ONFI" string check.
This is exactly what I saw too with a different Hynix chip, it replied ONFI, but the CRC check failed.

So your root detection issue is actually that this Hynix chip is neither ONFI-compliant nor does it have a good ID decoding scheme, hence the need for the ID table patch (separate; I'll review it soon).

Quoted 4 lines

Quoted 2 lines
The log "ONFI flash detected" made me misunderstood.
Yes I agree, the log is there too soon, thanks for your patch.

I agree as well. I'll ACK the other patch.

Thanks,
Brian

Huang Shijie

author

2012-02-16 03:15 UTC #7 lore raw

PATCH 1/1 mtd: fix the wrong check condition

In reply to Brian Norris

Hi,

Quoted 9 lines

(Add Florian and Matthieu)
Quoted 2 lines · Huang Shijie
If we use || check condition, many NAND chips which are not ONFI nands have to do the ONFI detection.
Running the ONFI detection on non-ONFI NAND should not, ideally, be a problem. They should fail one or both tests included in the routine:
the 'O N F I' string check or the CRC calculation.

NO.

I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). It is not an ONFI nand.

But it accidentally can pass the ONFI detection, and get the result :
page size 4192, oob:96. This is a wrong result.

Quoted 9 lines

Quoted 3 lines
Use && here to detect the ONFI NAND when we can not find any type in the nand_flash_ids.
There are many chips whose ID might be in the NAND table but for which it is preferable (or even required) to check by ONFI for one reason or another. For instance, some ONFI chips might use odd-sized OOB that isn't in the ID decoding algorithm.

This nand is 32Gb, but we can not parse it out from the id.
I ever want to add a new database which use the all the 8/6 bytes id as key. It seems it's time to change it now.

Huang Shijie

Quoted 8 lines

The current || check is, I think, designed to weed out old small-page NAND only, which define both 'name' and 'pagesize' in the table.
So in short: the current code works as intended.
Brian

Brian Norris

2012-02-16 08:32 UTC #8 lore raw

PATCH 1/1 mtd: fix the wrong check condition

(Add Angus)

Hi,

Quoted 11 lines · Huang Shijie

Quoted 9 lines · Huang Shijie
Quoted 3 lines
If we use || check condition, many NAND chips which are not ONFI nands have to do the ONFI detection.
Running the ONFI detection on non-ONFI NAND should not, ideally, be a problem. They should fail one or both tests included in the routine:
the 'O N F I' string check or the CRC calculation.
NO.

Whoa, no need to shout...

Quoted 5 lines

I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). It is not an ONFI nand.
But it accidentally can pass the ONFI detection, and get the result : page size 4192, oob:96. This is a wrong result.

Yes, this is a wrong result, and that's pretty freaky. Why have a standard if some chips are going to totally demolish it? Uggh... Anyway, you totally left this information out of your patch. If you had, then I would have told you that this patch does not solve *this* problem either; where do you expect the "OOB:448" to come from?

(BTW, your attachment didn't come through properly. I'll look for the datasheet elsewhere, I guess...)

Quoted 4 lines

Quoted 4 lines
Quoted 2 lines
Use && here to detect the ONFI NAND when we can not find any type in the nand_flash_ids.

Quoted 8 lines

Quoted 5 lines
There are many chips whose ID might be in the NAND table but for which it is preferable (or even required) to check by ONFI for one reason or another. For instance, some ONFI chips might use odd-sized OOB that isn't in the ID decoding algorithm.
This nand is 32Gb, but we can not parse it out from the id.
I ever want to add a new database which use the all the 8/6 bytes id as key. It seems it's time to change it now.

Yes, we need some kind of new database. Angus Clark had mentioned wanting to update the table/detection mechanisms. I can help out too. But it needs to be intelligent, since we *can't* log the 8-byte ID for *every* chip. I have some ideas, though, and will share a few. Feel free to do the same.

For starters, I'm thinking expand the table(s) to 8-byte ID, with some sort of mask (or length?) parameter to record how many of the 8 bytes are relevant. However, we would need a table for pre-ONFI checks (to catch exceptions like this) and one post-ONFI (for most typical non-ONFI chips):

(1) ID table for ONFI exceptions
(2) ONFI routines
(3) ID table for standard chips

We can extend (3) to use some of the current ID decoding mechanisms (for chips who only have 2-byte IDs in their table entry).

Angus: do you have work-in-progress that you want to share for fixing up NAND detection? Or shall we work on something new?

Brian