Skip to contentSkip to conversation

Bad assumption about ID field definition for Samsung NAND?

DISCUSSION Tilman Sauerbeck 5 participants 14 messages Aug 18, 2010 – Aug 20, 2010

Subsystems (2)
Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

Tilman Sauerbeck

author
#0 lore raw

Hi,
on my GuruPlug, I was seeing lots of bad block errors for the NAND chip when booting a 2.6.35 kernel. With earlier kernels (2.6.33, 2.6.34), no bad blocks were reported. See below for the exact error messages. Since _all_ of the first 128 blocks are claimed to be bad, and the NAND chips appears to be working just fine, I suspect the kernel is lying.

I bisected linux-2.6.35.y.git, and found that the kernel started reporting bad blocks starting with this commit:
426c457a3216fac74e mtd: nand: extend NAND flash detection to new MLC chips

Patching nand_base.c with the following diff (thus restoring the previously used behaviour) the kernel stops reporting bad blocks:

@@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,

                 */
                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] &&
                                id_data[0] == NAND_MFR_SAMSUNG &&
+                               (chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
                                id_data[5] != 0x00) {
                        /* Calc pagesize */
                        mtd->writesize = 2048 << (extid & 0x03);

I added a check for the MLC bit since
a) the commit message talks about MLC chips and
b) the NAND chip that the GuruPlug features is a Samsung K9F4G08U0B (which is SLC according to the part number decoder on samsung.com)

I have no idea if that's the right fix though. Unfortunately I couldn't find the data sheet for the K9F4G08U0B online, so I couldn't verify if it uses the old field definition or the new one...

Please CC me in your replies; I'm not subscribed to the list.

Thanks,
Tilman

Kernel messages:
Scanning device for bad blocks
Bad eraseblock 0 at 0x000000000000
Bad eraseblock 1 at 0x000000400000
[same for the following blocks]
Bad eraseblock 127 at 0x00001fc00000
Creating 3 MTD partitions on "orion_nand":
0x000000000000-0x000000100000 : "u-boot"
mtd: partition "u-boot" doesn't end on an erase block -- force read-only Moving partition 1: 0x000000100000 -> 0x000000400000 0x000000400000-0x000000800000 : "uImage" 0x000000800000-0x000020000000 : "root"

Signature · 4 lines
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
#1 lore raw

Hello,

I have similar problems on similar chips, however, I cannot determine that for sure; can you print out the full ID string (8 bytes) from nand_base?

This may be an issue with an unfortunate wraparound of the ID where it *should* give a 5-byte string, then wraparound to give the same ID again, but instead it gives a 6-byte string, then wraps around again. This would incorrectly classify this ID string under the detection scheme for new Samsung MLC flash instead of the old scheme. Your fix is safe, AFAIK, although there may be other Samsung MLC that still have unpredictable ID patterns that will cause this pattern. Maybe we can submit a similar patch to yours.

Thanks,
Brian

Quoted 49 lines

Hi,
on my GuruPlug, I was seeing lots of bad block errors for the NAND chip when booting a 2.6.35 kernel. With earlier kernels (2.6.33, 2.6.34), no bad blocks were reported. See below for the exact error messages. Since _all_ of the first 128 blocks are claimed to be bad, and the NAND chips appears to be working just fine, I suspect the kernel is lying.

I bisected linux-2.6.35.y.git, and found that the kernel started reporting bad blocks starting with this commit:
426c457a3216fac74e mtd: nand: extend NAND flash detection to new MLC chips

Patching nand_base.c with the following diff (thus restoring the previously used behaviour) the kernel stops reporting bad blocks:

(quoted diff) +1 -0
@@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                 */
                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] &&
                                id_data[0] == NAND_MFR_SAMSUNG &&
+                               (chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
                                id_data[5] != 0x00) {
                        /* Calc pagesize */
                        mtd->writesize = 2048 << (extid & 0x03);

I added a check for the MLC bit since
a) the commit message talks about MLC chips and
b) the NAND chip that the GuruPlug features is a Samsung K9F4G08U0B (which is SLC according to the part number decoder on samsung.com)

I have no idea if that's the right fix though. Unfortunately I couldn't find the data sheet for the K9F4G08U0B online, so I couldn't verify if it uses the old field definition or the new one...

Please CC me in your replies; I'm not subscribed to the list.

Thanks,
Tilman

Kernel messages:
Scanning device for bad blocks
Bad eraseblock 0 at 0x000000000000
Bad eraseblock 1 at 0x000000400000
[same for the following blocks]
Bad eraseblock 127 at 0x00001fc00000
Creating 3 MTD partitions on "orion_nand":
0x000000000000-0x000000100000 : "u-boot"
mtd: partition "u-boot" doesn't end on an erase block -- force read-only Moving partition 1: 0x000000100000 -> 0x000000400000 0x000000400000-0x000000800000 : "uImage" 0x000000800000-0x000020000000 : "root"

author
#2 lore raw

Brian Norris [2010-08-18 16:25]:

Hi,

Quoted 3 lines

I have similar problems on similar chips, however, I cannot determine that for sure; can you print out the full ID string (8 bytes) from nand_base?

ID: ec dc 10 95 54 ec ec dc

Quoted 4 lines

This may be an issue with an unfortunate wraparound of the ID where it *should* give a 5-byte string, then wraparound to give the same ID again, but instead it gives a 6-byte string, then wraps around again. This would incorrectly classify this ID string under the

I think I might have found the real problem.
Kevin's commit message says:

Quoted 4 lines

This patch uses the new 6-byte scheme if the following conditions are all true:
[...]
3) 6th byte is zero

... but what the code does is use new scheme if id_data[5] is _not_ zero.

The following diff works for me, but I cannot test with any other flash chip but the K9F4G08U0B:

drivers/mtd/nand/nand_base.c +2 -2
index 4a7b864..abc71cd 100644
@@ -2847,12 +2847,12 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                 * Old style (4,5 byte ID): Samsung K9GAG08U0M (p.32)
                 * New style   (6 byte ID): Samsung K9GAG08U0D (p.40)
                 *
-                * Check for wraparound + Samsung ID + nonzero 6th byte
+                * Check for wraparound + Samsung ID + zero 6th byte
                 * to decide what to do.
                 */
                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] &&
                                id_data[0] == NAND_MFR_SAMSUNG &&
-                               id_data[5] != 0x00) {
+                               id_data[5] == 0x00) {
                        /* Calc pagesize */
                        mtd->writesize = 2048 << (extid & 0x03);
                        extid >>= 2;

Thanks,
Tilman

Signature · 4 lines
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
#3 lore raw
Quoted 7 lines · Tilman Sauerbeck
Quoted 4 lines

  This patch uses the new 6-byte scheme if the following conditions are all true:
  [...]
  3) 6th byte is zero

... but what the code does is use new scheme if id_data[5] is _not_ zero.

ID for Samsung K9GAG08U0D (6-byte sequence): ec d5 94 29 34 41

The code is right; the comment is wrong.

#4 lore raw
Quoted 1 line · Kevin Cernekee

The code is right; the comment is wrong.

In that case, should we fix the comment and add the check that Tilman mentioned previously? (below) This looks safe and will at least eliminate a few weird cases where Samsung SLC happen to have 6-byte ID strings (such as the K9F4G08U0B in question).

I have another fix I will tack on in series to this; should I include you as a "Signed-off-by", Tilman?

Brian

Quoted 8 lines
drivers/mtd/nand/nand_base.c +1 -0
@@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                 */
                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] &&
                                id_data[0] == NAND_MFR_SAMSUNG &&
+                               (chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
                                id_data[5] != 0x00) {
                        /* Calc pagesize */
                        mtd->writesize = 2048 << (extid & 0x03);
#5 lore raw
Quoted 5 lines · Brian Norris
Quoted 1 line · Kevin Cernekee

The code is right; the comment is wrong.

In that case, should we fix the comment and add the check that Tilman mentioned previously? (below)

To clarify - the comment in the code is correct, but the checkin comment was inaccurate.

Quoted 8 lines
Quoted 8 lines
(quoted diff) +0 -0
@@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                  */

                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] && id_data[0] == NAND_MFR_SAMSUNG &&
+                               (chip->cellinfo & NAND_CI_CELLTYPE_MSK) && id_data[5] != 0x00) {
                        /* Calc pagesize */
                        mtd->writesize = 2048 << (extid & 0x03);

This looks OK (at least for K9GAG08U0D).

I wonder if Samsung could provide some firm guidelines for when to use the old style vs. the new style.

#6 lore raw
Quoted 20 lines

-----Original Message-----
From: [email protected] [mailto:linux-mtd- [email protected]] On Behalf Of Kevin Cernekee
Sent: Friday, August 20, 2010 11:30 AM
To: Brian Norris
Cc: [email protected]; Tilman Sauerbeck
Subject: Re: Bad assumption about ID field definition for Samsung NAND?

Quoted 5 lines · Brian Norris
Quoted 1 line · Kevin Cernekee

The code is right; the comment is wrong.

In that case, should we fix the comment and add the check that Tilman mentioned previously? (below)

To clarify - the comment in the code is correct, but the checkin comment was inaccurate.

Quoted 13 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +0 -0
@@ -2852,6 +2852,7 @@ static struct nand_flash_dev
 *nand_get_flash_type(struct mtd_info *mtd,
                  */

                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] &&
                                id_data[0] == NAND_MFR_SAMSUNG &&
+                               (chip->cellinfo &
+ NAND_CI_CELLTYPE_MSK) &&
                                id_data[5] != 0x00) { /* Calc pagesize */ mtd->writesize = 2048 << (extid & 0x03);

This looks OK (at least for K9GAG08U0D).

The temp fix is not the good solution. We can make a fix for Samsung here, what about other vendors? If we put such kind of fix into nand_base.c file, which will make the code ugly.

Quoted 7 lines

I wonder if Samsung could provide some firm guidelines for when to use the old style vs. the new style.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

author
#7 lore raw
In reply to Kevin Cernekee

Kevin Cernekee [2010-08-19 20:29]:

Quoted 13 lines · Brian Norris
Quoted 9 lines · Kevin Cernekee
Quoted 8 lines
(quoted diff) +0 -0
@@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                  */

                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] && id_data[0] == NAND_MFR_SAMSUNG &&
+                               (chip->cellinfo & NAND_CI_CELLTYPE_MSK) && id_data[5] != 0x00) {
                        /* Calc pagesize */
                        mtd->writesize = 2048 << (extid & 0x03);

This looks OK (at least for K9GAG08U0D).

Quoted 2 lines

I wonder if Samsung could provide some firm guidelines for when to use the old style vs. the new style.

Okay, how do we proceed? Should I send a proper patch with the diff above? Or does anyone want to try and come up with a better fix...?

Regards,
Tilman

Signature · 4 lines
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
#8 lore raw
Quoted 2 lines · Tilman Sauerbeck

Okay, how do we proceed? Should I send a proper patch with the diff above? Or does anyone want to try and come up with a better fix...?

I vote for Tilman's patch. There's nothing unnecessarily ugly about it; it simply checks cell-type in order to decide whether we use Samsung's new "standard" for MLC or fall-back to the real standard. If anything, the existing code (checking ID length) is ugly. However, both checks seem necessary.

Brian

#9 lore raw
Quoted 10 lines · Brian Norris
Quoted 2 lines · Tilman Sauerbeck

Okay, how do we proceed? Should I send a proper patch with the diff above? Or does anyone want to try and come up with a better fix...?

I vote for Tilman's patch. There's nothing unnecessarily ugly about it; it simply checks cell-type in order to decide whether we use Samsung's new "standard" for MLC or fall-back to the real standard. If anything, the existing code (checking ID length) is ugly. However, both checks seem necessary.

That's for 2.6.36 and -stable (for 2.6.35), yes?

@@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,

                 */
                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] &&
                                id_data[0] == NAND_MFR_SAMSUNG &&
+                               (chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
                                id_data[5] != 0x00) {
                        /* Calc pagesize */
                        mtd->writesize = 2048 << (extid & 0x03);

Can I have a signed-off-by for it?

Brian, I have a distinct impression that there's at least one more patch from you that I really ought to be sending to Linus for 2.6.36, but I can't find it right now. Other than this and what's already in mtd-2.6.git, is there anything else?

Thanks, btw.

Signature · 1 line
dwmw2
author
#10 lore raw

David Woodhouse [2010-08-20 20:53]:

Quoted 6 lines · Brian Norris
Quoted 5 lines · Tilman Sauerbeck
Quoted 3 lines

Okay, how do we proceed? Should I send a proper patch with the diff above? Or does anyone want to try and come up with a better fix...?

Quoted 7 lines
Quoted 5 lines

I vote for Tilman's patch. There's nothing unnecessarily ugly about it; it simply checks cell-type in order to decide whether we use Samsung's new "standard" for MLC or fall-back to the real standard. If anything, the existing code (checking ID length) is ugly. However, both checks seem necessary.

That's for 2.6.36 and -stable (for 2.6.35), yes?

Yes.

Quoted 10 lines
(quoted diff) +1 -0
@@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                 */
                if (id_data[0] == id_data[6] && id_data[1] == id_data[7] &&
                                id_data[0] == NAND_MFR_SAMSUNG &&
+                               (chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
                                id_data[5] != 0x00) {
                        /* Calc pagesize */
                        mtd->writesize = 2048 << (extid & 0x03);

Can I have a signed-off-by for it?

Signed-off-by Tilman Sauerbeck <[email protected]>

Thanks,
Tilman

Signature · 4 lines
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
#11 lore raw
In reply to David Woodhouse
Quoted 1 line

Can I have a signed-off-by for it?

I don't know what's "legal" here. I'm appending the patch with a sign-off for me and Tilman (since Tilman authored it). Hopefully that's ok.

Quoted 4 lines

Brian, I have a distinct impression that there's at least one more patch from you that I really ought to be sending to Linus for 2.6.36, but I can't find it right now. Other than this and what's already in mtd-2.6.git, is there anything else?

I'm really no expert on how inclusion for different versions goes; I just send 'em to you! Anyway, this patch is *very* important:

	* [PATCH] mtd: nand: Fix regression in BBM detection
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031594.html

It addresses issues I overlooked with a lot of Hynix small-page NAND (and others).

Other patches - they are ready, but not as important:

	* Spansion ORNAND
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031603.html
	* New Samsung MLC OOB sizes
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031621.html

No one has decided between these two (it's a "choose one or the other" situation). They may or may not be ready:

	mtd: nand: Expand nand_ecc_layout, deprecate ioctl ECCGETLAYOUT
	Cover page: http://lists.infradead.org/pipermail/linux-mtd/2010-August/031591.html
	Choice 1: http://lists.infradead.org/pipermail/linux-mtd/2010-August/031593.html
	Choice 2: http://lists.infradead.org/pipermail/linux-mtd/2010-August/031598.html
	Explanation: http://lists.infradead.org/pipermail/linux-mtd/2010-August/031619.html

And since you asked, the trivial...

	Indentation errors :)
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031588.html

You already got this one, I believe:

	Fixing a typo in on a buswidth option
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031518.html

Thanks for looking out for me :)

Brian

----------------------------------------------------------------

Apparently, the check for a 6-byte ID string is NOT sufficient to determine whether or not a Samsung chip uses their new MLC detection scheme or the old, standard scheme. This adds a condition to check cell type.

Signed-off-by Tilman Sauerbeck <[email protected]>
Signed-off-by Brian Norris <[email protected]>
---
 drivers/mtd/nand/nand_base.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/mtd/nand/nand_base.c +1 -0
index a3c7473..172a299 100644
@@ -2866,6 +2866,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		 */
 		if (id_data[0] == id_data[6] && id_data[1] == id_data[7] &&
 				id_data[0] == NAND_MFR_SAMSUNG &&
+				(chip->cellinfo & NAND_CI_CELLTYPE_MSK) &&
 				id_data[5] != 0x00) {
 			/* Calc pagesize */
 			mtd->writesize = 2048 << (extid & 0x03);
Signature · 1 line
1.7.0.4
#12 lore raw
Quoted 5 lines · Brian Norris
Quoted 1 line

Can I have a signed-off-by for it?

I don't know what's "legal" here. I'm appending the patch with a sign-off for me and Tilman (since Tilman authored it). Hopefully that's ok.

You have to cut and paste Tilman's own Signed-off-by: header; the magic doesn't work if you type it yourself. :)

Quoted 11 lines
Quoted 4 lines

Brian, I have a distinct impression that there's at least one more patch from you that I really ought to be sending to Linus for 2.6.36, but I can't find it right now. Other than this and what's already in mtd-2.6.git, is there anything else?

I'm really no expert on how inclusion for different versions goes; I just send 'em to you! Anyway, this patch is *very* important:

	* [PATCH] mtd: nand: Fix regression in BBM detection
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031594.html

It addresses issues I overlooked with a lot of Hynix small-page NAND (and others).

That's in the tree already:
http://git.infradead.org/mtd-2.6.git/commitdiff/065a1ed8

It's also https://bugzilla.kernel.org/show_bug.cgi?id=16639

Quoted 5 lines

Other patches - they are ready, but not as important:

	* Spansion ORNAND
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031603.html
	* New Samsung MLC OOB sizes
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031621.html

Those aren't regressions or important bug-fixes, so given the timing I think they've missed the merge window and are now candidates for 2.6.37 rather than 2.6.36?

I'll merge them as soon as I've asked Linus to pull what's in the tree right now. Unless you object to my classification?

Quoted 7 lines

No one has decided between these two (it's a "choose one or the other" situation). They may or may not be ready:

	mtd: nand: Expand nand_ecc_layout, deprecate ioctl ECCGETLAYOUT
	Cover page: http://lists.infradead.org/pipermail/linux-mtd/2010-August/031591.html
	Choice 1: http://lists.infradead.org/pipermail/linux-mtd/2010-August/031593.html
	Choice 2: http://lists.infradead.org/pipermail/linux-mtd/2010-August/031598.html
	Explanation: http://lists.infradead.org/pipermail/linux-mtd/2010-August/031619.html

Yeah, definitely 2.6.37 material.

Quoted 3 lines

And since you asked, the trivial...

	Indentation errors :)
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031588.html

That can wait for 2.6.37 too.

Quoted 3 lines

You already got this one, I believe:

	Fixing a typo in on a buswidth option
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031518.html

Yep, that's already in 2.6.36-rc1.

Quoted 6 lines

Thanks for looking out for me :)

Brian

----------------------------------------------------------------

Quoted 8 lines

Apparently, the check for a 6-byte ID string is NOT sufficient to determine whether or not a Samsung chip uses their new MLC detection scheme or the old, standard scheme. This adds a condition to check cell type.

Signed-off-by Tilman Sauerbeck <[email protected]>
Signed-off-by Brian Norris <[email protected]>

---

Just FYI; not to criticise when you're doing such excellent work -- this would ideally have a From: and Subject: "header" indicating that Tilman is the author, and giving the first line of the commit comment. That way, running 'git-am' on it would fairly much work. Not that it's a problem for me to do it either, of course.

Thanks again.

Signature · 2 lines
David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation
#13 lore raw
Quoted 9 lines · David Woodhouse
Quoted 5 lines · Brian Norris
Quoted 1 line

Can I have a signed-off-by for it?

I don't know what's "legal" here. I'm appending the patch with a sign-off for me and Tilman (since Tilman authored it). Hopefully that's ok.

You have to cut and paste Tilman's own Signed-off-by: header; the magic doesn't work if you type it yourself. :)

I'm glad someone has a sense of humor (or humour, depending on the country)

Quoted 9 lines
Quoted 6 lines

I'm really no expert on how inclusion for different versions goes; I just send 'em to you! Anyway, this patch is *very* important:

	* [PATCH] mtd: nand: Fix regression in BBM detection
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031594.html

It addresses issues I overlooked with a lot of Hynix small-page NAND (and others).

That's in the tree already:
http://git.infradead.org/mtd-2.6.git/commitdiff/065a1ed8

I realized that right after sending this.

Quoted 6 lines
Quoted 5 lines

Other patches - they are ready, but not as important:

	* Spansion ORNAND
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031603.html
	* New Samsung MLC OOB sizes
	http://lists.infradead.org/pipermail/linux-mtd/2010-August/031621.html
Quoted 6 lines

Those aren't regressions or important bug-fixes, so given the timing I think they've missed the merge window and are now candidates for 2.6.37 rather than 2.6.36?

I'll merge them as soon as I've asked Linus to pull what's in the tree right now. Unless you object to my classification?

No objection.

Quoted 5 lines

Just FYI; not to criticise when you're doing such excellent work -- this would ideally have a From: and Subject: "header" indicating that Tilman is the author, and giving the first line of the commit comment. That way, running 'git-am' on it would fairly much work. Not that it's a problem for me to do it either, of course.

I see. (Thanks for the compliment, btw, I'm rather new to this)

Brian