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

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

[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
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 <b32955@freescale.com>
Cc: stable@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

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

[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
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 <b32955@freescale.com>
Cc: stable@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

Re: [PATCH v2 fix] mtd: gpmi: fix the ecc regression

[Brian Norris]

On Thu, Oct 24, 2013 at 04:48:16PM +0800, Huang Shijie wrote:
> 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 <b32955@freescale.com>
> Cc: stable@vger.kernel.org

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

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

On Thu, 2013-10-24 at 16:14 +0800, Huang Shijie wrote:
>  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
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

On Fri, 2013-10-25 at 13:03 +0100, David Woodhouse wrote:
> 
> 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


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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

[Marek]

On Friday 25 of October 2013 13:20:54 David Woodhouse wrote:
> On Fri, 2013-10-25 at 13:03 +0100, David Woodhouse wrote:
> > 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)

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

Acked-by: Marek Vasut <marex@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 <marex@denx.de>

Cheers!

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote:
> 
>   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


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

Re: [PATCH v2 fix] mtd: gpmi: fix the ecc regression

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

On Thu, 2013-10-24 at 15:19 -0700, Brian Norris wrote:
> 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


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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

[Marek]

On Friday 25 of October 2013 14:29:42 David Woodhouse wrote:
> On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote:
> >   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 ?

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On Fri, 2013-10-25 at 21:41 -0400, Huang Shijie wrote:
> On Fri, Oct 25, 2013 at 02:29:42PM +0100, David Woodhouse wrote:
> > On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote:
> > > 
> > >   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 <shijie8@gmail.com>

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


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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

[Brian Norris]

On Fri, Oct 25, 2013 at 7:08 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Fri, 2013-10-25 at 21:41 -0400, Huang Shijie wrote:
>> On Fri, Oct 25, 2013 at 02:29:42PM +0100, David Woodhouse wrote:
>> > On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote:
>> > >
>> > >   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 <shijie8@gmail.com>
>
> 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...

> 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

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

[Huang Shijie]

On Fri, Oct 25, 2013 at 01:20:54PM +0100, David Woodhouse wrote:
> On Fri, 2013-10-25 at 13:03 +0100, David Woodhouse wrote:
> > 
> > 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:

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

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

[Huang Shijie]

On Fri, Oct 25, 2013 at 02:29:42PM +0100, David Woodhouse wrote:
> On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote:
> > 
> >   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 <shijie8@gmail.com>