Patchwork Bad assumption about ID field definition for Samsung NAND?

login
register
mail settings
Submitter Tilman Sauerbeck
Date Aug. 19, 2010, 5:16 p.m.
Message ID <20100819171558.GA8536@code-monkey.de>
Download mbox | patch
Permalink /patch/62207/
State New
Headers show

Comments

Tilman Sauerbeck - Aug. 19, 2010, 5:16 p.m.
Brian Norris [2010-08-18 16:25]:

Hi,

> 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

> 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:

>   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:


Thanks,
Tilman
Kevin Cernekee - Aug. 19, 2010, 7:46 p.m.
On Thu, Aug 19, 2010 at 10:16 AM, Tilman Sauerbeck
<tilman@code-monkey.de> wrote:
>>   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.
Brian Norris - Aug. 19, 2010, 10:28 p.m.
On 08/19/2010 12:46 PM, Kevin Cernekee wrote:
> 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

> @@ -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);
Kevin Cernekee - Aug. 20, 2010, 3:29 a.m.
On Thu, Aug 19, 2010 at 3:28 PM, Brian Norris <norris@broadcom.com> wrote:
> On 08/19/2010 12:46 PM, Kevin Cernekee wrote:
>> 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.

>> @@ -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.
Liu Hui-R64343 - Aug. 20, 2010, 5:38 a.m.
> -----Original Message-----

> From: linux-mtd-bounces@lists.infradead.org [mailto:linux-mtd-

> bounces@lists.infradead.org] On Behalf Of Kevin Cernekee

> Sent: Friday, August 20, 2010 11:30 AM

> To: Brian Norris

> Cc: linux-mtd@lists.infradead.org; Tilman Sauerbeck

> Subject: Re: Bad assumption about ID field definition for Samsung NAND?

> 

> On Thu, Aug 19, 2010 at 3:28 PM, Brian Norris <norris@broadcom.com> wrote:

> > On 08/19/2010 12:46 PM, Kevin Cernekee wrote:

> >> 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.

> 

> >> @@ -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. 

> 

> 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/
Tilman Sauerbeck - Aug. 20, 2010, 1:43 p.m.
Kevin Cernekee [2010-08-19 20:29]:
> On Thu, Aug 19, 2010 at 3:28 PM, Brian Norris <norris@broadcom.com> wrote:
> > On 08/19/2010 12:46 PM, Kevin Cernekee wrote:
> >> @@ -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.

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
Brian Norris - Aug. 20, 2010, 5:42 p.m.
On 08/20/2010 06:43 AM, Tilman Sauerbeck wrote:

> 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
David Woodhouse - Aug. 20, 2010, 7:53 p.m.
On Fri, 2010-08-20 at 10:42 -0700, Brian Norris wrote:
> On 08/20/2010 06:43 AM, Tilman Sauerbeck wrote:
> 
> > 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.
Tilman Sauerbeck - Aug. 20, 2010, 8:51 p.m.
David Woodhouse [2010-08-20 20:53]:
> On Fri, 2010-08-20 at 10:42 -0700, Brian Norris wrote:
> > On 08/20/2010 06:43 AM, Tilman Sauerbeck wrote:
> > 
> > > 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?

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?

Signed-off-by: Tilman Sauerbeck <tilman@code-monkey.de>

Thanks,
Tilman

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4a7b864..abc71cd 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -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;