GPMI-NAND: Wrong ECC size in driver

DISCUSSION Marek Vasut 4 participants 15 messages Jan 4, 2012 – Feb 3, 2012 Memory technology devices (MTD)

lore

Share & tools

lore

Marek Vasut

author

2012-01-04 00:48 UTC #0 lore raw

Hi,

the gpmi-nand.c driver apparently has misconfigured ecc.size field:

drivers/mtd/nand/gpmi-nand/gpmi-nand.c:
---------- >8 ----------

1493         chip->ecc.mode          = NAND_ECC_HW;
1494         chip->ecc.size          = 1;
1495         chip->ecc.layout        = &gpmi_hw_ecclayout;

---------- 8< ----------

This boils down to misconfigured mtd->subpage_sft in:

drivers/mtd/nand/nand_base.c:
---------- >8 ----------
3434 /* Allow subpage writes up to ecc.steps. Not possible for MLC flash
*/
3435 if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
3436 !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) {
3437 switch (chip->ecc.steps) {
3438 case 2:
3439 mtd->subpage_sft = 1;
3440 break;

3441                 case 4:
3442                 case 8:
3443                 case 16:

3444 mtd->subpage_sft = 2;
3445 break;
3446 }
3447 }
3448 chip->subpagesize = mtd->writesize >> mtd->subpage_sft;
---------- 8< ----------

The mtd->subpage_sft is accidentally 0 by default. Therefore, the subpage size is mtd->writesize >> 0. Therefore the subpage size reported to UBI formating tools is the size of whole page, which works. Now, if I correct the problem by setting ecc.size properly to 512 bytes, I get into trouble with UBI:

1) UBI formating tools are reported the driver CAN DO subpage writes
2) UBI formating tools are reported the subpage size is 512 bytes

and this happens even though the NAND_NO_SUBPAGE_WRITE option is set by the driver. This is because:

drivers/mtd/nand/nand_base.c:
---------- >8 ----------

3072         /* Get chip options, preserve non chip based options */
3073         chip->options &= ~NAND_CHIPOPTIONS_MSK;
3074         chip->options |= type->options & NAND_CHIPOPTIONS_MSK;

---------- 8< ----------

which effectively masks that bit away. So, how are we going to fix it?

Cheers!

Marek

Huang Shijie

2012-01-04 05:57 UTC #1 lore raw

Hi Marek:

Quoted 33 lines

Hi,
the gpmi-nand.c driver apparently has misconfigured ecc.size field:
drivers/mtd/nand/gpmi-nand/gpmi-nand.c:
---------->8 ----------
1493         chip->ecc.mode          = NAND_ECC_HW;
1494         chip->ecc.size          = 1;
1495         chip->ecc.layout        =&gpmi_hw_ecclayout;
---------- 8< ----------
This boils down to misconfigured mtd->subpage_sft in:
drivers/mtd/nand/nand_base.c:
---------->8 ----------
3434 /* Allow subpage writes up to ecc.steps. Not possible for MLC flash
*/
3435 if (!(chip->options& NAND_NO_SUBPAGE_WRITE)&&
3436 !(chip->cellinfo& NAND_CI_CELLTYPE_MSK)) {
3437 switch (chip->ecc.steps) {
3438 case 2:
3439 mtd->subpage_sft = 1;
3440 break;
3441                 case 4:
3442                 case 8:
3443                 case 16:
3444 mtd->subpage_sft = 2;
3445 break;
3446 }
3447 }
3448 chip->subpagesize = mtd->writesize>> mtd->subpage_sft;
---------- 8< ----------

Quoted 8 lines

The mtd->subpage_sft is accidentally 0 by default. Therefore, the subpage size is mtd->writesize>> 0. Therefore the subpage size reported to UBI formating tools is the size of whole page, which works. Now, if I correct the problem by setting ecc.size properly to 512 bytes, I get into trouble with UBI:
1) UBI formating tools are reported the driver CAN DO subpage writes
2) UBI formating tools are reported the subpage size is 512 bytes

The gpmi driver does not support the subpage read/write.
I will be happy if the driver works only by setting the NAND_NO_SUBPAGE_WRITE, without setting the ecc.size.

Best Regards
Huang Shijie

Quoted 16 lines

and this happens even though the NAND_NO_SUBPAGE_WRITE option is set by the driver. This is because:
drivers/mtd/nand/nand_base.c:
---------->8 ----------
3072         /* Get chip options, preserve non chip based options */
3073         chip->options&= ~NAND_CHIPOPTIONS_MSK;
3074         chip->options |= type->options&  NAND_CHIPOPTIONS_MSK;
---------- 8< ----------
which effectively masks that bit away. So, how are we going to fix it?
Cheers!
Marek

Scott Wood

2012-01-04 17:30 UTC #2 lore raw

Quoted 11 lines · Huang Shijie

Hi Marek:
Quoted 10 lines
Hi,
the gpmi-nand.c driver apparently has misconfigured ecc.size field:
drivers/mtd/nand/gpmi-nand/gpmi-nand.c:
---------->8 ----------
1493         chip->ecc.mode          = NAND_ECC_HW;
1494         chip->ecc.size          = 1;
1495         chip->ecc.layout        =&gpmi_hw_ecclayout;
---------- 8< ----------

[snip]

Quoted 3 lines

The gpmi driver does not support the subpage read/write. I will be happy if the driver works only by setting the NAND_NO_SUBPAGE_WRITE, without setting the ecc.size.

Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't set options that aren't appropriate? Possibly replace it with documentation about which options are for chips, which are for drivers, and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.

-Scott

Marek Vasut

author

2012-01-04 21:32 UTC #3 lore raw

Quoted 15 lines · Huang Shijie

Quoted 11 lines
Hi Marek:
Quoted 10 lines
Hi,
the gpmi-nand.c driver apparently has misconfigured ecc.size field:
drivers/mtd/nand/gpmi-nand/gpmi-nand.c:
---------->8 ----------
1493         chip->ecc.mode          = NAND_ECC_HW;
1494         chip->ecc.size          = 1;
1495         chip->ecc.layout        =&gpmi_hw_ecclayout;
---------- 8< ----------
[snip]

Quoted 8 lines

Quoted 3 lines
The gpmi driver does not support the subpage read/write. I will be happy if the driver works only by setting the NAND_NO_SUBPAGE_WRITE, without setting the ecc.size.
Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't set options that aren't appropriate? Possibly replace it with documentation about which options are for chips, which are for drivers, and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.

Rather let's just adjust the mask?

Quoted 2 lines

-Scott

Scott Wood

2012-01-04 21:49 UTC #4 lore raw

Quoted 19 lines · Marek Vasut

Quoted 19 lines · Huang Shijie
Quoted 11 lines
Hi Marek:
Quoted 10 lines
Hi,
the gpmi-nand.c driver apparently has misconfigured ecc.size field:
drivers/mtd/nand/gpmi-nand/gpmi-nand.c:
---------->8 ----------
1493         chip->ecc.mode          = NAND_ECC_HW;
1494         chip->ecc.size          = 1;
1495         chip->ecc.layout        =&gpmi_hw_ecclayout;
---------- 8< ----------
[snip]
Quoted 3 lines
The gpmi driver does not support the subpage read/write. I will be happy if the driver works only by setting the NAND_NO_SUBPAGE_WRITE, without setting the ecc.size.

Quoted 6 lines

Quoted 4 lines
Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't set options that aren't appropriate? Possibly replace it with documentation about which options are for chips, which are for drivers, and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.
Rather let's just adjust the mask?

The way the mask is used means that any given option can only be chip or driver, not both. Though, I don't see anywhere this option is set by a chip -- maybe we can just renumber it to be in the controller half.

I still don't see a whole lot of value in the mask, though -- seems to be just causing problems, especially given that bits set by the "wrong" component are silently discarded.

-Scott

Marek Vasut

author

2012-01-04 23:38 UTC #5 lore raw

Quoted 25 lines · Marek Vasut

Quoted 24 lines
Quoted 23 lines · Huang Shijie
Quoted 11 lines
Hi Marek:
Quoted 10 lines
Hi,
the gpmi-nand.c driver apparently has misconfigured ecc.size field:
drivers/mtd/nand/gpmi-nand/gpmi-nand.c:
---------->8 ----------
1493         chip->ecc.mode          = NAND_ECC_HW;
1494         chip->ecc.size          = 1;
1495         chip->ecc.layout        =&gpmi_hw_ecclayout;
---------- 8< ----------
[snip]
Quoted 3 lines
The gpmi driver does not support the subpage read/write. I will be happy if the driver works only by setting the NAND_NO_SUBPAGE_WRITE, without setting the ecc.size.
Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't set options that aren't appropriate? Possibly replace it with documentation about which options are for chips, which are for drivers, and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.

Quoted 5 lines

Quoted 1 line
Rather let's just adjust the mask?
The way the mask is used means that any given option can only be chip or driver, not both. Though, I don't see anywhere this option is set by a chip -- maybe we can just renumber it to be in the controller half.

I suspect this was meant to allow controllers where one chip can do subpage- write and the other can not.

Quoted 4 lines

I still don't see a whole lot of value in the mask, though -- seems to be just causing problems, especially given that bits set by the "wrong" component are silently discarded.

Yes. Patch is welcome to remove it and separate these two ;-)

Quoted 2 lines

-Scott

Scott Wood

2012-01-04 23:48 UTC #6 lore raw

Quoted 8 lines · Marek Vasut

Quoted 7 lines · Scott Wood
Quoted 6 lines · Marek Vasut
Quoted 4 lines · Scott Wood
Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't set options that aren't appropriate? Possibly replace it with documentation about which options are for chips, which are for drivers, and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Rather let's just adjust the mask?
The way the mask is used means that any given option can only be chip or driver, not both. Though, I don't see anywhere this option is set by a chip -- maybe we can just renumber it to be in the controller half.
I suspect this was meant to allow controllers where one chip can do subpage- write and the other can not.

What I mean is I don't see any place where this is actually used -- gpmi-nand is the only place I see this flag being set (though it belongs on at least elbc as well).

Quoted 5 lines

Quoted 3 lines
I still don't see a whole lot of value in the mask, though -- seems to be just causing problems, especially given that bits set by the "wrong" component are silently discarded.
Yes. Patch is welcome to remove it and separate these two ;-)

Do they need to be separated? Just OR them with no mask. If either the controller or the chip set the flag, then it's set, and subpage writes are disallowed. Obviously you can only do this for certain types of options -- for others, just don't set the flag in an inappropriate context.

-Scott

Marek Vasut

author

2012-01-31 11:33 UTC #7 lore raw

Quoted 27 lines · Marek Vasut

Quoted 9 lines
Quoted 8 lines · Scott Wood
Quoted 7 lines · Marek Vasut
Quoted 5 lines · Scott Wood
Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't set options that aren't appropriate? Possibly replace it with documentation about which options are for chips, which are for drivers, and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Rather let's just adjust the mask?
The way the mask is used means that any given option can only be chip or driver, not both. Though, I don't see anywhere this option is set by a chip -- maybe we can just renumber it to be in the controller half.
I suspect this was meant to allow controllers where one chip can do subpage- write and the other can not.
What I mean is I don't see any place where this is actually used -- gpmi-nand is the only place I see this flag being set (though it belongs on at least elbc as well).
Quoted 4 lines
Quoted 3 lines
I still don't see a whole lot of value in the mask, though -- seems to be just causing problems, especially given that bits set by the "wrong" component are silently discarded.

Quoted 8 lines

Quoted 1 line
Yes. Patch is welcome to remove it and separate these two ;-)
Do they need to be separated? Just OR them with no mask. If either the controller or the chip set the flag, then it's set, and subpage writes are disallowed. Obviously you can only do this for certain types of options -- for others, just don't set the flag in an inappropriate context.
-Scott

BUMP?

Brian Norris

2012-01-31 17:30 UTC #8 lore raw

Quoted 16 lines · Marek Vasut

Quoted 16 lines · Marek Vasut
Quoted 15 lines
Quoted 13 lines · Scott Wood
Quoted 8 lines · Marek Vasut
Quoted 5 lines · Scott Wood
Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't set options that aren't appropriate? Possibly replace it with documentation about which options are for chips, which are for drivers, and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.
Rather let's just adjust the mask?
The way the mask is used means that any given option can only be chip or driver, not both. Though, I don't see anywhere this option is set by a chip -- maybe we can just renumber it to be in the controller half.

Quoted 6 lines

Quoted 6 lines
Quoted 2 lines
I suspect this was meant to allow controllers where one chip can do subpage- write and the other can not.
What I mean is I don't see any place where this is actually used -- gpmi-nand is the only place I see this flag being set (though it belongs on at least elbc as well).

It is used out-of-tree as well (as mentioned below).

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
I still don't see a whole lot of value in the mask, though -- seems to be just causing problems, especially given that bits set by the "wrong" component are silently discarded.
Yes. Patch is welcome to remove it and separate these two ;-)

Quoted 8 lines

Quoted 6 lines
Do they need to be separated? Just OR them with no mask. If either the controller or the chip set the flag, then it's set, and subpage writes are disallowed. Obviously you can only do this for certain types of options -- for others, just don't set the flag in an inappropriate context.
-Scott
BUMP?

There's another distinct possibility (one that I use on an out-of-tree driver): instead of calling "nand_scan()", separate it into its two sub-functions "nand_scan_ident()" and "nand_scan_tail()" so you have something like this

    ret = nand_scan_ident(mtd, pdata->max_chip_count, NULL);
    chip->options |= NAND_NO_SUBPAGE_WRITE;
    if (!ret)
            ret = nand_scan_tail(mtd);

That way your option is set after we mask with NAND_CHIPOPTIONS_MSK. I'm not sure if this solution is better than really trying to tackle the issue of why we have that mask and whether it is still needed, but it solves the problem.

Brian

Scott Wood

2012-01-31 19:09 UTC #9 lore raw

Quoted 10 lines · Brian Norris

There's another distinct possibility (one that I use on an out-of-tree driver): instead of calling "nand_scan()", separate it into its two sub-functions "nand_scan_ident()" and "nand_scan_tail()" so you have something like this
    ret = nand_scan_ident(mtd, pdata->max_chip_count, NULL);
    chip->options |= NAND_NO_SUBPAGE_WRITE;
    if (!ret)
            ret = nand_scan_tail(mtd);

Quoted 4 lines

That way your option is set after we mask with NAND_CHIPOPTIONS_MSK. I'm not sure if this solution is better than really trying to tackle the issue of why we have that mask and whether it is still needed, but it solves the problem.

It's a bit hackish, but should work. Why not get just rid of the mask, though, if it's not actually doing anything constructive and is requiring such workarounds?

-Scott

Brian Norris

2012-02-03 02:43 UTC #10 lore raw

Quoted 17 lines · Scott Wood

Quoted 10 lines · Brian Norris
There's another distinct possibility (one that I use on an out-of-tree driver): instead of calling "nand_scan()", separate it into its two sub-functions "nand_scan_ident()" and "nand_scan_tail()" so you have something like this
ret = nand_scan_ident(mtd, pdata->max_chip_count, NULL); chip->options |= NAND_NO_SUBPAGE_WRITE;
if (!ret)
ret = nand_scan_tail(mtd);
Quoted 4 lines
That way your option is set after we mask with NAND_CHIPOPTIONS_MSK. I'm not sure if this solution is better than really trying to tackle the issue of why we have that mask and whether it is still needed, but it solves the problem.

Quoted 3 lines

It's a bit hackish, but should work. Why not get just rid of the mask, though, if it's not actually doing anything constructive and is requiring such workarounds?

I'm not sure "why not." I hadn't looked at it in enough detail to tell for sure. I was just providing my own solution, which doesn't require any big system changes.

And actually, I think that my solution isn't really too "hackish." I believe nand_scan_ident() and nand_scan_tail() are intentionally exported separately to support some driver-level tweaking between the initialization steps. For instance, my systems use the nand_base-detected parameters (size, page size, etc.) to initialize controller values before continuing to the tail, where we build our BBT. I don't see why other options can't be tweaked here as well.

But I do see how, on a system that otherwise could use the default nand_scan() interface, it seems counterintuitive that you cannot set NAND_NO_SUBPAGE_WRITE before scanning.

Brian

Brian Norris

2012-02-03 03:16 UTC #11 lore raw

In reply to Scott Wood

Quoted 15 lines · Scott Wood

Quoted 14 lines · Marek Vasut
Quoted 12 lines · Scott Wood
Quoted 7 lines · Marek Vasut
Quoted 4 lines · Scott Wood
Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't set options that aren't appropriate? Possibly replace it with documentation about which options are for chips, which are for drivers, and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.
Rather let's just adjust the mask?
The way the mask is used means that any given option can only be chip or driver, not both. Though, I don't see anywhere this option is set by a chip -- maybe we can just renumber it to be in the controller half.

Quoted 6 lines

Quoted 2 lines
I suspect this was meant to allow controllers where one chip can do subpage- write and the other can not.
What I mean is I don't see any place where this is actually used -- gpmi-nand is the only place I see this flag being set (though it belongs on at least elbc as well).

It's never actually set per-chip (i.e., it's not in the nand_ids table or similar), so I don't see a problem with Scott's suggestion: "maybe we can just renumber it to be in the controller half." But that may not be the best way (see below)

Quoted 10 lines

Quoted 5 lines
Quoted 3 lines
I still don't see a whole lot of value in the mask, though -- seems to be just causing problems, especially given that bits set by the "wrong" component are silently discarded.
Yes. Patch is welcome to remove it and separate these two ;-)
Do they need to be separated? Just OR them with no mask. If either the controller or the chip set the flag, then it's set, and subpage writes are disallowed. Obviously you can only do this for certain types of options -- for others, just don't set the flag in an inappropriate context.

It looks like this might work as well. Now that I look at the code a bit, it seems that there's at least one other option (NAND_NO_AUTOINCR) which is sometimes set by the controller *and* per-chip (in nand_ids.c) but the per-chip option will always be silently masked out in nand_base.c:

  chip->options |= type->options & NAND_CHIPOPTIONS_MSK;

But it also seems that NAND_NO_AUTOINCR has no effect in the kernel anyway... (seems like we should just kill the option, then, right? But I digress...)

So there seems to be more than one issue related to a mostly unnecessary mask, and unless there's a strong reason against it, I like Scott's last suggestions a little better ("Just OR them with no mask" and "just don't set the flag in an inappropriate context").

Brian

Marek Vasut

author

2012-01-04 21:33 UTC #12 lore raw

In reply to Huang Shijie

Quoted 40 lines

Hi Marek:
Quoted 39 lines
Hi,
the gpmi-nand.c driver apparently has misconfigured ecc.size field:
drivers/mtd/nand/gpmi-nand/gpmi-nand.c:
---------->8 ----------
1493         chip->ecc.mode          = NAND_ECC_HW;
1494         chip->ecc.size          = 1;
1495         chip->ecc.layout        =&gpmi_hw_ecclayout;
---------- 8< ----------
This boils down to misconfigured mtd->subpage_sft in:
drivers/mtd/nand/nand_base.c:
---------->8 ----------
3434 /* Allow subpage writes up to ecc.steps. Not possible for
MLC flash */
3435 if (!(chip->options& NAND_NO_SUBPAGE_WRITE)&&
3436 !(chip->cellinfo& NAND_CI_CELLTYPE_MSK)) {
3437 switch (chip->ecc.steps) {
3438 case 2:
3439 mtd->subpage_sft = 1;
3440 break;
3441                 case 4:
3442                 case 8:
3443                 case 16:
3444 mtd->subpage_sft = 2;
3445 break;
3446 }
3447 }
3448 chip->subpagesize = mtd->writesize>> mtd->subpage_sft;
---------- 8< ----------
The mtd->subpage_sft is accidentally 0 by default. Therefore, the subpage size is mtd->writesize>> 0. Therefore the subpage size reported to UBI formating tools is the size of whole page, which works. Now, if I correct the problem by setting ecc.size properly to 512 bytes, I get into trouble with UBI:

Quoted 4 lines

Quoted 2 lines
1) UBI formating tools are reported the driver CAN DO subpage writes
2) UBI formating tools are reported the subpage size is 512 bytes
The gpmi driver does not support the subpage read/write.

That's what I said above.

Quoted 2 lines

I will be happy if the driver works only by setting the NAND_NO_SUBPAGE_WRITE, without setting the ecc.size.

So you intentionally set the ecc.size to 1 to work around the issue?

Quoted 19 lines

Best Regards
Huang Shijie
Quoted 15 lines
and this happens even though the NAND_NO_SUBPAGE_WRITE option is set by the driver. This is because:
drivers/mtd/nand/nand_base.c:
---------->8 ----------
3072         /* Get chip options, preserve non chip based options */
3073         chip->options&= ~NAND_CHIPOPTIONS_MSK;
3074         chip->options |= type->options&  NAND_CHIPOPTIONS_MSK;
---------- 8< ----------
which effectively masks that bit away. So, how are we going to fix it?
Cheers!
Marek

Huang Shijie

2012-01-05 02:08 UTC #13 lore raw

Hi,

Quoted 4 lines

Quoted 2 lines
I will be happy if the driver works only by setting the
Quoted 1 line
NAND_NO_SUBPAGE_WRITE, without setting the ecc.size.
So you intentionally set the ecc.size to 1 to work around the issue?

yes.
I read the code, and it seemed setting the ecc.size to 1 is the right solution.

Best Regards
Huang Shijie

Marek Vasut

author

2012-01-05 09:07 UTC #14 lore raw

Quoted 6 lines

Hi,
Quoted 4 lines
Quoted 3 lines
I will be happy if the driver works only by setting the
Quoted 1 line
NAND_NO_SUBPAGE_WRITE, without setting the ecc.size.

Quoted 8 lines

Quoted 1 line
So you intentionally set the ecc.size to 1 to work around the issue?
yes.
I read the code, and it seemed setting the ecc.size to 1 is the right solution.
Best Regards
Huang Shijie

It's definitelly wrong because it relies on undefined behaviour! The right solution here is to fix NAND_NO_SUBPAGE_WRITE pass-through as Scott and I proposed.