Patchwork [12/13] mtd/docg3: add ECC correction code

login
register
mail settings
Submitter Robert Jarzmik
Date Oct. 28, 2011, 5:51 p.m.
Message ID <1319824292-11085-13-git-send-email-robert.jarzmik@free.fr>
Download mbox | patch
Permalink /patch/122464/
State New
Headers show

Comments

Robert Jarzmik - Oct. 28, 2011, 5:51 p.m.
Credit for discovering the BCH algorith parameters, and bit
reversing algorithm is to be give to Mike Dunn and Ivan
Djelic.

The BCH correction code relied upon the BCH library, where
all data and ECC is bit-reversed. The BCH library works
correctly when each input byte is bit-reversed, and
accordingly ECC output is also bit-reversed.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/devices/Kconfig |    1 +
 drivers/mtd/devices/docg3.c |  118 ++++++++++++++++++++++++++++++++-----------
 drivers/mtd/devices/docg3.h |   11 ++++-
 3 files changed, 99 insertions(+), 31 deletions(-)
Ivan Djelic - Oct. 29, 2011, 8:52 a.m.
On Fri, Oct 28, 2011 at 06:51:31PM +0100, Robert Jarzmik wrote:
>  
> +/**
> + * struct docg3_bch - BCH engine
> + */
> +static struct bch_control *docg3_bch;

Why not putting this into your struct docg3, instead of adding a global var ?

> +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *calc_ecc)
> +{
> +	u8 ecc[DOC_ECC_BCH_T + 1];

Should be 'u8 ecc[DOC_ECC_BCH_SIZE];'

> +	int errorpos[DOC_ECC_BCH_T + 1], i, numerrs;

Using 'errorpos[DOC_ECC_BCH_T]' is fine, no need for '+ 1'.

(...)

> +
> +	for (i = 0; i < DOC_ECC_BCH_SIZE; i++)
> +		ecc[i] = bitrev8(calc_ecc[i]);
> +	numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES,
> +			     NULL, ecc, NULL, errorpos);
> +	if (numerrs < 0)
> +		return numerrs;

Maybe do something like 'printk(KERN_ERR "ecc unrecoverable error\n");' when
numerrs < 0 ?

(...)

> +	for (i = 0; i < numerrs; i++)
> +		change_bit(errorpos[i], buf);

'buf' holds the buffer of read data (512 + 7 + 1 bytes); hence you should
change the above code into something like:

	for (i = 0; i < numerrs; i++)
		if (errorpos[i] < DOC_ECC_BCH_COVERED_BYTES*8)
			/* error is located in data, correct it */
			change_bit(errorpos[i], buf);
	        /* else error in ecc bytes, no data change needed */

otherwise 'change_bit' will be out of bounds when a bitflip occurs in ecc
bytes. Note that we still need to report bitflips in that case, to let upper
layers scrub them.

(...)
> +	docg3_bch = init_bch(DOC_ECC_BCH_M, DOC_ECC_BCH_T,
> +			     DOC_ECC_BCH_PRIMPOLY);
> +	if (!docg3_bch)
> +		goto nomem2;

Just a side note: if you need to get maximum performance from the BCH library,
you can set fixed values for M and T in your Kconfig, with something like:

 config MTD_DOCG3
        tristate "M-Systems Disk-On-Chip G3"
	select BCH
        ---help---
          This provides an MTD device driver for the M-Systems DiskOnChip
          G3 devices.

if MTD_DOCG3
config BCH_CONST_M
	default 14
config BCH_CONST_T
	default 4
endif


The only drawback is that it won't work if you select your DOCG3 driver and, at
the same time, other MTD drivers that also use fixed, but different parameters.

(...)
> @@ -1660,6 +1717,7 @@ static int __exit docg3_release(struct platform_device *pdev)
>  			doc_release_device(docg3_floors[floor]);
>  
>  	kfree(docg3_floors);
> +	kfree(docg3_bch);

This should be 'free_bch(docg3_bch)'.

Otherwise it looks OK to me; did you test BCH correction by simulating
corruptions (of 1-4 bits, and also 5 bits to trigger failures) in nand data ?

Best Regards,
--
Ivan
Ivan Djelic - Oct. 29, 2011, 9:09 a.m.
On Sat, Oct 29, 2011 at 10:52:48AM +0200, Ivan Djelic wrote:
> Just a side note: if you need to get maximum performance from the BCH library,
> you can set fixed values for M and T in your Kconfig, with something like:
> 
>  config MTD_DOCG3
>         tristate "M-Systems Disk-On-Chip G3"
> 	select BCH

Oops, just noticed that you would also need to add this line here:
	select BCH_CONST_PARAMS

>         ---help---
>           This provides an MTD device driver for the M-Systems DiskOnChip
>           G3 devices.
> 
> if MTD_DOCG3
> config BCH_CONST_M
> 	default 14
> config BCH_CONST_T
> 	default 4
> endif
> 
> 
> The only drawback is that it won't work if you select your DOCG3 driver and, at
> the same time, other MTD drivers that also use fixed, but different parameters.
Robert Jarzmik - Oct. 29, 2011, 4:37 p.m.
Ivan Djelic <ivan.djelic@parrot.com> writes:

> On Fri, Oct 28, 2011 at 06:51:31PM +0100, Robert Jarzmik wrote:
>>  
>> +/**
>> + * struct docg3_bch - BCH engine
>> + */
>> +static struct bch_control *docg3_bch;
>
> Why not putting this into your struct docg3, instead of adding a global var ?
Because I have multiple floors (ie. 4 floors for example), which are split into
4 different devices. If I put that in docg3 structures (ie. the 4 allocated
structures, each for one floor), I'd either have to :
 - allocate 4 different bch "engines"
 - or count docg3 releases and release the bch at the last kfree(docg3), which
 makes me have another global variable.


>
>> +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *calc_ecc)
>> +{
>> +	u8 ecc[DOC_ECC_BCH_T + 1];
>
> Should be 'u8 ecc[DOC_ECC_BCH_SIZE];'
OK, I'll amend it.

>
>> +	int errorpos[DOC_ECC_BCH_T + 1], i, numerrs;
>
> Using 'errorpos[DOC_ECC_BCH_T]' is fine, no need for '+ 1'.
OK, got it.

>> +
>> +	for (i = 0; i < DOC_ECC_BCH_SIZE; i++)
>> +		ecc[i] = bitrev8(calc_ecc[i]);
>> +	numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES,
>> +			     NULL, ecc, NULL, errorpos);
>> +	if (numerrs < 0)
>> +		return numerrs;
>
> Maybe do something like 'printk(KERN_ERR "ecc unrecoverable error\n");' when
> numerrs < 0 ?
That will be too verbose. As there are special partitions where the ECC is not
used that way (ie. SAFTL partitions I don't understand well yet), the ECC is
always wrong there.
Moreover, the error is reported as EBADMSG later (in doc_read), making the
information available to userland.

>
> (...)
>
>> +	for (i = 0; i < numerrs; i++)
>> +		change_bit(errorpos[i], buf);
>
> 'buf' holds the buffer of read data (512 + 7 + 1 bytes); hence you should
> change the above code into something like:
>
> 	for (i = 0; i < numerrs; i++)
> 		if (errorpos[i] < DOC_ECC_BCH_COVERED_BYTES*8)
> 			/* error is located in data, correct it */
> 			change_bit(errorpos[i], buf);
> 	        /* else error in ecc bytes, no data change needed */
>
> otherwise 'change_bit' will be out of bounds when a bitflip occurs in ecc
> bytes. Note that we still need to report bitflips in that case, to let upper
> layers scrub them.
Ah OK, I wasn't aware of that. I'll amend the code, thanks.

>
> (...)
>> +	docg3_bch = init_bch(DOC_ECC_BCH_M, DOC_ECC_BCH_T,
>> +			     DOC_ECC_BCH_PRIMPOLY);
>> +	if (!docg3_bch)
>> +		goto nomem2;
>
> Just a side note: if you need to get maximum performance from the BCH library,
> you can set fixed values for M and T in your Kconfig, with something like:
>
>  config MTD_DOCG3
>         tristate "M-Systems Disk-On-Chip G3"
> 	select BCH
>         ---help---
>           This provides an MTD device driver for the M-Systems DiskOnChip
>           G3 devices.
>
> if MTD_DOCG3
> config BCH_CONST_M
> 	default 14
> config BCH_CONST_T
> 	default 4
> endif
>
>
> The only drawback is that it won't work if you select your DOCG3 driver and, at
> the same time, other MTD drivers that also use fixed, but different
> parameters.
Oh, that seems nice. As I'm working with a smartphone, there is only one mtd
inside, so the speed-up will be nice.

>
> (...)
>> @@ -1660,6 +1717,7 @@ static int __exit docg3_release(struct platform_device *pdev)
>>  			doc_release_device(docg3_floors[floor]);
>>  
>>  	kfree(docg3_floors);
>> +	kfree(docg3_bch);
>
> This should be 'free_bch(docg3_bch)'.
Right.

>
> Otherwise it looks OK to me; did you test BCH correction by simulating
> corruptions (of 1-4 bits, and also 5 bits to trigger failures) in nand data ?
I did test the algorithm, yes.
To be more precise, I tested your last BCH encoding algorithm (emulate_docg4_hw)
which gives exactly the same ECC.

I then flipped 2 bits (buf[0] ^= 0x01 and buf[1] ^= 0x02), and got correct
errorpos (ie. 0 and 10 IIRC).

The thing I didn't check is the decode_bch() call with the hardware calculated
ECC. I tried on my PC:
  decode_bch(bch, data, 520, ecc, NULL, NULL, errorpos);
  => this *does* work
while in the kernel I did:
  decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES,
			     NULL, ecc, NULL, errorpos);
  => this might work...

What I'm a bit afraid of is my poor understanding of the hardware ECC engine. I
know that the write part is correct (ie. ECC calculation), but I'm a bit
confused by the read part.

What wories me is that the hardware ECC got back while reading (ie. what I
called calc_ecc) is always 00:00:00:00:00:00:00 when I read data (because I
don't have bitflips on my flash). This looks to me more a "syndrom" than a
"calc_ecc".

To be sure, I could write a page of 512 bytes + 16 bytes, where the BCH would be
forced (and incorrect), to check what the hardware generator gives me back. I'd
like you to help me, ie:
 - tell me what to write to the first 512 bytes (only 0, all 0 but one byte to
 1, other ...)
 - I think I'll write 8 bytes to 0x01 for the first 8 OOB bytes (Hamming false
 but I won't care)
 - tell me what to write to the 7 BCH ECC

That way, I could provide you the result and you could tell me if
doc_ecc_bch_fix_data() is correct or not. Would you agree to spend some time on
that ?

Cheers.

--
Robert
Ivan Djelic - Oct. 30, 2011, 12:10 a.m.
On Sat, Oct 29, 2011 at 05:37:35PM +0100, Robert Jarzmik wrote:
> >> +static struct bch_control *docg3_bch;
> >
> > Why not putting this into your struct docg3, instead of adding a global var ?
> Because I have multiple floors (ie. 4 floors for example), which are split into
> 4 different devices. If I put that in docg3 structures (ie. the 4 allocated
> structures, each for one floor), I'd either have to :
>  - allocate 4 different bch "engines"
>  - or count docg3 releases and release the bch at the last kfree(docg3), which
>  makes me have another global variable.

OK, got it; using a struct to hold all your common vars (docg3_floors,
docg3_bch, ...) and hook that to your platform data instead of docg3_floors
would still be a bit cleaner I think, but no big deal.

> What I'm a bit afraid of is my poor understanding of the hardware ECC engine. I
> know that the write part is correct (ie. ECC calculation), but I'm a bit
> confused by the read part.
> 
> What wories me is that the hardware ECC got back while reading (ie. what I
> called calc_ecc) is always 00:00:00:00:00:00:00 when I read data (because I
> don't have bitflips on my flash). This looks to me more a "syndrom" than a
> "calc_ecc".

OK, I'll try to clarify that. The hardware ECC engine divides a huge polynomial
(520*8 = 4160 bits) by a generator polynomial and computes a 56-bit remainder.
So this remainder (let's call it R) depends only on 520 input data bytes.

- during a write operation: input data is what you write to the controller,
you get R from the ecc engine and this is what you write to oob[8..14].

- during a read operation: the ecc engine computes R on 520 input bytes read
from flash (this is calc_ecc), and also reads oob[8..14] (this is recv_ecc,
previously programmed during the write operation).
Then the ecc engine computes calc_ecc^recv_ecc, and this is what you get from
the ecc registers. And as long as there is no bitflip, its all 00s (because
calc_ecc=recv_ecc).

> To be sure, I could write a page of 512 bytes + 16 bytes, where the BCH would be
> forced (and incorrect), to check what the hardware generator gives me back. I'd
> like you to help me, ie:
>  - tell me what to write to the first 512 bytes (only 0, all 0 but one byte to
>  1, other ...)
>  - I think I'll write 8 bytes to 0x01 for the first 8 OOB bytes (Hamming false
>  but I won't care)
>  - tell me what to write to the 7 BCH ECC

OK, this is really simple:

1. Prepare a buffer of 520 bytes of data, containing pseudo-random bytes or
any pattern you like. Let's call this buffer 'ref_buf'.

2. Program 'ref_buf' to a nand page; you will write ecc bytes to oob during
that operation; let's call those ecc bytes 'ref_ecc'.

3. Now, you are ready to perform corruption tests:

 3.1 Make a copy of 'ref_buf' in which you flip 1, 2, 3 or 4 bits selected
     at random.

 3.2 Program this corrupt buffer, _but_ write 'ref_ecc' to oob instead of hw
     generated ecc bytes.

 3.3 Read page back: you should get exactly 'ref_buf', and the errorpos[]
     array of corrected bits should match your flip bits.

After step 3.2, your flash is exactly in the same state as if it had produced
the bitflips itself.

Repeat steps 3.1 to 3.3 on a large enough set of random vectors to convince
yourself that your code works (be careful not to wear out your device,
though :-). You should also try a few 5-bit corruptions and see failures, just
to verify that your corruptions have some effect.

In theory, testing the BCH algorithm like you did should be enough, but real
hardware tests are helpful to verify that the entire system behaves as
expected.

Hope that helps,
BR,
--
Ivan
Mike Dunn - Oct. 31, 2011, 1:16 p.m.
Hi Robert,

A few comments (limited to the bch decode for now)...

On 10/28/2011 10:51 AM, Robert Jarzmik wrote:
>  
>  /**
> + * doc_correct_data - Fix if need be read data from flash
> + * @docg3: the device
> + * @buf: the buffer of read data (512 + 7 + 1 bytes)
> + * @calc_ecc: the hardware calculated ECC


To avoid confusion with the terminology in Ivan's BCH algorithm, I wouldn't call
it calc_ecc.  It's actually recv_ecc ^ calc_ecc, where recv_ecc is what the hw
read from the ecc field of the oob data, and calc_ecc is what the hw calculated
from the page data.  Maybe just hwecc or similiar.


> + *
> + * Checks if the received data matches the ECC, and if an error is detected,
> + * tries to fix the bit flips (at most 4) in the buffer buf.  As the docg3
> + * understands the (data, ecc, syndroms) in an inverted order in comparison to
> + * the BCH library, the function reverses the order of bits (ie. bit7 and bit0,
> + * bit6 and bit 1, ...) for all ECC data.
> + *
> + * Returns number of fixed bits (0, 1, 2, 3, 4) or -EBADMSG if too many bit
> + * errors were detected and cannot be fixed.
> + */
> +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *calc_ecc)
> +{
> +	u8 ecc[DOC_ECC_BCH_T + 1];
> +	int errorpos[DOC_ECC_BCH_T + 1], i, numerrs;
> +
> +	for (i = 0; i < DOC_ECC_BCH_SIZE; i++)
> +		ecc[i] = bitrev8(calc_ecc[i]);
> +	numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES,
> +			     NULL, ecc, NULL, errorpos);


This looks right, with the redefined DOC_ECC_BCH_COVERED_BYTES (now 520).   

Some commentary would be helpful (though maybe I'm too verbose)...

    /*
     * The hardware ecc unit produces oob_ecc ^ calc_ecc.  The kernel's bch
     * algorithm is used to decode this.  However the hw operates on page
     * data in a bit order that is the reverse of that of the bch alg,
     * requiring that the bits be reversed on the result.  Thanks to Ivan
     * Djelic for his analysis.
     */

I recommend you test this.  One way would be to compile the bch algorithm in
userspace and use it to generate the ecc for a 520  byte test vector.  Reverse
the bits of this ecc, then flip a few bits in the test vector and write it to a
page in flash, with your driver modified to write the calculated ecc instead of
that served up by the hardware.  Then when you read the page, the above code
should identify and correct the bits you flipped (assuming a genuine flash error
did not occur while reading back the page).  I have the bch alg modified for
userspace, if that would help.

Alternatively, you could just fill the flash with a fixed pattern, then read all
the pages, waiting for an error to occur so that correct correction (ha) can be
verified.


> +	if (numerrs < 0)
> +		return numerrs;

I recommend you check for the -EINVAL return value and issue a big fat error. 
Maybe BUG_ON(numerrs == -EINVAL), at least for now.

Another explanatory comment here...
    /* undo last step in BCH alg (modulo mirroring not needed) */


> +
> +	for (i = 0; i < numerrs; i++)
> +		errorpos[i] = (errorpos[i] & ~7) | (7 - (errorpos[i] & 7));
> +	for (i = 0; i < numerrs; i++)
> +		change_bit(errorpos[i], buf);
> +
> +	doc_dbg("doc_ecc_bch_fix_data: flipped %d bits\n", numerrs);
> +	return numerrs;
> +}
> +
> +

Thanks,
Mike
Robert Jarzmik - Oct. 31, 2011, 4:39 p.m.
Mike Dunn <mikedunn@newsguy.com> writes:

> Hi Robert,
>
> A few comments (limited to the bch decode for now)...
> To avoid confusion with the terminology in Ivan's BCH algorithm, I wouldn't call
> it calc_ecc.  It's actually recv_ecc ^ calc_ecc, where recv_ecc is what the hw
> read from the ecc field of the oob data, and calc_ecc is what the hw calculated
> from the page data.  Maybe just hwecc or similiar.
Yep, point taken for V2, hwecc sounds good.

...zip...
>> +	numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES,
>> +			     NULL, ecc, NULL, errorpos);
> This looks right, with the redefined DOC_ECC_BCH_COVERED_BYTES (now 520).   
>
> Some commentary would be helpful (though maybe I'm too verbose)...
>
>     /*
>      * The hardware ecc unit produces oob_ecc ^ calc_ecc.  The kernel's bch
>      * algorithm is used to decode this.  However the hw operates on page
>      * data in a bit order that is the reverse of that of the bch alg,
>      * requiring that the bits be reversed on the result.  Thanks to Ivan
>      * Djelic for his analysis.
>      */
Right again. I will copy paste that explanation in the code.

> I recommend you test this.  One way would be to compile the bch algorithm in
> userspace and use it to generate the ecc for a 520  byte test vector.  Reverse
> the bits of this ecc, then flip a few bits in the test vector and write it to a
> page in flash, with your driver modified to write the calculated ecc instead of
> that served up by the hardware.  Then when you read the page, the above code
> should identify and correct the bits you flipped (assuming a genuine flash error
> did not occur while reading back the page).  I have the bch alg modified for
> userspace, if that would help.
I did test it in userspace.
And after Ivan's proposition, I tested it also with nandwrite/nanddump by
introducing some random errors. The error correction code works, a few
amendments (for V2) will be added for "bitflipped" data writes/reads.

But the conclusion is that Ivan and your work is indeed directly applicable in
the G3 case, and tested :)

>> +	if (numerrs < 0)
>> +		return numerrs;
>
> I recommend you check for the -EINVAL return value and issue a big fat error. 
> Maybe BUG_ON(numerrs == -EINVAL), at least for now.
Okay.

>
> Another explanatory comment here...
>     /* undo last step in BCH alg (modulo mirroring not needed) */
Is that the same as the function comment about bit reversing (the modulo
mirroring part) ? If so, the function comment might not be clear enough. If not,
could you explain a bit further please ?

Thanks for the review.

Cheers.

--
Robert
Mike Dunn - Oct. 31, 2011, 5:32 p.m.
On 10/31/2011 09:39 AM, Robert Jarzmik wrote:
> Mike Dunn <mikedunn@newsguy.com> writes:
>
>> Another explanatory comment here...
>>     /* undo last step in BCH alg (modulo mirroring not needed) */
> Is that the same as the function comment about bit reversing (the modulo
> mirroring part) ? If so, the function comment might not be clear enough. If not,
> could you explain a bit further please ?
>
>


No, the "modulo mirroring" is not the same as the bit reversal.  See Ivan's
explanation here:

http://lists.infradead.org/pipermail/linux-mtd/2011-October/038060.html

Obviously the comment doesn't give much technical detail, but it points anyone
puzzled by the step in the right direction.  I don't like cryptic steps that
give no explanation whatsoever.  Just MHO.

Thanks,
Mike

Patch

diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index 6d91a1f..ae138c3 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -251,6 +251,7 @@  config MTD_DOC2001PLUS
 
 config MTD_DOCG3
 	tristate "M-Systems Disk-On-Chip G3"
+	select BCH
 	---help---
 	  This provides an MTD device driver for the M-Systems DiskOnChip
 	  G3 devices.
diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 9983594..3c4f7ed 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -29,6 +29,9 @@ 
 #include <linux/delay.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
+#include <linux/bitmap.h>
+#include <linux/bitrev.h>
+#include <linux/bch.h>
 
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
@@ -42,7 +45,6 @@ 
  * As no specification is available from M-Systems/Sandisk, this drivers lacks
  * several functions available on the chip, as :
  *  - IPL write
- *  - ECC fixing (lack of BCH algorith understanding)
  *  - powerdown / powerup
  *
  * The bus data width (8bits versus 16bits) is not handled (if_cfg flag), and
@@ -51,8 +53,7 @@ 
  * DocG3 relies on 2 ECC algorithms, which are handled in hardware :
  *  - a 1 byte Hamming code stored in the OOB for each page
  *  - a 7 bytes BCH code stored in the OOB for each page
- * The BCH part is only used for check purpose, no correction is available as
- * some information is missing. What is known is that :
+ * The BCH ECC is :
  *  - BCH is in GF(2^14)
  *  - BCH is over data of 520 bytes (512 page + 7 page_info bytes
  *                                   + 1 hamming byte)
@@ -73,6 +74,11 @@  static struct nand_ecclayout docg3_oobinfo = {
 	.oobfree = {{0, 7}, {15, 1} }
 };
 
+/**
+ * struct docg3_bch - BCH engine
+ */
+static struct bch_control *docg3_bch;
+
 static inline u8 doc_readb(struct docg3 *docg3, u16 reg)
 {
 	u8 val = readb(docg3->base + reg);
@@ -576,6 +582,43 @@  static void doc_hamming_ecc_init(struct docg3 *docg3, int nb_bytes)
 }
 
 /**
+ * doc_correct_data - Fix if need be read data from flash
+ * @docg3: the device
+ * @buf: the buffer of read data (512 + 7 + 1 bytes)
+ * @calc_ecc: the hardware calculated ECC
+ *
+ * Checks if the received data matches the ECC, and if an error is detected,
+ * tries to fix the bit flips (at most 4) in the buffer buf.  As the docg3
+ * understands the (data, ecc, syndroms) in an inverted order in comparison to
+ * the BCH library, the function reverses the order of bits (ie. bit7 and bit0,
+ * bit6 and bit 1, ...) for all ECC data.
+ *
+ * Returns number of fixed bits (0, 1, 2, 3, 4) or -EBADMSG if too many bit
+ * errors were detected and cannot be fixed.
+ */
+static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *calc_ecc)
+{
+	u8 ecc[DOC_ECC_BCH_T + 1];
+	int errorpos[DOC_ECC_BCH_T + 1], i, numerrs;
+
+	for (i = 0; i < DOC_ECC_BCH_SIZE; i++)
+		ecc[i] = bitrev8(calc_ecc[i]);
+	numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES,
+			     NULL, ecc, NULL, errorpos);
+	if (numerrs < 0)
+		return numerrs;
+
+	for (i = 0; i < numerrs; i++)
+		errorpos[i] = (errorpos[i] & ~7) | (7 - (errorpos[i] & 7));
+	for (i = 0; i < numerrs; i++)
+		change_bit(errorpos[i], buf);
+
+	doc_dbg("doc_ecc_bch_fix_data: flipped %d bits\n", numerrs);
+	return numerrs;
+}
+
+
+/**
  * doc_read_page_prepare - Prepares reading data from a flash page
  * @docg3: the device
  * @block0: the first plane block index on flash memory
@@ -756,7 +799,7 @@  static int doc_read(struct mtd_info *mtd, loff_t from, size_t len,
 {
 	struct docg3 *docg3 = mtd->priv;
 	int block0, block1, page, readlen, ret, ofs = 0;
-	u8 syn[DOC_ECC_BCH_SIZE], eccconf1;
+	u8 calc_ecc[DOC_ECC_BCH_SIZE], eccconf1;
 	u8 oob[DOC_LAYOUT_OOB_SIZE];
 
 	ret = -EINVAL;
@@ -777,7 +820,7 @@  static int doc_read(struct mtd_info *mtd, loff_t from, size_t len,
 		ret = doc_read_page_prepare(docg3, block0, block1, page, ofs);
 		if (ret < 0)
 			goto err;
-		ret = doc_read_page_ecc_init(docg3, DOC_ECC_BCH_COVERED_BYTES);
+		ret = doc_read_page_ecc_init(docg3, DOC_ECC_BCH_TOTAL_BYTES);
 		if (ret < 0)
 			goto err_in_read;
 		ret = doc_read_page_getbytes(docg3, readlen, buf, 1);
@@ -788,24 +831,11 @@  static int doc_read(struct mtd_info *mtd, loff_t from, size_t len,
 		if (ret < DOC_LAYOUT_OOB_SIZE)
 			goto err_in_read;
 
-		*retlen += readlen;
-		buf += readlen;
-		len -= readlen;
-
-		ofs ^= DOC_LAYOUT_PAGE_OOB_SIZE;
-		if (ofs == 0)
-			page += 2;
-		if (page > DOC_ADDR_PAGE_MASK) {
-			page = 0;
-			block0 += 2;
-			block1 += 2;
-		}
-
 		/*
 		 * There should be a BCH bitstream fixing algorithm here ...
 		 * By now, a page read failure is triggered by BCH error
 		 */
-		doc_get_hw_bch_syndroms(docg3, syn);
+		doc_get_hw_bch_syndroms(docg3, calc_ecc);
 		eccconf1 = doc_register_readb(docg3, DOC_ECCCONF1);
 
 		doc_dbg("OOB - INFO: %02x:%02x:%02x:%02x:%02x:%02x:%02x\n",
@@ -817,20 +847,41 @@  static int doc_read(struct mtd_info *mtd, loff_t from, size_t len,
 			 oob[13], oob[14]);
 		doc_dbg("OOB - UNUSED: %02x\n", oob[15]);
 		doc_dbg("ECC checks: ECCConf1=%x\n", eccconf1);
-		doc_dbg("ECC BCH syndrom: %02x:%02x:%02x:%02x:%02x:%02x:%02x\n",
-			syn[0], syn[1], syn[2], syn[3], syn[4], syn[5], syn[6]);
+		doc_dbg("ECC CALC_ECC: %02x:%02x:%02x:%02x:%02x:%02x:%02x\n",
+			calc_ecc[0], calc_ecc[1], calc_ecc[2], calc_ecc[3],
+			calc_ecc[4], calc_ecc[5], calc_ecc[6]);
 
 		ret = -EBADMSG;
-		if (block0 >= DOC_LAYOUT_BLOCK_FIRST_DATA) {
-			if (eccconf1 & DOC_ECCCONF1_BCH_SYNDROM_ERR)
-				goto err_in_read;
-			if (is_prot_seq_error(docg3))
-				goto err_in_read;
+		if (is_prot_seq_error(docg3))
+			goto err_in_read;
+		ret = 0;
+		if ((block0 >= DOC_LAYOUT_BLOCK_FIRST_DATA) &&
+		    (eccconf1 & DOC_ECCCONF1_BCH_SYNDROM_ERR)) {
+			ret = doc_ecc_bch_fix_data(docg3, buf, calc_ecc);
+			if (ret < 0)
+				mtd->ecc_stats.failed++;
+			if (ret > 0) {
+				mtd->ecc_stats.corrected += ret;
+				ret = 0;
+			}
 		}
+
 		doc_read_page_finish(docg3);
+		*retlen += readlen;
+		buf += readlen;
+		len -= readlen;
+
+		ofs ^= DOC_LAYOUT_PAGE_OOB_SIZE;
+		if (ofs == 0)
+			page += 2;
+		if (page > DOC_ADDR_PAGE_MASK) {
+			page = 0;
+			block0 += 2;
+			block1 += 2;
+		}
 	}
 
-	return 0;
+	return ret;
 err_in_read:
 	doc_read_page_finish(docg3);
 err:
@@ -1162,7 +1213,7 @@  static int doc_write_page(struct docg3 *docg3, loff_t to, const u_char *buf,
 	if (ret)
 		goto err;
 
-	doc_write_page_ecc_init(docg3, DOC_ECC_BCH_COVERED_BYTES);
+	doc_write_page_ecc_init(docg3, DOC_ECC_BCH_TOTAL_BYTES);
 	doc_delay(docg3, 2);
 	doc_write_page_putbytes(docg3, DOC_LAYOUT_PAGE_SIZE, buf);
 
@@ -1602,7 +1653,11 @@  static int __init docg3_probe(struct platform_device *pdev)
 	docg3_floors = kzalloc(sizeof(*docg3_floors) * DOC_MAX_NBFLOORS,
 			       GFP_KERNEL);
 	if (!docg3_floors)
-		goto nomem;
+		goto nomem1;
+	docg3_bch = init_bch(DOC_ECC_BCH_M, DOC_ECC_BCH_T,
+			     DOC_ECC_BCH_PRIMPOLY);
+	if (!docg3_bch)
+		goto nomem2;
 
 	ret = 0;
 	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) {
@@ -1635,7 +1690,9 @@  err_probe:
 	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
 		if (docg3_floors[floor])
 			doc_release_device(docg3_floors[floor]);
-nomem:
+nomem2:
+	kfree(docg3_floors);
+nomem1:
 	iounmap(base);
 noress:
 	return ret;
@@ -1660,6 +1717,7 @@  static int __exit docg3_release(struct platform_device *pdev)
 			doc_release_device(docg3_floors[floor]);
 
 	kfree(docg3_floors);
+	kfree(docg3_bch);
 	iounmap(base);
 	return 0;
 }
diff --git a/drivers/mtd/devices/docg3.h b/drivers/mtd/devices/docg3.h
index 397e461..33db727 100644
--- a/drivers/mtd/devices/docg3.h
+++ b/drivers/mtd/devices/docg3.h
@@ -51,10 +51,19 @@ 
 #define DOC_LAYOUT_WEAR_OFFSET		(DOC_LAYOUT_PAGE_OOB_SIZE * 2)
 #define DOC_LAYOUT_BLOCK_SIZE					\
 	(DOC_LAYOUT_PAGES_PER_BLOCK * DOC_LAYOUT_PAGE_SIZE)
+
+/*
+ * ECC related constants
+ */
+#define DOC_ECC_BCH_M			14
+#define DOC_ECC_BCH_T			4
+#define DOC_ECC_BCH_PRIMPOLY		0x4443
 #define DOC_ECC_BCH_SIZE		7
 #define DOC_ECC_BCH_COVERED_BYTES				\
 	(DOC_LAYOUT_PAGE_SIZE + DOC_LAYOUT_OOB_PAGEINFO_SZ +	\
-	 DOC_LAYOUT_OOB_HAMMING_SZ + DOC_LAYOUT_OOB_BCH_SZ)
+	 DOC_LAYOUT_OOB_HAMMING_SZ)
+#define DOC_ECC_BCH_TOTAL_BYTES					\
+	(DOC_ECC_BCH_COVERED_BYTES + DOC_LAYOUT_OOB_BCH_SZ)
 
 /*
  * Blocks distribution