[PATCH V2] mtd: gpmi: fix the ecc regression

Discussion:

Huang Shijie

2013-10-24 08:14:44 UTC

The legacy ecc layout is to use all the OOB area by computing the ecc strength
and ecc step size ourselves.

The patch "2febcdf mtd: gpmi: set the BCHs geometry with the ecc info"
makes the gpmi to use the ECC info(ecc strength and ecc step size)
provided by the MTD code, and creates a different NAND ecc layout
for the BCH , and use the new ecc layout, this brings a regression to us:
We can not mount the ubifs which was created by the old NAND ecc layout.

This patch fixes this issue by use the legacy ecc layout firstly, if it fails
we try to use the new ecc layout.

Signed-off-by: Huang Shijie <***@freescale.com>
Cc: ***@vger.kernel.org
---
v1 --> v2:
[0] remove the DT binding.
---
drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 6807d7c..f9ee7e5 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -352,7 +352,7 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)

int common_nfc_set_geometry(struct gpmi_nand_data *this)
{
- return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
+ return legacy_set_geometry(this) ? set_geometry_by_ecc_info(this) : 0;
}

struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)

--
1.7.2.rc3

Huang Shijie

2013-10-24 08:48:16 UTC

Permalink

The legacy ecc layout is to use all the OOB area by computing the ecc strength
and ecc step size ourselves.

The patch "2febcdf mtd: gpmi: set the BCHs geometry with the ecc info"
makes the gpmi to use the ECC info(ecc strength and ecc step size)
provided by the MTD code, and creates a different NAND ecc layout
for the BCH , and use the new ecc layout, this brings a regression to us:
We can not mount the ubifs which was created by the old NAND ecc layout.

This patch fixes this issue by use the legacy ecc layout firstly, if it fails
we try to use the new ecc layout.

Signed-off-by: Huang Shijie <***@freescale.com>
Cc: ***@vger.kernel.org
---
revert the return value of set_geometry_by_ecc_info().

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

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 6807d7c..43a62e8 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -352,7 +352,8 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)

int common_nfc_set_geometry(struct gpmi_nand_data *this)
{
- return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
+ return legacy_set_geometry(this) ?
+ (!set_geometry_by_ecc_info(this)) : 0;
}

struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)

--
1.7.2.rc3

Brian Norris

2013-10-24 22:19:20 UTC

Permalink

Post by Huang Shijie
The legacy ecc layout is to use all the OOB area by computing the ecc strength
and ecc step size ourselves.
The patch "2febcdf mtd: gpmi: set the BCHs geometry with the ecc info"
makes the gpmi to use the ECC info(ecc strength and ecc step size)
provided by the MTD code, and creates a different NAND ecc layout
We can not mount the ubifs which was created by the old NAND ecc layout.
This patch fixes this issue by use the legacy ecc layout firstly, if it fails
we try to use the new ecc layout.

I fixed up the language a little, dropped the 'stable', and pushed to
linux-mtd.git (in hopes of sending another pull request for 3.12). I'll
see if David wants to weigh in first.

Feel free to send your updated DT binding patch(es).

Brian

David Woodhouse

2013-10-25 13:36:34 UTC

Permalink

Post by Brian Norris
I fixed up the language a little, dropped the 'stable', and pushed to
linux-mtd.git (in hopes of sending another pull request for 3.12). I'll
see if David wants to weigh in first.

You're fine. Go ahead and send the request if you're happy to do so.

Thanks.

--
dwmw2

David Woodhouse

2013-10-25 12:03:27 UTC

Permalink

Post by Huang Shijie
int common_nfc_set_geometry(struct gpmi_nand_data *this)
{
- return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
+ return legacy_set_geometry(this) ? set_geometry_by_ecc_info(this) : 0;

So... what if someone has already shipped the new chips that require
stronger ECC, without realising that legacy_set_geometry() is
insufficient? (And is legacy_set_geometry *actually* doing precisely the
same as 3.10/3.11?)

Do we forcibly upgrade them to the new method, and compatibility be
damned?

I'm inclined to suggest that for the 3.12 release we just need to use
legacy_set_geometry(), and allow it to work with a *warning*, and then
for 3.13 we can finish thrashing out the precise behaviour we need â
which may indeed end up being that you do the new method *only* if the
corresponding property exists in the device tree.

--
David Woodhouse Open Source Technology Centre
***@intel.com Intel Corporation

David Woodhouse

2013-10-25 12:20:54 UTC

Permalink

Post by David Woodhouse
So... what if someone has already shipped the new chips that require
stronger ECC, without realising that legacy_set_geometry() is
insufficient? (And is legacy_set_geometry *actually* doing precisely the
same as 3.10/3.11?)

Answering my own question: If the required ECC strength is known and the
legacy ECC layout is insufficient, that's caused a failure since commit
92d0e09abeebd ("mtd: gpmi: add sanity check for the ECC") in 3.9, so I'm
not worried about supporting that.

And legacy_set_geometry() *is* doing what 3.11 did, verbatim.

So the question is whether we want this "if legacy is sufficient then
use it else use the new method" that you offer in v2 of the patch, or if
a device-tree property is the better way to do it.

I'm actually slightly in favour of the device-tree property. But since
3.12 is imminent I think the *best* option is just to do this to
preserve the 3.11 behaviour, and worry about getting it right for 3.13:

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 59ab069..a9830ff 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -349,7 +349,7 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)

int common_nfc_set_geometry(struct gpmi_nand_data *this)
{
- return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
+ return legacy_set_geometry(this);
}

struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)

--
dwmw2

Marek

2013-10-25 13:22:12 UTC

Permalink

Post by David Woodhouse

Answering my own question: If the required ECC strength is known and the
legacy ECC layout is insufficient, that's caused a failure since commit
92d0e09abeebd ("mtd: gpmi: add sanity check for the ECC") in 3.9, so I'm
not worried about supporting that.
And legacy_set_geometry() *is* doing what 3.11 did, verbatim.
So the question is whether we want this "if legacy is sufficient then
use it else use the new method" that you offer in v2 of the patch, or if
a device-tree property is the better way to do it.
I'm actually slightly in favour of the device-tree property. But since
3.12 is imminent I think the *best* option is just to do this to
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 59ab069..a9830ff 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -349,7 +349,7 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
int common_nfc_set_geometry(struct gpmi_nand_data *this)
{
- return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
+ return legacy_set_geometry(this);
}
struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)

I can confirm this patch fixes the regression, UBIFS can now again be mounted on
3.12rc6 . Thanks!

Acked-by: Marek Vasut <***@denx.de>

On M28EVK with NAND device:
NAND device: Manufacturer ID: 0x2c, Chip ID: 0xda (Micron MT29F2G08ABAEAWP)
NAND device: 256MiB, SLC, page size: 2048, OOB size: 64

Tested-by: Marek Vasut <***@denx.de>

Cheers!

Huang Shijie

2013-10-26 01:33:00 UTC

Permalink

Post by David Woodhouse

Hi David:
I am ok with your patch. but we will meet a compiler warning, since
the set_geometry_by_ecc_info() is not referenced.

thanks
Huang Shijie

Post by David Woodhouse
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 59ab069..a9830ff 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -349,7 +349,7 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
int common_nfc_set_geometry(struct gpmi_nand_data *this)
{
- return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this);
+ return legacy_set_geometry(this);
}
struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
--
dwmw2
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

David Woodhouse

2013-10-25 13:29:42 UTC

Permalink

Post by Huang Shijie
I am ok with your patch. but we will meet a compiler warning, since
the set_geometry_by_ecc_info() is not referenced.

Yeah, I predicted that but at this stage with *hours* to catch Linux and
get it into 3.12, I think that's the better option. I'll note it in the
commit comment.

Thanks for the prompt feedback.

--
dwmw2

Huang Shijie

2013-10-26 01:41:17 UTC

Permalink

Post by David Woodhouse

Post by Huang Shijie
I am ok with your patch. but we will meet a compiler warning, since
the set_geometry_by_ecc_info() is not referenced.

Yeah, I predicted that but at this stage with *hours* to catch Linux and
get it into 3.12, I think that's the better option. I'll note it in the
commit comment.

ok. Please add my ack for your formal patch:
Acked-by: Huang Shijie <***@gmail.com>

David Woodhouse

2013-10-25 14:08:12 UTC

Permalink

Post by David Woodhouse

Post by Huang Shijie
I am ok with your patch. but we will meet a compiler warning, since
the set_geometry_by_ecc_info() is not referenced.

Yeah, I predicted that but at this stage with *hours* to catch Linux and
get it into 3.12, I think that's the better option. I'll note it in the
commit comment.

I've just pushed this to the tip of the tree, replacing the version that
Brian had put there.

Brian, are you still happy to send the pull request to Linus or do you
want me to?

--
dwmw2

Brian Norris

2013-10-25 17:08:06 UTC

Permalink

Post by David Woodhouse

Post by Huang Shijie
I am ok with your patch. but we will meet a compiler warning, since
the set_geometry_by_ecc_info() is not referenced.

Yeah, I predicted that but at this stage with *hours* to catch Linux and
get it into 3.12, I think that's the better option. I'll note it in the
commit comment.

I've just pushed this to the tip of the tree, replacing the version that
Brian had put there.

A tiny comment on the description. You say:

"The "legacy" ECC layout used until 3.11 uses all the OOB area by
computing the ECC strength and ECC step size ourselves."

This phrase "until 3.11" sounds like the layout changed in 3.11 (which
it did not). The rest of the commit message might clear that up, but
still...

Post by David Woodhouse
Brian, are you still happy to send the pull request to Linus or do you
want me to?

I'm happy either way, as long as the fixes go in. I'll send it, since
I still have the day ahead of me in my timezone.

Thanks,
Brian

Marek

2013-10-25 13:38:37 UTC

Permalink

Post by David Woodhouse

Post by Huang Shijie
I am ok with your patch. but we will meet a compiler warning, since
the set_geometry_by_ecc_info() is not referenced.

Yeah, I predicted that but at this stage with *hours* to catch Linux and
get it into 3.12, I think that's the better option. I'll note it in the
commit comment.
Thanks for the prompt feedback.

Might __maybe_unused help here ?