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

[Ivan Djelic <ivan.djelic@parrot.com>]

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