Messages in this thread Patch in this message | | | Date | Sat, 29 Oct 2011 10:52:48 +0200 | From | Ivan Djelic <> | Subject | Re: [PATCH 12/13] mtd/docg3: add ECC correction code |
| |
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
| |